| Message ID | 1bbffb944fb9ce0363f5544db31baf2a911c1c55.1470306826.git.joabreu@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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; }
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(-)