[PATCH 0/3 v3] drm: bridge/dw-hdmi: Fixes for dw-hdmi and DWC support

[PATCH 0/3 v3] drm: bridge/dw-hdmi: Fixes for dw-hdmi and DWC support

[Jose Abreu]

In this patch series we add support for Synopsys DWC Phy
and improve current driver.

First patch adds the support and remaining patches correct
some minor things.

All patches were tested using HDMI compliance equipment.

Changes v2 -> v3:
- Fixed incorrect frequency values
- Added ISCR and ACP packets enable
- Moved EDID reading to .detect() callback

Changes v1 -> v2:
- Fixed Audio Infoframe

Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org

Jose Abreu (3):
  drm: bridge/dw-hdmi: Add support for DWC Phy
  drm: bridge/dw-hdmi: Enable ISCR1, ISCR2 and ACP packets
  drm: bridge/dw-hdmi: Move edid reading to .detect() callback

 drivers/gpu/drm/bridge/dw-hdmi.c | 103 +++++++++++++++++++++++++++++----------
 drivers/gpu/drm/bridge/dw-hdmi.h |   1 +
 include/drm/bridge/dw_hdmi.h     |   1 +
 3 files changed, 80 insertions(+), 25 deletions(-)

-- 
2.1.4

[PATCH 1/3 v3] drm: bridge/dw-hdmi: Add support for DWC Phy

[Jose Abreu]

This patch adds support for the Synopsys HDMI TX Phy in
bridge dw-hdmi.

The init flow is the same as the Rockchip Phy so we only
need to add one define and one if statement.

Also, the audio infoframe was fixed (before it was always
reporting 44.1k). With this patch this is now corrected
and validated. This is specific to Synopsys Phy.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---

Changes v2 -> v3:
- Fixed incorrect frequency values

Changes v1 -> v2:
- Fixed Audio Infoframe

 drivers/gpu/drm/bridge/dw-hdmi.c | 25 ++++++++++++++++++++++++-
 drivers/gpu/drm/bridge/dw-hdmi.h |  1 +
 include/drm/bridge/dw_hdmi.h     |  1 +
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 77ab473..b5fac27 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -215,6 +215,29 @@ static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
 	hdmi_writeb(hdmi, (n >> 16) & 0x0f, HDMI_AUD_N3);
 	hdmi_writeb(hdmi, (n >> 8) & 0xff, HDMI_AUD_N2);
 	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
+
+	/* Set Frame Composer Audio Sample sampling frequency */
+	if (hdmi->dev_type == DWC_HDMI) {
+		u8 val = 0x0;
+
+		switch (hdmi->sample_rate) {
+		case 32000:
+			val = 0x3;
+			break;
+		case 44100:
+			val = 0x0;
+			break;
+		case 48000:
+			val = 0x2;
+			break;
+		default:
+			dev_err(hdmi->dev, "unsupported sample rate (%d)\n",
+					hdmi->sample_rate);
+			return;
+		}
+
+		hdmi_writeb(hdmi, val, HDMI_FC_AUDSCHNL7);
+	}
 }
 
 static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk)
@@ -833,7 +856,7 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep,
 	dw_hdmi_phy_gen2_txpwron(hdmi, 1);
 	dw_hdmi_phy_gen2_pddq(hdmi, 0);
 
-	if (hdmi->dev_type == RK3288_HDMI)
+	if ((hdmi->dev_type == RK3288_HDMI) || (hdmi->dev_type == DWC_HDMI))
 		dw_hdmi_phy_enable_spare(hdmi, 1);
 
 	/*Wait for PHY PLL lock */
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h b/drivers/gpu/drm/bridge/dw-hdmi.h
index fc9a560..5f137fd 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/dw-hdmi.h
@@ -162,6 +162,7 @@
 #define HDMI_FC_SPDDEVICEINF                    0x1062
 #define HDMI_FC_AUDSCONF                        0x1063
 #define HDMI_FC_AUDSSTAT                        0x1064
+#define HDMI_FC_AUDSCHNL7			0x106E
 #define HDMI_FC_DATACH0FILL                     0x1070
 #define HDMI_FC_DATACH1FILL                     0x1071
 #define HDMI_FC_DATACH2FILL                     0x1072
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index bae79f3..6e4fb2e 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -25,6 +25,7 @@ enum dw_hdmi_devtype {
 	IMX6Q_HDMI,
 	IMX6DL_HDMI,
 	RK3288_HDMI,
+	DWC_HDMI,
 };
 
 struct dw_hdmi_mpll_config {
-- 
2.1.4

[PATCH 2/3 v3] drm: bridge/dw-hdmi: Enable ISCR1, ISCR2 and ACP packets

[Jose Abreu]

Currently ISCR and ACP packets are not being sent causing
HDMI compliance tests like CTS 7-19 HDMI 1.4b to fail.

With this pacth the mentioned packets are activated when
needed.

Verified using HDMI compliance equipment.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---

This patch was only introduced in v3.

 drivers/gpu/drm/bridge/dw-hdmi.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b5fac27..9122a20 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -127,6 +127,7 @@ struct dw_hdmi {
 	void __iomem *regs;
 	bool sink_is_hdmi;
 	bool sink_has_audio;
+	bool sink_supports_ai;
 
 	struct mutex mutex;		/* for state below and previous_mode */
 	enum drm_connector_force force;	/* mutex-protected force state */
@@ -216,6 +217,21 @@ static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
 	hdmi_writeb(hdmi, (n >> 8) & 0xff, HDMI_AUD_N2);
 	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
 
+	/* Set ISCR1, ISCR2, and ACP packets to automatic scheduling */
+	if (hdmi->sink_supports_ai) {
+		dev_dbg(hdmi->dev, "sink supports AI packets\n");
+		hdmi_writeb(hdmi, 0x06, HDMI_FC_ISCR1_0);
+		hdmi_writeb(hdmi, 0x03, HDMI_FC_DATAUTO0);
+		hdmi_writeb(hdmi, 0x01, HDMI_FC_DATAUTO1);
+		hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
+	} else {
+		dev_dbg(hdmi->dev, "sink does not support AI packets\n");
+		hdmi_writeb(hdmi, 0x00, HDMI_FC_ISCR1_0);
+		hdmi_writeb(hdmi, 0x00, HDMI_FC_DATAUTO0);
+		hdmi_writeb(hdmi, 0x00, HDMI_FC_DATAUTO1);
+		hdmi_writeb(hdmi, 0x00, HDMI_FC_DATAUTO2);
+	}
+
 	/* Set Frame Composer Audio Sample sampling frequency */
 	if (hdmi->dev_type == DWC_HDMI) {
 		u8 val = 0x0;
@@ -1474,8 +1490,13 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 		/* Store the ELD */
 		drm_edid_to_eld(connector, edid);
 		kfree(edid);
+
+		hdmi->sink_supports_ai = connector->eld[5] & (0x1 << 1);
 	} else {
 		dev_dbg(hdmi->dev, "failed to get edid\n");
+		hdmi->sink_is_hdmi = false;
+		hdmi->sink_has_audio = false;
+		hdmi->sink_supports_ai = false;
 	}
 
 	return ret;
-- 
2.1.4

Re: [PATCH 2/3 v3] drm: bridge/dw-hdmi: Enable ISCR1, ISCR2 and ACP packets

[Russell King - ARM Linux]

On Thu, Aug 04, 2016 at 11:44:50AM +0100, Jose Abreu wrote:
> Currently ISCR and ACP packets are not being sent causing
> HDMI compliance tests like CTS 7-19 HDMI 1.4b to fail.

Hmm.  Reading the HDMI specifications (v1.3, being the publically
available one), the specification does _not_ say that a source _must_
transmit ISRC and ACP packets if Supports_AI is set.

What it does say is that:
* a sink shall set Supports_AI if it supports at least one function
  which uses ISRC or ACP packets, and it shall accept such packets.
* a source shall not transmit ISRC or ACP packets to a sink that
  has Supports_AI=0.

I don't see anything that mandates the transmission of ISRC and ACP
packets when Supports_AI is set.

Moreover, transmission of wrong ISRC information probably goes against
the HDMI spec section 8.8, which requires specific ISRC status values
in HB1 - I don't see how that _could_ be handled, as the kernel has no
knowledge about the EAN of the audio content being played, nor when a
track starts or finishes.

Please clarify.

> +	/* Set ISCR1, ISCR2, and ACP packets to automatic scheduling */
> +	if (hdmi->sink_supports_ai) {
> +		dev_dbg(hdmi->dev, "sink supports AI packets\n");
> +		hdmi_writeb(hdmi, 0x06, HDMI_FC_ISCR1_0);
> +		hdmi_writeb(hdmi, 0x03, HDMI_FC_DATAUTO0);
> +		hdmi_writeb(hdmi, 0x01, HDMI_FC_DATAUTO1);
> +		hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);

So what data gets sent in these packets - I guess whatever random data
happens to be in the HDMI_FC_ACP* and HDMI_FC_ISCR1_* registers at the
time the sending is enabled.  I don't think that's particularly a good
idea.

> @@ -1474,8 +1490,13 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>  		/* Store the ELD */
>  		drm_edid_to_eld(connector, edid);
>  		kfree(edid);
> +
> +		hdmi->sink_supports_ai = connector->eld[5] & (0x1 << 1);

We have a definition for this bit - DRM_ELD_SUPPORTS_AI.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 2/3 v3] drm: bridge/dw-hdmi: Enable ISCR1, ISCR2 and ACP packets

[Jose Abreu]

Hi Russell,


On 04-08-2016 16:32, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 11:44:50AM +0100, Jose Abreu wrote:
>> Currently ISCR and ACP packets are not being sent causing
>> HDMI compliance tests like CTS 7-19 HDMI 1.4b to fail.
> Hmm.  Reading the HDMI specifications (v1.3, being the publically
> available one), the specification does _not_ say that a source _must_
> transmit ISRC and ACP packets if Supports_AI is set.
>
> What it does say is that:
> * a sink shall set Supports_AI if it supports at least one function
>   which uses ISRC or ACP packets, and it shall accept such packets.
> * a source shall not transmit ISRC or ACP packets to a sink that
>   has Supports_AI=0.
>
> I don't see anything that mandates the transmission of ISRC and ACP
> packets when Supports_AI is set.
>
> Moreover, transmission of wrong ISRC information probably goes against
> the HDMI spec section 8.8, which requires specific ISRC status values
> in HB1 - I don't see how that _could_ be handled, as the kernel has no
> knowledge about the EAN of the audio content being played, nor when a
> track starts or finishes.
>
> Please clarify.

You are correct. I ran the test again and indeed it is not
failing but it is skipped. Sorry for the confusion. Anyway, with
this patch the test that verifies the ISRC packets succeeds so I
think it would be a nice addition.

>
>> +	/* Set ISCR1, ISCR2, and ACP packets to automatic scheduling */
>> +	if (hdmi->sink_supports_ai) {
>> +		dev_dbg(hdmi->dev, "sink supports AI packets\n");
>> +		hdmi_writeb(hdmi, 0x06, HDMI_FC_ISCR1_0);
>> +		hdmi_writeb(hdmi, 0x03, HDMI_FC_DATAUTO0);
>> +		hdmi_writeb(hdmi, 0x01, HDMI_FC_DATAUTO1);
>> +		hdmi_writeb(hdmi, 0x11, HDMI_FC_DATAUTO2);
> So what data gets sent in these packets - I guess whatever random data
> happens to be in the HDMI_FC_ACP* and HDMI_FC_ISCR1_* registers at the
> time the sending is enabled.  I don't think that's particularly a good
> idea.

It is not random as the registers defaults to zero per databook.
It is sending an empty packet.

>> @@ -1474,8 +1490,13 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
>>  		/* Store the ELD */
>>  		drm_edid_to_eld(connector, edid);
>>  		kfree(edid);
>> +
>> +		hdmi->sink_supports_ai = connector->eld[5] & (0x1 << 1);
> We have a definition for this bit - DRM_ELD_SUPPORTS_AI.
>

Thanks. Will change in next version if you agree with this
addition. If not I will drop the patch.


Best regards,
Jose Miguel Abreu

[PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Jose Abreu]

When running HDMI compliance tests we noticed that sometimes
the edid changes but the get_modes() function is not called
so the edid is not updated. Moving the edid reading to the
detect() callback ensures that the edid is correctly updated
after an hotplug.

Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Cc: Carlos Palminha <palminha@synopsys.com>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---

This patch was only introduced in v3.

 drivers/gpu/drm/bridge/dw-hdmi.c | 67 +++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 9122a20..c795888 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -31,8 +31,6 @@
 #include "dw-hdmi.h"
 #include "dw-hdmi-audio.h"
 
-#define HDMI_EDID_LEN		512
-
 #define RGB			0
 #define YCBCR444		1
 #define YCBCR422_16BITS		2
@@ -117,7 +115,7 @@ struct dw_hdmi {
 
 	int vic;
 
-	u8 edid[HDMI_EDID_LEN];
+	struct edid *edid;
 	bool cable_plugin;
 
 	bool phy_enabled;
@@ -1457,6 +1455,7 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
 	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
 					     connector);
+	bool connected;
 
 	mutex_lock(&hdmi->mutex);
 	hdmi->force = DRM_FORCE_UNSPECIFIED;
@@ -1464,7 +1463,40 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 	dw_hdmi_update_phy_mask(hdmi);
 	mutex_unlock(&hdmi->mutex);
 
-	return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ?
+	connected = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD;
+
+	if (connected && hdmi->ddc) {
+		if (hdmi->edid) {
+			drm_mode_connector_update_edid_property(connector,
+					NULL);
+			kfree(hdmi->edid);
+			hdmi->edid = NULL;
+		}
+
+		hdmi->edid = drm_get_edid(connector, hdmi->ddc);
+		if (hdmi->edid) {
+			dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
+					hdmi->edid->width_cm,
+					hdmi->edid->height_cm);
+			drm_mode_connector_update_edid_property(connector,
+					hdmi->edid);
+			drm_edid_to_eld(connector, hdmi->edid);
+
+			hdmi->sink_is_hdmi =
+				drm_detect_hdmi_monitor(hdmi->edid);
+			hdmi->sink_has_audio =
+				drm_detect_monitor_audio(hdmi->edid);
+			hdmi->sink_supports_ai =
+				connector->eld[5] & (0x1 << 1);
+		} else {
+			dev_dbg(hdmi->dev, "failed to get edid\n");
+			hdmi->sink_is_hdmi = false;
+			hdmi->sink_has_audio = false;
+			hdmi->sink_supports_ai = false;
+		}
+	}
+
+	return connected ?
 		connector_status_connected : connector_status_disconnected;
 }
 
@@ -1472,33 +1504,10 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 {
 	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
 					     connector);
-	struct edid *edid;
 	int ret = 0;
 
-	if (!hdmi->ddc)
-		return 0;
-
-	edid = drm_get_edid(connector, hdmi->ddc);
-	if (edid) {
-		dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
-			edid->width_cm, edid->height_cm);
-
-		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
-		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
-		drm_mode_connector_update_edid_property(connector, edid);
-		ret = drm_add_edid_modes(connector, edid);
-		/* Store the ELD */
-		drm_edid_to_eld(connector, edid);
-		kfree(edid);
-
-		hdmi->sink_supports_ai = connector->eld[5] & (0x1 << 1);
-	} else {
-		dev_dbg(hdmi->dev, "failed to get edid\n");
-		hdmi->sink_is_hdmi = false;
-		hdmi->sink_has_audio = false;
-		hdmi->sink_supports_ai = false;
-	}
-
+	if (hdmi->edid)
+		ret = drm_add_edid_modes(connector, hdmi->edid);
 	return ret;
 }
 
-- 
2.1.4

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Russell King - ARM Linux]

On Thu, Aug 04, 2016 at 11:44:51AM +0100, Jose Abreu wrote:
> When running HDMI compliance tests we noticed that sometimes
> the edid changes but the get_modes() function is not called
> so the edid is not updated. Moving the edid reading to the
> detect() callback ensures that the edid is correctly updated
> after an hotplug.

Wouldn't this be a bug in the higher levels of DRM?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Jose Abreu]

Hi Russell,


On 04-08-2016 11:47, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 11:44:51AM +0100, Jose Abreu wrote:
>> When running HDMI compliance tests we noticed that sometimes
>> the edid changes but the get_modes() function is not called
>> so the edid is not updated. Moving the edid reading to the
>> detect() callback ensures that the edid is correctly updated
>> after an hotplug.
> Wouldn't this be a bug in the higher levels of DRM?
>

I am not sure if this is a bug in DRM or a bad implementation of
dw-hdmi. I've seen at least two more drivers that do the edid
reading at the .detect() callback: nouveau and gma500. This is
noticeable if while sending video the edid changes like in a
compliance environment. In the environment that I tested I was
sending video and swapped an edid that had support for AI packets
to another one without support and I noticed that although the
hotplug is generated the edid was not being updated.

Best regards,
Jose Miguel Abreu

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Russell King - ARM Linux]

On Thu, Aug 04, 2016 at 02:58:00PM +0100, Jose Abreu wrote:
> Hi Russell,
>
> I am not sure if this is a bug in DRM or a bad implementation of
> dw-hdmi. I've seen at least two more drivers that do the edid
> reading at the .detect() callback: nouveau and gma500. This is
> noticeable if while sending video the edid changes like in a
> compliance environment. In the environment that I tested I was
> sending video and swapped an edid that had support for AI packets
> to another one without support and I noticed that although the
> hotplug is generated the edid was not being updated.

The path should be:

- HPD interrupt
- drm_helper_hpd_irq_event()
  - generates uevent to userspace
  - output_poll_changed() called
    - drm_fb_helper_hotplug_event()
      - drm_fb_helper_probe_connector_modes()
        - connector->fill_modes()
          - drm_helper_probe_single_connector_modes()
            - connector_funcs->get_modes()

Now, if we don't have fbdev emulation, then things won't get much
fruther than output_poll_changed(), and the responsibility for
reading the EDID falls onto userspace.  However, as soon as userspace
or the kernel reads the new EDID, that is when any changes in it
should become visible.

Of course, that should occur in a timely fashion.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Jose Abreu]

Hi Russell,


On 04-08-2016 15:31, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 02:58:00PM +0100, Jose Abreu wrote:
>> Hi Russell,
>>
>> I am not sure if this is a bug in DRM or a bad implementation of
>> dw-hdmi. I've seen at least two more drivers that do the edid
>> reading at the .detect() callback: nouveau and gma500. This is
>> noticeable if while sending video the edid changes like in a
>> compliance environment. In the environment that I tested I was
>> sending video and swapped an edid that had support for AI packets
>> to another one without support and I noticed that although the
>> hotplug is generated the edid was not being updated.
> The path should be:
>
> - HPD interrupt
> - drm_helper_hpd_irq_event()
>   - generates uevent to userspace
>   - output_poll_changed() called
>     - drm_fb_helper_hotplug_event()
>       - drm_fb_helper_probe_connector_modes()
>         - connector->fill_modes()
>           - drm_helper_probe_single_connector_modes()
>             - connector_funcs->get_modes()
>
> Now, if we don't have fbdev emulation, then things won't get much
> fruther than output_poll_changed(), and the responsibility for
> reading the EDID falls onto userspace.  However, as soon as userspace
> or the kernel reads the new EDID, that is when any changes in it
> should become visible.
>
> Of course, that should occur in a timely fashion.
>

Hmm, I am not debugging it right now but I remember that
drm_fb_helper_probe_connector_modes() was not being called at the
time I set the new EDID but only after I stopped sending video (I
was using modetest).


Best regards,
Jose Miguel Abreu

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Russell King - ARM Linux]

On Thu, Aug 04, 2016 at 03:57:21PM +0100, Jose Abreu wrote:
> Hmm, I am not debugging it right now but I remember that
> drm_fb_helper_probe_connector_modes() was not being called at the
> time I set the new EDID but only after I stopped sending video (I
> was using modetest).

Please investigate - I'd prefer that your patch does not get applied
until we really know what's going on here.

Hmm, if you're using modetest, then userspace is setting a mode, and
userspace is in control of the DRM device - that's probably the reason
why you're not seeing anything happening - modetest probably doesn't
know anything about hotplug events, and so doesn't read the modes.

Have you tried with the framebuffer console and DRM fbdev emulation
enabled, without using modetest?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Jose Abreu]

Hi Russell,


On 04-08-2016 16:04, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 03:57:21PM +0100, Jose Abreu wrote:
>> Hmm, I am not debugging it right now but I remember that
>> drm_fb_helper_probe_connector_modes() was not being called at the
>> time I set the new EDID but only after I stopped sending video (I
>> was using modetest).
> Please investigate - I'd prefer that your patch does not get applied
> until we really know what's going on here.
>
> Hmm, if you're using modetest, then userspace is setting a mode, and
> userspace is in control of the DRM device - that's probably the reason
> why you're not seeing anything happening - modetest probably doesn't
> know anything about hotplug events, and so doesn't read the modes.
>
> Have you tried with the framebuffer console and DRM fbdev emulation
> enabled, without using modetest?
>

So, I didn't use framebuffer console but used X instead and it is
working as it should. I think we can drop this patch. I am now
making interoperability with DVI and I am facing the following
scenario:
    - I start the driver
    - An EDID is sent which tells the driver that HDMI is NOT
supported;
    - The driver configures itself to a DVI mode;

Until this point everything is working as it should. But:

    - Now I send an EDID which tells the driver that HDMI is
supported;
    - As the EDID has the same preferred mode the user will not
reconfigure the mode and there will be no change to HDMI mode.

The missing change to HDMI mode will cause the test to fail. The
workaround that I am using is to reconfigure to another video
mode and then configure to the preferred one but I think this
could be fixed in the driver, right?

Best regards,
Jose Miguel Abreu

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Russell King - ARM Linux]

On Thu, Aug 04, 2016 at 06:13:18PM +0100, Jose Abreu wrote:
> Hi Russell,
> 
> So, I didn't use framebuffer console but used X instead and it is
> working as it should. I think we can drop this patch. I am now
> making interoperability with DVI and I am facing the following
> scenario:
>     - I start the driver
>     - An EDID is sent which tells the driver that HDMI is NOT
> supported;
>     - The driver configures itself to a DVI mode;
> 
> Until this point everything is working as it should. But:
> 
>     - Now I send an EDID which tells the driver that HDMI is
> supported;
>     - As the EDID has the same preferred mode the user will not
> reconfigure the mode and there will be no change to HDMI mode.

The EDID should still be read, but as you say, userspace doesn't take
any action because it sees that the mode parameters are still the same,
as you have identified.

> The missing change to HDMI mode will cause the test to fail. The
> workaround that I am using is to reconfigure to another video
> mode and then configure to the preferred one but I think this
> could be fixed in the driver, right?

This one is extremely awkward - to fix it, we would need to check to
see whether we reconfigure the hardware each time we read the EDID,
and I don't think that's a particularly nice thing to do.

I'd like to hear what other DRM developers think about this issue.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Daniel Vetter]

On Fri, Aug 05, 2016 at 12:01:25AM +0100, Russell King - ARM Linux wrote:
> On Thu, Aug 04, 2016 at 06:13:18PM +0100, Jose Abreu wrote:
> > Hi Russell,
> > 
> > So, I didn't use framebuffer console but used X instead and it is
> > working as it should. I think we can drop this patch. I am now
> > making interoperability with DVI and I am facing the following
> > scenario:
> >     - I start the driver
> >     - An EDID is sent which tells the driver that HDMI is NOT
> > supported;
> >     - The driver configures itself to a DVI mode;
> > 
> > Until this point everything is working as it should. But:
> > 
> >     - Now I send an EDID which tells the driver that HDMI is
> > supported;
> >     - As the EDID has the same preferred mode the user will not
> > reconfigure the mode and there will be no change to HDMI mode.
> 
> The EDID should still be read, but as you say, userspace doesn't take
> any action because it sees that the mode parameters are still the same,
> as you have identified.
> 
> > The missing change to HDMI mode will cause the test to fail. The
> > workaround that I am using is to reconfigure to another video
> > mode and then configure to the preferred one but I think this
> > could be fixed in the driver, right?
> 
> This one is extremely awkward - to fix it, we would need to check to
> see whether we reconfigure the hardware each time we read the EDID,
> and I don't think that's a particularly nice thing to do.
> 
> I'd like to hear what other DRM developers think about this issue.

I pondored whether we should have a counter on each connector for probe
state, which helpers would increment whenever something changes, like:
- connector_status (as tracked by probe helpers)
- anything in the edid changes (when setting it
  drm_mode_connector_update_edid_property)
- other changes (like sink state changes in dpcd or whatever)

That way userspace would be able to reliably spot such changes and do a
new modeset.

The other side of this is making sure that the kernel doesn't optimize
away a modeset in this case. The problem is that current helpers (both
legacy and atomic) only look at the output routing and the display mode
and nothing else to decide whether a full modeset is required. At least
with atomic that's easy to fix:
- When the connector state changes we simply need to set the
  connectors_changed boolean in the corresponding CRTC state.
- We can use the probe lifecycle counter from above to do that: If it's
  changed in atomic_check, then we set connectors_changed to force a full
  modeset.

I guess with just the kernel changes (and not even upgrading userspace)
we'd probably fix many cases already.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Chris Wilson]

On Fri, Aug 05, 2016 at 10:06:08AM +0200, Daniel Vetter wrote:
> On Fri, Aug 05, 2016 at 12:01:25AM +0100, Russell King - ARM Linux wrote:
> > On Thu, Aug 04, 2016 at 06:13:18PM +0100, Jose Abreu wrote:
> > > Hi Russell,
> > > 
> > > So, I didn't use framebuffer console but used X instead and it is
> > > working as it should. I think we can drop this patch. I am now
> > > making interoperability with DVI and I am facing the following
> > > scenario:
> > >     - I start the driver
> > >     - An EDID is sent which tells the driver that HDMI is NOT
> > > supported;
> > >     - The driver configures itself to a DVI mode;
> > > 
> > > Until this point everything is working as it should. But:
> > > 
> > >     - Now I send an EDID which tells the driver that HDMI is
> > > supported;
> > >     - As the EDID has the same preferred mode the user will not
> > > reconfigure the mode and there will be no change to HDMI mode.
> > 
> > The EDID should still be read, but as you say, userspace doesn't take
> > any action because it sees that the mode parameters are still the same,
> > as you have identified.
> > 
> > > The missing change to HDMI mode will cause the test to fail. The
> > > workaround that I am using is to reconfigure to another video
> > > mode and then configure to the preferred one but I think this
> > > could be fixed in the driver, right?
> > 
> > This one is extremely awkward - to fix it, we would need to check to
> > see whether we reconfigure the hardware each time we read the EDID,
> > and I don't think that's a particularly nice thing to do.
> > 
> > I'd like to hear what other DRM developers think about this issue.
> 
> I pondored whether we should have a counter on each connector for probe
> state, which helpers would increment whenever something changes, like:
> - connector_status (as tracked by probe helpers)
> - anything in the edid changes (when setting it
>   drm_mode_connector_update_edid_property)
> - other changes (like sink state changes in dpcd or whatever)
> 
> That way userspace would be able to reliably spot such changes and do a
> new modeset.

Yes, please. I have had similar wishes for state changes and overall
modeset counters.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Jose Abreu]

Hi,


On 05-08-2016 09:13, Chris Wilson wrote:
> On Fri, Aug 05, 2016 at 10:06:08AM +0200, Daniel Vetter wrote:
>> On Fri, Aug 05, 2016 at 12:01:25AM +0100, Russell King - ARM Linux wrote:
>>> On Thu, Aug 04, 2016 at 06:13:18PM +0100, Jose Abreu wrote:
>>>> Hi Russell,
>>>>
>>>> So, I didn't use framebuffer console but used X instead and it is
>>>> working as it should. I think we can drop this patch. I am now
>>>> making interoperability with DVI and I am facing the following
>>>> scenario:
>>>>     - I start the driver
>>>>     - An EDID is sent which tells the driver that HDMI is NOT
>>>> supported;
>>>>     - The driver configures itself to a DVI mode;
>>>>
>>>> Until this point everything is working as it should. But:
>>>>
>>>>     - Now I send an EDID which tells the driver that HDMI is
>>>> supported;
>>>>     - As the EDID has the same preferred mode the user will not
>>>> reconfigure the mode and there will be no change to HDMI mode.
>>> The EDID should still be read, but as you say, userspace doesn't take
>>> any action because it sees that the mode parameters are still the same,
>>> as you have identified.
>>>
>>>> The missing change to HDMI mode will cause the test to fail. The
>>>> workaround that I am using is to reconfigure to another video
>>>> mode and then configure to the preferred one but I think this
>>>> could be fixed in the driver, right?
>>> This one is extremely awkward - to fix it, we would need to check to
>>> see whether we reconfigure the hardware each time we read the EDID,
>>> and I don't think that's a particularly nice thing to do.
>>>
>>> I'd like to hear what other DRM developers think about this issue.
>> I pondored whether we should have a counter on each connector for probe
>> state, which helpers would increment whenever something changes, like:
>> - connector_status (as tracked by probe helpers)
>> - anything in the edid changes (when setting it
>>   drm_mode_connector_update_edid_property)
>> - other changes (like sink state changes in dpcd or whatever)
>>
>> That way userspace would be able to reliably spot such changes and do a
>> new modeset.
> Yes, please. I have had similar wishes for state changes and overall
> modeset counters.
> -Chris
>

What about this: Assuming that the modes probed by EDID have the
picture aspect ratio field set we can check for one of this
probed modes when receiving a mode from userspace and then if the
modes match (except from the picture aspect field, which is not
set by userspace) then we can use the picture aspect value of the
probed mode. I am using something like this in my modeset callback:

    /* 'mode' comes from userspace, 'pmode' comes from EDID */
    if (mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_NONE) {
        struct drm_display_mode *pmode;

        list_for_each_entry(pmode, &connector.modes, head) {
            if (drm_mode_equal(pmode, mode))
                mode->picture_aspect_ratio =
pmode->picture_aspect_ratio;
        }
    }

Of course if the EDID has for example two exactly equal modes
that only differ in the picture aspect ratio then this will not
work unless user passes the picture aspect when setting the mode.

Best regards,
Jose Miguel Abreu

Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback

[Daniel Vetter]

On Mon, Aug 08, 2016 at 05:25:38PM +0100, Jose Abreu wrote:
> Hi,
> 
> 
> On 05-08-2016 09:13, Chris Wilson wrote:
> > On Fri, Aug 05, 2016 at 10:06:08AM +0200, Daniel Vetter wrote:
> >> On Fri, Aug 05, 2016 at 12:01:25AM +0100, Russell King - ARM Linux wrote:
> >>> On Thu, Aug 04, 2016 at 06:13:18PM +0100, Jose Abreu wrote:
> >>>> Hi Russell,
> >>>>
> >>>> So, I didn't use framebuffer console but used X instead and it is
> >>>> working as it should. I think we can drop this patch. I am now
> >>>> making interoperability with DVI and I am facing the following
> >>>> scenario:
> >>>>     - I start the driver
> >>>>     - An EDID is sent which tells the driver that HDMI is NOT
> >>>> supported;
> >>>>     - The driver configures itself to a DVI mode;
> >>>>
> >>>> Until this point everything is working as it should. But:
> >>>>
> >>>>     - Now I send an EDID which tells the driver that HDMI is
> >>>> supported;
> >>>>     - As the EDID has the same preferred mode the user will not
> >>>> reconfigure the mode and there will be no change to HDMI mode.
> >>> The EDID should still be read, but as you say, userspace doesn't take
> >>> any action because it sees that the mode parameters are still the same,
> >>> as you have identified.
> >>>
> >>>> The missing change to HDMI mode will cause the test to fail. The
> >>>> workaround that I am using is to reconfigure to another video
> >>>> mode and then configure to the preferred one but I think this
> >>>> could be fixed in the driver, right?
> >>> This one is extremely awkward - to fix it, we would need to check to
> >>> see whether we reconfigure the hardware each time we read the EDID,
> >>> and I don't think that's a particularly nice thing to do.
> >>>
> >>> I'd like to hear what other DRM developers think about this issue.
> >> I pondored whether we should have a counter on each connector for probe
> >> state, which helpers would increment whenever something changes, like:
> >> - connector_status (as tracked by probe helpers)
> >> - anything in the edid changes (when setting it
> >>   drm_mode_connector_update_edid_property)
> >> - other changes (like sink state changes in dpcd or whatever)
> >>
> >> That way userspace would be able to reliably spot such changes and do a
> >> new modeset.
> > Yes, please. I have had similar wishes for state changes and overall
> > modeset counters.
> > -Chris
> >
> 
> What about this: Assuming that the modes probed by EDID have the
> picture aspect ratio field set we can check for one of this
> probed modes when receiving a mode from userspace and then if the
> modes match (except from the picture aspect field, which is not
> set by userspace) then we can use the picture aspect value of the
> probed mode. I am using something like this in my modeset callback:
> 
>     /* 'mode' comes from userspace, 'pmode' comes from EDID */
>     if (mode->picture_aspect_ratio == HDMI_PICTURE_ASPECT_NONE) {
>         struct drm_display_mode *pmode;
> 
>         list_for_each_entry(pmode, &connector.modes, head) {
>             if (drm_mode_equal(pmode, mode))
>                 mode->picture_aspect_ratio =
> pmode->picture_aspect_ratio;
>         }
>     }
> 
> Of course if the EDID has for example two exactly equal modes
> that only differ in the picture aspect ratio then this will not
> work unless user passes the picture aspect when setting the mode.

Erhm, that seems to be an entirely different topic. And patches to fix
this are already on the mailing list, being reviewed. It's roughly your
idea, expect we fully expose the aspect ratio to userspace, and userspace
can select (if there multiple matching modes with different aspect
ratios).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch