[Intel-gfx] [PATCH 7/8] drm/i915: try harder to find WR PLL clock settings

[Paulo Zanoni]

Hi

2012/8/9 Jani Nikula <jani.nikula at linux.intel.com>:
> On Wed, 08 Aug 2012, Paulo Zanoni <przanoni at gmail.com> wrote:
>> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>>
>> If we don't find the exact refresh rate, go with the next one. This
>> makes some modes work for me. They won't have the best settings, but
>> will at least have something. Just returning from this function when
>> we don't find the perfect settings does not help us at all.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++-------------------
>>  1 file changed, 14 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index ff03a3a..db242cf 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -267,7 +267,8 @@ struct wrpll_tmds_clock {
>>       u16 r2;         /* Reference divider */
>>  };
>>
>> -/* Table of matching values for WRPLL clocks programming for each frequency */
>> +/* Table of matching values for WRPLL clocks programming for each frequency.
>> + * The code assumes this table is sorted. */
>
> I spotted some duplicate lines in the table. Perhaps you could remove
> them while at it?

Good catch. I will write a V2 removing the 3 duplicated lines.

>
>>  static const struct wrpll_tmds_clock wrpll_tmds_clock_table[] = {
>>       {19750, 38,     25,     18},
>>       {20000, 48,     32,     18},
>> @@ -658,7 +659,7 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>>       struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>>       int port = intel_hdmi->ddi_port;
>>       int pipe = intel_crtc->pipe;
>> -     int p, n2, r2, valid=0;
>> +     int p, n2, r2;
>>       u32 temp, i;
>>
>>       /* On Haswell, we need to enable the clocks and prepare DDI function to
>> @@ -666,26 +667,20 @@ void intel_ddi_mode_set(struct drm_encoder *encoder,
>>        */
>>       DRM_DEBUG_KMS("Preparing HDMI DDI mode for Haswell on port %c, pipe %c\n", port_name(port), pipe_name(pipe));
>>
>> -     for (i=0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++) {
>> -             if (crtc->mode.clock == wrpll_tmds_clock_table[i].clock) {
>> -                     p = wrpll_tmds_clock_table[i].p;
>> -                     n2 = wrpll_tmds_clock_table[i].n2;
>> -                     r2 = wrpll_tmds_clock_table[i].r2;
>> +     for (i = 0; i < ARRAY_SIZE(wrpll_tmds_clock_table); i++)
>> +             if (crtc->mode.clock <= wrpll_tmds_clock_table[i].clock)
>> +                     break;
>>
>> -                     DRM_DEBUG_KMS("WR PLL clock: found settings for %dKHz refresh rate: p=%d, n2=%d, r2=%d\n",
>> -                                     crtc->mode.clock,
>> -                                     p, n2, r2);
>
> You drop this debug message. Is it intentional? The below DRM_INFO will
> only be printed if an exact match is not found.

Yes. It had way more than 80 columns so I started feeling extremely
uncomfortable while reading the code and I didn't know why, until I
realized it :) </joke>

The thing is that if we actually find the mode on the table it means
we're on the "happy case", so we don't really need to pollute dmesg
even more. The refresh rate is print by drm_mode_debug_printmodeline
(or by the DRM_INFO in the unhappy case) and in case you really need
to know the extremely meaningful p, n2 and r2 values you can always
check the code. If we really need this I can always add it back... But
leaving only the "bad case" for dmesg makes it easier to spot while
reading the tons of messages we print.

>
> BR,
> Jani.
>
>> +     if (i == ARRAY_SIZE(wrpll_tmds_clock_table))
>> +             i--;
>>
>> -                     valid = 1;
>> -                     break;
>> -             }
>> -     }
>> +     if (wrpll_tmds_clock_table[i].clock != crtc->mode.clock)
>> +             DRM_INFO("WR PLL: using settings for %dKHz on %dKHz mode\n",
>> +                      wrpll_tmds_clock_table[i].clock, crtc->mode.clock);
>>
>> -     if (!valid) {
>> -             DRM_ERROR("Unable to find WR PLL clock settings for %dKHz refresh rate\n",
>> -                             crtc->mode.clock);
>> -             return;
>> -     }
>> +     p = wrpll_tmds_clock_table[i].p;
>> +     n2 = wrpll_tmds_clock_table[i].n2;
>> +     r2 = wrpll_tmds_clock_table[i].r2;
>>
>>       /* Enable LCPLL if disabled */
>>       temp = I915_READ(LCPLL_CTL);
>> --
>> 1.7.11.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni