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

[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?

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

With best wishes,
Tobias