From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 442FAC25B48 for ; Thu, 26 Oct 2023 12:46:33 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AE57C10E7CF; Thu, 26 Oct 2023 12:46:32 +0000 (UTC) Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 4E5F310E7D5 for ; Thu, 26 Oct 2023 12:46:30 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1698324390; x=1729860390; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=oEX//XGE2+X2yud3fnXI5v4ht7VLkldQycvUOqh4dX0=; b=ce9H7wxb3CfBQdN0crqHRXSh4C1PGjFUYFGAOnDuYuAxML+luFBvxyDp 4JxP1i+6wuHUlR9az0LHO7TKHCQ0j3fbwilNUYiLQlr2P5YaaYK9133DB jmjM2jdK+5lvXnHtmma/RRs/50MOVCEyEAuF23hksxm+3j3F3HI2O3mqx Y9IN1QS6EzN0MOMjQ3c86pMt1hjThGv6Htav15UetcL401gaTu9B3MyEE mJtRjpMU1aq9Ddl8qPLnzIEx2rNqCrI9SplzPENi7MrYnn7rL5tOk0hOk Z+PhDBEcQhZCvJ7VBMGXoek3EK1WhzJpRAar7vXVAuMtyboRpq3mg1CQm g==; X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="473773376" X-IronPort-AV: E=Sophos;i="6.03,253,1694761200"; d="scan'208";a="473773376" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 26 Oct 2023 05:46:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="1090595023" X-IronPort-AV: E=Sophos;i="6.03,253,1694761200"; d="scan'208";a="1090595023" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by fmsmga005.fm.intel.com with SMTP; 26 Oct 2023 05:46:27 -0700 Received: by stinkbox (sSMTP sendmail emulation); Thu, 26 Oct 2023 15:46:26 +0300 Date: Thu, 26 Oct 2023 15:46:26 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: "Hogander, Jouni" Message-ID: References: <20231020094103.1605942-1-jouni.hogander@intel.com> <54fe40ad3c9739eeff3c2e8716e2f5b9085bf1c5.camel@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <54fe40ad3c9739eeff3c2e8716e2f5b9085bf1c5.camel@intel.com> X-Patchwork-Hint: comment Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/display: Use dma_fence interfaces instead of i915_sw_fence X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "intel-gfx@lists.freedesktop.org" Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Thu, Oct 26, 2023 at 09:40:23AM +0000, Hogander, Jouni wrote: > On Wed, 2023-10-25 at 17:18 +0300, Ville Syrjälä wrote: > > On Fri, Oct 20, 2023 at 12:41:03PM +0300, Jouni Högander wrote: > > > We are preparing for Xe driver. Xe driver doesn't have > > > i915_sw_fence > > > implementation. Lets drop i915_sw_fence usage from display code and > > > use dma_fence interfaces directly. > > > > > > For this purpose stack dma fences from related objects into new > > > plane > > > state. Drm_gem_plane_helper_prepare_fb can be used for fences in > > > new > > > fb. Separate local implementation is used for Stacking fences from > > > old fb > > > into new plane state. Then wait for these stacked fences during > > > atomic > > > commit. There is no be need for separate GPU reset handling in > > > intel_atomic_commit_fence_wait as the fences are signaled when GPU > > > hang is > > > detected and GPU is being reset. > > > > > > v2: > > >   - Add fences from old fb into new_plane_state->uapi.fence rather > > > than > > >     into old_plane_state->uapi.fence > > > > > > Cc: Ville Syrjälä > > > Cc: Maarten Lankhorst > > > Cc: José Roberto de Souza > > > > > > Signed-off-by: Jouni Högander > > > Reviewed-by: Maarten Lankhorst > > > --- > > >  drivers/gpu/drm/i915/display/intel_atomic.c   |  3 - > > >  .../gpu/drm/i915/display/intel_atomic_plane.c | 89 +++++++++++---- > > > ---- > > >  drivers/gpu/drm/i915/display/intel_display.c  | 78 ++++++--------- > > > - > > >  .../drm/i915/display/intel_display_types.h    |  2 - > > >  4 files changed, 77 insertions(+), 95 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > > > b/drivers/gpu/drm/i915/display/intel_atomic.c > > > index 5d18145da279..ec0d5168b503 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > > > @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct > > > drm_atomic_state *_state) > > >   > > >         drm_atomic_state_default_release(&state->base); > > >         kfree(state->global_objs); > > > - > > > -       i915_sw_fence_fini(&state->commit_ready); > > > - > > >         kfree(state); > > >  } > > >   > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > index b1074350616c..20fd12df6850 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c > > > @@ -31,7 +31,10 @@ > > >   * prepare/check/commit/cleanup steps. > > >   */ > > >   > > > +#include > > > + > > >  #include > > > +#include > > >  #include > > >  #include > > >   > > > @@ -1012,6 +1015,44 @@ int intel_plane_check_src_coordinates(struct > > > intel_plane_state *plane_state) > > >         return 0; > > >  } > > >   > > > +static int add_fences(struct dma_resv *obj, > > > +                     struct drm_plane_state *dst) > > > > I would name things differently: > > s/obj/resv/ > > s/dst/plane_state/ > > > > The function name should probably be a bit more > > descriptive as well. > > > > > +{ > > > +       struct dma_fence *fence = dma_fence_get(dst->fence); > > > +       enum dma_resv_usage usage; > > > +       struct dma_fence *new; > > > +       int ret; > > > + > > > +       usage = fence ? DMA_RESV_USAGE_KERNEL : > > > DMA_RESV_USAGE_WRITE; > > > + > > > +       ret = dma_resv_get_singleton(obj, usage, &new); > > > +       if (ret) > > > +               goto error; > > > + > > > +       if (new && fence) { > > > +               struct dma_fence_chain *chain = > > > dma_fence_chain_alloc(); > > > + > > > +               if (!chain) { > > > +                       ret = -ENOMEM; > > > +                       goto error; > > > +               } > > > + > > > +               dma_fence_chain_init(chain, fence, new, 1); > > > +               fence = &chain->base; > > > + > > > +       } else if (new) { > > > +               fence = new; > > > +       } > > > + > > > +       dma_fence_put(dst->fence); > > > +       dst->fence = fence; > > > +       return 0; > > > + > > > +error: > > > +       dma_fence_put(fence); > > > +       return ret; > > > +} > > > + > > >  /** > > >   * intel_prepare_plane_fb - Prepare fb for usage on plane > > >   * @_plane: drm plane to prepare for > > > @@ -1035,7 +1076,7 @@ intel_prepare_plane_fb(struct drm_plane > > > *_plane, > > >         struct intel_atomic_state *state = > > >                 to_intel_atomic_state(new_plane_state->uapi.state); > > >         struct drm_i915_private *dev_priv = to_i915(plane- > > > >base.dev); > > > -       const struct intel_plane_state *old_plane_state = > > > +       struct intel_plane_state *old_plane_state = > > >                 intel_atomic_get_old_plane_state(state, plane); > > >         struct drm_i915_gem_object *obj = > > > intel_fb_obj(new_plane_state->hw.fb); > > >         struct drm_i915_gem_object *old_obj = > > > intel_fb_obj(old_plane_state->hw.fb); > > > @@ -1057,56 +1098,30 @@ intel_prepare_plane_fb(struct drm_plane > > > *_plane, > > >                  * This should only fail upon a hung GPU, in which > > > case we > > >                  * can safely continue. > > >                  */ > > > -               if (new_crtc_state && > > > intel_crtc_needs_modeset(new_crtc_state)) { > > > -                       ret = > > > i915_sw_fence_await_reservation(&state->commit_ready, > > > -                                                             > > > old_obj->base.resv, > > > -                                                             > > > false, 0, > > > -                                                             > > > GFP_KERNEL); > > > +               if (new_crtc_state && > > > intel_crtc_needs_modeset(new_crtc_state) && > > > +                   !dma_resv_test_signaled(old_obj->base.resv, > > > +                                           > > > dma_resv_usage_rw(false))) { > > > > Is there a specific reason for the "signaled?" test here? > > I will drop it. dma_resv_get_singleton will anyways provide NULL fence > if all are signaled. > > > > > > +                       ret = add_fences(old_obj->base.resv, > > > &new_plane_state->uapi); > > >                         if (ret < 0) > > >                                 return ret; > > >                 } > > >         } > > >   > > > -       if (new_plane_state->uapi.fence) { /* explicit fencing */ > > > -               i915_gem_fence_wait_priority(new_plane_state- > > > >uapi.fence, > > > -                                            &attr); > > > -               ret = i915_sw_fence_await_dma_fence(&state- > > > >commit_ready, > > > -                                                   > > > new_plane_state->uapi.fence, > > > -                                                   > > > i915_fence_timeout(dev_priv), > > > -                                                   GFP_KERNEL); > > > -               if (ret < 0) > > > -                       return ret; > > > -       } > > > - > > >         if (!obj) > > >                 return 0; > > >   > > > - > > >         ret = intel_plane_pin_fb(new_plane_state); > > >         if (ret) > > >                 return ret; > > >   > > > -       i915_gem_object_wait_priority(obj, 0, &attr); > > > +       ret = drm_gem_plane_helper_prepare_fb(_plane, > > > &new_plane_state->uapi); > > > > I'd prefer not to consult _plane variable anywhere but the initial > > cast. > > > > > +       if (ret < 0) > > > +               goto unpin_fb; > > >   > > > -       if (!new_plane_state->uapi.fence) { /* implicit fencing */ > > > -               struct dma_resv_iter cursor; > > > -               struct dma_fence *fence; > > > - > > > -               ret = i915_sw_fence_await_reservation(&state- > > > >commit_ready, > > > -                                                     obj- > > > >base.resv, false, > > > -                                                     > > > i915_fence_timeout(dev_priv), > > > -                                                     GFP_KERNEL); > > > -               if (ret < 0) > > > -                       goto unpin_fb; > > > +       if (new_plane_state->uapi.fence) { > > > +               i915_gem_fence_wait_priority(new_plane_state- > > > >uapi.fence, > > > +                                            &attr); > > >   > > > -               dma_resv_iter_begin(&cursor, obj->base.resv, > > > -                                   DMA_RESV_USAGE_WRITE); > > > -               dma_resv_for_each_fence_unlocked(&cursor, fence) { > > > - > > >                        intel_display_rps_boost_after_vblank(new_plan > > > e_state->hw.crtc, > > > -                                                            > > > fence); > > > -               } > > > -               dma_resv_iter_end(&cursor); > > > -       } else { > > >                 intel_display_rps_boost_after_vblank(new_plane_stat > > > e->hw.crtc, > > >                                                      > > > new_plane_state->uapi.fence); > > >         } > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index 28d85e1e858e..48df0559df00 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -48,6 +48,7 @@ > > >  #include "g4x_dp.h" > > >  #include "g4x_hdmi.h" > > >  #include "hsw_ips.h" > > > +#include "i915_config.h" > > >  #include "i915_drv.h" > > >  #include "i915_reg.h" > > >  #include "i915_utils.h" > > > @@ -7056,29 +7057,34 @@ void > > > intel_atomic_helper_free_state_worker(struct work_struct *work) > > >   > > >  static void intel_atomic_commit_fence_wait(struct > > > intel_atomic_state *intel_state) > > >  { > > > -       struct wait_queue_entry wait_fence, wait_reset; > > > -       struct drm_i915_private *dev_priv = to_i915(intel_state- > > > >base.dev); > > > - > > > -       init_wait_entry(&wait_fence, 0); > > > -       init_wait_entry(&wait_reset, 0); > > > -       for (;;) { > > > -               prepare_to_wait(&intel_state->commit_ready.wait, > > > -                               &wait_fence, TASK_UNINTERRUPTIBLE); > > > -               prepare_to_wait(bit_waitqueue(&to_gt(dev_priv)- > > > >reset.flags, > > > -                                             I915_RESET_MODESET), > > > -                               &wait_reset, TASK_UNINTERRUPTIBLE); > > > +       struct drm_i915_private *i915 = to_i915(intel_state- > > > >base.dev); > > > +       struct drm_plane *plane; > > > +       struct drm_plane_state *new_plane_state, *old_plane_state; > > > +       int ret, i; > > >   > > > +       for_each_oldnew_plane_in_state(&intel_state->base, plane, > > > +                                      old_plane_state, > > > new_plane_state, i) { > > > +               if (new_plane_state->fence) { > > > +                       ret = > > > dma_fence_wait_timeout(new_plane_state->fence, > > > +                                                    false, > > > +                                                    > > > i915_fence_timeout(i915)); > > > +                       if (ret <= 0) > > > +                               break; > > >   > > > -               if (i915_sw_fence_done(&intel_state->commit_ready) > > > || > > > -                   test_bit(I915_RESET_MODESET, &to_gt(dev_priv)- > > > >reset.flags)) > > > -                       break; > > > +                       dma_fence_put(new_plane_state->fence); > > > +                       new_plane_state->fence = NULL; > > > +               } > > > +               if (old_plane_state->fence) { > > > +                       ret = > > > dma_fence_wait_timeout(old_plane_state->fence, > > > +                                                    false, > > > +                                                    > > > i915_fence_timeout(i915)); > > > +                       if (ret <= 0) > > > +                               break; > > > > Why is this still here? > > Yeah, it isn't needed anymore. I think I can now use > drm_atomic_helper_wait_for_fences. I think the timeout it uses is different, so this might need some actual thought. > I will address this one and your > other comments and resend. > > BR, > > Jouni Högander > > >   > > > -               schedule(); > > > +                       dma_fence_put(old_plane_state->fence); > > > +                       old_plane_state->fence = NULL; > > > +               } > > >         } > > > -       finish_wait(&intel_state->commit_ready.wait, &wait_fence); > > > -       finish_wait(bit_waitqueue(&to_gt(dev_priv)->reset.flags, > > > -                                 I915_RESET_MODESET), > > > -                   &wait_reset); > > >  } > > >   > > >  static void intel_atomic_cleanup_work(struct work_struct *work) > > > @@ -7370,32 +7376,6 @@ static void intel_atomic_commit_work(struct > > > work_struct *work) > > >         intel_atomic_commit_tail(state); > > >  } > > >   > > > -static int > > > -intel_atomic_commit_ready(struct i915_sw_fence *fence, > > > -                         enum i915_sw_fence_notify notify) > > > -{ > > > -       struct intel_atomic_state *state = > > > -               container_of(fence, struct intel_atomic_state, > > > commit_ready); > > > - > > > -       switch (notify) { > > > -       case FENCE_COMPLETE: > > > -               /* we do blocking waits in the worker, nothing to > > > do here */ > > > -               break; > > > -       case FENCE_FREE: > > > -               { > > > -                       struct drm_i915_private *i915 = > > > to_i915(state->base.dev); > > > -                       struct intel_atomic_helper *helper = > > > -                               &i915->display.atomic_helper; > > > - > > > -                       if (llist_add(&state->freed, &helper- > > > >free_list)) > > > -                               queue_work(i915->unordered_wq, > > > &helper->free_work); > > > -                       break; > > > -               } > > > -       } > > > - > > > -       return NOTIFY_DONE; > > > -} > > > - > > >  static void intel_atomic_track_fbs(struct intel_atomic_state > > > *state) > > >  { > > >         struct intel_plane_state *old_plane_state, > > > *new_plane_state; > > > @@ -7418,10 +7398,6 @@ int intel_atomic_commit(struct drm_device > > > *dev, struct drm_atomic_state *_state, > > >   > > >         state->wakeref = intel_runtime_pm_get(&dev_priv- > > > >runtime_pm); > > >   > > > -       drm_atomic_state_get(&state->base); > > > -       i915_sw_fence_init(&state->commit_ready, > > > -                          intel_atomic_commit_ready); > > > - > > >         /* > > >          * The intel_legacy_cursor_update() fast path takes care > > >          * of avoiding the vblank waits for simple cursor > > > @@ -7454,7 +7430,6 @@ int intel_atomic_commit(struct drm_device > > > *dev, struct drm_atomic_state *_state, > > >         if (ret) { > > >                 drm_dbg_atomic(&dev_priv->drm, > > >                                "Preparing state failed with %i\n", > > > ret); > > > -               i915_sw_fence_commit(&state->commit_ready); > > >                 intel_runtime_pm_put(&dev_priv->runtime_pm, state- > > > >wakeref); > > >                 return ret; > > >         } > > > @@ -7470,8 +7445,6 @@ int intel_atomic_commit(struct drm_device > > > *dev, struct drm_atomic_state *_state, > > >                 struct intel_crtc *crtc; > > >                 int i; > > >   > > > -               i915_sw_fence_commit(&state->commit_ready); > > > - > > >                 for_each_new_intel_crtc_in_state(state, crtc, > > > new_crtc_state, i) > > >                         intel_color_cleanup_commit(new_crtc_state); > > >   > > > @@ -7485,7 +7458,6 @@ int intel_atomic_commit(struct drm_device > > > *dev, struct drm_atomic_state *_state, > > >         drm_atomic_state_get(&state->base); > > >         INIT_WORK(&state->base.commit_work, > > > intel_atomic_commit_work); > > >   > > > -       i915_sw_fence_commit(&state->commit_ready); > > >         if (nonblock && state->modeset) { > > >                 queue_work(dev_priv->display.wq.modeset, &state- > > > >base.commit_work); > > >         } else if (nonblock) { > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index 65ea37fe8cff..047fe3f8905a 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -676,8 +676,6 @@ struct intel_atomic_state { > > >   > > >         bool rps_interactive; > > >   > > > -       struct i915_sw_fence commit_ready; > > > - > > >         struct llist_node freed; > > >  }; > > >   > > > -- > > > 2.34.1 > > > -- Ville Syrjälä Intel