Re: [PATCH 1/7] drm/i915: Allow VGA on CRTC 2

Re: [PATCH 1/7] drm/i915: Allow VGA on CRTC 2

[Daniel Vetter]

On Mon, Aug 13, 2012 at 09:34:45PM -0700, Keith Packard wrote:
> This is left over from the old PLL sharing code and isn't useful now
> that PLLs are shared when possible.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Queued for -next, thanks for the patch. I'll hold off a bit on the others
until it's a bit clearer what's going on/wrong.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:

@@ -3728,7 +3728,8 @@ static inline bool intel_panel_use_ssc(struct
drm_i915_private *dev_priv)
  */
 static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
                                         unsigned int *pipe_bpp,
-                                        struct drm_display_mode *mode)
+                                        struct drm_display_mode *mode,
+                                        int max_fdi_bpp)

There's some kernel-doc for this function, maybe add a @max_fdi_bpp there?


> @@ -3800,6 +3801,15 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>                 display_bpc = 6;
>         }
>
> +       if (display_bpc * 3 > max_fdi_bpp) {
> +               if (max_fdi_bpp < 24)
> +                       display_bpc = 6;
> +               else if (max_fdi_bpp < 30)
> +                       display_bpc = 8;
> +               else if (max_fdi_bpp < 36)
> +                       display_bpc = 10;
> +               DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
> +       }

This chunk is being moved around in a later patch in the series,
merging the two patches in one looks like a good idea?


> +       /* If the other FDI link is using too many lanes, we can't have
> +        * any
> +        */
> +       if (other_intel_crtc->fdi_lanes > 2)
> +               return 0;
> +
> +       /* When both are running, we only get 2 lanes at most
> +        */
> +       return 2;
> +}

I guess this does not cover the case of pipe B using 3 lanes meaning
pipe C can use 1?

> @@ -4672,7 +4731,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>            according to current link config */
>         if (is_cpu_edp) {
>                 intel_edp_link_config(edp_encoder, &lane, &link_bw);
> +               max_fdi_bpp = 0;
> +               max_lane = lane;
>         } else {
> +               u32     fdi_bw;
> +
> +               /* [e]DP over FDI requires target mode clock
> +                  instead of link clock */
> +               if (is_dp)
> +                       target_clock = mode->clock;
> +               else
> +                       target_clock = adjusted_mode->clock;
> +

This duplicates the code just that is just a few lines away, instead
how about moving the logic to set target_clock up in front of this
whole if()?

>                 /* FDI is a binary signal running at ~2.7GHz, encoding
>                  * each output octet as 10 bits. The actual frequency
>                  * is stored as a divider into a 100MHz clock, and the
> @@ -4681,6 +4751,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                  * is:
>                  */
>                 link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> +
> +               max_lane = 4;
> +               if (IS_IVYBRIDGE(dev))
> +                       max_lane = ivb_fdi_max_lanes(crtc);
> +
> +               /*
> +                * Compute the available FDI bandwidth, use that
> +                * to compute the maximum supported BPP
> +                */
> +               fdi_bw = link_bw * max_lane * 19 / 20;
> +               max_fdi_bpp = fdi_bw / target_clock;
> +               DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
>         }

This chunk is also reworked in a later commit in this series.

-- 
Damien

Re: [Intel-gfx] [PATCH 4/7] drm/i915: Check display_bpc against max_fdi_bpp after display_bpc is set

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> @@ -3845,8 +3836,20 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>
>         display_bpc = min(display_bpc, bpc);
>
> -       DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d)\n",
> -                     bpc, display_bpc);
> +       display_bpc = 6;

It seems that you are overriding display_bpc unconditionally here,
some left over from debugging?

> +       if (display_bpc * 3 > max_fdi_bpp) {
> +               if (max_fdi_bpp < 24)
> +                       display_bpc = 6;
> +               else if (max_fdi_bpp < 30)
> +                       display_bpc = 8;
> +               else if (max_fdi_bpp < 36)
> +                       display_bpc = 10;
> +               DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
> +       }
> +
> +       DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d) (max_fdi_bpp %d)\n",
> +                     bpc, display_bpc, max_fdi_bpp);
>
>         *pipe_bpp = display_bpc * 3;

"setting pipe bpc to %d", bpc and  *pipe_bpp = display_bpc, looks like
a bogus debug message to me.

> @@ -4763,9 +4765,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                  * Compute the available FDI bandwidth, use that
>                  * to compute the maximum supported BPP
>                  */
> -               fdi_bw = link_bw * max_lane * 19 / 20;
> -               max_fdi_bpp = fdi_bw / target_clock;
> -               DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
> +               fdi_bw = (link_bw * 8) * max_lane;
> +               pps = target_clock * 21 / 20;
> +
> +               max_fdi_bpp = fdi_bw / pps;
> +               DRM_DEBUG_KMS("link_bw %d max_lane %d fdi_bw %u pps %u max_fdi_bpp %d\n",
> +                             link_bw, max_lane, fdi_bw, pps, max_fdi_bpp);
>         }

While I understood the first computation of max_fdi_bpp in patch 2 of
this series, I have to confess you lost me there. Would you mind
clarifying this?

-- 
Damien

Re: [Intel-gfx] [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1597 bytes --]

"Lespiau, Damien" <damien.lespiau@intel.com> writes:

> On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
>
> @@ -3728,7 +3728,8 @@ static inline bool intel_panel_use_ssc(struct
> drm_i915_private *dev_priv)
>   */
>  static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>                                          unsigned int *pipe_bpp,
> -                                        struct drm_display_mode *mode)
> +                                        struct drm_display_mode *mode,
> +                                        int max_fdi_bpp)
>
> There's some kernel-doc for this function, maybe add a @max_fdi_bpp
> there?

Will do

> This chunk is being moved around in a later patch in the series,
> merging the two patches in one looks like a good idea?

Or at least move this into its final position in this patch.

> I guess this does not cover the case of pipe B using 3 lanes meaning
> pipe C can use 1?

It didn't look like that was a supported mode from the docs.

> This duplicates the code just that is just a few lines away, instead
> how about moving the logic to set target_clock up in front of this
> whole if()?

It's not the same, it's the inverse -- computing bpp from lanes+clock
clock instead of computing lanes from bpp+clock. But, yeah, it would be
nice to have these merged somehow. I couldn't figure out a good way though.

> This chunk is also reworked in a later commit in this series.

I'll see if I can't avoid that as it's confusing. Thanks for your review!

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [Intel-gfx] [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

[Lespiau, Damien]

On Fri, Aug 17, 2012 at 4:00 PM, Keith Packard <keithp@keithp.com> wrote:
>> I guess this does not cover the case of pipe B using 3 lanes meaning
>> pipe C can use 1?
>
> It didn't look like that was a supported mode from the docs.

Ah yes, found it now "FDI B maximum port width is 4 lanes when FDI C
is disabled, 2 lanes when FDI C is enabled."

>> This duplicates the code just that is just a few lines away, instead
>> how about moving the logic to set target_clock up in front of this
>> whole if()?
>
> It's not the same, it's the inverse -- computing bpp from lanes+clock
> clock instead of computing lanes from bpp+clock. But, yeah, it would be
> nice to have these merged somehow. I couldn't figure out a good way though.

I meant the

> +               if (is_dp)
> +                       target_clock = mode->clock;
> +               else
> +                       target_clock = adjusted_mode->clock;

Just after that else block you have

        /* [e]DP over FDI requires target mode clock instead of link clock. */
        if (edp_encoder)
                target_clock = intel_edp_target_clock(edp_encoder, mode);
        else if (is_dp)
                target_clock = mode->clock;
        else
                target_clock = adjusted_mode->clock;

Those look strangely similar to me. The later could be moved up.

-- 
Damien

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Delay between FDI link training tries. Clear FDI_RX_IIR before training

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> Just a bit of cleanup; it appears to have no effect.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Clearing the locking bit in FDI_RX_IIR looks like a good move and
waiting between tries can't hurt, looks good to me.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Pipe-C only configurations would not get SR

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> These should probably all look like
>
>         enabled |= (1 << pipe)
>
> so that the intent is clear...
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 94aabca..1a84425 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1815,7 +1815,7 @@ static void sandybridge_update_wm(struct drm_device *dev)
>                 DRM_DEBUG_KMS("FIFO watermarks For pipe C -"
>                               " plane %d, cursor: %d\n",
>                               plane_wm, cursor_wm);
> -               enabled |= 3;
> +               enabled |= 4;
>         }

Looks like a very good catch!

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [Intel-gfx] [PATCH 6/7] drm/i915: Disable FDI RX before FDI TX

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> Doesn't make sense to disable in the other order.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

I can't see anything in the docs about an order requirement for those.
Not sure why the other way does not make sense. Somehow disabling TX
before RX makes some sense to me (TX enabled without a ready RX looks
weird?, no data should flow as the pipe is shutdown at that point
anyway). Maybe it just does not matter?

Another detail is that disabling the PLLs seem to have an order in the
disabling sequence, TX, then RX.

I.  Disable CPU FDI Transmitter PLL
II. Disable PCH FDI Receiver PLL

-- 
Damien

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Merge FDI RX reg writes during training

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> @@ -2324,6 +2324,8 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>                 temp |= FDI_LINK_TRAIN_NONE | FDI_TX_ENHANCE_FRAME_ENABLE;
>         }
>         I915_WRITE(reg, temp);
> +       POSTING_READ(reg);
> +       udelay(100);

The docs don't mention a delay between writing the TX and RX training
patterns, the POSTING_READ() seems like a good idea.

>         reg = FDI_RX_CTL(pipe);
>         temp = I915_READ(reg);
> @@ -2334,16 +2336,15 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>                 temp &= ~FDI_LINK_TRAIN_NONE;
>                 temp |= FDI_LINK_TRAIN_NONE;
>         }
> +       /* IVB wants error correction enabled */
> +       if (IS_IVYBRIDGE(dev))
> +               temp |= FDI_FS_ERRC_ENABLE | FDI_FE_ERRC_ENABLE;
> +
>         I915_WRITE(reg, temp | FDI_RX_ENHANCE_FRAME_ENABLE);
>
>         /* wait one idle pattern time */
>         POSTING_READ(reg);
>         udelay(1000);
> -
> -       /* IVB wants error correction enabled */
> -       if (IS_IVYBRIDGE(dev))
> -               I915_WRITE(reg, I915_READ(reg) | FDI_FS_ERRC_ENABLE |
> -                          FDI_FE_ERRC_ENABLE);
>  }
>
>  static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [Intel-gfx] [PATCH 6/7] drm/i915: Disable FDI RX before FDI TX

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

"Lespiau, Damien" <damien.lespiau@intel.com> writes:

> I can't see anything in the docs about an order requirement for those.

Right, the docs don't say anything, which is a bit disconcerting.

> Not sure why the other way does not make sense. Somehow disabling TX
> before RX makes some sense to me (TX enabled without a ready RX looks
> weird?, no data should flow as the pipe is shutdown at that point
> anyway). Maybe it just does not matter?

And here I figured disabling RX before TX made more sense -- otherwise
the receiver wouldn't be seeing anything. In other areas of the driver,
we're careful to disable receivers before senders (disable CRTC before
PLL, etc).

> Another detail is that disabling the PLLs seem to have an order in the
> disabling sequence, TX, then RX.
>
> I.  Disable CPU FDI Transmitter PLL
> II. Disable PCH FDI Receiver PLL

That ordering doesn't matter as the FDI receiver and transmitter are
both disabled by that point, so they aren't talking at all.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]