Re: [RFC dri3proto v6 01/14] Add modifier/multi-plane requests, bump to v1.1

[Louis-Francis Ratté-Boulianne]

Hi,

On Wed, 2018-02-21 at 11:32 -0800, Keith Packard wrote:
> Louis-Francis Ratté-Boulianne  writes:
> 
> I'll review just the specification today; once we've got that wired,
> I'll go ahead and verify that the encoding matches this spec.
> 
> > diff --git a/dri3proto.txt b/dri3proto.txt
> > index dac11d3..636c789 100644
> > --- a/dri3proto.txt
> > +++ b/dri3proto.txt
> > @@ -1,11 +1,12 @@
> >   The DRI3 Extension
> > -Version 1.0
> > -  2013-6-4
> > +Version 1.1
> > + 2017-06-27
> >
> > Keith Packard
> >   kei...@keithp.com
> >   Intel Corporation
> 
> You should add yourself to the authors list here.

Actually, Daniel is the one who should add his name :)

> > @@ -199,6 +202,125 @@ The name of this extension is "DRI3"
> > associated with a direct rendering device that 'fence' can
> > work with, otherwise a Match error results.
> >  
> > +┌───
> > +DRI3GetSupportedModifiers
> > +   window: WINDOW
> > +   depth: CARD8
> > +   bpp: CARD8
> 
> Hrm. You've uncovered a weird "feature" of the existing DRI3
> protocol. Why does it include both depth and bpp? In the server, each
> depth has a defined bpp for image format purposes, and (currently at
> least), image bpp always matches the pixmap bpp. I'm afraid the
> original
> DRI3 proposal has no clues for us, and I'm afraid the author doesn't
> remember...
> 
> The only thing I can think is that including both allows us to catch
> circumstances where the client guesses wrong about the bpp and
> creates
> an image in the wrong format. Checking a few drivers, I find that
> modesetting uses it to detect this kind of error, but (at least)
> nouveau
> simply ignores it.
> 
> Should you use a visual here instead of depth? While visuals are
> actually allowed to be shared across depths in the core protocol
> specification, we currently rely on them being unique, and in fact
> the
> server implementation has always enforced that.
> 
> I think using a visual would also let you support DeepColor visuals
> and
> gain access to those extended pixel formats. I'm not sure what bpp
> would
> need to be in those cases. Alternatively, we could explicitly allow
> depth to be more than 32 here?
> 
> Using a visual would also allow other visual-dependent modifiers to
> be
> supported; one can imagine that TrueColor and DirectColor having a
> different set of modifiers supported. Of course, anything other than
> TrueColor isn't well supported these days anyways, so that's probably
> less important.

Wouldn't that be weird to have a totally different way of specifying
the format from the old requests. I honestly don't know enough about
DeepColor/HDR to really have an opinion about this though.

> > +  ▶
> > +   num_drawable_modifiers: CARD32
> > +   num_screen_modifiers: CARD32
> > +   drawable_modifiers: ListOfCARD64
> > +   screen_modifiers: ListOfCARD64
> > +└───
> > +   Errors: Window, Match
> > +
> > +   For the Screen associated with 'window', return two lists
> > of
> > +   supported DRM FourCC modifiers, as defined in
> > drm_fourcc.h,
> > +   supported for DRI3 pixmap/buffer interchange. The first
> > list
> > +   'drawable_modifiers' contains modifiers that are specific
> > to
> > +   this window and should be used over the more generic
> > +   'screen_modifiers' set.
> 
> This is somewhat confusing. We use 'window' as a proxy for 'screen'
> in
> many requests, but in this request it doesn't seem to be just a proxy
> as
> you also have a set of modifiers that are specific to it.
> 
> I think what makes sense is to use 'window' only to name the screen,
> and
> then to have the specific modifiers depend on the depth requested,
> not
> the drawable specified. That way you can reliably request information
> about how to create pixmaps without needing to create a temporary
> window
> of the right format before using DRI3PixmapFromBuffers.

Like was mentionned by Daniel on #xorg-devel, we really need to have
the 'drawable' here and can't easily retrieve the drawable-specific
modifiers just from the 'depth'. (We check if the window is a possible
candidate for scanout to avoid useless client buffer reallocation).

We don't explicitely state that in the spec though to be as general as
possible. I guess it wouldn't hurt to add an example though if you
think it would make things clearer.

> > +┌───
> > +DRI3PixmapFromBuffers
> > +   pixmap: PIXMAP
> > +   drawable: DRAWABLE
> > +   num_buffers: CARD8
> > +   width, height: CARD16
> > +   stride0, offset0: CARD32
> > +   stride1, offset1: CARD32
> > +   stride2, offset2: CARD32
> > +   stride3, offset3: CARD32
> 
> Of course, it's weird to have a fixed list of values here. I suspect
> we
> will avoid numerous buffer overflow bugs in applications by doing it
> this way though. Given that KMS only supports 4, I 

[PATCH xserver 2/3] render: Cosmetic cleanup to default format creation

[Adam Jackson]

Signed-off-by: Adam Jackson 
---
 render/picture.c | 70 ++--
 1 file changed, 27 insertions(+), 43 deletions(-)

diff --git a/render/picture.c b/render/picture.c
index 1952785b2..f0638a39f 100644
--- a/render/picture.c
+++ b/render/picture.c
@@ -148,17 +148,17 @@ typedef struct _formatInit {
 CARD8 depth;
 } FormatInitRec, *FormatInitPtr;
 
-static int
-addFormat(FormatInitRec formats[256], int nformat, CARD32 format, CARD8 depth)
+static void
+addFormat(FormatInitRec formats[256], int *nformat, CARD32 format, CARD8 depth)
 {
 int n;
 
-for (n = 0; n < nformat; n++)
+for (n = 0; n < *nformat; n++)
 if (formats[n].format == format && formats[n].depth == depth)
-return nformat;
-formats[nformat].format = format;
-formats[nformat].depth = depth;
-return ++nformat;
+return;
+formats[*nformat].format = format;
+formats[*nformat].depth = depth;
+++*nformat;
 }
 
 #define Mask(n) ((1 << (n)) - 1)
@@ -166,7 +166,7 @@ addFormat(FormatInitRec formats[256], int nformat, CARD32 
format, CARD8 depth)
 static PictFormatPtr
 PictureCreateDefaultFormats(ScreenPtr pScreen, int *nformatp)
 {
-int nformats, f;
+int nformats = 0, f;
 PictFormatPtr pFormats;
 FormatInitRec formats[1024];
 CARD32 format;
@@ -239,18 +239,18 @@ PictureCreateDefaultFormats(ScreenPtr pScreen, int 
*nformatp)
 }
 if (type != PICT_TYPE_OTHER) {
 format = PICT_FORMAT(bpp, type, 0, r, g, b);
-nformats = addFormat(formats, nformats, format, depth);
+addFormat(formats, , format, depth);
 }
 break;
 case StaticColor:
 case PseudoColor:
 format = PICT_VISFORMAT(bpp, PICT_TYPE_COLOR, v);
-nformats = addFormat(formats, nformats, format, depth);
+addFormat(formats, , format, depth);
 break;
 case StaticGray:
 case GrayScale:
 format = PICT_VISFORMAT(bpp, PICT_TYPE_GRAY, v);
-nformats = addFormat(formats, nformats, format, depth);
+addFormat(formats, , format, depth);
 break;
 }
 }
@@ -265,50 +265,34 @@ PictureCreateDefaultFormats(ScreenPtr pScreen, int 
*nformatp)
 case 16:
 /* depth 12 formats */
 if (pDepth->depth >= 12) {
-nformats = addFormat(formats, nformats,
- PICT_x4r4g4b4, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_x4b4g4r4, pDepth->depth);
+addFormat(formats, , PICT_x4r4g4b4, pDepth->depth);
+addFormat(formats, , PICT_x4b4g4r4, pDepth->depth);
 }
 /* depth 15 formats */
 if (pDepth->depth >= 15) {
-nformats = addFormat(formats, nformats,
- PICT_x1r5g5b5, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_x1b5g5r5, pDepth->depth);
+addFormat(formats, , PICT_x1r5g5b5, pDepth->depth);
+addFormat(formats, , PICT_x1b5g5r5, pDepth->depth);
 }
 /* depth 16 formats */
 if (pDepth->depth >= 16) {
-nformats = addFormat(formats, nformats,
- PICT_a1r5g5b5, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_a1b5g5r5, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_r5g6b5, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_b5g6r5, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_a4r4g4b4, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_a4b4g4r4, pDepth->depth);
+addFormat(formats, , PICT_a1r5g5b5, pDepth->depth);
+addFormat(formats, , PICT_a1b5g5r5, pDepth->depth);
+addFormat(formats, , PICT_r5g6b5, pDepth->depth);
+addFormat(formats, , PICT_b5g6r5, pDepth->depth);
+addFormat(formats, , PICT_a4r4g4b4, pDepth->depth);
+addFormat(formats, , PICT_a4b4g4r4, pDepth->depth);
 }
 break;
 case 32:
 if (pDepth->depth >= 24) {
-nformats = addFormat(formats, nformats,
- PICT_x8r8g8b8, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_x8b8g8r8, pDepth->depth);
+addFormat(formats, , PICT_x8r8g8b8, pDepth->depth);
+

[PATCH xserver 1/3] render: Remove some 24bpp handling

[Adam Jackson]

This can't happen anymore.

Signed-off-by: Adam Jackson 
---
 render/picture.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/render/picture.c b/render/picture.c
index 9e4036e7d..1952785b2 100644
--- a/render/picture.c
+++ b/render/picture.c
@@ -293,14 +293,6 @@ PictureCreateDefaultFormats(ScreenPtr pScreen, int 
*nformatp)
  PICT_a4b4g4r4, pDepth->depth);
 }
 break;
-case 24:
-if (pDepth->depth >= 24) {
-nformats = addFormat(formats, nformats,
- PICT_r8g8b8, pDepth->depth);
-nformats = addFormat(formats, nformats,
- PICT_b8g8r8, pDepth->depth);
-}
-break;
 case 32:
 if (pDepth->depth >= 24) {
 nformats = addFormat(formats, nformats,
-- 
2.14.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 0/3] Render format list cleanups

[Adam Jackson]

Testing Mario's depth-30 series revealed some non-glamor bugs. I'm a
little nervous about 3/3 removing the 'depth 32' x8r8g8b8 format as
there could easily be code relying on it existing, but at least cairo
seems to get this right.

- ajax

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 3/3] render: Fix default picture format initialization

[Adam Jackson]

"depth" for a picture format is the sum of bits of a/r/g/b, and not x.
The default format list was creating an x8r8g8b8 format at depth 32,
which is wrong. Likewise, servers supporting depth 30 would get an
x8r8g8b8 format at depth 30, which is nonsense.

Signed-off-by: Adam Jackson 
---
 render/picture.c | 49 +
 1 file changed, 21 insertions(+), 28 deletions(-)

diff --git a/render/picture.c b/render/picture.c
index f0638a39f..86d61e401 100644
--- a/render/picture.c
+++ b/render/picture.c
@@ -196,13 +196,13 @@ PictureCreateDefaultFormats(ScreenPtr pScreen, int 
*nformatp)
 formats[nformats].depth = 32;
 nformats++;
 formats[nformats].format = PICT_x8r8g8b8;
-formats[nformats].depth = 32;
+formats[nformats].depth = 24;
 nformats++;
 formats[nformats].format = PICT_b8g8r8a8;
 formats[nformats].depth = 32;
 nformats++;
 formats[nformats].format = PICT_b8g8r8x8;
-formats[nformats].depth = 32;
+formats[nformats].depth = 24;
 nformats++;
 
 /* now look through the depths and visuals adding other formats */
@@ -264,36 +264,29 @@ PictureCreateDefaultFormats(ScreenPtr pScreen, int 
*nformatp)
 switch (bpp) {
 case 16:
 /* depth 12 formats */
-if (pDepth->depth >= 12) {
-addFormat(formats, , PICT_x4r4g4b4, pDepth->depth);
-addFormat(formats, , PICT_x4b4g4r4, pDepth->depth);
-}
+addFormat(formats, , PICT_x4r4g4b4, 12);
+addFormat(formats, , PICT_x4b4g4r4, 12);
 /* depth 15 formats */
-if (pDepth->depth >= 15) {
-addFormat(formats, , PICT_x1r5g5b5, pDepth->depth);
-addFormat(formats, , PICT_x1b5g5r5, pDepth->depth);
-}
+addFormat(formats, , PICT_x1r5g5b5, 15);
+addFormat(formats, , PICT_x1b5g5r5, 15);
 /* depth 16 formats */
-if (pDepth->depth >= 16) {
-addFormat(formats, , PICT_a1r5g5b5, pDepth->depth);
-addFormat(formats, , PICT_a1b5g5r5, pDepth->depth);
-addFormat(formats, , PICT_r5g6b5, pDepth->depth);
-addFormat(formats, , PICT_b5g6r5, pDepth->depth);
-addFormat(formats, , PICT_a4r4g4b4, pDepth->depth);
-addFormat(formats, , PICT_a4b4g4r4, pDepth->depth);
-}
+addFormat(formats, , PICT_a1r5g5b5, 16);
+addFormat(formats, , PICT_a1b5g5r5, 16);
+addFormat(formats, , PICT_r5g6b5, 16);
+addFormat(formats, , PICT_b5g6r5, 16);
+addFormat(formats, , PICT_a4r4g4b4, 16);
+addFormat(formats, , PICT_a4b4g4r4, 16);
 break;
 case 32:
-if (pDepth->depth >= 24) {
-addFormat(formats, , PICT_x8r8g8b8, pDepth->depth);
-addFormat(formats, , PICT_x8b8g8r8, pDepth->depth);
-}
-if (pDepth->depth >= 30) {
-addFormat(formats, , PICT_a2r10g10b10, pDepth->depth);
-addFormat(formats, , PICT_x2r10g10b10, pDepth->depth);
-addFormat(formats, , PICT_a2b10g10r10, pDepth->depth);
-addFormat(formats, , PICT_x2b10g10r10, pDepth->depth);
-}
+/* depth 24 formats */
+addFormat(formats, , PICT_x8r8g8b8, 24);
+addFormat(formats, , PICT_x8b8g8r8, 24);
+/* depth 30 formats */
+addFormat(formats, , PICT_x2r10g10b10, 30);
+addFormat(formats, , PICT_x2b10g10r10, 30);
+/* depth 32 formats */
+addFormat(formats, , PICT_a2r10g10b10, 32);
+addFormat(formats, , PICT_a2b10g10r10, 32);
 break;
 }
 }
-- 
2.14.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

RE: [PATCH xserver 3/3] render: Fix default picture format initialization

[Peter Harris]

> "depth" for a picture format is the sum of bits of a/r/g/b, and not x.
> The default format list was creating an x8r8g8b8 format at depth 32,
> which is wrong. Likewise, servers supporting depth 30 would get an
> x8r8g8b8 format at depth 30, which is nonsense.

>  formats[nformats].format = PICT_x8r8g8b8;
> -formats[nformats].depth = 32;
> +formats[nformats].depth = 24;

The RENDER extension forces a depth 32 pixmap format. It does not force depth 
24. So with a 16bpp root window, there may not be a depth 24 pixmap format, and 
this picture format would not be usable at all (BadMatch with every possible 
drawable). Also, d32 x8r8g8b8 is used in the wild (eg. it looks like gtk2 
sometimes uses it to avoid manually filling the alpha channel with 0xFF). I 
suspect we want to keep this one as-is.

A "depth 30 x8r8g8b8" probably isn't useful most of the time, but maybe it 
makes sense for PutImage-then-convert-in-the-server an x8r8g8b8 image to 
x2r10g10b10 without needing a second (not-depth-30) pixmap?

Peter Harris
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] composite: Fix use-after-free in compReparentWindow

[Peter Harris]

If an implicitly redirected window is unredirected by the reparent
operation, cw will be a stale pointer.

Signed-off-by: Peter Harris 
---
 composite/compwindow.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/composite/compwindow.c b/composite/compwindow.c
index e74ce661a..54b4e6ac4 100644
--- a/composite/compwindow.c
+++ b/composite/compwindow.c
@@ -432,7 +432,7 @@ compReparentWindow(WindowPtr pWin, WindowPtr pPriorParent)
 {
 ScreenPtr pScreen = pWin->drawable.pScreen;
 CompScreenPtr cs = GetCompScreen(pScreen);
-CompWindowPtr cw = GetCompWindow(pWin);
+CompWindowPtr cw;
 
 pScreen->ReparentWindow = cs->ReparentWindow;
 /*
@@ -471,6 +471,7 @@ compReparentWindow(WindowPtr pWin, WindowPtr pPriorParent)
 cs->ReparentWindow = pScreen->ReparentWindow;
 pScreen->ReparentWindow = compReparentWindow;
 
+cw = GetCompWindow(pWin);
 if (pWin->damagedDescendants || (cw && cw->damaged))
 compMarkAncestors(pWin);
 
-- 
2.14.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 2/3] render: Cosmetic cleanup to default format creation

[Keith Packard]

Adam Jackson  writes:

> Signed-off-by: Adam Jackson 

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/3] render: Remove some 24bpp handling

[Keith Packard]

Adam Jackson  writes:

> This can't happen anymore.
>
> Signed-off-by: Adam Jackson 

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/3] render: Fix default picture format initialization

[Keith Packard]

Adam Jackson  writes:

> "depth" for a picture format is the sum of bits of a/r/g/b, and not x.
> The default format list was creating an x8r8g8b8 format at depth 32,
> which is wrong. Likewise, servers supporting depth 30 would get an
> x8r8g8b8 format at depth 30, which is nonsense.

Hrm. It's not total nonsense, it's just not obviously the right idea.
You *can* draw r8g8b8 to a depth 30 object, but you probably meant to
use r10g10b10 instead.

I wonder if we should be adding both depth 32 and depth 24 888 options
at the top of this function? I fear losing a format that existing
applications are using. If we added both, I'd be less concerned about
compatibility.

>  switch (bpp) {
>  case 16:
>  /* depth 12 formats */
> -if (pDepth->depth >= 12) {
> -addFormat(formats, , PICT_x4r4g4b4, pDepth->depth);
> -addFormat(formats, , PICT_x4b4g4r4, pDepth->depth);
> -}
> +addFormat(formats, , PICT_x4r4g4b4, 12);
> +addFormat(formats, , PICT_x4b4g4r4, 12);
>  /* depth 15 formats */
> -if (pDepth->depth >= 15) {
> -addFormat(formats, , PICT_x1r5g5b5, pDepth->depth);
> -addFormat(formats, , PICT_x1b5g5r5, pDepth->depth);
> -}
> +addFormat(formats, , PICT_x1r5g5b5, 15);
> +addFormat(formats, , PICT_x1b5g5r5, 15);
>  /* depth 16 formats */
> -if (pDepth->depth >= 16) {
> -addFormat(formats, , PICT_a1r5g5b5, pDepth->depth);
> -addFormat(formats, , PICT_a1b5g5r5, pDepth->depth);
> -addFormat(formats, , PICT_r5g6b5, pDepth->depth);
> -addFormat(formats, , PICT_b5g6r5, pDepth->depth);
> -addFormat(formats, , PICT_a4r4g4b4, pDepth->depth);
> -addFormat(formats, , PICT_a4b4g4r4, pDepth->depth);
> -}
> +addFormat(formats, , PICT_a1r5g5b5, 16);
> +addFormat(formats, , PICT_a1b5g5r5, 16);
> +addFormat(formats, , PICT_r5g6b5, 16);
> +addFormat(formats, , PICT_b5g6r5, 16);
> +addFormat(formats, , PICT_a4r4g4b4, 16);
> +addFormat(formats, , PICT_a4b4g4r4, 16);

You can't just blindly add formats that the driver might not
support. Depth really means the bits supported by the driver; bits
outside of depth may not be present in the hardware. If you have a
driver which also supports depth 16 but only advertises depth 12, that
driver has a bug. Same comment on the 32bpp changes.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] composite: Fix use-after-free in compReparentWindow

[Keith Packard]

Peter Harris  writes:

> If an implicitly redirected window is unredirected by the reparent
> operation, cw will be a stale pointer.
>
> Signed-off-by: Peter Harris 

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel