[Mesa-dev] [PATCH 06/34] gbm: Export a per plane getter for stride

[Ben Widawsky]

On 17-01-31 11:33:39, Jason Ekstrand wrote:
>On Mon, Jan 23, 2017 at 10:19 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>
>> v2: Preserve legacy behavior when plane is 0 (Jason Ekstrand)
>> EINVAL when input plane is greater than total planes (Jason Ekstrand)
>> Don't leak the image after fromPlanar (Daniel)
>> Move bo->image check below plane count preventing bad index succeeding
>> (Daniel)
>>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com> (v1)
>> 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             | 15 ++++++++++++++-
>>  src/gbm/main/gbm.h             |  3 +++
>>  4 files changed, 52 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_
>> dri.c
>> index f055a665db..395f78e851 100644
>> --- a/src/gbm/backends/dri/gbm_dri.c
>> +++ b/src/gbm/backends/dri/gbm_dri.c
>> @@ -661,7 +661,40 @@ gbm_dri_bo_get_handle_for_plane(struct gbm_bo *_bo,
>> int plane)
>>  static uint32_t
>>  gbm_dri_bo_get_stride(struct gbm_bo *_bo, int plane)
>>  {
>> -   return _bo->stride;
>> +   struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);
>> +   struct gbm_dri_bo *bo = gbm_dri_bo(_bo);
>> +   __DRIimage *image;
>> +   int stride = 0;
>> +
>> +   /* Preserve legacy behavior if plane is 0 */
>> +   if (plane == 0)
>> +      return _bo->stride;
>>
>
>This implies that, for multi-planar images, you have to have the BO stride
>be the same as the image stride.  Do we want this or do we only want to
>return bo->stride if we don't have enough DRI stuff (in the if below) to
>use the image?  I think it's probably ok either way.
>

The biggest problem is if I change it (see hunk below) is that it potentially
doesn't match the old behavior, and I don't really have a great way to test this
outside of our environment. I went with the path of least risk, however you do
raise a good point.

Daniel, do you have an opinion here?


diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c
index 7c5723f0d8..9c24eab24f 100644
--- a/src/gbm/backends/dri/gbm_dri.c
+++ b/src/gbm/backends/dri/gbm_dri.c
@@ -666,11 +666,11 @@ gbm_dri_bo_get_stride(struct gbm_bo *_bo, int plane)
    __DRIimage *image;
    int stride = 0;
 
-   /* Preserve legacy behavior if plane is 0 */
-   if (plane == 0)
-      return _bo->stride;
-
    if (!dri->image || dri->image->base.version < 11 || !dri->image->fromPlanar) {
+      /* Preserve legacy behavior if plane is 0 */
+      if (plane == 0)
+         return _bo->stride;
+
       errno = ENOSYS;
       return 0;
    }


>
>> +
>> +   if (!dri->image || dri->image->base.version < 11 ||
>> !dri->image->fromPlanar) {
>> +      errno = ENOSYS;
>> +      return 0;
>> +   }
>> +
>> +   if (plane >= get_number_planes(dri, bo->image)) {
>> +      errno = EINVAL;
>> +      return 0;
>> +   }
>> +
>> +   if (bo->image == NULL)
>> +      return _bo->stride;
>> +
>> +   image = dri->image->fromPlanar(bo->image, plane, NULL);
>> +   if (!image) {
>> +      /* Use the parent stride */
>> +      image = bo->image;
>> +   }
>> +
>> +   dri->image->queryImage(image, __DRI_IMAGE_ATTRIB_STRIDE, &stride);
>> +
>> +   if (image != bo->image)
>> +      dri->image->destroyImage(image);
>> +
>> +   return (uint32_t)stride;
>>  }
>>
>>  static void
>> diff --git a/src/gbm/gbm-symbols-check b/src/gbm/gbm-symbols-check
>> index 1e6dd4d3ec..459006a63f 100755
>> --- a/src/gbm/gbm-symbols-check
>> +++ b/src/gbm/gbm-symbols-check
>> @@ -14,6 +14,7 @@ gbm_bo_unmap
>>  gbm_bo_get_width
>>  gbm_bo_get_height
>>  gbm_bo_get_stride
>> +gbm_bo_get_stride_for_plane
>>  gbm_bo_get_format
>>  gbm_bo_get_device
>>  gbm_bo_get_handle
>> diff --git a/src/gbm/main/gbm.c b/src/gbm/main/gbm.c
>> index 7462e90c4c..0a9f0bef7e 100644
>> --- a/src/gbm/main/gbm.c
>> +++ b/src/gbm/main/gbm.c
>> @@ -165,7 +165,20 @@ gbm_bo_get_height(struct gbm_bo *bo)
>>  GBM_EXPORT uint32_t
>>  gbm_bo_get_stride(struct gbm_bo *bo)
>>  {
>> -   return bo->gbm->bo_get_stride(bo, 0);
>> +   return gbm_bo_get_stride_for_plane(bo, 0);
>> +}
>> +
>> +/** Get the stride for the given plane
>> + *
>> + * \param bo The buffer object
>> + * \param plane for which you want the stride
>> + *
>> + * \sa gbm_bo_get_stride()
>> + */
>> +GBM_EXPORT uint32_t
>> +gbm_bo_get_stride_for_plane(struct gbm_bo *bo, int plane)
>> +{
>> +   return bo->gbm->bo_get_stride(bo, plane);
>>  }
>>
>>  /** Get the format of the buffer object
>> diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h
>> index 67548206c4..1719c5312a 100644
>> --- a/src/gbm/main/gbm.h
>> +++ b/src/gbm/main/gbm.h
>> @@ -304,6 +304,9 @@ uint32_t
>>  gbm_bo_get_stride(struct gbm_bo *bo);
>>
>>  uint32_t
>> +gbm_bo_get_stride_for_plane(struct gbm_bo *bo, int plane);
>> +
>> +uint32_t
>>  gbm_bo_get_format(struct gbm_bo *bo);
>>
>>  struct gbm_device *
>> --
>> 2.11.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>

-- 
Ben Widawsky, Intel Open Source Technology Center