| Message ID | 1352823883-5946-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/11/13 Daniel Vetter <daniel.vetter@ffwll.ch>: > After the recent pile of disable-cloning patches, e.g. > > commit e3b86d6941c7e5f90be05d986fce1fcb40c68d6b > Author: Egbert Eich <eich@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@chris-wilson.co.uk> > Signed-off-by: Daniel Vetter <daniel.vetter@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"? > + 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 16, 2012 at 12:04:35PM -0200, Paulo Zanoni wrote: > Hi > > 2012/11/13 Daniel Vetter <daniel.vetter@ffwll.ch>: > > After the recent pile of disable-cloning patches, e.g. > > > > commit e3b86d6941c7e5f90be05d986fce1fcb40c68d6b > > Author: Egbert Eich <eich@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@chris-wilson.co.uk> > > Signed-off-by: Daniel Vetter <daniel.vetter@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@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
Hi 2012/11/16 Daniel Vetter <daniel@ffwll.ch>: > On Fri, Nov 16, 2012 at 12:04:35PM -0200, Paulo Zanoni wrote: >> Hi >> >> 2012/11/13 Daniel Vetter <daniel.vetter@ffwll.ch>: >> > After the recent pile of disable-cloning patches, e.g. >> > >> > commit e3b86d6941c7e5f90be05d986fce1fcb40c68d6b >> > Author: Egbert Eich <eich@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@chris-wilson.co.uk> >> > Signed-off-by: Daniel Vetter <daniel.vetter@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 s/way to simplistic/way too simplistic/ ? > express such constraints, so just give up on cloning for SDVO encoders." > > Is that good enough for a full r-b? Yes. A simple "this doesn't work, maybe our code is bugged, maybe not, so disable it anyway" would also have done the job :) Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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@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
On Fri, Nov 16, 2012 at 02:48:36PM -0200, Paulo Zanoni wrote: > Hi > > 2012/11/16 Daniel Vetter <daniel@ffwll.ch>: > > On Fri, Nov 16, 2012 at 12:04:35PM -0200, Paulo Zanoni wrote: > >> Hi > >> > >> 2012/11/13 Daniel Vetter <daniel.vetter@ffwll.ch>: > >> > After the recent pile of disable-cloning patches, e.g. > >> > > >> > commit e3b86d6941c7e5f90be05d986fce1fcb40c68d6b > >> > Author: Egbert Eich <eich@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@chris-wilson.co.uk> > >> > Signed-off-by: Daniel Vetter <daniel.vetter@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 > > s/way to simplistic/way too simplistic/ ? > > > express such constraints, so just give up on cloning for SDVO encoders." > > > > Is that good enough for a full r-b? > > Yes. A simple "this doesn't work, maybe our code is bugged, maybe not, > so disable it anyway" would also have done the job :) > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Applied to -fixes, with the new comment (and the spelling fixed in the comment). Thanks for the review. -Daniel
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. */ + intel_sdvo->base.cloneable = false; + /* Only enable the hotplug irq if we need it, to work around noisy * hotplug lines. */
After the recent pile of disable-cloning patches, e.g. commit e3b86d6941c7e5f90be05d986fce1fcb40c68d6b Author: Egbert Eich <eich@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@chris-wilson.co.uk> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_sdvo.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)