| Message ID | 1353946943-18745-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>: > Currently we have two encoder specific bits in the common mode_set > functions: > - lvds pin pair enabling > - dp m/n setting and computation > > Now the lvds stuff needs to happen before the pll is enabled. Since > that is done in the crtc_mode_set functions, we need to add a new > callback to be able to move them to the encoder code (where they > belong). The dp m/n stuff is a giant mess anyway (since it also > confuses itself with the fdi link m/n handling), so that needs to be > handled separately. > > I think that we can move the pll enabling down quite a bit, which > might allow us to eventually merge encoder->pre_enable with this new > pre_pll_enable callback. But for now this will allow us to clean > things up a bit. > > Note that vlv doesn't support lvds, hence we don't need to change > anything in there. My only worry is that in the future we might use the pre_pll_enable callback for some non-LVDS encoder, and then we'll forget about calling this on VLV. Even if you don't think this is necessary, the patch looks correct, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Although I'd prefer to see "v3". > > v2: Fixup commit message, both suggested from Paulo Zanoni. > - dp m/n doesn't need to happen before pll enabling > - lvds doesn't exist on vlv, hence no changes required in the vlv pll > function. > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 769fc8f..647f16e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4462,6 +4462,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc, > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > u32 dpll; > bool is_sdvo; > @@ -4530,6 +4531,10 @@ static void i9xx_update_pll(struct drm_crtc *crtc, > POSTING_READ(DPLL(pipe)); > udelay(150); > > + for_each_encoder_on_crtc(dev, crtc, encoder) > + if (encoder->pre_pll_enable) > + encoder->pre_pll_enable(encoder); > + > /* The LVDS pin pair needs to be on before the DPLLs are enabled. > * This is an exception to the general rule that mode_set doesn't turn > * things on. > @@ -4574,6 +4579,7 @@ static void i8xx_update_pll(struct drm_crtc *crtc, > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_encoder *encoder; > int pipe = intel_crtc->pipe; > u32 dpll; > > @@ -4607,6 +4613,10 @@ static void i8xx_update_pll(struct drm_crtc *crtc, > POSTING_READ(DPLL(pipe)); > udelay(150); > > + for_each_encoder_on_crtc(dev, crtc, encoder) > + if (encoder->pre_pll_enable) > + encoder->pre_pll_enable(encoder); > + > /* The LVDS pin pair needs to be on before the DPLLs are enabled. > * This is an exception to the general rule that mode_set doesn't turn > * things on. > @@ -5535,6 +5545,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, > I915_WRITE(TRANSDPLINK_N1(pipe), 0); > } > > + for_each_encoder_on_crtc(dev, crtc, encoder) > + if (encoder->pre_pll_enable) > + encoder->pre_pll_enable(encoder); > + > if (intel_crtc->pch_pll) { > I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index bcc5241..42a40a1 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -153,6 +153,7 @@ struct intel_encoder { > bool cloneable; > bool connectors_active; > void (*hot_plug)(struct intel_encoder *); > + void (*pre_pll_enable)(struct intel_encoder *); > void (*pre_enable)(struct intel_encoder *); > void (*enable)(struct intel_encoder *); > void (*disable)(struct intel_encoder *); > -- > 1.7.11.7 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 27, 2012 at 06:38:18PM -0200, Paulo Zanoni wrote: > Hi > > 2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>: > > Currently we have two encoder specific bits in the common mode_set > > functions: > > - lvds pin pair enabling > > - dp m/n setting and computation > > > > Now the lvds stuff needs to happen before the pll is enabled. Since > > that is done in the crtc_mode_set functions, we need to add a new > > callback to be able to move them to the encoder code (where they > > belong). The dp m/n stuff is a giant mess anyway (since it also > > confuses itself with the fdi link m/n handling), so that needs to be > > handled separately. > > > > I think that we can move the pll enabling down quite a bit, which > > might allow us to eventually merge encoder->pre_enable with this new > > pre_pll_enable callback. But for now this will allow us to clean > > things up a bit. > > > > Note that vlv doesn't support lvds, hence we don't need to change > > anything in there. > > My only worry is that in the future we might use the pre_pll_enable > callback for some non-LVDS encoder, and then we'll forget about > calling this on VLV. I really hope that we've cleaned things up by then and this pre_pll_enable hook is gone again. So merged as-is. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 769fc8f..647f16e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4462,6 +4462,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_encoder *encoder; int pipe = intel_crtc->pipe; u32 dpll; bool is_sdvo; @@ -4530,6 +4531,10 @@ static void i9xx_update_pll(struct drm_crtc *crtc, POSTING_READ(DPLL(pipe)); udelay(150); + for_each_encoder_on_crtc(dev, crtc, encoder) + if (encoder->pre_pll_enable) + encoder->pre_pll_enable(encoder); + /* The LVDS pin pair needs to be on before the DPLLs are enabled. * This is an exception to the general rule that mode_set doesn't turn * things on. @@ -4574,6 +4579,7 @@ static void i8xx_update_pll(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct intel_encoder *encoder; int pipe = intel_crtc->pipe; u32 dpll; @@ -4607,6 +4613,10 @@ static void i8xx_update_pll(struct drm_crtc *crtc, POSTING_READ(DPLL(pipe)); udelay(150); + for_each_encoder_on_crtc(dev, crtc, encoder) + if (encoder->pre_pll_enable) + encoder->pre_pll_enable(encoder); + /* The LVDS pin pair needs to be on before the DPLLs are enabled. * This is an exception to the general rule that mode_set doesn't turn * things on. @@ -5535,6 +5545,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc, I915_WRITE(TRANSDPLINK_N1(pipe), 0); } + for_each_encoder_on_crtc(dev, crtc, encoder) + if (encoder->pre_pll_enable) + encoder->pre_pll_enable(encoder); + if (intel_crtc->pch_pll) { I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index bcc5241..42a40a1 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -153,6 +153,7 @@ struct intel_encoder { bool cloneable; bool connectors_active; void (*hot_plug)(struct intel_encoder *); + void (*pre_pll_enable)(struct intel_encoder *); void (*pre_enable)(struct intel_encoder *); void (*enable)(struct intel_encoder *); void (*disable)(struct intel_encoder *);
Currently we have two encoder specific bits in the common mode_set functions: - lvds pin pair enabling - dp m/n setting and computation Now the lvds stuff needs to happen before the pll is enabled. Since that is done in the crtc_mode_set functions, we need to add a new callback to be able to move them to the encoder code (where they belong). The dp m/n stuff is a giant mess anyway (since it also confuses itself with the fdi link m/n handling), so that needs to be handled separately. I think that we can move the pll enabling down quite a bit, which might allow us to eventually merge encoder->pre_enable with this new pre_pll_enable callback. But for now this will allow us to clean things up a bit. Note that vlv doesn't support lvds, hence we don't need to change anything in there. v2: Fixup commit message, both suggested from Paulo Zanoni. - dp m/n doesn't need to happen before pll enabling - lvds doesn't exist on vlv, hence no changes required in the vlv pll function. Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 15 insertions(+)