drm: bridge/dw_hdmi: Filter modes > 165MHz for DVI

Commit Message

Russell King - ARM Linux June 18, 2015, 4:09 p.m. UTC

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(+)

Comments

Doug Anderson June 18, 2015, 4:22 p.m. UTC | #1

Russell,

On Thu, Jun 18, 2015 at 9:09 AM, Russell King - ARM Linux
<[e-mail]> wrote:
> 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.

OK.  I've mostly been jumping in here to do a bugfix or two, not to
take over maintenance of the driver.  My general policy is to submit
things upstream if at all possible, but I think in the case of HDMI we
are just too different from upstream for this to be easy.

I'll let you and Yakir figure out the way forward to keep everyone
happy.  If that means you submitting some patches then great.  If that
means Yakir submitting some patches then that's great too.  In such a
case you can consider my patch to be a bug report and I'll be happy
with folks figure out the proper way to do this in the upstream
driver.  :)

-Doug

Patch

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