From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 3/6] drm/edid: detect SCDC support in HF-VSDB Date: Thu, 2 Feb 2017 19:02:50 +0100 Message-ID: <20170202180250.GE9055@ulmo.ba.sec> References: <1485953081-7630-1-git-send-email-shashank.sharma@intel.com> <1485953081-7630-4-git-send-email-shashank.sharma@intel.com> <20170201161002.GB18725@ulmo.ba.sec> <1781b63f-3219-cb0c-312d-fb126764d4b4@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0098066037==" Return-path: In-Reply-To: <1781b63f-3219-cb0c-312d-fb126764d4b4@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Sharma, Shashank" Cc: jose.abreu@synopsys.com, =daniel.vetter@intel.com, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============0098066037== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="idY8LE8SD6/8DnRI" Content-Disposition: inline --idY8LE8SD6/8DnRI Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 02, 2017 at 10:58:43AM +0530, Sharma, Shashank wrote: > Thanks for the review Thierry. My comments inline. >=20 > 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. > >=20 > > > - 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 > > > --- > > > drivers/gpu/drm/drm_edid.c | 14 ++++++++++++++ > > > include/drm/drm_connector.h | 26 ++++++++++++++++++++++++++ > > > 2 files changed, 40 insertions(+) > > >=20 > > > 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 =3D &connector->display_info.hdmi_info; > > > + > > > + if (hf_vsdb[6] & 0x80) { > > > + hdmi->scdc_supported =3D true; > > > + if (hf_vsdb[6] & 0x40) > > > + hdmi->scdc_rr =3D true; > > > + } > > > +} > > > + > > > static void drm_parse_hdmi_deep_color_info(struct drm_connector *co= nnector, > > > const u8 *hdmi) > > > { > > > @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connec= tor *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 featu= tes. > > "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 rem= ove > 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 --idY8LE8SD6/8DnRI Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAliTdEkACgkQ3SOs138+ s6EMOQ/+LO1P1IYUlIcDN/C/gCwE7b88Jyq6tnc5I3EUlzjZjYuSU4zJYMpDZk8S 8eBt2haZ3kMiXpNHlWoTN3YZNgAJEgdzO1huWnwCEQ+o1NsSGAN5B1kvvteG6G01 NskvACjfQzwBYOA8de7O0El7rE41Ga2P82NR5WzyqeT82+0dRbgYyNk0p2accEJr O6vvHen8jZu/yNsCX1+pHtzyYmd14gUhB34r6NB+iLm9le0xluahuvuYHHBW6nok jZUKo1zJq95TsuJQlpkcMzy11h5acQbLx5ThD6dkg0bLNZAOAmMmAS9tvbsUbyNb /npyt+HZs8pTFfRpfLBTWzxZXPnPmturbHMo1HxJnwFHFk8qiQjHX+Hd6gSQSupn KQZWq1PD+Iwc/vcSmIhPST5bdxlIIRF7T977ZgujawQhIHV5+tHg9jX+BeYz46in V4vFFAQ/igc8hCdIEdj6lWBuDnT9OzcI5JL2wJiq1GkunlmcPv3MtGpfRiBFe3xX 1WiqTf3p13hcY3dqJ9HwdoFbhT44/cVOaRi2i0y6GdUitCPAvr6lh/6WkwQapiDc iZi7CU8A8ngmcdHPmdygWqxvMD37Gcs2KLJ9/rarNWD5hMxKmPICjqpRKRwQyMI+ u7cV/zaetS1BnH7j39UsI19PNPAQGYW/VfmOxwqIj7UY9b46S6M= =9DTx -----END PGP SIGNATURE----- --idY8LE8SD6/8DnRI-- --===============0098066037== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0098066037==--