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@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville
> > > Syrjala
> > > Sent: Monday, March 25, 2024 11:28 PM
> > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: stable@xxxxxxxxxxxxxxx; Borislav Petkov <bp@xxxxxxxxx>
> > > Subject: [PATCH] drm/i915: Pre-populate the cursor physical dma address
> > >
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxx>
>
> Thanks.
>
> >
> > > Reported-by: Borislav Petkov <bp@xxxxxxxxx>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
--
Ville Syrjälä
Intel
