| Message ID | 20180503142603.28513-5-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -Chris
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. -Daniel
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. -Daniel
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 ;) -Chris
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. It's ofc not all that useful when looking only through the i915 perspective, where we overwrite almost everything anyway. But the ecosystem is a bit bigger than just i915. -Daniel
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.) -Chris
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
Am 04.05.2018 um 11:25 schrieb Daniel Vetter: > 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. 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
On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote: > Am 04.05.2018 um 11:25 schrieb Daniel Vetter: > > 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. > > 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. Hm, maybe I've seen too much bad code, but modeset helpers is choke full of exactly that pattern. It's imo also a trade-off. If you have a fairly specialized library like ttm that's used by relatively few things, doing everything explicitly is probably better. It's also where kms started out from. But if you have a huge pile of fairly simple drivers, imo the balance starts to tip the other way, and a bit of additional logic in the shared code to make all the implementations a notch simpler is good. If we wouldn't have acquired quite a pile of dma_fence implementations I wouldn't have bothered with all this. -Daniel
On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote: > On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote: > > Am 04.05.2018 um 11:25 schrieb Daniel Vetter: > > > 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. > > > > 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. > > Hm, maybe I've seen too much bad code, but modeset helpers is choke full > of exactly that pattern. It's imo also a trade-off. If you have a fairly > specialized library like ttm that's used by relatively few things, doing > everything explicitly is probably better. It's also where kms started out > from. > > But if you have a huge pile of fairly simple drivers, imo the balance > starts to tip the other way, and a bit of additional logic in the shared > code to make all the implementations a notch simpler is good. If we > wouldn't have acquired quite a pile of dma_fence implementations I > wouldn't have bothered with all this. So ack/nack on this (i.e. do you retract your original r-b or not)? It's kinda holding up all the cleanup patches below ... I went ahead and applied the first three patches of this series meanwhile. -Daniel
Am 02.07.2018 um 10:23 schrieb Daniel Vetter: > On Fri, May 04, 2018 at 03:47:59PM +0200, Daniel Vetter wrote: >> On Fri, May 04, 2018 at 03:17:08PM +0200, Christian König wrote: >>> Am 04.05.2018 um 11:25 schrieb Daniel Vetter: >>>> 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. >>> 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. >> Hm, maybe I've seen too much bad code, but modeset helpers is choke full >> of exactly that pattern. It's imo also a trade-off. If you have a fairly >> specialized library like ttm that's used by relatively few things, doing >> everything explicitly is probably better. It's also where kms started out >> from. >> >> But if you have a huge pile of fairly simple drivers, imo the balance >> starts to tip the other way, and a bit of additional logic in the shared >> code to make all the implementations a notch simpler is good. If we >> wouldn't have acquired quite a pile of dma_fence implementations I >> wouldn't have bothered with all this. > So ack/nack on this (i.e. do you retract your original r-b or not)? It's > kinda holding up all the cleanup patches below ... Feel free to add my Acked-by for now, but I still have a kind of a gut feeling that we might want to revisit this decision at some time. Christian. > > I went ahead and applied the first three patches of this series meanwhile. > -Daniel
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index dd1edfb27b61..a8c254497251 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -104,7 +104,6 @@ const struct dma_fence_ops dma_fence_array_ops = { .get_timeline_name = dma_fence_array_get_timeline_name, .enable_signaling = dma_fence_array_enable_signaling, .signaled = dma_fence_array_signaled, - .wait = dma_fence_default_wait, .release = dma_fence_array_release, }; EXPORT_SYMBOL(dma_fence_array_ops); diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 59049375bd19..41ec19c9efc7 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -158,7 +158,10 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) return -EINVAL; trace_dma_fence_wait_start(fence); - ret = fence->ops->wait(fence, intr, timeout); + if (fence->ops->wait) + ret = fence->ops->wait(fence, intr, timeout); + else + ret = dma_fence_default_wait(fence, intr, timeout); trace_dma_fence_wait_end(fence); return ret; } @@ -562,8 +565,7 @@ dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, unsigned seqno) { BUG_ON(!lock); - BUG_ON(!ops || !ops->wait || - !ops->get_driver_name || !ops->get_timeline_name); + BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name); kref_init(&fence->refcount); fence->ops = ops; diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 3d78ca89a605..53c1d6d36a64 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -188,7 +188,6 @@ static const struct dma_fence_ops timeline_fence_ops = { .get_timeline_name = timeline_fence_get_timeline_name, .enable_signaling = timeline_fence_enable_signaling, .signaled = timeline_fence_signaled, - .wait = dma_fence_default_wait, .release = timeline_fence_release, .fence_value_str = timeline_fence_value_str, .timeline_value_str = timeline_fence_timeline_value_str, diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index c053d19e1e24..02dba8cd033d 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -191,11 +191,14 @@ struct dma_fence_ops { /** * @wait: * - * Custom wait implementation, or dma_fence_default_wait. + * Custom wait implementation, defaults to dma_fence_default_wait() if + * not set. * - * Must not be NULL, set to dma_fence_default_wait for default implementation. - * the dma_fence_default_wait implementation should work for any fence, as long - * as enable_signaling works correctly. + * The dma_fence_default_wait implementation should work for any fence, as long + * as @enable_signaling works correctly. This hook allows drivers to + * have an optimized version for the case where a process context is + * already available, e.g. if @enable_signaling for the general case + * needs to set up a worker thread. * * Must return -ERESTARTSYS if the wait is intr = true and the wait was * interrupted, and remaining jiffies if fence has signaled, or 0 if wait @@ -203,7 +206,7 @@ struct dma_fence_ops { * which should be treated as if the fence is signaled. For example a hardware * lockup could be reported like that. * - * This callback is mandatory. + * This callback is optional. */ signed long (*wait)(struct dma_fence *fence, bool intr, signed long timeout);