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

[Emil Velikov <emil.l.velikov@gmail.com>]

On 4 July 2018 at 13:34, Daniel Vetter <daniel@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@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.

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.

Thanks
Emil
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx