Re: [PATCH v3 04/14] drm/edid: parse YCBCR 420 videomodes from EDID

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jun 15, 2017 at 10:18:40PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 6/15/2017 9:42 PM, Ville Syrjälä wrote:
> > On Thu, Jun 15, 2017 at 09:05:10PM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 6/15/2017 8:13 PM, Ville Syrjälä wrote:
> >>> On Wed, Jun 14, 2017 at 11:17:35PM +0530, Shashank Sharma wrote:
> >>>> HDMI 2.0 spec adds support for YCBCR420 sub-sampled output.
> >>>> CEA-861-F adds two new blocks in EDID's CEA extension blocks,
> >>>> to provide information about sink's YCBCR420 output capabilities.
> >>>>
> >>>> These blocks are:
> >>>>
> >>>> - YCBCR420vdb(YCBCR 420 video data block):
> >>>> This block contains VICs of video modes, which can be sopported only
> >>>> in YCBCR420 output mode (Not in RGB/YCBCR444/422. Its like a normal
> >>>> SVD block, valid for YCBCR420 modes only.
> >>>>
> >>>> - YCBCR420cmdb(YCBCR 420 capability map data block):
> >>>> This block gives information about video modes which can support
> >>>> YCBCR420 output mode also (along with RGB,YCBCR444/422 etc) This
> >>>> block contains a bitmap index of normal svd videomodes, which can
> >>>> support YCBCR420 output too.
> >>>> So if bit 0 from first vcb byte is set, first video mode in the svd
> >>>> list can support YCBCR420 output too. Bit 1 means second video mode
> >>>> from svd list can support YCBCR420 output too, and so on.
> >>>>
> >>>> This patch adds two bitmaps in display's hdmi_info structure, one each
> >>>> for VCB and VDB modes. If the source is HDMI 2.0 capable, this patch
> >>>> adds:
> >>>> - VDB modes (YCBCR 420 only modes) in connector's mode list, also makes
> >>>>     an entry in the vdb_bitmap per vic.
> >>>> - VCB modes (YCBCR 420 also modes) only entry in the vcb_bitmap.
> >>>>
> >>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >>>> Cc: Jose Abreu <joabreu@synopsys.com>
> >>>> Cc: Emil Velikov <emil.l.velikov@gmail.com>
> >>>>
> >>>> V2: Addressed
> >>>>       Review comments from Emil:
> >>>>       - Use 1ULL<<i instead of 1<<i to make sure the output is 64bit.
> >>>>       - Use the suggested method for updating dbmap.
> >>>>       - Add documentation for YCBCR420_vcb_map to fix kbuild warning.
> >>>>
> >>>>       Review comments from Ville:
> >>>>       - Do not expose the YCBCR420 flags in uabi layer, keep it internal.
> >>>>       - Save a map of YCBCR420 modes for future reference.
> >>>>       - Check db length before trying to parse extended tag.
> >>>>       - Add a warning if there are > 64 modes in capability map block.
> >>>>       - Use y420cmdb in function names and macros while dealing with vcb
> >>>>         to be aligned with spec.
> >>>>       - Move the display information parsing block ahead of mode parsing
> >>>>         blocks.
> >>>>
> >>>> V3: Addressed design/review comments from Ville
> >>>>       - Do not add flags in video modes, else we have to expose them to user
> >>>>       - There should not be a UABI change, and kernel should detect the
> >>>>         choice of the output based on type of mode, and the bitmaps.
> >>>>       - Use standard bitops from kernel bitmap header, instead of calculating
> >>>>         bit positions manually.
> >>>>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_edid.c  | 193 ++++++++++++++++++++++++++++++++++++++++++--
> >>>>    include/drm/drm_connector.h |  23 ++++++
> >>>>    2 files changed, 211 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >>>> index 6fd8a98..4953f87 100644
> >>>> --- a/drivers/gpu/drm/drm_edid.c
> >>>> +++ b/drivers/gpu/drm/drm_edid.c
> >>>> @@ -2781,7 +2781,9 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
> >>>>    #define VIDEO_BLOCK     0x02
> >>>>    #define VENDOR_BLOCK    0x03
> >>>>    #define SPEAKER_BLOCK	0x04
> >>>> -#define VIDEO_CAPABILITY_BLOCK	0x07
> >>>> +#define VIDEO_CAPABILITY_BLOCK 0x07
> >>>> +#define VIDEO_DATA_BLOCK_420	0x0E
> >>>> +#define VIDEO_CAP_BLOCK_Y420CMDB 0x0F
> >>>>    #define EDID_BASIC_AUDIO	(1 << 6)
> >>>>    #define EDID_CEA_YCRCB444	(1 << 5)
> >>>>    #define EDID_CEA_YCRCB422	(1 << 4)
> >>>> @@ -3143,15 +3145,103 @@ drm_display_mode_from_vic_index(struct drm_connector *connector,
> >>>>    	return newmode;
> >>>>    }
> >>>>    
> >>>> +/*
> >>>> + * do_ycbcr_420_vdb_modes - Parse YCBCR 420 only modes
> >>>> + * @connector: connector corresponding to the HDMI sink
> >>>> + * @svds: start of the data block of CEA YCBCR 420 VDB
> >>>> + * @len: length of the CEA YCBCR 420 VDB
> >>>> + *
> >>>> + * CEA-861-F has added a new video block called YCBCR 420 Video
> >>>> + * Data Block (ycbcr 420 vdb). This block contains modes which
> >>>> + * support YCBCR 420 HDMI output (only YCBCR 420). This function
> >>>> + * parses the block and adds these modes into connector's mode list.
> >>> Seems a bit verbose. Maybe something like:
> >>> "Parse the CEA-861-F YCbCr 4:2:0 Video Data Block (Y420VDB)
> >>> which contains modes which only support YCbCr 4:2:0 output."
> >> Got it.
> >>>> + */
> >>>> +static int do_ycbcr_420_vdb_modes(struct drm_connector *connector,
> >>>> +				   const u8 *svds, u8 svds_len)
> >>> Probably we could s/ycbcr_420_vdb/y420vdb/ all over to keep things
> >>> shorter and match the terminology in the spec. Same for y420cmdb.
> >> Got it.
> >>>> +{
> >>>> +	int modes = 0, i;
> >>>> +	struct drm_device *dev = connector->dev;
> >>>> +	struct drm_display_mode *newmode;
> >>> This variable can be moved into the loop scope.
> >> Ok.
> >>>> +	struct drm_display_info *info = &connector->display_info;
> >>>> +	struct drm_hdmi_info *hdmi = &info->hdmi;
> >>>> +
> >>>> +	for (i = 0; i < svds_len; i++) {
> >>>> +		u8 vic = svds[i] & 0x7f;
> >>> What's the 0x7f here? Native bit stuff? I'm thinkign we probably want
> >>> some kind of svd_to_vic() function to make sure everyone deals with
> >>> this stuff correctly.
> >> Current VICs are 1 < VIC < 108, so seven bits required to show a VIC.
> >> This is inspired from
> >> drm_mode_from_cea_vic_index().
> >>>> +
> >>>> +		newmode = drm_mode_duplicate(dev, &edid_cea_modes[vic]);
> >>>> +		if (!newmode)
> >>>> +			break;
> >>>> +		/*
> >>>> +		 * ycbcr420_vdb_modes is a fix position 128 bit bitmap.
> >>>> +		 * Every bit here represents a VIC, from CEA-861-F list.
> >>>> +		 * So if bit[n] is set, it indicates vic[n+1] supports
> >>>> +		 * YCBCR420 output. Bit 0 is dummy, as VICs start at 1.
> >>>> +		 * ycbcr420_vcb_modes[0] = |VIC=64 |VIC=63 |.....|VIC=2 |VIC=1 |
> >>>> +		 * ycbcr420_vcb_modes[1] = |VIC=128|VIC=127|.....|VIC=66|VIC=65|
> >>>> +		 */
> >>> I think people can figure out how the standard bitmaps work without
> >>> having to explain it with such detail. If you want to elaborate on the
> >>> contents of the bitmap, then I think the comment should be next to
> >>> where the bitmap is defined.
> >> There is already similar explanation :), I will probably remove this.
> >>>> +		bitmap_set(hdmi->ycbcr420_vdb_modes, vic, 1);
> >>> __set_bit()
> >> Any harm on bitmap_set ? I guess this is specially for bitmaps ...
> >>>> +		drm_mode_probed_add(connector, newmode);
> >>>> +		modes++;
> >>>> +	}
> >>>> +
> >>>> +	if (modes > 0)
> >>>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> >>>> +	return modes;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * drm_add_vcb_modes - Add a YCBCR 420 mode into bitmap
> >>>> + * @connector: connector corresponding to the HDMI sink
> >>>> + * @vic: CEA vic for the video mode to be added in the map
> >>>> + *
> >>>> + * Makes an entry for a videomode in the YCBCR 420 bitmap
> >>>> + */
> >>>> +static void
> >>>> +drm_add_vcb_modes(struct drm_connector *connector, u8 vic)
> >>> The "vcb" term doesn't seem to be in the spec. Should be "cmdb" maybe?
> >>>
> >>> Or maybe we want ycbcr420_vdb_modes and ycbcr420_y420vdb_modes just to
> >>> make it clear to which VDB they relate with.
> >> cmdb seems appropriate, which is aligned to the spec too, will do the
> >> change.
> >>>> +{
> >>>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
> >>>> +
> >>>> +	/* VICs are 1 to 127(107 defined till CEA-861-F) */
> >>>> +	vic &= 127;
> >>>> +
> >>>> +	/*
> >>>> +	 * ycbcr420_vcb_modes is a fix position 128 bit bitmap.
> >>>> +	 * Every bit here represents a VIC, from CEA-861-F list.
> >>>> +	 * So if bit[n] is set, it indicates vic[n+1] supports
> >>>> +	 * YCBCR420 output. Bit 0 is dummy, as VICs start at 1.
> >>>> +	 * ycbcr420_vcb_modes[0] = |VIC=64 |VIC=63 |.....|VIC=2 |VIC=1 |
> >>>> +	 * ycbcr420_vcb_modes[1] = |VIC=128|VIC=127|.....|VIC=66|VIC=65|
> >>>> +	 * Difference between a vcb_mode and vdb_mode is that a vcb_mode
> >>>> +	 * can support other HDMI outputs like RGB/YCBCR444/422 also
> >>>> +	 * along with YCBCR 240, whereas a vdb_mode can only support YCBCR
> >>>> +	 * 420 HDMI output.
> >>>> +	 */
> >>>> +	bitmap_set(hdmi->ycbcr420_vcb_modes, vic, 1);
> >>>> +}
> >>>> +
> >>>>    static int
> >>>>    do_cea_modes(struct drm_connector *connector, const u8 *db, u8 len)
> >>>>    {
> >>>>    	int i, modes = 0;
> >>>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
> >>>>    
> >>>>    	for (i = 0; i < len; i++) {
> >>>>    		struct drm_display_mode *mode;
> >>>>    		mode = drm_display_mode_from_vic_index(connector, db, len, i);
> >>>>    		if (mode) {
> >>>> +			/*
> >>>> +			 * YCBCR420 capability block contains a bitmap which
> >>>> +			 * gives the index of CEA modes from CEA VDB, which
> >>>> +			 * can support YCBCR 420 sampling output also (apart
> >>>> +			 * from RGB/YCBCR444 etc).
> >>>> +			 * For example, if the bit 0 in bitmap is set,
> >>>> +			 * first mode in VDB can support YCBCR420 output too.
> >>>> +			 * Add YCBCR420 modes only if sink is HDMI 2.0 capable.
> >>>> +			 */
> >>>> +			if (connector->is_hdmi2_src &&
> >>> Hmm. I wonder if need this here at all. I guess we do need something for
> >>> the "420 only" modes, but not sure if it should be here or if we should
> >>> reject them only during mode validation. The latter would be nice
> >>> because it would then leave a message into the log that the mode was
> >>> present but was rejected.
> >> Seems like a good idea, the only benefit of doing this is, this is in
> >> DRM layer, and will protect other drivers
> >> from accidentally adding 420_vcb modes. Mode valid would be internal to
> >> driver, and applicable in I915 layer.
> > No, we should put into the probe helpers standard mode validation stuff.
> > No driver specifics needed apart from setting the support_ycbcr420 or
> > whatever flag.
> Ok, got it.
> >
> >> So your wish is my command :)
> >>> I'm thinking we should probably call this
> >>> flag ycbcr420_allowed or something like that to match the other
> >>> foo_allowed flags we have in the connector for mode validation.
> >> I don't like that honestly. I am hoping that this flag would be used
> >> further, to identify a HDMI 2.0 src over HDMI 1.4
> > And what happens when when you have an otherwise HDMI 2.0 capable
> > source that can't output YCbCr 4:2:0?
> This makes equation difficult, but in this way, we might need flags for 
> all sub-features like
> 4k@60, scrambling, scdc, scdc_rr, ycbcr420. May be, I will add a comment 

Yes, we should check for features, not for some standard version that
implements some/all of those features. But not convinced we need any
featur flags for the other things you listed. Aren't we already doing
all those things anyway?

> that if you
> enable this flag, this is going to happen or something ?
> >> source, beyond YCBCR420 feature, and at that time, it might be more
> >> suitable to have hdmi_2_src. Does it make sense ?
> >>> So I think this could perhaps just be an uncoditional
> >>> if (test_bit(i, ...))
> >>> 	__set_bit(vic, ...);
> >>>
> >>>> +				test_bit(i, &hdmi->ycbcr420_vcb_map))
> >>>> +				drm_add_vcb_modes(connector, db[i]);
> >>>> +
> >>>>    			drm_mode_probed_add(connector, mode);
> >>>>    			modes++;
> >>>>    		}
> >>>> @@ -3427,6 +3517,12 @@ do_hdmi_vsdb_modes(struct drm_connector *connector, const u8 *db, u8 len,
> >>>>    }
> >>>>    
> >>>>    static int
> >>>> +cea_db_extended_tag(const u8 *db)
> >>>> +{
> >>>> +	return db[1];
> >>>> +}
> >>> Could you clean up the current extended tag usage as a separate
> >>> prep patch? Would be nice to avoid the confusion of calling the
> >>> "Use extended tag" tag VIDEO_CAPABILITY_BLOCK.
> >> Sure, adding in my to-do list.
> >>>> +
> >>>> +static int
> >>>>    cea_db_payload_len(const u8 *db)
> >>>>    {
> >>>>    	return db[0] & 0x1f;
> >>>> @@ -3487,9 +3583,81 @@ static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
> >>>>    	return oui == HDMI_FORUM_IEEE_OUI;
> >>>>    }
> >>>>    
> >>>> +static bool cea_db_is_ycbcr_420_cmdb(const u8 *db)
> >>>> +{
> >>>> +	u8 len = cea_db_payload_len(db);
> >>> 'len' seems like a fairly pointless variable since it's used exactly
> >>> once.
> >> Agree,
> >>>> +
> >>>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> >>>> +		return false;
> >>>> +
> >>>> +	if (!len)
> >>>> +		return false;
> >>>> +
> >>>> +	if (cea_db_extended_tag(db) != VIDEO_CAP_BLOCK_Y420CMDB)
> >>>> +		return false;
> >>>> +
> >>>> +	return true;
> >>>> +}
> >>>> +
> >>>> +static bool cea_db_is_ycbcr_420_vdb(const u8 *db)
> >>>> +{
> >>>> +	if (cea_db_tag(db) != VIDEO_CAPABILITY_BLOCK)
> >>>> +		return false;
> >>>> +
> >>> Length check missing.
> >> Oops, got it.
> >>>> +	if (cea_db_extended_tag(db) != VIDEO_DATA_BLOCK_420)
> >>>> +		return false;
> >>>> +
> >>>> +	return true;
> >>>> +}
> >>>> +
> >>>>    #define for_each_cea_db(cea, i, start, end) \
> >>>>    	for ((i) = (start); (i) < (end) && (i) + cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) + 1)
> >>>>    
> >>>> +static void drm_parse_y420cmdb_bitmap(struct drm_connector *connector,
> >>>> +				     const u8 *db)
> >>>> +{
> >>>> +	struct drm_display_info *info = &connector->display_info;
> >>>> +	struct drm_hdmi_info *hdmi = &info->hdmi;
> >>>> +	u8 map_len = cea_db_payload_len(db) - 1;
> >>>> +	u8 count;
> >>>> +	u64 map = 0;
> >>>> +
> >>>> +	if (!db)
> >>>> +		return;
> >>> Can't happen.
> >> I assumed you were convince on my previous (lame) excuse of corrupt db :)
> >> Will remove this check.
> >>>> +
> >>>> +	if (map_len == 0) {
> >>>> +		/* All CEA modes support ycbcr420 sampling also.*/
> >>>> +		hdmi->ycbcr420_vcb_map = U64_MAX;
> >>>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> >>>> +		return;
> >>>> +	}
> >>>> +
> >>>> +	/*
> >>>> +	 * This map indicates which of the existing CEA block modes
> >>>> +	 * from VDB can support YCBCR420 output too. So if bit=0 is
> >>>> +	 * set, first mode from VDB can support YCBCR420 output too.
> >>>> +	 * We will parse and keep this map, before parsing VDB itself
> >>>> +	 * to avoid going through the same block again and again.
> >>>> +	 *
> >>>> +	 * Spec is not clear about max possible size of this block.
> >>>> +	 * Clamping max bitmap block size at 8 bytes. Every byte can
> >>>> +	 * address 8 CEA modes, in this way this map can address
> >>>> +	 * 8*8 = first 64 SVDs.
> >>>> +	 */
> >>>> +	if (WARN_ON_ONCE(map_len > 8))
> >>>> +		map_len = 8;
> >>>> +
> >>>> +	for (count = 0; count < map_len; count++)
> >>>> +		map |= (u64)db[2 + count] << (8 * count);
> >>>> +
> >>>> +	if (map) {
> >>>> +		DRM_DEBUG_KMS("Sink supports ycbcr 420\n");
> >>> If we want to print something about the sinks color capabilities I think
> >>> it should be in central place instead of being sprinkled all over the
> >>> place.
> >> Yes, seems like a good idea. I will remove this, and may be add a
> >> separate patch to dump
> >> sink capabilities.
> >>>> +		info->color_formats |= DRM_COLOR_FORMAT_YCRCB420;
> >>>> +	}
> >>>> +
> >>>> +	hdmi->ycbcr420_vcb_map = map;
> >>>> +}
> >>>> +
> >>>>    static int
> >>>>    add_cea_modes(struct drm_connector *connector, struct edid *edid)
> >>>>    {
> >>>> @@ -3512,10 +3680,17 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
> >>>>    				video = db + 1;
> >>>>    				video_len = dbl;
> >>>>    				modes += do_cea_modes(connector, video, dbl);
> >>>> -			}
> >>>> -			else if (cea_db_is_hdmi_vsdb(db)) {
> >>>> +			} else if (cea_db_is_hdmi_vsdb(db)) {
> >>>>    				hdmi = db;
> >>>>    				hdmi_len = dbl;
> >>>> +			} else if (cea_db_is_ycbcr_420_vdb(db) &&
> >>>> +					connector->is_hdmi2_src) {
> >>> Broken indentation in a bunch of places.
> >> Damn, I thought I am improving :(
> >>>> +				const u8 *vdb420 = &db[2];
> >>>> +
> >>>> +				/* Add 4:2:0(only) modes present in EDID */
> >>>> +				modes += do_ycbcr_420_vdb_modes(connector,
> >>>> +								vdb420,
> >>>> +								dbl - 1);
> >>>>    			}
> >>>>    		}
> >>>>    	}
> >>>> @@ -4196,6 +4371,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
> >>>>    			drm_parse_hdmi_vsdb_video(connector, db);
> >>>>    		if (cea_db_is_hdmi_forum_vsdb(db))
> >>>>    			drm_parse_hdmi_forum_vsdb(connector, db);
> >>>> +		if (cea_db_is_ycbcr_420_cmdb(db))
> >>>> +			drm_parse_y420cmdb_bitmap(connector, db);
> >>>>    	}
> >>>>    }
> >>>>    
> >>>> @@ -4430,6 +4607,13 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> >>>>    	quirks = edid_get_quirks(edid);
> >>>>    
> >>>>    	/*
> >>>> +	 * CEA-861-F adds ycbcr capability map block, for HDMI 2.0 sinks.
> >>>> +	 * To avoid multiple parsing of same block, lets get the sink info
> >>>> +	 * before parsing CEA modes.
> >>>> +	 */
> >>>> +	drm_add_display_info(connector, edid);
> >>>> +
> >>>> +	/*
> >>>>    	 * EDID spec says modes should be preferred in this order:
> >>>>    	 * - preferred detailed mode
> >>>>    	 * - other detailed modes from base block
> >>>> @@ -4450,14 +4634,13 @@ int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid)
> >>>>    	num_modes += add_cea_modes(connector, edid);
> >>>>    	num_modes += add_alternate_cea_modes(connector, edid);
> >>>>    	num_modes += add_displayid_detailed_modes(connector, edid);
> >>>> +
> >>>>    	if (edid->features & DRM_EDID_FEATURE_DEFAULT_GTF)
> >>>>    		num_modes += add_inferred_modes(connector, edid);
> >>>>    
> >>>>    	if (quirks & (EDID_QUIRK_PREFER_LARGE_60 | EDID_QUIRK_PREFER_LARGE_75))
> >>>>    		edid_fixup_preferred(connector, quirks);
> >>>>    
> >>>> -	drm_add_display_info(connector, edid);
> >>>> -
> >>> I think this could be a separate patch, just in case it causes a
> >>> regression.
> >> Got it.
> >>>>    	if (quirks & EDID_QUIRK_FORCE_6BPC)
> >>>>    		connector->display_info.bpc = 6;
> >>>>    
> >>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> >>>> index 390319c..5a47c33 100644
> >>>> --- a/include/drm/drm_connector.h
> >>>> +++ b/include/drm/drm_connector.h
> >>>> @@ -137,6 +137,28 @@ struct drm_scdc {
> >>>>    struct drm_hdmi_info {
> >>>>    	/** @scdc: sink's scdc support and capabilities */
> >>>>    	struct drm_scdc scdc;
> >>>> +
> >>>> +	/**
> >>>> +	 * @ycbcr420_vdb_modes: bitmap of modes which can support ycbcr420
> >>>> +	 * output only (not normal RGB/YCBCR444/422 outputs). There are total
> >>>> +	 * 107 VICs defined by CEA-861-F spec, so the size is 128 bits to map
> >>>> +	 * upto 128 VICs;
> >>>> +	 */
> >>>> +	unsigned long ycbcr420_vdb_modes[BITS_TO_LONGS(128)];
> >>>> +
> >>>> +	/**
> >>>> +	 * @ycbcr420_vcb_modes: bitmap of modes which can support ycbcr420
> >>>> +	 * output also, along with normal HDMI outputs. There are total 107
> >>>> +	 * VICs defined by CEA-861-F spec, so the size is 128 bits to map upto
> >>>> +	 * 128 VICs;
> >>>> +	 */
> >>>> +	unsigned long ycbcr420_vcb_modes[BITS_TO_LONGS(128)];
> >>>> +
> >>>> +	/** @ycbcr420_vcb_map: bitmap of SVD index, to extraxt vcb modes */
> >>>> +	unsigned long ycbcr420_vcb_map;
> >>>> +
> >>>> +	/** @ycbcr420_dc_modes: bitmap of deep color support index */
> >>>> +	u8 ycbcr420_dc_modes;
> >>> unused
> >> Its used. We are parsing 420 deep color info and checking the it in
> >> hdmi_12bpc_possible()
> > Not in this patch.
> Yes, actually that's in next patch (parse ycbcr_420_deep_color). I will 
> move it in next patch.
> 
> - Shashank
> >
> >> Shashank
> >>>>    };
> >>>>    
> >>>>    /**
> >>>> @@ -200,6 +222,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<<3)
> >>>>    
> >>>>    	/**
> >>>>    	 * @color_formats: HDMI Color formats, selects between RGB and YCrCb
> >>>> -- 
> >>>> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx