Re: [PATCH 1/5] drm/exynos: mixer: refactor layer setup

[Tobias Jakobi <tjakobi@math.uni-bielefeld.de>]

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