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: 8+ 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-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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox