| Message ID | 1520378352-31260-1-git-send-email-john.stultz@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hey John, On 03/07/2018 12:19 AM, John Stultz wrote: > As suggested by Alexandru-Cosmin Gheorghe: > > ConvertHALFormatToDrm logic would work only for 1 plane formats, > and probably gets rejected by drmModeAddFb2, but to save > debugging time maybe it worth removing DRM_FORMAT_YVU420 from > ConvertHALFormatToDrm and checking it's return code. > > So this patch tries to do this. > > Cc: Marissa Wall <marissaw@google.com> > Cc: Sean Paul <seanpaul@google.com> > Cc: Dmitry Shmidt <dimitrysh@google.com> > Cc: Robert Foss <robert.foss@collabora.com> > Cc: Matt Szczesiak <matt.szczesiak@arm.com> > Cc: Liviu Dudau <Liviu.Dudau@arm.com> > Cc: David Hanna <david.hanna11@gmail.com> > Cc: Rob Herring <rob.herring@linaro.org> > Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > platformdrmgeneric.cpp | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/platformdrmgeneric.cpp b/platformdrmgeneric.cpp > index 741d42b..33f1ea0 100644 > --- a/platformdrmgeneric.cpp > +++ b/platformdrmgeneric.cpp > @@ -76,8 +76,6 @@ uint32_t DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { > return DRM_FORMAT_ABGR8888; > case HAL_PIXEL_FORMAT_RGB_565: > return DRM_FORMAT_BGR565; > - case HAL_PIXEL_FORMAT_YV12: > - return DRM_FORMAT_YVU420; I'm not sure I understand the rationale for removing YVU420. > default: > ALOGE("Cannot convert hal format to drm format %u", hal_format); > return -EINVAL; > @@ -88,10 +86,15 @@ EGLImageKHR DrmGenericImporter::ImportImage(EGLDisplay egl_display, buffer_handl > gralloc_drm_handle_t *gr_handle = gralloc_drm_handle(handle); > if (!gr_handle) > return NULL; > + > + EGLint fmt = ConvertHalFormatToDrm(gr_handle->format); > + if (fmt < 0) > + return NULL; > + > EGLint attr[] = { > EGL_WIDTH, gr_handle->width, > EGL_HEIGHT, gr_handle->height, > - EGL_LINUX_DRM_FOURCC_EXT, (EGLint)ConvertHalFormatToDrm(gr_handle->format), > + EGL_LINUX_DRM_FOURCC_EXT, fmt, > EGL_DMA_BUF_PLANE0_FD_EXT, gr_handle->prime_fd, > EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0, > EGL_DMA_BUF_PLANE0_PITCH_EXT, gr_handle->stride, > @@ -112,10 +115,14 @@ int DrmGenericImporter::ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) { > return ret; > } > > + uint32_t fmt = ConvertHalFormatToDrm(gr_handle->format); > + if (fmt < 0) > + return fmt; > + > memset(bo, 0, sizeof(hwc_drm_bo_t)); > bo->width = gr_handle->width; > bo->height = gr_handle->height; > - bo->format = ConvertHalFormatToDrm(gr_handle->format); > + bo->format = fmt; > bo->usage = gr_handle->usage; > bo->pitches[0] = gr_handle->stride; > bo->gem_handles[0] = gem_handle; >
On Thu, Mar 8, 2018 at 3:10 AM, Robert Foss <robert.foss@collabora.com> wrote: > Hey John, > > > On 03/07/2018 12:19 AM, John Stultz wrote: >> >> As suggested by Alexandru-Cosmin Gheorghe: >> >> ConvertHALFormatToDrm logic would work only for 1 plane formats, >> and probably gets rejected by drmModeAddFb2, but to save >> debugging time maybe it worth removing DRM_FORMAT_YVU420 from >> ConvertHALFormatToDrm and checking it's return code. >> >> So this patch tries to do this. >> >> Cc: Marissa Wall <marissaw@google.com> >> Cc: Sean Paul <seanpaul@google.com> >> Cc: Dmitry Shmidt <dimitrysh@google.com> >> Cc: Robert Foss <robert.foss@collabora.com> >> Cc: Matt Szczesiak <matt.szczesiak@arm.com> >> Cc: Liviu Dudau <Liviu.Dudau@arm.com> >> Cc: David Hanna <david.hanna11@gmail.com> >> Cc: Rob Herring <rob.herring@linaro.org> >> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> >> Signed-off-by: John Stultz <john.stultz@linaro.org> >> --- >> platformdrmgeneric.cpp | 15 +++++++++++---- >> 1 file changed, 11 insertions(+), 4 deletions(-) >> >> diff --git a/platformdrmgeneric.cpp b/platformdrmgeneric.cpp >> index 741d42b..33f1ea0 100644 >> --- a/platformdrmgeneric.cpp >> +++ b/platformdrmgeneric.cpp >> @@ -76,8 +76,6 @@ uint32_t >> DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { >> return DRM_FORMAT_ABGR8888; >> case HAL_PIXEL_FORMAT_RGB_565: >> return DRM_FORMAT_BGR565; >> - case HAL_PIXEL_FORMAT_YV12: >> - return DRM_FORMAT_YVU420; > > > I'm not sure I understand the rationale for removing YVU420. Mostly its on Alexandru's suggestion, as I don't have any experience w/ using YVU420. Per his suggestion, my sense was that since its a multi-plane format it was expected to fail with drmModeAddFB2(). If that's incorrect, I'm fine with dropping this change. thanks -john
On Thu, Mar 08, 2018 at 11:43:25AM -0800, John Stultz wrote: > On Thu, Mar 8, 2018 at 3:10 AM, Robert Foss <robert.foss@collabora.com> wrote: > > Hey John, > > > > > > On 03/07/2018 12:19 AM, John Stultz wrote: > >> > >> As suggested by Alexandru-Cosmin Gheorghe: > >> > >> ConvertHALFormatToDrm logic would work only for 1 plane formats, > >> and probably gets rejected by drmModeAddFb2, but to save > >> debugging time maybe it worth removing DRM_FORMAT_YVU420 from > >> ConvertHALFormatToDrm and checking it's return code. > >> > >> So this patch tries to do this. > >> > >> Cc: Marissa Wall <marissaw@google.com> > >> Cc: Sean Paul <seanpaul@google.com> > >> Cc: Dmitry Shmidt <dimitrysh@google.com> > >> Cc: Robert Foss <robert.foss@collabora.com> > >> Cc: Matt Szczesiak <matt.szczesiak@arm.com> > >> Cc: Liviu Dudau <Liviu.Dudau@arm.com> > >> Cc: David Hanna <david.hanna11@gmail.com> > >> Cc: Rob Herring <rob.herring@linaro.org> > >> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> > >> Signed-off-by: John Stultz <john.stultz@linaro.org> > >> --- > >> platformdrmgeneric.cpp | 15 +++++++++++---- > >> 1 file changed, 11 insertions(+), 4 deletions(-) > >> > >> diff --git a/platformdrmgeneric.cpp b/platformdrmgeneric.cpp > >> index 741d42b..33f1ea0 100644 > >> --- a/platformdrmgeneric.cpp > >> +++ b/platformdrmgeneric.cpp > >> @@ -76,8 +76,6 @@ uint32_t > >> DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { > >> return DRM_FORMAT_ABGR8888; > >> case HAL_PIXEL_FORMAT_RGB_565: > >> return DRM_FORMAT_BGR565; > >> - case HAL_PIXEL_FORMAT_YV12: > >> - return DRM_FORMAT_YVU420; > > > > > > I'm not sure I understand the rationale for removing YVU420. > > Mostly its on Alexandru's suggestion, as I don't have any experience > w/ using YVU420. Per his suggestion, my sense was that since its a > multi-plane format it was expected to fail with drmModeAddFB2(). > > If that's incorrect, I'm fine with dropping this change. > My line of thought was both DrmGenericImporter::ImportBuffer and HisiImporter::ConvertHalFormatToDrm don't work for planar formats(YVU420), so ConvertHalFormatToDrm should convert only the formats supported by the Importer. I realize now, that I might be wrong since this would also affect ImportImage, however at first sight DrmGenericImporter::ImportImage doesn't seem to support multi-planar as well, isn't it? > thanks > -john
Hi John, On 8 March 2018 at 19:43, John Stultz <john.stultz@linaro.org> wrote: > On Thu, Mar 8, 2018 at 3:10 AM, Robert Foss <robert.foss@collabora.com> wrote: >>> @@ -76,8 +76,6 @@ uint32_t >>> DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { >>> return DRM_FORMAT_ABGR8888; >>> case HAL_PIXEL_FORMAT_RGB_565: >>> return DRM_FORMAT_BGR565; >>> - case HAL_PIXEL_FORMAT_YV12: >>> - return DRM_FORMAT_YVU420; >> >> I'm not sure I understand the rationale for removing YVU420. > > Mostly its on Alexandru's suggestion, as I don't have any experience > w/ using YVU420. Per his suggestion, my sense was that since its a > multi-plane format it was expected to fail with drmModeAddFB2(). > > If that's incorrect, I'm fine with dropping this change. drmModeAddFB2 absolutely works with multi-planar formats. I have no idea about the internals of HWC or why multi-planar formats aren't supported there though. Cheers, Daniel
Hi Daniel, On Fri, Mar 09, 2018 at 09:29:24AM +0000, Daniel Stone wrote: > Hi John, > > On 8 March 2018 at 19:43, John Stultz <john.stultz@linaro.org> wrote: > > On Thu, Mar 8, 2018 at 3:10 AM, Robert Foss <robert.foss@collabora.com> wrote: > >>> @@ -76,8 +76,6 @@ uint32_t > >>> DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { > >>> return DRM_FORMAT_ABGR8888; > >>> case HAL_PIXEL_FORMAT_RGB_565: > >>> return DRM_FORMAT_BGR565; > >>> - case HAL_PIXEL_FORMAT_YV12: > >>> - return DRM_FORMAT_YVU420; > >> > >> I'm not sure I understand the rationale for removing YVU420. > > > > Mostly its on Alexandru's suggestion, as I don't have any experience > > w/ using YVU420. Per his suggestion, my sense was that since its a > > multi-plane format it was expected to fail with drmModeAddFB2(). > > > > If that's incorrect, I'm fine with dropping this change. > > drmModeAddFB2 absolutely works with multi-planar formats. I have no > idea about the internals of HWC or why multi-planar formats aren't > supported there though. > drmModeAddFb2 is fine, it definitely works if you pass the right data to it. Which I don't think ImportBuffer does for DRM_FORMAT_YVU420, it populates just one buffer handle. > Cheers, > Daniel Regards, Alex Gheorghe
Hey, On 03/09/2018 10:55 AM, Alexandru-Cosmin Gheorghe wrote: > Hi Daniel, > > On Fri, Mar 09, 2018 at 09:29:24AM +0000, Daniel Stone wrote: >> Hi John, >> >> On 8 March 2018 at 19:43, John Stultz <john.stultz@linaro.org> wrote: >>> On Thu, Mar 8, 2018 at 3:10 AM, Robert Foss <robert.foss@collabora.com> wrote: >>>>> @@ -76,8 +76,6 @@ uint32_t >>>>> DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { >>>>> return DRM_FORMAT_ABGR8888; >>>>> case HAL_PIXEL_FORMAT_RGB_565: >>>>> return DRM_FORMAT_BGR565; >>>>> - case HAL_PIXEL_FORMAT_YV12: >>>>> - return DRM_FORMAT_YVU420; >>>> >>>> I'm not sure I understand the rationale for removing YVU420. >>> >>> Mostly its on Alexandru's suggestion, as I don't have any experience >>> w/ using YVU420. Per his suggestion, my sense was that since its a >>> multi-plane format it was expected to fail with drmModeAddFB2(). >>> >>> If that's incorrect, I'm fine with dropping this change. >> >> drmModeAddFB2 absolutely works with multi-planar formats. I have no >> idea about the internals of HWC or why multi-planar formats aren't >> supported there though. >> > drmModeAddFb2 is fine, it definitely works if you pass the right data to it. > Which I don't think ImportBuffer does for DRM_FORMAT_YVU420, it > populates just one buffer handle. > So maybe the proper solution is to split ConvertHalFormatToDrm() into two parts, one for doing format conversion, and one for doing verifying that a format is supported. Additionally, maybe the better place for a format conversion function to live is in libdrm/android/gralloc_handle.h, since it will come in handy in other projects too. >> Cheers, >> Daniel > > Regards, > Alex Gheorghe >
On Fri, Mar 09, 2018 at 12:06:09PM +0100, Robert Foss wrote: > Hey, > > > On 03/09/2018 10:55 AM, Alexandru-Cosmin Gheorghe wrote: > >Hi Daniel, > > > >On Fri, Mar 09, 2018 at 09:29:24AM +0000, Daniel Stone wrote: > >>Hi John, > >> > >>On 8 March 2018 at 19:43, John Stultz <john.stultz@linaro.org> wrote: > >>>On Thu, Mar 8, 2018 at 3:10 AM, Robert Foss <robert.foss@collabora.com> wrote: > >>>>>@@ -76,8 +76,6 @@ uint32_t > >>>>>DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { > >>>>> return DRM_FORMAT_ABGR8888; > >>>>> case HAL_PIXEL_FORMAT_RGB_565: > >>>>> return DRM_FORMAT_BGR565; > >>>>>- case HAL_PIXEL_FORMAT_YV12: > >>>>>- return DRM_FORMAT_YVU420; > >>>> > >>>>I'm not sure I understand the rationale for removing YVU420. > >>> > >>>Mostly its on Alexandru's suggestion, as I don't have any experience > >>>w/ using YVU420. Per his suggestion, my sense was that since its a > >>>multi-plane format it was expected to fail with drmModeAddFB2(). > >>> > >>>If that's incorrect, I'm fine with dropping this change. > >> > >>drmModeAddFB2 absolutely works with multi-planar formats. I have no > >>idea about the internals of HWC or why multi-planar formats aren't > >>supported there though. > >> > >drmModeAddFb2 is fine, it definitely works if you pass the right data to it. > >Which I don't think ImportBuffer does for DRM_FORMAT_YVU420, it > >populates just one buffer handle. > > > > So maybe the proper solution is to split ConvertHalFormatToDrm() into two parts, > one for doing format conversion, and one for doing verifying that a format > is supported. > > Additionally, maybe the better place for a format conversion function to > live is in libdrm/android/gralloc_handle.h, since it will come in handy in > other projects too. > I agree, it would make much more sense to have the format conversion function in gralloc_handle.h, since it doesn't/shouldn't depend at all of the importer type. > >>Cheers, > >>Daniel > > > >Regards, > >Alex Gheorghe > >
diff --git a/platformdrmgeneric.cpp b/platformdrmgeneric.cpp index 741d42b..33f1ea0 100644 --- a/platformdrmgeneric.cpp +++ b/platformdrmgeneric.cpp @@ -76,8 +76,6 @@ uint32_t DrmGenericImporter::ConvertHalFormatToDrm(uint32_t hal_format) { return DRM_FORMAT_ABGR8888; case HAL_PIXEL_FORMAT_RGB_565: return DRM_FORMAT_BGR565; - case HAL_PIXEL_FORMAT_YV12: - return DRM_FORMAT_YVU420; default: ALOGE("Cannot convert hal format to drm format %u", hal_format); return -EINVAL; @@ -88,10 +86,15 @@ EGLImageKHR DrmGenericImporter::ImportImage(EGLDisplay egl_display, buffer_handl gralloc_drm_handle_t *gr_handle = gralloc_drm_handle(handle); if (!gr_handle) return NULL; + + EGLint fmt = ConvertHalFormatToDrm(gr_handle->format); + if (fmt < 0) + return NULL; + EGLint attr[] = { EGL_WIDTH, gr_handle->width, EGL_HEIGHT, gr_handle->height, - EGL_LINUX_DRM_FOURCC_EXT, (EGLint)ConvertHalFormatToDrm(gr_handle->format), + EGL_LINUX_DRM_FOURCC_EXT, fmt, EGL_DMA_BUF_PLANE0_FD_EXT, gr_handle->prime_fd, EGL_DMA_BUF_PLANE0_OFFSET_EXT, 0, EGL_DMA_BUF_PLANE0_PITCH_EXT, gr_handle->stride, @@ -112,10 +115,14 @@ int DrmGenericImporter::ImportBuffer(buffer_handle_t handle, hwc_drm_bo_t *bo) { return ret; } + uint32_t fmt = ConvertHalFormatToDrm(gr_handle->format); + if (fmt < 0) + return fmt; + memset(bo, 0, sizeof(hwc_drm_bo_t)); bo->width = gr_handle->width; bo->height = gr_handle->height; - bo->format = ConvertHalFormatToDrm(gr_handle->format); + bo->format = fmt; bo->usage = gr_handle->usage; bo->pitches[0] = gr_handle->stride; bo->gem_handles[0] = gem_handle;
As suggested by Alexandru-Cosmin Gheorghe: ConvertHALFormatToDrm logic would work only for 1 plane formats, and probably gets rejected by drmModeAddFb2, but to save debugging time maybe it worth removing DRM_FORMAT_YVU420 from ConvertHALFormatToDrm and checking it's return code. So this patch tries to do this. Cc: Marissa Wall <marissaw@google.com> Cc: Sean Paul <seanpaul@google.com> Cc: Dmitry Shmidt <dimitrysh@google.com> Cc: Robert Foss <robert.foss@collabora.com> Cc: Matt Szczesiak <matt.szczesiak@arm.com> Cc: Liviu Dudau <Liviu.Dudau@arm.com> Cc: David Hanna <david.hanna11@gmail.com> Cc: Rob Herring <rob.herring@linaro.org> Cc: Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com> Signed-off-by: John Stultz <john.stultz@linaro.org> --- platformdrmgeneric.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)