| Message ID | dec5992d78db5bc556600c64ce72aa9b19c96c77.1726658138.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915: wakeref fixes and improvements | expand |
On 18/09/2024 12:17, Jani Nikula wrote: > CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND is an int, defaulting to 250. When > the wakeref is non-zero, it's either -1 or a dynamically allocated > pointer, depending on CONFIG_DRM_I915_DEBUG_RUNTIME_PM. It's likely that > the code works by coincidence with the bitwise AND, but with > CONFIG_DRM_I915_DEBUG_RUNTIME_PM=y, there's the off chance that the > condition evaluates to false, and intel_wakeref_auto() doesn't get > called. Switch to the intended logical AND. > > Fixes: ad74457a6b5a ("drm/i915/dgfx: Release mmap on rpm suspend") > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: <stable@vger.kernel.org> # v6.1+ > Signed-off-by: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Hi Jani, On Wed, Sep 18, 2024 at 02:17:44PM +0300, Jani Nikula wrote: > CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND is an int, defaulting to 250. When > the wakeref is non-zero, it's either -1 or a dynamically allocated > pointer, depending on CONFIG_DRM_I915_DEBUG_RUNTIME_PM. It's likely that > the code works by coincidence with the bitwise AND, but with > CONFIG_DRM_I915_DEBUG_RUNTIME_PM=y, there's the off chance that the > condition evaluates to false, and intel_wakeref_auto() doesn't get > called. Switch to the intended logical AND. > > Fixes: ad74457a6b5a ("drm/i915/dgfx: Release mmap on rpm suspend") > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: <stable@vger.kernel.org> # v6.1+ > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 5c72462d1f57..c157ade48c39 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -1131,7 +1131,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf) > GEM_WARN_ON(!i915_ttm_cpu_maps_iomem(bo->resource)); > } > > - if (wakeref & CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > + if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) ops! Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com> Andi > intel_wakeref_auto(&to_i915(obj->base.dev)->runtime_pm.userfault_wakeref, > msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)); > > -- > 2.39.2
On Wed, Sep 18, 2024 at 02:17:44PM +0300, Jani Nikula wrote: > CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND is an int, defaulting to 250. When > the wakeref is non-zero, it's either -1 or a dynamically allocated > pointer, depending on CONFIG_DRM_I915_DEBUG_RUNTIME_PM. It's likely that > the code works by coincidence with the bitwise AND, but with > CONFIG_DRM_I915_DEBUG_RUNTIME_PM=y, there's the off chance that the > condition evaluates to false, and intel_wakeref_auto() doesn't get > called. Switch to the intended logical AND. > > Fixes: ad74457a6b5a ("drm/i915/dgfx: Release mmap on rpm suspend") > Cc: Matthew Auld <matthew.auld@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Cc: Andi Shyti <andi.shyti@linux.intel.com> > Cc: <stable@vger.kernel.org> # v6.1+ > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > --- > drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > index 5c72462d1f57..c157ade48c39 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c > @@ -1131,7 +1131,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf) > GEM_WARN_ON(!i915_ttm_cpu_maps_iomem(bo->resource)); > } > > - if (wakeref & CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > + if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) This is going to result in a clang warning: drivers/gpu/drm/i915/gem/i915_gem_ttm.c:1134:14: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand] 1134 | if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/i915/gem/i915_gem_ttm.c:1134:14: note: use '&' for a bitwise operation 1134 | if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) | ^~ | & drivers/gpu/drm/i915/gem/i915_gem_ttm.c:1134:14: note: remove constant to silence this warning 1134 | if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 error generated. Consider adding the explicit '!= 0' to make it clear this should be a boolean expression. Cheers, Nathan > intel_wakeref_auto(&to_i915(obj->base.dev)->runtime_pm.userfault_wakeref, > msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)); > > -- > 2.39.2 >
On Wed, 18 Sep 2024, Nathan Chancellor <nathan@kernel.org> wrote: > On Wed, Sep 18, 2024 at 02:17:44PM +0300, Jani Nikula wrote: >> CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND is an int, defaulting to 250. When >> the wakeref is non-zero, it's either -1 or a dynamically allocated >> pointer, depending on CONFIG_DRM_I915_DEBUG_RUNTIME_PM. It's likely that >> the code works by coincidence with the bitwise AND, but with >> CONFIG_DRM_I915_DEBUG_RUNTIME_PM=y, there's the off chance that the >> condition evaluates to false, and intel_wakeref_auto() doesn't get >> called. Switch to the intended logical AND. >> >> Fixes: ad74457a6b5a ("drm/i915/dgfx: Release mmap on rpm suspend") >> Cc: Matthew Auld <matthew.auld@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Anshuman Gupta <anshuman.gupta@intel.com> >> Cc: Andi Shyti <andi.shyti@linux.intel.com> >> Cc: <stable@vger.kernel.org> # v6.1+ >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> --- >> drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> index 5c72462d1f57..c157ade48c39 100644 >> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c >> @@ -1131,7 +1131,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf) >> GEM_WARN_ON(!i915_ttm_cpu_maps_iomem(bo->resource)); >> } >> >> - if (wakeref & CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) >> + if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > > This is going to result in a clang warning: > > drivers/gpu/drm/i915/gem/i915_gem_ttm.c:1134:14: error: use of logical '&&' with constant operand [-Werror,-Wconstant-logical-operand] > 1134 | if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/gpu/drm/i915/gem/i915_gem_ttm.c:1134:14: note: use '&' for a bitwise operation > 1134 | if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > | ^~ > | & > drivers/gpu/drm/i915/gem/i915_gem_ttm.c:1134:14: note: remove constant to silence this warning > 1134 | if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1 error generated. > > Consider adding the explicit '!= 0' to make it clear this should be a > boolean expression. Thanks, will be fixed in v2. BR, Jani. > > Cheers, > Nathan > >> intel_wakeref_auto(&to_i915(obj->base.dev)->runtime_pm.userfault_wakeref, >> msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND)); >> >> -- >> 2.39.2 >>
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 5c72462d1f57..c157ade48c39 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -1131,7 +1131,7 @@ static vm_fault_t vm_fault_ttm(struct vm_fault *vmf) GEM_WARN_ON(!i915_ttm_cpu_maps_iomem(bo->resource)); } - if (wakeref & CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) + if (wakeref && CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND) intel_wakeref_auto(&to_i915(obj->base.dev)->runtime_pm.userfault_wakeref, msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND is an int, defaulting to 250. When the wakeref is non-zero, it's either -1 or a dynamically allocated pointer, depending on CONFIG_DRM_I915_DEBUG_RUNTIME_PM. It's likely that the code works by coincidence with the bitwise AND, but with CONFIG_DRM_I915_DEBUG_RUNTIME_PM=y, there's the off chance that the condition evaluates to false, and intel_wakeref_auto() doesn't get called. Switch to the intended logical AND. Fixes: ad74457a6b5a ("drm/i915/dgfx: Release mmap on rpm suspend") Cc: Matthew Auld <matthew.auld@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Anshuman Gupta <anshuman.gupta@intel.com> Cc: Andi Shyti <andi.shyti@linux.intel.com> Cc: <stable@vger.kernel.org> # v6.1+ Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)