Re: [PATCH 1/2] drm/tidss: Fix initial plane zpos values

[Marius Vlad <marius.vlad@collabora.com>]

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

On Tue, Feb 13, 2024 at 11:57:59AM +0200, Tomi Valkeinen wrote:
> Hi,
Hi,
> 
> On 13/02/2024 11:04, Pekka Paalanen wrote:
> > On Tue, 13 Feb 2024 10:16:36 +0200
> > Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> wrote:
> > 
> > > When the driver sets up the zpos property it sets the default zpos value
> > > to the HW id of the plane. That is fine as such, but as on many DSS
> > > versions the driver arranges the DRM planes in a different order than
> > > the HW planes (to keep the non-scalable planes first), this leads to odd
> > > initial zpos values. An example is J721e, where the initial zpos values
> > > for DRM planes are 1, 3, 0, 2.
> > > 
> > > In theory the userspace should configure the zpos values properly when
> > > using multiple planes, and in that sense the initial zpos values
> > > shouldn't matter, but there's really no reason not to fix this and help
> > > the userspace apps which don't handle zpos perfectly. In particular,
> > > Weston seems to have issues dealing with the planes with the current
> > > default zpos values.
> > > 
> > > So let's change the zpos values for the DRM planes to 0, 1, 2, 3.
> > > 
> > > Another option would be to configure the planes marked as primary planes
> > > to zpos 0. On a two display system this would give us plane zpos values
> > > of 0, 0, 1, 2. The end result and behavior would be very similar in this
> > > option, and I'm not aware that this would actually help us in any way.
> > > So, to keep the code simple, I opted for the 0, 1, 2, 3 values.
> > > 
> > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> > > Fixes: 32a1795f57ee ("drm/tidss: New driver for TI Keystone platform Display SubSystem")
> > 
> > Hi Tomi,
> > 
> > have you reported this to Weston? What exactly is the problem?
> 
> I haven't. I'm quite unfamiliar with Weston, and Randolph from TI (cc'd) has
> been working on the Weston side of things. I also don't know if there's
> something TI specific here, as the use case is with non-mainline GPU drivers
> and non-mainline Mesa. I should have been a bit clearer in the patch
> description, as I didn't mean that upstream Weston has a bug (maybe it has,
> maybe it has not).
> 
> The issue seen is that when Weston decides to use DRM planes for
> composition, the plane zpositions are not configured correctly (or at all?).
> Afaics, this leads to e.g. weston showing a window with a DRM "overlay"
> plane that is behind the "primary" root plane, so the window is not visible.
> And as Weston thinks that the area supposedly covered by the overlay plane
> does not need to be rendered on the root plane, there are also artifacts on
> that area.
> 
> Also, the Weston I used is a bit older one (10.0.1), as I needed to go back
> in my buildroot versions to get all that non-mainline GPU stuff compiled and
> working. A more recent Weston may behave differently.
Right after Weston 10, we had a few minor changes related to the
zpos-sorting list of planes and how we parse the plan list without having
a temporary zpos ordered list to pick planes from.

And there's another fix for missing out to set out the zpos for scanout
to the minimum available - which seems like a good candidate to explain
what happens in the issue described above. So if trying Weston again,
please try with at least Weston 12, which should have those changes
in.

> 
> > It doesn't seem like a good idea to work around userspace bugs
> > (non-regression, I presume?) with kernel changes.
> 
> Presuming this is not related to any TI specific code, I guess it's a
> regression in the sense that at some point Weston added the support to use
> planes for composition, so previously with only a single plane per display
> there was no issue.
> 
> I agree with you, this patch shouldn't be merged to "fix" issues with tidss
> + Weston. However, the current default zpos values really don't make sense,
> so I think this patch can stand on its own, and should be merged just to
> make the tidss behavior a bit saner.
> 
> But even if this patch merged, the issue with Weston should be looked at
> (*poke* Randolph =).
> 
>  Tomi
> 
> > 
> > 
> > Thanks,
> > pq
> > 
> > > ---
> > >   drivers/gpu/drm/tidss/tidss_plane.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/tidss/tidss_plane.c b/drivers/gpu/drm/tidss/tidss_plane.c
> > > index e1c0ef0c3894..68fed531f6a7 100644
> > > --- a/drivers/gpu/drm/tidss/tidss_plane.c
> > > +++ b/drivers/gpu/drm/tidss/tidss_plane.c
> > > @@ -213,7 +213,7 @@ struct tidss_plane *tidss_plane_create(struct tidss_device *tidss,
> > >   	drm_plane_helper_add(&tplane->plane, &tidss_plane_helper_funcs);
> > > -	drm_plane_create_zpos_property(&tplane->plane, hw_plane_id, 0,
> > > +	drm_plane_create_zpos_property(&tplane->plane, tidss->num_planes, 0,
> > >   				       num_planes - 1);
> > >   	ret = drm_plane_create_color_properties(&tplane->plane,
> > > 
> > 
> 

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