drm: bridge/dw-hdmi: Fixes for dw-hdmi and DWC support

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

Quoted 2 lines · Jose Abreu

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.

drivers/gpu/drm/bridge/dw-hdmi.c static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts, · line 226

Quoted 8 lines

drivers/gpu/drm/bridge/dw-hdmi.c +7 -0

+	/* 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.

drivers/gpu/drm/bridge/dw-hdmi.c static int dw_hdmi_connector_get_modes(struct drm_connector *connector) · line 1494

Quoted 6 lines

drivers/gpu/drm/bridge/dw-hdmi.c +2 -0

@@ -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.

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.

Quoted 5 lines · 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.

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

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.

Quoted 10 lines · Jose Abreu

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).

Quoted 4 lines · Jose Abreu

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?

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

Quoted 11 lines · Jose Abreu

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;

Quoted 6 lines

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.

Quoted 4 lines

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.

Quoted 28 lines · Russell King - ARM Linux

Quoted 11 lines · Jose Abreu

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;

Quoted 6 lines

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.

Quoted 4 lines

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?

Quoted 5 lines

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.

Quoted 39 lines · Daniel Vetter

Quoted 26 lines · Russell King - ARM Linux

Quoted 11 lines · Jose Abreu

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;

Quoted 6 lines

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.

Quoted 4 lines

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?

Quoted 5 lines

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)

Quoted 2 lines

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,

Quoted 40 lines · Chris Wilson

Quoted 39 lines · Daniel Vetter

Quoted 26 lines · Russell King - ARM Linux

Quoted 11 lines · Jose Abreu

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;

Quoted 6 lines

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.

Quoted 4 lines

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?

Quoted 5 lines

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)

Quoted 6 lines

Quoted 2 lines

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.