| Message ID | 1486389566-28613-4-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Shashank, On 06-02-2017 13:59, 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 advanced HDMI 2.0 features > - Adds another structure drm_scdc within drm_hdmi_info, to > reflect scdc support and capabilities in connected HDMI 2.0 sink. > - Checks the HF-VSDB block for presence of SCDC, and marks it > in scdc structure > - If SCDC is present, checks if sink is capable of generating > SCDC read request, and marks it in scdc structure. > > V2: Addressed review comments > Thierry: > - Fix typos in commit message and make abbreviation consistent > across the commit message. > - Change structure object name from hdmi_info -> hdmi > - Fix typos and abbreviations in description of structure drm_hdmi_info > end the description with a full stop. > - Create a structure drm_scdc, and keep all information related to SCDC > register set (supported, read request supported) etc in it. > > Ville: > - Change rr -> read_request > - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all > of HF-VSDB parsing can be kept in same function, in incremental > patches. > > Reviewed-by: Thierry Reding <treding@nvidia.com> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/drm_edid.c | 14 ++++++++++++++ > include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 96d3e47..a487b80 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_parse_hdmi_forum_vsdb(struct drm_connector *connector, > + const u8 *hf_vsdb) > +{ > + struct drm_hdmi_info *hdmi = &connector->display_info.hdmi; > + > + if (hf_vsdb[6] & 0x80) { BIT(7) ? > + hdmi->scdc.supported = true; > + if (hf_vsdb[6] & 0x40) BIT(6) ? > + hdmi->scdc.read_request = 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_parse_hdmi_forum_vsdb(connector, db); > } > } > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index e5e1edd..6d5304e 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -87,6 +87,34 @@ enum subpixel_order { > SubPixelVerticalRGB, > SubPixelVerticalBGR, > SubPixelNone, > + > +}; > + > +/* > + * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink > + * > + * Provides SCDC register support and capabilities related information on a > + * HDMI 2.0 sink. In case of a HDMI 1.4 sink, all parameter must be 0. > + */ > +struct drm_scdc { > + /** > + * @supported: status control & data channel present. > + */ > + bool supported; > + /** > + * @read_request: sink is capable of generating scdc read request. > + */ > + bool read_request; > +}; > + > +/** > + * struct drm_hdmi_info - runtime information about the connected HDMI sink > + * > + * Describes if a given display supports advanced HDMI 2.0 features. > + * This information is available in CEA-861-F extension blocks (like HF-VSDB). > + */ > +struct drm_hdmi_info { > + struct drm_scdc scdc; > }; > > /** > @@ -188,6 +216,11 @@ struct drm_display_info { > * @cea_rev: CEA revision of the HDMI sink. > */ > u8 cea_rev; > + > + /** > + * @hdmi: advance features of a HDMI sink. Maybe change to the same general description you used above: "Runtime information about the connected HDMI sink" ? > + */ > + struct drm_hdmi_info hdmi; > }; > > int drm_display_info_set_bus_formats(struct drm_display_info *info, Best regards, Jose Miguel Abreu
Regards Shashank On 2/7/2017 4:31 PM, Jose Abreu wrote: > Hi Shashank, > > > > On 06-02-2017 13:59, 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 advanced HDMI 2.0 features >> - Adds another structure drm_scdc within drm_hdmi_info, to >> reflect scdc support and capabilities in connected HDMI 2.0 sink. >> - Checks the HF-VSDB block for presence of SCDC, and marks it >> in scdc structure >> - If SCDC is present, checks if sink is capable of generating >> SCDC read request, and marks it in scdc structure. >> >> V2: Addressed review comments >> Thierry: >> - Fix typos in commit message and make abbreviation consistent >> across the commit message. >> - Change structure object name from hdmi_info -> hdmi >> - Fix typos and abbreviations in description of structure drm_hdmi_info >> end the description with a full stop. >> - Create a structure drm_scdc, and keep all information related to SCDC >> register set (supported, read request supported) etc in it. >> >> Ville: >> - Change rr -> read_request >> - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all >> of HF-VSDB parsing can be kept in same function, in incremental >> patches. >> >> Reviewed-by: Thierry Reding <treding@nvidia.com> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/drm_edid.c | 14 ++++++++++++++ >> include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ >> 2 files changed, 47 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >> index 96d3e47..a487b80 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_parse_hdmi_forum_vsdb(struct drm_connector *connector, >> + const u8 *hf_vsdb) >> +{ >> + struct drm_hdmi_info *hdmi = &connector->display_info.hdmi; >> + >> + if (hf_vsdb[6] & 0x80) { > BIT(7) ? Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ? > >> + hdmi->scdc.supported = true; >> + if (hf_vsdb[6] & 0x40) > BIT(6) ? Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB. > >> + hdmi->scdc.read_request = 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_parse_hdmi_forum_vsdb(connector, db); >> } >> } >> >> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h >> index e5e1edd..6d5304e 100644 >> --- a/include/drm/drm_connector.h >> +++ b/include/drm/drm_connector.h >> @@ -87,6 +87,34 @@ enum subpixel_order { >> SubPixelVerticalRGB, >> SubPixelVerticalBGR, >> SubPixelNone, >> + >> +}; >> + >> +/* >> + * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink >> + * >> + * Provides SCDC register support and capabilities related information on a >> + * HDMI 2.0 sink. In case of a HDMI 1.4 sink, all parameter must be 0. >> + */ >> +struct drm_scdc { >> + /** >> + * @supported: status control & data channel present. >> + */ >> + bool supported; >> + /** >> + * @read_request: sink is capable of generating scdc read request. >> + */ >> + bool read_request; >> +}; >> + >> +/** >> + * struct drm_hdmi_info - runtime information about the connected HDMI sink >> + * >> + * Describes if a given display supports advanced HDMI 2.0 features. >> + * This information is available in CEA-861-F extension blocks (like HF-VSDB). >> + */ >> +struct drm_hdmi_info { >> + struct drm_scdc scdc; >> }; >> >> /** >> @@ -188,6 +216,11 @@ struct drm_display_info { >> * @cea_rev: CEA revision of the HDMI sink. >> */ >> u8 cea_rev; >> + >> + /** >> + * @hdmi: advance features of a HDMI sink. > Maybe change to the same general description you used above: > "Runtime information about the connected HDMI sink" ? Actually the description I used in patch set 1, was similar, but this is based on feedbacks from review set 1. - Shashank >> + */ >> + struct drm_hdmi_info hdmi; >> }; >> >> int drm_display_info_set_bus_formats(struct drm_display_info *info, > Best regards, > Jose Miguel Abreu
On Tue, Feb 07, 2017 at 09:43:15PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 2/7/2017 4:31 PM, Jose Abreu wrote: > > Hi Shashank, > > > > > > > > On 06-02-2017 13:59, 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 advanced HDMI 2.0 features > > > - Adds another structure drm_scdc within drm_hdmi_info, to > > > reflect scdc support and capabilities in connected HDMI 2.0 sink. > > > - Checks the HF-VSDB block for presence of SCDC, and marks it > > > in scdc structure > > > - If SCDC is present, checks if sink is capable of generating > > > SCDC read request, and marks it in scdc structure. > > > > > > V2: Addressed review comments > > > Thierry: > > > - Fix typos in commit message and make abbreviation consistent > > > across the commit message. > > > - Change structure object name from hdmi_info -> hdmi > > > - Fix typos and abbreviations in description of structure drm_hdmi_info > > > end the description with a full stop. > > > - Create a structure drm_scdc, and keep all information related to SCDC > > > register set (supported, read request supported) etc in it. > > > > > > Ville: > > > - Change rr -> read_request > > > - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all > > > of HF-VSDB parsing can be kept in same function, in incremental > > > patches. > > > > > > Reviewed-by: Thierry Reding <treding@nvidia.com> > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > > > --- > > > drivers/gpu/drm/drm_edid.c | 14 ++++++++++++++ > > > include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ > > > 2 files changed, 47 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > > > index 96d3e47..a487b80 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_parse_hdmi_forum_vsdb(struct drm_connector *connector, > > > + const u8 *hf_vsdb) > > > +{ > > > + struct drm_hdmi_info *hdmi = &connector->display_info.hdmi; > > > + > > > + if (hf_vsdb[6] & 0x80) { > > BIT(7) ? > Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ? > > > > > + hdmi->scdc.supported = true; > > > + if (hf_vsdb[6] & 0x40) > > BIT(6) ? > Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB. I think what Jose was trying to say is that you should be using BIT(7) instead of 0x80 and BIT(6) instead of 0x40. That said, I think either is fine, but perhaps another idea would be to define macros for these. I know that most (all?) of the EDID parsing code uses literals, so this is consistent with existing code. Also usually code will be like: if (hf_vsdb[X] & 0xYZ) foo_supported = true; So the meaning of the bit is easy to read from the context. I think literals are fine in this case. Thierry
Hi, On 07-02-2017 16:36, Thierry Reding wrote: > On Tue, Feb 07, 2017 at 09:43:15PM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 2/7/2017 4:31 PM, Jose Abreu wrote: >>> Hi Shashank, >>> >>> >>> >>> On 06-02-2017 13:59, 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 advanced HDMI 2.0 features >>>> - Adds another structure drm_scdc within drm_hdmi_info, to >>>> reflect scdc support and capabilities in connected HDMI 2.0 sink. >>>> - Checks the HF-VSDB block for presence of SCDC, and marks it >>>> in scdc structure >>>> - If SCDC is present, checks if sink is capable of generating >>>> SCDC read request, and marks it in scdc structure. >>>> >>>> V2: Addressed review comments >>>> Thierry: >>>> - Fix typos in commit message and make abbreviation consistent >>>> across the commit message. >>>> - Change structure object name from hdmi_info -> hdmi >>>> - Fix typos and abbreviations in description of structure drm_hdmi_info >>>> end the description with a full stop. >>>> - Create a structure drm_scdc, and keep all information related to SCDC >>>> register set (supported, read request supported) etc in it. >>>> >>>> Ville: >>>> - Change rr -> read_request >>>> - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all >>>> of HF-VSDB parsing can be kept in same function, in incremental >>>> patches. >>>> >>>> Reviewed-by: Thierry Reding <treding@nvidia.com> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/drm_edid.c | 14 ++++++++++++++ >>>> include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ >>>> 2 files changed, 47 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>>> index 96d3e47..a487b80 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_parse_hdmi_forum_vsdb(struct drm_connector *connector, >>>> + const u8 *hf_vsdb) >>>> +{ >>>> + struct drm_hdmi_info *hdmi = &connector->display_info.hdmi; >>>> + >>>> + if (hf_vsdb[6] & 0x80) { >>> BIT(7) ? >> Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ? >>>> + hdmi->scdc.supported = true; >>>> + if (hf_vsdb[6] & 0x40) >>> BIT(6) ? >> Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB. > I think what Jose was trying to say is that you should be using BIT(7) > instead of 0x80 and BIT(6) instead of 0x40. That said, I think either is > fine, but perhaps another idea would be to define macros for these. I > know that most (all?) of the EDID parsing code uses literals, so this is > consistent with existing code. Also usually code will be like: > > if (hf_vsdb[X] & 0xYZ) > foo_supported = true; > > So the meaning of the bit is easy to read from the context. I think > literals are fine in this case. > > Thierry Thats exactly what I meant :) I think with BIT(x) the code is easier to read (my hex skills are not very good :)). Anyway, if the remaining code uses literals then maybe its better to keep consistency. Best regards, Jose Miguel Abreu
Regards Shashank On 2/8/2017 5:06 PM, Jose Abreu wrote: > Hi, > > > > On 07-02-2017 16:36, Thierry Reding wrote: >> On Tue, Feb 07, 2017 at 09:43:15PM +0530, Sharma, Shashank wrote: >>> Regards >>> >>> Shashank >>> >>> >>> On 2/7/2017 4:31 PM, Jose Abreu wrote: >>>> Hi Shashank, >>>> >>>> >>>> >>>> On 06-02-2017 13:59, 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 advanced HDMI 2.0 features >>>>> - Adds another structure drm_scdc within drm_hdmi_info, to >>>>> reflect scdc support and capabilities in connected HDMI 2.0 sink. >>>>> - Checks the HF-VSDB block for presence of SCDC, and marks it >>>>> in scdc structure >>>>> - If SCDC is present, checks if sink is capable of generating >>>>> SCDC read request, and marks it in scdc structure. >>>>> >>>>> V2: Addressed review comments >>>>> Thierry: >>>>> - Fix typos in commit message and make abbreviation consistent >>>>> across the commit message. >>>>> - Change structure object name from hdmi_info -> hdmi >>>>> - Fix typos and abbreviations in description of structure drm_hdmi_info >>>>> end the description with a full stop. >>>>> - Create a structure drm_scdc, and keep all information related to SCDC >>>>> register set (supported, read request supported) etc in it. >>>>> >>>>> Ville: >>>>> - Change rr -> read_request >>>>> - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all >>>>> of HF-VSDB parsing can be kept in same function, in incremental >>>>> patches. >>>>> >>>>> Reviewed-by: Thierry Reding <treding@nvidia.com> >>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>>> --- >>>>> drivers/gpu/drm/drm_edid.c | 14 ++++++++++++++ >>>>> include/drm/drm_connector.h | 33 +++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 47 insertions(+) >>>>> >>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c >>>>> index 96d3e47..a487b80 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_parse_hdmi_forum_vsdb(struct drm_connector *connector, >>>>> + const u8 *hf_vsdb) >>>>> +{ >>>>> + struct drm_hdmi_info *hdmi = &connector->display_info.hdmi; >>>>> + >>>>> + if (hf_vsdb[6] & 0x80) { >>>> BIT(7) ? >>> Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ? >>>>> + hdmi->scdc.supported = true; >>>>> + if (hf_vsdb[6] & 0x40) >>>> BIT(6) ? >>> Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB. >> I think what Jose was trying to say is that you should be using BIT(7) >> instead of 0x80 and BIT(6) instead of 0x40. That said, I think either is >> fine, but perhaps another idea would be to define macros for these. I >> know that most (all?) of the EDID parsing code uses literals, so this is >> consistent with existing code. Also usually code will be like: >> >> if (hf_vsdb[X] & 0xYZ) >> foo_supported = true; >> >> So the meaning of the bit is easy to read from the context. I think >> literals are fine in this case. >> >> Thierry > Thats exactly what I meant :) I think with BIT(x) the code is > easier to read (my hex skills are not very good :)). Anyway, if > the remaining code uses literals then maybe its better to keep > consistency. Thanks, then we will keep this code, as it is. - Shashank > Best regards, > Jose Miguel Abreu >
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 96d3e47..a487b80 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_parse_hdmi_forum_vsdb(struct drm_connector *connector, + const u8 *hf_vsdb) +{ + struct drm_hdmi_info *hdmi = &connector->display_info.hdmi; + + if (hf_vsdb[6] & 0x80) { + hdmi->scdc.supported = true; + if (hf_vsdb[6] & 0x40) + hdmi->scdc.read_request = 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_parse_hdmi_forum_vsdb(connector, db); } } diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index e5e1edd..6d5304e 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -87,6 +87,34 @@ enum subpixel_order { SubPixelVerticalRGB, SubPixelVerticalBGR, SubPixelNone, + +}; + +/* + * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink + * + * Provides SCDC register support and capabilities related information on a + * HDMI 2.0 sink. In case of a HDMI 1.4 sink, all parameter must be 0. + */ +struct drm_scdc { + /** + * @supported: status control & data channel present. + */ + bool supported; + /** + * @read_request: sink is capable of generating scdc read request. + */ + bool read_request; +}; + +/** + * struct drm_hdmi_info - runtime information about the connected HDMI sink + * + * Describes if a given display supports advanced HDMI 2.0 features. + * This information is available in CEA-861-F extension blocks (like HF-VSDB). + */ +struct drm_hdmi_info { + struct drm_scdc scdc; }; /** @@ -188,6 +216,11 @@ struct drm_display_info { * @cea_rev: CEA revision of the HDMI sink. */ u8 cea_rev; + + /** + * @hdmi: advance features of a HDMI sink. + */ + struct drm_hdmi_info hdmi; }; int drm_display_info_set_bus_formats(struct drm_display_info *info,