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@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.
* 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.
- 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.
- */
+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.
+{
- 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.
return false;- }
- status &= SCDC_SCRAMBLING_STATUS;
- return status != 0;
Maybe make this a single line:
return (status & SCDC_SCRAMBLING_STATUS) != 0;
+}
+/**
- drm_enable_scrambling - enable scrambling
- @connector: target drm_connector
"target DRM connector"?
- @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.
+{
- 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?
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.
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().
@@ -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".
*/- 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.
- /**
* @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.
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?
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; };
Thierry