On Tue, Mar 7, 2017 at 2:31 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
> On 17-03-07 08:28:09, Jason Ekstrand wrote:
>
>> On Mon, Mar 6, 2017 at 6:37 PM, Ben Widawsky <b...@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 <b...@bwidawsk.net>
>>> Acked-by: Daniel Stone <dani...@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. :-)
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
