[Intel-gfx] [PATCH 1/8] drm/i915: add encoder->pre_pll_enable callback

[Daniel Vetter]

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