* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 11:51 ` Rob Clark
@ 2014-06-05 12:00 ` Maarten Lankhorst
2014-06-05 15:52 ` Greg Kroah-Hartman
2014-06-05 12:26 ` Sumit Semwal
2014-06-05 15:48 ` Greg Kroah-Hartman
2 siblings, 1 reply; 18+ messages in thread
From: Maarten Lankhorst @ 2014-06-05 12:00 UTC (permalink / raw)
To: Rob Clark, Greg Kroah-Hartman; +Cc: Thierry Reding, Sumit Semwal, LKML
op 05-06-14 13:51, Rob Clark schreef:
> On Wed, Jun 4, 2014 at 1:49 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Wed, Jun 04, 2014 at 03:28:33PM +0200, Thierry Reding wrote:
>>> On Wed, Jun 04, 2014 at 04:57:07PM +0530, Sumit Semwal wrote:
>>>> Hi Greg,
>>>>
>>>>
>>>> On 30 May 2014 21:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>>>> On Fri, May 30, 2014 at 10:15:05AM +0200, Thierry Reding wrote:
>>>>>> On Wed, May 28, 2014 at 01:51:45PM -0700, Greg Kroah-Hartman wrote:
>>>>>>> On Wed, May 28, 2014 at 04:26:32PM +0200, Thierry Reding wrote:
>>>>>>>> From: Thierry Reding <treding@nvidia.com>
>>>>>>>>
>>>>>>>> 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 <treding@nvidia.com>
>>>>>>>> ---
>>>>>>>> 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.
>>>>> 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?
>>> Perhaps something like the following would help?
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index fb39c9c3f0c1..d582f54adec8 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2867,7 +2867,9 @@ L: linux-media@vger.kernel.org
>>> L: dri-devel@lists.freedesktop.org
>>> L: linaro-mm-sig@lists.linaro.org
>>> 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.
>> That doesn't really work for most people, I'll still be "responsible"
>> for the code.
>>
>>> 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.
> 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.
>
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 12:00 ` Maarten Lankhorst
@ 2014-06-05 15:52 ` Greg Kroah-Hartman
0 siblings, 0 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2014-06-05 15:52 UTC (permalink / raw)
To: Maarten Lankhorst; +Cc: Rob Clark, Thierry Reding, Sumit Semwal, LKML
On Thu, Jun 05, 2014 at 02:00:16PM +0200, Maarten Lankhorst wrote:
> >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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 11:51 ` Rob Clark
2014-06-05 12:00 ` Maarten Lankhorst
@ 2014-06-05 12:26 ` Sumit Semwal
2014-06-05 15:51 ` Greg Kroah-Hartman
2014-06-05 15:48 ` Greg Kroah-Hartman
2 siblings, 1 reply; 18+ messages in thread
From: Sumit Semwal @ 2014-06-05 12:26 UTC (permalink / raw)
To: Rob Clark; +Cc: Greg Kroah-Hartman, Thierry Reding, Maarten Lankhorst, LKML
Hi Greg,
On 5 June 2014 17:21, Rob Clark <robdclark@gmail.com> wrote:
> On Wed, Jun 4, 2014 at 1:49 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> On Wed, Jun 04, 2014 at 03:28:33PM +0200, Thierry Reding wrote:
>>> On Wed, Jun 04, 2014 at 04:57:07PM +0530, Sumit Semwal wrote:
>>> > Hi Greg,
>>> >
>>> >
>>> > On 30 May 2014 21:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>> > > On Fri, May 30, 2014 at 10:15:05AM +0200, Thierry Reding wrote:
>>> > >> On Wed, May 28, 2014 at 01:51:45PM -0700, Greg Kroah-Hartman wrote:
>>> > >> > On Wed, May 28, 2014 at 04:26:32PM +0200, Thierry Reding wrote:
>>> > >> > > From: Thierry Reding <treding@nvidia.com>
>>> > >> > >
>>> > >> > > 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 <treding@nvidia.com>
>>> > >> > > ---
>>> > >> > > 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.
>>> > >
>>> > > 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?
>>>
>>> Perhaps something like the following would help?
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index fb39c9c3f0c1..d582f54adec8 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -2867,7 +2867,9 @@ L: linux-media@vger.kernel.org
>>> L: dri-devel@lists.freedesktop.org
>>> L: linaro-mm-sig@lists.linaro.org
>>> 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.
>>
>> That doesn't really work for most people, I'll still be "responsible"
>> for the code.
>>
>>> 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.
>
> 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.
>
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?
> BR,
> -R
>
>
Best regards,
~Sumit.
>> greg k-h
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 12:26 ` Sumit Semwal
@ 2014-06-05 15:51 ` Greg Kroah-Hartman
0 siblings, 0 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2014-06-05 15:51 UTC (permalink / raw)
To: Sumit Semwal; +Cc: Rob Clark, Thierry Reding, Maarten Lankhorst, LKML
On Thu, Jun 05, 2014 at 05:56:36PM +0530, Sumit Semwal wrote:
> Hi Greg,
> 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),
See my disagreement there :)
> 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?
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 11:51 ` Rob Clark
2014-06-05 12:00 ` Maarten Lankhorst
2014-06-05 12:26 ` Sumit Semwal
@ 2014-06-05 15:48 ` Greg Kroah-Hartman
2014-06-05 16:39 ` Sumit Semwal
2014-06-05 21:56 ` Rob Clark
2 siblings, 2 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2014-06-05 15:48 UTC (permalink / raw)
To: Rob Clark; +Cc: Thierry Reding, Sumit Semwal, Maarten Lankhorst, LKML
On Thu, Jun 05, 2014 at 07:51:10AM -0400, Rob Clark wrote:
> On Wed, Jun 4, 2014 at 1:49 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 04, 2014 at 03:28:33PM +0200, Thierry Reding wrote:
> >> On Wed, Jun 04, 2014 at 04:57:07PM +0530, Sumit Semwal wrote:
> >> > Hi Greg,
> >> >
> >> >
> >> > On 30 May 2014 21:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >> > > On Fri, May 30, 2014 at 10:15:05AM +0200, Thierry Reding wrote:
> >> > >> On Wed, May 28, 2014 at 01:51:45PM -0700, Greg Kroah-Hartman wrote:
> >> > >> > On Wed, May 28, 2014 at 04:26:32PM +0200, Thierry Reding wrote:
> >> > >> > > From: Thierry Reding <treding@nvidia.com>
> >> > >> > >
> >> > >> > > 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 <treding@nvidia.com>
> >> > >> > > ---
> >> > >> > > 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.
> >> > >
> >> > > 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?
> >>
> >> Perhaps something like the following would help?
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index fb39c9c3f0c1..d582f54adec8 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -2867,7 +2867,9 @@ L: linux-media@vger.kernel.org
> >> L: dri-devel@lists.freedesktop.org
> >> L: linaro-mm-sig@lists.linaro.org
> >> 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.
> >
> > That doesn't really work for most people, I'll still be "responsible"
> > for the code.
> >
> >> 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.
>
> 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.
> 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
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...
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 15:48 ` Greg Kroah-Hartman
@ 2014-06-05 16:39 ` Sumit Semwal
2014-06-05 17:49 ` Greg Kroah-Hartman
2014-06-05 21:56 ` Rob Clark
1 sibling, 1 reply; 18+ messages in thread
From: Sumit Semwal @ 2014-06-05 16:39 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Rob Clark, Thierry Reding, Maarten Lankhorst, LKML
Hi Greg,
On 5 June 2014 21:18, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 05, 2014 at 07:51:10AM -0400, Rob Clark wrote:
>> On Wed, Jun 4, 2014 at 1:49 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Jun 04, 2014 at 03:28:33PM +0200, Thierry Reding wrote:
>> >> On Wed, Jun 04, 2014 at 04:57:07PM +0530, Sumit Semwal wrote:
>> >> > Hi Greg,
>> >> >
>> >> >
>> >> > On 30 May 2014 21:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>> >> > > On Fri, May 30, 2014 at 10:15:05AM +0200, Thierry Reding wrote:
>> >> > >> On Wed, May 28, 2014 at 01:51:45PM -0700, Greg Kroah-Hartman wrote:
>> >> > >> > On Wed, May 28, 2014 at 04:26:32PM +0200, Thierry Reding wrote:
>> >> > >> > > From: Thierry Reding <treding@nvidia.com>
>> >> > >> > >
>> >> > >> > > 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 <treding@nvidia.com>
>> >> > >> > > ---
>> >> > >> > > 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.
>> >> > >
>> >> > > 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?
>> >>
>> >> Perhaps something like the following would help?
>> >>
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index fb39c9c3f0c1..d582f54adec8 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -2867,7 +2867,9 @@ L: linux-media@vger.kernel.org
>> >> L: dri-devel@lists.freedesktop.org
>> >> L: linaro-mm-sig@lists.linaro.org
>> >> 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.
>> >
>> > That doesn't really work for most people, I'll still be "responsible"
>> > for the code.
>> >
>> >> 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.
>>
>> 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.
>> 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
>
> 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.
>> 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.
> greg k-h
[1]: http://lwn.net/Articles/569704/
[2]: https://lkml.org/lkml/2012/7/13/270
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 16:39 ` Sumit Semwal
@ 2014-06-05 17:49 ` Greg Kroah-Hartman
0 siblings, 0 replies; 18+ messages in thread
From: Greg Kroah-Hartman @ 2014-06-05 17:49 UTC (permalink / raw)
To: Sumit Semwal; +Cc: Rob Clark, Thierry Reding, Maarten Lankhorst, LKML
On Thu, Jun 05, 2014 at 10:09:29PM +0530, Sumit Semwal wrote:
> Hi Greg,
>
> On 5 June 2014 21:18, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >> > But again, why is the fence.c code 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.
> >
> (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 :)
> >> 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
> >
> > 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.
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.
> >> 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).
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] fence: Use smp_mb__before_atomic()
2014-06-05 15:48 ` Greg Kroah-Hartman
2014-06-05 16:39 ` Sumit Semwal
@ 2014-06-05 21:56 ` Rob Clark
1 sibling, 0 replies; 18+ messages in thread
From: Rob Clark @ 2014-06-05 21:56 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Thierry Reding, Sumit Semwal, Maarten Lankhorst, LKML
On Thu, Jun 5, 2014 at 11:48 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, Jun 05, 2014 at 07:51:10AM -0400, Rob Clark wrote:
>> On Wed, Jun 4, 2014 at 1:49 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Wed, Jun 04, 2014 at 03:28:33PM +0200, Thierry Reding wrote:
>> >> On Wed, Jun 04, 2014 at 04:57:07PM +0530, Sumit Semwal wrote:
>> >> > Hi Greg,
>> >> >
>> >> >
>> >> > On 30 May 2014 21:38, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>> >> > > On Fri, May 30, 2014 at 10:15:05AM +0200, Thierry Reding wrote:
>> >> > >> On Wed, May 28, 2014 at 01:51:45PM -0700, Greg Kroah-Hartman wrote:
>> >> > >> > On Wed, May 28, 2014 at 04:26:32PM +0200, Thierry Reding wrote:
>> >> > >> > > From: Thierry Reding <treding@nvidia.com>
>> >> > >> > >
>> >> > >> > > 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 <treding@nvidia.com>
>> >> > >> > > ---
>> >> > >> > > 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.
>> >> > >
>> >> > > 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?
>> >>
>> >> Perhaps something like the following would help?
>> >>
>> >> diff --git a/MAINTAINERS b/MAINTAINERS
>> >> index fb39c9c3f0c1..d582f54adec8 100644
>> >> --- a/MAINTAINERS
>> >> +++ b/MAINTAINERS
>> >> @@ -2867,7 +2867,9 @@ L: linux-media@vger.kernel.org
>> >> L: dri-devel@lists.freedesktop.org
>> >> L: linaro-mm-sig@lists.linaro.org
>> >> 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.
>> >
>> > That doesn't really work for most people, I'll still be "responsible"
>> > for the code.
>> >
>> >> 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.
>>
>> 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..
>> 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
>
> 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)
>> 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
> greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread