| Message ID | 1362670228-19494-4-git-send-email-damien.lespiau@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2013/3/7 Damien Lespiau <damien.lespiau@intel.com>: > In the case where the hardware has been wrongly programmed and the EDP > TRANS_DDI_FUNC_CTL register has a bogus value in its EDP Input field, we > were using the pipe variable uninitialized. > > In this case, shutdown the transcoder. It will be programmed correctly > the next time we try to enabled eDP. > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > 1 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 502cb28..875baf1 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9107,6 +9107,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > case TRANS_DDI_EDP_INPUT_C_ONOFF: > pipe = PIPE_C; > break; > + default: > + /* A bogus value has been programmed, disable > + * the transcoder */ > + WARN(1, "Bogus eDP source %08x\n", tmp); > + intel_ddi_disable_transcoder_func(dev_priv, > + TRANSCODER_EDP); I imagine this patch is a result of checking some code analyzer and not real hardware, so we didn't really test this code on real hardware, right? The WARN is definitely a must-have, but I kinda fear this "disable" code since we can easily freeze the machine if we get the mode set sequence wrong. Anyway, if the hardware is programmed in an inconsistent way I don't really think there's a "right way" to disable it, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> But we need to watch out in case users start reporting about machines that freeze during boot. > + goto setup_pipes; > } > > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > @@ -9117,6 +9124,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > } > } > > +setup_pipes: > for_each_pipe(pipe) { > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > -- > 1.7.7.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Mar 22, 2013 at 06:34:43PM -0300, Paulo Zanoni wrote: > Hi > > 2013/3/7 Damien Lespiau <damien.lespiau@intel.com>: > > In the case where the hardware has been wrongly programmed and the EDP > > TRANS_DDI_FUNC_CTL register has a bogus value in its EDP Input field, we > > were using the pipe variable uninitialized. > > > > In this case, shutdown the transcoder. It will be programmed correctly > > the next time we try to enabled eDP. > > > > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ > > 1 files changed, 8 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 502cb28..875baf1 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -9107,6 +9107,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > > case TRANS_DDI_EDP_INPUT_C_ONOFF: > > pipe = PIPE_C; > > break; > > + default: > > + /* A bogus value has been programmed, disable > > + * the transcoder */ > > + WARN(1, "Bogus eDP source %08x\n", tmp); > > + intel_ddi_disable_transcoder_func(dev_priv, > > + TRANSCODER_EDP); > > I imagine this patch is a result of checking some code analyzer and > not real hardware, so we didn't really test this code on real > hardware, right? The WARN is definitely a must-have, but I kinda fear > this "disable" code since we can easily freeze the machine if we get > the mode set sequence wrong. Anyway, if the hardware is programmed in > an inconsistent way I don't really think there's a "right way" to > disable it, so: > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Queued for -next, thanks for the patch. > But we need to watch out in case users start reporting about machines > that freeze during boot. And added a little note to the patch to this effect. -Daniel > > > + goto setup_pipes; > > } > > > > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > @@ -9117,6 +9124,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, > > } > > } > > > > +setup_pipes: > > for_each_pipe(pipe) { > > crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > > > > -- > > 1.7.7.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 502cb28..875baf1 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9107,6 +9107,13 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, case TRANS_DDI_EDP_INPUT_C_ONOFF: pipe = PIPE_C; break; + default: + /* A bogus value has been programmed, disable + * the transcoder */ + WARN(1, "Bogus eDP source %08x\n", tmp); + intel_ddi_disable_transcoder_func(dev_priv, + TRANSCODER_EDP); + goto setup_pipes; } crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); @@ -9117,6 +9124,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, } } +setup_pipes: for_each_pipe(pipe) { crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
In the case where the hardware has been wrongly programmed and the EDP TRANS_DDI_FUNC_CTL register has a bogus value in its EDP Input field, we were using the pipe variable uninitialized. In this case, shutdown the transcoder. It will be programmed correctly the next time we try to enabled eDP. Signed-off-by: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 8 ++++++++ 1 files changed, 8 insertions(+), 0 deletions(-)