| Message ID | 20180704092909.6599-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Removing the default .wait and .release hooks is fine. HTH Emil
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
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
On Wed, Jul 4, 2018 at 7:22 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote: > 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. 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
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, - .wait = dma_fence_default_wait, .release = i915_clflush_release, }; diff --git a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c index 570e325af93e..cdbc8f134e5e 100644 --- a/drivers/gpu/drm/i915/selftests/i915_sw_fence.c +++ b/drivers/gpu/drm/i915/selftests/i915_sw_fence.c @@ -611,17 +611,9 @@ static const char *mock_name(struct dma_fence *fence) return "mock"; } -static bool mock_enable_signaling(struct dma_fence *fence) -{ - return true; -} - static const struct dma_fence_ops mock_fence_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name, - .enable_signaling = mock_enable_signaling, - .wait = dma_fence_default_wait, - .release = dma_fence_free, }; static DEFINE_SPINLOCK(mock_fence_lock);
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(-)