[Mesa-dev] [PATCH 2/5] gbm: Export a getter for per plane handles

[Jason Ekstrand]

On Tue, Mar 7, 2017 at 2:31 PM, Ben Widawsky <ben at bwidawsk.net> wrote:

> On 17-03-07 08:28:09, Jason Ekstrand wrote:
>
>> On Mon, Mar 6, 2017 at 6:37 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>>
>> v2: Make the error return be -1 instead of 0 because I think 0 is
>>> actually valid.
>>>
>>> v3: Set errno to EINVAL when the specified plane is above the total
>>> planes. (Jason Ekstrand)
>>> Return the bo's handle if there is no image ie. for dumb images like
>>> cursor (Daniel)
>>>
>>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>>> Acked-by: Daniel Stone <daniels at collabora.com>
>>> ---
>>>  src/gbm/backends/dri/gbm_dri.c | 35 +++++++++++++++++++++++++++++++++++
>>>  src/gbm/gbm-symbols-check      |  1 +
>>>  src/gbm/main/gbm.c             | 18 ++++++++++++++++++
>>>  src/gbm/main/gbm.h             |  3 +++
>>>  src/gbm/main/gbmint.h          |  1 +
>>>  5 files changed, 58 insertions(+)
>>>
>>> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_
>>> dri.c
>>> index 0b75e411df..c3704e505b 100644
>>> --- a/src/gbm/backends/dri/gbm_dri.c
>>> +++ b/src/gbm/backends/dri/gbm_dri.c
>>> @@ -624,6 +624,40 @@ gbm_dri_bo_get_planes(struct gbm_bo *_bo)
>>>     return get_number_planes(dri, bo->image);
>>>  }
>>>
>>> +static union gbm_bo_handle
>>> +gbm_dri_bo_get_handle_for_plane(struct gbm_bo *_bo, int plane)
>>> +{
>>> +   struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
>>> +   struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
>>> +   union gbm_bo_handle ret;
>>> +   ret.s32 = -1;
>>> +
>>> +   if (!dri->image || dri->image->base.version < 13 ||
>>> !dri->image->fromPlanar) {
>>> +      errno = ENOSYS;
>>> +      return ret;
>>> +   }
>>> +
>>> +   if (plane >= get_number_planes(dri, bo->image)) {
>>> +      errno = EINVAL;
>>> +      return ret;
>>> +   }
>>> +
>>> +   if (!bo->image) {
>>> +      ret.s32 = bo->handle;
>>> +      return ret;
>>> +   }
>>> +
>>> +   __DRIimage *image = dri->image->fromPlanar(bo->image, plane, NULL);
>>> +   if (!image) {
>>> +      /* Use the parent's handle */
>>> +      image = bo->image;
>>>
>>>
>> Assuming get_number_of_planes does the right thing, I think this is
>> correct.  Would it make sense to add an assert(plane == 0) to this error
>> case and the one above?
>>
>>
>>
> Let me claim ignorance...
>
> For "the one above": I thought it might be possible to have a planar dumb
> bo, in
> which case, the assertion wouldn't always hold. However, I have other code
> here
> which prevents that, so I think I'll modify the commit message, add some
> comments, and most importantly make a patch before this which prevents
> dumb BOs
> from being planar formats.
>
> For the !image case: this assertion should also hold, but since fromPlanar
> can
> be implemented however by the DRI implementation, I don't know if it's the
> right
> thing to add assertions in that path. I'm fine with adding it, I just
> claim some
> level of ignorance.
>

Yes, the top one is definitely assert-worthy.  The bottom one maybe not?
For the bottom one, I guess I could see someone making fromPlanar as just
return ref(bo->image) in which case maybe that's not safe?  Then again, if
the implemented it that way, all of the planes would have to have the same
offset/stride so the next two entrypoints would be toast.  It seems
somewhat reasonable to assert for the second one too but I also must claim
ignorance at this point. :-)
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170307/ca71b0cb/attachment.html>