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@xxxxxxxxx>
> >> ---
> >> 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.
> >> + 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
