[PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Keith Packard]

Here are a couple of small bug fixes which make DRI3/Present work
better with UXA.

 [PATCH 1/2] Do not clear pending kernel events on mode switch

This patch prevents GL-based compositing managers from wedging when
performing video mode setting. The problem was that DIX was never
receiving notification about page flips being completed when one was
pending across a mode switch.

 [PATCH 2/2] Correct BO allocation alignment

This patch makes UXA and Mesa agree about how buffers are allocated
for images. Without this, UXA was requiring larger padding, which
meant that converting some textures into pixmaps using DRI3 would
fail.

-keith

[PATCH 1/2] Do not clear pending kernel events on mode switch

[Keith Packard]

Let the kernel send these back to us so that DIX hears about them in
the usual way.

Mode setting while Present has a flip active will trigger an unflip
before the mode is changed. The event from that unflip will not get
processed before the mode switch is executed. Clearing the driver
queue at mode switch time will discard the connection between the
kernel event and the present callback so that DIX will never know that
the flip pixmap is idle.

Signed-off-by: Keith Packard <keithp-aN4HjG94KOLQT0dZR+AlfA@public.gmane.org>
---
 src/uxa/intel_display.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
index 97af76d..8c43ae9 100644
--- a/src/uxa/intel_display.c
+++ b/src/uxa/intel_display.c
@@ -71,9 +71,6 @@ struct intel_drm_queue {
         intel_drm_abort_proc abort;
 };
 
-static void
-intel_drm_abort_scrn(ScrnInfoPtr scrn);
-
 static uint32_t intel_drm_seq;
 static struct list intel_drm_queue;
 
@@ -398,7 +395,6 @@ intel_crtc_apply(xf86CrtcPtr crtc)
 
 	if (scrn->pScreen)
 		xf86_reload_cursors(scrn->pScreen);
-        intel_drm_abort_scrn(scrn);
 
 done:
 	free(output_ids);
-- 
2.0.1

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 2/2] Correct BO allocation alignment

[Keith Packard]

BO allocations for pixmaps must be aligned to the tile height, but at
some point the code was changed to align them to twice the tile
height. This overallocates pixmaps, wasting memory, but more
importantly, for buffers allocated by DRM and shared through DRI3, the
stricter alignment check causes sharing to fail.

>From reading through the history of the code and related bugs, it
seems like this change was part of a set of changes trying to address
what turned out to be a kernel regression. Reverting this change
solves the DRI3 problem and saves a bit of memory for pixmap
allocations.

Signed-off-by: Keith Packard <keithp-aN4HjG94KOLQT0dZR+AlfA@public.gmane.org>
---
 src/uxa/intel_uxa.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/uxa/intel_uxa.c b/src/uxa/intel_uxa.c
index d33eca5..4ce6eae 100644
--- a/src/uxa/intel_uxa.c
+++ b/src/uxa/intel_uxa.c
@@ -206,7 +206,7 @@ intel_uxa_compute_size(struct intel_screen_private *intel,
 			tile_height = 8;
 		else
 			tile_height = 32;
-		aligned_h = ALIGN(h, 2*tile_height);
+		aligned_h = ALIGN(h, tile_height);
 
 		*stride = intel_get_fence_pitch(intel,
 						ALIGN(pitch, 512),
@@ -768,7 +768,7 @@ free_priv:
 			else
 				height = 32;
 
-			height = ALIGN(pixmap->drawable.height, 2*height);
+			height = ALIGN(pixmap->drawable.height, height);
 			size = intel_get_fence_size(intel, priv->stride * height);
 		} else
 			size = priv->stride * pixmap->drawable.height;
-- 
2.0.1

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Chris Wilson]

On Wed, Sep 10, 2014 at 02:09:07PM -0700, Keith Packard wrote:
>  [PATCH 2/2] Correct BO allocation alignment
> 
> This patch makes UXA and Mesa agree about how buffers are allocated
> for images. Without this, UXA was requiring larger padding, which
> meant that converting some textures into pixmaps using DRI3 would
> fail.

That extra alignment is due to gen2 and early gen3 (if
(!intel->has_relaxed_fencing) covers them). 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Jasper St. Pierre]

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

Why doesn't mesa allocate buffers in the same way for those chips, then?

Do you have any documentation about this?

On Thu, Sep 11, 2014 at 12:37 AM, Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
wrote:

> On Wed, Sep 10, 2014 at 02:09:07PM -0700, Keith Packard wrote:
> >  [PATCH 2/2] Correct BO allocation alignment
> >
> > This patch makes UXA and Mesa agree about how buffers are allocated
> > for images. Without this, UXA was requiring larger padding, which
> > meant that converting some textures into pixmaps using DRI3 would
> > fail.
>
> That extra alignment is due to gen2 and early gen3 (if
> (!intel->has_relaxed_fencing) covers them).
> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>



-- 
  Jasper

[-- Attachment #1.2: Type: text/html, Size: 1792 bytes --]

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

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Chris Wilson]

On Thu, Sep 11, 2014 at 12:47:21AM -0600, Jasper St. Pierre wrote:
>    Why doesn't mesa allocate buffers in the same way for those chips, then?

Good question.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Keith Packard]

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

Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org> writes:

> That extra alignment is due to gen2 and early gen3 (if
> (!intel->has_relaxed_fencing) covers them).

Here's the patch which changed the alignment requirment:

        commit 736b89504a32239a0c7dfb5961c1b8292dd744bd
        Author: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
        Date:   Sun Dec 30 10:32:18 2012 +0000

            uxa: Align surface allocations to even tile rows

            Align surface sizes to an even number of tile rows to cater for sampler
            prefetch. If we read beyond the last page we may catch the PTE in a
            state of flux and trigger a GPU hang. Also detected by enabling invalid
            PTE access checking.

            References: https://bugs.freedesktop.org/show_bug.cgi?id=56916
            References: https://bugs.freedesktop.org/show_bug.cgi?id=55984
            Signed-off-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>

Both of these bugs report regressions found past the 3.6 kernel, one on
965gm and the other on Ironlake. Are there additional bug reports on UXA
which actually relate to this patch as it affects gen2 and gen3
hardware?

Here's the patch that added the additional alignment restriction to SNA:

        commit 1b6c1a30723b1d13e9bd3df0b59a8d75639c89be
        Author: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
        Date:   Fri Nov 30 09:27:57 2012 +0000

            sna: Increase tiling alignment to an even tile

            Seems to help g4x.

            Signed-off-by: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>

Note that this does not reference gen2 or gen3 either.

From the above two patches, all that I can learn is that both of these
larger alignments were introduced to fix bugs on newer hardware, and
that the larger alignment is now specifically disabled on that same
hardware in the SNA code. Reading only this history, I felt reasonably
confident that changing UXA back to what libdrm does was the best plan.

If you have specific bug reports that were resolved by this patch, or
specific hardware documentation which indicates that this patch is
required, especially as it relates to gen2 and gen3 hardware, I'd love
to see them.

In any case, we've now got four versions of the pixmap alignment code
(libdrm, uxa and sna in two varieties). They're all subtly different;
one suspects that each one works on some set of problems and fails on
others...

Here's what the height alignment requirements are:

                libdrm  uxa     uxa     sna     sna
                      +keithp        >=2.6.38 <2.6.38

gen2 none         2      2       2       1       2
gen3 none         2      2       2       1       2
gen4+ none        2      2       2       1       1

gen2 X           16     16      32      16      32
gen3 X            8      8      16       8      16
gen4+ X           8      8      16       8       8

gen2 Y           16     16      32      16      32
i915 Y            8     32      64       8      16
i945 Y           32     32      64       8      16
gen4+ Y          32     32      64      32      32

It looks like the SNA alignment for untiled buffers is incorrect? 965
hardware is documented to read buffers in 2x2 chunks, so a failure to
height align allocations to 2 can result in reads off the end of the
buffer.

For uxa's intel_set_pixmap_bo, and sna's sna_dri3_pixmap_from_fd,
there's a clear requirement that the 2D driver impose no stricter
alignment than libdrm, so that, buffer passing from Mesa to X will work.

For pixmap allocations within the X server, we should ensure that the
alignment requirements are at least as strict as those in libdrm so that
pixmap passing from X to Mesa will work. Not that Mesa actually checks
the provided buffer size at all, although it probably should.x

It seems obvious to me that we should be doing this work in one place
and sharing the code across the 2D and 3D drivers, and yet we never
have.

-- 
keith.packard-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

[-- Attachment #1.2: Type: application/pgp-signature, Size: 810 bytes --]

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

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Intel-gfx] [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Chris Wilson]

On Thu, Sep 11, 2014 at 12:53:30PM -0700, Keith Packard wrote:
> Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org> writes:
> 
> > That extra alignment is due to gen2 and early gen3 (if
> > (!intel->has_relaxed_fencing) covers them).
> 
> Here's the patch which changed the alignment requirment:

[snip commits picked at random]

This is the root

commit d21d781466785c317131a8a57606925867265dc8
Author: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
Date:   Tue Feb 22 18:31:44 2011 +0100

    Fix relaxed tiling on gen2

Later we went on to disable relaxed tiling even after believing we had
fixed all the kernel bugs:

commit 686018f283f1d131073ef5917213e6a8ac013f26
Author: Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org>
Date:   Tue Apr 12 08:23:04 2011 +0100

    Turn relaxed-fencing off by default for older (pre-G33) chipsets
        
I believe the even-tile row alignment is still key to having gen2/gen3
function with relaxed fencing.

> If you have specific bug reports that were resolved by this patch, or
> specific hardware documentation which indicates that this patch is
> required, especially as it relates to gen2 and gen3 hardware, I'd love
> to see them.

Try enabling relaxed fencing again.
 
> In any case, we've now got four versions of the pixmap alignment code
> (libdrm, uxa and sna in two varieties). They're all subtly different;
> one suspects that each one works on some set of problems and fails on
> others...
> 
> Here's what the height alignment requirements are:
> 
>                 libdrm  uxa     uxa     sna     sna
>                       +keithp        >=2.6.38 <2.6.38
> 
> gen2 none         2      2       2       1       2
> gen3 none         2      2       2       1       2
> gen4+ none        2      2       2       1       1
> 
> gen2 X           16     16      32      16      32
> gen3 X            8      8      16       8      16
> gen4+ X           8      8      16       8       8
> 
> gen2 Y           16     16      32      16      32
> i915 Y            8     32      64       8      16
> i945 Y           32     32      64       8      16
> gen4+ Y          32     32      64      32      32
> 
> It looks like the SNA alignment for untiled buffers is incorrect? 965
> hardware is documented to read buffers in 2x2 chunks, so a failure to
> height align allocations to 2 can result in reads off the end of the
> buffer.

Reading from the scratch page is not a problem. Reading from
neighbouring surfaces is of no concern. The allocation must be suitable
and aligned appropriately for writes, but writes themselves are
appropriately clipped. Otherwise one extra row doesn't save you from
scribbling over anywhere in your gtt.
 
> For uxa's intel_set_pixmap_bo, and sna's sna_dri3_pixmap_from_fd,
> there's a clear requirement that the 2D driver impose no stricter
> alignment than libdrm, so that, buffer passing from Mesa to X will work.

No. The clearest requirement is that the ddx (or other display server)
must treat incoming surfaces as tainted and validate them to be sure
that they work with its code paths. If it can't we have a choice of
either rejecting them outright, or staging them.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Intel-gfx] [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Keith Packard]

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

Chris Wilson <chris-Y6uKTt2uX1cEflXRtASbqLVCufUGDwFn@public.gmane.org> writes:

> commit d21d781466785c317131a8a57606925867265dc8
> Author: Daniel Vetter <daniel.vetter-/w4YWyX8dFk@public.gmane.org>
> Date:   Tue Feb 22 18:31:44 2011 +0100
>
>     Fix relaxed tiling on gen2

This one matches libdrm in using 16 for the tile height alignment on
gen2.

> Try enabling relaxed fencing again.

> No. The clearest requirement is that the ddx (or other display server)
> must treat incoming surfaces as tainted and validate them to be sure
> that they work with its code paths. If it can't we have a choice of
> either rejecting them outright, or staging them.

If there's a stricter alignment requirement, then we must fix both the
2D driver and libdrm. Otherwise, the user's session will simply crash at
startup.

However, I still see absolutely no evidence that gen2 requires tile
alignment to 32 rows, or that gen3+ requires tile alignment to 16
rows in any software configuration at all.

-- 
keith.packard-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

[-- Attachment #1.2: Type: application/pgp-signature, Size: 810 bytes --]

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

_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/2 xf86-video-intel] Two DRI3/Present bug fixes for UXA

[Kenneth Graunke]

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

On Wednesday, September 10, 2014 02:09:07 PM Keith Packard wrote:
> Here are a couple of small bug fixes which make DRI3/Present work
> better with UXA.
> 
>  [PATCH 1/2] Do not clear pending kernel events on mode switch
> 
> This patch prevents GL-based compositing managers from wedging when
> performing video mode setting. The problem was that DIX was never
> receiving notification about page flips being completed when one was
> pending across a mode switch.
> 
>  [PATCH 2/2] Correct BO allocation alignment
> 
> This patch makes UXA and Mesa agree about how buffers are allocated
> for images. Without this, UXA was requiring larger padding, which
> meant that converting some textures into pixmaps using DRI3 would
> fail.
> 
> -keith
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Both are:
Tested-by: Kenneth Graunke <kenneth@whitecape.org>

I tested them using DRI3/Present + UXA and DRI3/Present + Glamor on Haswell GT3e.
1. Plug external 2560x1440 DisplayPort monitor into laptop.
2. echo 'exec startkde' > ~/.xinitrc
3. startx
4. xrandr --output DP1 --auto

This used to result in DP1 switching to 2560x1440, but KWin getting stuck waiting on a buffer idle event that never came, so you'd only see a 1920x1080 screen in the top left corner of the display, with either black or white bars in the other area.  Other than the mouse cursor, nothing worked.

With these patches, X works as expected.  Thanks for the fixes, Keith!

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx