From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org, intel-xe@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/i915: Track fence region ID in plane state
Date: Wed, 1 Apr 2026 17:29:01 +0300 [thread overview]
Message-ID: <ac0rrbHIe_R3b5Ck@intel.com> (raw)
In-Reply-To: <4dee82289c67c00562cbb4fcc8a16c8b3c7a6e4c@intel.com>
On Wed, Apr 01, 2026 at 04:10:16PM +0300, Jani Nikula wrote:
> On Tue, 31 Mar 2026, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Get rid of the needlessly complicated PLANE_HAS_FENCE +
> > intel_parent_vma_fence_id() dance by simply tracking the
> > fence_id directly in the plane state.
>
> This is good cleanup, but I still dread what to do about the whole
> {intel,xe}_fb_pin.[ch] interface. Needs to be moved to the parent
> interface, but there's just too much direct display structure poking
> from the i915 and xe cores there. Ugh.
Hmm. I'll put on my thinking cap and ponder about it a bit...
>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > .../drm/i915/display/intel_display_types.h | 5 +--
> > drivers/gpu/drm/i915/display/intel_fb_pin.c | 33 ++++++++++---------
> > drivers/gpu/drm/i915/display/intel_fb_pin.h | 5 ++-
> > drivers/gpu/drm/i915/display/intel_fbc.c | 11 ++-----
> > drivers/gpu/drm/i915/display/intel_fbdev.c | 14 ++++----
> > drivers/gpu/drm/i915/display/intel_plane.c | 3 +-
> > drivers/gpu/drm/i915/i915_initial_plane.c | 2 +-
> > drivers/gpu/drm/xe/display/xe_fb_pin.c | 8 ++---
> > drivers/gpu/drm/xe/display/xe_initial_plane.c | 2 +-
> > 9 files changed, 42 insertions(+), 41 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index e2496db1642a..73eb4f38620c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -683,14 +683,15 @@ struct intel_plane_state {
> >
> > struct i915_vma *ggtt_vma;
> > struct i915_vma *dpt_vma;
> > - unsigned long flags;
> > -#define PLANE_HAS_FENCE BIT(0)
> >
> > struct intel_fb_view view;
> >
> > /* for legacy cursor fb unpin */
> > struct drm_vblank_work unpin_work;
> >
> > + /* fenced region ID (-1 if none) */
> > + s8 fence_id;
>
> I guess I would've made this an int for consistency with all the
> parameters and local variables.
I considered it after the fact. But FBC had the s8, and I found it
really hard to give up on that nice little hole I found :/
>
> > +
> > /* 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 738d77a1468a..0b8b057647af 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
> > @@ -26,7 +26,6 @@ static struct i915_vma *
> > intel_fb_pin_to_dpt(const struct drm_framebuffer *fb,
> > const struct i915_gtt_view *view,
> > unsigned int alignment,
> > - unsigned long *out_flags,
> > struct intel_dpt *dpt)
> > {
> > struct drm_device *dev = fb->dev;
> > @@ -115,8 +114,7 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> > unsigned int alignment,
> > unsigned int phys_alignment,
> > unsigned int vtd_guard,
> > - bool uses_fence,
> > - unsigned long *out_flags)
> > + int *out_fence_id)
> > {
> > struct drm_device *dev = fb->dev;
> > struct intel_display *display = to_intel_display(dev);
> > @@ -177,7 +175,10 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> > goto err_unpin;
> > }
> >
> > - if (uses_fence && i915_vma_is_map_and_fenceable(vma)) {
> > + if (out_fence_id)
> > + *out_fence_id = -1;
> > +
> > + if (out_fence_id && i915_vma_is_map_and_fenceable(vma)) {
> > /*
> > * Install a fence for tiled scan-out. Pre-i965 always needs a
> > * fence, whereas 965+ only requires a fence if using
> > @@ -203,7 +204,7 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> > ret = 0;
> >
> > if (vma->fence)
> > - *out_flags |= PLANE_HAS_FENCE;
> > + *out_fence_id |= vma->fence->id;
> > }
> >
> > i915_vma_get(vma);
> > @@ -225,9 +226,9 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> > return vma;
> > }
> >
> > -void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
> > +void intel_fb_unpin_vma(struct i915_vma *vma, int fence_id)
> > {
> > - if (flags & PLANE_HAS_FENCE)
> > + if (fence_id >= 0)
> > i915_vma_unpin_fence(vma);
> > i915_vma_unpin(vma);
> > i915_vma_put(vma);
> > @@ -271,17 +272,18 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state,
> > struct i915_vma *vma;
> >
> > if (!intel_fb_uses_dpt(&fb->base)) {
> > + int fence_id = -1;
> > +
> > vma = intel_fb_pin_to_ggtt(&fb->base, &plane_state->view.gtt,
> > intel_plane_fb_min_alignment(plane_state),
> > intel_plane_fb_min_phys_alignment(plane_state),
> > intel_plane_fb_vtd_guard(plane_state),
> > - intel_plane_uses_fence(plane_state),
> > - &plane_state->flags);
> > + intel_plane_uses_fence(plane_state) ? &fence_id : NULL);
>
> E.g. here you could pass &plane_state->fence_id direcly if it was an int
> and ditch the local variable.
>
> > if (IS_ERR(vma))
> > return PTR_ERR(vma);
> >
> > plane_state->ggtt_vma = vma;
> > -
> > + plane_state->fence_id = fence_id;
> > } else {
> > unsigned int alignment = intel_plane_fb_min_alignment(plane_state);
> >
> > @@ -292,8 +294,7 @@ int intel_plane_pin_fb(struct intel_plane_state *plane_state,
> > plane_state->ggtt_vma = vma;
> >
> > vma = intel_fb_pin_to_dpt(&fb->base, &plane_state->view.gtt,
> > - alignment, &plane_state->flags,
> > - fb->dpt);
> > + alignment, fb->dpt);
> > if (IS_ERR(vma)) {
> > i915_dpt_unpin_from_ggtt(fb->dpt);
> > plane_state->ggtt_vma = NULL;
> > @@ -338,12 +339,14 @@ void intel_plane_unpin_fb(struct intel_plane_state *old_plane_state)
> >
> > if (!intel_fb_uses_dpt(&fb->base)) {
> > vma = fetch_and_zero(&old_plane_state->ggtt_vma);
> > - if (vma)
> > - intel_fb_unpin_vma(vma, old_plane_state->flags);
> > + if (vma) {
> > + intel_fb_unpin_vma(vma, old_plane_state->fence_id);
> > + old_plane_state->fence_id = -1;
> > + }
> > } else {
> > vma = fetch_and_zero(&old_plane_state->dpt_vma);
> > if (vma)
> > - intel_fb_unpin_vma(vma, old_plane_state->flags);
> > + intel_fb_unpin_vma(vma, -1);
> >
> > vma = fetch_and_zero(&old_plane_state->ggtt_vma);
> > if (vma)
> > diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.h b/drivers/gpu/drm/i915/display/intel_fb_pin.h
> > index 81ab79da1af7..2eca42b74c4a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fb_pin.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.h
> > @@ -20,10 +20,9 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> > unsigned int alignment,
> > unsigned int phys_alignment,
> > unsigned int vtd_guard,
> > - bool uses_fence,
> > - unsigned long *out_flags);
> > + int *out_fence_id);
> >
> > -void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags);
> > +void intel_fb_unpin_vma(struct i915_vma *vma, int fence_id);
> >
> > int intel_plane_pin_fb(struct intel_plane_state *new_plane_state,
> > const struct intel_plane_state *old_plane_state);
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index ea0ce00c8474..677ac5be749b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -1458,13 +1458,10 @@ static void intel_fbc_update_state(struct intel_atomic_state *state,
> >
> > fbc_state->fence_y_offset = intel_plane_fence_y_offset(plane_state);
> >
> > - drm_WARN_ON(display->drm, plane_state->flags & PLANE_HAS_FENCE &&
> > + drm_WARN_ON(display->drm, plane_state->fence_id >= 0 &&
> > !intel_fbc_has_fences(display));
> >
> > - if (plane_state->flags & PLANE_HAS_FENCE)
> > - fbc_state->fence_id = intel_parent_vma_fence_id(display, plane_state->ggtt_vma);
> > - else
> > - fbc_state->fence_id = -1;
> > + fbc_state->fence_id = plane_state->fence_id;
> >
> > fbc_state->cfb_stride = intel_fbc_cfb_stride(plane_state);
> > fbc_state->cfb_size = intel_fbc_cfb_size(plane_state);
> > @@ -1487,9 +1484,7 @@ static bool intel_fbc_is_fence_ok(const struct intel_plane_state *plane_state)
> > * so have no fence associated with it) due to aperture constraints
> > * at the time of pinning.
> > */
> > - return DISPLAY_VER(display) >= 9 ||
> > - (plane_state->flags & PLANE_HAS_FENCE &&
> > - intel_parent_vma_fence_id(display, plane_state->ggtt_vma) != -1);
> > + return DISPLAY_VER(display) >= 9 || plane_state->fence_id >= 0;
> > }
> >
> > static bool intel_fbc_is_cfb_ok(const struct intel_plane_state *plane_state)
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > index bdaaf3edba0c..0d7be5186393 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> > @@ -60,7 +60,7 @@
> > struct intel_fbdev {
> > struct intel_framebuffer *fb;
> > struct i915_vma *vma;
> > - unsigned long vma_flags;
> > + s8 fence_id;
>
> Ditto.
>
> Regardless,
>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>
>
> > };
> >
> > static struct intel_fbdev *to_intel_fbdev(struct drm_fb_helper *fb_helper)
> > @@ -141,7 +141,7 @@ static void intel_fbdev_fb_destroy(struct fb_info *info)
> > * the info->screen_base mmaping. Leaking the VMA is simpler than
> > * trying to rectify all the possible error paths leading here.
> > */
> > - intel_fb_unpin_vma(ifbdev->vma, ifbdev->vma_flags);
> > + intel_fb_unpin_vma(ifbdev->vma, ifbdev->fence_id);
> > drm_framebuffer_remove(fb_helper->fb);
> >
> > drm_client_release(&fb_helper->client);
> > @@ -269,9 +269,9 @@ int intel_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
> > struct fb_info *info = helper->info;
> > struct ref_tracker *wakeref;
> > struct i915_vma *vma;
> > - unsigned long flags = 0;
> > bool prealloc = false;
> > struct drm_gem_object *obj;
> > + int fence_id = -1;
> > int ret;
> >
> > ifbdev->fb = NULL;
> > @@ -314,7 +314,7 @@ int intel_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
> > fb->min_alignment, 0,
> > intel_fb_view_vtd_guard(&fb->base, &fb->normal_view,
> > DRM_MODE_ROTATE_0),
> > - false, &flags);
> > + &fence_id);
> > if (IS_ERR(vma)) {
> > ret = PTR_ERR(vma);
> > goto out_unlock;
> > @@ -345,14 +345,14 @@ int intel_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
> > drm_dbg_kms(display->drm, "allocated %dx%d fb\n", fb->base.width, fb->base.height);
> > ifbdev->fb = fb;
> > ifbdev->vma = vma;
> > - ifbdev->vma_flags = flags;
> > + ifbdev->fence_id = fence_id;
> >
> > intel_display_rpm_put(display, wakeref);
> >
> > return 0;
> >
> > out_unpin:
> > - intel_fb_unpin_vma(vma, flags);
> > + intel_fb_unpin_vma(vma, fence_id);
> > out_unlock:
> > intel_display_rpm_put(display, wakeref);
> >
> > @@ -539,6 +539,8 @@ void intel_fbdev_setup(struct intel_display *display)
> > if (!ifbdev)
> > return;
> >
> > + ifbdev->fence_id = -1;
> > +
> > display->fbdev.fbdev = ifbdev;
> > if (intel_fbdev_init_bios(display, ifbdev))
> > preferred_bpp = intel_fbdev_color_mode(ifbdev->fb->base.format);
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane.c b/drivers/gpu/drm/i915/display/intel_plane.c
> > index 5390ceb21ca4..f15dd9e91243 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane.c
> > @@ -70,6 +70,7 @@ static void intel_plane_state_reset(struct intel_plane_state *plane_state,
> > __drm_atomic_helper_plane_state_reset(&plane_state->uapi, &plane->base);
> >
> > plane_state->scaler_id = -1;
> > + plane_state->fence_id = -1;
> > }
> >
> > struct intel_plane *intel_plane_alloc(void)
> > @@ -137,7 +138,7 @@ intel_plane_duplicate_state(struct drm_plane *plane)
> >
> > intel_state->ggtt_vma = NULL;
> > intel_state->dpt_vma = NULL;
> > - intel_state->flags = 0;
> > + intel_state->fence_id = -1;
> > intel_state->damage = DRM_RECT_INIT(0, 0, 0, 0);
> >
> > /* add reference to fb */
> > diff --git a/drivers/gpu/drm/i915/i915_initial_plane.c b/drivers/gpu/drm/i915/i915_initial_plane.c
> > index 390a9248d631..db51a468ddd4 100644
> > --- a/drivers/gpu/drm/i915/i915_initial_plane.c
> > +++ b/drivers/gpu/drm/i915/i915_initial_plane.c
> > @@ -268,7 +268,7 @@ i915_initial_plane_setup(struct drm_plane_state *_plane_state,
> > plane_state->ggtt_vma = i915_vma_get(vma);
> > if (intel_plane_uses_fence(plane_state) &&
> > i915_vma_pin_fence(vma) == 0 && vma->fence)
> > - plane_state->flags |= PLANE_HAS_FENCE;
> > + plane_state->fence_id = vma->fence->id;
> >
> > plane_state->surf = i915_ggtt_offset(plane_state->ggtt_vma);
> >
> > diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > index e45a1e7a4670..739d9c019094 100644
> > --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > @@ -418,15 +418,15 @@ intel_fb_pin_to_ggtt(const struct drm_framebuffer *fb,
> > unsigned int alignment,
> > unsigned int phys_alignment,
> > unsigned int vtd_guard,
> > - bool uses_fence,
> > - unsigned long *out_flags)
> > + int *out_fence_id)
> > {
> > - *out_flags = 0;
> > + if (out_fence_id)
> > + *out_fence_id = -1;
> >
> > return __xe_pin_fb_vma(to_intel_framebuffer(fb), view, alignment);
> > }
> >
> > -void intel_fb_unpin_vma(struct i915_vma *vma, unsigned long flags)
> > +void intel_fb_unpin_vma(struct i915_vma *vma, int fence_id)
> > {
> > __xe_unpin_fb_vma(vma);
> > }
> > diff --git a/drivers/gpu/drm/xe/display/xe_initial_plane.c b/drivers/gpu/drm/xe/display/xe_initial_plane.c
> > index 8bcae552dddc..09ec8f94bf57 100644
> > --- a/drivers/gpu/drm/xe/display/xe_initial_plane.c
> > +++ b/drivers/gpu/drm/xe/display/xe_initial_plane.c
> > @@ -153,7 +153,7 @@ xe_initial_plane_setup(struct drm_plane_state *_plane_state,
> > struct i915_vma *vma;
> >
> > vma = intel_fb_pin_to_ggtt(fb, &plane_state->view.gtt,
> > - 0, 0, 0, false, &plane_state->flags);
> > + 0, 0, 0, NULL);
> > if (IS_ERR(vma))
> > return PTR_ERR(vma);
>
> --
> Jani Nikula, Intel
--
Ville Syrjälä
Intel
next prev parent reply other threads:[~2026-04-01 14:29 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-31 16:21 [PATCH 1/2] drm/i915: Track fence region ID in plane state Ville Syrjala
2026-03-31 16:21 ` [PATCH 2/2] drm/i915: Remove the vma parent interface Ville Syrjala
2026-04-01 13:10 ` Jani Nikula
2026-03-31 17:15 ` ✗ i915.CI.BAT: failure for series starting with [1/2] drm/i915: Track fence region ID in plane state Patchwork
2026-03-31 17:19 ` ✓ CI.KUnit: success " Patchwork
2026-03-31 18:19 ` ✓ Xe.CI.BAT: " Patchwork
2026-04-01 0:28 ` ✗ Xe.CI.FULL: failure " Patchwork
2026-04-01 13:10 ` [PATCH 1/2] " Jani Nikula
2026-04-01 14:29 ` Ville Syrjälä [this message]
2026-04-02 14:54 ` Jani Nikula
2026-04-02 17:20 ` Ville Syrjälä
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ac0rrbHIe_R3b5Ck@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=jani.nikula@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.