[PATCH] drm/i915: don't chnage the original mode in dp_mode_fixup

[PATCH] drm/i915: don't chnage the original mode in dp_mode_fixup

[Daniel Vetter]

We should only frob adjusted_mode. This is in preparation of
a massive patch by Laurent Pinchart to make the mode argument
const.

The only thing we actually touch is mode->clock, but only if
it's a panel. And in that case we also set adjusted_mode->clock
to the same value. All the generic code already uses the
adjusted_mode exclusively, so we only have to move the dp
link bw calculations over to that. This requires a small
changes so that the shared code with mode_valid doesn't
touch the mode argument.

Also mark the mode argument of pch_panel_fitting const.

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c    |   19 +++++++------------
 drivers/gpu/drm/i915/intel_drv.h   |    2 +-
 drivers/gpu/drm/i915/intel_panel.c |    2 +-
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 296cfc2..05c4748 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -222,7 +222,7 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
 static bool
 intel_dp_adjust_dithering(struct intel_dp *intel_dp,
 			  struct drm_display_mode *mode,
-			  struct drm_display_mode *adjusted_mode)
+			  bool adjust_mode)
 {
 	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
 	int max_lanes = intel_dp_max_lane_count(intel_dp);
@@ -236,8 +236,8 @@ intel_dp_adjust_dithering(struct intel_dp *intel_dp,
 		if (mode_rate > max_rate)
 			return false;
 
-		if (adjusted_mode)
-			adjusted_mode->private_flags
+		if (adjust_mode)
+			mode->private_flags
 				|= INTEL_MODE_DP_FORCE_6BPC;
 
 		return true;
@@ -260,7 +260,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 	}
 
-	if (!intel_dp_adjust_dithering(intel_dp, mode, NULL))
+	if (!intel_dp_adjust_dithering(intel_dp, mode, false))
 		return MODE_CLOCK_HIGH;
 
 	if (mode->clock < 10000)
@@ -698,25 +698,20 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
 		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
 					mode, adjusted_mode);
-		/*
-		 * the mode->clock is used to calculate the Data&Link M/N
-		 * of the pipe. For the eDP the fixed clock should be used.
-		 */
-		mode->clock = intel_dp->panel_fixed_mode->clock;
 	}
 
-	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
 		return false;
 
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %02x pixel clock %iKHz\n",
 		      max_lane_count, bws[max_clock], mode->clock);
 
-	if (!intel_dp_adjust_dithering(intel_dp, mode, adjusted_mode))
+	if (!intel_dp_adjust_dithering(intel_dp, adjusted_mode, true))
 		return false;
 
 	bpp = adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
-	mode_rate = intel_dp_link_required(mode->clock, bpp);
+	mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
 
 	for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
 		for (clock = 0; clock <= max_clock; clock++) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3e09188..3e3b60c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -372,7 +372,7 @@ extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 				   struct drm_display_mode *adjusted_mode);
 extern void intel_pch_panel_fitting(struct drm_device *dev,
 				    int fitting_mode,
-				    struct drm_display_mode *mode,
+				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adjusted_mode);
 extern u32 intel_panel_get_max_backlight(struct drm_device *dev);
 extern u32 intel_panel_get_backlight(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 2a1625d..7180cc8 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -56,7 +56,7 @@ intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 void
 intel_pch_panel_fitting(struct drm_device *dev,
 			int fitting_mode,
-			struct drm_display_mode *mode,
+			const struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.7.10

Re: [PATCH] drm/i915: don't chnage the original mode in dp_mode_fixup

[Chris Wilson]

On Wed, 30 May 2012 12:28:04 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We should only frob adjusted_mode. This is in preparation of
> a massive patch by Laurent Pinchart to make the mode argument
> const.
> 
> The only thing we actually touch is mode->clock, but only if
> it's a panel. And in that case we also set adjusted_mode->clock
> to the same value. All the generic code already uses the
> adjusted_mode exclusively, so we only have to move the dp
> link bw calculations over to that. This requires a small
> changes so that the shared code with mode_valid doesn't
> touch the mode argument.

Separate patch please, I'm sure you are right, but that is the scary
one...

> 
> Also mark the mode argument of pch_panel_fitting const.
> 
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c    |   19 +++++++------------
>  drivers/gpu/drm/i915/intel_drv.h   |    2 +-
>  drivers/gpu/drm/i915/intel_panel.c |    2 +-
>  3 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 296cfc2..05c4748 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -222,7 +222,7 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
>  static bool
>  intel_dp_adjust_dithering(struct intel_dp *intel_dp,
>  			  struct drm_display_mode *mode,
> -			  struct drm_display_mode *adjusted_mode)
> +			  bool adjust_mode)

Would this look more pleasant if you rewrote this function so that the
adjustment of flags was done in the caller?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: don't chnage the original mode in dp_mode_fixup

[Daniel Vetter]

On Wed, May 30, 2012 at 11:58:43AM +0100, Chris Wilson wrote:
> On Wed, 30 May 2012 12:28:04 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > We should only frob adjusted_mode. This is in preparation of
> > a massive patch by Laurent Pinchart to make the mode argument
> > const.
> > 
> > The only thing we actually touch is mode->clock, but only if
> > it's a panel. And in that case we also set adjusted_mode->clock
> > to the same value. All the generic code already uses the
> > adjusted_mode exclusively, so we only have to move the dp
> > link bw calculations over to that. This requires a small
> > changes so that the shared code with mode_valid doesn't
> > touch the mode argument.
> 
> Separate patch please, I'm sure you are right, but that is the scary
> one...

Will do.

> > Also mark the mode argument of pch_panel_fitting const.
> > 
> > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c    |   19 +++++++------------
> >  drivers/gpu/drm/i915/intel_drv.h   |    2 +-
> >  drivers/gpu/drm/i915/intel_panel.c |    2 +-
> >  3 files changed, 9 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 296cfc2..05c4748 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -222,7 +222,7 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
> >  static bool
> >  intel_dp_adjust_dithering(struct intel_dp *intel_dp,
> >  			  struct drm_display_mode *mode,
> > -			  struct drm_display_mode *adjusted_mode)
> > +			  bool adjust_mode)
> 
> Would this look more pleasant if you rewrote this function so that the
> adjustment of flags was done in the caller?

Well, I've added the adjusted_mode parameter originally to exactly avoid
this duplication of code between mode_fixup and mode_valid (which caused a
bug). Atm we could just pass back a needs_6bpc_dither flag, but I guess
we'll eventually need similar logic for higher bpc (and maybe other funky
stuff). So I think setting the flag here is ok and the least ugly version.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 1/2] drm/i915: adjusted_mode->clock in the dp mode_fixup

[Daniel Vetter]

... instead of changing mode->clock, which we should leave as-is.

We only touch that if it's a panel, and then adjusted mode->clock
equals adjusted_mode->clock. Outside of intel_dp.c we only use
ajusted_mode->clock in the mode_set functions.

Within intel_dp.c we only use it to calculate the dp dithering
and link bw parameters, so that's the only thing we need to fix
up.

As a temporary ugliness (until the cleanup in the next patch) we
pass the adjusted_mode into dp_dither for both parameters (because
that one still looks at mode->clock).

Note that we do overwrite adjusted_mode->clock with the selected dp
link clock, but that only happens after we've calculated everything we
need based on the dotclock of the adjusted output configuration.

v2: Adjust the debug message to also use adjusted_mode->clock.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 296cfc2..a9dffa6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -698,11 +698,6 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
 		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
 					mode, adjusted_mode);
-		/*
-		 * the mode->clock is used to calculate the Data&Link M/N
-		 * of the pipe. For the eDP the fixed clock should be used.
-		 */
-		mode->clock = intel_dp->panel_fixed_mode->clock;
 	}
 
 	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
@@ -710,13 +705,13 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %02x pixel clock %iKHz\n",
-		      max_lane_count, bws[max_clock], mode->clock);
+		      max_lane_count, bws[max_clock], adjusted_mode->clock);
 
-	if (!intel_dp_adjust_dithering(intel_dp, mode, adjusted_mode))
+	if (!intel_dp_adjust_dithering(intel_dp, adjusted_mode, adjusted_mode))
 		return false;
 
 	bpp = adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
-	mode_rate = intel_dp_link_required(mode->clock, bpp);
+	mode_rate = intel_dp_link_required(adjusted_mode->clock, bpp);
 
 	for (lane_count = 1; lane_count <= max_lane_count; lane_count <<= 1) {
 		for (clock = 0; clock <= max_clock; clock++) {
-- 
1.7.10

[PATCH 2/2] drm/i915: don't chnage the original mode in dp_mode_fixup

[Daniel Vetter]

We should only frob adjusted_mode. This is in preparation of
a massive patch by Laurent Pinchart to make the mode argument
const.

After the prevous prep patch to use adjusted_mode->clock instead of
mode->clock the only thing left is to clean up things a bit. I've
opted to pass in an adjust_mode param to dp_adjust_dithering because
that way we can be sure to avoid duplicating this logic - which was
the cause behind a dp link bw calculation bug in the past.

Also mark the mode argument of pch_panel_fitting const.

v2: Split up the mode->clock => adjusted_mode->clock change,
as suggested by Chris Wilson.

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c    |   12 ++++++------
 drivers/gpu/drm/i915/intel_drv.h   |    2 +-
 drivers/gpu/drm/i915/intel_panel.c |    2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a9dffa6..c5c5669 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -222,7 +222,7 @@ intel_dp_max_data_rate(int max_link_clock, int max_lanes)
 static bool
 intel_dp_adjust_dithering(struct intel_dp *intel_dp,
 			  struct drm_display_mode *mode,
-			  struct drm_display_mode *adjusted_mode)
+			  bool adjust_mode)
 {
 	int max_link_clock = intel_dp_link_clock(intel_dp_max_link_bw(intel_dp));
 	int max_lanes = intel_dp_max_lane_count(intel_dp);
@@ -236,8 +236,8 @@ intel_dp_adjust_dithering(struct intel_dp *intel_dp,
 		if (mode_rate > max_rate)
 			return false;
 
-		if (adjusted_mode)
-			adjusted_mode->private_flags
+		if (adjust_mode)
+			mode->private_flags
 				|= INTEL_MODE_DP_FORCE_6BPC;
 
 		return true;
@@ -260,7 +260,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			return MODE_PANEL;
 	}
 
-	if (!intel_dp_adjust_dithering(intel_dp, mode, NULL))
+	if (!intel_dp_adjust_dithering(intel_dp, mode, false))
 		return MODE_CLOCK_HIGH;
 
 	if (mode->clock < 10000)
@@ -700,14 +700,14 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 					mode, adjusted_mode);
 	}
 
-	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
 		return false;
 
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %02x pixel clock %iKHz\n",
 		      max_lane_count, bws[max_clock], adjusted_mode->clock);
 
-	if (!intel_dp_adjust_dithering(intel_dp, adjusted_mode, adjusted_mode))
+	if (!intel_dp_adjust_dithering(intel_dp, adjusted_mode, true))
 		return false;
 
 	bpp = adjusted_mode->private_flags & INTEL_MODE_DP_FORCE_6BPC ? 18 : 24;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3e09188..3e3b60c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -372,7 +372,7 @@ extern void intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 				   struct drm_display_mode *adjusted_mode);
 extern void intel_pch_panel_fitting(struct drm_device *dev,
 				    int fitting_mode,
-				    struct drm_display_mode *mode,
+				    const struct drm_display_mode *mode,
 				    struct drm_display_mode *adjusted_mode);
 extern u32 intel_panel_get_max_backlight(struct drm_device *dev);
 extern u32 intel_panel_get_backlight(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 2a1625d..7180cc8 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -56,7 +56,7 @@ intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 void
 intel_pch_panel_fitting(struct drm_device *dev,
 			int fitting_mode,
-			struct drm_display_mode *mode,
+			const struct drm_display_mode *mode,
 			struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.7.10

Re: [PATCH 1/2] drm/i915: adjusted_mode->clock in the dp mode_fixup

[Chris Wilson]

On Wed, 30 May 2012 13:52:02 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> ... instead of changing mode->clock, which we should leave as-is.
> 
> We only touch that if it's a panel, and then adjusted mode->clock
> equals adjusted_mode->clock. Outside of intel_dp.c we only use
> ajusted_mode->clock in the mode_set functions.
> 
> Within intel_dp.c we only use it to calculate the dp dithering
> and link bw parameters, so that's the only thing we need to fix
> up.
> 
> As a temporary ugliness (until the cleanup in the next patch) we
> pass the adjusted_mode into dp_dither for both parameters (because
> that one still looks at mode->clock).
> 
> Note that we do overwrite adjusted_mode->clock with the selected dp
> link clock, but that only happens after we've calculated everything we
> need based on the dotclock of the adjusted output configuration.
> 
> v2: Adjust the debug message to also use adjusted_mode->clock.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 296cfc2..a9dffa6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -698,11 +698,6 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
>  		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
>  		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
>  					mode, adjusted_mode);
> -		/*
> -		 * the mode->clock is used to calculate the Data&Link M/N
> -		 * of the pipe. For the eDP the fixed clock should be used.
> -		 */
> -		mode->clock = intel_dp->panel_fixed_mode->clock;

But in ironlake_crtc_mode_set() we have:

 if (is_cpu_edp) {
     target_clock = mode->clock;
 } else {
   if (is_dp)
     target_clock = mode->clock;
   else
     target_clock = adjusted_mode->clock;
 }

It would seem like eDP would have had mode->clock adjusted previously.
Similarly PCH eDP uses the adjusted mode->clock in intel_dp_set_m_n().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: adjusted_mode->clock in the dp mode_fixup

[Daniel Vetter]

On Wed, May 30, 2012 at 01:18:35PM +0100, Chris Wilson wrote:
> On Wed, 30 May 2012 13:52:02 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > ... instead of changing mode->clock, which we should leave as-is.
> > 
> > We only touch that if it's a panel, and then adjusted mode->clock
> > equals adjusted_mode->clock. Outside of intel_dp.c we only use
> > ajusted_mode->clock in the mode_set functions.
> > 
> > Within intel_dp.c we only use it to calculate the dp dithering
> > and link bw parameters, so that's the only thing we need to fix
> > up.
> > 
> > As a temporary ugliness (until the cleanup in the next patch) we
> > pass the adjusted_mode into dp_dither for both parameters (because
> > that one still looks at mode->clock).
> > 
> > Note that we do overwrite adjusted_mode->clock with the selected dp
> > link clock, but that only happens after we've calculated everything we
> > need based on the dotclock of the adjusted output configuration.
> > 
> > v2: Adjust the debug message to also use adjusted_mode->clock.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 296cfc2..a9dffa6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -698,11 +698,6 @@ intel_dp_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
> >  		intel_fixed_panel_mode(intel_dp->panel_fixed_mode, adjusted_mode);
> >  		intel_pch_panel_fitting(dev, DRM_MODE_SCALE_FULLSCREEN,
> >  					mode, adjusted_mode);
> > -		/*
> > -		 * the mode->clock is used to calculate the Data&Link M/N
> > -		 * of the pipe. For the eDP the fixed clock should be used.
> > -		 */
> > -		mode->clock = intel_dp->panel_fixed_mode->clock;
> 
> But in ironlake_crtc_mode_set() we have:
> 
>  if (is_cpu_edp) {
>      target_clock = mode->clock;
>  } else {
>    if (is_dp)
>      target_clock = mode->clock;
>    else
>      target_clock = adjusted_mode->clock;
>  }
> 
> It would seem like eDP would have had mode->clock adjusted previously.
> Similarly PCH eDP uses the adjusted mode->clock in intel_dp_set_m_n().

Well, adjusted_mode->clock after we've run intel_dp_mode_fixup should be
the same in all cases, because we overwrite it with the fixed dp link
clock we're selecting. The target_clock otoh looks more worrisome. I guess
I've managed to not hit this because I don't have an eDP panel (where we
change mode->clock), but still I guess we need to clean this up a bit.

I'll try to come up with a way to avoid this maze.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm/i915: compute the target_clock for edp directly

[Daniel Vetter]

Instead of abusing into mode->clock, because we should touch that
one at all. First prep step to constify the mode argument to the
intel_dp_mode_fixup function.

The next patch will stop us from modifying mode->clock.

Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   16 ++++++++--------
 drivers/gpu/drm/i915/intel_dp.c      |   12 ++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    2 ++
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9147894..a5d06ed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4431,16 +4431,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* CPU eDP doesn't require FDI link, so just set DP M/N
 	   according to current link config */
 	if (is_cpu_edp) {
-		target_clock = mode->clock;
 		intel_edp_link_config(edp_encoder, &lane, &link_bw);
 	} else {
-		/* [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;
-
 		/* 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
@@ -4451,6 +4443,14 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
 	}
 
+	/* [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;
+
 	/* determine panel color depth */
 	temp = I915_READ(PIPECONF(pipe));
 	temp &= ~PIPE_BPC_MASK;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 296cfc2..4190ed6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -152,6 +152,18 @@ intel_edp_link_config(struct intel_encoder *intel_encoder,
 		*link_bw = 270000;
 }
 
+int
+intel_edp_target_clock(struct intel_encoder *intel_encoder,
+		       struct drm_display_mode *mode)
+{
+	struct intel_dp *intel_dp = container_of(intel_encoder, struct intel_dp, base);
+
+	if (intel_dp->panel_fixed_mode)
+		return intel_dp->panel_fixed_mode->clock;
+	else
+		return mode->clock;
+}
+
 static int
 intel_dp_max_lane_count(struct intel_dp *intel_dp)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3e09188..706c21c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -360,6 +360,8 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 		 struct drm_display_mode *adjusted_mode);
 extern bool intel_dpd_is_edp(struct drm_device *dev);
 extern void intel_edp_link_config(struct intel_encoder *, int *, int *);
+extern int intel_edp_target_clock(struct intel_encoder *,
+				  struct drm_display_mode *mode);
 extern bool intel_encoder_is_pch_edp(struct drm_encoder *encoder);
 extern int intel_plane_init(struct drm_device *dev, enum pipe pipe);
 extern void intel_flush_display_plane(struct drm_i915_private *dev_priv,
-- 
1.7.10

Re: [PATCH 2/2] drm/i915: don't chnage the original mode in dp_mode_fixup

[Paul Menzel]

[-- Attachment #1.1: Type: text/plain, Size: 1155 bytes --]

Am Mittwoch, den 30.05.2012, 13:52 +0200 schrieb Daniel Vetter:

Typo in »change« in the commit message.

> We should only frob adjusted_mode. This is in preparation of
> a massive patch by Laurent Pinchart to make the mode argument
> const.
> 
> After the prevous prep patch to use adjusted_mode->clock instead of

previous

> mode->clock the only thing left is to clean up things a bit. I've
> opted to pass in an adjust_mode param to dp_adjust_dithering because
> that way we can be sure to avoid duplicating this logic - which was
> the cause behind a dp link bw calculation bug in the past.
> 
> Also mark the mode argument of pch_panel_fitting const.
> 
> v2: Split up the mode->clock => adjusted_mode->clock change,
> as suggested by Chris Wilson.
> 
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_dp.c    |   12 ++++++------
>  drivers/gpu/drm/i915/intel_drv.h   |    2 +-
>  drivers/gpu/drm/i915/intel_panel.c |    2 +-
>  3 files changed, 8 insertions(+), 8 deletions(-)

[…]


Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: compute the target_clock for edp directly

[Chris Wilson]

On Wed, 30 May 2012 15:34:20 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Instead of abusing into mode->clock, because we should touch that
> one at all. First prep step to constify the mode argument to the
> intel_dp_mode_fixup function.
> 
> The next patch will stop us from modifying mode->clock.
> 
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   16 ++++++++--------
>  drivers/gpu/drm/i915/intel_dp.c      |   12 ++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
>  3 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9147894..a5d06ed 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4431,16 +4431,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	/* CPU eDP doesn't require FDI link, so just set DP M/N
>  	   according to current link config */
>  	if (is_cpu_edp) {
> -		target_clock = mode->clock;
>  		intel_edp_link_config(edp_encoder, &lane, &link_bw);
>  	} else {
> -		/* [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;
> -
>  		/* 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
> @@ -4451,6 +4443,14 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
>  	}
>  
> +	/* [e]DP over FDI requires target mode clock instead of link clock. */
> +	if (edp_encoder)
> +		target_clock = intel_edp_target_clock(edp_encoder, mode);

Given the edp_encoder, we know that adjusted_mode contains either the
fixed_panel clock or the original clock depending on whether a fixed
mode was found. So the layering violation could be dropped here in
favour of target_clock = adjusted_mode->clok.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: compute the target_clock for edp directly

[Daniel Vetter]

On Thu, May 31, 2012 at 10:49:59AM +0100, Chris Wilson wrote:
> On Wed, 30 May 2012 15:34:20 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Instead of abusing into mode->clock, because we should touch that
> > one at all. First prep step to constify the mode argument to the
> > intel_dp_mode_fixup function.
> > 
> > The next patch will stop us from modifying mode->clock.
> > 
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   16 ++++++++--------
> >  drivers/gpu/drm/i915/intel_dp.c      |   12 ++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
> >  3 files changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9147894..a5d06ed 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4431,16 +4431,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >  	/* CPU eDP doesn't require FDI link, so just set DP M/N
> >  	   according to current link config */
> >  	if (is_cpu_edp) {
> > -		target_clock = mode->clock;
> >  		intel_edp_link_config(edp_encoder, &lane, &link_bw);
> >  	} else {
> > -		/* [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;
> > -
> >  		/* 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
> > @@ -4451,6 +4443,14 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> >  		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> >  	}
> >  
> > +	/* [e]DP over FDI requires target mode clock instead of link clock. */
> > +	if (edp_encoder)
> > +		target_clock = intel_edp_target_clock(edp_encoder, mode);
> 
> Given the edp_encoder, we know that adjusted_mode contains either the
> fixed_panel clock or the original clock depending on whether a fixed
> mode was found. So the layering violation could be dropped here in
> favour of target_clock = adjusted_mode->clok.

It's not that simple. Assuming I understand this maze correctly, we're
dealing with 3 different clocks.
- The dp link clock, both the old and new code store that in
  adjusted_mode->clock at the end of intel_dp_mode_fixup.
- The dotclock of the scanned-out region, i.e. what we have in mode->clock
  before any fixup happens. No one cares about that one because we don't
  need to program that anywhere (the panel fitter doesn't need it to do
  it's job).
- The dotclock of the displayed mode, i.e. what comes out after the panel
  fitting. The old code stored that in mode->clock (simply because that
  was available I guess). The new code recomputes it (which is rather
  simple because we only use the panel fitter for laptop panels and not to
  e.g. upscale progressive content to a 1080i TV which can't display
  1080p).

If I understand things correctly, we need to program the link clock into
the pch pll, but the fdi clock actually wants the dotclock of the
displayed mode. We store the later in target_clock in ilk_crtc_mode_set.

Obviously, given the complexity and the rampant bad naming schemes for
the involved variables I'm pretty sure I'm still getting this wrong
somewhere ...

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: compute the target_clock for edp directly

[Chris Wilson]

On Thu, 31 May 2012 12:03:53 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, May 31, 2012 at 10:49:59AM +0100, Chris Wilson wrote:
> > On Wed, 30 May 2012 15:34:20 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > Instead of abusing into mode->clock, because we should touch that
> > > one at all. First prep step to constify the mode argument to the
> > > intel_dp_mode_fixup function.
> > > 
> > > The next patch will stop us from modifying mode->clock.
> > > 
> > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   16 ++++++++--------
> > >  drivers/gpu/drm/i915/intel_dp.c      |   12 ++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
> > >  3 files changed, 22 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 9147894..a5d06ed 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4431,16 +4431,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> > >  	/* CPU eDP doesn't require FDI link, so just set DP M/N
> > >  	   according to current link config */
> > >  	if (is_cpu_edp) {
> > > -		target_clock = mode->clock;
> > >  		intel_edp_link_config(edp_encoder, &lane, &link_bw);
> > >  	} else {
> > > -		/* [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;
> > > -
> > >  		/* 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
> > > @@ -4451,6 +4443,14 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> > >  		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> > >  	}
> > >  
> > > +	/* [e]DP over FDI requires target mode clock instead of link clock. */
> > > +	if (edp_encoder)
> > > +		target_clock = intel_edp_target_clock(edp_encoder, mode);
> > 
> > Given the edp_encoder, we know that adjusted_mode contains either the
> > fixed_panel clock or the original clock depending on whether a fixed
> > mode was found. So the layering violation could be dropped here in
> > favour of target_clock = adjusted_mode->clok.
> 
> It's not that simple. Assuming I understand this maze correctly, we're
> dealing with 3 different clocks.
> - The dp link clock, both the old and new code store that in
>   adjusted_mode->clock at the end of intel_dp_mode_fixup.

Right, missed that adjusted_mode->clock was rewritten in dp_mode_fixup.
In fact, wouldn't that make the better export from intel_dp?
   target_clock = intel_dp_link_clock(adjusted_mode);
with a judicious adjusted_mode->private_flags |= INTEL_DP_LINK_BW_2_7/1_8?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: compute the target_clock for edp directly

[Daniel Vetter]

On Thu, May 31, 2012 at 11:31:50AM +0100, Chris Wilson wrote:
> On Thu, 31 May 2012 12:03:53 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, May 31, 2012 at 10:49:59AM +0100, Chris Wilson wrote:
> > > On Wed, 30 May 2012 15:34:20 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > > Instead of abusing into mode->clock, because we should touch that
> > > > one at all. First prep step to constify the mode argument to the
> > > > intel_dp_mode_fixup function.
> > > > 
> > > > The next patch will stop us from modifying mode->clock.
> > > > 
> > > > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c |   16 ++++++++--------
> > > >  drivers/gpu/drm/i915/intel_dp.c      |   12 ++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h     |    2 ++
> > > >  3 files changed, 22 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 9147894..a5d06ed 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -4431,16 +4431,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> > > >  	/* CPU eDP doesn't require FDI link, so just set DP M/N
> > > >  	   according to current link config */
> > > >  	if (is_cpu_edp) {
> > > > -		target_clock = mode->clock;
> > > >  		intel_edp_link_config(edp_encoder, &lane, &link_bw);
> > > >  	} else {
> > > > -		/* [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;
> > > > -
> > > >  		/* 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
> > > > @@ -4451,6 +4443,14 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> > > >  		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> > > >  	}
> > > >  
> > > > +	/* [e]DP over FDI requires target mode clock instead of link clock. */
> > > > +	if (edp_encoder)
> > > > +		target_clock = intel_edp_target_clock(edp_encoder, mode);
> > > 
> > > Given the edp_encoder, we know that adjusted_mode contains either the
> > > fixed_panel clock or the original clock depending on whether a fixed
> > > mode was found. So the layering violation could be dropped here in
> > > favour of target_clock = adjusted_mode->clok.
> > 
> > It's not that simple. Assuming I understand this maze correctly, we're
> > dealing with 3 different clocks.
> > - The dp link clock, both the old and new code store that in
> >   adjusted_mode->clock at the end of intel_dp_mode_fixup.
> 
> Right, missed that adjusted_mode->clock was rewritten in dp_mode_fixup.
> In fact, wouldn't that make the better export from intel_dp?
>    target_clock = intel_dp_link_clock(adjusted_mode);
> with a judicious adjusted_mode->private_flags |= INTEL_DP_LINK_BW_2_7/1_8?

I've considered this but then freaked out thinking about hunting down all
the places we use adjusted_mode->clock and checking whether I need to
change something ... So I've opted for the more minimal "mode->clock"
grep.

Call me a wimp, but before I try to slaugther that dragon I'd like to have
a more clear understanding of how this actually works and what it best
should look like.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48