[PATCH v2] drm/tegra: Add zpos property for cursor planes

[PATCH v2] drm/tegra: Add zpos property for cursor planes

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

As of commit 4dc55525b095 ("drm: plane: Verify that no or all planes
have a zpos property") a warning is emitted if there's a mix of planes
with and without a zpos property.

On Tegra, cursor planes are always composited on top of all other
planes, which is why they never had a zpos property attached to them.
However, since the composition order is fixed, this is trivial to
remedy by simply attaching an immutable zpos property to them.

Changes in v2:
- hardcode cursor plane zpos to 255 instead of 0 (Ville)

Reported-by: Jonathan Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
 drivers/gpu/drm/tegra/hub.c | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 83f31c6e891c..85408eed4685 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -787,7 +787,7 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm,
 	}
 
 	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
-	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
+	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
 
 	err = drm_plane_create_rotation_property(&plane->base,
 						 DRM_MODE_ROTATE_0,
@@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
 	}
 
 	drm_plane_helper_add(&plane->base, &tegra_cursor_plane_helper_funcs);
+	drm_plane_create_zpos_immutable_property(&plane->base, 255);
 
 	return &plane->base;
 }
@@ -1074,7 +1075,11 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
 	}
 
 	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
-	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
+
+	if (!cursor)
+		drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
+	else
+		drm_plane_create_zpos_immutable_property(&plane->base, 255);
 
 	err = drm_plane_create_rotation_property(&plane->base,
 						 DRM_MODE_ROTATE_0,
diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
index 22a03f7ffdc1..f8afc05bceb3 100644
--- a/drivers/gpu/drm/tegra/hub.c
+++ b/drivers/gpu/drm/tegra/hub.c
@@ -590,7 +590,7 @@ struct drm_plane *tegra_shared_plane_create(struct drm_device *drm,
 	}
 
 	drm_plane_helper_add(p, &tegra_shared_plane_helper_funcs);
-	drm_plane_create_zpos_property(p, 0, 0, 255);
+	drm_plane_create_zpos_property(p, index, 0, 254);
 
 	return p;
 }
-- 
2.24.1

Re: [PATCH v2] drm/tegra: Add zpos property for cursor planes

[Dmitry Osipenko]

16.06.2020 15:17, Thierry Reding пишет:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> As of commit 4dc55525b095 ("drm: plane: Verify that no or all planes
> have a zpos property") a warning is emitted if there's a mix of planes
> with and without a zpos property.
> 
> On Tegra, cursor planes are always composited on top of all other
> planes, which is why they never had a zpos property attached to them.
> However, since the composition order is fixed, this is trivial to
> remedy by simply attaching an immutable zpos property to them.
> 
> Changes in v2:
> - hardcode cursor plane zpos to 255 instead of 0 (Ville)
> 
> Reported-by: Jonathan Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
>  drivers/gpu/drm/tegra/hub.c | 2 +-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> index 83f31c6e891c..85408eed4685 100644
> --- a/drivers/gpu/drm/tegra/dc.c
> +++ b/drivers/gpu/drm/tegra/dc.c
> @@ -787,7 +787,7 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm,
>  	}
>  
>  	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
> -	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
> +	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
>  
>  	err = drm_plane_create_rotation_property(&plane->base,
>  						 DRM_MODE_ROTATE_0,
> @@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
>  	}
>  
>  	drm_plane_helper_add(&plane->base, &tegra_cursor_plane_helper_funcs);
> +	drm_plane_create_zpos_immutable_property(&plane->base, 255);
>  
>  	return &plane->base;
>  }
> @@ -1074,7 +1075,11 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
>  	}
>  
>  	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
> -	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
> +
> +	if (!cursor)
> +		drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
> +	else
> +		drm_plane_create_zpos_immutable_property(&plane->base, 255);

On T20/30 we're are setting the plane's type to CURSOR because we want
to use one overlay plane for the mouse cursor. Nevertheless, it's still
a generic overlay plane that can change its z-position, and thus, it's
wrong to make zpos immutable here.

Re: [PATCH v2] drm/tegra: Add zpos property for cursor planes

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3238 bytes --]

On Tue, Jun 16, 2020 at 06:54:35PM +0300, Dmitry Osipenko wrote:
> 16.06.2020 15:17, Thierry Reding пишет:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > As of commit 4dc55525b095 ("drm: plane: Verify that no or all planes
> > have a zpos property") a warning is emitted if there's a mix of planes
> > with and without a zpos property.
> > 
> > On Tegra, cursor planes are always composited on top of all other
> > planes, which is why they never had a zpos property attached to them.
> > However, since the composition order is fixed, this is trivial to
> > remedy by simply attaching an immutable zpos property to them.
> > 
> > Changes in v2:
> > - hardcode cursor plane zpos to 255 instead of 0 (Ville)
> > 
> > Reported-by: Jonathan Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
> >  drivers/gpu/drm/tegra/hub.c | 2 +-
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index 83f31c6e891c..85408eed4685 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -787,7 +787,7 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm,
> >  	}
> >  
> >  	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
> > -	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
> > +	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
> >  
> >  	err = drm_plane_create_rotation_property(&plane->base,
> >  						 DRM_MODE_ROTATE_0,
> > @@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
> >  	}
> >  
> >  	drm_plane_helper_add(&plane->base, &tegra_cursor_plane_helper_funcs);
> > +	drm_plane_create_zpos_immutable_property(&plane->base, 255);
> >  
> >  	return &plane->base;
> >  }
> > @@ -1074,7 +1075,11 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
> >  	}
> >  
> >  	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
> > -	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
> > +
> > +	if (!cursor)
> > +		drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
> > +	else
> > +		drm_plane_create_zpos_immutable_property(&plane->base, 255);
> 
> On T20/30 we're are setting the plane's type to CURSOR because we want
> to use one overlay plane for the mouse cursor. Nevertheless, it's still
> a generic overlay plane that can change its z-position, and thus, it's
> wrong to make zpos immutable here.

But it doesn't really make sense for the cursor plane to change z-
position, even if it's technically possible. We do want it to always be
on top anyway. Doing it this way makes the cursor behave the same way
irrespective of the Tegra generation that we're running on.

Yes, that may not fully expose the capabilities of the hardware, but we
are already restricting the hardware capabilities by exposing the
overlay plane as a cursor plane in the first place.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] drm/tegra: Add zpos property for cursor planes

[Dmitry Osipenko]

16.06.2020 19:13, Thierry Reding пишет:
> On Tue, Jun 16, 2020 at 06:54:35PM +0300, Dmitry Osipenko wrote:
>> 16.06.2020 15:17, Thierry Reding пишет:
>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> As of commit 4dc55525b095 ("drm: plane: Verify that no or all planes
>>> have a zpos property") a warning is emitted if there's a mix of planes
>>> with and without a zpos property.
>>>
>>> On Tegra, cursor planes are always composited on top of all other
>>> planes, which is why they never had a zpos property attached to them.
>>> However, since the composition order is fixed, this is trivial to
>>> remedy by simply attaching an immutable zpos property to them.
>>>
>>> Changes in v2:
>>> - hardcode cursor plane zpos to 255 instead of 0 (Ville)
>>>
>>> Reported-by: Jonathan Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>  drivers/gpu/drm/tegra/dc.c  | 9 +++++++--
>>>  drivers/gpu/drm/tegra/hub.c | 2 +-
>>>  2 files changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
>>> index 83f31c6e891c..85408eed4685 100644
>>> --- a/drivers/gpu/drm/tegra/dc.c
>>> +++ b/drivers/gpu/drm/tegra/dc.c
>>> @@ -787,7 +787,7 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm,
>>>  	}
>>>  
>>>  	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
>>> -	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
>>> +	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
>>>  
>>>  	err = drm_plane_create_rotation_property(&plane->base,
>>>  						 DRM_MODE_ROTATE_0,
>>> @@ -957,6 +957,7 @@ static struct drm_plane *tegra_dc_cursor_plane_create(struct drm_device *drm,
>>>  	}
>>>  
>>>  	drm_plane_helper_add(&plane->base, &tegra_cursor_plane_helper_funcs);
>>> +	drm_plane_create_zpos_immutable_property(&plane->base, 255);
>>>  
>>>  	return &plane->base;
>>>  }
>>> @@ -1074,7 +1075,11 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,
>>>  	}
>>>  
>>>  	drm_plane_helper_add(&plane->base, &tegra_plane_helper_funcs);
>>> -	drm_plane_create_zpos_property(&plane->base, plane->index, 0, 255);
>>> +
>>> +	if (!cursor)
>>> +		drm_plane_create_zpos_property(&plane->base, plane->index, 0, 254);
>>> +	else
>>> +		drm_plane_create_zpos_immutable_property(&plane->base, 255);
>>
>> On T20/30 we're are setting the plane's type to CURSOR because we want
>> to use one overlay plane for the mouse cursor. Nevertheless, it's still
>> a generic overlay plane that can change its z-position, and thus, it's
>> wrong to make zpos immutable here.
> 
> But it doesn't really make sense for the cursor plane to change z-
> position, even if it's technically possible. We do want it to always be
> on top anyway. Doing it this way makes the cursor behave the same way
> irrespective of the Tegra generation that we're running on.
> 
> Yes, that may not fully expose the capabilities of the hardware, but we
> are already restricting the hardware capabilities by exposing the
> overlay plane as a cursor plane in the first place.

Userspace is free to reuse cursor's plane for different purposes. For
example, cursor may be not needed at all and then cursor plane could be
used as a regular overlay plane.

The patch's title and commit's description says "*Add* zpos property for
cursor planes", but in this case zpos property already existed before
this patch, hence you're changing the old behavior here.