| Message ID | 1445964628-30226-24-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 27, 2015 at 02:50:25PM -0200, Paulo Zanoni wrote: > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a9434d1..fdbe068 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -917,9 +917,11 @@ struct i915_fbc { > bool active; > > struct intel_fbc_work { > - struct delayed_work work; > + bool scheduled; Ah, I found this confusingly named. scheduled implied to me that you wanted work_pending(), but you just want to asynchronously cancel the fbc worker. Just bool cancel? Or bool active? Though now I've come full circle and suggest bool scheduled. So /* Track whether the FBC worker has already been queued, * or asynchronously cancel the worker whilst it waits * before activation. */ What happens then if we quickly queue, cancel and want to requeue? The schedule_work() fails as the task is already pending, but the scheduled flag gets reset, so it just works. Perfect. > + struct work_struct work; > struct drm_framebuffer *fb; Hmm, don't we actually need to take references on the fb we schedule for activation? Since we already account for that the crtc->fb may be changed between queuing the work and executing it, for extra paranoia we should ensure that we have a reference in work->fb. (long standing bug, might as well fix it before we see it in the wild, time for another kms-flip race!) I think I've covered the basic issues with changing the type of worker and it looks fine, Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Em Ter, 2015-10-27 às 20:29 +0000, Chris Wilson escreveu: > On Tue, Oct 27, 2015 at 02:50:25PM -0200, Paulo Zanoni wrote: > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index a9434d1..fdbe068 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -917,9 +917,11 @@ struct i915_fbc { > > bool active; > > > > struct intel_fbc_work { > > - struct delayed_work work; > > + bool scheduled; > > Ah, I found this confusingly named. scheduled implied to me that you > wanted work_pending(), but you just want to asynchronously cancel the > fbc worker. Just bool cancel? Or bool active? Though now I've come > full > circle and suggest bool scheduled. So I agree 'bool scheduled' is not the perfect name. I thought about renaming it to 'bool cancel' many times. I'm willing to change in case someone requests it, but my understanding is that the paragraph above is not asking for a rename. > > /* Track whether the FBC worker has already been queued, > * or asynchronously cancel the worker whilst it waits > * before activation. > */ I can add this, although if someone suggests a better name we may not need it :) > > What happens then if we quickly queue, cancel and want to requeue? > The > schedule_work() fails as the task is already pending, but the > scheduled > flag gets reset, so it just works. Perfect. /me is confused. This case should work since everything is done with fbc.lock grabbed. > > + struct work_struct work; > > struct drm_framebuffer *fb; > > Hmm, don't we actually need to take references on the fb we schedule > for > activation? Since we already account for that the crtc->fb may be > changed between queuing the work and executing it, for extra paranoia > we > should ensure that we have a reference in work->fb. (long standing > bug, > might as well fix it before we see it in the wild, time for another > kms-flip race!) I'm not super familiar with this area, so I have to ask: what bad things can happen if we don't have a reference on work->fb? We're just comparing pointers here, so if work->fb is not referenced by the CRTC we won't do anything with it. If work->fb is referenced by the CRTC, it will already have a reference, right? I'm also not 100% sure if it's even possible to have crtc->fb != work- >fb without anything else canceling the work thread, so I just kept the old code around for now. > > I think I've covered the basic issues with changing the type of > worker > and it looks fine, > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > -Chris Thanks! >
On Wed, Oct 28, 2015 at 05:24:18PM +0000, Zanoni, Paulo R wrote: > Em Ter, 2015-10-27 às 20:29 +0000, Chris Wilson escreveu: > > On Tue, Oct 27, 2015 at 02:50:25PM -0200, Paulo Zanoni wrote: > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index a9434d1..fdbe068 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -917,9 +917,11 @@ struct i915_fbc { > > > bool active; > > > > > > struct intel_fbc_work { > > > - struct delayed_work work; > > > + bool scheduled; > > > > Ah, I found this confusingly named. scheduled implied to me that you > > wanted work_pending(), but you just want to asynchronously cancel the > > fbc worker. Just bool cancel? Or bool active? Though now I've come > > full > > circle and suggest bool scheduled. So > > I agree 'bool scheduled' is not the perfect name. I thought about > renaming it to 'bool cancel' many times. I'm willing to change in case > someone requests it, but my understanding is that the paragraph above > is not asking for a rename. > > > > > /* Track whether the FBC worker has already been queued, > > * or asynchronously cancel the worker whilst it waits > > * before activation. > > */ > > I can add this, although if someone suggests a better name we may not > need it :) Even if we do rename the variable, we might as well keep the comment. I think it's a reasonably succinct description of why that bool should exist. > > > > What happens then if we quickly queue, cancel and want to requeue? > > The > > schedule_work() fails as the task is already pending, but the > > scheduled > > flag gets reset, so it just works. Perfect. > > /me is confused. > > This case should work since everything is done with fbc.lock grabbed. I was just thinking about the case where the bool scheduled=false but the work was still queued, and you then wanted to reschedule it. It works just fine, the original work item remains queued and gets reactivated correctly (I was trying to find a flaw in the code and decided that it wasn't a flaw at all, at which point I was happy!) > > > > + struct work_struct work; > > > struct drm_framebuffer *fb; > > > > Hmm, don't we actually need to take references on the fb we schedule > > for > > activation? Since we already account for that the crtc->fb may be > > changed between queuing the work and executing it, for extra paranoia > > we > > should ensure that we have a reference in work->fb. (long standing > > bug, > > might as well fix it before we see it in the wild, time for another > > kms-flip race!) > > I'm not super familiar with this area, so I have to ask: what bad > things can happen if we don't have a reference on work->fb? The worst depends on the locking - if we borrow the reference and operate on work->fb == crtc->fb without the right lock, that can blow up. Otherwise, as just use work->fb as a pointer, it is possible for the fb to unref'ed and reallocated without us noticing and for us to then incorrectly schedule the fbc (unless you can demonstrate that the work->fb is guarded by the fbc.lock + bool scheduled). > We're just comparing pointers here, so if work->fb is not referenced by > the CRTC we won't do anything with it. If work->fb is referenced by the > CRTC, it will already have a reference, right? > > I'm also not 100% sure if it's even possible to have crtc->fb != work- > >fb without anything else canceling the work thread, so I just kept the > old code around for now. Indeed, I'm not sure if it is possible but (a) we should check that we do have sufficient locks to prevent crtc->fb disappearing, and (b) verify that when crtc->fb is changed the work is cancelled. As a bonus, document that work->fb is just a pointer whose validity is guarded by bool scheduled (or whatever). Or even better, having just completed (a) + (b), you can drop the drm_framebuffer pointer from the work item entirely and rely on crtc->fb. -Chris
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a9434d1..fdbe068 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -917,9 +917,11 @@ struct i915_fbc { bool active; struct intel_fbc_work { - struct delayed_work work; + bool scheduled; + struct work_struct work; struct drm_framebuffer *fb; - } *fbc_work; + unsigned long enable_jiffies; + } work; const char *no_fbc_reason; diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 4bb2323..a7192dd 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -444,85 +444,71 @@ static void intel_fbc_activate(const struct drm_framebuffer *fb) static void intel_fbc_work_fn(struct work_struct *__work) { - struct intel_fbc_work *work = - container_of(to_delayed_work(__work), - struct intel_fbc_work, work); - struct drm_i915_private *dev_priv = work->fb->dev->dev_private; - struct drm_framebuffer *crtc_fb = dev_priv->fbc.crtc->base.primary->fb; + struct drm_i915_private *dev_priv = + container_of(__work, struct drm_i915_private, fbc.work.work); + struct intel_fbc_work *work = &dev_priv->fbc.work; + struct intel_crtc *crtc = dev_priv->fbc.crtc; + unsigned long delay_jiffies = msecs_to_jiffies(50); + +retry: + /* Delay the actual enabling to let pageflipping cease and the + * display to settle before starting the compression. Note that + * this delay also serves a second purpose: it allows for a + * vblank to pass after disabling the FBC before we attempt + * to modify the control registers. + * + * A more complicated solution would involve tracking vblanks + * following the termination of the page-flipping sequence + * and indeed performing the enable as a co-routine and not + * waiting synchronously upon the vblank. + * + * WaFbcWaitForVBlankBeforeEnable:ilk,snb + */ + wait_remaining_ms_from_jiffies(work->enable_jiffies, delay_jiffies); mutex_lock(&dev_priv->fbc.lock); - if (work == dev_priv->fbc.fbc_work) { - /* Double check that we haven't switched fb without cancelling - * the prior work. - */ - if (crtc_fb == work->fb) - intel_fbc_activate(work->fb); - dev_priv->fbc.fbc_work = NULL; + /* Were we cancelled? */ + if (!work->scheduled) + goto out; + + /* Were we delayed again while this function was sleeping? */ + if (time_after(work->enable_jiffies + delay_jiffies, jiffies)) { + mutex_unlock(&dev_priv->fbc.lock); + goto retry; } - mutex_unlock(&dev_priv->fbc.lock); - kfree(work); + if (crtc->base.primary->fb == work->fb) + intel_fbc_activate(work->fb); + + work->scheduled = false; + +out: + mutex_unlock(&dev_priv->fbc.lock); } static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv) { WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); - - if (dev_priv->fbc.fbc_work == NULL) - return; - - /* Synchronisation is provided by struct_mutex and checking of - * dev_priv->fbc.fbc_work, so we can perform the cancellation - * entirely asynchronously. - */ - if (cancel_delayed_work(&dev_priv->fbc.fbc_work->work)) - /* tasklet was killed before being run, clean up */ - kfree(dev_priv->fbc.fbc_work); - - /* Mark the work as no longer wanted so that if it does - * wake-up (because the work was already running and waiting - * for our mutex), it will discover that is no longer - * necessary to run. - */ - dev_priv->fbc.fbc_work = NULL; + dev_priv->fbc.work.scheduled = false; } static void intel_fbc_schedule_activation(struct intel_crtc *crtc) { - struct intel_fbc_work *work; struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + struct intel_fbc_work *work = &dev_priv->fbc.work; WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); - intel_fbc_cancel_work(dev_priv); - - work = kzalloc(sizeof(*work), GFP_KERNEL); - if (work == NULL) { - DRM_ERROR("Failed to allocate FBC work structure\n"); - intel_fbc_activate(crtc->base.primary->fb); - return; - } - + /* It is useless to call intel_fbc_cancel_work() in this function since + * we're not releasing fbc.lock, so it won't have an opportunigy to grab + * it to discover that it was cancelled. So we just update the expected + * jiffy count. */ work->fb = crtc->base.primary->fb; - INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn); + work->scheduled = true; + work->enable_jiffies = jiffies; - dev_priv->fbc.fbc_work = work; - - /* Delay the actual enabling to let pageflipping cease and the - * display to settle before starting the compression. Note that - * this delay also serves a second purpose: it allows for a - * vblank to pass after disabling the FBC before we attempt - * to modify the control registers. - * - * A more complicated solution would involve tracking vblanks - * following the termination of the page-flipping sequence - * and indeed performing the enable as a co-routine and not - * waiting synchronously upon the vblank. - * - * WaFbcWaitForVBlankBeforeEnable:ilk,snb - */ - schedule_delayed_work(&work->work, msecs_to_jiffies(50)); + schedule_work(&work->work); } static void __intel_fbc_deactivate(struct drm_i915_private *dev_priv) @@ -1039,7 +1025,7 @@ void intel_fbc_flip_prepare(struct drm_i915_private *dev_priv, fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); if (fbc_bits & frontbuffer_bits) dev_priv->fbc.flip_prepare(dev_priv); - } else if (dev_priv->fbc.fbc_work) { + } else if (dev_priv->fbc.work.scheduled) { fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe); if (fbc_bits & frontbuffer_bits) __intel_fbc_deactivate(dev_priv); @@ -1195,9 +1181,11 @@ void intel_fbc_init(struct drm_i915_private *dev_priv) { enum pipe pipe; + INIT_WORK(&dev_priv->fbc.work.work, intel_fbc_work_fn); mutex_init(&dev_priv->fbc.lock); dev_priv->fbc.enabled = false; dev_priv->fbc.active = false; + dev_priv->fbc.work.scheduled = false; if (!HAS_FBC(dev_priv)) { dev_priv->fbc.no_fbc_reason = "unsupported by this chipset";
This was already on my TODO list, and was requested both by Chris and Ville, for different reasons. The advantages are avoiding a frequent malloc/free pair, and the locality of having the work structure embedded in dev_priv. The maximum used memory is also smaller since previously we could have multiple allocated intel_fbc_work structs at the same time, and now we'll always have a single one - the one embedded on dev_priv. Of course, we're now using a little more memory on the cases where there's nothing scheduled. The biggest challenge here is to keep everything synchronized the way it was before. Currently, when we try to activate FBC, we allocate a new intel_fbc_work structure. Then later when we conclude we must delay the FBC activation a little more, we allocate a new intel_fbc_work struct, and then adjust dev_priv->fbc.fbc_work to point to the new struct. So when the old work runs - at intel_fbc_work_fn() - it will check that dev_priv->fbc.fbc_work points to something else, so it does nothing. Everything is also protected by fbc.lock. Just cancelling the old delayed work doesn't work because we might just cancel it after the work function already started to run, but while it is still waiting to grab fbc.lock. That's why we use the "dev_priv->fbc.fbc_work == work" check described in the paragraph above. So now that we have a single work struct we have to introduce a new way to synchronize everything. So we're making the work function a normal work instead of a delayed work, and it will be responsible for sleeping the appropriate amount of time itself. This way, after it wakes up it can grab the lock, ask "were we delayed or cancelled?" and then go back to sleep, enable FBC or give up. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 6 ++- drivers/gpu/drm/i915/intel_fbc.c | 108 +++++++++++++++++---------------------- 2 files changed, 52 insertions(+), 62 deletions(-)