On Fri, Dec 1, 2017 at 7:30 AM, Robert Foss <robert.f...@collabora.com> wrote: > On Thu, 2017-11-30 at 11:14 -0600, Rob Herring wrote: >> On Thu, Nov 30, 2017 at 12:11 AM, Tapani Pälli <tapani.pa...@intel.co >> m> wrote: >> > >> > >> > On 11/30/2017 06:13 AM, Tomasz Figa wrote: >> > > >> > > On Thu, Nov 30, 2017 at 3:43 AM, Robert Foss <robert.foss@collabo >> > > ra.com> >> > > wrote: >> > > > >> > > > Hey, >> > > > >> > > > On Tue, 2017-11-28 at 11:49 +0000, Emil Velikov wrote: >> > > > > >> > > > > On 28 November 2017 at 10:45, Tapani Pälli <tapani.palli@inte >> > > > > l.com> >> > > > > wrote: >> > > > > > >> > > > > > Hi; >> > > > > > >> > > > > > >> > > > > > On 11/27/2017 04:14 PM, Robert Foss wrote: >> > > > > > > >> > > > > > > >> > > > > > > From: Tomasz Figa <tf...@chromium.org> >> > > > > > > >> > > > > > > There is no API available to properly query the >> > > > > > > IMPLEMENTATION_DEFINED >> > > > > > > format. As a workaround we rely here on gralloc >> > > > > > > allocating either >> > > > > > > an arbitrary YCbCr 4:2:0 or RGBX_8888, with the latter >> > > > > > > being >> > > > > > > recognized >> > > > > > > by lock_ycbcr failing. >> > > > > > > >> > > > > > > Reviewed-on: https://chromium-review.googlesource.com/566 >> > > > > > > 793 >> > > > > > > >> > > > > > > Signed-off-by: Tomasz Figa <tf...@chromium.org> >> > > > > > > Reviewed-by: Chad Versace <chadvers...@chromium.org> >> > > > > > > Signed-off-by: Robert Foss <robert.f...@collabora.com> >> > > > > > > --- >> > > > > > > src/egl/drivers/dri2/platform_android.c | 39 >> > > > > > > +++++++++++++++++++++++++++++++-- >> > > > > > > 1 file changed, 37 insertions(+), 2 deletions(-) >> > > > > > > >> > > > > > > diff --git a/src/egl/drivers/dri2/platform_android.c >> > > > > > > b/src/egl/drivers/dri2/platform_android.c >> > > > > > > index 63223e9a69..ae914d79c1 100644 >> > > > > > > --- a/src/egl/drivers/dri2/platform_android.c >> > > > > > > +++ b/src/egl/drivers/dri2/platform_android.c >> > > > > > > @@ -59,6 +59,10 @@ static const struct droid_yuv_format >> > > > > > > droid_yuv_formats[] = { >> > > > > > > { HAL_PIXEL_FORMAT_YCbCr_420_888, 0, 1, >> > > > > > > __DRI_IMAGE_FOURCC_YUV420 >> > > > > > > }, >> > > > > > > { HAL_PIXEL_FORMAT_YCbCr_420_888, 1, 1, >> > > > > > > __DRI_IMAGE_FOURCC_YVU420 >> > > > > > > }, >> > > > > > > { HAL_PIXEL_FORMAT_YV12, 1, 1, >> > > > > > > __DRI_IMAGE_FOURCC_YVU420 >> > > > > > > }, >> > > > > > > + /* HACK: See droid_create_image_from_prime_fd() and >> > > > > > > b/32077885. */ >> > > > > > > + { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, 0, 2, >> > > > > > > __DRI_IMAGE_FOURCC_NV12 }, >> > > > > > > + { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, 0, 1, >> > > > > > > __DRI_IMAGE_FOURCC_YUV420 }, >> > > > > > > + { HAL_PIXEL_FORMAT_IMPLEMENTATION_DEFINED, 1, 1, >> > > > > > > __DRI_IMAGE_FOURCC_YVU420 }, >> > > > > > >> > > > > > >> > > > > > >> > > > > > One alternative way would be to ask gralloc about these >> > > > > > formats. On >> > > > > > gralloc0 >> > > > > > this would need a perform() hook and gralloc1 has >> > > > > > getFormat(). This >> > > > > > is how >> > > > > > it is done currently on Android-IA, see following commits: >> > > > > > >> > > > > > https://github.com/intel/external-mesa/commit/deb323eafa321 >> > > > > > c725805a >> > > > > > 702ed19cb4983346b60 >> > > > > > >> > > > > > https://github.com/intel/external-mesa/commit/7cc01beaf540e >> > > > > > 29862853 >> > > > > > 561ef93c6c4e86c4c1a >> > > > > > >> > > > > > Do you think this approach would work with Chromium as >> > > > > > well? >> > > > > > >> > > > > >> > > > > i think the Android-IA approach looks good, although it >> > > > > depends on >> > > > > local gralloc0 changes. With gralloc1 on the horizon, I don't >> > > > > know >> > > > > how >> > > > > much sense it makes to extend the predecessor. >> > > > > AFAICT the patch should not cause any issues and it's nicely >> > > > > documented. >> > > > >> > > > >> > > > I had a look at the chromiumos/minigbm implementation, and it >> > > > does not >> > > > contain a gralloc1 implementation as far as I can see. I assume >> > > > that it >> > > > is available somewhere, but maybe not on a public branch. >> > > > >> > > > Would it be possible to make the minigbm gralloc1 impl. public? >> > > > That >> > > > way I could submit a patch mirroring what intel/minigbm does. >> > > > >> > > > If you fine folks as at Google prefer to roll it yourselves, >> > > > just give >> > > > me a poke. >> > > >> > > >> > > There is no gralloc1 implementation for ChromiumOS minigbm and >> > > AFAIK >> > > we don't have any plans of adding one. AFAICT there is nothing we >> > > would gain with it over gralloc0. >> > > >> > > > >> > > > Those are the two options I'm seeing. >> > > > >> > > > As for gralloc0 support, would it be needed? >> > > > >> > > > > >> > > > > Perhaps someone from the Google/CrOS team can assist in >> > > > > making the >> > > > > bug >> > > > > public, although even then it might be better to focus on a >> > > > > 'perfect' >> > > > > gralloc1? >> > > > > >> > > > > IMHO the patch looks perfectly reasonable and we could merge >> > > > > it even, >> > > > > if we were to switch to gralloc1 in the not too distant >> > > > > future ;-) >> > > > >> > > > >> > > > Maybe doing both is reasonable. >> > > >> > > >> > > I believe there isn't much adoption of gralloc1 in the wild. >> > > Android-IA is the first I saw (might have missed something, >> > > though). >> > > Tapani, what was the reason for switching to gralloc1? >> > >> > >> > Main reason was that we thought this is something Android will be >> > moving in >> > to (and deprecating gralloc0). But now if it's gone, it does not >> > make sense >> > to support it. >> > >> > > Could we just support gralloc0 for now in Mesa, make sure the >> > > next >> > > generation IAllocator/IMapper stuff suites our needs and switch >> > > to it >> > > later when it happens? >> >> For CTS/VTS compliance, AIUI, you may have to switch based on the >> release a device is shipping on. >> >> > Yes, this sounds good to me. >> > >> > > (As a side note, I had an idea to create a new interface, >> > > standardized >> > > by Mesa, let's say libdri_android, completely free of any >> > > gralloc-internals. It would have to be exposed additionally by >> > > any >> > > Android that intends to run Mesa. Given the need to deal with 3 >> > > different gralloc versions already, it could be something easier >> > > to >> > > manage.) >> > >> > >> > Makes sense, it is a bit messy and we have bit too much patches on >> > our tree >> > because of these differences. >> >> Seems overly complicated to me. The information needed is within the >> ints in the native_handle in most/all implementations. I don't think >> there's another way to globally store dmabuf metadata unless you have >> a custom interface in your DRM driver. So standardizing to a common >> library implies a common handle struct here. I think the options are: >> >> - common struct definition (native_handle_t + dmabuf fd(s) + width, >> height, stride, format, usage, etc.) >> - common struct plus inline accessor functions >> - common opaque struct plus accessor library > > So these common parts would be much like what currently exists in > minigbm/cros_gralloc_handle.h and gbm_gralloc/gralloc_drm_handle.h > then, but extended with the above suggestions?
Yes, but which part do you think needs to be extended? As we discussed on IRC, I think perhaps we just need to change the handle format field in gralloc_drm_handle.h to use fourcc (aka DRM and GBM formats) instead of the Android format. I think all the users just end up converting it to their own internal format anyway. >> Also, I don't think whatever is standardized should live in Mesa. >> There's a need to support drm_hwcomposer (which has the same >> dependencies as mesa) with non-Mesa GL implementations (yes, vendor >> binaries). > > Excluding Mesa and the composer leaves us with the allocator or > creating a new library. > I would assume that creating a new library is the worse option. Why excluding the composer? If we have to pick, I'd put it there or perhaps libdrm? > On a seperate note, my immidiate concern is making the Android platform > more functional, and in my mind the ^^ patch is minimally invasive and > won't alter other usecases, but simply make mesa more predictable. > I would like to make the bug IDs into full URLs though, but with that > change in place, is the patch acceptable? No issues for me applying the patch. I did just notice something I'm confused about with it. I'll reply separately on that. Rob _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
