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

[Paulo Zanoni]

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