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: Thu, 2 Feb 2017 19:13:50 +0100 Message-ID: <20170202181350.GF9055@ulmo.ba.sec> References: <1485953081-7630-1-git-send-email-shashank.sharma@intel.com> <1485953081-7630-5-git-send-email-shashank.sharma@intel.com> <20170201163201.GC18725@ulmo.ba.sec> <6bd29f50-aac5-3fbd-ab1b-c6f4b79e99f9@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1716642308==" Return-path: In-Reply-To: <6bd29f50-aac5-3fbd-ab1b-c6f4b79e99f9@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 --===============1716642308== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GV0iVqYguTV4Q9ER" Content-Disposition: inline --GV0iVqYguTV4Q9ER Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Feb 02, 2017 at 11:08:22AM +0530, Sharma, Shashank wrote: > Regards >=20 > Shashank >=20 >=20 > On 2/1/2017 10:02 PM, Thierry Reding wrote: > > 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 > > > #define version_greater(edid, maj, min) \ > > > (((edid)->version > (maj)) || \ > > > @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_c= onnector *connector, > > > } > > > } > > > +static void drm_detect_hdmi_scrambling(struct drm_connector *connect= or, > > > + 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. > Agree. > > > + * 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. > Got it. > > > + * > > > + * 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. > Ok. > > > + */ > > > + > > > +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. > >=20 > > 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. > Yeah, the same is mentioned by Ville too, will do that. > > > +{ > > > + u8 status; > > > + > > > + if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) { > > How about storing the error code... > >=20 > > > + DRM_ERROR("Failed to read scrambling status\n"); > > ... 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. > Agree, in fact while debugging and testing this patch series, I had to pr= int > it explicitly. > >=20 > > > + return false; > > > + } > > > + > > > + status &=3D SCDC_SCRAMBLING_STATUS; > > > + return status !=3D 0; > > Maybe make this a single line: > >=20 > > return (status & SCDC_SCRAMBLING_STATUS) !=3D 0; > Got it. > >=20 > > > +} > > > + > > > +/** > > > + * drm_enable_scrambling - enable scrambling > > > + * @connector: target drm_connector > > "target DRM connector"? > Got it. > > > + * @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 otherwi= se. > > > + */ > > > + > > > +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. > Agree, will move it. > > > +{ > > > + u8 config; > > > + struct drm_hdmi_info *hdmi_info =3D &connector->display_info.hdmi_i= nfo; > > > + > > > + 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? > Ok. > > > + 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. > Ok > > > + 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 otherw= ise. > > > + */ > > > +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_i= nfo; > > > + > > > + 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(). > Got it. > > > @@ -3928,8 +4055,10 @@ static void drm_parse_cea_ext(struct drm_conne= ctor *connector, > > > 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); > > > + } > > > } > > > } > > > 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 { > > > }; > > > + > > > +/** > > > + * 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". > Got it. > > > + */ > > > + 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. > Agree, will change it. > > > + /** > > > + * @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. > Sure, can be done. > >=20 > > 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? > My opinion while designing was: > - SCDC rr support is platform specific, and a platform can choose not to > support read_req but just allow > scrambling using scdc read and write, hence kept that separate > - You need to look into this internal structure, only if scdc is supporte= d. > - Also, SCDC registers can be used beyond scrambling too, like for error > detection, and other cases, so I thought > it would be better to keep it independent. >=20 > Does it make sense ? Yes, I think that makes sense, but it's not what I was trying to say. =3D) What I was trying to say is that read request and scrambling are defined in the SCDC specification, and therefore they require SCDC. That's why I think the below is a natural representation because it captures the dependency in a hierarchy: > > struct drm_hdmi_scdc_scrambling_info { > > bool supported; > > bool low_rates; > > bool status; > > }; > >=20 > > struct drm_hdmi_scdc_info { > > bool supported; > > bool read_request; > >=20 > > struct drm_hdmi_scdc_scrambling_info scrambling; > > }; I should have added to the above: struct drm_hdmi_info { struct drm_hdmi_scdc_info scdc; }; So when conditionalizing code this allows for a very natural ordering of the code: struct drm_display_info *info =3D ...; struct drm_hdmi_scdc_info *scdc =3D &info->hdmi.scdc; if (scdc->supported) { ... if (scdc->read_request) { ... e.g. set up handler for read requests ... } ... if (scdc->scrambling.supported) { if (mode->clock >=3D 340000 || scdc->scrambling.low_rates) { ... set up scrambling ...; } } ... } Thierry --GV0iVqYguTV4Q9ER Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAliTdtwACgkQ3SOs138+ s6F84A/8CRzVDVfuEOE8c6ZXM0KTFSegBwiq966Jphq6ZmKHHg1L6VGm+IO9zwYu g6Gsa2cwDfjbfG+FDOyOLjZ184n/64k6qJmJmChbGRca7f+aD1AGEQkWht4smaoN Ql/n1m9vHYdi7qWlCjuApHcpBF4IWwb2ENpez/7QXR4X6kgZl7eNsR2l5H1dTYR1 ou4qZvG2S07Oo31XRjU7coR+KTRNHyGckFLlP6tt6s6XD+MYpkIvNs8oAz1Lp0Ty 0WpPL2b3ESR251dhDj81IZ6ee9d4m5emSrZ9sK0s7zpH7fKK6og8ClKWblPTLegJ /3gft67Irz+3ijCiYxBqKNx7KRZo9oVTcC1hzviccG+oDIs1ABppq1ugaXYVSDB3 whJzKrqlvtdJZoGgWxSozI9LLqjiCZ4vF2b2WDQfaTh2ZO3iFCEGAHbIh6l97x4j UdINih8m+4hxZJ3YOlMfWjmr7lhSoN43m+R152V42MG3lixDRuvlus7BYIDJhwhh PXKg4YFqzUYP9RKQ/QNCFf7/yjlXfULSLuGPOPq/aS9+aQR1aZ4fQYhag6uPuoOP FyO//bWKVnwKS5ZdP7f2MWl6fzU9yFBrHdrkKJopPoXij3jLAtJjlxldCRDL2rv3 V8dqMbcPfSDpuCU5h+DDy4KoDauMeJdORNuxP42cpbzJbUi5eEk= =rq59 -----END PGP SIGNATURE----- --GV0iVqYguTV4Q9ER-- --===============1716642308== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============1716642308==--