Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Daniel Stone]

Hi,

On 9 October 2017 at 16:29, Marek Olšák  wrote:
> On Mon, Oct 9, 2017 at 5:22 PM, Daniel Stone  wrote:
>> Which is exactly the problem validateUsage() is supposed to solve. All
>> we can do is rely on gbm_bo_import failing (which is best, as it can
>> never be used for scanout), or at least the KMS commit fail (worse,
>> especially pre-atomic, but still beats displaying garbage?). We don't
>> need magic blits, or untiled views, or anything: just failing is fine
>> and we'll fall back to composition.
>
> I prefer page flipping over composition. With X/DRI, the DRI state
> tracker (or maybe libGL/libEGL) always sets the scanout flag.
>
> Starting with our Raven APU, the scanout and non-scanout tiling mode
> is the same for 32bpp, so there won't be any compatibility issue.

Oh totally, I prefer it as well. I'm just saying that 'Wayland' is
stuck here: without validateUsage() actually validating usage (ditto
the KMS driver) on older generations, we don't really have a way to
ever attempt pageflipping without it showing garbage sometimes. To
expand: pageflipping > composition > garbage on screen. :)

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Marek Olšák]

On Mon, Oct 9, 2017 at 5:22 PM, Daniel Stone  wrote:
> Hi Marek,
>
> On 4 October 2017 at 12:38, Marek Olšák  wrote:
>> Yeah I don't have an answer to that. I think it's better to pageflip than
>> blit. Can Wayland reallocate the buffer to make it displayable? If not, it
>> may be better to create displayable buffers always.
>
> Not sure what you mean by 'Wayland reallocate'? The buffers are
> (DRI3-style) allocated by the client's EGLSurface, with no
> intermediate server-allocated storage at any point There is no
> explicit hint to the client whether it is being scanned out or not.
> Even if there was, there is no indication of which device the buffer
> might be scanned out on - a device the client might not have access to
> (e.g. a display controller with no render nodes). I've mostly been
> waiting for the allocator to come around before trying to deal with
> this. But even with that, we'd still require the tiling information to
> actually be exposed externally (i.e. through modifiers); currently the
> compositor can't introspect the tiling flags at all, so it has no idea
> that it shouldn't try to scan this particular buffer out.
>
> Which is exactly the problem validateUsage() is supposed to solve. All
> we can do is rely on gbm_bo_import failing (which is best, as it can
> never be used for scanout), or at least the KMS commit fail (worse,
> especially pre-atomic, but still beats displaying garbage?). We don't
> need magic blits, or untiled views, or anything: just failing is fine
> and we'll fall back to composition.

I prefer page flipping over composition. With X/DRI, the DRI state
tracker (or maybe libGL/libEGL) always sets the scanout flag.

Starting with our Raven APU, the scanout and non-scanout tiling mode
is the same for 32bpp, so there won't be any compatibility issue.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Daniel Stone]

Hi Marek,

On 4 October 2017 at 12:38, Marek Olšák  wrote:
> Yeah I don't have an answer to that. I think it's better to pageflip than
> blit. Can Wayland reallocate the buffer to make it displayable? If not, it
> may be better to create displayable buffers always.

Not sure what you mean by 'Wayland reallocate'? The buffers are
(DRI3-style) allocated by the client's EGLSurface, with no
intermediate server-allocated storage at any point There is no
explicit hint to the client whether it is being scanned out or not.
Even if there was, there is no indication of which device the buffer
might be scanned out on - a device the client might not have access to
(e.g. a display controller with no render nodes). I've mostly been
waiting for the allocator to come around before trying to deal with
this. But even with that, we'd still require the tiling information to
actually be exposed externally (i.e. through modifiers); currently the
compositor can't introspect the tiling flags at all, so it has no idea
that it shouldn't try to scan this particular buffer out.

Which is exactly the problem validateUsage() is supposed to solve. All
we can do is rely on gbm_bo_import failing (which is best, as it can
never be used for scanout), or at least the KMS commit fail (worse,
especially pre-atomic, but still beats displaying garbage?). We don't
need magic blits, or untiled views, or anything: just failing is fine
and we'll fall back to composition.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Marek Olšák]

Yeah I don't have an answer to that. I think it's better to pageflip than
blit. Can Wayland reallocate the buffer to make it displayable? If not, it
may be better to create displayable buffers always.

Marek

On Oct 4, 2017 12:48 PM, "Daniel Stone"  wrote:

> Hi Marek,
>
> On 3 October 2017 at 17:00, Marek Olšák  wrote:
> > On Mon, Oct 2, 2017 at 8:09 PM, Daniel Stone 
> wrote:
> >> Perhaps unsurprisingly, adding __DRI_IMAGE_USE_SCANOUT to
> >> src/egl/drivers/dri2/platform_wayland.c in the createImage() fallback
> >> path (i.e. not createImageWithModifiers) fixes things. That being
> >> said, Weston does do the GBM BO import with the scanout flag, which
> >> will call the DRIImage's validateUsage() hook with the SCANOUT bit
> >> set; for now, it should be enough to just detect that the image is not
> >> scanout-compatible in radeonsi's validateUsage() hook, rejecting the
> >> import which will make Weston fall back to GLES composition.
> >>
> >> That being said, st/dri's dri2_validate_usage() doesn't really fill me
> >> with too much confidence.
> >
> > We don't really validate flags during DMABUF import. The idea is that
> > all clients should know the flags since buffer creation, and have no
> > reason to pass different flags during the import.
>
> In that case, I guess the only option is to just make Gallium's dri2
> st pass PIPE_USAGE_SCANOUT to every winsys allocation. This pessimises
> allocation and disqualifies tiling/etc modes for buffers which can
> never be scanout, but if there can be no validation (and thus we can't
> reject a compositor which wishes to start using a client buffer for
> scanout), then it's the only other option. Either that or just fail
> every gbm_bo_import for scanout on Gallium.
>
> Cheers,
> Daniel
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Daniel Stone]

Hi Marek,

On 3 October 2017 at 17:00, Marek Olšák  wrote:
> On Mon, Oct 2, 2017 at 8:09 PM, Daniel Stone  wrote:
>> Perhaps unsurprisingly, adding __DRI_IMAGE_USE_SCANOUT to
>> src/egl/drivers/dri2/platform_wayland.c in the createImage() fallback
>> path (i.e. not createImageWithModifiers) fixes things. That being
>> said, Weston does do the GBM BO import with the scanout flag, which
>> will call the DRIImage's validateUsage() hook with the SCANOUT bit
>> set; for now, it should be enough to just detect that the image is not
>> scanout-compatible in radeonsi's validateUsage() hook, rejecting the
>> import which will make Weston fall back to GLES composition.
>>
>> That being said, st/dri's dri2_validate_usage() doesn't really fill me
>> with too much confidence.
>
> We don't really validate flags during DMABUF import. The idea is that
> all clients should know the flags since buffer creation, and have no
> reason to pass different flags during the import.

In that case, I guess the only option is to just make Gallium's dri2
st pass PIPE_USAGE_SCANOUT to every winsys allocation. This pessimises
allocation and disqualifies tiling/etc modes for buffers which can
never be scanout, but if there can be no validation (and thus we can't
reject a compositor which wishes to start using a client buffer for
scanout), then it's the only other option. Either that or just fail
every gbm_bo_import for scanout on Gallium.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Marek Olšák]

On Mon, Oct 2, 2017 at 8:09 PM, Daniel Stone  wrote:
> On 2 October 2017 at 18:34, Daniel Stone  wrote:
>> Marek, Michel, any ideas? Looks like the winsys buffers we create lose
>> all their tiling information between client and KMS. You can test it
>> just by running 'weston-simple-egl -f', with whatever version of
>> Weston, and the two patches in this thread.
>
> Perhaps unsurprisingly, adding __DRI_IMAGE_USE_SCANOUT to
> src/egl/drivers/dri2/platform_wayland.c in the createImage() fallback
> path (i.e. not createImageWithModifiers) fixes things. That being
> said, Weston does do the GBM BO import with the scanout flag, which
> will call the DRIImage's validateUsage() hook with the SCANOUT bit
> set; for now, it should be enough to just detect that the image is not
> scanout-compatible in radeonsi's validateUsage() hook, rejecting the
> import which will make Weston fall back to GLES composition.
>
> That being said, st/dri's dri2_validate_usage() doesn't really fill me
> with too much confidence.

We don't really validate flags during DMABUF import. The idea is that
all clients should know the flags since buffer creation, and have no
reason to pass different flags during the import.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Daniel Stone]

On 2 October 2017 at 18:34, Daniel Stone  wrote:
> Marek, Michel, any ideas? Looks like the winsys buffers we create lose
> all their tiling information between client and KMS. You can test it
> just by running 'weston-simple-egl -f', with whatever version of
> Weston, and the two patches in this thread.

Perhaps unsurprisingly, adding __DRI_IMAGE_USE_SCANOUT to
src/egl/drivers/dri2/platform_wayland.c in the createImage() fallback
path (i.e. not createImageWithModifiers) fixes things. That being
said, Weston does do the GBM BO import with the scanout flag, which
will call the DRIImage's validateUsage() hook with the SCANOUT bit
set; for now, it should be enough to just detect that the image is not
scanout-compatible in radeonsi's validateUsage() hook, rejecting the
import which will make Weston fall back to GLES composition.

That being said, st/dri's dri2_validate_usage() doesn't really fill me
with too much confidence.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Daniel Stone]

Hi Andy,

On 2 October 2017 at 17:51, Andy Furniss  wrote:
> These are still a regression for me over git revert a65db0ad1c
>
> weston-simple-egl works as does mpv opengl windowed.
>
> The issue is that kodi or fullscreen mpv is mostly junk, IIRC from an old
> bug this is to do with direct scan out and tiling.

This is a Radeon driver bug which looks to have been there for at
least quite a while now?

Weston (in its current upstream form) doesn't import dmabufs for
scanout. Without Marek's patch, everything ends up using the dmabuf
interface, which succeeds when it shouldn't, and then because Weston
won't import the dmabuf for scanout, it uses the GPU instead and this
works just fine. With Marek's patch, Weston fails to import the
buffer. With Marek's and my patch, we go back to what we did up until
a couple of months ago, using the old wl_drm interface. That can then
be used for direct scanout, and is.

Marek, Michel, any ideas? Looks like the winsys buffers we create lose
all their tiling information between client and KMS. You can test it
just by running 'weston-simple-egl -f', with whatever version of
Weston, and the two patches in this thread.

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2 1/2] egl/wayland: Check queryImage return for wl_buffer

[Andy Furniss]

These are still a regression for me over git revert a65db0ad1c

weston-simple-egl works as does mpv opengl windowed.

The issue is that kodi or fullscreen mpv is mostly junk, IIRC from an 
old bug this is to do with direct scan out and tiling.


Daniel Stone wrote:

When creating a wl_buffer from a DRIImage, we extract all the DRIImage
information via queryImage. Check whether or not it actually succeeds,
either bailing out if the query was critical, or providing sensible
fallbacks for information which was not available in older DRIImage
versions.

Fixes: a65db0ad1c ("st/dri: don't expose modifiers in EGL if the driver doesn't 
implement them")
Fixes: 02cc359372 ("egl/wayland: Use linux-dmabuf interface for buffers")
Reported-by: Andy Furniss 
Cc: Marek Olšák 
Signed-off-by: Daniel Stone 
---
  src/egl/drivers/dri2/platform_wayland.c | 56 -
  1 file changed, 41 insertions(+), 15 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_wayland.c 
b/src/egl/drivers/dri2/platform_wayland.c
index 011dddfabf..04c04cc304 100644
--- a/src/egl/drivers/dri2/platform_wayland.c
+++ b/src/egl/drivers/dri2/platform_wayland.c
@@ -678,23 +678,37 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
   __DRIimage *image)
  {
 struct wl_buffer *ret;
+   EGLBoolean query;
 int width, height, fourcc, num_planes;
  
-   dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_WIDTH, );

-   dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_HEIGHT, );
-   dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FOURCC, );
-   dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_NUM_PLANES,
-   _planes);
+   query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_WIDTH, 
);
+   query &= dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_HEIGHT,
+);
+   query &= dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_FOURCC,
+);
+   if (!query)
+  return NULL;
+
+   query = dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_NUM_PLANES,
+   _planes);
+   if (!query)
+  num_planes = 1;
  
 if (dri2_dpy->wl_dmabuf && dri2_dpy->image->base.version >= 15) {

struct zwp_linux_buffer_params_v1 *params;
int mod_hi, mod_lo;
int i;
  
-  dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_MODIFIER_UPPER,

-  _hi);
-  dri2_dpy->image->queryImage(image, __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
-  _lo);
+  query = dri2_dpy->image->queryImage(image,
+  __DRI_IMAGE_ATTRIB_MODIFIER_UPPER,
+  _hi);
+  query &= dri2_dpy->image->queryImage(image,
+   __DRI_IMAGE_ATTRIB_MODIFIER_LOWER,
+   _lo);
+  if (!query) {
+ mod_hi = DRM_FORMAT_MOD_INVALID >> 32;
+ mod_lo = DRM_FORMAT_MOD_INVALID & 0x;
+  }
  
/* We don't need a wrapper for wl_dmabuf objects, because we have to

 * create the intermediate params object; we can set the queue on this,
@@ -705,7 +719,8 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
  
for (i = 0; i < num_planes; i++) {

   __DRIimage *p_image;
- int stride, offset, fd;
+ int stride, offset;
+ int fd = -1;
  
   if (i == 0)

  p_image = image;
@@ -716,14 +731,25 @@ create_wl_buffer(struct dri2_egl_display *dri2_dpy,
  return NULL;
   }
  
- dri2_dpy->image->queryImage(p_image, __DRI_IMAGE_ATTRIB_FD, );

- dri2_dpy->image->queryImage(p_image, __DRI_IMAGE_ATTRIB_STRIDE,
- );
- dri2_dpy->image->queryImage(p_image, __DRI_IMAGE_ATTRIB_OFFSET,
- );
+ query = dri2_dpy->image->queryImage(p_image,
+ __DRI_IMAGE_ATTRIB_FD,
+ );
+ query &= dri2_dpy->image->queryImage(p_image,
+  __DRI_IMAGE_ATTRIB_STRIDE,
+  );
+ query &= dri2_dpy->image->queryImage(p_image,
+  __DRI_IMAGE_ATTRIB_OFFSET,
+  );
   if (image != p_image)
  dri2_dpy->image->destroyImage(p_image);
  
+ if (!query) {

+if (fd >= 0)
+   close(fd);
+zwp_linux_buffer_params_v1_destroy(params);
+return NULL;
+ }
+
   zwp_linux_buffer_params_v1_add(params, fd, i, offset, stride,
  mod_hi, mod_lo);