[Intel-gfx] [PATCH 13/35] drm/i915: Store the watermark latency values in dev_priv
Paulo Zanoni
przanoni at gmail.com
Tue Jul 30 23:42:45 CEST 2013
I forgot to add intel-gfx in the review email...
2013/7/30 Paulo Zanoni <przanoni at gmail.com>:
> 2013/7/5 <ville.syrjala at linux.intel.com>:
>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> Rather than having to read the latency values out every time, just
>> store them in dev_priv.
>>
>> On ILK and IVB there is a difference between some of the latency
>> values for different planes, so store the latency values for each
>> plane type separately, and apply the necesary fixups during init.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 9 ++++++
>> drivers/gpu/drm/i915/intel_pm.c | 62 +++++++++++++++++++++++++++++++++++------
>> 2 files changed, 62 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 99eb980..60f9437 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1184,6 +1184,15 @@ typedef struct drm_i915_private {
>>
>> struct i915_suspend_saved_registers regfile;
>>
>> + struct {
>> + /* watermark latency values for primary */
>> + uint16_t pri_latency[5];
>> + /* watermark latency values for sprite */
>> + uint16_t spr_latency[5];
>> + /* watermark latency values for cursor */
>> + uint16_t cur_latency[5];
>> + } wm;
>> +
>> /* Old dri1 support infrastructure, beware the dragons ya fools entering
>> * here! */
>> struct i915_dri1_state dri1;
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index e4d2477..68a1de4 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2367,6 +2367,39 @@ static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[5])
>> }
>> }
>>
>> +static void intel_fixup_spr_wm_latency(struct drm_device *dev, uint16_t wm[5])
>> +{
>> + /* ILK sprite LP0 latency is 1300 ns */
>> + if (INTEL_INFO(dev)->gen == 5)
>> + wm[0] = 13;
>> +}
>> +
>> +static void intel_fixup_cur_wm_latency(struct drm_device *dev, uint16_t wm[5])
>> +{
>> + /* ILK cursor LP0 latency is 1300 ns */
>> + if (INTEL_INFO(dev)->gen == 5)
>> + wm[0] = 13;
>> +
>> + /* WaDoubleCursorLP3Latency:ivb */
>> + if (IS_IVYBRIDGE(dev))
>> + wm[3] *= 2;
>
> Doesn't this WA apply only to pre-production steppings?
>
>
>> +}
>> +
>> +static void intel_setup_wm_latency(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + intel_read_wm_latency(dev, dev_priv->wm.pri_latency);
>> +
>> + memcpy(dev_priv->wm.spr_latency, dev_priv->wm.pri_latency,
>> + sizeof dev_priv->wm.pri_latency);
>> + memcpy(dev_priv->wm.cur_latency, dev_priv->wm.pri_latency,
>> + sizeof dev_priv->wm.pri_latency);
>
> Checkpatch.pl complains about these sizeof without parens.
>
>
>> +
>> + intel_fixup_spr_wm_latency(dev, dev_priv->wm.spr_latency);
>> + intel_fixup_cur_wm_latency(dev, dev_priv->wm.cur_latency);
>> +}
>> +
>> static void hsw_compute_wm_parameters(struct drm_device *dev,
>> struct hsw_pipe_wm_parameters *params,
>> struct hsw_wm_maximums *lp_max_1_2,
>> @@ -2612,16 +2645,17 @@ static void haswell_update_wm(struct drm_device *dev)
>> struct hsw_wm_maximums lp_max_1_2, lp_max_5_6;
>> struct hsw_pipe_wm_parameters params[3];
>> struct hsw_wm_values results_1_2, results_5_6, *best_results;
>> - uint16_t wm[5] = {};
>> enum hsw_data_buf_partitioning partitioning;
>>
>> - intel_read_wm_latency(dev, wm);
>> hsw_compute_wm_parameters(dev, params, &lp_max_1_2, &lp_max_5_6);
>>
>> - hsw_compute_wm_results(dev, params, wm, &lp_max_1_2, &results_1_2);
>> + hsw_compute_wm_results(dev, params,
>> + dev_priv->wm.pri_latency,
>> + &lp_max_1_2, &results_1_2);
>> if (lp_max_1_2.pri != lp_max_5_6.pri) {
>> - hsw_compute_wm_results(dev, params, wm, &lp_max_5_6,
>> - &results_5_6);
>> + hsw_compute_wm_results(dev, params,
>> + dev_priv->wm.pri_latency,
>> + &lp_max_5_6, &results_5_6);
>
> My vote would be to just not pass dev_priv->wm.pri_latency here, it's
> useless since we're already passing dev. And in the place where we
> actually use dev_priv->wm.pri_latency we should add a comment saying
> that on Haswell pri_latency should be the same as the others,
> otherwise people will start wondering why you're using pri_latency and
> not the others.
>
>
>> best_results = hsw_find_best_result(&results_1_2, &results_5_6);
>> } else {
>> best_results = &results_1_2;
>> @@ -5211,8 +5245,12 @@ void intel_init_pm(struct drm_device *dev)
>>
>> /* For FIFO watermark updates */
>> if (HAS_PCH_SPLIT(dev)) {
>> + intel_setup_wm_latency(dev);
>> +
>> if (IS_GEN5(dev)) {
>> - if (I915_READ(MLTR_ILK) & ILK_SRLT_MASK)
>> + if (dev_priv->wm.pri_latency[1] &&
>> + dev_priv->wm.spr_latency[1] &&
>> + dev_priv->wm.cur_latency[1])
>
> I always wondered why do we have these checks. Do we ever see real
> machines that have zero values in the latency fields? On some gens we
> only check wm[0] and not the others, but on ILK we check wm[1] and not
> the others... Anyway, this is not a problem that should be solved by
> this patch, but I'd love answers :)
>
>
>> dev_priv->display.update_wm = ironlake_update_wm;
>> else {
>> DRM_DEBUG_KMS("Failed to get proper latency. "
>> @@ -5221,7 +5259,9 @@ void intel_init_pm(struct drm_device *dev)
>> }
>> dev_priv->display.init_clock_gating = ironlake_init_clock_gating;
>> } else if (IS_GEN6(dev)) {
>> - if (SNB_READ_WM0_LATENCY()) {
>> + if (dev_priv->wm.pri_latency[0] &&
>> + dev_priv->wm.spr_latency[0] &&
>> + dev_priv->wm.cur_latency[0]) {
>> dev_priv->display.update_wm = sandybridge_update_wm;
>> dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm;
>> } else {
>> @@ -5231,7 +5271,9 @@ void intel_init_pm(struct drm_device *dev)
>> }
>> dev_priv->display.init_clock_gating = gen6_init_clock_gating;
>> } else if (IS_IVYBRIDGE(dev)) {
>> - if (SNB_READ_WM0_LATENCY()) {
>> + if (dev_priv->wm.pri_latency[0] &&
>> + dev_priv->wm.spr_latency[0] &&
>> + dev_priv->wm.cur_latency[0]) {
>> dev_priv->display.update_wm = ivybridge_update_wm;
>> dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm;
>> } else {
>> @@ -5241,7 +5283,9 @@ void intel_init_pm(struct drm_device *dev)
>> }
>> dev_priv->display.init_clock_gating = ivybridge_init_clock_gating;
>> } else if (IS_HASWELL(dev)) {
>> - if (I915_READ64(MCH_SSKPD)) {
>> + if (dev_priv->wm.pri_latency[0] &&
>> + dev_priv->wm.spr_latency[0] &&
>> + dev_priv->wm.cur_latency[0]) {
>> dev_priv->display.update_wm = haswell_update_wm;
>> dev_priv->display.update_sprite_wm =
>> haswell_update_sprite_wm;
>> --
>> 1.8.1.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
--
Paulo Zanoni
More information about the Intel-gfx
mailing list