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älä wrote:
> On Thu, Aug 07, 2014 at 06:40:03PM +0530, Vandana Kannan wrote:
>> From: Vidya Srinivas <vidya.srinivas@xxxxxxxxx>
>>
>> PFI credit programming is required when CD clock (related to data flow from
>> display pipeline to end display) is greater than CZ clock (related to data
>> flow from memory to display plane). This programming should be done when all
>> planes are OFF to avoid intermittent hangs while accessing memory even from
>> different Gfx units (not just display).
>>
>> If cdclk/czclk >=1, PFI credits could be set as any number. To get better
>> 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 <vidya.srinivas@xxxxxxxxx>
>> Signed-off-by: Gajanan Bhat <gajanan.bhat@xxxxxxxxx>
>> Signed-off-by: Vandana Kannan <vandana.kannan@xxxxxxxxx>
>> ---
>> 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, bool restore_gtt_mappings)
>> dev_priv->modeset_restore = 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 *dev)
>> {
>> 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/intel_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_private *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 = dev_priv->display.get_display_clock_speed(dev_priv->dev);
>> + cz_clk = valleyview_get_cz_clock_speed(dev_priv->dev);
>> +
>> + if (cd_clk >= 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<cz case? Shouldn't we have
> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
