[Intel-gfx] [PATCH] drm/i915: disable cloning on sdvo

[Daniel Vetter]

On Fri, Nov 16, 2012 at 12:04:35PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/11/13 Daniel Vetter <daniel.vetter at ffwll.ch>:
> > After the recent pile of disable-cloning patches, e.g.
> >
> > commit e3b86d6941c7e5f90be05d986fce1fcb40c68d6b
> > Author: Egbert Eich <eich at suse.de>
> > Date:   Sat Oct 13 14:30:15 2012 +0200
> >
> >     DRM/i915: Don't clone SDVO LVDS with analog
> >
> > and a bug report from Chris Wilson indicating that cloning doesn't
> > even work for DVI-SDVO and native VGA, let's just disable cloning on
> > sdvo encoders completely.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=29259
> > Tested-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_sdvo.c |   12 +++++-------
> >  1 file changed, 5 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index d6a5fb9..909d34f 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -2208,7 +2208,6 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device)
> >                 connector->connector_type = DRM_MODE_CONNECTOR_HDMIA;
> >                 intel_sdvo->is_hdmi = true;
> >         }
> > -       intel_sdvo->base.cloneable = true;
> >
> >         intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> >         if (intel_sdvo->is_hdmi)
> > @@ -2239,7 +2238,6 @@ intel_sdvo_tv_init(struct intel_sdvo *intel_sdvo, int type)
> >
> >         intel_sdvo->is_tv = true;
> >         intel_sdvo->base.needs_tv_clock = true;
> > -       intel_sdvo->base.cloneable = false;
> >
> >         intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> >
> > @@ -2282,8 +2280,6 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device)
> >                 intel_sdvo_connector->output_flag = SDVO_OUTPUT_RGB1;
> >         }
> >
> > -       intel_sdvo->base.cloneable = true;
> > -
> >         intel_sdvo_connector_init(intel_sdvo_connector,
> >                                   intel_sdvo);
> >         return true;
> > @@ -2314,9 +2310,6 @@ intel_sdvo_lvds_init(struct intel_sdvo *intel_sdvo, int device)
> >                 intel_sdvo_connector->output_flag = SDVO_OUTPUT_LVDS1;
> >         }
> >
> > -       /* SDVO LVDS is not cloneable because the input mode gets adjusted by the encoder */
> > -       intel_sdvo->base.cloneable = false;
> > -
> >         intel_sdvo_connector_init(intel_sdvo_connector, intel_sdvo);
> >         if (!intel_sdvo_create_enhance_property(intel_sdvo, intel_sdvo_connector))
> >                 goto err;
> > @@ -2726,6 +2719,11 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)
> >                 goto err_output;
> >         }
> >
> > +       /* SDVO because we need different clocks for SDVO encoders compared to
> > +        * VGA. And the sdvo encoder is also allowed to adjust the mode. So just
> > +        * give up and disable it. */
> 
> The code looks good, but the comment above is quite confusing. "SDVO because"?

Oops. What about:

"Cloning SDVO with anything is often impossible, since the SDVO encoder
can request a special input timing mode. And even if that's not the case
we have evidence that cloning a plain unscaled mode with VGA doesn't
really work. Furthermore the cloning flags are way to simplistic anyway to
express such constraints, so just give up on cloning for SDVO encoders."

Is that good enough for a full r-b?

Yours, Daniel

> 
> > +       intel_sdvo->base.cloneable = false;
> > +
> >         /* Only enable the hotplug irq if we need it, to work around noisy
> >          * hotplug lines.
> >          */
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch