On Wed, Nov 09, 2016 at 01:45:20PM +0100, Maarten Lankhorst wrote:
> Op 08-11-16 om 15:11 schreef Ville Syrjälä:
> > On Tue, Nov 08, 2016 at 01:55:36PM +0100, Maarten Lankhorst wrote:
> >> This is a hack and not needed. Use the right mask by checking
> >> intel_state->modeset. This works for watermark sanitization too.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/intel_pm.c | 38 +++++++++++++++-----------------------
> >> 1 file changed, 15 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >> index 02f52b52a03d..d38a46efcfed 100644
> >> --- a/drivers/gpu/drm/i915/intel_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_pm.c
> >> @@ -3089,26 +3089,22 @@ skl_ddb_get_pipe_allocation_limits(struct drm_device *dev,
> >> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >> 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 pipe_size, ddb_size, active_crtcs;
> >> int nth_active_pipe;
> >>
> >> + if (intel_state->modeset)
> >> + active_crtcs = intel_state->active_crtcs;
> >> + else
> >> + active_crtcs = dev_priv->active_crtcs;
> > What's the story with the locking here?
> if !modeset, 3 things can happen:
> 1. fastset, connection_mutex held, dev_priv->active_crtcs cannot change.
> 2. crtc disabled, active_crtcs is potentially garbage, but harmless since we don't write disabled wm's when the crtc is already disabled.
> (same as what happens currently)
But we still compute them? Can the computation fail on account of
that? I guess it shouldn't because when things are disabled all wms
should come out as 0?
> 3. crtc enabled, dev_priv->active_crtcs is valid because ddb reallocation requires locking all active crtc's for reallocation,
> which requires taking this lock for this crtc.
Case 3 at least looks a little suspect to me. What the code does is:
for_each_intel_crtc_in_mask(realloc_pipes) {
intel_atomic_get_crtc_state();
skl_allocate_pipe_ddb() {
skl_ddb_get_pipe_allocation_limits();
}
...
}
Since it starts to compute this stuff already before it's necessarily
locked all the crtcs, I'm having a hard time convincing myself that
the state will always be up to date. I guess it might since
realloc_pipes should only have the one pipe unless active_crtcs is going
to change, but I don't think it's obvious at all when looking at the
code. Might want to change the way this is done for clarity if nothing
else.
I think we need to start documenting the locking rules for this kind
of device wide state better, and maybe try to sprinkle more locking
asserts around.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
