RE: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Fabien DESSENNE]

Hi,
> -Original Message-
> From: Daniel Stone [mailto:dan...@fooishbar.org]
> Sent: lundi 2 novembre 2015 12:44
> To: Fabien DESSENNE
> Cc: Bryce Harrington; Giulio Camuffo; Vincent ABRIOU; Benjamin Gaignard;
> wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH] dmabuf: get supported dmabuf formats from
> compositor backend
> 
> Hi,
> 
> On 2 November 2015 at 09:39, Fabien DESSENNE <fabien.desse...@st.com>
> wrote:
> >> On 30 October 2015 at 00:27, Bryce Harrington
> <br...@osg.samsung.com>
> >> wrote:
> >> > I'd like to better understand what this is going to be used for,
> >> > before landing it.  Another R-b on this would be nice as well;
> >> > Giulio perhaps you could give this patch a review?
> >>
> >> Hm, to be honest I'd prefer this not land just now, or like this.
> >>
> >> dmabuf usage in compositor-drm is just an optimisation, where the
> >> fallback path is through gl-renderer. Anything gl-renderer doesn't
> >> support, we essentially can't display.
> >
> > Maybe this is not always true : I use a forked version of
> > compositor-drm (I plan to upstream some patches later) where the
> > dmabuf buffers are displayed by the DRM planes: these buffers are not
> used by gl-renderer.
> >
> > This is a typical optimization use case: video playback frames being
> > sent as dmabuf buffer to the compositor, and rendered without any copy
> > through a DRM plane.
> 
> Yeah, of course. In your confined usecase this will always be true, but in
> general we cannot rely on it being true: we will always need a fallback to gl-
> renderer. Since gl-renderer is the fallback, we need to limit the exposed
> formats to what it can display, for the general case.
> 
> > Now, back to the proposed patch: in my proposal building the list of
> > dmabuf formats is delegated to the backend, not built by the
> > compositor itself.
> > Not a big change and it does not solve the "EGL dmabuf formats" issue
> > but there are two improvements:
> > 1/ IMHO the supported formats are backend-dependent, so this patch
> > makes some sense here.
> > 2/ Depending on the backend implementation, the list of formats may be
> > successfully built: this is the case with the compositor I use as it
> > does not use gl-renderer for dmabuf buffers.
> 
> I'd say more renderer-dependent than backend-dependent, as below.
> (Again, talking about the _general_ upstream case, not the specific usecase
> you have in a closed - in the engineering rather than freedom sense -
> system.)
> 
> >> Those formats are what we need to send to the compositor (possibly
> >> with the intersection between gl-renderer and compositor-drm marked
> >> as preferred), but right now, as the comment notes, we lack a way to
> >> query this through EGL. This is being worked on though.
> >
> > Good news! Do you know if there is any schedule for this?
> 
> Unfortunately Khronos work is necessarily opaque, but it is being actively
> worked on and is a relatively straightforward change, so.
> 
> > By the way, I think that my proposed patch and this EGL future patch
> > are not incompatible: as explained, EGL is not the only dmabuf consumer:
> > DRM plane is another consumer. At the end the format list shall be
> > built by the backend, not by the compositor.
> 
> Yes, but the problem is that while there are a million different ways we can
> require EGL but not KMS (recording/screenshot, view transformation,
> partially-occluded buffer, etc etc), and if you end up in a situation where 
> you
> both require EGL, but also have a buffer you cannot display through EGL, the
> result is not pretty.
> 
> I don't think your patch is in any way wrong for your usecase, but
> unfortunately it is very much unsuitable for upstream, as it does break
> everything listed above.
> 
> My preference, as stated, would be to generate a list of formats/modifiers
> from the renderer (in practical terms, always EGL), then pass this list to the
> backend for filtering, which would essentially just be adding 'preferred' 
> flags
> to formats which could be pulled out to hardware planes.
> 
> Cheers,
> Daniel

OK, so let's wait for the EGL update and see how we can implement all of this.
My proposed patch can be abandoned.
BR,
Fabien
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

RE: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Fabien DESSENNE]

Hi,

> -Original Message-
> From: Daniel Stone [mailto:dan...@fooishbar.org]
> Sent: vendredi 30 octobre 2015 09:31
> To: Bryce Harrington
> Cc: Fabien DESSENNE; Giulio Camuffo; Vincent ABRIOU; Benjamin Gaignard;
> wayland-devel@lists.freedesktop.org
> Subject: Re: [PATCH] dmabuf: get supported dmabuf formats from
> compositor backend
> 
> Hi,
> 
> On 30 October 2015 at 00:27, Bryce Harrington <br...@osg.samsung.com>
> wrote:
> > On Tue, Oct 20, 2015 at 08:45:50AM -0700, Bryce Harrington wrote:
> >> On Tue, Oct 20, 2015 at 04:54:30PM +0200, Fabien Dessenne wrote:
> >> > Add the possibility for the compositor backend to provide with the
> >> > list of supported pixel formats for dmabuf-based buffers.
> >> > This information is used by linux_dmabuf to inform clients when
> binding.
> >> >
> >> > Signed-off-by: Fabien Dessenne <fabien.desse...@st.com>
> >>
> >> Reviewed-by <br...@osg.samsung.com>
> >>
> >> There's not a way to verify the list is getting sent across to the
> >> client is there?
> >
> > Also, patch no longer applies since e3c0d8af.
> >
> > I'd like to better understand what this is going to be used for,
> > before landing it.  Another R-b on this would be nice as well; Giulio
> > perhaps you could give this patch a review?
> 
> Hm, to be honest I'd prefer this not land just now, or like this.
> 
> dmabuf usage in compositor-drm is just an optimisation, where the fallback
> path is through gl-renderer. Anything gl-renderer doesn't support, we
> essentially can't display.

Maybe this is not always true : I use a forked version of compositor-drm
(I plan to upstream some patches later) where the dmabuf buffers are
displayed by the DRM planes: these buffers are not used by gl-renderer.

This is a typical optimization use case: video playback frames being sent
as dmabuf buffer to the compositor, and rendered without any copy
through a DRM plane.

Now, back to the proposed patch: in my proposal building the list of
dmabuf formats is delegated to the backend, not built by the compositor
itself.
Not a big change and it does not solve the "EGL dmabuf formats" issue
but there are two improvements:
1/ IMHO the supported formats are backend-dependent, so this patch
makes some sense here.
2/ Depending on the backend implementation, the list of formats may
be successfully built: this is the case with the compositor I use as it does
not use gl-renderer for dmabuf buffers.

> Those formats are what we need to send to the
> compositor (possibly with the intersection between gl-renderer and
> compositor-drm marked as preferred), but right now, as the comment notes,
> we lack a way to query this through EGL. This is being worked on though.

Good news! Do you know if there is any schedule for this?

By the way, I think that my proposed patch and this EGL future patch
are not incompatible: as explained, EGL is not the only dmabuf consumer:
DRM plane is another consumer. At the end the format list shall be built
by the backend, not by the compositor.

Please, let me know your feedback.

Fabien

> 
> Cheers,
> Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Daniel Stone]

Hi,

On 2 November 2015 at 09:39, Fabien DESSENNE  wrote:
>> On 30 October 2015 at 00:27, Bryce Harrington 
>> wrote:
>> > I'd like to better understand what this is going to be used for,
>> > before landing it.  Another R-b on this would be nice as well; Giulio
>> > perhaps you could give this patch a review?
>>
>> Hm, to be honest I'd prefer this not land just now, or like this.
>>
>> dmabuf usage in compositor-drm is just an optimisation, where the fallback
>> path is through gl-renderer. Anything gl-renderer doesn't support, we
>> essentially can't display.
>
> Maybe this is not always true : I use a forked version of compositor-drm
> (I plan to upstream some patches later) where the dmabuf buffers are
> displayed by the DRM planes: these buffers are not used by gl-renderer.
>
> This is a typical optimization use case: video playback frames being sent
> as dmabuf buffer to the compositor, and rendered without any copy
> through a DRM plane.

Yeah, of course. In your confined usecase this will always be true,
but in general we cannot rely on it being true: we will always need a
fallback to gl-renderer. Since gl-renderer is the fallback, we need to
limit the exposed formats to what it can display, for the general
case.

> Now, back to the proposed patch: in my proposal building the list of
> dmabuf formats is delegated to the backend, not built by the compositor
> itself.
> Not a big change and it does not solve the "EGL dmabuf formats" issue
> but there are two improvements:
> 1/ IMHO the supported formats are backend-dependent, so this patch
> makes some sense here.
> 2/ Depending on the backend implementation, the list of formats may
> be successfully built: this is the case with the compositor I use as it does
> not use gl-renderer for dmabuf buffers.

I'd say more renderer-dependent than backend-dependent, as below.
(Again, talking about the _general_ upstream case, not the specific
usecase you have in a closed - in the engineering rather than freedom
sense - system.)

>> Those formats are what we need to send to the
>> compositor (possibly with the intersection between gl-renderer and
>> compositor-drm marked as preferred), but right now, as the comment notes,
>> we lack a way to query this through EGL. This is being worked on though.
>
> Good news! Do you know if there is any schedule for this?

Unfortunately Khronos work is necessarily opaque, but it is being
actively worked on and is a relatively straightforward change, so.

> By the way, I think that my proposed patch and this EGL future patch
> are not incompatible: as explained, EGL is not the only dmabuf consumer:
> DRM plane is another consumer. At the end the format list shall be built
> by the backend, not by the compositor.

Yes, but the problem is that while there are a million different ways
we can require EGL but not KMS (recording/screenshot, view
transformation, partially-occluded buffer, etc etc), and if you end up
in a situation where you both require EGL, but also have a buffer you
cannot display through EGL, the result is not pretty.

I don't think your patch is in any way wrong for your usecase, but
unfortunately it is very much unsuitable for upstream, as it does
break everything listed above.

My preference, as stated, would be to generate a list of
formats/modifiers from the renderer (in practical terms, always EGL),
then pass this list to the backend for filtering, which would
essentially just be adding 'preferred' flags to formats which could be
pulled out to hardware planes.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Daniel Stone]

Hi,

On 30 October 2015 at 00:27, Bryce Harrington  wrote:
> On Tue, Oct 20, 2015 at 08:45:50AM -0700, Bryce Harrington wrote:
>> On Tue, Oct 20, 2015 at 04:54:30PM +0200, Fabien Dessenne wrote:
>> > Add the possibility for the compositor backend to provide with the list
>> > of supported pixel formats for dmabuf-based buffers.
>> > This information is used by linux_dmabuf to inform clients when binding.
>> >
>> > Signed-off-by: Fabien Dessenne 
>>
>> Reviewed-by 
>>
>> There's not a way to verify the list is getting sent across to the
>> client is there?
>
> Also, patch no longer applies since e3c0d8af.
>
> I'd like to better understand what this is going to be used for, before
> landing it.  Another R-b on this would be nice as well; Giulio perhaps
> you could give this patch a review?

Hm, to be honest I'd prefer this not land just now, or like this.

dmabuf usage in compositor-drm is just an optimisation, where the
fallback path is through gl-renderer. Anything gl-renderer doesn't
support, we essentially can't display. Those formats are what we need
to send to the compositor (possibly with the intersection between
gl-renderer and compositor-drm marked as preferred), but right now, as
the comment notes, we lack a way to query this through EGL. This is
being worked on though.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Giulio Camuffo]

2015-10-30 2:27 GMT+02:00 Bryce Harrington :
> On Tue, Oct 20, 2015 at 08:45:50AM -0700, Bryce Harrington wrote:
>> On Tue, Oct 20, 2015 at 04:54:30PM +0200, Fabien Dessenne wrote:
>> > Add the possibility for the compositor backend to provide with the list
>> > of supported pixel formats for dmabuf-based buffers.
>> > This information is used by linux_dmabuf to inform clients when binding.
>> >
>> > Signed-off-by: Fabien Dessenne 
>>
>> Reviewed-by 
>>
>> There's not a way to verify the list is getting sent across to the
>> client is there?

I'm not sure i understand this question.

>
> Also, patch no longer applies since e3c0d8af.
>
> I'd like to better understand what this is going to be used for, before
> landing it.  Another R-b on this would be nice as well; Giulio perhaps
> you could give this patch a review?

Well, it doesn't apply because we now have a new vfunc in
weston_backend, but rebasing it should be trivial.

>
> Bryce
>
>> Bryce
>>
>> > ---
>> >  src/compositor.h   |  2 ++
>> >  src/linux-dmabuf.c | 16 +---
>> >  2 files changed, 15 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/src/compositor.h b/src/compositor.h
>> > index 2e2a185..f58e0cc 100644
>> > --- a/src/compositor.h
>> > +++ b/src/compositor.h
>> > @@ -613,6 +613,8 @@ enum weston_capability {
>> >  struct weston_backend {
>> > void (*destroy)(struct weston_compositor *ec);
>> > void (*restore)(struct weston_compositor *ec);
>> > +   void (*get_dmabuf_formats)(struct weston_compositor *ec,
>> > +  struct wl_array *formats);
>> >  };
>> >
>> >  struct weston_compositor {
>> > diff --git a/src/linux-dmabuf.c b/src/linux-dmabuf.c
>> > index 90c9757..ee02ea4 100644
>> > --- a/src/linux-dmabuf.c
>> > +++ b/src/linux-dmabuf.c
>> > @@ -433,9 +433,19 @@ bind_linux_dmabuf(struct wl_client *client,
>> > wl_resource_set_implementation(resource, _dmabuf_implementation,
>> >compositor, NULL);
>> >
>> > -   /* EGL_EXT_image_dma_buf_import does not provide a way to query the
>> > -* supported pixel formats. */
>> > -   /* XXX: send formats */
>> > +   if (compositor->backend->get_dmabuf_formats) {

Can the formats change in the compositor lifetime? If not, it may be
better to ask for them once and store the result instead of asking for
each new client.

>> > +   struct wl_array dmabuf_formats;
>> > +   uint32_t *p;
>> > +
>> > +   wl_array_init(_formats);
>> > +
>> > +   compositor->backend->get_dmabuf_formats(compositor,
>> > +   _formats);
>> > +   wl_array_for_each(p, _formats)
>> > +   zlinux_dmabuf_send_format(resource, *p);
>> > +
>> > +   wl_array_release(_formats);
>> > +   }
>> >  }
>> >
>> >  /** Advertise linux_dmabuf support
>> > --
>> > 1.9.1
>> >
>> > ___
>> > wayland-devel mailing list
>> > wayland-devel@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>> ___
>> wayland-devel mailing list
>> wayland-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Bryce Harrington]

On Tue, Oct 20, 2015 at 08:45:50AM -0700, Bryce Harrington wrote:
> On Tue, Oct 20, 2015 at 04:54:30PM +0200, Fabien Dessenne wrote:
> > Add the possibility for the compositor backend to provide with the list
> > of supported pixel formats for dmabuf-based buffers.
> > This information is used by linux_dmabuf to inform clients when binding.
> > 
> > Signed-off-by: Fabien Dessenne 
> 
> Reviewed-by 
> 
> There's not a way to verify the list is getting sent across to the
> client is there?

Also, patch no longer applies since e3c0d8af.

I'd like to better understand what this is going to be used for, before
landing it.  Another R-b on this would be nice as well; Giulio perhaps
you could give this patch a review?
 
Bryce

> Bryce
> 
> > ---
> >  src/compositor.h   |  2 ++
> >  src/linux-dmabuf.c | 16 +---
> >  2 files changed, 15 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/compositor.h b/src/compositor.h
> > index 2e2a185..f58e0cc 100644
> > --- a/src/compositor.h
> > +++ b/src/compositor.h
> > @@ -613,6 +613,8 @@ enum weston_capability {
> >  struct weston_backend {
> > void (*destroy)(struct weston_compositor *ec);
> > void (*restore)(struct weston_compositor *ec);
> > +   void (*get_dmabuf_formats)(struct weston_compositor *ec,
> > +  struct wl_array *formats);
> >  };
> >  
> >  struct weston_compositor {
> > diff --git a/src/linux-dmabuf.c b/src/linux-dmabuf.c
> > index 90c9757..ee02ea4 100644
> > --- a/src/linux-dmabuf.c
> > +++ b/src/linux-dmabuf.c
> > @@ -433,9 +433,19 @@ bind_linux_dmabuf(struct wl_client *client,
> > wl_resource_set_implementation(resource, _dmabuf_implementation,
> >compositor, NULL);
> >  
> > -   /* EGL_EXT_image_dma_buf_import does not provide a way to query the
> > -* supported pixel formats. */
> > -   /* XXX: send formats */
> > +   if (compositor->backend->get_dmabuf_formats) {
> > +   struct wl_array dmabuf_formats;
> > +   uint32_t *p;
> > +
> > +   wl_array_init(_formats);
> > +
> > +   compositor->backend->get_dmabuf_formats(compositor,
> > +   _formats);
> > +   wl_array_for_each(p, _formats)
> > +   zlinux_dmabuf_send_format(resource, *p);
> > +
> > +   wl_array_release(_formats);
> > +   }
> >  }
> >  
> >  /** Advertise linux_dmabuf support
> > -- 
> > 1.9.1
> > 
> > ___
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Fabien Dessenne]

Add the possibility for the compositor backend to provide with the list
of supported pixel formats for dmabuf-based buffers.
This information is used by linux_dmabuf to inform clients when binding.

Signed-off-by: Fabien Dessenne 
---
 src/compositor.h   |  2 ++
 src/linux-dmabuf.c | 16 +---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/src/compositor.h b/src/compositor.h
index 2e2a185..f58e0cc 100644
--- a/src/compositor.h
+++ b/src/compositor.h
@@ -613,6 +613,8 @@ enum weston_capability {
 struct weston_backend {
void (*destroy)(struct weston_compositor *ec);
void (*restore)(struct weston_compositor *ec);
+   void (*get_dmabuf_formats)(struct weston_compositor *ec,
+  struct wl_array *formats);
 };
 
 struct weston_compositor {
diff --git a/src/linux-dmabuf.c b/src/linux-dmabuf.c
index 90c9757..ee02ea4 100644
--- a/src/linux-dmabuf.c
+++ b/src/linux-dmabuf.c
@@ -433,9 +433,19 @@ bind_linux_dmabuf(struct wl_client *client,
wl_resource_set_implementation(resource, _dmabuf_implementation,
   compositor, NULL);
 
-   /* EGL_EXT_image_dma_buf_import does not provide a way to query the
-* supported pixel formats. */
-   /* XXX: send formats */
+   if (compositor->backend->get_dmabuf_formats) {
+   struct wl_array dmabuf_formats;
+   uint32_t *p;
+
+   wl_array_init(_formats);
+
+   compositor->backend->get_dmabuf_formats(compositor,
+   _formats);
+   wl_array_for_each(p, _formats)
+   zlinux_dmabuf_send_format(resource, *p);
+
+   wl_array_release(_formats);
+   }
 }
 
 /** Advertise linux_dmabuf support
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] dmabuf: get supported dmabuf formats from compositor backend

[Bryce Harrington]

On Tue, Oct 20, 2015 at 04:54:30PM +0200, Fabien Dessenne wrote:
> Add the possibility for the compositor backend to provide with the list
> of supported pixel formats for dmabuf-based buffers.
> This information is used by linux_dmabuf to inform clients when binding.
> 
> Signed-off-by: Fabien Dessenne 

Reviewed-by 

There's not a way to verify the list is getting sent across to the
client is there?

Bryce

> ---
>  src/compositor.h   |  2 ++
>  src/linux-dmabuf.c | 16 +---
>  2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/src/compositor.h b/src/compositor.h
> index 2e2a185..f58e0cc 100644
> --- a/src/compositor.h
> +++ b/src/compositor.h
> @@ -613,6 +613,8 @@ enum weston_capability {
>  struct weston_backend {
>   void (*destroy)(struct weston_compositor *ec);
>   void (*restore)(struct weston_compositor *ec);
> + void (*get_dmabuf_formats)(struct weston_compositor *ec,
> +struct wl_array *formats);
>  };
>  
>  struct weston_compositor {
> diff --git a/src/linux-dmabuf.c b/src/linux-dmabuf.c
> index 90c9757..ee02ea4 100644
> --- a/src/linux-dmabuf.c
> +++ b/src/linux-dmabuf.c
> @@ -433,9 +433,19 @@ bind_linux_dmabuf(struct wl_client *client,
>   wl_resource_set_implementation(resource, _dmabuf_implementation,
>  compositor, NULL);
>  
> - /* EGL_EXT_image_dma_buf_import does not provide a way to query the
> -  * supported pixel formats. */
> - /* XXX: send formats */
> + if (compositor->backend->get_dmabuf_formats) {
> + struct wl_array dmabuf_formats;
> + uint32_t *p;
> +
> + wl_array_init(_formats);
> +
> + compositor->backend->get_dmabuf_formats(compositor,
> + _formats);
> + wl_array_for_each(p, _formats)
> + zlinux_dmabuf_send_format(resource, *p);
> +
> + wl_array_release(_formats);
> + }
>  }
>  
>  /** Advertise linux_dmabuf support
> -- 
> 1.9.1
> 
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel