Re: [Intel-xe] [RFC PATCH 0/3] Xe dma fence handling on atomic commit

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 28, 2023 at 08:23:41AM +0000, Hogander, Jouni wrote:
> On Wed, 2023-09-27 at 14:35 +0200, Maarten Lankhorst wrote:
> > Hey,
> > 
> > On 2023-09-27 12:45, Hogander, Jouni wrote:
> > > On Wed, 2023-09-27 at 12:33 +0200, Maarten Lankhorst wrote:
> > > > Hey,
> > > > 
> > > > When we wrote the original display support, we purposely decided
> > > > on
> > > > not
> > > > adding i915_sw_fence support.
> > > > 
> > > > In this case, I think a better approach would be to remove this
> > > > code
> > > > from i915 as well, and end up with cleaner display code for both
> > > > drivers.
> > > 
> > > Yes, I agree eventually this would be the goal. I did some
> > > experiments
> > > here:
> > > 
> > > https://patchwork.freedesktop.org/patch/558982/?series=123898&rev=4
> > > 
> > > Which is replacing i915_sw_fence with same code I'm using for Xe
> > > driver
> > > in this patch set. The problem is GPU reset detection. I don't have
> > > currently good ideas how to tackle that without compromizing i915
> > > functionality in this scenario -> ended up doing this only for Xe
> > > to
> > > ensure this is not blocking upstreaming Xe. Would this be
> > > acceptable as
> > > temporary solution to be solved after upstreaming? Anyways what I'm
> > > doing in these patches is not really an i915_sw_fence revised, but
> > > using dma_fences.
> > In atomic, plane_state->fence is set for all fences, and that should
> > be 
> > enough. I think creating a reservation object is a massive overkill,
> > and we should try to use the drm_atomic_helper_wait_for_fences if at
> > all 
> > possible.
> 
> Checked this plane_state->fence. To me it looks like it's for userspace
> to set? I don't think this is the case for i915. For Xe, I don't know
> is it done?
> 
> Also i915 is waiting for fences on old frame buffer objects and new fb
> objects. Shouldn't we do the same for Xe as well?

xe shouldn't need the old fb fence since userspace there won't be
doing any MI_SCANLINE_WAIT stuff.

> Anyways there is
> currently no mechanism in dma_fence implementation to have common fence
> for two separate dma_resv objects (dma_resv_get_singleton) -> I did
> this by creating a new dma_resv for this purpose. If there is no reason
> to wait for possible fences on old fb object, maybe we could just do
> dma_resv_get_singleton in prepare planes for Xe and wait for that and
> uapi.fence in commit?

drm_gem_plane_helper_prepare_fb() supposedly does something that already
works for xe. Looks like it does a few things:
- populates plane_state->fence with dma_resv_get_singleton() if using
  implicit fencing
- uses dma_fence_chain internally to add additional (kernel internal 
  for memory movement/etc. maybe?) fences to the uapi supplied explicit fence

Presumably we should be able to glue on the old fb fence using dma_fence_chain
as well?

-- 
Ville Syrjälä
Intel