Re: [PATCH 4/6] drm: scrambling support in drm layer

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

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

On Thu, Feb 02, 2017 at 11:08:22AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> 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.
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > ---
> > >   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(-)
> > > 
> > > 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 <drm/drm_edid.h>
> > >   #include <drm/drm_encoder.h>
> > >   #include <drm/drm_displayid.h>
> > > +#include <drm/drm_scdc_helper.h>
> > >   #define version_greater(edid, maj, min) \
> > >   	(((edid)->version > (maj)) || \
> > > @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
> > >   	}
> > >   }
> > > +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
> > > +				 const u8 *hf_vsdb)
> > > +{
> > > +	struct drm_display_info *display = &connector->display_info;
> > > +	struct drm_hdmi_info *hdmi = &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 = 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 = true;
> > > +
> > > +			/* Few sinks support scrambling for cloks < 340M */
> > > +			if ((hf_vsdb[6] & 0x8))
> > > +				hdmi->scr_info.low_clocks = 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.
> > 
> > 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...
> > 
> > > +		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 print
> it explicitly.
> > 
> > > +		return false;
> > > +	}
> > > +
> > > +	status &= SCDC_SCRAMBLING_STATUS;
> > > +	return status != 0;
> > Maybe make this a single line:
> > 
> > 	return (status & SCDC_SCRAMBLING_STATUS) != 0;
> Got it.
> > 
> > > +}
> > > +
> > > +/**
> > > + * 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 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.
> Agree, will move it.
> > > +{
> > > +	u8 config;
> > > +	struct drm_hdmi_info *hdmi_info = &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?
> Ok.
> > > +		return false;
> > > +	}
> > > +
> > > +	config |= 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 = 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 = &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 &= ~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 = 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_connector *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 <= 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.
> > 
> > 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 supported.
> - 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.
> 
> Does it make sense ?

Yes, I think that makes sense, but it's not what I was trying to say. =)
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;
> > 	};
> > 
> > 	struct drm_hdmi_scdc_info {
> > 		bool supported;
> > 		bool read_request;
> > 
> > 		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 = ...;
	struct drm_hdmi_scdc_info *scdc = &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 >= 340000 || scdc->scrambling.low_rates) {
				...
				set up scrambling
				...;
			}
		}

		...
	}

Thierry

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel