diff mbox series

[RFC,1/2] drm_hwcomposer: Error out on YUV layer as it would fail for single planes

Message ID 1520378352-31260-1-git-send-email-john.stultz@linaro.org
State New
Headers show
Series [RFC,1/2] drm_hwcomposer: Error out on YUV layer as it would fail for single planes | expand

Commit Message

John Stultz March 6, 2018, 11:19 p.m. UTC 
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(-)

-- 
2.7.4

Comments

Robert Foss March 8, 2018, 11:10 a.m. UTC | #1 
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;
>
John Stultz March 8, 2018, 7:43 p.m. UTC | #2 
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
Alexandru-Cosmin Gheorghe March 9, 2018, 9:28 a.m. UTC | #3 
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
Daniel Stone March 9, 2018, 9:29 a.m. UTC | #4 
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
Alexandru-Cosmin Gheorghe March 9, 2018, 9:55 a.m. UTC | #5 
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
Robert Foss March 9, 2018, 11:06 a.m. UTC | #6 
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
>
Alexandru-Cosmin Gheorghe March 12, 2018, 10:48 a.m. UTC | #7 
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 mbox series

Patch

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;