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@xxxxxxxxx>
---
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@xxxxxxxxxx>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
