Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hello,

On 2015-12-17 03:55, Joonyoung Shim wrote:
> +Cc: Boram Park,
>
> Hi Marek,
>
> On 12/16/2015 09:21 PM, Marek Szyprowski wrote:
>> This patch adds all infrastructure to make zpos plane property
>> configurable from userspace.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 ++-
>>   drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++---
>>   2 files changed, 49 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> index 588b6763f9c7..f0827dbebb7d 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> @@ -64,6 +64,7 @@ struct exynos_drm_plane_state {
>>   	struct exynos_drm_rect src;
>>   	unsigned int h_ratio;
>>   	unsigned int v_ratio;
>> +	unsigned int zpos;
>>   };
>>   
>>   static inline struct exynos_drm_plane_state *
>> @@ -91,11 +92,12 @@ struct exynos_drm_plane {
>>   
>>   #define EXYNOS_DRM_PLANE_CAP_DOUBLE	(1 << 0)
>>   #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
>> +#define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
>>   
>>   /*
>>    * Exynos DRM plane configuration structure.
>>    *
>> - * @zpos: z-position of the plane.
>> + * @zpos: initial z-position of the plane.
>>    * @type: type of the plane (primary, cursor or overlay).
>>    * @pixel_formats: supported pixel formats.
>>    * @num_pixel_formats: number of elements in 'pixel_formats'.
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index fd6cb4cee01a..a2bdab836b50 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state)
>>   
>>   static void exynos_drm_plane_reset(struct drm_plane *plane)
>>   {
>> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>>   	struct exynos_drm_plane_state *exynos_state;
>>   
>>   	if (plane->state) {
>> @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
>>   
>>   	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
>>   	if (exynos_state) {
>> +		exynos_state->zpos = exynos_plane->config->zpos;
>>   		plane->state = &exynos_state->base;
>>   		plane->state->plane = plane;
>>   	}
>> @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
>>   		return NULL;
>>   
>>   	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
>> +	copy->zpos = exynos_state->zpos;
>>   	return &copy->base;
>>   }
>>   
>> @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
>>   	kfree(old_exynos_state);
>>   }
>>   
>> +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
>> +						struct drm_plane_state *state,
>> +						struct drm_property *property,
>> +						uint64_t val)
>> +{
>> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>> +	struct exynos_drm_plane_state *exynos_state =
>> +					to_exynos_plane_state(state);
>> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
>> +	const struct exynos_drm_plane_config *config = exynos_plane->config;
>> +
>> +	if (property == dev_priv->plane_zpos_property &&
>> +	    (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
>> +		exynos_state->zpos = val;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
>> +					  const struct drm_plane_state *state,
>> +					  struct drm_property *property,
>> +					  uint64_t *val)
>> +{
>> +	const struct exynos_drm_plane_state *exynos_state =
>> +		container_of(state, const struct exynos_drm_plane_state, base);
>> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
>> +
>> +	if (property == dev_priv->plane_zpos_property)
>> +		*val = exynos_state->zpos;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   static struct drm_plane_funcs exynos_plane_funcs = {
>>   	.update_plane	= drm_atomic_helper_update_plane,
>>   	.disable_plane	= drm_atomic_helper_disable_plane,
>>   	.destroy	= drm_plane_cleanup,
>> +	.set_property	= drm_atomic_helper_plane_set_property,
>>   	.reset		= exynos_drm_plane_reset,
>>   	.atomic_duplicate_state = exynos_drm_plane_duplicate_state,
>>   	.atomic_destroy_state = exynos_drm_plane_destroy_state,
>> +	.atomic_set_property = exynos_drm_plane_atomic_set_property,
>> +	.atomic_get_property = exynos_drm_plane_atomic_get_property,
>>   };
>>   
>>   static int
>> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
>>   
>>   	prop = dev_priv->plane_zpos_property;
>>   	if (!prop) {
>> -		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
>> -						 "zpos", 0, MAX_PLANE - 1);
>> +		prop = drm_property_create_range(dev, 0, "zpos",
>> +						 0, MAX_PLANE - 1);
> User are using zpos property as index now, if we make zpos property
> configurable, i think we need a property immutable like index or a
> property like type or name that can know this plane is which plane
> (e.g. video plane or graphic plane).

I don't get this. zpos property is attached only to OVERLAY planes, 
PRIMARY and CURSOR don't have it, so userspace cannot rely on this 
property. In my patch the initial zpos value is same as before, so if 
application doesn't relies on zpos values, it will get the same values 
as now. Application can also check the plane type by reading 'type' 
property (PRIMARY, OVERLAY or CURSOR). However after my patch 
application will get the possibility to rearrange plane order for the 
given crtc by chaning zpos values. That's especially useful to configure 
2 typical use cases: PRIMARY plane over OVERLAY plane (PRIMARY is used 
as typical on-screen-display) or OVERLAY plane over PRIMARY (OVERLAY is 
used as a window for displaying video data).

> Another way, user may be possible to check plane index as order of
> plane object id.

Application can easily find all the planes, which belongs to given crtc 
by checking 'possible crtcs' plane parameter, I see no additional need 
for 'index' property in such case. Please note that the initial plane 
configuration in case of atomic api will be always the same for all 
application (determined by exynos_drm_plane_reset function) and it will 
match the current setup.

>
> Thanks.
>
>>   		if (!prop)
>>   			return;
>>   
>> @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev,
>>   	exynos_plane->index = index;
>>   	exynos_plane->config = config;
>>   
>> -	if (config->type == DRM_PLANE_TYPE_OVERLAY)
>> -		exynos_plane_attach_zpos_property(&exynos_plane->base,
>> -						  config->zpos);
>> +	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
>>   
>>   	return 0;
>>   }
>>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland