[Intel-gfx] [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 Intel-gfx
mailing list