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

[Jose Abreu <Jose.Abreu@synopsys.com>]

Hi Shashank,



On 07-02-2017 16:19, Sharma, Shashank wrote:
> 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 ?

Hmm, ok. It was the safest thing to do but if we have monitors
with scrambling support and without this then its better not to.

Best regards,
Jose Miguel Abreu

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx