From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm/i915: adjusted_mode->clock in the dp mode_fixup Date: Wed, 30 May 2012 14:51:39 +0200 Message-ID: <20120530125139.GG6888@phenom.ffwll.local> References: <20120530113252.GE6888@phenom.ffwll.local> <1338378723-4752-1-git-send-email-daniel.vetter@ffwll.ch> <1338380345_356426@CP5-2952> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49]) by gabe.freedesktop.org (Postfix) with ESMTP id 4D8EE9E78D for ; Wed, 30 May 2012 05:50:15 -0700 (PDT) Received: by eekd17 with SMTP id d17so2010417eek.36 for ; Wed, 30 May 2012 05:50:14 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1338380345_356426@CP5-2952> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: Daniel Vetter , Intel Graphics Development , DRI Development List-Id: dri-devel@lists.freedesktop.org On Wed, May 30, 2012 at 01:18:35PM +0100, Chris Wilson wrote: > On Wed, 30 May 2012 13:52:02 +0200, Daniel Vetter 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 > > --- > > 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