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@xxxxxxxxx>
---
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 ?
