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

["Sharma, Shashank" <shashank.sharma@intel.com>]

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 <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/drm_edid.c  | 131 +++++++++++++++++++++++++++++++++++++++++++-
>>   include/drm/drm_connector.h |  24 ++++++++
>>   include/drm/drm_edid.h      |   6 +-
>>   3 files changed, 159 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 37902e5..f0d940a 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -37,6 +37,7 @@
>>   #include <drm/drm_edid.h>
>>   #include <drm/drm_encoder.h>
>>   #include <drm/drm_displayid.h>
>> +#include <drm/drm_scdc_helper.h>
>>   
>>   #define version_greater(edid, maj, min) \
>>   	(((edid)->version > (maj)) || \
>> @@ -3814,6 +3815,132 @@ static void drm_detect_hdmi_scdc(struct drm_connector *connector,
>>   	}
>>   }
>>   
>> +static void drm_detect_hdmi_scrambling(struct drm_connector *connector,
>> +				 const u8 *hf_vsdb)
> 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

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