| Message ID | 1418301491-23020-5-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Dec 11, 2014 at 02:38:07PM +0200, Ander Conselvan de Oliveira wrote: > In function that define a local pipe_config variable to point to > crtc->config, replace remaining references to crtc->config with > the local variable. This makes the code more consistent and easier > to change in an automated manner. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3bceacb..da5af23 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4602,7 +4602,7 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc_state *pipe_config = &crtc->config; > > - if (!crtc->config.gmch_pfit.control) > + if (!pipe_config->gmch_pfit.control) > return; > > /* > @@ -5925,7 +5925,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, > vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW10(pipe), > 0x00d0000f); > > - if (crtc->config.has_dp_encoder) { > + if (pipe_config->has_dp_encoder) { Does this one change the behavior in some cases? The pipe_config parameter passed to this function from vlv_force_pll_on() is a hardcoded pipe_config that is never going to have has_dp_encoder set, regardless of what the current config is. > /* Use SSC source */ > if (pipe == PIPE_A) > vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW5(pipe), > @@ -7541,7 +7541,7 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > void intel_dp_get_m_n(struct intel_crtc *crtc, > struct intel_crtc_state *pipe_config) > { > - if (crtc->config.has_pch_encoder) > + if (pipe_config->has_pch_encoder) I think this one might also change the semantics? The callchain check_crtc_state() -> intel_{dp,ddi}_get_config() -> intel_dp_get_m_n() passes down a fresh pipe_config() that isn't the same structure stored in crtc->config (although it hopefully has the same values in the end). Matt > intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); > else > intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Dec 15, 2014 at 10:37:57AM -0800, Matt Roper wrote: > On Thu, Dec 11, 2014 at 02:38:07PM +0200, Ander Conselvan de Oliveira wrote: > > In function that define a local pipe_config variable to point to > > crtc->config, replace remaining references to crtc->config with > > the local variable. This makes the code more consistent and easier > > to change in an automated manner. > > > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3bceacb..da5af23 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4602,7 +4602,7 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_crtc_state *pipe_config = &crtc->config; > > > > - if (!crtc->config.gmch_pfit.control) > > + if (!pipe_config->gmch_pfit.control) > > return; > > > > /* > > @@ -5925,7 +5925,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, > > vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW10(pipe), > > 0x00d0000f); > > > > - if (crtc->config.has_dp_encoder) { > > + if (pipe_config->has_dp_encoder) { > > Does this one change the behavior in some cases? The pipe_config > parameter passed to this function from vlv_force_pll_on() is a hardcoded > pipe_config that is never going to have has_dp_encoder set, regardless > of what the current config is. The force stuff is only done when the pipe isn't active, so crtc->config will be stale anyway (well, actually the pipe could be driving DSI but that shouldn't really matter either). So using the passed pipe_config leads to a more consistent result. The value of has_dp_encoder shouldn't actually matter as long as we get some kind of clock from the DPLL. > > > > /* Use SSC source */ > > if (pipe == PIPE_A) > > vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW5(pipe), > > @@ -7541,7 +7541,7 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > > void intel_dp_get_m_n(struct intel_crtc *crtc, > > struct intel_crtc_state *pipe_config) > > { > > - if (crtc->config.has_pch_encoder) > > + if (pipe_config->has_pch_encoder) > > I think this one might also change the semantics? The callchain > > check_crtc_state() -> intel_{dp,ddi}_get_config() -> intel_dp_get_m_n() > > passes down a fresh pipe_config() that isn't the same structure stored > in crtc->config (although it hopefully has the same values in the end). This looks like a straight up bug fix to me. Apparently I already fumbled it when I introduced the DP M/N readout. The state checker hasn't tripped on this since we've already written the new pipe config to crtc->config by the time we do the readout during modeset. And intel_modeset_readout_hw_state() reads the state directly into crtc->config, so even the initial state would have come out correct. So, while the code is wrong, doesn't look like it could have caused any real issues. > > > Matt > > > intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); > > else > > intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder, > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Matt Roper > Graphics Software Engineer > IoTG Platform Enabling & Development > Intel Corporation > (916) 356-2795 > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Dec 15, 2014 at 09:21:41PM +0200, Ville Syrjälä wrote: > On Mon, Dec 15, 2014 at 10:37:57AM -0800, Matt Roper wrote: > > On Thu, Dec 11, 2014 at 02:38:07PM +0200, Ander Conselvan de Oliveira wrote: > > > @@ -7541,7 +7541,7 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, > > > void intel_dp_get_m_n(struct intel_crtc *crtc, > > > struct intel_crtc_state *pipe_config) > > > { > > > - if (crtc->config.has_pch_encoder) > > > + if (pipe_config->has_pch_encoder) > > > > I think this one might also change the semantics? The callchain > > > > check_crtc_state() -> intel_{dp,ddi}_get_config() -> intel_dp_get_m_n() > > > > passes down a fresh pipe_config() that isn't the same structure stored > > in crtc->config (although it hopefully has the same values in the end). > > This looks like a straight up bug fix to me. Apparently I already > fumbled it when I introduced the DP M/N readout. > > The state checker hasn't tripped on this since we've already > written the new pipe config to crtc->config by the time we do the > readout during modeset. > > And intel_modeset_readout_hw_state() reads the state directly into > crtc->config, so even the initial state would have come out correct. > > So, while the code is wrong, doesn't look like it could have caused > any real issues. We also have very light coverage of direct modeset changes (i.e. without going through off in-between). Iirc most of the igts shut down stuff in between before setting the new modes (because that's the only way to get somewhat reliably modesets without atomic updates). I think we should expect more fun in this area once we have real atomic modesets. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3bceacb..da5af23 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4602,7 +4602,7 @@ static void i9xx_pfit_enable(struct intel_crtc *crtc) struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc_state *pipe_config = &crtc->config; - if (!crtc->config.gmch_pfit.control) + if (!pipe_config->gmch_pfit.control) return; /* @@ -5925,7 +5925,7 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW10(pipe), 0x00d0000f); - if (crtc->config.has_dp_encoder) { + if (pipe_config->has_dp_encoder) { /* Use SSC source */ if (pipe == PIPE_A) vlv_dpio_write(dev_priv, pipe, VLV_PLL_DW5(pipe), @@ -7541,7 +7541,7 @@ static void intel_cpu_transcoder_get_m_n(struct intel_crtc *crtc, void intel_dp_get_m_n(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { - if (crtc->config.has_pch_encoder) + if (pipe_config->has_pch_encoder) intel_pch_transcoder_get_m_n(crtc, &pipe_config->dp_m_n); else intel_cpu_transcoder_get_m_n(crtc, pipe_config->cpu_transcoder,
In function that define a local pipe_config variable to point to crtc->config, replace remaining references to crtc->config with the local variable. This makes the code more consistent and easier to change in an automated manner. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)