Re: [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch

From: Paulo Zanoni <przanoni@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/3] drm/i915: add PCH_NONE to enum intel_pch
Date: Tue, 3 Jul 2012 17:29:54 -0300	[thread overview]
Message-ID: <CA+gsUGSjuJv-3+-+AXDYck8gx6SYYzG2WzmgZh6bb0H-MDyFHQ@mail.gmail.com> (raw)
In-Reply-To: <20120703191002.GC5103@phenom.ffwll.local>

2012/7/3 Daniel Vetter <daniel@ffwll.ch>:
> I think most of the HAS_PCH_xxx are implicitly guarded because we've split
> up the pch modeset into it's own functions. I think there might only be a
> few issues in the encoder functions maybe. Have your checked all the
> HAS_PCH_IBX checks there? If you want, I can go through the code, too.
>

I did check. At the moment we have just a few HAS_PCH_IBX calls in our
driver. The only possible issues may be inside intel_hdmi.c and
intel_dp.c (and they need more investigation).

My biggest worry here is being "future-proof": are we sure whenever
someone suggests adding HAS_PCH_IBX we'll remember that machines
without a PCH return true for HAS_PCH_IBX? This is highly
counter-intuitive. I really think that in future hardware enablement
code we'll replace a lot of the "if (HAS_PCH_SPLIT) { foo(); } else {
bar(); }" code for "if (HAS_PCH_NEW) { baz(); } else if (HAS_PCH_OLD)
{ foo(); } else { bar(); }".

Thanks,
Paulo

> Otherwise I really like this.
> -Daniel
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> Another alternative would have been to change HAS_PCH_IBX to also
>> check for HAS_PCH_SPLIT, but I'm not exactly in favor of adding more
>> conditionals...
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index b7a1eaa..b12e79a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -333,6 +333,7 @@ enum no_fbc_reason {
>>  };
>>
>>  enum intel_pch {
>> +     PCH_NONE = 0,   /* No PCH present */
>>       PCH_IBX,        /* Ibexpeak PCH */
>>       PCH_CPT,        /* Cougarpoint PCH */
>>       PCH_LPT,        /* Lynxpoint PCH */
>> --
>> 1.7.10.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

-- 
Paulo Zanoni