[Intel-gfx] [PATCH 1/5] drm/i915: Remove unecessary dma_fence_ops

[Daniel Vetter]

On Wed, Jul 4, 2018 at 7:22 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 4 July 2018 at 13:34, Daniel Vetter <daniel at ffwll.ch> wrote:
>> 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 at 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 at ffwll.ch>
>>> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
>>> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>>> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> > Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>> > Cc: Colin Ian King <colin.king at canonical.com>
>>> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>> > Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>>> > Cc: intel-gfx at 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.
>
> Nothing obvious comes to mind, yet again my knowledge of the fence API
> is limited.
> Was simply pointing out something that was removed without a small
> note covering it.
>
> A fraction of your explanation would have been great, but obviously
> not a big deal either way.

Yeah would have been good to add to the commit message that made
->enable_signaling optional, but that's now baked into history already
:-/
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch