| Message ID | 1352118507-6933-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi 2012/11/5 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 > > Both need to happen before we enable the pll. Not true, at least for the docs I checked (gen6+), setting/computing the m/n registers can be done anytime before enabling the CPU pipe. Please change the commit message :) > 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). > > 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 callbakc. But for now this will allow us to clean > things up a bit. > > 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 2ecc7f8..1ad6d34 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4465,6 +4465,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc, Don't we also need to patch vlv_update_pll? > 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; We kinda have a "naming standard" where variables of type "struct intel_xxx" are called "intel_xxx" and variables of type "struct drm_xxx" are called "xxx". I I'd vote to call this intel_encoder. > int pipe = intel_crtc->pipe; > u32 dpll; > bool is_sdvo; > @@ -4533,6 +4534,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. > @@ -4577,6 +4582,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; Same here. > int pipe = intel_crtc->pipe; > u32 dpll; > > @@ -4610,6 +4616,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. > @@ -5537,6 +5547,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 Fri, Nov 16, 2012 at 5:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > Hi > > 2012/11/5 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 >> >> Both need to happen before we enable the pll. > > Not true, at least for the docs I checked (gen6+), setting/computing > the m/n registers can be done anytime before enabling the CPU pipe. > Please change the commit message :) Yeah, I've written this commit message before I've cleared up my confusion around this code. Now I think that even pre_pll_enable isn't strictly required, but we need it because we currently enable the pll in ->mode_set already. Which is bogus. I'll update the commit message. >> 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). >> >> 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 callbakc. But for now this will allow us to clean >> things up a bit. >> >> 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 2ecc7f8..1ad6d34 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -4465,6 +4465,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc, > > Don't we also need to patch vlv_update_pll? Luckily vlv doesn't support lvds. I can add that to the commit message, too. >> 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; > > We kinda have a "naming standard" where variables of type "struct > intel_xxx" are called "intel_xxx" and variables of type "struct > drm_xxx" are called "xxx". I I'd vote to call this intel_encoder. I kinda want to move to the intel_xxx variant being the main one, now that we rely much less on the common helper stuff. Safes tons of needless downcasting, but will result in a bit of intermediate inconsistency. Generally I think we should cut down on our usage of prefixes a bit, after reading too many patches from Ben I have to admit that it's easier on the eyes ;-) So I'd prefer if we leave things as-is. And in any case, if a function is too big or has too many local variables that you typecheck a local variable quickly, it's too big. I know, we suck on that metric ... -Daniel
Hi 2012/11/16 Daniel Vetter <daniel.vetter@ffwll.ch>: > On Fri, Nov 16, 2012 at 5:05 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >> Hi >> >> 2012/11/5 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 >>> >>> Both need to happen before we enable the pll. >> >> Not true, at least for the docs I checked (gen6+), setting/computing >> the m/n registers can be done anytime before enabling the CPU pipe. >> Please change the commit message :) > > Yeah, I've written this commit message before I've cleared up my > confusion around this code. Now I think that even pre_pll_enable isn't > strictly required, but we need it because we currently enable the pll > in ->mode_set already. Which is bogus. I'll update the commit message. > >>> 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). >>> >>> 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 callbakc. But for now this will allow us to clean >>> things up a bit. >>> >>> 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 2ecc7f8..1ad6d34 100644 >>> --- a/drivers/gpu/drm/i915/intel_display.c >>> +++ b/drivers/gpu/drm/i915/intel_display.c >>> @@ -4465,6 +4465,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc, >> >> Don't we also need to patch vlv_update_pll? > > Luckily vlv doesn't support lvds. I can add that to the commit message, too. > >>> 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; >> >> We kinda have a "naming standard" where variables of type "struct >> intel_xxx" are called "intel_xxx" and variables of type "struct >> drm_xxx" are called "xxx". I I'd vote to call this intel_encoder. > > I kinda want to move to the intel_xxx variant being the main one, now > that we rely much less on the common helper stuff. Safes tons of > needless downcasting, but will result in a bit of intermediate > inconsistency. > > Generally I think we should cut down on our usage of prefixes a bit, > after reading too many patches from Ben I have to admit that it's > easier on the eyes ;-) So I'd prefer if we leave things as-is. And in > any case, if a function is too big or has too many local variables > that you typecheck a local variable quickly, it's too big. I know, we > suck on that metric ... I noticed you don't always use intel_xxx, and I actually thought it was mostly due to distraction/not-caring instead of actually trying to change the style. In some of my patches I even added intel_ prefixes to a lot of variables that were missing. If we're going to change the standard, it's ok, as long as we know we're changing the standard :) > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2ecc7f8..1ad6d34 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4465,6 +4465,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; @@ -4533,6 +4534,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. @@ -4577,6 +4582,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; @@ -4610,6 +4616,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. @@ -5537,6 +5547,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 Both need to happen before we enable the pll. 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). 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 callbakc. But for now this will allow us to clean things up a bit. 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(+)