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.valkei...@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.valkei...@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, > > > > > >
signature.asc
Description: PGP signature
