Re: [PATCH 04/15] dma-fence: Make ->wait callback optional

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Daniel Vetter (2018-05-04 09:57:59)
>> On Fri, May 04, 2018 at 09:31:33AM +0100, Chris Wilson wrote:
>> > Quoting Daniel Vetter (2018-05-04 09:23:01)
>> > > On Fri, May 04, 2018 at 10:17:22AM +0200, Daniel Vetter wrote:
>> > > > On Fri, May 04, 2018 at 09:09:10AM +0100, Chris Wilson wrote:
>> > > > > Quoting Daniel Vetter (2018-05-03 15:25:52)
>> > > > > > Almost everyone uses dma_fence_default_wait.
>> > > > > >
>> > > > > > v2: Also remove the BUG_ON(!ops->wait) (Chris).
>> > > > >
>> > > > > I just don't get the rationale for implicit over explicit.
>> > > >
>> > > > Closer approximation of dwim semantics. There's been tons of patch series
>> > > > all over drm and related places to get there, once we have a big pile of
>> > > > implementations and know what the dwim semantics should be. Individually
>> > > > they're all not much, in aggregate they substantially simplify simple
>> > > > drivers.
>> > >
>> > > I also think clearer separation between optional optimization hooks and
>> > > mandatory core parts is useful in itself.
>> >
>> > A new spelling of midlayer ;) I don't see the contradiction with a
>> > driver saying use the default and simplicity. (I know which one the
>> > compiler thinks is simpler ;)
>>
>> If the compiler overhead is real then I guess it would makes to be
>> explicit. I don't expect that to be a problem though for a blocking
>> function.
>>
>> I disagree on this being a midlayer - you can still overwrite everything
>> you please to. What it does help is people doing less copypasting (and
>> assorted bugs), at least in the grand scheme of things. And we do have a
>> _lot_ more random small drivers than just a few years ago. Reducing the
>> amount of explicit typing just to get default bahaviour has been an
>> ongoing theme for a few years now, and your objection here is about the
>> first that this is not a good idea. So I'm somewhat confused.
>
> I'm just saying I don't see any rationale for this patch.
>
>         "Almost everyone uses dma_fence_default_wait."
>
> Why change?
>
> Making it look simpler on the surface, so that you don't have to think
> about things straight away? I understand the appeal, but I do worry
> about it just being an illusion. (Cutting and pasting a line saying
> .wait = default_wait, doesn't feel that onerous, as you likely cut and
> paste the ops anyway, and at the very least you are reminded about some
> of the interactions. You could even have default initializers and/or
> magic macros to hide the cut and paste; maybe a simple_dma_fence [now
> that's a midlayer!] but I haven't looked.)

In really monolithic vtables like drm_driver we do use default
function macros, so you type 1 line, get them all. But dma_fence_ops
is pretty small, and most drivers only implement a few callbacks. Also
note that e.g. the ->release callback already works like that, so this
pattern is there already. I simply extended it to ->wait and
->enable_signaling. Also note that I leave the EXPORT_SYMBOL in place,
you can still wrap dma_fence_default_wait if you wish to do so.

But I just realized that I didn't clean out the optional release
hooks, I guess I should do that too (for the few cases it's not yet
done) and respin.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch