Re: [PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops

From: Daniel Vetter <daniel@ffwll.ch>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Rodrigo Vivi <rodrigo.vivi@intel.com>,
	Colin Ian King <colin.king@canonical.com>,
	Mika Kuoppala <mika.kuoppala@intel.com>
Subject: Re: [PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops
Date: Wed, 4 Jul 2018 14:34:02 +0200	[thread overview]
Message-ID: <20180704123402.GC4693@phenom.ffwll.local> (raw)
In-Reply-To: <CACvgo505v-qz2Oz2CtZL2p=0iKvrxsmYKNx-Lc7btHK7E-SvrQ@mail.gmail.com>

On Wed, Jul 04, 2018 at 01:03:18PM +0100, Emil Velikov wrote:
> Hi Daniel,
> 
> On 4 July 2018 at 10:29, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > dma_fence_default_wait is the default now, same for the trivial
> > enable_signaling implementation.
> >
> > v2: Also remove the relase hook, dma_fence_free is the default.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Colin Ian King <colin.king@canonical.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/i915/i915_gem_clflush.c        | 7 -------
> >  drivers/gpu/drm/i915/selftests/i915_sw_fence.c | 8 --------
> >  2 files changed, 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_clflush.c b/drivers/gpu/drm/i915/i915_gem_clflush.c
> > index f5c570d35b2a..8e74c23cbd91 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_clflush.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_clflush.c
> > @@ -45,11 +45,6 @@ static const char *i915_clflush_get_timeline_name(struct dma_fence *fence)
> >         return "clflush";
> >  }
> >
> > -static bool i915_clflush_enable_signaling(struct dma_fence *fence)
> > -{
> > -       return true;
> > -}
> > -
> >  static void i915_clflush_release(struct dma_fence *fence)
> >  {
> >         struct clflush *clflush = container_of(fence, typeof(*clflush), dma);
> > @@ -63,8 +58,6 @@ static void i915_clflush_release(struct dma_fence *fence)
> >  static const struct dma_fence_ops i915_clflush_ops = {
> >         .get_driver_name = i915_clflush_get_driver_name,
> >         .get_timeline_name = i915_clflush_get_timeline_name,
> > -       .enable_signaling = i915_clflush_enable_signaling,
> From a quick look through drm-misc/drm-misc-next removing the
> enable_signalling hook may cause functional changes.
> 
> Namely:
> A call to trace_dma_fence_enable_signal() (in
> dma_fence_enable_sw_signaling(), dma_fence_add_callback() and others)
> will be omitted.

I'm not sure what this tracepoint is useful for in the absenve of a real
signaling mechanism that must be enabled (like interrupts).

For all the other bits (begin/end wait, fence signalling itsefl) we have
tracepoints already, so I think we're all covered. What do you think will
be lost with the tracepoint here? If there's a real need for it I think I
can rework the already merged patch to still call the tracpoint, while
avoiding everything else. I just don't see the use-case for that.
-Daniel

> 
> Removing the default .wait and .release hooks is fine.
> 
> HTH
> Emil

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx