[Intel-gfx] [PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB

[Sharma, Shashank]

Thanks for the review Thierry. My comments inline.

Regards
Shashank
On 2/1/2017 9:40 PM, Thierry Reding wrote:
> On Wed, Feb 01, 2017 at 06:14:38PM +0530, Shashank Sharma wrote:
>> This patch does following:
>> - Adds a new structure (drm_hdmi_info) in drm_display_info.
>>    This structure will be used to save and indicate if sink
>>    supports advance HDMI 2.0 features
> "advanced"
got it.
>
>> - Checks the HF-VSDB block for presence of SCDC, and marks it
>>    in hdmi_info structure.
> "drm_hdmi_info structure"?
yep, sure.
>> - If SCDC is present, checks if sink is capable of generating
>>    scdc read request, and marks it in hdmi_info structure.
> "SCDC" to be consistent and because it's an abbreviation.
Agree.
>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 14 ++++++++++++++
>>   include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 96d3e47..37902e5 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
>>   }
>>   EXPORT_SYMBOL(drm_default_rgb_quant_range);
>>   
>> +static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>> +				 const u8 *hf_vsdb)
>> +{
>> +	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi_info;
>> +
>> +	if (hf_vsdb[6] & 0x80) {
>> +		hdmi->scdc_supported = true;
>> +		if (hf_vsdb[6] & 0x40)
>> +			hdmi->scdc_rr = true;
>> +	}
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector *connector,
>>   
>>   		if (cea_db_is_hdmi_vsdb(db))
>>   			drm_parse_hdmi_vsdb_video(connector, db);
>> +		if (cea_db_is_hdmi_forum_vsdb(db))
>> +			drm_detect_hdmi_scdc(connector, db);
>>   	}
>>   }
>>   
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index e5e1edd..2435598 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -87,6 +87,27 @@ enum subpixel_order {
>>   	SubPixelVerticalRGB,
>>   	SubPixelVerticalBGR,
>>   	SubPixelNone,
>> +
>> +};
>> +
>> +/**
>> + * struct drm_hdmi_info - runtime data about the connected sink
> Maybe "connected HDMI sink"?
Agree.
>> + *
>> + * Describes if a given hdmi display supports advance HDMI 2.0 featutes.
> "HDMI", "advanced", "features"
Oops, got it :-)
>> + * This information is available in CEA-861-F extension blocks (like
>> + * HF-VSDB)
> This should be terminated by a full-stop.
Ok
>> + * For sinks which provide an EDID this can be filled out by calling
>> + * drm_add_edid_modes().
> And maybe make this sentence start right after the one above rather than
> breaking it to the next line.
Ok
> I'm not sure how useful this line is. Most
> driver will be calling drm_add_edid_modes() anyway, but the above makes
> it sound like drm_add_edid_modes() is something you have to explicitly
> call to get these fields parsed.
Mostly a 'yy' and 'p' from the function above, but makes sense, I can 
remove this line.
>> + */
>> +struct drm_hdmi_info {
>> +	/**
>> +	 * @scdc_supported: status control & data channel present.
>> +	 */
>> +	bool scdc_supported;
>> +	/**
>> +	 * @scdc_rr: sink is capable of generating scdc read request.
>> +	 */
>> +	bool scdc_rr;
>>   };
>>   
>>   /**
>> @@ -188,6 +209,11 @@ struct drm_display_info {
>>   	 * @cea_rev: CEA revision of the HDMI sink.
>>   	 */
>>   	u8 cea_rev;
>> +
>> +	/**
>> +	 * @hdmi_info: advance features of a HDMI sink.
>> +	 */
>> +	struct drm_hdmi_info hdmi_info;
> I think we can safely drop the _info suffix on the field name. It's
> already inside a structure that carries this suffix.
Sure, should I call it hdmi_sink OR connected_hdmi ?
- Shashank
>
> Other than that:
>
> Reviewed-by: Thierry Reding <treding at nvidia.com>