From: Jani Nikula <jani.nikula@linux.intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>,
intel-gfx@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/i915: Track fence region ID in plane state
Date: Wed, 01 Apr 2026 16:10:16 +0300 [thread overview]
Message-ID: <4dee82289c67c00562cbb4fcc8a16c8b3c7a6e4c@intel.com> (raw)
In-Reply-To: <20260331162138.19258-1-ville.syrjala@linux.intel.com>
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.
>
> 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.
> +
> /* 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
next prev parent reply other threads:[~2026-04-01 13:10 UTC|newest]
Thread overview: 10+ 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:19 ` ✓ CI.KUnit: success for series starting with [1/2] drm/i915: Track fence region ID in plane state 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 ` Jani Nikula [this message]
2026-04-01 14:29 ` [PATCH 1/2] " Ville Syrjälä
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=4dee82289c67c00562cbb4fcc8a16c8b3c7a6e4c@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=ville.syrjala@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