[PATCH 4/6] drm: scrambling support in drm layer
Sharma, Shashank
shashank.sharma at intel.com
Fri Feb 3 04:03:15 UTC 2017
Regards
Shashank
On 2/2/2017 11:43 PM, Thierry Reding wrote:
> On Thu, Feb 02, 2017 at 11:08:22AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/1/2017 10:02 PM, Thierry Reding 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 at 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)
>>>> +{
>>>> + 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.
>>> In comments below you use Mhz as the abbreviations. This should be
>>> consistent. Also I think "MHz" is actually the correct spelling.
>> Agree.
>>>> + * 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);
>>>> +
>>>> + 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
>>> "I2C", same in other parts of this patch.
>> Got it.
>>>> + *
>>>> + * Read the scrambler status over SCDC channel, and check the
>>>> + * scrambling status.
>>>> + *
>>>> + * Return: True if the scrambling is enabled, false otherwise.
>>> I think the rest of DRM/KMS kerneldoc tries to use "Returns:\n" as a
>>> standard way to document return values.
>> Ok.
>>>> + */
>>>> +
>>>> +bool drm_check_scrambling_status(struct i2c_adapter *adapter)
>>> Maybe use a drm_scdc_*() prefix for this to make it more consistent with
>>> other SCDC API.
>>>
>>> While at it, would this not be better located in drm_scdc.c along with
>>> the other helpers? drm_edid.c is more focussed on the parsing aspects of
>>> all things EDID.
>> Yeah, the same is mentioned by Ville too, will do that.
>>>> +{
>>>> + u8 status;
>>>> +
>>>> + if (drm_scdc_readb(adapter, SCDC_SCRAMBLER_STATUS, &status) < 0) {
>>> How about storing the error code...
>>>
>>>> + DRM_ERROR("Failed to read scrambling status\n");
>>> ... and making it part of the error message? Sometimes its useful to
>>> know what exact error triggered this because it helps narrowing down
>>> where things went wrong.
>> Agree, in fact while debugging and testing this patch series, I had to print
>> it explicitly.
>>>> + return false;
>>>> + }
>>>> +
>>>> + status &= SCDC_SCRAMBLING_STATUS;
>>>> + return status != 0;
>>> Maybe make this a single line:
>>>
>>> return (status & SCDC_SCRAMBLING_STATUS) != 0;
>> Got it.
>>>> +}
>>>> +
>>>> +/**
>>>> + * drm_enable_scrambling - enable scrambling
>>>> + * @connector: target drm_connector
>>> "target DRM connector"?
>> Got it.
>>>> + * @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)
>>> I think I'd move this to drm_scdc.c as well because it primarily
>>> operates on SCDC. If you do so, might be worth making adapter the first
>>> argument for consistency with other SCDC API.
>> Agree, will move it.
>>>> +{
>>>> + 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;
>>>> + }
>>>> +
>>>> + if (drm_scdc_readb(adapter, SCDC_TMDS_CONFIG, &config) < 0) {
>>>> + DRM_ERROR("Failed to read tmds config\n");
>>> Maybe also print the error code?
>> Ok.
>>>> + 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");
>>> Same here.
>> Ok
>>>> + 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;
>>>> +}
>>> Same comments as for drm_enable_scrambling().
>> Got it.
>>>> @@ -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.
>>> I think it's common to separate number and unit by a space, so "340
>>> MHz".
>> Got it.
>>>> + */
>>>> + bool supported;
>>>> + /**
>>>> + * @low_clocks: scrambling supported for rates <= 340Mhz.
>>>> + */
>>>> + bool low_clocks;
>>> Maybe "low_rates" because a clock is technically the source of a signal
>>> that oscillates at the given rate.
>> Agree, will change it.
>>>> + /**
>>>> + * @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;
>>> Again, I think I'd drop _info and instead spell out "scrambling" to make
>>> this easier to remember.
>> Sure, can be done.
>>> Also I'm wondering why scdc_supported and scdc_rr are not in a structure
>>> if scrambling info is. Also since scrambling depends on SCDC, would it
>>> make sense to embed it in a struct drm_hdmi_scdc_info?
>> My opinion while designing was:
>> - SCDC rr support is platform specific, and a platform can choose not to
>> support read_req but just allow
>> scrambling using scdc read and write, hence kept that separate
>> - You need to look into this internal structure, only if scdc is supported.
>> - Also, SCDC registers can be used beyond scrambling too, like for error
>> detection, and other cases, so I thought
>> it would be better to keep it independent.
>>
>> Does it make sense ?
> Yes, I think that makes sense, but it's not what I was trying to say. =)
> What I was trying to say is that read request and scrambling are defined
> in the SCDC specification, and therefore they require SCDC. That's why I
> think the below is a natural representation because it captures the
> dependency in a hierarchy:
>
>>> struct drm_hdmi_scdc_scrambling_info {
>>> bool supported;
>>> bool low_rates;
>>> bool status;
>>> };
>>>
>>> struct drm_hdmi_scdc_info {
>>> bool supported;
>>> bool read_request;
>>>
>>> struct drm_hdmi_scdc_scrambling_info scrambling;
>>> };
> I should have added to the above:
>
> struct drm_hdmi_info {
> struct drm_hdmi_scdc_info scdc;
> };
>
> So when conditionalizing code this allows for a very natural ordering of
> the code:
>
> struct drm_display_info *info = ...;
> struct drm_hdmi_scdc_info *scdc = &info->hdmi.scdc;
>
> if (scdc->supported) {
> ...
>
> if (scdc->read_request) {
> ...
> e.g. set up handler for read requests
> ...
> }
>
> ...
>
> if (scdc->scrambling.supported) {
> if (mode->clock >= 340000 || scdc->scrambling.low_rates) {
> ...
> set up scrambling
> ...;
> }
> }
>
> ...
> }
>
> Thierry
Well, makes perfect sense, will change the code as suggested :=)
- Shashank
More information about the dri-devel
mailing list