<div dir="ltr"><div>I made a few trivial comments (mostly just asking for plane == 0 assertions more places). With that, these 5 are<br><br></div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 8, 2017 at 3:09 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="h5">On Tue, Mar 7, 2017 at 9:31 PM, Ben Widawsky <span dir="ltr"><<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-3582020962758831698m_848714118394336865gmail-">Unlike stride, there was no previous offset getter, so it can be right<br>
on the first try.<br>
<br>
v2: Return EINVAL when plane is greater than total planes to make it<br>
match the similar APIs.<br>
Avoid leak after fromPlanar (Daniel)<br>
Make sure when getting offsets we consider dumb images (Daniel)<br>
<br>
v3: Use Jason's recommendation for handling the non-planar case.<br>
<br>
</span>v4: Return int64_t so we can get real errors<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-"><br>
Signed-off-by: Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>><br>
Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com" target="_blank">eric.engestrom@imgtec.com</a>><br>
Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">daniels@collabora.com</a>><br>
---<br>
src/gbm/backends/dri/gbm_dri.<wbr>c | 33 ++++++++++++++++++++++++++++++<wbr>+++<br>
src/gbm/gbm-symbols-check | 1 +<br>
src/gbm/main/gbm.c | 15 +++++++++++++++<br>
src/gbm/main/gbm.h | 3 +++<br>
src/gbm/main/gbmint.h | 1 +<br>
5 files changed, 53 insertions(+)<br>
<br>
diff --git a/src/gbm/backends/dri/gbm_dri<wbr>.c b/src/gbm/backends/dri/gbm_dri<wbr>.c<br>
</span>index 56a976fe78..b53f52f8df 100644<br>
--- a/src/gbm/backends/dri/gbm_dri<wbr>.c<br>
+++ b/src/gbm/backends/dri/gbm_dri<wbr>.c<br>
@@ -699,6 +699,38 @@ gbm_dri_bo_get_stride(struct gbm_bo *_bo, int plane)<br>
return (uint32_t)stride;<br>
}<br>
<br>
+static int64_t<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-">+gbm_dri_bo_get_offset(struct gbm_bo *_bo, int plane)<br>
+{<br>
+ struct gbm_dri_device *dri = gbm_dri_device(_bo->gbm);<br>
+ struct gbm_dri_bo *bo = gbm_dri_bo(_bo);<br>
+ int offset = 0;<br>
+<br>
+ if (!dri->image || dri->image->base.version < 13 || !dri->image->fromPlanar) {<br>
+ errno = ENOSYS;<br>
</span>+ return -1;<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-">+ }<br>
+<br>
+ if (plane >= get_number_planes(dri, bo->image)) {<br>
+ errno = EINVAL;<br>
</span>+ return -2;<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-">+ }<br>
+<br>
+ /* Dumb images have no offset */<br>
+ if (!bo->image)<br></span></blockquote><div><br></div></div></div><div>assert(plane == 0);<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-3582020962758831698m_848714118394336865gmail-">
+ return 0;<br>
</span><span class="m_-3582020962758831698m_848714118394336865gmail-">+<br>
+ __DRIimage *image = dri->image->fromPlanar(bo->ima<wbr>ge, plane, NULL);<br>
+ if (image) {<br>
+ dri->image->queryImage(image, __DRI_IMAGE_ATTRIB_OFFSET, &offset);<br>
+ dri->image->destroyImage(image<wbr>);<br>
+ } else {<br></span></blockquote></span><div><br>assert(plane == 0);<br> </div><div><div class="h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-3582020962758831698m_848714118394336865gmail-">
+ dri->image->queryImage(bo->ima<wbr>ge, __DRI_IMAGE_ATTRIB_OFFSET, &offset);<br>
+ }<br>
+<br>
+ return (uint32_t)offset;<br>
+}<br>
+<br>
static void<br>
gbm_dri_bo_destroy(struct gbm_bo *_bo)<br>
{<br>
</span>@@ -1194,6 +1226,7 @@ dri_device_create(int fd)<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-"> dri->base.base.bo_get_planes = gbm_dri_bo_get_planes;<br>
dri->base.base.bo_get_handle = gbm_dri_bo_get_handle_for_plan<wbr>e;<br>
dri->base.base.bo_get_stride = gbm_dri_bo_get_stride;<br>
+ dri->base.base.bo_get_offset = gbm_dri_bo_get_offset;<br>
dri->base.base.bo_destroy = gbm_dri_bo_destroy;<br>
dri->base.base.destroy = dri_destroy;<br>
dri->base.base.surface_create = gbm_dri_surface_create;<br>
diff --git a/src/gbm/gbm-symbols-check b/src/gbm/gbm-symbols-check<br>
index 459006a63f..7ff78ab400 100755<br>
--- a/src/gbm/gbm-symbols-check<br>
+++ b/src/gbm/gbm-symbols-check<br>
@@ -16,6 +16,7 @@ gbm_bo_get_height<br>
gbm_bo_get_stride<br>
gbm_bo_get_stride_for_plane<br>
gbm_bo_get_format<br>
+gbm_bo_get_offset<br>
gbm_bo_get_device<br>
gbm_bo_get_handle<br>
gbm_bo_get_fd<br>
diff --git a/src/gbm/main/gbm.c b/src/gbm/main/gbm.c<br>
</span>index 0a9f0bef7e..afcca63da3 100644<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-">--- a/src/gbm/main/gbm.c<br>
+++ b/src/gbm/main/gbm.c<br>
@@ -194,6 +194,21 @@ gbm_bo_get_format(struct gbm_bo *bo)<br>
return bo->format;<br>
}<br>
<br>
+/** Get the offset for the data of the specified plane<br>
+ *<br>
+ * Extra planes, and even the first plane, may have an offset from the start of<br>
+ * the buffer object. This function will provide the offset for the given plane<br>
+ * to be used in various KMS APIs.<br>
+ *<br>
+ * \param bo The buffer object<br>
+ * \return The offset<br></span></blockquote><div><br></div></div></div><div>Maybe a comment about < 0 on error?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="m_-3582020962758831698m_848714118394336865gmail-">
+ */<br>
</span>+GBM_EXPORT int64_t<br></blockquote><div><br></div><div>I find the int64_t a bit ugly given the rest of the API, but I also can't really see any better options. I guess we could say (uint32_t)-1 is an invalid offset (it almost certainly is) and use that. In any case, int64_t is just fine.<br></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span class="m_-3582020962758831698m_848714118394336865gmail-">+gbm_bo_get_offset(struct gbm_bo *bo, int plane)<br>
+{<br>
+ return bo->gbm->bo_get_offset(bo, plane);<br>
+}<br>
+<br>
/** Get the gbm device used to create the buffer object<br>
*<br>
* \param bo The buffer object<br>
diff --git a/src/gbm/main/gbm.h b/src/gbm/main/gbm.h<br>
</span>index 1719c5312a..e3e5d34d97 100644<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-">--- a/src/gbm/main/gbm.h<br>
+++ b/src/gbm/main/gbm.h<br>
@@ -309,6 +309,9 @@ gbm_bo_get_stride_for_plane(st<wbr>ruct gbm_bo *bo, int plane);<br>
uint32_t<br>
gbm_bo_get_format(struct gbm_bo *bo);<br>
<br>
</span>+int64_t<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-">+gbm_bo_get_offset(struct gbm_bo *bo, int plane);<br>
+<br>
struct gbm_device *<br>
gbm_bo_get_device(struct gbm_bo *bo);<br>
<br>
diff --git a/src/gbm/main/gbmint.h b/src/gbm/main/gbmint.h<br>
</span>index 26d18bab6b..a6541d91c5 100644<br>
<span class="m_-3582020962758831698m_848714118394336865gmail-">--- a/src/gbm/main/gbmint.h<br>
+++ b/src/gbm/main/gbmint.h<br>
@@ -79,6 +79,7 @@ struct gbm_device {<br>
int (*bo_get_planes)(struct gbm_bo *bo);<br>
union gbm_bo_handle (*bo_get_handle)(struct gbm_bo *bo, int plane);<br>
uint32_t (*bo_get_stride)(struct gbm_bo *bo, int plane);<br>
</span>+ int64_t (*bo_get_offset)(struct gbm_bo *bo, int plane);<br>
<div class="m_-3582020962758831698m_848714118394336865gmail-HOEnZb"><div class="m_-3582020962758831698m_848714118394336865gmail-h5"> void (*bo_destroy)(struct gbm_bo *bo);<br>
<br>
struct gbm_surface *(*surface_create)(struct gbm_device *gbm,<br>
--<br>
2.12.0<br>
<br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>