On Fri, Nov 16, 2012 at 5:05 PM, Paulo Zanoni <przanoni at gmail.com> wrote: > Hi > > 2012/11/5 Daniel Vetter <daniel.vetter at 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 at 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
