Re: [git pull] drm for 5.8-rc1

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5556 bytes --]

On Fri, Aug 14, 2020 at 06:12:56PM +0200, Karol Herbst wrote:
> On Fri, Aug 14, 2020 at 6:06 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Fri, Aug 14, 2020 at 05:40:34PM +0200, Karol Herbst wrote:
> > > On Fri, Aug 14, 2020 at 5:35 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > On Fri, Aug 14, 2020 at 04:44:43PM +0200, Karol Herbst wrote:
> > > > > On Fri, Aug 14, 2020 at 4:05 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Aug 13, 2020 at 10:45:34AM -0700, James Jones wrote:
> > > > > > > I'll defer to Thierry, but I think that may be by design.  Tegra format
> > > > > > > modifiers were added to get things like this working in the first place,
> > > > > > > right?  It's not a regression, is it?
> > > > > >
> > > > > > I recall that things used to work with or without modifiers at some
> > > > > > point. That was basically the point of the "pitch-linear by default"
> > > > > > patch, see:
> > > > > >
> > > > > >     https://gitlab.freedesktop.org/mesa/mesa/-/commit/9603d81df05105857b676f20dff964ef3ab0ecff
> > > > > >
> > > > > > If we don't force pitch-linear for cases where we don't have modifiers,
> > > > > > there's no way we can properly display these as framebuffers because we
> > > > > > lack the modifier information at the kernel level.
> > > > > >
> > > > >
> > > > > I suspect that's related to
> > > > > https://gitlab.freedesktop.org/mesa/mesa/commit/c56fe4118a2dd991ff1b2a532c0f234eddd435e9
> > > >
> > > > But we're checking the the PIPE_USAGE_SCANOUT flag specifically for that
> > > > type of case, so this should work. On the other hand, it does sound like
> > > > that logic might actually work, but for some reason Nouveau then ends up
> > > > allocating a linear render buffer and a tiled depth buffer, and that's
> > > > the combination that doesn't work. Did I understand that correctly?
> > > >
> > > > So it sounds like there's a few options:
> > > >
> > > >   a) Make Nouveau render without a depth buffer in these cases (not sure
> > > >      if that's even possible).
> > > >
> > > >   b) Allocate a linear buffer *in addition* to the normal buffer that
> > > >      Nouveau allocates (with whatever it selects as the default layout)
> > > >      and then blit from the tiled buffer to the linear buffer everytime
> > > >      we need to. I think that'd be basically reproducing the code that
> > > >      is controlled by the DRI_PRIME environment variable.
> > > >
> > >
> > > yeah, I think Ilia suggested something like that a long time ago as well.
> > >
> > > >   c) Accept that modifiers are the way to go and rely on them for proper
> > > >      functionality. This is obviously a really bad solution because not
> > > >      everyone has transitioned to framebuffer modifiers yet. On the
> > > >      other hand, this is precisely the kind of use-case that framebuffer
> > > >      modifiers were meant to address, so it isn't all that far-fetched
> > > >      to make them a requirement.
> > > >
> > >
> > > the problem with that is, that by default we don't have modifier aware
> > > desktops. gnome-shell by default and no Xorg release.
> > >
> > > > None of those are really great options... does anyone have any other
> > > > ideas?
> > > >
> > >
> > > What I am wondering about is why with my patch it simply works in X,
> > > but that could be a mere coincidence.
> >
> > So I was testing your revert with Weston and that "works", but only as
> > in "it doesn't crash". As expected, since there's now a mismatch between
> > what layout Nouveau assumes and the pitch-linear buffer that modesetting
> > assumes it got, it'll now be completely scrambled. That said, even
> > without the revert I can't seem to be able to make Weston work without
> > modifiers support.
> >
> 
> yeah, that does seem to reproduce what I noticed with
> gnome-shell/wayland as well.

Oh, here's the patch I used to disable framebuffer modifiers in Weston,
in case anyone is interested:

--- >8 ---
diff --git a/libweston/backend-drm/kms.c b/libweston/backend-drm/kms.c
index f5215f20d694..889b2444b99f 100644
--- a/libweston/backend-drm/kms.c
+++ b/libweston/backend-drm/kms.c
@@ -1480,6 +1480,10 @@ init_kms_caps(struct drm_backend *b)
 	else
 		b->fb_modifiers = 0;
 
+	if (getenv("WESTON_DISABLE_FB_MODIFIERS")) {
+		b->fb_modifiers = 0;
+	}
+
 	/*
 	 * KMS support for hardware planes cannot properly synchronize
 	 * without nuclear page flip. Without nuclear/atomic, hw plane
--- >8 ---

I suspect that the reason why this works in X but not in Wayland is
because X passes the right usage flags, whereas Weston may not. But I'll
have to investigate more in order to be sure.

In either case, it does seem to me like b) above would be the best
solution for the legacy case (i.e. where we don't have modifiers). It's
not going to be great in terms of performance because of the extra copy,
but I thought I had seen one of the render-only drivers do something
similar, so I'll go look at them to see if I can use them as inspiration
for implementing something like this on Tegra.

This should ideally only be an issue for desktops that don't support FB
modifiers at the moment. Even if that's the default for most at this
point, hopefully that will change down the road.

Perhaps we can go and release X 1.21.0 with that modifier enablement
patch and that'll motivate desktops to adopt it as well as the default?

Thierry

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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel