[Intel-gfx] [PATCH 03/18] drm/i915: simplify intel_crtc_driving_pch

[Paulo Zanoni]

Hi

2012/10/25 Jani Nikula <jani.nikula at linux.intel.com>:
> On Tue, 23 Oct 2012, Paulo Zanoni <przanoni at gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> By forking Ironlake and Haswell functions. The only callers are
>> {ironlake,haswell}_crtc_enable anyway, and this way we won't need to
>> add other checks on the Haswell version for the next gens.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++--------------------
>>  1 file changed, 11 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index a90da35..0c4e9c5 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -2849,7 +2849,7 @@ static void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>>       mutex_unlock(&dev->struct_mutex);
>>  }
>>
>> -static bool intel_crtc_driving_pch(struct drm_crtc *crtc)
>> +static bool ironlake_crtc_driving_pch(struct drm_crtc *crtc)
>>  {
>>       struct drm_device *dev = crtc->dev;
>>       struct intel_encoder *intel_encoder;
>> @@ -2859,23 +2859,6 @@ static bool intel_crtc_driving_pch(struct drm_crtc *crtc)
>>        * must be driven by its own crtc; no sharing is possible.
>>        */
>>       for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
>> -
>> -             /* On Haswell, LPT PCH handles the VGA connection via FDI, and Haswell
>> -              * CPU handles all others */
>> -             if (IS_HASWELL(dev)) {
>> -                     /* It is still unclear how this will work on PPT, so throw up a warning */
>> -                     WARN_ON(!HAS_PCH_LPT(dev));
>> -
>> -                     if (intel_encoder->type == INTEL_OUTPUT_ANALOG) {
>> -                             DRM_DEBUG_KMS("Haswell detected DAC encoder, assuming is PCH\n");
>> -                             return true;
>> -                     } else {
>> -                             DRM_DEBUG_KMS("Haswell detected encoder %d, assuming is CPU\n",
>> -                                           intel_encoder->type);
>> -                             return false;
>> -                     }
>> -             }
>> -
>>               switch (intel_encoder->type) {
>>               case INTEL_OUTPUT_EDP:
>>                       if (!intel_encoder_is_pch_edp(&intel_encoder->base))
>> @@ -2887,6 +2870,14 @@ static bool intel_crtc_driving_pch(struct drm_crtc *crtc)
>>       return true;
>>  }
>>
>> +static bool haswell_crtc_driving_pch(struct drm_crtc *crtc)
>> +{
>> +     if (intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG))
>> +             return true;
>> +     else
>> +             return false;
>> +}
>
> Nitpick, this could be written just:
>
>         return intel_pipe_has_type(crtc, INTEL_OUTPUT_ANALOG);

Now my code looks so stupid! =)

>
> A pedantic observation: the previous version in intel_crtc_driving_pch
> unconditionally stopped at the first encoder on the crtc on HSW, this
> one checks that *none* of the encoders has type INTEL_OUTPUT_ANALOG
> before it returns false. It doesn't matter either way, does it?

I think the main reason for this patch is that I don't like the fact
that the old code unconditionally returned on the first iteration, but
that's not real a problem, it's just my opinion about coding style. On
Haswell there's just 1 encoder for each crtc and we check for this
condition on haswell_crtc_mode_set. On previous gens we can have more
than 1 encoder, so we have to loop over all the encoders.

>
> Reviewed-by: Jani Nikula <jani.nikula at intel.com>
>
>
>> +
>>  /* Program iCLKIP clock to the desired frequency */
>>  static void lpt_program_iclkip(struct drm_crtc *crtc)
>>  {
>> @@ -3215,7 +3206,7 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc)
>>                       I915_WRITE(PCH_LVDS, temp | LVDS_PORT_EN);
>>       }
>>
>> -     is_pch_port = intel_crtc_driving_pch(crtc);
>> +     is_pch_port = ironlake_crtc_driving_pch(crtc);
>>
>>       if (is_pch_port) {
>>               ironlake_fdi_pll_enable(intel_crtc);
>> @@ -3293,7 +3284,7 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>       intel_crtc->active = true;
>>       intel_update_watermarks(dev);
>>
>> -     is_pch_port = intel_crtc_driving_pch(crtc);
>> +     is_pch_port = haswell_crtc_driving_pch(crtc);
>>
>>       if (is_pch_port) {
>>               ironlake_fdi_pll_enable(intel_crtc);
>> --
>> 1.7.11.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni