[PATCH 03/11] drm: parse ycbcr 420 vdb block

Tue May 9 15:28:16 UTC 2017

On Tue, May 09, 2017 at 02:04:55PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 5/8/2017 10:39 PM, Ville Syrjälä wrote:
> > On Mon, May 08, 2017 at 10:11:53PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 5/8/2017 9:54 PM, Ville Syrjälä wrote:
> >>> On Fri, Apr 07, 2017 at 07:39:20PM +0300, Shashank Sharma wrote:
> >>>> From: Jose Abreu <jose.abreu at synopsys.com>
> >>>>
> >>>> HDMI 2.0 spec adds support for ycbcr420 subsampled output.
> >>>> CEA-861-F adds two new blocks in EDID, to provide information about
> >>>> sink's support for ycbcr420 output.
> >>>>
> >>>> These new blocks are:
> >>>> - ycbcr420 video data (vdb) block: video modes which can be supported
> >>>>     only in ycbcr420 output mode.
> >>>> - ycbcr420 video capability data (vcb) block: video modes which can be
> >>>>     support in ycbcr420 output mode also (along with RGB, YCBCR 444/422 etc)
> >>>>
> >>>> This patch adds parsing and handling of ycbcr420-vdb in the DRM
> >>>> layer.
> >>>>
> >>>> This patch is a modified version of Jose's RFC patch:
> >>>> https://patchwork.kernel.org/patch/9492327/
> >>>> so the authorship is maintained.
> >>>>
> >>>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> >>>> Signed-off-by: Jose Abreu <joabreu at synopsys.com>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_edid.c  | 54 +++++++++++++++++++++++++++++++++++++++++++--
> >>>>    drivers/gpu/drm/drm_modes.c | 10 +++++++--
> >>>>    include/drm/drm_connector.h |  1 +
> >>>>    include/uapi/drm/drm_mode.h |  6 +++++
> >>>>    4 files changed, 67 insertions(+), 4 deletions(-)
> >>> <snip>
> >>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >>>> index 4eeda12..cef76b2 100644
> >>>> --- a/include/drm/drm_connector.h
> >>>> +++ b/include/drm/drm_connector.h
> >>>> @@ -199,6 +199,7 @@ struct drm_display_info {
> >>>>    #define DRM_COLOR_FORMAT_RGB444		(1<<0)
> >>>>    #define DRM_COLOR_FORMAT_YCRCB444	(1<<1)
> >>>>    #define DRM_COLOR_FORMAT_YCRCB422	(1<<2)
> >>>> +#define DRM_COLOR_FORMAT_YCRCB420	(1<<2)
> >>>>    
> >>>>    	/**
> >>>>    	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
> >>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >>>> index 8c67fc0..1e74d8e 100644
> >>>> --- a/include/uapi/drm/drm_mode.h
> >>>> +++ b/include/uapi/drm/drm_mode.h
> >>>> @@ -84,6 +84,12 @@ extern "C" {
> >>>>    #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
> >>>>    #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
> >>>>    #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
> >>>> +/*
> >>>> + * HDMI 2.0
> >>>> + */
> >>>> +#define DRM_MODE_FLAG_420_MASK			(0x03<<23)
> >>>> +#define  DRM_MODE_FLAG_420			(1<<23)
> >>>> +#define  DRM_MODE_FLAG_420_ONLY			(1<<24)
> >>> Adding those would again break the uabi. We can't add new mode flags
> >>> without some kind of client cap.
> >>> But I think we agreed that new user
> >>> space visible mode flags aren't needed, and instad we can keep it all
> >>> internal?
> >> Yep you are right, we had decided to keep it internal, and this whole
> >> patch series is implemented in such a way only, to control everything
> >> through the HDMI output property itself.
> >> But may be I slightly misunderstood that we shouldn't add the flags bits
> >> all together, and I added this flag to differentiate between YCBCR420
> >> and notmal modes.
> >> Can you please suggest me on:
> >> - how to differentiate a YCBCR420 mode with normal mode ? I still need
> >> to add a flag, but not expose it into uapi layer.
> > I guess we can just tack on a few new bools to the end of
> > drm_display_mode. And then when we get the mode passed in by the user
> > we'll have to check whether that mode matches any CEA mode and
> > then look up the correct YCbCr 4:2:0 mode for it.
> seems good to me, I can add a is_ycbcr420 flag, and we need not to 
> bother about converting it to drm_mode_modeinfo as we are keeping it 
> internal.
> >
> > Hmm. Actually, that probably means that it isn't sufficient to
> > actually store this information on the modes we have on the connector's
> > mode list, because that list has been filtered and so may not actually
> > have all the modes that were declared in the EDID.
> I dint get this point,  Why do you think its not sufficient ? Do we need 
> to care about the modes which are getting rejected from the driver ?

Yes, that was my worry. In general I don't think connector->modes should
ever be used for mode validation or state computation. Eg. if fbdev
handled the previus hotplug connector->modes will have been filtered
based on the size of the fb_helper framebuffer, and then a new master
might just blindly come in and blindly set a new mode without doing a
full connector probe.

> I guess they cant be applied anyways.  Do you think we will miss some of 
> the YCBCR modes due to mode filtering ?
> > So I'm thinking we
> > should perhaps make the bitmap parsed from the Y402CMDB index the
> > full CEA mode list. That we can just lookup the matching VIC for
> > the user provided mode and check whether the bit for that VIC
> > indicates 4:2:0 support. And maybe we can handle Y420VDB in exactly
> > the same way (ie. just a second bitmap). That would have the additional
> > nice feature that the maximum length of those bitmaps is well defined
> > (at most 256 VICs).
> >
> We can do this, but do we really need 2 bitmaps ? A YCBCR420 support is 
> same whether its coming from VCB or VDB, we just need a ORing of these 
> supports.

We'll need to know if the mode is one of the "4:2:0 only" modes so that
we'll automagically pick 4:2:0 whenever the user asks for this mode.
And we'll need to know whether the mode is one of the "4:2:0 too" modes
when the user asks for 4:2:0 for explicitly. The latter case of course
needs a new property for that purpose.

> Even in the current implementation, I have been using only the 
> YCBCR420_MASK to identify support.
> 
> - Shashank

-- 
Ville Syrjälä
Intel OTC