| Message ID | 1485953081-7630-4-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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" > - Checks the HF-VSDB block for presence of SCDC, and marks it > in hdmi_info structure. "drm_hdmi_info structure"? > - 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. > > 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"? > + * > + * Describes if a given hdmi display supports advance HDMI 2.0 featutes. "HDMI", "advanced", "features" > + * This information is available in CEA-861-F extension blocks (like > + * HF-VSDB) This should be terminated by a full-stop. > + * 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. 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. > + */ > +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. Other than that: Reviewed-by: Thierry Reding <treding@nvidia.com>
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 > - Checks the HF-VSDB block for presence of SCDC, and marks it > in hdmi_info structure. > - If SCDC is present, checks if sink is capable of generating > scdc read request, and marks it in hdmi_info structure. > > 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 > + * > + * Describes if a given hdmi display supports advance HDMI 2.0 featutes. > + * This information is available in CEA-861-F extension blocks (like > + * HF-VSDB) > + * For sinks which provide an EDID this can be filled out by calling > + * drm_add_edid_modes(). > + */ > +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; Probably worth spelling the thing out. > }; > > /** > @@ -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; > }; > > int drm_display_info_set_bus_formats(struct drm_display_info *info, > -- > 1.9.1
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 ? - Shashank > > Other than that: > > Reviewed-by: Thierry Reding <treding@nvidia.com>
Thanks for the review Ville. My comments inline. Regards Shashank On 2/1/2017 10:03 PM, Ville Syrjälä 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 >> - Checks the HF-VSDB block for presence of SCDC, and marks it >> in hdmi_info structure. >> - If SCDC is present, checks if sink is capable of generating >> scdc read request, and marks it in hdmi_info structure. >> >> 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 >> + * >> + * Describes if a given hdmi display supports advance HDMI 2.0 featutes. >> + * This information is available in CEA-861-F extension blocks (like >> + * HF-VSDB) >> + * For sinks which provide an EDID this can be filled out by calling >> + * drm_add_edid_modes(). >> + */ >> +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; > Probably worth spelling the thing out. Agree. I was afraid it would make it difficult for 80 char stuff, but maybe I can make it scdc->read_req - Shashank >> }; >> >> /** >> @@ -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; >> }; >> >> int drm_display_info_set_bus_formats(struct drm_display_info *info, >> -- >> 1.9.1
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
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 + * + * Describes if a given hdmi display supports advance HDMI 2.0 featutes. + * This information is available in CEA-861-F extension blocks (like + * HF-VSDB) + * For sinks which provide an EDID this can be filled out by calling + * drm_add_edid_modes(). + */ +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; }; int drm_display_info_set_bus_formats(struct drm_display_info *info,
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 - Checks the HF-VSDB block for presence of SCDC, and marks it in hdmi_info structure. - If SCDC is present, checks if sink is capable of generating scdc read request, and marks it in hdmi_info structure. 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(+)