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

Commit Message

Sharma, Shashank Feb. 1, 2017, 12:44 p.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Thierry Reding Feb. 1, 2017, 4:32 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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.

> +	 * 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.

> + *
> + * 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");

... 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 &= SCDC_SCRAMBLING_STATUS;
> +	return status != 0;

Maybe make this a single line:

	return (status & SCDC_SCRAMBLING_STATUS) != 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 = &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 |= 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 = 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().

> @@ -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".

> +	 */
> +	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.

> +	/**
> +	 * @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

Ville Syrjälä Feb. 1, 2017, 4:32 p.m. UTC | #2

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 <[e-mail]>
> ---
>  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)

That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
something.

> +{
> +	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.
> +	 * 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);

I wonder if we should be a little paranoid and ignore this if it's
<=340?

> +
> +		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
> + *
> + * Read the scrambler status over SCDC channel, and check the
> + * scrambling status.
> + *
> + * Return: True if the scrambling is enabled, false otherwise.
> + */
> +
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)

The name is again a little wonky. And it looks like something that
should live alognside the SCDC helper stuff.

> +{
> +	u8 status;
> +
> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
> +		DRM_ERROR("Failed to read scrambling status\n");
> +		return false;
> +	}
> +
> +	status &= SCDC_SCRAMBLING_STATUS;
> +	return status != 0;

return status & SCDC_SCRAMBLING_STATUS

> +}
> +
> +/**
> + * drm_enable_scrambling - enable scrambling
> + * @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)
> +{
> +	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;
> +	}

I don't think we want to track any dynamic state in the display info.
That belongs to the atomic state stuff.

And the function again looks like something that belongs in the SCDC
helper.

Since the two pieces of infromaton in this registrer are the scramble
control and the clock ratio, I think we might want to just have one
function to configure both.

> +
> +	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;
> +}
> +
> +/**
> + * 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;
> +}
> +
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  					   const u8 *hdmi)
>  {
> @@ -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.
> +	 */
> +	bool supported;
> +	/**
> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
> +	 */
> +	bool low_clocks;
> +	/**
> +	 * @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;
>  };
>  
>  /**
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 43fb0ac..d24c974 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>  					   int hsize, int vsize, int fresh,
>  					   bool rb);
> -
> +bool drm_enable_scrambling(struct drm_connector *connector,
> +				struct i2c_adapter *adapter, bool force);
> +bool drm_disable_scrambling(struct drm_connector *connector,
> +				struct i2c_adapter *adapter, bool force);
> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);
>  #endif /* __DRM_EDID_H__ */
> -- 
> 1.9.1

Dhinakaran Pandiyan Feb. 1, 2017, 7:53 p.m. UTC | #3

On Wed, 2017-02-01 at 18:14 +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 <[e-mail]>

> ---

>  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.

> +	 * 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;


Comment explaining or quoting where the 5000 comes from?

> +		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

> + *

> + * Read the scrambler status over SCDC channel, and check the

> + * scrambling status.

> + *

> + * Return: True if the scrambling is enabled, false otherwise.

> + */

> +

> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)

> +{

> +	u8 status;

> +

> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {

> +		DRM_ERROR("Failed to read scrambling status\n");

> +		return false;

> +	}

> +

> +	status &= SCDC_SCRAMBLING_STATUS;

> +	return status != 0;

> +}

> +

> +/**

> + * drm_enable_scrambling - enable scrambling

> + * @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)

> +{

> +	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");

> +		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;

> +}

> +

> +/**

> + * 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;

> +}

> +

>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,

>  					   const u8 *hdmi)

>  {

> @@ -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.

> +	 */

> +	bool supported;

> +	/**

> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.

> +	 */

> +	bool low_clocks;


The naming for low and high clock rates looks asymmetric.

> +	/**

> +	 * @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;

>  };

>  

>  /**

> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h

> index 43fb0ac..d24c974 100644

> --- a/include/drm/drm_edid.h

> +++ b/include/drm/drm_edid.h

> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,

>  struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,

>  					   int hsize, int vsize, int fresh,

>  					   bool rb);

> -

> +bool drm_enable_scrambling(struct drm_connector *connector,

> +				struct i2c_adapter *adapter, bool force);

> +bool drm_disable_scrambling(struct drm_connector *connector,

> +				struct i2c_adapter *adapter, bool force);

> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);

>  #endif /* __DRM_EDID_H__ */

Sharma, Shashank Feb. 2, 2017, 5:38 a.m. UTC | #4

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 <[e-mail]>
>> ---
>>   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 ?

-Shashank
>
> 	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

Sharma, Shashank Feb. 2, 2017, 5:48 a.m. UTC | #5

Regards

Shashank


On 2/1/2017 10:02 PM, Ville Syrjälä 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 <[e-mail]>
>> ---
>>   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)
> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
> something.
Actually, unlike the last patch set, we are not parsing the whole 
hf_vsdb, but parsing it only for
scrambling status byte (hf_vsdb[5]). But may be I can make it 
drm_detect_scrambling_from_hfvsdb
ot something similar. We will have more hf_vsdb parsing for 3d flags, 
yuv420_deep_color etc.
>> +{
>> +	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.
>> +	 * 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);
> I wonder if we should be a little paranoid and ignore this if it's
> <=340?
Sure, will do it.
>
>> +
>> +		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
>> + *
>> + * Read the scrambler status over SCDC channel, and check the
>> + * scrambling status.
>> + *
>> + * Return: True if the scrambling is enabled, false otherwise.
>> + */
>> +
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
> The name is again a little wonky. And it looks like something that
> should live alognside the SCDC helper stuff.
Yep, Thierry also suggested the same, to move it along with SCDC. Will 
do it.
>> +{
>> +	u8 status;
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
>> +		DRM_ERROR("Failed to read scrambling status\n");
>> +		return false;
>> +	}
>> +
>> +	status &= SCDC_SCRAMBLING_STATUS;
>> +	return status != 0;
> return status & SCDC_SCRAMBLING_STATUS
Sure.
>> +}
>> +
>> +/**
>> + * drm_enable_scrambling - enable scrambling
>> + * @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)
>> +{
>> +	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;
>> +	}
> I don't think we want to track any dynamic state in the display info.
> That belongs to the atomic state stuff.
Hummm, ok.
>
> And the function again looks like something that belongs in the SCDC
> helper.
Agree.
>
> Since the two pieces of infromaton in this registrer are the scramble
> control and the clock ratio, I think we might want to just have one
> function to configure both.
That's a good suggestion, thanks.

- Shashank
>
>> +
>> +	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;
>> +}
>> +
>> +/**
>> + * 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;
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -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.
>> +	 */
>> +	bool supported;
>> +	/**
>> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
>> +	 */
>> +	bool low_clocks;
>> +	/**
>> +	 * @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;
>>   };
>>   
>>   /**
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 43fb0ac..d24c974 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>>   struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>>   					   int hsize, int vsize, int fresh,
>>   					   bool rb);
>> -
>> +bool drm_enable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_disable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);
>>   #endif /* __DRM_EDID_H__ */
>> -- 
>> 1.9.1

Sharma, Shashank Feb. 2, 2017, 5:55 a.m. UTC | #6

Thanks for the review, Dhinakaran.


Regards

Shashank


On 2/2/2017 1:23 AM, Pandiyan, Dhinakaran wrote:
> On Wed, 2017-02-01 at 18:14 +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 <[e-mail]>
>> ---
>>   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.
>> +	 * 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;
> Comment explaining or quoting where the 5000 comes from?
Sure, can be done.
>
>> +		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
>> + *
>> + * Read the scrambler status over SCDC channel, and check the
>> + * scrambling status.
>> + *
>> + * Return: True if the scrambling is enabled, false otherwise.
>> + */
>> +
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
>> +{
>> +	u8 status;
>> +
>> +	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
>> +		DRM_ERROR("Failed to read scrambling status\n");
>> +		return false;
>> +	}
>> +
>> +	status &= SCDC_SCRAMBLING_STATUS;
>> +	return status != 0;
>> +}
>> +
>> +/**
>> + * drm_enable_scrambling - enable scrambling
>> + * @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)
>> +{
>> +	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");
>> +		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;
>> +}
>> +
>> +/**
>> + * 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;
>> +}
>> +
>>   static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>   					   const u8 *hdmi)
>>   {
>> @@ -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.
>> +	 */
>> +	bool supported;
>> +	/**
>> +	 * @low_clocks: scrambling supported for rates <= 340Mhz.
>> +	 */
>> +	bool low_clocks;
> The naming for low and high clock rates looks asymmetric.
I dont get it, I dont have a high clock here, can you please elaborate ?

- Shashank
>> +	/**
>> +	 * @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;
>>   };
>>   
>>   /**
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 43fb0ac..d24c974 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -462,5 +462,9 @@ void drm_edid_get_monitor_name(struct edid *edid, char *name,
>>   struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
>>   					   int hsize, int vsize, int fresh,
>>   					   bool rb);
>> -
>> +bool drm_enable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_disable_scrambling(struct drm_connector *connector,
>> +				struct i2c_adapter *adapter, bool force);
>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter);
>>   #endif /* __DRM_EDID_H__ */

Ville Syrjälä Feb. 2, 2017, 9:51 a.m. UTC | #7

On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/1/2017 10:02 PM, Ville Syrjälä 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 <[e-mail]>
> >> ---
> >>   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)
> > That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
> > something.
> Actually, unlike the last patch set, we are not parsing the whole 
> hf_vsdb, but parsing it only for
> scrambling status byte (hf_vsdb[5]). But may be I can make it 
> drm_detect_scrambling_from_hfvsdb
> ot something similar. We will have more hf_vsdb parsing for 3d flags, 
> yuv420_deep_color etc.

Well, so far I'm not seeing much point in splitting it up. So I'd stuff
it all into one place, for now at least.

Sharma, Shashank Feb. 2, 2017, 10:16 a.m. UTC | #8

Regards

Shashank


On 2/2/2017 3:21 PM, Ville Syrjälä wrote:
> On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/1/2017 10:02 PM, Ville Syrjälä 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 <[e-mail]>
>>>> ---
>>>>    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)
>>> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
>>> something.
>> Actually, unlike the last patch set, we are not parsing the whole
>> hf_vsdb, but parsing it only for
>> scrambling status byte (hf_vsdb[5]). But may be I can make it
>> drm_detect_scrambling_from_hfvsdb
>> ot something similar. We will have more hf_vsdb parsing for 3d flags,
>> yuv420_deep_color etc.
> Well, so far I'm not seeing much point in splitting it up. So I'd stuff
> it all into one place, for now at least.
I had published a patch just doing as you suggested which was parsing 
complete hf-vsdb in one shot, and adding info in
hdmi_info structure, to accommodate it.
(Patch https://patchwork.freedesktop.org/patch/128963/ and 
https://patchwork.freedesktop.org/patch/128962/)

But you gave a review comment not to add anything which is not being 
used in the patch series:
https://patchwork.freedesktop.org/patch/128962/

That was the only reason I have split hf-vsdb parsing patch into 3 parts
- parse SCDC and scrambling info (here)
- with YUV420 deep color ( upcoming )
- with 3d handling (upcoming)

So I am confused, split or no split :-) ?

- Shashank

Ville Syrjälä Feb. 2, 2017, 10:28 a.m. UTC | #9

On Thu, Feb 02, 2017 at 03:46:55PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/2/2017 3:21 PM, Ville Syrjälä wrote:
> > On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 2/1/2017 10:02 PM, Ville Syrjälä 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 <[e-mail]>
> >>>> ---
> >>>>    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)
> >>> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
> >>> something.
> >> Actually, unlike the last patch set, we are not parsing the whole
> >> hf_vsdb, but parsing it only for
> >> scrambling status byte (hf_vsdb[5]). But may be I can make it
> >> drm_detect_scrambling_from_hfvsdb
> >> ot something similar. We will have more hf_vsdb parsing for 3d flags,
> >> yuv420_deep_color etc.
> > Well, so far I'm not seeing much point in splitting it up. So I'd stuff
> > it all into one place, for now at least.
> I had published a patch just doing as you suggested which was parsing 
> complete hf-vsdb in one shot, and adding info in
> hdmi_info structure, to accommodate it.
> (Patch https://patchwork.freedesktop.org/patch/128963/ and 
> https://patchwork.freedesktop.org/patch/128962/)
> 
> But you gave a review comment not to add anything which is not being 
> used in the patch series:
> https://patchwork.freedesktop.org/patch/128962/

That's doesn't require the things that are left to be split into
separate functions.

> 
> That was the only reason I have split hf-vsdb parsing patch into 3 parts
> - parse SCDC and scrambling info (here)
> - with YUV420 deep color ( upcoming )
> - with 3d handling (upcoming)
> 
> So I am confused, split or no split :-) ?
> 
> - Shashank

Sharma, Shashank Feb. 2, 2017, 10:35 a.m. UTC | #10

Regards

Shashank


On 2/2/2017 3:58 PM, Ville Syrjälä wrote:
> On Thu, Feb 02, 2017 at 03:46:55PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/2/2017 3:21 PM, Ville Syrjälä wrote:
>>> On Thu, Feb 02, 2017 at 11:18:51AM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 2/1/2017 10:02 PM, Ville Syrjälä 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 <[e-mail]>
>>>>>> ---
>>>>>>     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)
>>>>> That names seems off. Should probably be drm_parse_hdmi_forum_vsdb() or
>>>>> something.
>>>> Actually, unlike the last patch set, we are not parsing the whole
>>>> hf_vsdb, but parsing it only for
>>>> scrambling status byte (hf_vsdb[5]). But may be I can make it
>>>> drm_detect_scrambling_from_hfvsdb
>>>> ot something similar. We will have more hf_vsdb parsing for 3d flags,
>>>> yuv420_deep_color etc.
>>> Well, so far I'm not seeing much point in splitting it up. So I'd stuff
>>> it all into one place, for now at least.
>> I had published a patch just doing as you suggested which was parsing
>> complete hf-vsdb in one shot, and adding info in
>> hdmi_info structure, to accommodate it.
>> (Patch https://patchwork.freedesktop.org/patch/128963/ and
>> https://patchwork.freedesktop.org/patch/128962/)
>>
>> But you gave a review comment not to add anything which is not being
>> used in the patch series:
>> https://patchwork.freedesktop.org/patch/128962/
> That's doesn't require the things that are left to be split into
> separate functions.
So please let me know if my understanding is correct.
- Name the function as *_parse_hfvsdb only
- Call it while parsing the EDID extension blocks (the same place)
- Keep on adding parsing of the HF-VSDB blocks, in the same function, in 
an incremental way, as we add patches for them.
In this way the function won't be split into multiple small functions, 
but all the parsing will be done in one function.

- Shashank
>> That was the only reason I have split hf-vsdb parsing patch into 3 parts
>> - parse SCDC and scrambling info (here)
>> - with YUV420 deep color ( upcoming )
>> - with 3d handling (upcoming)
>>
>> So I am confused, split or no split :-) ?
>>
>> - Shashank

Thierry Reding Feb. 2, 2017, 6:13 p.m. UTC | #11

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 <[e-mail]>
> > > ---
> > >   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

Sharma, Shashank Feb. 3, 2017, 4:03 a.m. UTC | #12

Regards

Shashank


On 2/2/2017 11:43 PM, Thierry Reding wrote:
> 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 <[e-mail]>
>>>> ---
>>>>    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
Well, makes perfect sense, will change the code as suggested :=)

- Shashank

Patch

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.
+	 * 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
+ *
+ * Read the scrambler status over SCDC channel, and check the
+ * scrambling status.
+ *
+ * Return: True if the scrambling is enabled, false otherwise.
+ */
+
+bool drm_check_scrambling_status(struct i2c_adapter *adapter)
+{
+	u8 status;
+
+	if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
+		DRM_ERROR("Failed to read scrambling status\n");
+		return false;
+	}
+
+	status &= SCDC_SCRAMBLING_STATUS;
+	return status != 0;
+}
+
+/**
+ * drm_enable_scrambling - enable scrambling
+ * @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)
+{
+	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");
+		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;
+}
+
+/**
+ * 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;
+}
+
 static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
 					   const u8 *hdmi)
 {
@@ -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.
+	 */
+	bool supported;
+	/**
+	 * @low_clocks: scrambling supported for rates <= 340Mhz.
+	 */
+	bool low_clocks;
+	/**
+	 * @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;
 };
 
 /**
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 43fb0ac..d24c974 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -462,5 +462,9 @@  void drm_edid_get_monitor_name(struct edid *edid, char *name,
 struct drm_display_mode *drm_mode_find_dmt(struct drm_device *dev,
 					   int hsize, int vsize, int fresh,
 					   bool rb);
-
+bool drm_enable_scrambling(struct drm_connector *connector,
+				struct i2c_adapter *adapter, bool force);
+bool drm_disable_scrambling(struct drm_connector *connector,
+				struct i2c_adapter *adapter, bool force);
+bool drm_check_scrambling_status(struct i2c_adapter *adapter);
 #endif /* __DRM_EDID_H__ */