On Thu, Feb 02, 2017 at 10:58:43AM +0530, Sharma, Shashank wrote:
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@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 ?
No, I think just plain "hdmi" would be fine. This is part of drm_display_info, which kind of implies that it's a sink, and I think it's also fair to assume that this isn't valid if nothing's connected.
Thierry