Re: [PATCH] drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

On Thu, Jun 18, 2015 at 04:55:45PM +0100, Russell King - ARM Linux wrote:
> On Thu, Jun 18, 2015 at 08:26:39AM -0700, Doug Anderson wrote:
> > Perhaps you can try <https://patchwork.kernel.org/patch/5906771/>
> 
> Something like that needs to be done, but let's get rid of the mdvi
> thing in struct hdmi_vmode - it doesn't belong there, it isn't part
> of the currently set video mode, but becomes a property of the
> connected sink.
> 
> I'd also prefer it to be called "is_dvi_sink", especially as its
> function is changing from "is it a CEA mode" to "is the attached
> device a DVI sink".
> 
> Even better would be to call it "is_hdmi_sink" to maintain positive
> logic with single-negation where required, rather than double-
> negation in places.

This is actually a _very_ important point.  Changing the function of
mdvi when it's used in multiple places throughout the driver is not on -
it's too big a change:

        /*check csc whether needed activated in HDMI mode */
        cscon = (is_color_space_conversion(hdmi) &&
                        !hdmi->hdmi_data.video_mode.mdvi);

        inv_val |= (vmode->mdvi ?
                HDMI_FC_INVIDCONF_DVI_MODEZ_DVI_MODE :
                HDMI_FC_INVIDCONF_DVI_MODEZ_HDMI_MODE);

        if (hdmi->cable_plugin && !hdmi->hdmi_data.video_mode.mdvi)
                hdmi_enable_overflow_interrupts(hdmi);

It's unclear what the effect would be to change the meaning of mdvi
from "this is a CEA mode" to "the attached device is DVI" in all these
locations, and it's just not on to do this in a patch which merely
says:

   If the monitor support audio, so we should support audio for it, even if
   the display resolution is No-CEA mode.

In other words, doesn't even describe this change.

In any case, this patch has been dropped from more recent audio driver
series.

So, what I'd like to see is a patch series which starts with the change
below, and builds on that, with explainations why each change is needed.
This is important, as this is shared IP, and we need to make sure that
we don't regress non-Rockchip users of this IP.  I'll try and do some
work in this area if nothing crops up in the next month.

 drivers/gpu/drm/bridge/dw_hdmi.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 49cafb61d290..8834e8142ea6 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -119,6 +119,8 @@ struct dw_hdmi {
 
 	u8 edid[HDMI_EDID_LEN];
 	bool cable_plugin;
+	bool sink_is_hdmi;
+	bool sink_has_audio;
 
 	bool phy_enabled;
 	struct drm_display_mode previous_mode;
@@ -1402,6 +1404,9 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 
 	edid = drm_get_edid(connector, hdmi->ddc);
 	if (edid) {
+		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
+		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
+
 		dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
 			edid->width_cm, edid->height_cm);
 


-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.