Am 04.05.2018 um 11:25 schrieb Daniel Vetter:
On Fri, May 4, 2018 at 11:16 AM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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.
I kind of agree with Chris here, but also see the practical problem to
copy the default function in all the implementations.
We had the same problem in TTM and I also don't really like the result
to always have that "if (some_callback) default(); else some_callback();".
Might be that the run time overhead is negligible, but it doesn't feels
right from the coding style perspective.
Christian.
-Daniel
