fence: Use smp_mb__before_atomic()
PATCH 1/1 fence: Use smp_mb__before_atomic()
From: Thierry Reding <[email protected]>
Commit febdbfe8a91c (arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().
--- drivers/base/fence.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/base/fence.c +2 -2
@@ -65,7 +65,7 @@ int __fence_signal(struct fence *fence)if (!ktime_to_ns (fence->timestamp)) { fence->timestamp =ktime_get (); -smp_mb__before_clear_bit (); +smp_mb__before_atomic (); }if (test_and_set_bit (FENCE_FLAG_SIGNALED_BIT , &fence->flags)) {
@@ -105,7 +105,7 @@ int fence_signal(struct fence *fence)if (!ktime_to_ns (fence->timestamp)) { fence->timestamp =ktime_get (); -smp_mb__before_clear_bit (); +smp_mb__before_atomic (); }if (test_and_set_bit (FENCE_FLAG_SIGNALED_BIT , &fence->flags))
Signature · 1 line
1.9.2
Quoted 7 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>
I saw the patches, but hard to clean up if it's not in the fences tree yet. :-)
~maarten
Greg Kroah-Hartman
maintainerQuoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--
Where does this file come from? I've not seen it before, and it's not in my tree.
confused,
greg k-h
Quoted 7 lines · Greg Kroah-Hartman
Quoted 6 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().
Quoted 6 lines
Quoted 3 lines
Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Thierry
Greg Kroah-Hartman
maintainerQuoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--
Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
greg k-h
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Quoted 2 lines
greg k-h
Best regards,
~Sumit.
Quoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4 -0
@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git
@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
That is, if that's something that Greg wants.
Thierry
Greg Kroah-Hartman
maintainerQuoted 74 lines · Thierry Reding
Quoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4
-0@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*
Quoted 2 lines
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
That doesn't really work for most people, I'll still be "responsible" for the code.
Quoted 2 lines
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
That might be best for some of this.
But again, why is the fence.c code needed at all anyway? I'm not sold on that.
greg k-h
Quoted 81 lines · Greg Kroah-Hartman
Quoted 74 lines · Thierry Reding
Quoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4
-0@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*Quoted 2 lines
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
That doesn't really work for most people, I'll still be "responsible" for the code.
Quoted 7 lines
Quoted 2 lines
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
That might be best for some of this.
But again, why is the
fence.ccode needed at all anyway? I'm not sold on that.
Fence serves as a way to synchronize between (for example) multiple asynchronous gpu's. There is definitely a need for this. Otherwise performance for optimus/prime type setups is going to suck.
I thought we had added something under Documentation/ about it, but I can't find it now (although possibly looking at the wrong trees).. there is at least a bit of a description in the commit msg:
https://lkml.org/lkml/2014/2/24/602
I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
BR,
-R
Quoted 6 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
op 05-06-14 13:51, Rob Clark schreef:
Quoted 91 lines
Quoted 75 lines · Greg Kroah-Hartman
Quoted 69 lines · Thierry Reding
Quoted 37 lines · Sumit Semwal
Hi Greg,
Quoted 16 lines · Greg Kroah-Hartman
Quoted 8 lines · Thierry Reding
Quoted 7 lines · Greg Kroah-Hartman
Quoted 6 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Quoted 7 lines
Quoted 5 lines
Quoted 3 lines
Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4
-0@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*Quoted 2 lines
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
That doesn't really work for most people, I'll still be "responsible" for the code.
Quoted 6 lines
Quoted 2 lines
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
That might be best for some of this.
But again, why is the
fence.ccode needed at all anyway? I'm not sold on that.Fence serves as a way to synchronize between (for example) multiple asynchronous gpu's. There is definitely a need for this. Otherwise performance for optimus/prime type setups is going to suck.
I thought we had added something under
Documentation/about it, but I can't find it now (although possibly looking at the wrong trees).. there is at least a bit of a description in the commit msg:
Quoted 7 lines
https://lkml.org/lkml/2014/2/24/602I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
Additionally we already have a user for this in the kernel: the GPU drivers that use TTM.
In my tree I have a branch that converts TTM fences to this fence mechanism, roughly preserving the same functionality. As soon as the fence code is merged I'll move TTM over to the new code.
Additionally I added a series on top that allows drm drivers to get additional fences through dma-buf, allowing nouveau and i915 to work on the same buffer object,
with both drivers being aware that the other driver is performing work on it.
~Maarten
Greg Kroah-Hartman
maintainerQuoted 7 lines · Maarten Lankhorst
Quoted 5 lines
I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
Additionally we already have a user for this in the kernel: the GPU drivers that use TTM.
What about the sync code in the android directory? I thought that was supposed to be what GPU drivers were going to use for this type of thing.
thanks,
greg k-h
Hi Greg,
Quoted 99 lines · Rob Clark
Quoted 81 lines · Greg Kroah-Hartman
Quoted 74 lines · Thierry Reding
Quoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4
-0@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*Quoted 2 lines
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
That doesn't really work for most people, I'll still be "responsible" for the code.
Quoted 7 lines
Quoted 2 lines
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
That might be best for some of this.
But again, why is the
fence.ccode needed at all anyway? I'm not sold on that.Fence serves as a way to synchronize between (for example) multiple asynchronous gpu's. There is definitely a need for this. Otherwise performance for optimus/prime type setups is going to suck.
I thought we had added something under
Documentation/about it, but I can't find it now (although possibly looking at the wrong trees).. there is at least a bit of a description in the commit msg:
Quoted 7 lines
https://lkml.org/lkml/2014/2/24/602
I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
Pending agreement on the need of fence.c (Rob has given the most prominent use case for it, and the lack of existing mechanisms to use already), would you be ok if I move all the dma-buf* and related (fence.c etc) to its own subdirectory and queue it up for this release?
Quoted 4 lines
BR,
-R
Best regards,
~Sumit.
Quoted 6 lines
Quoted 6 lines
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Greg Kroah-Hartman
maintainerQuoted 4 lines · Sumit Semwal
Hi Greg,
Pending agreement on the need offence.c(Rob has given the most prominent use case for it, and the lack of existing mechanisms to use already),
See my disagreement there :)
Quoted 2 lines
would you be ok if I move all the dma-buf* and related (
fence.cetc) to its own subdirectory and queue it up for this release?
This release (i.e. 3.16) is way too early. You need to get agreement of what gets into linux-next first, tested there, and then you can get it merged into Linus's tree.
In the middle of the merge window is not the time to be moving files around and disagreeing on if the code is needed or not at all.
So, again, please drop the code from your trees (or whomever trees this code is in), resend it, notifying everyone involved, and we can take it from there.
And yes, to be clear, this isn't going to make 3.16, sorry.
thanks,
gre k-h
Greg Kroah-Hartman
maintainerQuoted 86 lines · Rob Clark
Quoted 84 lines · Greg Kroah-Hartman
Quoted 74 lines · Thierry Reding
Quoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4
-0@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*Quoted 2 lines
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
That doesn't really work for most people, I'll still be "responsible" for the code.
Quoted 2 lines
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
Quoted 8 lines
Quoted 4 lines
That might be best for some of this.
But again, why is the
fence.ccode needed at all anyway? I'm not sold on that.Fence serves as a way to synchronize between (for example) multiple asynchronous gpu's. There is definitely a need for this. Otherwise performance for optimus/prime type setups is going to suck.
What's wrong with the 'sync' code in the drivers/staging/android/ directory? I thought that is what that codebase was supposed to be doing.
Quoted 5 lines
I thought we had added something under
Documentation/about it, but I can't find it now (although possibly looking at the wrong trees).. there is at least a bit of a description in the commit msg:
Ah, so no documenation, no discussion, and you want to just throw it in a directory I am responsible for? That sounds like a major rush job...
Quoted 4 lines
I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
Ok, then let's give it a proper review cycle, and notify everyone involved. It doesn't look like this happened at all. We don't add core primitives to the kernel without a lot of discussion and agreement. And we sure don't add them without telling the person who owns the directory (again, my pet peeve, I know...)
greg k-h
Hi Greg,
Quoted 92 lines · Greg Kroah-Hartman
Quoted 91 lines · Rob Clark
Quoted 88 lines · Greg Kroah-Hartman
Quoted 74 lines · Thierry Reding
Quoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4
-0@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*Quoted 2 lines
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
That doesn't really work for most people, I'll still be "responsible" for the code.
Quoted 2 lines
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
That might be best for some of this.
But again, why is the
fence.ccode needed at all anyway? I'm not sold on that.
Quoted 8 lines
Quoted 3 lines
Fence serves as a way to synchronize between (for example) multiple asynchronous gpu's. There is definitely a need for this. Otherwise performance for optimus/prime type setups is going to suck.
What's wrong with the 'sync' code in the
drivers/staging/android/directory? I thought that is what that codebase was supposed to be doing.
(Just a quick recap on sync v/s fences):
Well, there was a discussion on Sync v/s Fences at LPC 2013 [1], and the agreement was that if (explicit) sync could be implemented on top of (implicitly synchronized) fences, fences might provide more flexibility.
Maarten implemented that (it is a part of this patch series), and Android guys confirmed that it worked the same way as expected in the Android system. That is a major reason for acceptance of fence as the solution.
Quoted 9 lines
Quoted 5 lines
I thought we had added something under
Documentation/about it, but I can't find it now (although possibly looking at the wrong trees).. there is at least a bit of a description in the commit msg:Ah, so no documenation, no discussion, and you want to just throw it in a directory I am responsible for? That sounds like a major rush job...
While I totally agree that it was an honest mistake to not have you notified in CC for the patch series, for your other observations, please allow me to answer one-by-one:
(1) No documentation: there is in-code documentation which is also
added to Documentation/DocBook/device-drivers.tmpl; the commit message has a lot of description about it. Of course, it could do with its own documentation as well.
(2) No discussion: the patch series is into v17 - v2 was posted in
July 2012, so it does seem like it has gone through a series of reviews - I would admit that it has been mostly limited to the people it seemed to matter the most for - the dri-devel and v4l communities.
Quoted 11 lines
Quoted 4 lines
I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
Ok, then let's give it a proper review cycle, and notify everyone involved. It doesn't look like this happened at all. We don't add core primitives to the kernel without a lot of discussion and agreement. And we sure don't add them without telling the person who owns the directory (again, my pet peeve, I know...)
And again, I do apologize that we all missed the fact that you weren't CC'ed onto the patch series.
Still, even in the wake of above information, if you feel we are not ready to take it for 3.16, I would drop it from my queue for 3.16 (though there are quite a few people who've waited long for this to land in).
Thanks, and best regards,
~Sumit.
Quoted 1 line
greg k-h
[1]: http://lwn.net/Articles/569704/
[2]: https://lkml.org/lkml/2012/7/13/270
Greg Kroah-Hartman
maintainerQuoted 14 lines · Sumit Semwal
Hi Greg,
Quoted 11 lines · Greg Kroah-Hartman
Quoted 6 lines
Quoted 2 lines
But again, why is the
fence.ccode needed at all anyway? I'm not sold on that.Fence serves as a way to synchronize between (for example) multiple asynchronous gpu's. There is definitely a need for this. Otherwise performance for optimus/prime type setups is going to suck.
What's wrong with the 'sync' code in the
drivers/staging/android/directory? I thought that is what that codebase was supposed to be doing.
Quoted 9 lines
(Just a quick recap on sync v/s fences):
Well, there was a discussion on Sync v/s Fences at LPC 2013 [1], and the agreement was that if (explicit) sync could be implemented on top of (implicitly synchronized) fences, fences might provide more flexibility.
Maarten implemented that (it is a part of this patch series), and Android guys confirmed that it worked the same way as expected in the Android system. That is a major reason for acceptance of fence as the solution.
That's great to hear, again, it should have been conveyed somehow :)
Quoted 9 lines
Quoted 9 lines
Quoted 5 lines
I thought we had added something under
Documentation/about it, but I can't find it now (although possibly looking at the wrong trees).. there is at least a bit of a description in the commit msg:Ah, so no documenation, no discussion, and you want to just throw it in a directory I am responsible for? That sounds like a major rush job...
Quoted 11 lines
While I totally agree that it was an honest mistake to not have you notified in CC for the patch series, for your other observations, please allow me to answer one-by-one:
(1) No documentation: there is in-code documentation which is also
added toDocumentation/DocBook/device-drivers.tmpl; the commit message has a lot of description about it. Of course, it could do with its own documentation as well.
(2) No discussion: the patch series is into v17 - v2 was posted in
July 2012, so it does seem like it has gone through a series of reviews - I would admit that it has been mostly limited to the people it seemed to matter the most for - the dri-devel and v4l communities.
Limiting the discussion to the people involved is fine, and great, but really, for a core kernel function, you need to pull in _some_ core kernel people to at least go over the api.
With just a quick glance, I already had api objections to how you were implementing things, those should be addressed at the least before the code is merged.
Quoted 11 lines
Quoted 11 lines
Quoted 4 lines
I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
Ok, then let's give it a proper review cycle, and notify everyone involved. It doesn't look like this happened at all. We don't add core primitives to the kernel without a lot of discussion and agreement. And we sure don't add them without telling the person who owns the directory (again, my pet peeve, I know...)
Quoted 7 lines
And again, I do apologize that we all missed the fact that you weren't CC'ed onto the patch series.
Still, even in the wake of above information, if you feel we are not ready to take it for 3.16, I would drop it from my queue for 3.16 (though there are quite a few people who've waited long for this to land in).
Given that I have yet to even be cc:ed on a patch, and the merge window is 1 week in, _and_ people are still discussing where to put the files, yes, it's too late for 3.16, sorry.
Please feel free to respin and resend after 3.16-rc1 is out. There is no "deadline" here that is requiring code to ever be merged. We do things correctly, not rushed.
thanks,
greg k-h
Quoted 92 lines · Greg Kroah-Hartman
Quoted 91 lines · Rob Clark
Quoted 88 lines · Greg Kroah-Hartman
Quoted 74 lines · Thierry Reding
Quoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
Perhaps something like the following would help?
MAINTAINERS +4
-0@@ -2867,7 +2867,9 @@ L: [email protected] L: [email protected] L: [email protected] F: drivers/base/dma-buf* +F: drivers/base/fence.c F: include/linux/dma-buf* +F: include/linux/fence.h F: Documentation/dma-buf-sharing.txt T: git git://git.linaro.org/people/sumitsemwal/linux-dma-buf.git@@ -2936,6 +2938,8 @@ T: git git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git S: Supported F: Documentation/kobject.txt F: drivers/base/ +X: drivers/base/dma-buf* +X: drivers/base/fence.c F: fs/sysfs/ F: fs/debugfs/ F: include/linux/kobj*Quoted 2 lines
That removes Greg from the list generated by get_maintainer.pl for anything that touches the DMA-BUF files.
That doesn't really work for most people, I'll still be "responsible" for the code.
Quoted 2 lines
Thinking about it, perhaps moving DMA-BUF into its own subdirectory would be an option too, to make the separation more obvious.
That might be best for some of this.
But again, why is the
fence.ccode needed at all anyway? I'm not sold on that.
Quoted 7 lines
Quoted 3 lines
Fence serves as a way to synchronize between (for example) multiple asynchronous gpu's. There is definitely a need for this. Otherwise performance for optimus/prime type setups is going to suck.
What's wrong with the 'sync' code in the
drivers/staging/android/directory? I thought that is what that codebase was supposed to be doing.
the 'explicit sync' approach of the android stuff really doesn't work out well for x11 or wayland userspace
The current fence stuff from Maarten is the result of coming up with a single mechanism that works both for "traditional" linux userspace, which can also sit beneath the android stuff. Not completely dissimilar to dma-buf vs ION..
Quoted 8 lines
Quoted 5 lines
I thought we had added something under
Documentation/about it, but I can't find it now (although possibly looking at the wrong trees).. there is at least a bit of a description in the commit msg:Ah, so no documenation, no discussion, and you want to just throw it in a directory I am responsible for? That sounds like a major rush job...
well, I mean beyond the docbook/headerdoc stuff (of which there is plenty for fence)
Quoted 10 lines
Quoted 4 lines
I don't think the question about whether we need something like fence to augment dma-buf is really in doubt. Maybe it should live somewhere else, I'm not sure. But it makes sense for it to live wherever dma-buf does, as they are intended to work together.
Ok, then let's give it a proper review cycle, and notify everyone involved. It doesn't look like this happened at all. We don't add core primitives to the kernel without a lot of discussion and agreement. And we sure don't add them without telling the person who owns the directory (again, my pet peeve, I know...)
In this case, the only thing missing was the one who owned the directory. The discussions were CC'd to linux-media, dri-devel, linaro-mm-sig, various android folks, linux-kernel... and discussed and discussed and discussed ;-)
I think the only controversial thing is where the code lives. Maybe one week in is too late to be moving things, in which case fine, 3.17.
But for such a superficial thing it seems a bit sad to let it slip
another cycle.
BR,
-R
Quoted 1 line
greg k-h
Greg Kroah-Hartman
maintainerQuoted 41 lines · Sumit Semwal
Hi Greg,
Quoted 19 lines · Greg Kroah-Hartman
Quoted 12 lines · Thierry Reding
Quoted 11 lines · Greg Kroah-Hartman
Quoted 9 lines · Thierry Reding
From: Thierry Reding <[email protected]>
Commit
febdbfe8a91c(arch: Prepare for smp_mb__{before,after}_atomic()) deprecated the smp_mb__{before,after}_{atomic,clear}_{dec,inc,bit}*() functions in favour of the unified smp_mb__{before,after}_atomic().Signed-off-by Thierry Reding <[email protected]>--- drivers/base/fence.c | 4 ++--Quoted 5 lines
Quoted 2 lines
Where does this file come from? I've not seen it before, and it's not in my tree.
I think it came in through Sumit's tree and it's only in linux-next I believe.
Quoted 8 lines
Odd, linux-next is for merging things in Linus's next release.
And as I have never seen this code that will end up being my responsibility to maintain, it seems strange that it will be merged in the next kernel development cycle.
What broke down here with our review process that required something to be merged without at least a cc: to me?
This is a new file added by Maarten's patches [1], that got reviewed on dri-devel and other mailing lists. Since it was quite closely associated with dma-buf, I figured I should take it through the dma-buf tree.
I am sorry I didn't notice that you weren't CC'ed on these patches - Sincere apologies, since I should've noticed that during the patch review process - I would take part of the blame here as well :(
Quoted 6 lines
I do realize now that atleast on my part, I should've asked you before taking it through the dma-buf tree - I will make sure things like this don't happen again through me.
May I request you to help us handle this - would it help if we add Maarten as the maintainer for this file? Any other suggestions?
I'd prefer the fence.c file not be in the driver core at all for now as it doesn't follow the rules for other files in that directory (symbol exports, __functions that are global, config option for no good reason, etc.)
Any chance you can just drop these and go through another review cycle? I'm not sold on even why this file is needed at all, and that our current kernel primitives don't work instead.
Doubly good reason is that you, or someone, is assuming that I was going to maintain this...
So, care to drop the series and have Maarten resend?
thanks,
greg k-h