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

[Thierry Reding <thierry.reding@gmail.com>]

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

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

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

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx