Re: [Intel-gfx] [PATCH v2 3/4] drm/i915: Add new frontbuffer tracking interface to queue flush

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

On Thu, Jul 27, 2023 at 08:35:17AM +0300, Jouni Högander wrote:
> We want to wait dma fences in dirtyfb ioctl. As we don't want to make
> dirtyfb ioctl as blocking call we need to use
> dma_fence_add_callback. Callback used for dma_fence_add_callback is
> called from atomic context. Due to this we need to add a new
> frontbuffer tracking interface to queue flush.
> 
> v2: Check if flush work is already pending
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_frontbuffer.c  | 33 +++++++++++++++++++
>  .../gpu/drm/i915/display/intel_frontbuffer.h  |  4 +++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> index 17a7aa8b28c2..d33b6021d9ed 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> @@ -202,6 +202,39 @@ void __intel_fb_flush(struct intel_frontbuffer *front,
>  		frontbuffer_flush(i915, frontbuffer_bits, origin);
>  }
>  
> +static void intel_frontbuffer_flush_work(struct work_struct *work)
> +{
> +	struct intel_frontbuffer *front =
> +		container_of(work, struct intel_frontbuffer, flush_work);
> +
> +	i915_gem_object_flush_if_display(front->obj);
> +	intel_frontbuffer_flush(front, ORIGIN_DIRTYFB);
> +	intel_frontbuffer_put(front);
> +}
> +
> +/**
> + * intel_frontbuffer_queue_flush - queue flushing frontbuffer object
> + * @front: GEM object to flush
> + *
> + * This function is targeted for our dirty callback for queueing flush when
> + * dma fence is signales
> + */
> +void intel_frontbuffer_queue_flush(struct intel_frontbuffer *front)
> +{
> +	unsigned int frontbuffer_bits;
> +
> +	if (!front)
> +		return;
> +
> +	frontbuffer_bits = atomic_read(&front->bits);

I'm thinking we could check that already before we add the fence cb.

> +	if (!frontbuffer_bits || work_pending(&front->flush_work))

The work_pending() check is here to kep the kref in sync?
Maybe better to just use the return value of schedule_work() for
that?

> +		return;
> +
> +	kref_get(&front->ref);
> +	INIT_WORK(&front->flush_work, intel_frontbuffer_flush_work);

Don't we have a better place for the INIT_WORK() (eg. when we initialize
the intel_frontbuffer)?

> +	schedule_work(&front->flush_work);
> +}
> +
>  static int frontbuffer_active(struct i915_active *ref)
>  {
>  	struct intel_frontbuffer *front =
> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.h b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> index 3c474ed937fb..11760b5ce9fa 100644
> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.h
> @@ -47,6 +47,8 @@ struct intel_frontbuffer {
>  	struct i915_active write;
>  	struct drm_i915_gem_object *obj;
>  	struct rcu_head rcu;
> +
> +	struct work_struct flush_work;
>  };
>  
>  /*
> @@ -163,6 +165,8 @@ static inline void intel_frontbuffer_flush(struct intel_frontbuffer *front,
>  	__intel_fb_flush(front, origin, frontbuffer_bits);
>  }
>  
> +void intel_frontbuffer_queue_flush(struct intel_frontbuffer *front);
> +
>  void intel_frontbuffer_track(struct intel_frontbuffer *old,
>  			     struct intel_frontbuffer *new,
>  			     unsigned int frontbuffer_bits);
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel