From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 4/6] drm: scrambling support in drm layer Date: Wed, 1 Feb 2017 17:32:01 +0100 Message-ID: <20170201163201.GC18725@ulmo.ba.sec> References: <1485953081-7630-1-git-send-email-shashank.sharma@intel.com> <1485953081-7630-5-git-send-email-shashank.sharma@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0719233030==" Return-path: In-Reply-To: <1485953081-7630-5-git-send-email-shashank.sharma@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Shashank Sharma 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 --===============0719233030== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="DSayHWYpDlRfCAAQ" Content-Disposition: inline --DSayHWYpDlRfCAAQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 01, 2017 at 06:14:39PM +0530, Shashank Sharma wrote: > HDMI 2.0 spec mandates scrambling for modes with pixel clock higher > than 340Mhz. This patch adds few new functions in drm layer for > core drivers to enable/disable scrambling. >=20 > This patch adds: > - A function to detect scrambling support parsing HF-VSDB > - A function to check scrambling status runtime using SCDC read. > - Two functions to enable/disable scrambling using SCDC read/write. > - Few new bools to reflect scrambling support and status. >=20 > Signed-off-by: Shashank Sharma > --- > drivers/gpu/drm/drm_edid.c | 131 ++++++++++++++++++++++++++++++++++++++= +++++- > include/drm/drm_connector.h | 24 ++++++++ > include/drm/drm_edid.h | 6 +- > 3 files changed, 159 insertions(+), 2 deletions(-) >=20 > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 37902e5..f0d940a 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -37,6 +37,7 @@ > #include > #include > #include > +#include > =20 > #define version_greater(edid, maj, min) \ > (((edid)->version > (maj)) || \ > @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_conne= ctor *connector, > } > } > =20 > +static void drm_detect_hdmi_scrambling(struct drm_connector *connector, > + const u8 *hf_vsdb) > +{ > + struct drm_display_info *display =3D &connector->display_info; > + struct drm_hdmi_info *hdmi =3D &display->hdmi_info; > + > + /* > + * All HDMI 2.0 monitors must support scrambling at rates > 340M. In comments below you use Mhz as the abbreviations. This should be consistent. Also I think "MHz" is actually the correct spelling. > + * And as per the spec, three factors confirm this: > + * * Availability of a HF-VSDB block in EDID (check) > + * * Non zero Max_TMDS_Char_Rate filed in HF-VSDB > + * * SCDC support available > + * Lets check it out. > + */ > + > + if (hf_vsdb[5]) { > + display->max_tmds_clock =3D hf_vsdb[5] * 5000; > + DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n", > + display->max_tmds_clock); > + > + if (hdmi->scdc_supported) { > + hdmi->scr_info.supported =3D true; > + > + /* Few sinks support scrambling for cloks < 340M */ > + if ((hf_vsdb[6] & 0x8)) > + hdmi->scr_info.low_clocks =3D true; > + } > + } > +} > + > +/** > + * drm_check_scrambling_status - what is status of scrambling? > + * @adapter: i2c adapter for SCDC channel "I2C", same in other parts of this patch. > + * > + * Read the scrambler status over SCDC channel, and check the > + * scrambling status. > + * > + * Return: True if the scrambling is enabled, false otherwise. I think the rest of DRM/KMS kerneldoc tries to use "Returns:\n" as a standard way to document return values. > + */ > + > +bool drm_check_scrambling_status(struct i2c_adapter *adapter) Maybe use a drm_scdc_*() prefix for this to make it more consistent with other SCDC API. While at it, would this not be better located in drm_scdc.c along with the other helpers? drm_edid.c is more focussed on the parsing aspects of all things EDID. > +{ > + u8 status; > + > + if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) { How about storing the error code... > + DRM_ERROR("Failed to read scrambling status\n"); =2E.. and making it part of the error message? Sometimes its useful to know what exact error triggered this because it helps narrowing down where things went wrong. > + return false; > + } > + > + status &=3D SCDC_SCRAMBLING_STATUS; > + return status !=3D 0; Maybe make this a single line: return (status & SCDC_SCRAMBLING_STATUS) !=3D 0; > +} > + > +/** > + * drm_enable_scrambling - enable scrambling > + * @connector: target drm_connector "target DRM connector"? > + * @adapter: i2c adapter for SCDC channel > + * @force: enable scrambling, even if its already enabled > + * > + * Write the TMDS config over SCDC channel, and enable scrambling > + * Return: True if scrambling is successfully enabled, false otherwise. > + */ > + > +bool drm_enable_scrambling(struct drm_connector *connector, > + struct i2c_adapter *adapter, bool force) I think I'd move this to drm_scdc.c as well because it primarily operates on SCDC. If you do so, might be worth making adapter the first argument for consistency with other SCDC API. > +{ > + u8 config; > + struct drm_hdmi_info *hdmi_info =3D &connector->display_info.hdmi_info; > + > + if (hdmi_info->scr_info.status && !force) { > + DRM_DEBUG_KMS("Scrambling already enabled\n"); > + return true; > + } > + > + if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) { > + DRM_ERROR("Failed to read tmds config\n"); Maybe also print the error code? > + return false; > + } > + > + config |=3D SCDC_SCRAMBLING_ENABLE; > + > + if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) { > + DRM_ERROR("Failed to enable scrambling, write error\n"); Same here. > + return false; > + } > + > + hdmi_info->scr_info.status =3D drm_check_scrambling_status(adapter); > + return hdmi_info->scr_info.status; > +} > + > +/** > + * drm_disable_scrambling - disable scrambling > + * @connector: target drm_connector > + * @adapter: i2c adapter for SCDC channel > + * @force: disable scrambling, even if its already disabled > + * > + * Write the TMDS config over SCDC channel, and disable scrambling > + * Return: True if scrambling is successfully disabled, false otherwise. > + */ > +bool drm_disable_scrambling(struct drm_connector *connector, > + struct i2c_adapter *adapter, bool force) > +{ > + u8 config; > + struct drm_hdmi_info *hdmi_info =3D &connector->display_info.hdmi_info; > + > + if (!hdmi_info->scr_info.status && !force) { > + DRM_DEBUG_KMS("Scrambling already disabled\n"); > + return true; > + } > + > + if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) { > + DRM_ERROR("Failed to read tmds config\n"); > + return false; > + } > + > + config &=3D ~SCDC_SCRAMBLING_ENABLE; > + > + if (drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config) < 0) { > + DRM_ERROR("Failed to enable scrambling, write error\n"); > + return false; > + } > + > + hdmi_info->scr_info.status =3D drm_check_scrambling_status(adapter); > + return !hdmi_info->scr_info.status; > +} Same comments as for drm_enable_scrambling(). > @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_connector= *connector, > =20 > if (cea_db_is_hdmi_vsdb(db)) > drm_parse_hdmi_vsdb_video(connector, db); > - if (cea_db_is_hdmi_forum_vsdb(db)) > + if (cea_db_is_hdmi_forum_vsdb(db)) { > drm_detect_hdmi_scdc(connector, db); > + drm_detect_hdmi_scrambling(connector, db); > + } > } > } > =20 > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index 2435598..b9735bd 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -90,6 +90,26 @@ enum subpixel_order { > =20 > }; > =20 > + > +/** > + * struct scrambling: sink's scrambling support. > + */ > +struct scrambling { > + /** > + * @supported: scrambling supported for rates > 340Mhz. I think it's common to separate number and unit by a space, so "340 MHz". > + */ > + bool supported; > + /** > + * @low_clocks: scrambling supported for rates <=3D 340Mhz. > + */ > + bool low_clocks; Maybe "low_rates" because a clock is technically the source of a signal that oscillates at the given rate. > + /** > + * @status: scrambling enabled/disabled ? > + */ > + bool status; > +}; > + > + > /** > * struct drm_hdmi_info - runtime data about the connected sink > * > @@ -108,6 +128,10 @@ struct drm_hdmi_info { > * @scdc_rr: sink is capable of generating scdc read request. > */ > bool scdc_rr; > + /** > + * scr_info: sink's scrambling support and capabilities. > + */ > + struct scrambling scr_info; Again, I think I'd drop _info and instead spell out "scrambling" to make this easier to remember. Also I'm wondering why scdc_supported and scdc_rr are not in a structure if scrambling info is. Also since scrambling depends on SCDC, would it make sense to embed it in a struct drm_hdmi_scdc_info? struct drm_hdmi_scdc_scrambling_info { bool supported; bool low_rates; bool status; }; struct drm_hdmi_scdc_info { bool supported; bool read_request; struct drm_hdmi_scdc_scrambling_info scrambling; }; Thierry --DSayHWYpDlRfCAAQ Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAliSDX4ACgkQ3SOs138+ s6HIsw//QpuoVmmcTj90upegqTsrr4nVAqPAUBproA31zIDTqPlkY3IkqRf5it5T uBuWH71Z/N09KA6WKt8HhcqopstP/YAJNZrIQFO+NZs0BgYCPNfLi6JAZjLTL9iW 7TCKMP1GwzKCsOcAmX0/D3hqEkueKyLzaYvSmInvDxvT0U/Pc/nNlv4m2Pr86xef uPgPdoiheN9Hg9lcliZ/qDY3aydXJF1srDHXQc+15KMuWnErG0f9oHWGYTTX+EOx lYsp0XwpuB/modHSKaTQdH+kjhTR/bLJ9kJRya7hiFMNTim0ikzQto02qFzqrJwb J+PrrJA/xqDjqDq/qLrJl7GXkYNmpNc0T9FWqBDNnmlueFeJjpxCGzjV/JEKbgJQ jVwuzIe3IyJnmQGhB7B9ADI7Z3nRxNdhosgnGcuZWClaOAbZx/QbGr16Jb7+AzQ+ 2QaoFkUoNIMuGkijpEyqewzD8uduGUBfvJoQA3ioUMJ0M6DzQy9G4FFdlc2sH4o8 FzDDcxtsDGgOANR6Hyc0XNYSzaRJjM82jO/fxZQBE8HDYbgTX8hlK/uy1tkIkEfb kawQ1yYDf+pnG1qvAagtrFG8GUtZRWUUIKd/vI1TtTg2uDjc5QoOkkShgudVoQ42 PvfrWOcEWzN/VCZTAh65cjKBCGztbAim7ylM4L64GBx+/Keu9PQ= =geGc -----END PGP SIGNATURE----- --DSayHWYpDlRfCAAQ-- --===============0719233030== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0719233030==--