| Message ID | 20241021222744.294371-4-gustavo.sousa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dmc_wl: Fixes and enablement for Xe3_LPD | expand |
On Mon, 2024-10-21 at 19:27 -0300, Gustavo Sousa wrote: > When the DMC wakelock refcount reaches zero, we know that there are no > users and that we can do the actual release operation on the hardware, > which is queued with a delayed work. The idea of the delayed work is to > avoid performing the release if a new lock user appears (i.e. refcount > gets incremented) in a very short period of time. > > Based on the above, the release work should bail out if refcount is > non-zero (meaning new lock users appeared in the meantime), but our > current code actually does the opposite: it bails when refcount is zero. > That means that the wakelock is not released when it should be; and > that, when the work is not canceled in time, it ends up being releasing > when it should not. > > Fix that by inverting the condition. > > Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> > --- > drivers/gpu/drm/i915/display/intel_dmc_wl.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c > index 8056a3c8666c..c298aef89449 100644 > --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c > +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c > @@ -72,8 +72,11 @@ static void intel_dmc_wl_work(struct work_struct *work) > > spin_lock_irqsave(&wl->lock, flags); > > - /* Bail out if refcount reached zero while waiting for the spinlock */ > - if (!refcount_read(&wl->refcount)) > + /* > + * Bail out if refcount became non-zero while waiting for the spinlock, > + * meaning that the lock is now taken again. > + */ > + if (refcount_read(&wl->refcount)) > goto out_unlock; > > __intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0); Oh, this was probably hard to catch. I think this is a buggy leftover from my earlier implementations where I was decreasing the refcount only here. Reviewed-by: Luca Coelho <luciano.coelho@intel.com> -- Cheers, Luca.
diff --git a/drivers/gpu/drm/i915/display/intel_dmc_wl.c b/drivers/gpu/drm/i915/display/intel_dmc_wl.c index 8056a3c8666c..c298aef89449 100644 --- a/drivers/gpu/drm/i915/display/intel_dmc_wl.c +++ b/drivers/gpu/drm/i915/display/intel_dmc_wl.c @@ -72,8 +72,11 @@ static void intel_dmc_wl_work(struct work_struct *work) spin_lock_irqsave(&wl->lock, flags); - /* Bail out if refcount reached zero while waiting for the spinlock */ - if (!refcount_read(&wl->refcount)) + /* + * Bail out if refcount became non-zero while waiting for the spinlock, + * meaning that the lock is now taken again. + */ + if (refcount_read(&wl->refcount)) goto out_unlock; __intel_de_rmw_nowl(display, DMC_WAKELOCK1_CTL, DMC_WAKELOCK_CTL_REQ, 0);
When the DMC wakelock refcount reaches zero, we know that there are no users and that we can do the actual release operation on the hardware, which is queued with a delayed work. The idea of the delayed work is to avoid performing the release if a new lock user appears (i.e. refcount gets incremented) in a very short period of time. Based on the above, the release work should bail out if refcount is non-zero (meaning new lock users appeared in the meantime), but our current code actually does the opposite: it bails when refcount is zero. That means that the wakelock is not released when it should be; and that, when the work is not canceled in time, it ends up being releasing when it should not. Fix that by inverting the condition. Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com> --- drivers/gpu/drm/i915/display/intel_dmc_wl.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)