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@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 ?
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:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
