Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1456325 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to .detect() callback Jose Abreu <Jose.Abreu@synopsys.com> - 2016-08-04 12:50 +0200
    Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-04 12:50 +0200
      Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Jose Abreu <Jose.Abreu@synopsys.com> - 2016-08-04 16:00 +0200
        Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-04 16:40 +0200
          Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Jose Abreu <Jose.Abreu@synopsys.com> - 2016-08-04 17:00 +0200
            Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-04 17:10 +0200
              Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Jose Abreu <Jose.Abreu@synopsys.com> - 2016-08-04 19:20 +0200
                Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Russell King - ARM Linux <linux@armlinux.org.uk> - 2016-08-05 01:20 +0200
                  Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Daniel Vetter <daniel@ffwll.ch> - 2016-08-05 10:10 +0200
                    Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Chris Wilson <chris@chris-wilson.co.uk> - 2016-08-05 10:20 +0200
                      Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Jose Abreu <Jose.Abreu@synopsys.com> - 2016-08-08 18:30 +0200
                        Re: [PATCH 3/3 v3] drm: bridge/dw-hdmi: Move edid reading to  .detect() callback Daniel Vetter <daniel@ffwll.ch> - 2016-08-09 08:10 +0200

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

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

[toc] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web