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

Sharma, Shashank
2017-02-07T16:19:19Z

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

Regards

Shashank

On 2/7/2017 4:44 PM, Jose Abreu wrote:
> Hi Shashank,
>
>
>
> On 06-02-2017 13:59, 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.
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>> drivers/gpu/drm/drm_edid.c | 33 ++++++++++++-
>> drivers/gpu/drm/drm_scdc_helper.c | 100 ++++++++++++++++++++++++++++++++++++++
>> include/drm/drm_connector.h | 19 ++++++++
>> include/drm/drm_edid.h | 6 ++-
>> include/drm/drm_scdc_helper.h | 20 ++++++++
>> 5 files changed, 176 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index a487b80..dc85eb9 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)) || \
>> @@ -3805,13 +3806,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))
> BIT(3) ?
Yes, bit 3 is LTE_340Mcsc_scramble, indicating that the sink support
scrambling at rates below 340Mhz too, isn't it ?
>
>> + 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..311f62e 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,101 @@ 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;
> "return (status & SCDC_SCRAMBLING_STATUS) > 0;" ?
I think Jani has made an agreement already on this.
>
>> +}
>> +EXPORT_SYMBOL(drm_scdc_check_scrambling_status);
>> +
>> +/**
>> + * 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 | SCDC_TMDS_BIT_CLOCK_RATIO_BY_40);
> Hmm, I did not read the spec exhaustively but shouldn't the clock
> ratio by 40 only be set for data rates above 3.4Gbps?
You are right, for few monitors scrambling can be done below 340 MHz
too, and I am not sure if we should
set the clock ratio bit on that. Let me check the spec for those cases.
>> +
>> + ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>> + if (ret < 0) {
>> + DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>> + return false;
>> + }
>> +
>> + /*
>> + * The spec says that the source should wait min 1ms and max 100ms
>> + * after writing the TMDS config for clock ratio. Lets obey the spec.
>> + */
>> + usleep_range(1000, 100000);
> Shall we read here the clock_detected status to make sure the
> sink is okay?
This is optional in spec, so I am afraid few monitors wont implement
this, and we will unnecessary add
lot of noise in code. Do you think so ?

- Shashank
>> + 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 | SCDC_TMDS_BIT_CLOCK_RATIO_BY_40);
>> +
>> + ret = drm_scdc_writeb(adapter, SCDC_TMDS_CONFIG, config);
>> + if (ret < 0) {
>> + DRM_ERROR("Failed to enable scrambling, error %d\n", ret);
>> + return false;
>> + }
>> +
>> + /*
>> + * The spec says that the source should wait min 1ms and max 100ms
>> + * after writing the TMDS config for clock ratio. Lets obey the spec.
>> + */
>> + usleep_range(1000, 100000);
>> + return true;
>> +}
>> +EXPORT_SYMBOL(drm_scdc_disable_scrambling);
>> 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..dc727a5 100644
>> --- a/include/drm/drm_scdc_helper.h
>> +++ b/include/drm/drm_scdc_helper.h
>> @@ -129,4 +129,24 @@ 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);
>> +
>> #endif
> Best regards,
> Jose Miguel Abreu
>

[-- Attachment #1.2: Type: text/html, Size: 49097 bytes --]

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