[RFC] drm/exynos: improve pixel format handling

[RFC] drm/exynos: improve pixel format handling

[Tobias Jakobi]

Hello,

this is an attempt to somewhat improve the handling for supported
pixel formats in the Exynos DRM. Currently DRM planes are always
created with XRGB8888, ARGB8888 and NV12 as supported formats,
even though the formats depend on the 'consumer' (mixer, FIMD,
video processor, etc.).

This series is based on the remaining patches of this series:
http://www.spinics.net/lists/linux-samsung-soc/msg43103.html

With best wishes,
Tobias

[RFC 1/6] drm/exynos: mixer: move pixelformat defines

[Tobias Jakobi]

Move the defines for the pixelformats that the mixer supports out
of mixer_graph_buffer() to the top of the source.
Also add handling of RGB565 and exit if the pixelformat is not
supported.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 3e07f04..9c398d5 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -44,6 +44,11 @@
 #define MIXER_WIN_NR		3
 #define MIXER_DEFAULT_WIN	0
 
+#define MIXER_PIXELFORMAT_RGB565 4
+#define MIXER_PIXELFORMAT_ARGB1555 5
+#define MIXER_PIXELFORMAT_ARGB4444 6
+#define MIXER_PIXELFORMAT_ARGB8888 7
+
 struct mixer_resources {
 	int			irq;
 	void __iomem		*mixer_regs;
@@ -536,31 +541,30 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
 
 	plane = &ctx->planes[win];
 
-	#define RGB565 4
-	#define ARGB1555 5
-	#define ARGB4444 6
-	#define ARGB8888 7
-
 	switch (plane->pixel_format) {
 	case DRM_FORMAT_ARGB4444:
-		fmt = ARGB4444;
+		fmt = MIXER_PIXELFORMAT_ARGB4444;
 		blend = 1;
 		break;
 
 	case DRM_FORMAT_ARGB8888:
-		fmt = ARGB8888;
+		fmt = MIXER_PIXELFORMAT_ARGB8888;
 		blend = 1;
 		break;
 
 	case DRM_FORMAT_XRGB8888:
-		fmt = ARGB8888;
+		fmt = MIXER_PIXELFORMAT_ARGB8888;
 		blend = 0;
 		break;
 
-	default:
-		fmt = ARGB8888;
+	case DRM_FORMAT_RGB565:
+		fmt = MIXER_PIXELFORMAT_RGB565;
 		blend = 0;
 		break;
+
+	default:
+		DRM_DEBUG_KMS("pixelformat unsupported by mixer\n");
+		return;
 	}
 
 	/* check if mixer supports requested scaling setup */
-- 
2.0.5

Re: [RFC 1/6] drm/exynos: mixer: move pixelformat defines

[Gustavo Padovan]

Hi Tobias,

2015-04-15 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:

> Move the defines for the pixelformats that the mixer supports out
> of mixer_graph_buffer() to the top of the source.
> Also add handling of RGB565 and exit if the pixelformat is not
> supported.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 2/6] drm/exynos: remove used 'activated' field from exynos_drm_plane

[Tobias Jakobi]

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 6a849cf..4c14a89 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -77,7 +77,6 @@ extern void exynos4412_qos(u8 tm, u8 ac);
  * @color_key: color key on or off.
  * @local_path: in case of lcd type, local path mode on or off.
  * @transparency: transparency on or off.
- * @activated: activated or not.
  * @enabled: enabled or not.
  * @resume: to resume or not.
  *
@@ -112,7 +111,6 @@ struct exynos_drm_plane {
 	bool color_key:1;
 	bool local_path:1;
 	bool transparency:1;
-	bool activated:1;
 	bool enabled:1;
 	bool resume:1;
 };
-- 
2.0.5

Re: [RFC 2/6] drm/exynos: remove used 'activated' field from exynos_drm_plane

[Gustavo Padovan]

Hi Tobias,

2015-04-15 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:

> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

I think you mean "unused" in the commit subject. And would also be good to
have some commit message as well. Other than that:

Reviewed-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

	Gustavo

Re: [RFC 2/6] drm/exynos: remove used 'activated' field from exynos_drm_plane

[Tobias Jakobi]

Gustavo Padovan wrote:
> Hi Tobias,
> 
> 2015-04-15 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> 
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> 
> I think you mean "unused" in the commit subject. And would also be good to
> have some commit message as well. Other than that:
Right, I'm going to change that and respin the series!

With best wishes,
Tobias

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 3/6] drm/exynos: introduce struct exynos_drm_plane_config

[Tobias Jakobi]

This struct encapsulates the configuration for a plane
object. The pixel format config is currently unused.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c | 17 ++++++++++-------
 drivers/gpu/drm/exynos/exynos_drm_drv.h    | 19 +++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 17 ++++++++++-------
 drivers/gpu/drm/exynos/exynos_drm_plane.c  | 14 +++++++-------
 drivers/gpu/drm/exynos/exynos_drm_plane.h  |  3 +--
 drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 17 ++++++++++-------
 drivers/gpu/drm/exynos/exynos_mixer.c      | 17 ++++++++++-------
 7 files changed, 67 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 84a3638..ca70599 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -756,8 +756,8 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 	struct decon_context *ctx = dev_get_drvdata(dev);
 	struct drm_device *drm_dev = data;
 	struct exynos_drm_plane *exynos_plane;
-	enum drm_plane_type type;
-	unsigned int zpos;
+	struct exynos_drm_plane_config plane_config = { 0 };
+	unsigned int i;
 	int ret;
 
 	ret = decon_ctx_initialize(ctx, drm_dev);
@@ -766,11 +766,14 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
-	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
-		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
-						DRM_PLANE_TYPE_OVERLAY;
-		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
-					1 << ctx->pipe, type, zpos);
+	plane_config.possible_crtcs = 1 << ctx->pipe;
+
+	for (i = 0; i < WINDOWS_NR; i++) {
+		plane_config.type = (i == ctx->default_win) ?
+			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+		plane_config.zpos = i;
+
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 4c14a89..35698f3 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -116,6 +116,25 @@ struct exynos_drm_plane {
 };
 
 /*
+ * Exynos DRM plane configuration structure.
+ *
+ * @possible_crtcs: bitfield describing the valid CRTCs
+ *			for this plane.
+ * @type: plane type (primary, overlay, etc.)
+ * @zpos: z-position of the plane.
+ * @pixel_formats: supported pixel formats.
+ * @num_pixel_formats: number of elements in 'pixel_formats'.
+ */
+
+struct exynos_drm_plane_config {
+	unsigned long possible_crtcs;
+	enum drm_plane_type type;
+	unsigned int zpos;
+	const uint32_t *pixel_formats;
+	unsigned int num_pixel_formats;
+};
+
+/*
  * Exynos DRM Display Structure.
  *	- this structure is common to analog tv, digital tv and lcd panel.
  *
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 589579f..93fbaa5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -997,18 +997,21 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
 	struct drm_device *drm_dev = data;
 	struct exynos_drm_private *priv = drm_dev->dev_private;
 	struct exynos_drm_plane *exynos_plane;
-	enum drm_plane_type type;
-	unsigned int zpos;
+	struct exynos_drm_plane_config plane_config = { 0 };
+	unsigned int i;
 	int ret;
 
 	ctx->drm_dev = drm_dev;
 	ctx->pipe = priv->pipe++;
 
-	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
-		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
-						DRM_PLANE_TYPE_OVERLAY;
-		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
-					1 << ctx->pipe, type, zpos);
+	plane_config.possible_crtcs = 1 << ctx->pipe;
+
+	for (i = 0; i < WINDOWS_NR; i++) {
+		plane_config.type = (i == ctx->default_win) ?
+			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+		plane_config.zpos = i;
+
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 043a6ba..d24b32a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -206,23 +206,23 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
 
 int exynos_plane_init(struct drm_device *dev,
 		      struct exynos_drm_plane *exynos_plane,
-		      unsigned long possible_crtcs, enum drm_plane_type type,
-		      unsigned int zpos)
+		      const struct exynos_drm_plane_config *config)
 {
 	int err;
 
-	err = drm_universal_plane_init(dev, &exynos_plane->base, possible_crtcs,
+	err = drm_universal_plane_init(dev, &exynos_plane->base,
+					   config->possible_crtcs,
 				       &exynos_plane_funcs, formats,
-				       ARRAY_SIZE(formats), type);
+				       ARRAY_SIZE(formats), config->type);
 	if (err) {
 		DRM_ERROR("failed to initialize plane\n");
 		return err;
 	}
 
-	exynos_plane->zpos = zpos;
+	exynos_plane->zpos = config->zpos;
 
-	if (type == DRM_PLANE_TYPE_OVERLAY)
-		exynos_plane_attach_zpos_property(&exynos_plane->base, zpos);
+	if (config->type == DRM_PLANE_TYPE_OVERLAY)
+		exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
index f360590..12189c0 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
@@ -22,5 +22,4 @@ int exynos_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 			uint32_t src_w, uint32_t src_h);
 int exynos_plane_init(struct drm_device *dev,
 		      struct exynos_drm_plane *exynos_plane,
-		      unsigned long possible_crtcs, enum drm_plane_type type,
-		      unsigned int zpos);
+		      const struct exynos_drm_plane_config *config);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 27e84ec..ca7cc8a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -463,17 +463,20 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
 	struct vidi_context *ctx = dev_get_drvdata(dev);
 	struct drm_device *drm_dev = data;
 	struct exynos_drm_plane *exynos_plane;
-	enum drm_plane_type type;
-	unsigned int zpos;
+	struct exynos_drm_plane_config plane_config = { 0 };
+	unsigned int i;
 	int ret;
 
 	vidi_ctx_initialize(ctx, drm_dev);
 
-	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
-		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
-						DRM_PLANE_TYPE_OVERLAY;
-		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
-					1 << ctx->pipe, type, zpos);
+	plane_config.possible_crtcs = 1 << ctx->pipe;
+
+	for (i = 0; i < WINDOWS_NR; i++) {
+		plane_config.type = (i == ctx->default_win) ?
+			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+		plane_config.zpos = i;
+
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 9c398d5..207d5c9 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -1225,19 +1225,22 @@ static int mixer_bind(struct device *dev, struct device *manager, void *data)
 	struct mixer_context *ctx = dev_get_drvdata(dev);
 	struct drm_device *drm_dev = data;
 	struct exynos_drm_plane *exynos_plane;
-	enum drm_plane_type type;
-	unsigned int zpos;
+	struct exynos_drm_plane_config plane_config = { 0 };
+	unsigned int i;
 	int ret;
 
 	ret = mixer_initialize(ctx, drm_dev);
 	if (ret)
 		return ret;
 
-	for (zpos = 0; zpos < MIXER_WIN_NR; zpos++) {
-		type = (zpos == MIXER_DEFAULT_WIN) ? DRM_PLANE_TYPE_PRIMARY :
-						DRM_PLANE_TYPE_OVERLAY;
-		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
-					1 << ctx->pipe, type, zpos);
+	plane_config.possible_crtcs = 1 << ctx->pipe;
+
+	for (i = 0; i < MIXER_WIN_NR; i++) {
+		plane_config.type = (i == MIXER_DEFAULT_WIN) ?
+			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
+		plane_config.zpos = i;
+
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
 		if (ret)
 			return ret;
 	}
-- 
2.0.5

Re: [RFC 3/6] drm/exynos: introduce struct exynos_drm_plane_config

[Gustavo Padovan]

Hi Tobias,

2015-04-15 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:

> This struct encapsulates the configuration for a plane
> object. The pixel format config is currently unused.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 17 ++++++++++-------
>  drivers/gpu/drm/exynos/exynos_drm_drv.h    | 19 +++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 17 ++++++++++-------
>  drivers/gpu/drm/exynos/exynos_drm_plane.c  | 14 +++++++-------
>  drivers/gpu/drm/exynos/exynos_drm_plane.h  |  3 +--
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 17 ++++++++++-------
>  drivers/gpu/drm/exynos/exynos_mixer.c      | 17 ++++++++++-------
>  7 files changed, 67 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index 84a3638..ca70599 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -756,8 +756,8 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  	struct decon_context *ctx = dev_get_drvdata(dev);
>  	struct drm_device *drm_dev = data;
>  	struct exynos_drm_plane *exynos_plane;
> -	enum drm_plane_type type;
> -	unsigned int zpos;
> +	struct exynos_drm_plane_config plane_config = { 0 };
> +	unsigned int i;
>  	int ret;
>  
>  	ret = decon_ctx_initialize(ctx, drm_dev);
> @@ -766,11 +766,14 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  		return ret;
>  	}
>  
> -	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
> -		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
> -						DRM_PLANE_TYPE_OVERLAY;
> -		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
> -					1 << ctx->pipe, type, zpos);
> +	plane_config.possible_crtcs = 1 << ctx->pipe;
> +
> +	for (i = 0; i < WINDOWS_NR; i++) {
> +		plane_config.type = (i == ctx->default_win) ?
> +			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> +		plane_config.zpos = i;
> +
> +		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
>  		if (ret)
>  			return ret;
>  	}
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 4c14a89..35698f3 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -116,6 +116,25 @@ struct exynos_drm_plane {
>  };
>  
>  /*
> + * Exynos DRM plane configuration structure.
> + *
> + * @possible_crtcs: bitfield describing the valid CRTCs
> + *			for this plane.
> + * @type: plane type (primary, overlay, etc.)
> + * @zpos: z-position of the plane.
> + * @pixel_formats: supported pixel formats.
> + * @num_pixel_formats: number of elements in 'pixel_formats'.
> + */
> +
> +struct exynos_drm_plane_config {
> +	unsigned long possible_crtcs;
> +	enum drm_plane_type type;
> +	unsigned int zpos;
> +	const uint32_t *pixel_formats;
> +	unsigned int num_pixel_formats;
> +};

As a follow up of my atomic series I started cleaning up exynos drm a bit more
and right now I'm removing most of struct exynos_drm_plane. Most of the plane
information there is also present in plane->state thus I'm basically removing
all the duplicated information there.

That said, I think we avoid creating exynos_drm_plane_config and stuff
everything directly in struct exynos_drm_plane, it will be quite small and
easier to manipulate.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/6] drm/exynos: introduce struct exynos_drm_plane_config

[Tobias Jakobi]

Hello Gustavo!

Gustavo Padovan wrote:
> Hi Tobias,
> 
> 2015-04-15 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> 
>> This struct encapsulates the configuration for a plane
>> object. The pixel format config is currently unused.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 17 ++++++++++-------
>>  drivers/gpu/drm/exynos/exynos_drm_drv.h    | 19 +++++++++++++++++++
>>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 17 ++++++++++-------
>>  drivers/gpu/drm/exynos/exynos_drm_plane.c  | 14 +++++++-------
>>  drivers/gpu/drm/exynos/exynos_drm_plane.h  |  3 +--
>>  drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 17 ++++++++++-------
>>  drivers/gpu/drm/exynos/exynos_mixer.c      | 17 ++++++++++-------
>>  7 files changed, 67 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> index 84a3638..ca70599 100644
>> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
>> @@ -756,8 +756,8 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>>  	struct decon_context *ctx = dev_get_drvdata(dev);
>>  	struct drm_device *drm_dev = data;
>>  	struct exynos_drm_plane *exynos_plane;
>> -	enum drm_plane_type type;
>> -	unsigned int zpos;
>> +	struct exynos_drm_plane_config plane_config = { 0 };
>> +	unsigned int i;
>>  	int ret;
>>  
>>  	ret = decon_ctx_initialize(ctx, drm_dev);
>> @@ -766,11 +766,14 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>>  		return ret;
>>  	}
>>  
>> -	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
>> -		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
>> -						DRM_PLANE_TYPE_OVERLAY;
>> -		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
>> -					1 << ctx->pipe, type, zpos);
>> +	plane_config.possible_crtcs = 1 << ctx->pipe;
>> +
>> +	for (i = 0; i < WINDOWS_NR; i++) {
>> +		plane_config.type = (i == ctx->default_win) ?
>> +			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
>> +		plane_config.zpos = i;
>> +
>> +		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
>>  		if (ret)
>>  			return ret;
>>  	}
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> index 4c14a89..35698f3 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> @@ -116,6 +116,25 @@ struct exynos_drm_plane {
>>  };
>>  
>>  /*
>> + * Exynos DRM plane configuration structure.
>> + *
>> + * @possible_crtcs: bitfield describing the valid CRTCs
>> + *			for this plane.
>> + * @type: plane type (primary, overlay, etc.)
>> + * @zpos: z-position of the plane.
>> + * @pixel_formats: supported pixel formats.
>> + * @num_pixel_formats: number of elements in 'pixel_formats'.
>> + */
>> +
>> +struct exynos_drm_plane_config {
>> +	unsigned long possible_crtcs;
>> +	enum drm_plane_type type;
>> +	unsigned int zpos;
>> +	const uint32_t *pixel_formats;
>> +	unsigned int num_pixel_formats;
>> +};
> 
> As a follow up of my atomic series I started cleaning up exynos drm a bit more
> and right now I'm removing most of struct exynos_drm_plane. Most of the plane
> information there is also present in plane->state thus I'm basically removing
> all the duplicated information there.
Sounds like a good plan.


> That said, I think we avoid creating exynos_drm_plane_config and stuff
> everything directly in struct exynos_drm_plane, it will be quite small and
> easier to manipulate.
So that would imply that we then just have:
int exynos_plane_init(struct drm_device *dev, struct exynos_drm_plane
*exynos_plane);

Correct?

Anyway, I'm going to wait then until the cleanups are posted and rebase
this series onto it.

With best wishes,
Tobias

Re: [RFC 3/6] drm/exynos: introduce struct exynos_drm_plane_config

[Gustavo Padovan]

Hi Tobias,

2015-04-15 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:

> Hello Gustavo!
> 
> Gustavo Padovan wrote:
> > Hi Tobias,
> > 
> > 2015-04-15 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> > 
> >> This struct encapsulates the configuration for a plane
> >> object. The pixel format config is currently unused.
> >>
> >> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> >> ---
> >>  drivers/gpu/drm/exynos/exynos7_drm_decon.c | 17 ++++++++++-------
> >>  drivers/gpu/drm/exynos/exynos_drm_drv.h    | 19 +++++++++++++++++++
> >>  drivers/gpu/drm/exynos/exynos_drm_fimd.c   | 17 ++++++++++-------
> >>  drivers/gpu/drm/exynos/exynos_drm_plane.c  | 14 +++++++-------
> >>  drivers/gpu/drm/exynos/exynos_drm_plane.h  |  3 +--
> >>  drivers/gpu/drm/exynos/exynos_drm_vidi.c   | 17 ++++++++++-------
> >>  drivers/gpu/drm/exynos/exynos_mixer.c      | 17 ++++++++++-------
> >>  7 files changed, 67 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> >> index 84a3638..ca70599 100644
> >> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> >> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> >> @@ -756,8 +756,8 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
> >>  	struct decon_context *ctx = dev_get_drvdata(dev);
> >>  	struct drm_device *drm_dev = data;
> >>  	struct exynos_drm_plane *exynos_plane;
> >> -	enum drm_plane_type type;
> >> -	unsigned int zpos;
> >> +	struct exynos_drm_plane_config plane_config = { 0 };
> >> +	unsigned int i;
> >>  	int ret;
> >>  
> >>  	ret = decon_ctx_initialize(ctx, drm_dev);
> >> @@ -766,11 +766,14 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
> >>  		return ret;
> >>  	}
> >>  
> >> -	for (zpos = 0; zpos < WINDOWS_NR; zpos++) {
> >> -		type = (zpos == ctx->default_win) ? DRM_PLANE_TYPE_PRIMARY :
> >> -						DRM_PLANE_TYPE_OVERLAY;
> >> -		ret = exynos_plane_init(drm_dev, &ctx->planes[zpos],
> >> -					1 << ctx->pipe, type, zpos);
> >> +	plane_config.possible_crtcs = 1 << ctx->pipe;
> >> +
> >> +	for (i = 0; i < WINDOWS_NR; i++) {
> >> +		plane_config.type = (i == ctx->default_win) ?
> >> +			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> >> +		plane_config.zpos = i;
> >> +
> >> +		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
> >>  		if (ret)
> >>  			return ret;
> >>  	}
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> >> index 4c14a89..35698f3 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> >> @@ -116,6 +116,25 @@ struct exynos_drm_plane {
> >>  };
> >>  
> >>  /*
> >> + * Exynos DRM plane configuration structure.
> >> + *
> >> + * @possible_crtcs: bitfield describing the valid CRTCs
> >> + *			for this plane.
> >> + * @type: plane type (primary, overlay, etc.)
> >> + * @zpos: z-position of the plane.
> >> + * @pixel_formats: supported pixel formats.
> >> + * @num_pixel_formats: number of elements in 'pixel_formats'.
> >> + */
> >> +
> >> +struct exynos_drm_plane_config {
> >> +	unsigned long possible_crtcs;
> >> +	enum drm_plane_type type;
> >> +	unsigned int zpos;
> >> +	const uint32_t *pixel_formats;
> >> +	unsigned int num_pixel_formats;
> >> +};
> > 
> > As a follow up of my atomic series I started cleaning up exynos drm a bit more
> > and right now I'm removing most of struct exynos_drm_plane. Most of the plane
> > information there is also present in plane->state thus I'm basically removing
> > all the duplicated information there.
> Sounds like a good plan.
> 
> 
> > That said, I think we avoid creating exynos_drm_plane_config and stuff
> > everything directly in struct exynos_drm_plane, it will be quite small and
> > easier to manipulate.
> So that would imply that we then just have:
> int exynos_plane_init(struct drm_device *dev, struct exynos_drm_plane
> *exynos_plane);
> 
> Correct?

Correct, passing exynos_drm_plane simplifies things a lot for us.

> 
> Anyway, I'm going to wait then until the cleanups are posted and rebase
> this series onto it.

Right, I'll probably have that the next week or the other one, I'm currently
working on some testing scripts to speed up my testing and make sure I'm not
breaking anything.

	Gustavo
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 3/6] drm/exynos: introduce struct exynos_drm_plane_config

[Tobias Jakobi]

Hello!

On 2015-04-15 22:33, Gustavo Padovan wrote:
>> > As a follow up of my atomic series I started cleaning up exynos drm a bit more
>> > and right now I'm removing most of struct exynos_drm_plane. Most of the plane
>> > information there is also present in plane->state thus I'm basically removing
>> > all the duplicated information there.
>> Sounds like a good plan.
>> 
>> 
>> > That said, I think we avoid creating exynos_drm_plane_config and stuff
>> > everything directly in struct exynos_drm_plane, it will be quite small and
>> > easier to manipulate.
>> So that would imply that we then just have:
>> int exynos_plane_init(struct drm_device *dev, struct exynos_drm_plane
>> *exynos_plane);
>> 
>> Correct?
> 
> Correct, passing exynos_drm_plane simplifies things a lot for us.
> 
>> 
>> Anyway, I'm going to wait then until the cleanups are posted and 
>> rebase
>> this series onto it.
> 
> Right, I'll probably have that the next week or the other one, I'm 
> currently
> working on some testing scripts to speed up my testing and make sure 
> I'm not
> breaking anything.
Cool, take your time. And sorry that I haven't provided any feedback on 
the atomic patches yet. Something urgent came up that day and I haven't 
found enough time since then.

With best wishes,
Tobias

[RFC 4/6] drm/exynos: remove global pixel format list

[Tobias Jakobi]

Currently the pixel formats that are supported by a plane
object are hard-coded to three entries.

This is not correct since depending on the particular
hardware block (DECON7, FIMD, VP, etc.) the supported
formats are different.

Let each block specify its own list of formats.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos7_drm_decon.c |  7 +++++++
 drivers/gpu/drm/exynos/exynos_drm_fimd.c   |  7 +++++++
 drivers/gpu/drm/exynos/exynos_drm_plane.c  | 10 ++--------
 drivers/gpu/drm/exynos/exynos_drm_vidi.c   |  7 +++++++
 drivers/gpu/drm/exynos/exynos_mixer.c      | 18 ++++++++++++++++++
 5 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index ca70599..73788a1 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -42,6 +42,11 @@
 
 #define WINDOWS_NR	2
 
+static const uint32_t decon_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+};
+
 struct decon_context {
 	struct device			*dev;
 	struct drm_device		*drm_dev;
@@ -767,6 +772,8 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 	}
 
 	plane_config.possible_crtcs = 1 << ctx->pipe;
+	plane_config.pixel_formats = decon_formats;
+	plane_config.num_pixel_formats = ARRAY_SIZE(decon_formats);
 
 	for (i = 0; i < WINDOWS_NR; i++) {
 		plane_config.type = (i == ctx->default_win) ?
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 93fbaa5..1db8db7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -144,6 +144,11 @@ static struct fimd_driver_data exynos5_fimd_driver_data = {
 	.has_vtsel = 1,
 };
 
+static const uint32_t fimd_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+};
+
 struct fimd_context {
 	struct device			*dev;
 	struct drm_device		*drm_dev;
@@ -1005,6 +1010,8 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
 	ctx->pipe = priv->pipe++;
 
 	plane_config.possible_crtcs = 1 << ctx->pipe;
+	plane_config.pixel_formats = fimd_formats;
+	plane_config.num_pixel_formats = ARRAY_SIZE(fimd_formats);
 
 	for (i = 0; i < WINDOWS_NR; i++) {
 		plane_config.type = (i == ctx->default_win) ?
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index d24b32a..563d471 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -19,12 +19,6 @@
 #include "exynos_drm_gem.h"
 #include "exynos_drm_plane.h"
 
-static const uint32_t formats[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_NV12,
-};
-
 /*
  * This function is to get X or Y size shown via screen. This needs length and
  * start position of CRTC.
@@ -212,8 +206,8 @@ int exynos_plane_init(struct drm_device *dev,
 
 	err = drm_universal_plane_init(dev, &exynos_plane->base,
 					   config->possible_crtcs,
-				       &exynos_plane_funcs, formats,
-				       ARRAY_SIZE(formats), config->type);
+				       &exynos_plane_funcs, config->pixel_formats,
+				       config->num_pixel_formats, config->type);
 	if (err) {
 		DRM_ERROR("failed to initialize plane\n");
 		return err;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index ca7cc8a..94d2e84 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -33,6 +33,11 @@
 #define ctx_from_connector(c)	container_of(c, struct vidi_context, \
 					connector)
 
+static const uint32_t vidi_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+};
+
 struct vidi_context {
 	struct exynos_drm_display	display;
 	struct platform_device		*pdev;
@@ -470,6 +475,8 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
 	vidi_ctx_initialize(ctx, drm_dev);
 
 	plane_config.possible_crtcs = 1 << ctx->pipe;
+	plane_config.pixel_formats = vidi_formats;
+	plane_config.num_pixel_formats = ARRAY_SIZE(vidi_formats);
 
 	for (i = 0; i < WINDOWS_NR; i++) {
 		plane_config.type = (i == ctx->default_win) ?
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 207d5c9..50df981 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -43,6 +43,7 @@
 
 #define MIXER_WIN_NR		3
 #define MIXER_DEFAULT_WIN	0
+#define MIXER_VP_WIN		2
 
 #define MIXER_PIXELFORMAT_RGB565 4
 #define MIXER_PIXELFORMAT_ARGB1555 5
@@ -123,6 +124,15 @@ static const u8 filter_cr_horiz_tap4[] = {
 	70,	59,	48,	37,	27,	19,	11,	5,
 };
 
+static const uint32_t mixer_formats[] = {
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_ARGB8888,
+};
+
+static const uint32_t vp_formats[] = {
+	DRM_FORMAT_NV12,
+};
+
 static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
 {
 	return readl(res->vp_regs + reg_id);
@@ -1240,6 +1250,14 @@ static int mixer_bind(struct device *dev, struct device *manager, void *data)
 			DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
 		plane_config.zpos = i;
 
+		if (i == MIXER_VP_WIN && ctx->vp_enabled) {
+			plane_config.pixel_formats = vp_formats;
+			plane_config.num_pixel_formats = ARRAY_SIZE(vp_formats);
+		} else {
+			plane_config.pixel_formats = mixer_formats;
+			plane_config.num_pixel_formats = ARRAY_SIZE(mixer_formats);
+		}
+
 		ret = exynos_plane_init(drm_dev, &ctx->planes[i], &plane_config);
 		if (ret)
 			return ret;
-- 
2.0.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC 5/6] drm/exynos: mixer: use more general check for VP format

[Tobias Jakobi]

The mixer itself can't handle 'video' formats like NV12 or
NV21. It needs the VP (video processor) for this task.

With the previous patch we ensure that DRM planes with
a NV12 format can only be created when this setup is
supported. Hence the check now reduces to checking the
pixel format (currently only NV12).

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 50df981..512f7b3 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -133,6 +133,16 @@ static const uint32_t vp_formats[] = {
 	DRM_FORMAT_NV12,
 };
 
+static inline bool is_vp_format(const struct mixer_context *ctx, unsigned int win)
+{
+	switch (ctx->planes[win].pixel_format) {
+	case DRM_FORMAT_NV12:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
 {
 	return readl(res->vp_regs + reg_id);
@@ -970,7 +980,7 @@ static void mixer_win_commit(struct exynos_drm_crtc *crtc, unsigned int win)
 	}
 	mutex_unlock(&mixer_ctx->mixer_mutex);
 
-	if (win > 1 && mixer_ctx->vp_enabled)
+	if (is_vp_format(mixer_ctx, win))
 		vp_video_buffer(mixer_ctx, win);
 	else
 		mixer_graph_buffer(mixer_ctx, win);
-- 
2.0.5

[RFC 6/6] drm/exynos: mixer: add more pixel formats

[Tobias Jakobi]

The mixer natively support RGB565, ARGB4444 and ARGB1555
so expose these formats. Also, since being of 16-bit size,
these formats have a lower bandwidth requirement, making
them useful in situations where this is a bottleneck.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 512f7b3..7dec7c7 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -127,6 +127,11 @@ static const u8 filter_cr_horiz_tap4[] = {
 static const uint32_t mixer_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+	DRM_FORMAT_XRGB4444,
+	DRM_FORMAT_ARGB4444,
 };
 
 static const uint32_t vp_formats[] = {
@@ -556,30 +561,32 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
 	unsigned int x_ratio = 0, y_ratio = 0;
 	unsigned int src_x_offset, src_y_offset, dst_x_offset, dst_y_offset;
 	dma_addr_t dma_addr;
-	unsigned int fmt, blend;
+	unsigned int fmt, blend = 0;
 	u32 val;
 
 	plane = &ctx->planes[win];
 
 	switch (plane->pixel_format) {
 	case DRM_FORMAT_ARGB4444:
-		fmt = MIXER_PIXELFORMAT_ARGB4444;
 		blend = 1;
+	case DRM_FORMAT_XRGB4444:
+		fmt = MIXER_PIXELFORMAT_ARGB4444;
 		break;
 
 	case DRM_FORMAT_ARGB8888:
-		fmt = MIXER_PIXELFORMAT_ARGB8888;
 		blend = 1;
-		break;
-
 	case DRM_FORMAT_XRGB8888:
 		fmt = MIXER_PIXELFORMAT_ARGB8888;
-		blend = 0;
+		break;
+
+	case DRM_FORMAT_ARGB1555:
+		blend = 1;
+	case DRM_FORMAT_XRGB1555:
+		fmt = MIXER_PIXELFORMAT_ARGB1555;
 		break;
 
 	case DRM_FORMAT_RGB565:
 		fmt = MIXER_PIXELFORMAT_RGB565;
-		blend = 0;
 		break;
 
 	default:
-- 
2.0.5