<div dir="ltr"><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">So the plan is for alloc_handle_t to not be sub-classed by the implementations, but have all necessary information that an implementation would need?  </span><div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline"><br></span></div><div><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline">If so, how do we reconcile the implementation specific information that is often in </span>the handle:</div><div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><a href="https://github.com/intel/minigbm/blob/master/cros_gralloc/cros_gralloc_handle.h" target="_blank">https://github.com/intel/<wbr>minigbm/blob/master/cros_<wbr>gralloc/cros_gralloc_handle.h</a> [consumer_usage, producer_usage, yuv_color_<wbr>range, is_updated etc.] </div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><a href="https://chromium.googlesource.com/chromiumos/platform/minigbm/+/master/cros_gralloc/cros_gralloc_handle.h" target="_blank">https://chromium.googlesource.<wbr>com/chromiumos/platform/<wbr>minigbm/+/master/cros_gralloc/<wbr>cros_gralloc_handle.h</a> [use_flags, pixel_stride]</div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial">In our case, removing our minigbm specific use flags from the handle would add complexity to our (*registerBuffer) path. </div><div style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:small;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;text-decoration-style:initial;text-decoration-color:initial"><br></div></div><div class="gmail_extra"><div class="gmail_quote">On Thu, Dec 21, 2017 at 10:14 AM, Rob Herring <span dir="ltr"><<a href="mailto:robh@kernel.org" target="_blank">robh@kernel.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Dec 13, 2017 at 5:02 PM, Gurchetan Singh<br>
<<a href="mailto:gurchetansingh@chromium.org">gurchetansingh@chromium.org</a>> wrote:<br>
> Hi Robert,<br>
><br>
> Thanks for looking into this!  We need to decide if we want:<br>
><br>
> (1) A common struct that implementations can subclass, i.e:<br>
><br>
> struct blah_gralloc_handle {<br>
>     alloc_handle_t alloc_handle;<br>
>     int x, y, z;<br>
>     ....<br>
> }<br>
><br>
> (2) An accessor library that vendors can implement, i.e:<br>
><br>
> struct drmAndroidHandleInfo {<br>
>    uint32_t (*get_fourcc)(buffer_handle_t handle);<br>
>    uint32_t (*get_stride)(buffer_handle_t handle, uint32_t plane);<br>
>    uint32_t (*get_offsets)(buffer_handle_t handle, uint32_t plane);<br>
>    uint64_t (*get_modifier)(buffer_handle_<wbr>t handle);<br>
> };<br>
><br>
> From my perspective as someone who has to maintain the minigbm gralloc<br>
> implementation, (2) is preferable since:<br>
<br>
</span>Yeah, I'd prefer not to encourage 1 as the default.<br>
<span class=""><br>
> a) We really don't have a need for fields like data_owner, void *data, etc.<br>
<br>
</span>We should be able to get rid of this. It's just for tracking imports.<br>
<span class=""><br>
> Also, minigbm puts per plane fds, strides, offsets into the handle.<br>
> Separating the information for the first plane (for the alloc_handle_t) and<br>
> then rest of the planes would be annoying.<br>
<br>
</span>The plan is to add those to alloc_handle_t.<br>
<span class=""><br>
> b) we can avoid the struct within a struct that happens when we subclass,<br>
> since alignment/padding issues often pop up during<br>
> serialization/de-<wbr>serialization.  Using __attribute__((aligned(xx))) is less<br>
> portable than maintaining a POD struct.<br>
<br>
</span>Yes. Even just between 32 and 64 bit it's problematic. </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> c) IMO creating the handle should be left to the gralloc implementation.<br>
> Having accessor functions clearly defines what we need from libdrm -- to<br>
> make up for shortcomings of the gralloc API for DRM/KMS use cases.<br>
><br>
><br>
> On Wed, Dec 13, 2017 at 9:30 AM, Robert Foss <<a href="mailto:robert.foss@collabora.com">robert.foss@collabora.com</a>><br>
> wrote:<br>
>><br>
>> This series moves {gbm,drm,cros}_gralloc_handle_<wbr>t struct to libdrm,<br>
>> since at least 4 implementations exist, and share a lot of contents.<br>
>> The idea is to keep the common stuff defined in one place, and libdrm<br>
>> is the common codebase to all of these platforms.<br>
>><br>
>> Additionally, having this struct defined in libdrm will make it<br>
>> easier for mesa and grallocs to communicate.<br>
>><br>
>> Curretly missing is:<br>
>>  - Planar formats<br>
>>  - Get/Set functions<br>
>><br>
>><br>
>> Planar formats<br>
>> --------------<br>
>> Support for planar formats is needed, but has not been added<br>
>> yet, mostly since this was not already implemented in {gbm,drm}_gralloc<br>
>> and the fact the having at least initial backwards compatability would<br>
>> be nice. Anonymous unions can of course be used later on to provide<br>
>> backwards compatability if so desired.<br>
>><br>
>><br>
>> Get/Set functions<br>
>> -----------------<br>
>> During the previous discussion[1] one suggestion was to add accessor<br>
>> functions. In this RFC I've only provided a alloc_handle_create()<br>
>> function.<br>
>><br>
>> The Get/Set functions have not been added yet, I was hoping for some<br>
>> conclusive arguments for them being adeded.<br>
>><br>
>> Lastly it was suggested by Rob Herring that having a fourcc<->android<br>
>> pixel format conversion function would be useful.<br>
>><br>
>><br>
>> [1]<br>
>> <a href="https://lists.freedesktop.org/archives/mesa-dev/2017-November/178199.html" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>archives/mesa-dev/2017-<wbr>November/178199.html</a><br>
>><br>
>> Robert Foss (5):<br>
>>   android: Move gralloc handle struct to libdrm<br>
>>   android: Add version variable to alloc_handle_t<br>
>>   android: Mark alloc_handle_t magic variable as const<br>
>>   android: Remove member name from alloc_handle_t<br>
>>   android: Change alloc_handle_t format from Android format to fourcc<br>
>><br>
>>  Android.mk                   |  8 +++-<br>
>>  Makefile.sources             |  3 ++<br>
>>  android/alloc_handle.h       | 87<br>
>> ++++++++++++++++++++++++++++++<wbr>++++++++++++++<br>
>>  android/gralloc_drm_handle.h |  1 +<br>
>>  4 files changed, 97 insertions(+), 2 deletions(-)<br>
>>  create mode 100644 android/alloc_handle.h<br>
>>  create mode 120000 android/gralloc_drm_handle.h<br>
>><br>
>> --<br>
>> 2.14.1<br>
>><br>
>> ______________________________<wbr>_________________<br>
>> mesa-dev mailing list<br>
>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
>> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>