[1/5] drm/exynos: mixer: refactor layer setup
diff mbox

Message ID 1430919410-9917-2-git-send-email-tjakobi@math.uni-bielefeld.de
State New
Headers show

Commit Message

Tobias Jakobi May 6, 2015, 1:36 p.m. UTC
First step in allowing a more generic way to setup complex
blending for the different layers.

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

Comments

Joonyoung Shim May 22, 2015, 9:12 a.m. UTC | #1
On 05/06/2015 10:36 PM, Tobias Jakobi wrote:
> First step in allowing a more generic way to setup complex
> blending for the different layers.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 90 ++++++++++++++++++++++++++++++-----
>  1 file changed, 79 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 4a1656b..e4a5e76 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -63,6 +63,11 @@ struct mixer_resources {
>  	struct clk		*mout_mixer;
>  };
>  
> +struct layer_config {
> +	unsigned int index;
> +	unsigned int priority;

I think layer index and priority information can go to struct
exynos_drm_plane.

> +};
> +
>  enum mixer_version_id {
>  	MXR_VER_0_0_0_16,
>  	MXR_VER_16_0_33_0,
> @@ -75,6 +80,8 @@ struct mixer_context {
>  	struct drm_device	*drm_dev;
>  	struct exynos_drm_crtc	*crtc;
>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
> +	const struct layer_config *layer_config;
> +	unsigned int num_layer;
>  	int			pipe;
>  	bool			interlace;
>  	bool			powered;
> @@ -95,6 +102,40 @@ struct mixer_drv_data {
>  	bool					has_sclk;
>  };
>  
> +/*
> + * The default layer priorities. A higher priority means that
> + * the layer is at the top of layer stack.
> + * The current configuration assumes the following usage scenario:
> + * layer1: OSD [top]
> + * layer0: main framebuffer
> + * video layer: video overlay [bottom]
> + * Note that the video layer is only usable when the
> + * video processor is available.
> + */
> +
> +static const struct layer_config default_layer_config[] = {
> +	{
> +		.index = 0,			/* layer0 */
> +		.priority = 1
> +	}, {
> +		.index = 1,			/* layer1 */
> +		.priority = 2
> +	}
> +};
> +
> +static const struct layer_config vp_layer_config[] = {
> +	{
> +		.index = 2,			/* video layer */
> +		.priority = 1
> +	}, {
> +		.index = 0,			/* layer0 */
> +		.priority = 2
> +	}, {
> +		.index = 1,			/* layer1 */
> +		.priority = 3
> +	}
> +};
> +

I feel there is no reason to split config, actually video layer exists
and video layer isn't enabled if no vp even if we use 3 layer.
Tobias Jakobi May 22, 2015, 9:47 a.m. UTC | #2
Hello Joonyoung,

On 2015-05-22 11:12, Joonyoung Shim wrote:
> On 05/06/2015 10:36 PM, Tobias Jakobi wrote:
>> First step in allowing a more generic way to setup complex
>> blending for the different layers.
>> 
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 90 
>> ++++++++++++++++++++++++++++++-----
>>  1 file changed, 79 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
>> b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 4a1656b..e4a5e76 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -63,6 +63,11 @@ struct mixer_resources {
>>  	struct clk		*mout_mixer;
>>  };
>> 
>> +struct layer_config {
>> +	unsigned int index;
>> +	unsigned int priority;
> 
> I think layer index and priority information can go to struct
> exynos_drm_plane.
No, because of two reasons:
- plane configuration is done at run-time while this here is const 
static
- this is specific to the mixer


> 
>> +};
>> +
>>  enum mixer_version_id {
>>  	MXR_VER_0_0_0_16,
>>  	MXR_VER_16_0_33_0,
>> @@ -75,6 +80,8 @@ struct mixer_context {
>>  	struct drm_device	*drm_dev;
>>  	struct exynos_drm_crtc	*crtc;
>>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>> +	const struct layer_config *layer_config;
>> +	unsigned int num_layer;
>>  	int			pipe;
>>  	bool			interlace;
>>  	bool			powered;
>> @@ -95,6 +102,40 @@ struct mixer_drv_data {
>>  	bool					has_sclk;
>>  };
>> 
>> +/*
>> + * The default layer priorities. A higher priority means that
>> + * the layer is at the top of layer stack.
>> + * The current configuration assumes the following usage scenario:
>> + * layer1: OSD [top]
>> + * layer0: main framebuffer
>> + * video layer: video overlay [bottom]
>> + * Note that the video layer is only usable when the
>> + * video processor is available.
>> + */
>> +
>> +static const struct layer_config default_layer_config[] = {
>> +	{
>> +		.index = 0,			/* layer0 */
>> +		.priority = 1
>> +	}, {
>> +		.index = 1,			/* layer1 */
>> +		.priority = 2
>> +	}
>> +};
>> +
>> +static const struct layer_config vp_layer_config[] = {
>> +	{
>> +		.index = 2,			/* video layer */
>> +		.priority = 1
>> +	}, {
>> +		.index = 0,			/* layer0 */
>> +		.priority = 2
>> +	}, {
>> +		.index = 1,			/* layer1 */
>> +		.priority = 3
>> +	}
>> +};
>> +
> 
> I feel there is no reason to split config, actually video layer exists
> and video layer isn't enabled if no vp even if we use 3 layer.
Well, as you point out yourself the current code is totally broken. Even 
if we don't support the video processor we still expose the 
corresponding DRM plane to userspace, but then just ignore any 
operations to it. This needs fixing (!), and my series is a first step 
in that direction.


With best wishes,
Tobias
Tobias Jakobi May 22, 2015, 4:01 p.m. UTC | #3
On 2015-05-22 11:12, Joonyoung Shim wrote:
> On 05/06/2015 10:36 PM, Tobias Jakobi wrote:
>> First step in allowing a more generic way to setup complex
>> blending for the different layers.
>> 
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 90 
>> ++++++++++++++++++++++++++++++-----
>>  1 file changed, 79 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c 
>> b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 4a1656b..e4a5e76 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -63,6 +63,11 @@ struct mixer_resources {
>>  	struct clk		*mout_mixer;
>>  };
>> 
>> +struct layer_config {
>> +	unsigned int index;
>> +	unsigned int priority;
> 
> I think layer index and priority information can go to struct
> exynos_drm_plane.
> 
>> +};
>> +
>>  enum mixer_version_id {
>>  	MXR_VER_0_0_0_16,
>>  	MXR_VER_16_0_33_0,
>> @@ -75,6 +80,8 @@ struct mixer_context {
>>  	struct drm_device	*drm_dev;
>>  	struct exynos_drm_crtc	*crtc;
>>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>> +	const struct layer_config *layer_config;
>> +	unsigned int num_layer;
>>  	int			pipe;
>>  	bool			interlace;
>>  	bool			powered;
>> @@ -95,6 +102,40 @@ struct mixer_drv_data {
>>  	bool					has_sclk;
>>  };
>> 
>> +/*
>> + * The default layer priorities. A higher priority means that
>> + * the layer is at the top of layer stack.
>> + * The current configuration assumes the following usage scenario:
>> + * layer1: OSD [top]
>> + * layer0: main framebuffer
>> + * video layer: video overlay [bottom]
>> + * Note that the video layer is only usable when the
>> + * video processor is available.
>> + */
>> +
>> +static const struct layer_config default_layer_config[] = {
>> +	{
>> +		.index = 0,			/* layer0 */
>> +		.priority = 1
>> +	}, {
>> +		.index = 1,			/* layer1 */
>> +		.priority = 2
>> +	}
>> +};
>> +
>> +static const struct layer_config vp_layer_config[] = {
>> +	{
>> +		.index = 2,			/* video layer */
>> +		.priority = 1
>> +	}, {
>> +		.index = 0,			/* layer0 */
>> +		.priority = 2
>> +	}, {
>> +		.index = 1,			/* layer1 */
>> +		.priority = 3
>> +	}
>> +};
>> +
> 
> I feel there is no reason to split config, actually video layer exists
> and video layer isn't enabled if no vp even if we use 3 layer.
Maybe I should add this here. Both default_layer_config and 
vp_layer_config function as default layer configurations. My plan is to 
expose custom configuration through a exynosdrm module parameter. The 
zpos property is immutable, so we can't modify it anymore after mixer 
probing (I understand that the immutabilility is a requirement of 
atomic). But we still can allow the user to supply his configuraton 
through the module parameter approach.


With best wishes,
Tobias

Patch
diff mbox

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 4a1656b..e4a5e76 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -63,6 +63,11 @@  struct mixer_resources {
 	struct clk		*mout_mixer;
 };
 
+struct layer_config {
+	unsigned int index;
+	unsigned int priority;
+};
+
 enum mixer_version_id {
 	MXR_VER_0_0_0_16,
 	MXR_VER_16_0_33_0,
@@ -75,6 +80,8 @@  struct mixer_context {
 	struct drm_device	*drm_dev;
 	struct exynos_drm_crtc	*crtc;
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
+	const struct layer_config *layer_config;
+	unsigned int num_layer;
 	int			pipe;
 	bool			interlace;
 	bool			powered;
@@ -95,6 +102,40 @@  struct mixer_drv_data {
 	bool					has_sclk;
 };
 
+/*
+ * The default layer priorities. A higher priority means that
+ * the layer is at the top of layer stack.
+ * The current configuration assumes the following usage scenario:
+ * layer1: OSD [top]
+ * layer0: main framebuffer
+ * video layer: video overlay [bottom]
+ * Note that the video layer is only usable when the
+ * video processor is available.
+ */
+
+static const struct layer_config default_layer_config[] = {
+	{
+		.index = 0,			/* layer0 */
+		.priority = 1
+	}, {
+		.index = 1,			/* layer1 */
+		.priority = 2
+	}
+};
+
+static const struct layer_config vp_layer_config[] = {
+	{
+		.index = 2,			/* video layer */
+		.priority = 1
+	}, {
+		.index = 0,			/* layer0 */
+		.priority = 2
+	}, {
+		.index = 1,			/* layer1 */
+		.priority = 3
+	}
+};
+
 static const u8 filter_y_horiz_tap8[] = {
 	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
 	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
@@ -253,6 +294,34 @@  static void vp_default_filter(struct mixer_resources *res)
 		filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
 }
 
+static void mixer_layer_priority(struct mixer_context *ctx)
+{
+	u32 val = 0;
+	unsigned int i, priority;
+
+	for (i = 0; i < ctx->num_layer; ++i) {
+		priority = ctx->layer_config[i].priority;
+		BUG_ON(priority > 15);
+
+		switch (ctx->layer_config[i].index) {
+		case 0:
+			val |= MXR_LAYER_CFG_GRP0_VAL(priority);
+			break;
+		case 1:
+			val |= MXR_LAYER_CFG_GRP1_VAL(priority);
+			break;
+		case 2:
+			val |= MXR_LAYER_CFG_VP_VAL(priority);
+			break;
+		default:
+			BUG_ON(true);
+			break;
+		}
+	}
+
+	mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
+}
+
 static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
@@ -654,17 +723,7 @@  static void mixer_win_reset(struct mixer_context *ctx)
 	mixer_reg_writemask(res, MXR_STATUS, MXR_STATUS_16_BURST,
 		MXR_STATUS_BURST_MASK);
 
-	/* setting default layer priority: layer1 > layer0 > video
-	 * because typical usage scenario would be
-	 * layer1 - OSD
-	 * layer0 - framebuffer
-	 * video - video overlay
-	 */
-	val = MXR_LAYER_CFG_GRP1_VAL(3);
-	val |= MXR_LAYER_CFG_GRP0_VAL(2);
-	if (ctx->vp_enabled)
-		val |= MXR_LAYER_CFG_VP_VAL(1);
-	mixer_reg_write(res, MXR_LAYER_CFG, val);
+	mixer_layer_priority(ctx);
 
 	/* setting background color */
 	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
@@ -1274,6 +1333,15 @@  static int mixer_probe(struct platform_device *pdev)
 	ctx->vp_enabled = drv->is_vp_enabled;
 	ctx->has_sclk = drv->has_sclk;
 	ctx->mxr_ver = drv->version;
+
+	if (drv->is_vp_enabled) {
+		ctx->layer_config = vp_layer_config;
+		ctx->num_layer = ARRAY_SIZE(vp_layer_config);
+	} else {
+		ctx->layer_config = default_layer_config;
+		ctx->num_layer = ARRAY_SIZE(default_layer_config);
+	}
+
 	init_waitqueue_head(&ctx->wait_vsync_queue);
 	atomic_set(&ctx->wait_vsync_event, 0);