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

[Thierry Reding <treding@nvidia.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3177 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel