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

Sharma, Shashank shashank.sharma at intel.com
Thu Feb 23 12:13:15 UTC 2017


Regards

Shashank


On 2/23/2017 5:15 PM, Ville Syrjälä wrote:
> On Thu, Feb 23, 2017 at 09:05:16AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/22/2017 10:54 PM, Ville Syrjälä wrote:
>>> On Wed, Feb 22, 2017 at 06:48:29PM +0530, Shashank Sharma wrote:
>>>> HDMI 2.0 spec mandates scrambling for modes with pixel clock higher
>>>> than 340 MHz. 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.
>>>>
>>>> V2: Addressed review comments from Thierry, Ville and Dhinakaran
>>>> Thierry:
>>>> - Mhz -> MHz in comments and commit message.
>>>> - i2c -> I2C in comments.
>>>> - Fix the function documentations, keep in sync with drm_scdc_helper.c
>>>> - drm_connector -> DRM connector.
>>>> - Create structure for SCDC, and save scrambling status inside that,
>>>>     in a sub-structure.
>>>> - Call this sub-structure scrambling instead of scr_info.
>>>> - low_rates -> low_clocks in scrambling status structure.
>>>> - Store the return value of I2C read/write and print the error code
>>>>     in case of failure.
>>>>
>>>> Thierry and Ville:
>>>> - Move the scrambling enable/disable/query functions in
>>>>     drm_scdc_helper.c file.
>>>> - Add drm_SCDC prefix for the functions.
>>>> - Optimize the return statement from function
>>>>     drm_SCDC_check_scrambling_status.
>>>>
>>>> Ville:
>>>> - Dont overwrite saved max TMDS clock value in display_info,
>>>>     if max tmds clock from HF-VSDB is not > 340 MHz.
>>>> - drm_detect_hdmi_scrambling -> drm_parse_hdmi_forum_vsdb.
>>>> - Remove dynamic tracking of SCDC status from DRM layer, force bool.
>>>> - Program clock ratio bit also, while enabling scrambling.
>>>>
>>>> Dhinakaran:
>>>> - Add a comment about *5000 while extracting max clock supported.
>>>>
>>>> V3: Addressed review comments from Jose.
>>>> - Create separate functions to enable scrambling and to set TMDS
>>>>     clock/character rate ratio.
>>>>
>>>> V4: Rebase
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_edid.c        |  33 +++++++-
>>>>    drivers/gpu/drm/drm_scdc_helper.c | 164 ++++++++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_connector.h       |  19 +++++
>>>>    include/drm/drm_edid.h            |   6 +-
>>>>    include/drm/drm_scdc_helper.h     |  41 ++++++++++
>>>>    5 files changed, 261 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>>>> index 63b79be..1e18c0a 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>
>>>>    
>>>>    #include "drm_crtc_internal.h"
>>>>    
>>>> @@ -3811,13 +3812,43 @@ enum hdmi_quantization_range
>>>>    static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
>>>>    				 const u8 *hf_vsdb)
>>>>    {
>>>> -	struct drm_hdmi_info *hdmi = &connector->display_info.hdmi;
>>>> +	struct drm_display_info *display = &connector->display_info;
>>>> +	struct drm_hdmi_info *hdmi = &display->hdmi;
>>>>    
>>>>    	if (hf_vsdb[6] & 0x80) {
>>>>    		hdmi->scdc.supported = true;
>>>>    		if (hf_vsdb[6] & 0x40)
>>>>    			hdmi->scdc.read_request = true;
>>>>    	}
>>>> +
>>>> +	/*
>>>> +	 * All HDMI 2.0 monitors must support scrambling at rates > 340 MHz.
>>>> +	 * 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 (let's check)
>>>> +	 * * SCDC support available (let's check)
>>>> +	 * Lets check it out.
>>>> +	 */
>>>> +
>>>> +	if (hf_vsdb[5]) {
>>>> +		/* max clock is 5000 KHz times block value */
>>>> +		u32 max_tmds_clock = hf_vsdb[5] * 5000;
>>>> +		struct drm_scdc *scdc = &hdmi->scdc;
>>>> +
>>>> +		if (max_tmds_clock > 340000) {
>>>> +			display->max_tmds_clock = max_tmds_clock;
>>>> +			DRM_DEBUG_KMS("HF-VSDB: max TMDS clock %d kHz\n",
>>>> +				display->max_tmds_clock);
>>>> +		}
>>>> +
>>>> +		if (scdc->supported) {
>>>> +			scdc->scrambling.supported = true;
>>>> +
>>>> +			/* Few sinks support scrambling for cloks < 340M */
>>>> +			if ((hf_vsdb[6] & 0x8))
>>>> +				scdc->scrambling.low_rates = true;
>>>> +		}
>>>> +	}
>>>>    }
>>>>    
>>>>    static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>>>> diff --git a/drivers/gpu/drm/drm_scdc_helper.c b/drivers/gpu/drm/drm_scdc_helper.c
>>>> index fe0e121..c0cf82b 100644
>>>> --- a/drivers/gpu/drm/drm_scdc_helper.c
>>>> +++ b/drivers/gpu/drm/drm_scdc_helper.c
>>>> @@ -22,8 +22,10 @@
>>>>     */
>>>>    
>>>>    #include <linux/slab.h>
>>>> +#include <linux/delay.h>
>>>>    
>>>>    #include <drm/drm_scdc_helper.h>
>>>> +#include <drm/drmP.h>
>>>>    
>>>>    /**
>>>>     * DOC: scdc helpers
>>>> @@ -109,3 +111,165 @@ ssize_t drm_scdc_write(struct i2c_adapter *adapter, u8 offset,
>>>>    	return err;
>>>>    }
>>>>    EXPORT_SYMBOL(drm_scdc_write);
>>>> +
>>>> +/**
>>>> + * drm_scdc_check_scrambling_status - what is status of scrambling?
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Reads the scrambler status over SCDC, and checks the
>>>> + * scrambling status.
>>>> + *
>>>> + * Returns:
>>>> + * True if the scrambling is enabled, false otherwise.
>>>> + */
>>>> +
>>>> +bool drm_scdc_check_scrambling_status(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 status;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read scrambling status, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return status & SCDC_SCRAMBLING_STATUS;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_check_scrambling_status);
>>> s/check/get/ would make more sense to me.
>> got it.
>>>> +
>>>> +/**
>>>> + * drm_scdc_enable_scrambling - enable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and enables scrambling
>>>> + * Returns:
>>>> + * True if scrambling is successfully enabled, false otherwise.
>>>> + */
>>>> +
>>>> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config |= SCDC_SCRAMBLING_ENABLE;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_enable_scrambling);
>>>> +
>>>> +/**
>>>> + * drm_scdc_disable_scrambling - disable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Write the TMDS config over SCDC channel, and disable scrambling
>>>> + * Return: True if scrambling is successfully disabled, false otherwise.
>>>> + */
>>>> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config &= ~SCDC_SCRAMBLING_ENABLE;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_disable_scrambling);
>>>> +
>>>> +/**
>>>> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio to 1/40
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config |= SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to set TMDS clock ratio, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * The spec says that the source should wait minimum 1ms and maximum
>>>> +	 * 100ms after writing the TMDS config for clock ratio.
>>>> +	 */
>>>> +	usleep_range(1000, 100000);
>>> Allowing the max to be 100ms here seems overly generous to me. In by
>>> allowing 100ms we might be violating the spec since it will take a bit
>>> of additonal time before the driver will enable the output. So I'd just
>>> use something like 1-2 msec.
>> Makes sense, but wouldn't 1-2 ms be too small ?
>> Do you think we should allow at least 10ms ?
> I don't think there's much merit in going that high. We do still want to
> keep the modeset as short as possible, so explicitly saying that we are
> happy to delay for up to 10ms doesn't seem like the best idea to me.
Got it, 2 ms it is.
- Shashank
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_set_high_tmds_clock_ratio);
>>>> +
>>>> +/**
>>>> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio back to 1/10 (from 1/40)
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter)
>>>> +{
>>>> +	u8 config;
>>>> +	int ret;
>>>> +
>>>> +	ret = drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to read tmds config, err=%d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	config &= ~SCDC_TMDS_BIT_CLOCK_RATIO_BY_40;
>>>> +
>>>> +	ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>>>> +	if (ret < 0) {
>>>> +		DRM_ERROR("Failed to clear TMDS clock ratio, error %d\n", ret);
>>>> +		return false;
>>>> +	}
>>>> +
>>>> +	/*
>>>> +	 * The spec says that the source should wait minimum 1ms and maximum
>>>> +	 * 100ms after writing the TMDS config for clock ratio.
>>>> +	 */
>>>> +	usleep_range(1000, 100000);
>>>> +	return true;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_scdc_clear_high_tmds_clock_ratio);
>>> Having separate set/clear functions just leads to duplicated code IMO.
>>> I'd just pass the desired state as a parameter.
>> Ok, I will make a common function for both set/clear, and pass the
>> enable/disable state
>> to it (similar to that which handles monitor scrambling)
>>> And what I suggested earlier was to even combine this with the scrambling
>>> write which would reduce the i2c traffic from 2 read and 2 writes to
>>> just 1 write. But I can live with them being separate as well.
>> Thanks. Actually, if you see V2, this first implementation was just as
>> you suggested.
>> But there was a problem here, few monitors support scrambling at a clock
>> lower than
>> 340Mhz too, in these cases we wont set the high_tmds_clock_ratio but
>> only set the scrambling.
> Then the caller just passes clock_ratio=low and scrambling=true. I don't
> see the problem.
>
>> This was comment from Jose, which we addressed into splitting this into
>> two functions, which gives
>> more control to caller function, by selecting what they want to set. but
>> yeah, cant beat the I2C traffic optimization.
>>
>> - Shashank
>>>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>>>> index 6d5304e..78618308 100644
>>>> --- a/include/drm/drm_connector.h
>>>> +++ b/include/drm/drm_connector.h
>>>> @@ -90,6 +90,20 @@ enum subpixel_order {
>>>>    
>>>>    };
>>>>    
>>>> +/**
>>>> + * struct drm_scrambling: sink's scrambling support.
>>>> + */
>>>> +struct drm_scrambling {
>>>> +	/**
>>>> +	 * @supported: scrambling supported for rates > 340 Mhz.
>>>> +	 */
>>>> +	bool supported;
>>>> +	/**
>>>> +	 * @low_rates: scrambling supported for rates <= 340 Mhz.
>>>> +	 */
>>>> +	bool low_rates;
>>>> +};
>>>> +
>>>>    /*
>>>>     * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
>>>>     *
>>>> @@ -105,8 +119,13 @@ struct drm_scdc {
>>>>    	 * @read_request: sink is capable of generating scdc read request.
>>>>    	 */
>>>>    	bool read_request;
>>>> +	/**
>>>> +	 * @scrambling: sink's scrambling capabilities
>>>> +	 */
>>>> +	struct drm_scrambling scrambling;
>>>>    };
>>>>    
>>>> +
>>>>    /**
>>>>     * struct drm_hdmi_info - runtime information about the connected HDMI sink
>>>>     *
>>>> 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__ */
>>>> diff --git a/include/drm/drm_scdc_helper.h b/include/drm/drm_scdc_helper.h
>>>> index 93b07bc..c5f2229 100644
>>>> --- a/include/drm/drm_scdc_helper.h
>>>> +++ b/include/drm/drm_scdc_helper.h
>>>> @@ -129,4 +129,45 @@ static inline int drm_scdc_writeb(struct i2c_adapter *adapter, u8 offset,
>>>>    	return drm_scdc_write(adapter, offset, &value, sizeof(value));
>>>>    }
>>>>    
>>>> +/**
>>>> + * drm_scdc_enable_scrambling - enable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and enables scrambling
>>>> + * Returns:
>>>> + * True if scrambling is successfully enabled, false otherwise.
>>>> + */
>>>> +
>>>> +bool drm_scdc_enable_scrambling(struct i2c_adapter *adapter);
>>>> +
>>>> +/**
>>>> + * drm_scdc_disable_scrambling - disable scrambling
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Write the TMDS config over SCDC channel, and disable scrambling
>>>> + * Return: True if scrambling is successfully disabled, false otherwise.
>>>> + */
>>>> +bool drm_scdc_disable_scrambling(struct i2c_adapter *adapter);
>>>> +
>>>> +/**
>>>> + * drm_scdc_set_high_tmds_clock_ratio - set TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio to 1/40
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_set_high_tmds_clock_ratio(struct i2c_adapter *adapter);
>>>> +
>>>> +/**
>>>> + * drm_scdc_clear_high_tmds_clock_ratio - clear TMDS clock ratio
>>>> + * @adapter: I2C adapter for DDC channel
>>>> + *
>>>> + * Writes the TMDS config over SCDC channel, and sets TMDS
>>>> + * clock ratio back to 1/10 (from 1/40)
>>>> + * Returns:
>>>> + * True if write is successful, false otherwise.
>>>> + */
>>>> +bool drm_scdc_clear_high_tmds_clock_ratio(struct i2c_adapter *adapter);
>>>>    #endif
>>>> -- 
>>>> 1.9.1



More information about the dri-devel mailing list