From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vandana Kannan Subject: Re: [PATCH 3/3] drm/i915: Program PFI credits for VLV Date: Mon, 18 Aug 2014 14:44:45 +0530 Message-ID: <53F1C405.6080403@intel.com> References: <20140805154037.GO4193@intel.com> <1407417003-10564-1-git-send-email-vandana.kannan@intel.com> <1407417003-10564-3-git-send-email-vandana.kannan@intel.com> <20140808130312.GG4193@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id AED096E287 for ; Mon, 18 Aug 2014 02:14:48 -0700 (PDT) In-Reply-To: <20140808130312.GG4193@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: "intel-gfx@lists.freedesktop.org" , "Srinivas, Vidya" List-Id: intel-gfx@lists.freedesktop.org Hi Ville, Apologize for the delay in reply. For your inputs on the programming side (modeset global resources handling and self documenting code parts), I agree with you and will make the changes. For opens related to info on the feature, internal discussions are ongoing. I will get back to you on them next week.. Thanks, Vandana On Aug-08-2014 6:33 PM, Ville Syrj=E4l=E4 wrote: > On Thu, Aug 07, 2014 at 06:40:03PM +0530, Vandana Kannan wrote: >> From: Vidya Srinivas >> >> PFI credit programming is required when CD clock (related to data flow f= rom >> display pipeline to end display) is greater than CZ clock (related to da= ta >> flow from memory to display plane). This programming should be done when= all >> planes are OFF to avoid intermittent hangs while accessing memory even f= rom >> different Gfx units (not just display). >> >> If cdclk/czclk >=3D1, PFI credits could be set as any number. To get bet= ter >> performance, larger PFI credit can be assigned to PND. Otherwise if >> cdclk/czclk<1, the default PFI credit of 8 should be set. > = > Do we have any docs for this? I see the register in the docs but nothing > about the correct programming. Some kind of refrence to the used > documentation would be nice. > = >> >> v2: >> - Change log to lower log level instead of DRM_ERROR >> - Change function name to valleyview_program_pfi_credits >> - Move program PFI credits to modeset_init instead of intel_set_mode >> - Change magic numbers to logical constants >> >> Signed-off-by: Vidya Srinivas >> Signed-off-by: Gajanan Bhat >> Signed-off-by: Vandana Kannan >> --- >> drivers/gpu/drm/i915/i915_drv.c | 6 ++++++ >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/i915_reg.h | 5 +++++ >> drivers/gpu/drm/i915/intel_display.c | 4 +++- >> drivers/gpu/drm/i915/intel_pm.c | 22 ++++++++++++++++++++++ >> 5 files changed, 38 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915= _drv.c >> index 6c4b25c..00e0b4a 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.c >> +++ b/drivers/gpu/drm/i915/i915_drv.c >> @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev) >> intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED); >> console_unlock(); >> = >> + if (IS_VALLEYVIEW(dev)) >> + valleyview_program_pfi_credits(dev_priv, false); > = > If we want to do this for system suspend I think we should stick it > into i915_drm_freeze() (just after turning off the pipes). Not sure if > we should also force cdclk to minimum there... > = > Hmm. Is the GCI_CONTROL register is part of the disp2d power well? If it > is we probably need to enable the power well around the register write. > = >> + >> dev_priv->suspend_count++; >> = >> intel_display_set_init_power(dev_priv, false); >> @@ -693,6 +696,9 @@ static int __i915_drm_thaw(struct drm_device *dev, b= ool restore_gtt_mappings) >> dev_priv->modeset_restore =3D MODESET_DONE; >> mutex_unlock(&dev_priv->modeset_restore_lock); >> = >> + if (IS_VALLEYVIEW(dev)) >> + valleyview_program_pfi_credits(dev_priv, true); > = > I think this thing can be dropped. We need to do the reprogramming as > part of the modeset global resources handling. > = >> + >> intel_opregion_notify_adapter(dev, PCI_D0); >> = >> return 0; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915= _drv.h >> index 881e0a6..88fd4c7 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2172,6 +2172,8 @@ extern struct i915_params i915 __read_mostly; >> = >> /* i915_dma.c */ >> void i915_update_dri1_breadcrumb(struct drm_device *dev); >> +extern void valleyview_program_pfi_credits(struct drm_i915_private *dev= _priv, >> + bool flag); >> extern void i915_kernel_lost_context(struct drm_device * dev); >> extern int i915_driver_load(struct drm_device *, unsigned long flags); >> extern int i915_driver_unload(struct drm_device *); >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915= _reg.h >> index fb111cd..7f4c2f5 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -1937,6 +1937,11 @@ enum punit_power_well { >> #define CZCLK_FREQ_MASK 0xf >> #define GMBUSFREQ_VLV (VLV_DISPLAY_BASE + 0x6510) >> = >> +#define GCI_CONTROL (VLV_DISPLAY_BASE + 0x650C) >> +#define PFI_CREDIT (7 << 28) > = > Maybe something like this for a bit more self documenting code. > = > #define PFI_CREDIT(x) (((x)-8) << 28) /* 8-15 */ > = >> +#define PFI_CREDIT_RESEND (1 << 27) >> +#define VGA_FAST_MODE_DISABLE (1 << 14) >> + >> /* >> * Palette regs >> */ >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915= /intel_display.c >> index 2089319..2af2e60 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -12691,8 +12691,10 @@ void intel_modeset_init_hw(struct drm_device *d= ev) >> { >> intel_prepare_ddi(dev); >> = >> - if (IS_VALLEYVIEW(dev)) >> + if (IS_VALLEYVIEW(dev)) { >> vlv_update_cdclk(dev); >> + valleyview_program_pfi_credits(dev->dev_private, true); > = > The planes may be be active here. So for the initial state we just need > to trust that the BIOS got it right. > = > For runtime cdclk changes this needs to be handled in > valleyview_modeset_global_resources(). > = >> + } >> = >> intel_init_clock_gating(dev); >> = >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/inte= l_pm.c >> index ba8dfeb..ad8e190 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -7154,6 +7154,28 @@ void intel_fini_runtime_pm(struct drm_i915_privat= e *dev_priv) >> pm_runtime_disable(device); >> } >> = >> +void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv, >> + bool flag) >> +{ >> + int cd_clk, cz_clk; >> + >> + if (!flag) { >> + I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE); >> + return; >> + } >> + >> + cd_clk =3D dev_priv->display.get_display_clock_speed(dev_priv->dev); >> + cz_clk =3D valleyview_get_cz_clock_speed(dev_priv->dev); >> + >> + if (cd_clk >=3D cz_clk) { >> + /* WA - write default credits before re-programming */ >> + I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE); > = > Why do you write just the vga fast mode disable bit first? > = >> + I915_WRITE(GCI_CONTROL, (PFI_CREDIT | PFI_CREDIT_RESEND | >> + VGA_FAST_MODE_DISABLE)); >> + } else >> + DRM_DEBUG_KMS("cd clk < cz clk"); > = > So we never reprogram the credits for the cd something like this here? > I915_WRITE(GCI_CONTROL, PFI_CREDIT(8) | PFI_CREDIT_RESEND | VGA_FAST_MODE= _DISABLE); > = > Also does the PFI_CREDIT_RESEND bit get cleared immediately by the > hardware or do we have to poll for it to clear? > = >> +} >> + >> /* Set up chip specific power management-related functions */ >> void intel_init_pm(struct drm_device *dev) >> { >> -- = >> 2.0.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > =