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.
> +
> +/**
> + * 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.
> + 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.
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.
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
