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;
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
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
... 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++) {
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;
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
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
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
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Daniel Vetter -
Daniel Vetter -
Paul Menzel