[Intel-gfx] [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Mahesh Kumar]

Hi,


On Saturday 27 May 2017 02:53 AM, Rodrigo Vivi wrote:
> On Fri, May 26, 2017 at 8:15 AM, Mahesh Kumar <mahesh1.kumar at intel.com> wrote:
>> Don't trust cached DDB values. Recalculate the ddb value if cached value
>> is zero.
>>
>> If i915 is build as a module, there may be a race condition when
>> cursor_disable call comes even before intel_fbdev_initial_config.
>> Which may lead to cached value being 0. And further commit will fail
>> until a modeset.
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_pm.c | 15 ++++++++++-----
>>   1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 936eef1634c7..b67be1355e39 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>          struct drm_i915_private *dev_priv = to_i915(dev);
>>          struct drm_crtc *for_crtc = cstate->base.crtc;
>>          unsigned int pipe_size, ddb_size;
>> +       unsigned int active_crtcs;
>>          int nth_active_pipe;
>>
>>          if (WARN_ON(!state) || !cstate->base.active) {
>> @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>          }
>>
>>          if (intel_state->active_pipe_changes)
>> -               *num_active = hweight32(intel_state->active_crtcs);
>> +               active_crtcs = intel_state->active_crtcs;
>>          else
>> -               *num_active = hweight32(dev_priv->active_crtcs);
>> +               active_crtcs = dev_priv->active_crtcs;
>>
>> +       *num_active = hweight32(active_crtcs);
>>          ddb_size = INTEL_INFO(dev_priv)->ddb_size;
>>          WARN_ON(ddb_size == 0);
>>
>> @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
>>                   * copy from old state to be sure
>>                   */
>>                  *alloc = to_intel_crtc_state(for_crtc->state)->wm.skl.ddb;
>> -               return;
>> +               if (!skl_ddb_entry_size(alloc))
>> +                       DRM_DEBUG_KMS("Cached pipe DDB is 0 recalculate\n");
>> +               else
>> +                       return;
> I see 2 different patches inside this patch here. One is this chunk
> here that does what commit message tells.
>
>>          }
>>
>> -       nth_active_pipe = hweight32(intel_state->active_crtcs &
>> +       nth_active_pipe = hweight32(active_crtcs &
>>                                      (drm_crtc_mask(for_crtc) - 1));
>> -       pipe_size = ddb_size / hweight32(intel_state->active_crtcs);
>> +       pipe_size = ddb_size / hweight32(active_crtcs);
> While this is fixing another bug where we were still using
> intel_state->active_crtcs here even when we use dev_priv->active_crtcs
> for num_active.
Thanks for review :)

This one is prerequisite for the first part. In earlier scenario, we 
will reach to this point only if (intel_state->active_pipe_changes) is true.
So it was ok to get num of active crtcs from 
"intel_state->active_crtcs". But after above change we may reach to this 
point even if
"intel_state->active_pipe_changes" is not true. So we should get the 
active_crtcs from correct struct.

Thats why I think this should be part of same patch. Please let me know 
if you still have different opinion.

thanks,
-Mahesh
>
> Am I missing or misunderstanding something?
>
>>          alloc->start = nth_active_pipe * ddb_size / *num_active;
>>          alloc->end = alloc->start + pipe_size;
>>   }
>> --
>> 2.11.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>