[PATCH] drm/gma500: Create the primary plane in the driver

[PATCH] drm/gma500: Create the primary plane in the driver

[oushixiong1025]

From: Shixiong Ou <oushixiong@kylinos.cn>

drm_crtc_init() creates the primary plane from a fixed format list
that includes ARGB8888. The display engine programs the primary plane
with DISPPLANE_32BPP_NO_ALPHA, so it does not support per-pixel alpha
and must not advertise alpha formats. Since commit 860e748bddcc
("drm: ensure blend mode supported if pixel format with alpha exposed"),
drm_mode_config_validate() warns about this at probe time.

Replace drm_crtc_init() with a driver-owned primary plane that
advertises only XRGB8888.

Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
---
 drivers/gpu/drm/gma500/psb_intel_display.c | 38 +++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
index 0df75a4a7739..336943cfeb68 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -9,8 +9,11 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 
+#include <drm/drm_fourcc.h>
 #include <drm/drm_modeset_helper.h>
 #include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_plane_helper.h>
 #include <drm/drm_print.h>
 
 #include "framebuffer.h"
@@ -471,11 +474,24 @@ static void psb_intel_cursor_init(struct drm_device *dev,
 	REG_WRITE(base[gma_crtc->pipe], 0);
 }
 
+static const uint32_t gma_primary_formats[] = {
+	/*
+	 * The display engine programs the primary plane with
+	 * DISPPLANE_32BPP_NO_ALPHA, so no formats with alpha.
+	 */
+	DRM_FORMAT_XRGB8888,
+};
+
+static const struct drm_plane_funcs gma_primary_plane_funcs = {
+	DRM_PLANE_NON_ATOMIC_FUNCS,
+};
+
 void psb_intel_crtc_init(struct drm_device *dev, int pipe,
 		     struct psb_intel_mode_device *mode_dev)
 {
 	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
 	struct gma_crtc *gma_crtc;
+	struct drm_plane *primary;
 	int i;
 
 	/* We allocate a extra array of drm_connector pointers
@@ -494,7 +510,27 @@ void psb_intel_crtc_init(struct drm_device *dev, int pipe,
 		return;
 	}
 
-	drm_crtc_init(dev, &gma_crtc->base, &gma_crtc_funcs);
+	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+					      &gma_primary_plane_funcs,
+					      gma_primary_formats,
+					      ARRAY_SIZE(gma_primary_formats),
+					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (IS_ERR(primary)) {
+		dev_err(dev->dev, "Failed to allocate primary plane: %pe\n",
+			primary);
+		kfree(gma_crtc->crtc_state);
+		kfree(gma_crtc);
+		return;
+	}
+
+	if (drm_crtc_init_with_planes(dev, &gma_crtc->base, primary, NULL,
+				      &gma_crtc_funcs, NULL)) {
+		dev_err(dev->dev, "Failed to init CRTC %d\n", pipe);
+		drm_plane_cleanup(primary);
+		kfree(primary);
+		kfree(gma_crtc->crtc_state);
+		return;
+	}
 
 	/* Set the CRTC clock functions from chip specific data */
 	gma_crtc->clock_funcs = dev_priv->ops->clock_funcs;
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH] drm/gma500: Create the primary plane in the driver

[Thomas Zimmermann]

Hi,

thanks for the follow-up patch.

Am 02.09.26 um 12:04 schrieb oushixiong1025@163.com:
> From: Shixiong Ou <oushixiong@kylinos.cn>
>
> drm_crtc_init() creates the primary plane from a fixed format list
> that includes ARGB8888. The display engine programs the primary plane
> with DISPPLANE_32BPP_NO_ALPHA, so it does not support per-pixel alpha
> and must not advertise alpha formats. Since commit 860e748bddcc
> ("drm: ensure blend mode supported if pixel format with alpha exposed"),
> drm_mode_config_validate() warns about this at probe time.
>
> Replace drm_crtc_init() with a driver-owned primary plane that
> advertises only XRGB8888.
>
> Signed-off-by: Shixiong Ou <oushixiong@kylinos.cn>
> ---
>   drivers/gpu/drm/gma500/psb_intel_display.c | 38 +++++++++++++++++++++-
>   1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
> index 0df75a4a7739..336943cfeb68 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_display.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_display.c
> @@ -9,8 +9,11 @@
>   #include <linux/delay.h>
>   #include <linux/i2c.h>
>   
> +#include <drm/drm_fourcc.h>
>   #include <drm/drm_modeset_helper.h>
>   #include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_plane.h>
> +#include <drm/drm_plane_helper.h>
>   #include <drm/drm_print.h>
>   
>   #include "framebuffer.h"
> @@ -471,11 +474,24 @@ static void psb_intel_cursor_init(struct drm_device *dev,
>   	REG_WRITE(base[gma_crtc->pipe], 0);
>   }
>   
> +static const uint32_t gma_primary_formats[] = {
> +	/*
> +	 * The display engine programs the primary plane with
> +	 * DISPPLANE_32BPP_NO_ALPHA, so no formats with alpha.
> +	 */

That comment applies to the complete array, so it should go first.

> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static const struct drm_plane_funcs gma_primary_plane_funcs = {
> +	DRM_PLANE_NON_ATOMIC_FUNCS,

I think you should open-code the initializer macro.  Because the 
non-atomic modesetting is deprecated, open-coding the macro now might 
help if we ever want to convert the gma500 driver to atomic mode setting.


> +};
> +
>   void psb_intel_crtc_init(struct drm_device *dev, int pipe,
>   		     struct psb_intel_mode_device *mode_dev)
>   {
>   	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>   	struct gma_crtc *gma_crtc;
> +	struct drm_plane *primary;
>   	int i;
>   
>   	/* We allocate a extra array of drm_connector pointers
> @@ -494,7 +510,27 @@ void psb_intel_crtc_init(struct drm_device *dev, int pipe,
>   		return;
>   	}
>   
> -	drm_crtc_init(dev, &gma_crtc->base, &gma_crtc_funcs);
> +	primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
> +					      &gma_primary_plane_funcs,
> +					      gma_primary_formats,
> +					      ARRAY_SIZE(gma_primary_formats),
> +					      NULL, DRM_PLANE_TYPE_PRIMARY, NULL);

Please call drmm_universal_plane_alloc() instead. It will handle cleanup 
for you.

Best regards
Thomas

> +	if (IS_ERR(primary)) {
> +		dev_err(dev->dev, "Failed to allocate primary plane: %pe\n",
> +			primary);
> +		kfree(gma_crtc->crtc_state);
> +		kfree(gma_crtc);
> +		return;
> +	}
> +
> +	if (drm_crtc_init_with_planes(dev, &gma_crtc->base, primary, NULL,
> +				      &gma_crtc_funcs, NULL)) {
> +		dev_err(dev->dev, "Failed to init CRTC %d\n", pipe);
> +		drm_plane_cleanup(primary);
> +		kfree(primary);
> +		kfree(gma_crtc->crtc_state);
> +		return;
> +	}
>   
>   	/* Set the CRTC clock functions from chip specific data */
>   	gma_crtc->clock_funcs = dev_priv->ops->clock_funcs;

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)