gbm: add gbm_{bo, surface}_create_with_modifiers2

Submitted by Simon Ser on June 24, 2019, 6:51 p.m.

Details

Message ID 2MLYuVXgXTGMRpmxMxPhgs2leViaW_aeTsrmNO0qjgfibesAbwULx4iIXz26YxRGKUx5Fz1rSTJS9_O7Eg29I7RRiTtsT5tiyIeJSTC78os=@emersion.fr
State New
Headers show
Series "gbm: add gbm_{bo, surface}_create_with_modifiers2" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Simon Ser June 24, 2019, 6:51 p.m.
gbm_{bo,surface}_create_with_modifiers is missing the usage flags. Add a new
function which lets library users specify it.

Signed-off-by: Simon Ser <contact@emersion.fr>
---
 src/gbm/main/gbm.c | 38 ++++++++++++++++++++++++++++++++++++++
 src/gbm/main/gbm.h | 17 +++++++++++++++++
 2 files changed, 55 insertions(+)

--
2.22.0

Patch hide | download patch | download mbox

diff --git a/src/gbm/main/gbm.c b/src/gbm/main/gbm.c
index 38480ca966c6..ca68a3292327 100644
--- a/src/gbm/main/gbm.c
+++ b/src/gbm/main/gbm.c
@@ -491,6 +491,27 @@  gbm_bo_create_with_modifiers(struct gbm_device *gbm,
    return gbm->bo_create(gbm, width, height, format, 0, modifiers, count);
 }

+GBM_EXPORT struct gbm_bo *
+gbm_bo_create_with_modifiers2(struct gbm_device *gbm,
+                              uint32_t width, uint32_t height,
+                              uint32_t format,
+                              const uint64_t *modifiers,
+                              const unsigned int count,
+                              uint32_t usage)
+{
+   if (width == 0 || height == 0) {
+      errno = EINVAL;
+      return NULL;
+   }
+
+   if ((count && !modifiers) || (modifiers && !count)) {
+      errno = EINVAL;
+      return NULL;
+   }
+
+   return gbm->bo_create(gbm, width, height, format, usage, modifiers, count);
+}
+
 /**
  * Create a gbm buffer object from a foreign object
  *
@@ -616,6 +637,23 @@  gbm_surface_create_with_modifiers(struct gbm_device *gbm,
                               modifiers, count);
 }

+GBM_EXPORT struct gbm_surface *
+gbm_surface_create_with_modifiers2(struct gbm_device *gbm,
+                                   uint32_t width, uint32_t height,
+                                   uint32_t format,
+                                   const uint64_t *modifiers,
+                                   const unsigned int count,
+                                   uint32_t flags)
+{
+   if ((count && !modifiers) || (modifiers && !count)) {
+      errno = EINVAL;
+      return NULL;
+   }
+
+   return gbm->surface_create(gbm, width, height, format, flags,
+                              modifiers, count);
+}
+
 /**
  * Destroys the given surface and frees all resources associated with
  * it.
diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h
index 9b5288710a5b..0bb2e4443f97 100644
--- a/src/gbm/main/gbm.h
+++ b/src/gbm/main/gbm.h
@@ -263,6 +263,15 @@  gbm_bo_create_with_modifiers(struct gbm_device *gbm,
                              uint32_t format,
                              const uint64_t *modifiers,
                              const unsigned int count);
+
+struct gbm_bo *
+gbm_bo_create_with_modifiers2(struct gbm_device *gbm,
+                              uint32_t width, uint32_t height,
+                              uint32_t format,
+                              const uint64_t *modifiers,
+                              const unsigned int count,
+                              uint32_t flags);
+
 #define GBM_BO_IMPORT_WL_BUFFER         0x5501
 #define GBM_BO_IMPORT_EGL_IMAGE         0x5502
 #define GBM_BO_IMPORT_FD                0x5503
@@ -390,6 +399,14 @@  gbm_surface_create_with_modifiers(struct gbm_device *gbm,
                                   const uint64_t *modifiers,
                                   const unsigned int count);

+struct gbm_surface *
+gbm_surface_create_with_modifiers2(struct gbm_device *gbm,
+                                   uint32_t width, uint32_t height,
+                                   uint32_t format,
+                                   const uint64_t *modifiers,
+                                   const unsigned int count,
+                                   uint32_t flags);
+
 struct gbm_bo *
 gbm_surface_lock_front_buffer(struct gbm_surface *surface);


Comments

On 6/24/19 9:51 PM, Simon Ser wrote:
> gbm_{bo,surface}_create_with_modifiers is missing the usage flags. Add a new
> function which lets library users specify it.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> ---
>   src/gbm/main/gbm.c | 38 ++++++++++++++++++++++++++++++++++++++
>   src/gbm/main/gbm.h | 17 +++++++++++++++++
>   2 files changed, 55 insertions(+)
> 
> diff --git a/src/gbm/main/gbm.c b/src/gbm/main/gbm.c
> index 38480ca966c6..ca68a3292327 100644
> --- a/src/gbm/main/gbm.c
> +++ b/src/gbm/main/gbm.c
> @@ -491,6 +491,27 @@ gbm_bo_create_with_modifiers(struct gbm_device *gbm,
>      return gbm->bo_create(gbm, width, height, format, 0, modifiers, count);
>   }
> 
> +GBM_EXPORT struct gbm_bo *
> +gbm_bo_create_with_modifiers2(struct gbm_device *gbm,
> +                              uint32_t width, uint32_t height,
> +                              uint32_t format,
> +                              const uint64_t *modifiers,
> +                              const unsigned int count,
> +                              uint32_t usage)
> +{
> +   if (width == 0 || height == 0) {
> +      errno = EINVAL;
> +      return NULL;
> +   }
> +
> +   if ((count && !modifiers) || (modifiers && !count)) {
> +      errno = EINVAL;
> +      return NULL;
> +   }
> +
> +   return gbm->bo_create(gbm, width, height, format, usage, modifiers, count);
> +}
> +
>   /**
>    * Create a gbm buffer object from a foreign object
>    *
> @@ -616,6 +637,23 @@ gbm_surface_create_with_modifiers(struct gbm_device *gbm,
>                                 modifiers, count);
>   }
> 
> +GBM_EXPORT struct gbm_surface *
> +gbm_surface_create_with_modifiers2(struct gbm_device *gbm,
> +                                   uint32_t width, uint32_t height,
> +                                   uint32_t format,
> +                                   const uint64_t *modifiers,
> +                                   const unsigned int count,
> +                                   uint32_t flags)
> +{
> +   if ((count && !modifiers) || (modifiers && !count)) {
> +      errno = EINVAL;
> +      return NULL;
> +   }
> +
> +   return gbm->surface_create(gbm, width, height, format, flags,
> +                              modifiers, count);
> +}
> +
>   /**
>    * Destroys the given surface and frees all resources associated with
>    * it.
> diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h
> index 9b5288710a5b..0bb2e4443f97 100644
> --- a/src/gbm/main/gbm.h
> +++ b/src/gbm/main/gbm.h
> @@ -263,6 +263,15 @@ gbm_bo_create_with_modifiers(struct gbm_device *gbm,
>                                uint32_t format,
>                                const uint64_t *modifiers,
>                                const unsigned int count);
> +
> +struct gbm_bo *
> +gbm_bo_create_with_modifiers2(struct gbm_device *gbm,
> +                              uint32_t width, uint32_t height,
> +                              uint32_t format,
> +                              const uint64_t *modifiers,
> +                              const unsigned int count,
> +                              uint32_t flags);

Declaration here says 'flags' while definition says 'usage'.

I noticed that original patch (v1) for gbm_bo_create_with_modifiers did 
have usage at first but it was removed during the review. I'm having 
trouble digging what was the reason for this?


> +
>   #define GBM_BO_IMPORT_WL_BUFFER         0x5501
>   #define GBM_BO_IMPORT_EGL_IMAGE         0x5502
>   #define GBM_BO_IMPORT_FD                0x5503
> @@ -390,6 +399,14 @@ gbm_surface_create_with_modifiers(struct gbm_device *gbm,
>                                     const uint64_t *modifiers,
>                                     const unsigned int count);
> 
> +struct gbm_surface *
> +gbm_surface_create_with_modifiers2(struct gbm_device *gbm,
> +                                   uint32_t width, uint32_t height,
> +                                   uint32_t format,
> +                                   const uint64_t *modifiers,
> +                                   const unsigned int count,
> +                                   uint32_t flags);
> +
>   struct gbm_bo *
>   gbm_surface_lock_front_buffer(struct gbm_surface *surface);
> 
> --
> 2.22.0
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
> > +struct gbm_bo *
> > +gbm_bo_create_with_modifiers2(struct gbm_device *gbm,
> > +                              uint32_t width, uint32_t height,
> > +                              uint32_t format,
> > +                              const uint64_t *modifiers,
> > +                              const unsigned int count,
> > +                              uint32_t flags);
>
> Declaration here says 'flags' while definition says 'usage'.

Yeah, I tried to be consistent with gbm_bo_create, which sometimes uses
flags and sometimes usage.

I'm not sure which terminology is better. I guess flags would be more
appropriate since the enum is called gbm_bo_flags?

> I noticed that original patch (v1) for gbm_bo_create_with_modifiers did
> have usage at first but it was removed during the review. I'm having
> trouble digging what was the reason for this?

I'm not sure either. Daniel said it was a mistake.

Adding the 63bd2ae7452d4 folks to the discussion. Ben, do you remember
the details?
Hi,

On Tue, 25 Jun 2019 at 07:26, Simon Ser <contact@emersion.fr> wrote:
> > I noticed that original patch (v1) for gbm_bo_create_with_modifiers did
> > have usage at first but it was removed during the review. I'm having
> > trouble digging what was the reason for this?
>
> I'm not sure either. Daniel said it was a mistake.
>
> Adding the 63bd2ae7452d4 folks to the discussion. Ben, do you remember
> the details?

We decided to remove it since we decided that modifiers were a good
enough proxy for usage; no need to pass SCANOUT or TEXTURE anymore,
because we already get the scanout modifiers from KMS and the texture
modifiers from EGL.

In hindsight, I think this was a mistake since it only handles buffer
layout, and not buffer placement or cache configuration.

Cheers,
Daniel
Hi,

On Mon, 24 Jun 2019 at 19:51, Simon Ser <contact@emersion.fr> wrote:
> +GBM_EXPORT struct gbm_bo *
> +gbm_bo_create_with_modifiers2(struct gbm_device *gbm,
> +                              uint32_t width, uint32_t height,
> +                              uint32_t format,
> +                              const uint64_t *modifiers,
> +                              const unsigned int count,
> +                              uint32_t usage)
> +{
> +   if (width == 0 || height == 0) {
> +      errno = EINVAL;
> +      return NULL;
> +   }
> +
> +   if ((count && !modifiers) || (modifiers && !count)) {
> +      errno = EINVAL;
> +      return NULL;
> +   }
> +
> +   return gbm->bo_create(gbm, width, height, format, usage, modifiers, count);

This will trip an assert inside gbm_dri_bo_create:
   /* Callers of this may specify a modifier, or a dri usage, but not both. The
    * newer modifier interface deprecates the older usage flags.
    */
   assert(!(usage && count));

You can fix this without modifying the libgbm <-> backend interface,
given that the two should be tightly coupled together. But within
Mesa, you'll need a new version of the DRIimageExtension, adding a
createImageWithModifiers2() which re-adds the flags, given that
they're not there in the current createImageWithModifiers() hook.

Cheers,
Daniel

Hi,

On Tue, 25 Jun 2019 at 16:07, Jason Ekstrand <jason@jlekstrand.net> wrote:
> On Tue, Jun 25, 2019 at 4:04 AM Daniel Stone <daniel@fooishbar.org> wrote:
>> On Tue, 25 Jun 2019 at 07:26, Simon Ser <contact@emersion.fr> wrote:
>> > > I noticed that original patch (v1) for gbm_bo_create_with_modifiers did
>> > > have usage at first but it was removed during the review. I'm having
>> > > trouble digging what was the reason for this?
>> >
>> > I'm not sure either. Daniel said it was a mistake.
>> >
>> > Adding the 63bd2ae7452d4 folks to the discussion. Ben, do you remember
>> > the details?
>>
>> We decided to remove it since we decided that modifiers were a good
>> enough proxy for usage; no need to pass SCANOUT or TEXTURE anymore,
>> because we already get the scanout modifiers from KMS and the texture
>> modifiers from EGL.
>>
>> In hindsight, I think this was a mistake since it only handles buffer
>> layout, and not buffer placement or cache configuration.
>
>
> It's not great but modifiers should be able to handle that as well.  You can have _CONTIGUOUS versions of the modifiers supported by scanout and scanout will only advertise those and the caller has to know to place them in contiguous memory.  That's just an example but I think it would probably work for a lot of the cases.  If not, I'd love to know why not.

Sometimes it's _CONTIGUOUS, sometimes it's _ON_THIS_PCIE_DEVICE.
Either way, it does seem like a bit of an abuse: it has nothing to do
with internal buffer layout, but how and where the backing pages are
sourced.

Given that it's completely orthogonal, I wouldn't like to go trying to
combine it into the same namespace.

Cheers,
Daniel

On 19-06-25 10:02:55, Daniel Stone wrote:
> Hi,
> 
> On Tue, 25 Jun 2019 at 07:26, Simon Ser <contact@emersion.fr> wrote:
> > > I noticed that original patch (v1) for gbm_bo_create_with_modifiers did
> > > have usage at first but it was removed during the review. I'm having
> > > trouble digging what was the reason for this?
> >
> > I'm not sure either. Daniel said it was a mistake.
> >
> > Adding the 63bd2ae7452d4 folks to the discussion. Ben, do you remember
> > the details?
> 
> We decided to remove it since we decided that modifiers were a good
> enough proxy for usage; no need to pass SCANOUT or TEXTURE anymore,
> because we already get the scanout modifiers from KMS and the texture
> modifiers from EGL.
> 
> In hindsight, I think this was a mistake since it only handles buffer
> layout, and not buffer placement or cache configuration.
> 
> Cheers,
> Daniel

Yeah... leaving an optional usage would have been ideal, however, I think the
way I peddled it, it was required - so we were all wrong :-)
Hi Daniel, et. al,

Am Dienstag, den 25.06.2019, 16:27 +0100 schrieb Daniel Stone:
> Hi,
> 
> > On Tue, 25 Jun 2019 at 16:07, Jason Ekstrand <jason@jlekstrand.net> wrote:
> > > > On Tue, Jun 25, 2019 at 4:04 AM Daniel Stone <daniel@fooishbar.org> wrote:
> > > > > > On Tue, 25 Jun 2019 at 07:26, Simon Ser <contact@emersion.fr> wrote:
> > > > > I noticed that original patch (v1) for gbm_bo_create_with_modifiers did
> > > > > have usage at first but it was removed during the review. I'm having
> > > > > trouble digging what was the reason for this?
> > > > 
> > > > I'm not sure either. Daniel said it was a mistake.
> > > > 
> > > > Adding the 63bd2ae7452d4 folks to the discussion. Ben, do you remember
> > > > the details?
> > > 
> > > We decided to remove it since we decided that modifiers were a good
> > > enough proxy for usage; no need to pass SCANOUT or TEXTURE anymore,
> > > because we already get the scanout modifiers from KMS and the texture
> > > modifiers from EGL.
> > > 
> > > In hindsight, I think this was a mistake since it only handles buffer
> > > layout, and not buffer placement or cache configuration.
> > 
> > 
> > It's not great but modifiers should be able to handle that as well.  You can have _CONTIGUOUS versions of the modifiers supported by scanout and scanout will only advertise those and the caller has to know to place them in contiguous memory.  That's just an example but I think it would probably work for a lot of the cases.  If not, I'd love to know why not.
> 
> Sometimes it's _CONTIGUOUS, sometimes it's _ON_THIS_PCIE_DEVICE.
> Either way, it does seem like a bit of an abuse: it has nothing to do
> with internal buffer layout, but how and where the backing pages are
> sourced.
> 
> Given that it's completely orthogonal, I wouldn't like to go trying to
> combine it into the same namespace.

If this is about the specifics of the backing store placement, I fear
that a simple uint32_t usage won't get us far at all. It might be able
to cover the most pressing "I really need contig" issue, but it will
break down shortly after this.

At the risk of opening a big can of worms: has anyone thought about
describing the usage as a set of allocator [1]
constraints/capabilities? I feel like those are in a much better
position to actually cover the use-cases you touched above.

Regards,
Lucas

[1] https://gitlab.freedesktop.org/allocator/allocator