| Message ID | 20240325175738.3440-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Pre-populate the cursor physical dma address | expand |
On Mon, Mar 25, 2024 at 07:57:38PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Calling i915_gem_object_get_dma_address() from the vblank > evade critical section triggers might_sleep(). > > While we know that we've already pinned the framebuffer > and thus i915_gem_object_get_dma_address() will in fact > not sleep in this case, it seems reasonable to keep the > unconditional might_sleep() for maximum coverage. > > So let's instead pre-populate the dma address during > fb pinning, which all happens before we enter the > vblank evade critical section. > > We can use u32 for the dma address as this class of > hardware doesn't support >32bit addresses. > > Cc: stable@vger.kernel.org > Fixes: 0225a90981c8 ("drm/i915: Make cursor plane registers unlocked") > Link: https://lore.kernel.org/intel-gfx/20240227100342.GAZd2zfmYcPS_SndtO@fat_crate.local/ > Reported-by: Borislav Petkov <bp@alien8.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks for the fix - splat is gone. Tested-by: Borislav Petkov (AMD) <bp@alien8.de>
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > Syrjala > Sent: Monday, March 25, 2024 11:28 PM > To: intel-gfx@lists.freedesktop.org > Cc: stable@vger.kernel.org; Borislav Petkov <bp@alien8.de> > Subject: [PATCH] drm/i915: Pre-populate the cursor physical dma address > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Calling i915_gem_object_get_dma_address() from the vblank evade critical > section triggers might_sleep(). > > While we know that we've already pinned the framebuffer and thus > i915_gem_object_get_dma_address() will in fact not sleep in this case, it > seems reasonable to keep the unconditional might_sleep() for maximum > coverage. > > So let's instead pre-populate the dma address during fb pinning, which all > happens before we enter the vblank evade critical section. > > We can use u32 for the dma address as this class of hardware doesn't > support >32bit addresses. > > Cc: stable@vger.kernel.org > Fixes: 0225a90981c8 ("drm/i915: Make cursor plane registers unlocked") > Link: https://lore.kernel.org/intel- > gfx/20240227100342.GAZd2zfmYcPS_SndtO@fat_crate.local/ Nit. This could be changed to Closes and moved after Reported-by to keep checkpatch happy but otherwise, LGTM. Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > Reported-by: Borislav Petkov <bp@alien8.de> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_cursor.c | 4 +--- > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > drivers/gpu/drm/i915/display/intel_fb_pin.c | 10 ++++++++++ > 3 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > b/drivers/gpu/drm/i915/display/intel_cursor.c > index f8b33999d43f..0d3da55e1c24 100644 > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > @@ -36,12 +36,10 @@ static u32 intel_cursor_base(const struct > intel_plane_state *plane_state) { > struct drm_i915_private *dev_priv = > to_i915(plane_state->uapi.plane->dev); > - const struct drm_framebuffer *fb = plane_state->hw.fb; > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > u32 base; > > if (DISPLAY_INFO(dev_priv)->cursor_needs_physical) > - base = i915_gem_object_get_dma_address(obj, 0); > + base = plane_state->phys_dma_addr; > else > base = intel_plane_ggtt_offset(plane_state); > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index 8a35fb6b2ade..68f26a33870b 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -728,6 +728,7 @@ struct intel_plane_state { #define PLANE_HAS_FENCE > BIT(0) > > struct intel_fb_view view; > + u32 phys_dma_addr; /* for cursor_needs_physical */ > > /* Plane pxp decryption state */ > bool decrypt; > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c > b/drivers/gpu/drm/i915/display/intel_fb_pin.c > index 7b42aef37d2f..b6df9baf481b 100644 > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > @@ -255,6 +255,16 @@ int intel_plane_pin_fb(struct intel_plane_state > *plane_state) > return PTR_ERR(vma); > > plane_state->ggtt_vma = vma; > + > + /* > + * Pre-populate the dma address before we enter the vblank > + * evade critical section as > i915_gem_object_get_dma_address() > + * will trigger might_sleep() even if it won't actually sleep, > + * which is the case when the fb has already been pinned. > + */ > + if (phys_cursor) > + plane_state->phys_dma_addr = > + > i915_gem_object_get_dma_address(intel_fb_obj(fb), 0); > } else { > struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); > > -- > 2.43.2
On Tue, Mar 26, 2024 at 04:57:57AM +0000, Borah, Chaitanya Kumar wrote: > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > > Syrjala > > Sent: Monday, March 25, 2024 11:28 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: stable@vger.kernel.org; Borislav Petkov <bp@alien8.de> > > Subject: [PATCH] drm/i915: Pre-populate the cursor physical dma address > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Calling i915_gem_object_get_dma_address() from the vblank evade critical > > section triggers might_sleep(). > > > > While we know that we've already pinned the framebuffer and thus > > i915_gem_object_get_dma_address() will in fact not sleep in this case, it > > seems reasonable to keep the unconditional might_sleep() for maximum > > coverage. > > > > So let's instead pre-populate the dma address during fb pinning, which all > > happens before we enter the vblank evade critical section. > > > > We can use u32 for the dma address as this class of hardware doesn't > > support >32bit addresses. > > > > Cc: stable@vger.kernel.org > > Fixes: 0225a90981c8 ("drm/i915: Make cursor plane registers unlocked") > > Link: https://lore.kernel.org/intel- > > gfx/20240227100342.GAZd2zfmYcPS_SndtO@fat_crate.local/ > > Nit. This could be changed to Closes and moved after Reported-by to keep checkpatch happy but otherwise, LGTM. It's not a gitlab link, so Closes doesn't seem appropriate. > > Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> Thanks. > > > Reported-by: Borislav Petkov <bp@alien8.de> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_cursor.c | 4 +--- > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > drivers/gpu/drm/i915/display/intel_fb_pin.c | 10 ++++++++++ > > 3 files changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > > b/drivers/gpu/drm/i915/display/intel_cursor.c > > index f8b33999d43f..0d3da55e1c24 100644 > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > > @@ -36,12 +36,10 @@ static u32 intel_cursor_base(const struct > > intel_plane_state *plane_state) { > > struct drm_i915_private *dev_priv = > > to_i915(plane_state->uapi.plane->dev); > > - const struct drm_framebuffer *fb = plane_state->hw.fb; > > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > > u32 base; > > > > if (DISPLAY_INFO(dev_priv)->cursor_needs_physical) > > - base = i915_gem_object_get_dma_address(obj, 0); > > + base = plane_state->phys_dma_addr; > > else > > base = intel_plane_ggtt_offset(plane_state); > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index 8a35fb6b2ade..68f26a33870b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -728,6 +728,7 @@ struct intel_plane_state { #define PLANE_HAS_FENCE > > BIT(0) > > > > struct intel_fb_view view; > > + u32 phys_dma_addr; /* for cursor_needs_physical */ > > > > /* Plane pxp decryption state */ > > bool decrypt; > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c > > b/drivers/gpu/drm/i915/display/intel_fb_pin.c > > index 7b42aef37d2f..b6df9baf481b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > > @@ -255,6 +255,16 @@ int intel_plane_pin_fb(struct intel_plane_state > > *plane_state) > > return PTR_ERR(vma); > > > > plane_state->ggtt_vma = vma; > > + > > + /* > > + * Pre-populate the dma address before we enter the vblank > > + * evade critical section as > > i915_gem_object_get_dma_address() > > + * will trigger might_sleep() even if it won't actually sleep, > > + * which is the case when the fb has already been pinned. > > + */ > > + if (phys_cursor) > > + plane_state->phys_dma_addr = > > + > > i915_gem_object_get_dma_address(intel_fb_obj(fb), 0); > > } else { > > struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); > > > > -- > > 2.43.2 >
On Tue, Mar 26, 2024 at 01:36:29PM +0200, Ville Syrjälä wrote: > On Tue, Mar 26, 2024 at 04:57:57AM +0000, Borah, Chaitanya Kumar wrote: > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville > > > Syrjala > > > Sent: Monday, March 25, 2024 11:28 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: stable@vger.kernel.org; Borislav Petkov <bp@alien8.de> > > > Subject: [PATCH] drm/i915: Pre-populate the cursor physical dma address > > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > Calling i915_gem_object_get_dma_address() from the vblank evade critical > > > section triggers might_sleep(). > > > > > > While we know that we've already pinned the framebuffer and thus > > > i915_gem_object_get_dma_address() will in fact not sleep in this case, it > > > seems reasonable to keep the unconditional might_sleep() for maximum > > > coverage. > > > > > > So let's instead pre-populate the dma address during fb pinning, which all > > > happens before we enter the vblank evade critical section. > > > > > > We can use u32 for the dma address as this class of hardware doesn't > > > support >32bit addresses. > > > > > > Cc: stable@vger.kernel.org > > > Fixes: 0225a90981c8 ("drm/i915: Make cursor plane registers unlocked") > > > Link: https://lore.kernel.org/intel- > > > gfx/20240227100342.GAZd2zfmYcPS_SndtO@fat_crate.local/ > > > > Nit. This could be changed to Closes and moved after Reported-by to keep checkpatch happy but otherwise, LGTM. > > It's not a gitlab link, so Closes doesn't seem appropriate. Hmm. Seems people have started to used Closes: for non-gitlab stuff as well. I suppose it's OK then. > > > > > Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > > Thanks. > > > > > > Reported-by: Borislav Petkov <bp@alien8.de> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_cursor.c | 4 +--- > > > drivers/gpu/drm/i915/display/intel_display_types.h | 1 + > > > drivers/gpu/drm/i915/display/intel_fb_pin.c | 10 ++++++++++ > > > 3 files changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > > > b/drivers/gpu/drm/i915/display/intel_cursor.c > > > index f8b33999d43f..0d3da55e1c24 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > > > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c > > > @@ -36,12 +36,10 @@ static u32 intel_cursor_base(const struct > > > intel_plane_state *plane_state) { > > > struct drm_i915_private *dev_priv = > > > to_i915(plane_state->uapi.plane->dev); > > > - const struct drm_framebuffer *fb = plane_state->hw.fb; > > > - struct drm_i915_gem_object *obj = intel_fb_obj(fb); > > > u32 base; > > > > > > if (DISPLAY_INFO(dev_priv)->cursor_needs_physical) > > > - base = i915_gem_object_get_dma_address(obj, 0); > > > + base = plane_state->phys_dma_addr; > > > else > > > base = intel_plane_ggtt_offset(plane_state); > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 8a35fb6b2ade..68f26a33870b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -728,6 +728,7 @@ struct intel_plane_state { #define PLANE_HAS_FENCE > > > BIT(0) > > > > > > struct intel_fb_view view; > > > + u32 phys_dma_addr; /* for cursor_needs_physical */ > > > > > > /* Plane pxp decryption state */ > > > bool decrypt; > > > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c > > > b/drivers/gpu/drm/i915/display/intel_fb_pin.c > > > index 7b42aef37d2f..b6df9baf481b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c > > > @@ -255,6 +255,16 @@ int intel_plane_pin_fb(struct intel_plane_state > > > *plane_state) > > > return PTR_ERR(vma); > > > > > > plane_state->ggtt_vma = vma; > > > + > > > + /* > > > + * Pre-populate the dma address before we enter the vblank > > > + * evade critical section as > > > i915_gem_object_get_dma_address() > > > + * will trigger might_sleep() even if it won't actually sleep, > > > + * which is the case when the fb has already been pinned. > > > + */ > > > + if (phys_cursor) > > > + plane_state->phys_dma_addr = > > > + > > > i915_gem_object_get_dma_address(intel_fb_obj(fb), 0); > > > } else { > > > struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb); > > > > > > -- > > > 2.43.2 > > > > -- > Ville Syrjälä > Intel
On Mon, Mar 25, 2024 at 07:21:35PM +0100, Borislav Petkov wrote: > On Mon, Mar 25, 2024 at 07:57:38PM +0200, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Calling i915_gem_object_get_dma_address() from the vblank > > evade critical section triggers might_sleep(). > > > > While we know that we've already pinned the framebuffer > > and thus i915_gem_object_get_dma_address() will in fact > > not sleep in this case, it seems reasonable to keep the > > unconditional might_sleep() for maximum coverage. > > > > So let's instead pre-populate the dma address during > > fb pinning, which all happens before we enter the > > vblank evade critical section. > > > > We can use u32 for the dma address as this class of > > hardware doesn't support >32bit addresses. > > > > Cc: stable@vger.kernel.org > > Fixes: 0225a90981c8 ("drm/i915: Make cursor plane registers unlocked") > > Link: https://lore.kernel.org/intel-gfx/20240227100342.GAZd2zfmYcPS_SndtO@fat_crate.local/ > > Reported-by: Borislav Petkov <bp@alien8.de> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Thanks for the fix - splat is gone. > > Tested-by: Borislav Petkov (AMD) <bp@alien8.de> Thanks. Pushed to drm-intel-next.
diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c index f8b33999d43f..0d3da55e1c24 100644 --- a/drivers/gpu/drm/i915/display/intel_cursor.c +++ b/drivers/gpu/drm/i915/display/intel_cursor.c @@ -36,12 +36,10 @@ static u32 intel_cursor_base(const struct intel_plane_state *plane_state) { struct drm_i915_private *dev_priv = to_i915(plane_state->uapi.plane->dev); - const struct drm_framebuffer *fb = plane_state->hw.fb; - struct drm_i915_gem_object *obj = intel_fb_obj(fb); u32 base; if (DISPLAY_INFO(dev_priv)->cursor_needs_physical) - base = i915_gem_object_get_dma_address(obj, 0); + base = plane_state->phys_dma_addr; else base = intel_plane_ggtt_offset(plane_state); diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 8a35fb6b2ade..68f26a33870b 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -728,6 +728,7 @@ struct intel_plane_state { #define PLANE_HAS_FENCE BIT(0) struct intel_fb_view view; + u32 phys_dma_addr; /* for cursor_needs_physical */ /* Plane pxp decryption state */ bool decrypt; diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c b/drivers/gpu/drm/i915/display/intel_fb_pin.c index 7b42aef37d2f..b6df9baf481b 100644 --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c @@ -255,6 +255,16 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state) return PTR_ERR(vma); plane_state->ggtt_vma = vma; + + /* + * Pre-populate the dma address before we enter the vblank + * evade critical section as i915_gem_object_get_dma_address() + * will trigger might_sleep() even if it won't actually sleep, + * which is the case when the fb has already been pinned. + */ + if (phys_cursor) + plane_state->phys_dma_addr = + i915_gem_object_get_dma_address(intel_fb_obj(fb), 0); } else { struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);