Hi 2012/11/16 Daniel Vetter <daniel.vetter at ffwll.ch>: > 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 ... 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 -- Paulo Zanoni
