* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-29 2:21 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>
> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, May 29, 2013 1:50 AM
> To: Inki Dae
> Cc: Rob Clark; Maarten Lankhorst; linux-fbdev; YoungJun Cho; Kyungmin
Park;
> myungjoo.ham; DRI mailing list; linux-arm-kernel@lists.infradead.org;
> linux-media@vger.kernel.org
> Subject: Re: Introduce a new helper framework for buffer synchronization
>
> On Tue, May 28, 2013 at 4:50 PM, Inki Dae <inki.dae@samsung.com> wrote:
> > I think I already used reservation stuff any time in that way except
> > ww-mutex. And I'm not sure that embedded system really needs ww-mutex.
> If
> > there is any case,
> > could you tell me the case? I really need more advice and
> understanding :)
>
> If you have only one driver, you can get away without ww_mutex.
> drm/i915 does it, all buffer state is protected by dev->struct_mutex.
> But as soon as you have multiple drivers sharing buffers with dma_buf
> things will blow up.
>
> Yep, current prime is broken and can lead to deadlocks.
>
> In practice it doesn't (yet) matter since only the X server does the
> sharing dance, and that one's single-threaded. Now you can claim that
> since you have all buffers pinned in embedded gfx anyway, you don't
> care. But both in desktop gfx and embedded gfx the real fun starts
> once you put fences into the mix and link them up with buffers, then
> every command submission risks that deadlock. Furthermore you can get
> unlucky and construct a circle of fences waiting on each another (only
> though if the fence singalling fires off the next batchbuffer
> asynchronously).
In our case, we haven't ever experienced deadlock yet but there is still
possible to face with deadlock in case that a process is sharing two buffer
with another process like below,
Process A committed buffer A and waits for buffer B,
Process B committed buffer B and waits for buffer A
That is deadlock and it seems that you say we can resolve deadlock issue
with ww-mutexes. And it seems that we can replace our block-wakeup mechanism
with mutex lock for more performance.
>
> To prevent such deadlocks you _absolutely_ need to lock _all_ buffers
> that take part in a command submission at once. To do that you either
> need a global lock (ugh) or ww_mutexes.
>
> So ww_mutexes are the fundamental ingredient of all this, if you don't
> see why you need them then everything piled on top is broken. I think
> until you've understood why exactly we need ww_mutexes there's not
> much point in discussing the finer issues of fences, reservation
> objects and how to integrate it with dma_bufs exactly.
>
> I'll try to clarify the motivating example in the ww_mutex
> documentation a bit, but I dunno how else I could explain this ...
>
I don't really want for you waste your time on me. I will trying to apply
ww-mutexes (v4) to the proposed framework for more understanding.
Thanks for your advices.:)
Inki Dae
> Yours, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [RFC] Add co-maintainer for fbdev
From: Jingoo Han @ 2013-05-29 1:32 UTC (permalink / raw)
To: 'Arnd Bergmann', 'Florian Tobias Schandinat'
Cc: 'Olof Johansson',
'Jean-Christophe PLAGNIOL-VILLARD', linux-fbdev,
'Linus Torvalds', linux-kernel, 'Tomi Valkeinen',
'Andrew Morton', Jingoo Han
In-Reply-To: <201305282319.30311.arnd@arndb.de>
On Wednesday, May 29, 2013 6:20 AM, Arnd Bergmann wrote:
> On Monday 27 May 2013, Florian Tobias Schandinat wrote:
> > >> On Fri, May 24, 2013 at 8:38 AM, Jean-Christophe PLAGNIOL-VILLARD
> > >> <plagnioj@jcrosoft.com> wrote:
> > >>> Hi Florian,
> > >>>
> > >>> As you seems very busy I'd like to propose the help you to handle the
> > >>> fbdev subsystem to easier the rich of the fbdev patch to Linus
> > >>>
> > >>> As I'm working on fbdev on at91 and others and already Co-Maintain the
> > >>> at91 mach on ARM
> > >>>
> > >>> And if you are not willing to continue I could take over
> >
> > Yeah, it would be great if you could help, at the moment I get barely
> > any sleep, let alone that I could keep up with the majority of mail I
> > get. I'll let you decide whether you want to be sole maintainer or not.
>
> For any bigger subsystems, it's good practice to have two maintainers,
> even if one of them does all the work. I think it makes sense to have
> Jean-Christophe as the primary maintainer and owner of the git tree,
> but it may also be good to have you or Tomi listed as a second maintainer
> for times when Jean-Christophe isn't available.
Hi Arnd,
I agree with Arnd's opinion.
It looks good to have Florian or Tomi listed as a second maintainer.
Best regards,
Jingoo Han
>
> I don't know how many patches you'd expect per release, maybe just one
> person is enough after all.
>
> Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [RFC] Add co-maintainer for fbdev
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-29 0:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Florian Tobias Schandinat, Olof Johansson, linux-fbdev,
Linus Torvalds, linux-kernel@vger.kernel.org, Tomi Valkeinen,
Andrew Morton
In-Reply-To: <201305282319.30311.arnd@arndb.de>
On 23:19 Tue 28 May , Arnd Bergmann wrote:
> On Monday 27 May 2013, Florian Tobias Schandinat wrote:
> > >> On Fri, May 24, 2013 at 8:38 AM, Jean-Christophe PLAGNIOL-VILLARD
> > >> <plagnioj@jcrosoft.com> wrote:
> > >>> Hi Florian,
> > >>>
> > >>> As you seems very busy I'd like to propose the help you to handle the
> > >>> fbdev subsystem to easier the rich of the fbdev patch to Linus
> > >>>
> > >>> As I'm working on fbdev on at91 and others and already Co-Maintain the
> > >>> at91 mach on ARM
> > >>>
> > >>> And if you are not willing to continue I could take over
> >
> > Yeah, it would be great if you could help, at the moment I get barely
> > any sleep, let alone that I could keep up with the majority of mail I
> > get. I'll let you decide whether you want to be sole maintainer or not.
>
> For any bigger subsystems, it's good practice to have two maintainers,
> even if one of them does all the work. I think it makes sense to have
> Jean-Christophe as the primary maintainer and owner of the git tree,
> but it may also be good to have you or Tomi listed as a second maintainer
> for times when Jean-Christophe isn't available.
I agree with Arnd
As example I'll be mostly off first week of June.
Tomi Florian what do you think
I plan to send the patch + PULL to Linus end of this week before I work-off
For the record the new git tree is here
git://git.kernel.org/pub/scm/linux/kernel/git/plagnioj/linux-fbdev.git
Best Regrds,
J.
^ permalink raw reply
* Re: [RFC] Add co-maintainer for fbdev
From: Arnd Bergmann @ 2013-05-28 21:19 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: Olof Johansson, Jean-Christophe PLAGNIOL-VILLARD, linux-fbdev,
Linus Torvalds, linux-kernel@vger.kernel.org, Tomi Valkeinen,
Andrew Morton
In-Reply-To: <51A29DB9.7030505@gmx.de>
On Monday 27 May 2013, Florian Tobias Schandinat wrote:
> >> On Fri, May 24, 2013 at 8:38 AM, Jean-Christophe PLAGNIOL-VILLARD
> >> <plagnioj@jcrosoft.com> wrote:
> >>> Hi Florian,
> >>>
> >>> As you seems very busy I'd like to propose the help you to handle the
> >>> fbdev subsystem to easier the rich of the fbdev patch to Linus
> >>>
> >>> As I'm working on fbdev on at91 and others and already Co-Maintain the
> >>> at91 mach on ARM
> >>>
> >>> And if you are not willing to continue I could take over
>
> Yeah, it would be great if you could help, at the moment I get barely
> any sleep, let alone that I could keep up with the majority of mail I
> get. I'll let you decide whether you want to be sole maintainer or not.
For any bigger subsystems, it's good practice to have two maintainers,
even if one of them does all the work. I think it makes sense to have
Jean-Christophe as the primary maintainer and owner of the git tree,
but it may also be good to have you or Tomi listed as a second maintainer
for times when Jean-Christophe isn't available.
I don't know how many patches you'd expect per release, maybe just one
person is enough after all.
Arnd
^ permalink raw reply
* Re: Introduce a new helper framework for buffer synchronization
From: Daniel Vetter @ 2013-05-28 16:49 UTC (permalink / raw)
To: Inki Dae
Cc: Rob Clark, Maarten Lankhorst, linux-fbdev, YoungJun Cho,
Kyungmin Park, myungjoo.ham, DRI mailing list,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <00de01ce5bb2$aa7c6b30$ff754190$%dae@samsung.com>
On Tue, May 28, 2013 at 4:50 PM, Inki Dae <inki.dae@samsung.com> wrote:
> I think I already used reservation stuff any time in that way except
> ww-mutex. And I'm not sure that embedded system really needs ww-mutex. If
> there is any case,
> could you tell me the case? I really need more advice and understanding :)
If you have only one driver, you can get away without ww_mutex.
drm/i915 does it, all buffer state is protected by dev->struct_mutex.
But as soon as you have multiple drivers sharing buffers with dma_buf
things will blow up.
Yep, current prime is broken and can lead to deadlocks.
In practice it doesn't (yet) matter since only the X server does the
sharing dance, and that one's single-threaded. Now you can claim that
since you have all buffers pinned in embedded gfx anyway, you don't
care. But both in desktop gfx and embedded gfx the real fun starts
once you put fences into the mix and link them up with buffers, then
every command submission risks that deadlock. Furthermore you can get
unlucky and construct a circle of fences waiting on each another (only
though if the fence singalling fires off the next batchbuffer
asynchronously).
To prevent such deadlocks you _absolutely_ need to lock _all_ buffers
that take part in a command submission at once. To do that you either
need a global lock (ugh) or ww_mutexes.
So ww_mutexes are the fundamental ingredient of all this, if you don't
see why you need them then everything piled on top is broken. I think
until you've understood why exactly we need ww_mutexes there's not
much point in discussing the finer issues of fences, reservation
objects and how to integrate it with dma_bufs exactly.
I'll try to clarify the motivating example in the ww_mutex
documentation a bit, but I dunno how else I could explain this ...
Yours, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH] video: mxsfb: Let device core handle pinctrl
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-28 15:25 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1369619294-10362-1-git-send-email-festevam@gmail.com>
On 12:09 Tue 28 May , Fabio Estevam wrote:
> Hi Jean-Christophe,
>
> On Mon, May 27, 2013 at 11:44 AM, Jean-Christophe PLAGNIOL-VILLARD
> <plagnioj@jcrosoft.com> wrote:
> > On 22:48 Sun 26 May , Fabio Estevam wrote:
> >> From: Fabio Estevam <fabio.estevam@freescale.com>
> >>
> >> Since commit ab78029 (drivers/pinctrl: grab default handles from device core)
> >> we can rely on device core for handling pinctrl, so remove
> >> devm_pinctrl_get_select_default() from the driver.
> > Linus we should do a pass on the kernel to clean this
>
> Not sure which 'Linus' the comment was meant to be directed to, but I
> have already sent several patches removing
> devm_pinctrl_get_select_default() and many of them were already
> applied by the maintainers.
here I mean linusw
Best Regards,
J.
^ permalink raw reply
* Re: [PATCH] video: mxsfb: Let device core handle pinctrl
From: Fabio Estevam @ 2013-05-28 15:09 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1369619294-10362-1-git-send-email-festevam@gmail.com>
Hi Jean-Christophe,
On Mon, May 27, 2013 at 11:44 AM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> On 22:48 Sun 26 May , Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> Since commit ab78029 (drivers/pinctrl: grab default handles from device core)
>> we can rely on device core for handling pinctrl, so remove
>> devm_pinctrl_get_select_default() from the driver.
> Linus we should do a pass on the kernel to clean this
Not sure which 'Linus' the comment was meant to be directed to, but I
have already sent several patches removing
devm_pinctrl_get_select_default() and many of them were already
applied by the maintainers.
Regards,
Fabio Estevam
^ permalink raw reply
* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-28 14:50 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>
> -----Original Message-----
> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> owner@vger.kernel.org] On Behalf Of Rob Clark
> Sent: Tuesday, May 28, 2013 10:49 PM
> To: Inki Dae
> Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho; Kyungmin
> Park; myungjoo.ham; DRI mailing list;
linux-arm-kernel@lists.infradead.org;
> linux-media@vger.kernel.org
> Subject: Re: Introduce a new helper framework for buffer synchronization
>
> On Mon, May 27, 2013 at 11:56 PM, Inki Dae <inki.dae@samsung.com> wrote:
> >
> >
> >> -----Original Message-----
> >> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> >> owner@vger.kernel.org] On Behalf Of Rob Clark
> >> Sent: Tuesday, May 28, 2013 12:48 AM
> >> To: Inki Dae
> >> Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho;
> Kyungmin
> >> Park; myungjoo.ham; DRI mailing list;
> > linux-arm-kernel@lists.infradead.org;
> >> linux-media@vger.kernel.org
> >> Subject: Re: Introduce a new helper framework for buffer
> synchronization
> >>
> >> On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com> wrote:
> >> > Hi all,
> >> >
> >> > I have been removed previous branch and added new one with more
> cleanup.
> >> > This time, the fence helper doesn't include user side interfaces and
> >> cache
> >> > operation relevant codes anymore because not only we are not sure
> that
> >> > coupling those two things, synchronizing caches and buffer access
> >> between
> >> > CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side
> is
> >> a
> >> > good idea yet but also existing codes for user side have problems
> with
> >> badly
> >> > behaved or crashing userspace. So this could be more discussed later.
> >> >
> >> > The below is a new branch,
> >> >
> >> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> >> exynos.git/?h=dma-f
> >> > ence-helper
> >> >
> >> > And fence helper codes,
> >> >
> >> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> >> exynos.git/commit/?
> >> > h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
> >> >
> >> > And example codes for device driver,
> >> >
> >> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> >> exynos.git/commit/?
> >> > h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
> >> >
> >> > I think the time is not yet ripe for RFC posting: maybe existing dma
> >> fence
> >> > and reservation need more review and addition work. So I'd glad for
> >> somebody
> >> > giving other opinions and advices in advance before RFC posting.
> >>
> >> thoughts from a *really* quick, pre-coffee, first look:
> >> * any sort of helper to simplify single-buffer sort of use-cases (v4l)
> >> probably wouldn't want to bake in assumption that seqno_fence is used.
> >> * I guess g2d is probably not actually a simple use case, since I
> >> expect you can submit blits involving multiple buffers :-P
> >
> > I don't think so. One and more buffers can be used: seqno_fence also has
> > only one buffer. Actually, we have already applied this approach to most
> > devices; multimedia, gpu and display controller. And this approach shows
> > more performance; reduced power consumption against traditional way. And
> g2d
> > example is just to show you how to apply my approach to device driver.
>
> no, you need the ww-mutex / reservation stuff any time you have
> multiple independent devices (or rings/contexts for hw that can
> support multiple contexts) which can do operations with multiple
> buffers.
I think I already used reservation stuff any time in that way except
ww-mutex. And I'm not sure that embedded system really needs ww-mutex. If
there is any case,
could you tell me the case? I really need more advice and understanding :)
Thanks,
Inki Dae
So you could conceivably hit this w/ gpu + g2d if multiple
> buffers where shared between the two. vram migration and such
> 'desktop stuff' might make the problem worse, but just because you
> don't have vram doesn't mean you don't have a problem with multiple
> buffers.
>
> >> * otherwise, you probably don't want to depend on dmabuf, which is why
> >> reservation/fence is split out the way it is.. you want to be able to
> >> use a single reservation/fence mechanism within your driver without
> >> having to care about which buffers are exported to dmabuf's and which
> >> are not. Creating a dmabuf for every GEM bo is too heavyweight.
> >
> > Right. But I think we should dealt with this separately. Actually, we
> are
> > trying to use reservation for gpu pipe line synchronization such as sgx
> sync
> > object and this approach is used without dmabuf. In order words, some
> device
> > can use only reservation for such pipe line synchronization and at the
> same
> > time, fence helper or similar thing with dmabuf for buffer
> synchronization.
>
> it is probably easier to approach from the reverse direction.. ie, get
> reservation/synchronization right first, and then dmabuf. (Well, that
> isn't really a problem because Maarten's reservation/fence patches
> support dmabuf from the beginning.)
>
> BR,
> -R
>
> >>
> >> I'm not entirely sure if reservation/fence could/should be made any
> >> simpler for multi-buffer users. Probably the best thing to do is just
> >> get reservation/fence rolled out in a few drivers and see if some
> >> common patterns emerge.
> >>
> >> BR,
> >> -R
> >>
> >> >
> >> > Thanks,
> >> > Inki Dae
> >> >
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-fbdev"
> in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-28 14:43 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>
Hi Daniel,
Thank you so much. And so very useful.:) Sorry but could be give me more
comments to the below my comments? There are still things making me
confusing.:(
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Tuesday, May 28, 2013 7:33 PM
> To: Inki Dae
> Cc: 'Rob Clark'; 'Maarten Lankhorst'; 'Daniel Vetter'; 'linux-fbdev';
> 'YoungJun Cho'; 'Kyungmin Park'; 'myungjoo.ham'; 'DRI mailing list';
> linux-arm-kernel@lists.infradead.org; linux-media@vger.kernel.org
> Subject: Re: Introduce a new helper framework for buffer synchronization
>
> On Tue, May 28, 2013 at 12:56:57PM +0900, Inki Dae wrote:
> >
> >
> > > -----Original Message-----
> > > From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> > > owner@vger.kernel.org] On Behalf Of Rob Clark
> > > Sent: Tuesday, May 28, 2013 12:48 AM
> > > To: Inki Dae
> > > Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho;
> Kyungmin
> > > Park; myungjoo.ham; DRI mailing list;
> > linux-arm-kernel@lists.infradead.org;
> > > linux-media@vger.kernel.org
> > > Subject: Re: Introduce a new helper framework for buffer
> synchronization
> > >
> > > On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com>
wrote:
> > > > Hi all,
> > > >
> > > > I have been removed previous branch and added new one with more
> cleanup.
> > > > This time, the fence helper doesn't include user side interfaces and
> > > cache
> > > > operation relevant codes anymore because not only we are not sure
> that
> > > > coupling those two things, synchronizing caches and buffer access
> > > between
> > > > CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel
> side is
> > > a
> > > > good idea yet but also existing codes for user side have problems
> with
> > > badly
> > > > behaved or crashing userspace. So this could be more discussed
later.
> > > >
> > > > The below is a new branch,
> > > >
> > > > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> > > exynos.git/?h=dma-f
> > > > ence-helper
> > > >
> > > > And fence helper codes,
> > > >
> > > > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> > > exynos.git/commit/?
> > > > h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
> > > >
> > > > And example codes for device driver,
> > > >
> > > > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> > > exynos.git/commit/?
> > > > h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
> > > >
> > > > I think the time is not yet ripe for RFC posting: maybe existing dma
> > > fence
> > > > and reservation need more review and addition work. So I'd glad for
> > > somebody
> > > > giving other opinions and advices in advance before RFC posting.
> > >
> > > thoughts from a *really* quick, pre-coffee, first look:
> > > * any sort of helper to simplify single-buffer sort of use-cases (v4l)
> > > probably wouldn't want to bake in assumption that seqno_fence is used.
> > > * I guess g2d is probably not actually a simple use case, since I
> > > expect you can submit blits involving multiple buffers :-P
> >
> > I don't think so. One and more buffers can be used: seqno_fence also has
> > only one buffer. Actually, we have already applied this approach to most
> > devices; multimedia, gpu and display controller. And this approach shows
> > more performance; reduced power consumption against traditional way. And
> g2d
> > example is just to show you how to apply my approach to device driver.
>
> Note that seqno_fence is an implementation pattern for a certain type of
> direct hw->hw synchronization which uses a shared dma_buf to exchange the
> sync cookie.
I'm afraid that I don't understand hw->hw synchronization. hw->hw
synchronization means that device has a hardware feature which supports
buffer synchronization hardware internally? And what is the sync cookie?
> The dma_buf attached to the seqno_fence has _nothing_ to do
> with the dma_buf the fence actually coordinates access to.
>
> I think that confusing is a large reason for why Maarten&I don't
> understand what you want to achieve with your fence helpers. Currently
> they're using the seqno_fence, but totally not in a way the seqno_fence
> was meant to be used.
>
> Note that with the current code there is only a pointer from dma_bufs to
> the fence. The fence itself has _no_ pointer to the dma_buf it syncs. This
> shouldn't be a problem since the fence fastpath for already signalled
> fences is completely barrier&lock free (it's just a load+bit-test), and
> fences are meant to be embedded into whatever dma tracking structure you
> already have, so no overhead there. The only ugly part is the fence
> refcounting, but I don't think we can drop that.
The below is the proposed way,
dma device has to create a fence before accessing a shared buffer, and then
check if there are other dma which are accessing the shared buffer; if exist
then the dma device should be blocked, and then it sets the fence to
reservation object of the shared buffer. And then the dma begins access to
the shared buffer. And finally, the dma signals its own fence so that other
blocked can be waked up. However, if there was another dma blocked before
signaling then another dma can access invalid fence object after waked up
because the fence object can be released after signaling. So I made another
dma takes a one reference before blocked. And I think origin version also
takes a reference at ticket_commit(). Is there wrong point in proposed way?
>
> Note that you completely reinvent this part of Maarten's fence patches by
> adding new r/w_complete completions to the reservation object, which
> completely replaces the fence stuff.
>
> Also note that a list of reservation entries is again meant to be used
> only when submitting the dma to the gpu. With your patches you seem to
> hang onto that list until dma completes.
Yeah, that is for prevent a dma from accessing a shared buffer while the
other is accessing the shared buffer. Maybe this way means software
synchronization. And your saying seems that there is another mechanism for
dealing with such thing. Could you give me more comments for it? is there
really another mechanism?
> This has the ugly side-effect
> that you need to allocate these reservation entries with kmalloc, whereas
> if you just use them in the execbuf ioctl to construct the dma you can
> usually embed it into something else you need already anyway. At least
> i915 and ttm based drivers can work that way.
>
Maybe there is my misunderstanding but I thought now dma fence and relevant
stubs tend to depend on Desktop world, especially x86 gpu. So I thought we
need a little bit customizing the dma fence for embedded system.
> Furthermore fences are specifically constructed as frankenstein-monsters
> between completion/waitqueues and callbacks. All the different use-cases
> need the different aspects:
> - busy/idle checks and bo retiring need the completion semantics
> - callbacks (in interrupt context) are used for hybrid hw->irq handler->hw
> sync approaches
>
> >
> > > * otherwise, you probably don't want to depend on dmabuf, which is why
> > > reservation/fence is split out the way it is.. you want to be able to
> > > use a single reservation/fence mechanism within your driver without
> > > having to care about which buffers are exported to dmabuf's and which
> > > are not. Creating a dmabuf for every GEM bo is too heavyweight.
> >
> > Right. But I think we should dealt with this separately. Actually, we
> are
> > trying to use reservation for gpu pipe line synchronization such as sgx
> sync
> > object and this approach is used without dmabuf. In order words, some
> device
> > can use only reservation for such pipe line synchronization and at the
> same
> > time, fence helper or similar thing with dmabuf for buffer
> synchronization.
>
> I think a quick overview of the different pieces would be in order.
>
> - ww_mutex: This is just the locking primite which allows you to grab
> multiple mutexes without the need to check for ordering (and so fear
> deadlocks).
Should ww_mutex be necessarily used for embedded system? I'm not sure that
there are any cases we have to use this feature like x86 gpu. That is also
why I removed ticket stubs from existing reservation framework.
>
> - fence: This is just a fancy kind of completion with a bit of support for
> hw->hw completion events.
hw->hw completion event means completion of dma operation? Otherwise,
hardware buffer synchronization feature? If the latter, how fence can be
used for embedded system which has no hardware feature for buffer
synchronization? Actually, that made me confusing at initial work.
> The fences themselve have no tie-in with
> buffers, ww_mutexes or anything else.
>
> - reservation: This ties together an object (doesn't need to be a buffer,
> could be any other gpu resource - see the drm/vmwgfx driver if you want
> your mind blown) with fences. Note that there's no connection the other
> way round, i.e. with the current patches you can't see which
> reservations are using which fences. Also note that other objects than
> reservations could point at fences, e.g. when the provide
> shared/exclusive semantics are not good enough for your needs.
>
> The ww_mutex in the reservation is simply the (fancy) lock which
> protects each reservation. The reason we need something fancy is that
> you need to lock all objects which are synced by the same fence
> toghether, otherwise you can race and construct deadlocks in the hw->hw
> sync part of fences.
>
> - dma_buf integration: This is simply a pointer to the reservation object
> of the underlying buffer object. We need a pointer here since otherwise
> you can construct deadlocks between dma_buf objects and native buffer
> objects.
>
> The crux is also that dma_buf integration comes last - before you can do
> that you need to have your driver converted over to use
> ww_mutexes/fences/reservations.
>
> I hope that helps to unconfuse things a bit and help you understand the
> different pieces of the fence/reservation/ww_mutex patches floating
> around.
Thank you again.:)
Thanks,
Inki Dae
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: Introduce a new helper framework for buffer synchronization
From: Rob Clark @ 2013-05-28 13:48 UTC (permalink / raw)
To: Inki Dae
Cc: Maarten Lankhorst, Daniel Vetter, linux-fbdev, YoungJun Cho,
Kyungmin Park, myungjoo.ham, DRI mailing list, linux-arm-kernel,
linux-media
In-Reply-To: <005701ce5b57$667c7d40$337577c0$%dae@samsung.com>
On Mon, May 27, 2013 at 11:56 PM, Inki Dae <inki.dae@samsung.com> wrote:
>
>
>> -----Original Message-----
>> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
>> owner@vger.kernel.org] On Behalf Of Rob Clark
>> Sent: Tuesday, May 28, 2013 12:48 AM
>> To: Inki Dae
>> Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho; Kyungmin
>> Park; myungjoo.ham; DRI mailing list;
> linux-arm-kernel@lists.infradead.org;
>> linux-media@vger.kernel.org
>> Subject: Re: Introduce a new helper framework for buffer synchronization
>>
>> On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> > Hi all,
>> >
>> > I have been removed previous branch and added new one with more cleanup.
>> > This time, the fence helper doesn't include user side interfaces and
>> cache
>> > operation relevant codes anymore because not only we are not sure that
>> > coupling those two things, synchronizing caches and buffer access
>> between
>> > CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
>> a
>> > good idea yet but also existing codes for user side have problems with
>> badly
>> > behaved or crashing userspace. So this could be more discussed later.
>> >
>> > The below is a new branch,
>> >
>> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/?h=dma-f
>> > ence-helper
>> >
>> > And fence helper codes,
>> >
>> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/commit/?
>> > h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
>> >
>> > And example codes for device driver,
>> >
>> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/commit/?
>> > h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
>> >
>> > I think the time is not yet ripe for RFC posting: maybe existing dma
>> fence
>> > and reservation need more review and addition work. So I'd glad for
>> somebody
>> > giving other opinions and advices in advance before RFC posting.
>>
>> thoughts from a *really* quick, pre-coffee, first look:
>> * any sort of helper to simplify single-buffer sort of use-cases (v4l)
>> probably wouldn't want to bake in assumption that seqno_fence is used.
>> * I guess g2d is probably not actually a simple use case, since I
>> expect you can submit blits involving multiple buffers :-P
>
> I don't think so. One and more buffers can be used: seqno_fence also has
> only one buffer. Actually, we have already applied this approach to most
> devices; multimedia, gpu and display controller. And this approach shows
> more performance; reduced power consumption against traditional way. And g2d
> example is just to show you how to apply my approach to device driver.
no, you need the ww-mutex / reservation stuff any time you have
multiple independent devices (or rings/contexts for hw that can
support multiple contexts) which can do operations with multiple
buffers. So you could conceivably hit this w/ gpu + g2d if multiple
buffers where shared between the two. vram migration and such
'desktop stuff' might make the problem worse, but just because you
don't have vram doesn't mean you don't have a problem with multiple
buffers.
>> * otherwise, you probably don't want to depend on dmabuf, which is why
>> reservation/fence is split out the way it is.. you want to be able to
>> use a single reservation/fence mechanism within your driver without
>> having to care about which buffers are exported to dmabuf's and which
>> are not. Creating a dmabuf for every GEM bo is too heavyweight.
>
> Right. But I think we should dealt with this separately. Actually, we are
> trying to use reservation for gpu pipe line synchronization such as sgx sync
> object and this approach is used without dmabuf. In order words, some device
> can use only reservation for such pipe line synchronization and at the same
> time, fence helper or similar thing with dmabuf for buffer synchronization.
it is probably easier to approach from the reverse direction.. ie, get
reservation/synchronization right first, and then dmabuf. (Well, that
isn't really a problem because Maarten's reservation/fence patches
support dmabuf from the beginning.)
BR,
-R
>>
>> I'm not entirely sure if reservation/fence could/should be made any
>> simpler for multi-buffer users. Probably the best thing to do is just
>> get reservation/fence rolled out in a few drivers and see if some
>> common patterns emerge.
>>
>> BR,
>> -R
>>
>> >
>> > Thanks,
>> > Inki Dae
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: fb_monspecs and device tree
From: Richard Genoud @ 2013-05-28 11:37 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Steffen Trumtrar,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat
In-Reply-To: <20130528104748.GL1952-RQcB7r2h9QmfDR2tN2SG5Ni2O/JbrIOy@public.gmane.org>
2013/5/28 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
> On 11:41 Tue 28 May , Richard Genoud wrote:
>> Hi !
>>
>> I've been using the display-timings device tree node (great job !) on
>> a sam9x5 soc.
>>
>> I was wondering if there's a planned support for struct fb_monspecs in
>> device tree, (or if it is irrelevant), if someone is already working
>> on that etc...
>
> I'm working on this but not on fbdev on DRI/KMS
> so we can have a full support of the overlay & co
> but I'm going to change complely the dt binding with the one present on
> github
>
> beawre the lcdc is already done on fbdev with display-timings
>
> but I'll be off the first week on June so wait a few for sam9x5
>
> Best Regards,
> J.
ok, that's good news !
thanks.
^ permalink raw reply
* Re: [PATCH v6] video: imxfb: Add DT support
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-28 11:09 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1369564538-21835-1-git-send-email-mpa@pengutronix.de>
On 12:35 Sun 26 May , Markus Pargmann wrote:
> Add devicetree support for imx framebuffer driver. It uses the generic
> display bindings and helper functions.
>
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Applied thanks
Best Regards,
J.
> ---
>
> Notes:
> Changed in v6:
> - Rebased onto v3.10-rc2
> - Dropped alpha support patch in this series
>
> Changed in v5:
> - Fix compatible property of the example
> - Rename property fsl,bpp to bits-per-pixel
> - Add selects for FB_MODE_HELPERS and VIDEOMODE_HELPERS to Kconfig FB_IMX
>
> Changes in v4:
> - Remove eukrea specific dmacr property.
> - Add optional dmacr property. If not present, the dmacr reset value is not
> changed.
>
> Changes in v3:
> - Fix returncodes of of_read_mode function and print error messages
> - Introduce a lower bound check for bits per pixel
> - Calculate correct bytes per pixel value before using it for the calculation of
> memory size
> - Change DT property names
>
> Changes in v2:
> - Removed pwmr register property
> - Cleanup of devicetree binding documentation
> - Use default values for pwmr and lscr1
>
> .../devicetree/bindings/video/fsl,imx-fb.txt | 51 ++++++
> drivers/video/Kconfig | 2 +
> drivers/video/imxfb.c | 194 +++++++++++++++++----
> 3 files changed, 212 insertions(+), 35 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/fsl,imx-fb.txt
>
> diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> new file mode 100644
> index 0000000..46da08d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
> @@ -0,0 +1,51 @@
> +Freescale imx21 Framebuffer
> +
> +This framebuffer driver supports devices imx1, imx21, imx25, and imx27.
> +
> +Required properties:
> +- compatible : "fsl,<chip>-fb", chip should be imx1 or imx21
> +- reg : Should contain 1 register ranges(address and length)
> +- interrupts : One interrupt of the fb dev
> +
> +Required nodes:
> +- display: Phandle to a display node as described in
> + Documentation/devicetree/bindings/video/display-timing.txt
> + Additional, the display node has to define properties:
> + - bits-per-pixel: Bits per pixel
> + - fsl,pcr: LCDC PCR value
> +
> +Optional properties:
> +- fsl,dmacr: DMA Control Register value. This is optional. By default, the
> + register is not modified as recommended by the datasheet.
> +- fsl,lscr1: LCDC Sharp Configuration Register value.
> +
> +Example:
> +
> + imxfb: fb@10021000 {
> + compatible = "fsl,imx21-fb";
> + interrupts = <61>;
> + reg = <0x10021000 0x1000>;
> + display = <&display0>;
> + };
> +
> + ...
> +
> + display0: display0 {
> + model = "Primeview-PD050VL1";
> + native-mode = <&timing_disp0>;
> + bits-per-pixel = <16>;
> + fsl,pcr = <0xf0c88080>; /* non-standard but required */
> + display-timings {
> + timing_disp0: 640x480 {
> + hactive = <640>;
> + vactive = <480>;
> + hback-porch = <112>;
> + hfront-porch = <36>;
> + hsync-len = <32>;
> + vback-porch = <33>;
> + vfront-porch = <33>;
> + vsync-len = <2>;
> + clock-frequency = <25000000>;
> + };
> + };
> + };
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index d71d60f..588af1d 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -367,6 +367,8 @@ config FB_IMX
> select FB_CFB_FILLRECT
> select FB_CFB_COPYAREA
> select FB_CFB_IMAGEBLIT
> + select FB_MODE_HELPERS
> + select VIDEOMODE_HELPERS
>
> config FB_CYBER2000
> tristate "CyberPro 2000/2010/5000 support"
> diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
> index 0abf2bf..a2ed71d 100644
> --- a/drivers/video/imxfb.c
> +++ b/drivers/video/imxfb.c
> @@ -31,6 +31,12 @@
> #include <linux/dma-mapping.h>
> #include <linux/io.h>
> #include <linux/math64.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +
> +#include <video/of_display_timing.h>
> +#include <video/of_videomode.h>
> +#include <video/videomode.h>
>
> #include <linux/platform_data/video-imxfb.h>
>
> @@ -112,10 +118,11 @@
> #define LCDISR_EOF (1<<1)
> #define LCDISR_BOF (1<<0)
>
> +#define IMXFB_LSCR1_DEFAULT 0x00120300
> +
> /* Used fb-mode. Can be set on kernel command line, therefore file-static. */
> static const char *fb_mode;
>
> -
> /*
> * These are the bitfields for each
> * display depth that we support.
> @@ -187,6 +194,19 @@ static struct platform_device_id imxfb_devtype[] = {
> };
> MODULE_DEVICE_TABLE(platform, imxfb_devtype);
>
> +static struct of_device_id imxfb_of_dev_id[] = {
> + {
> + .compatible = "fsl,imx1-fb",
> + .data = &imxfb_devtype[IMX1_FB],
> + }, {
> + .compatible = "fsl,imx21-fb",
> + .data = &imxfb_devtype[IMX21_FB],
> + }, {
> + /* sentinel */
> + }
> +};
> +MODULE_DEVICE_TABLE(of, imxfb_of_dev_id);
> +
> static inline int is_imx1_fb(struct imxfb_info *fbi)
> {
> return fbi->devtype = IMX1_FB;
> @@ -319,6 +339,9 @@ static const struct imx_fb_videomode *imxfb_find_mode(struct imxfb_info *fbi)
> struct imx_fb_videomode *m;
> int i;
>
> + if (!fb_mode)
> + return &fbi->mode[0];
> +
> for (i = 0, m = &fbi->mode[0]; i < fbi->num_modes; i++, m++) {
> if (!strcmp(m->mode.name, fb_mode))
> return m;
> @@ -474,6 +497,9 @@ static int imxfb_bl_update_status(struct backlight_device *bl)
> struct imxfb_info *fbi = bl_get_data(bl);
> int brightness = bl->props.brightness;
>
> + if (!fbi->pwmr)
> + return 0;
> +
> if (bl->props.power != FB_BLANK_UNBLANK)
> brightness = 0;
> if (bl->props.fb_blank != FB_BLANK_UNBLANK)
> @@ -684,10 +710,14 @@ static int imxfb_activate_var(struct fb_var_screeninfo *var, struct fb_info *inf
>
> writel(fbi->pcr, fbi->regs + LCDC_PCR);
> #ifndef PWMR_BACKLIGHT_AVAILABLE
> - writel(fbi->pwmr, fbi->regs + LCDC_PWMR);
> + if (fbi->pwmr)
> + writel(fbi->pwmr, fbi->regs + LCDC_PWMR);
> #endif
> writel(fbi->lscr1, fbi->regs + LCDC_LSCR1);
> - writel(fbi->dmacr, fbi->regs + LCDC_DMACR);
> +
> + /* dmacr = 0 is no valid value, as we need DMA control marks. */
> + if (fbi->dmacr)
> + writel(fbi->dmacr, fbi->regs + LCDC_DMACR);
>
> return 0;
> }
> @@ -723,13 +753,12 @@ static int imxfb_resume(struct platform_device *dev)
> #define imxfb_resume NULL
> #endif
>
> -static int __init imxfb_init_fbinfo(struct platform_device *pdev)
> +static int imxfb_init_fbinfo(struct platform_device *pdev)
> {
> struct imx_fb_platform_data *pdata = pdev->dev.platform_data;
> struct fb_info *info = dev_get_drvdata(&pdev->dev);
> struct imxfb_info *fbi = info->par;
> - struct imx_fb_videomode *m;
> - int i;
> + struct device_node *np;
>
> pr_debug("%s\n",__func__);
>
> @@ -760,41 +789,95 @@ static int __init imxfb_init_fbinfo(struct platform_device *pdev)
> info->fbops = &imxfb_ops;
> info->flags = FBINFO_FLAG_DEFAULT |
> FBINFO_READS_FAST;
> - info->var.grayscale = pdata->cmap_greyscale;
> - fbi->cmap_inverse = pdata->cmap_inverse;
> - fbi->cmap_static = pdata->cmap_static;
> - fbi->lscr1 = pdata->lscr1;
> - fbi->dmacr = pdata->dmacr;
> - fbi->pwmr = pdata->pwmr;
> - fbi->lcd_power = pdata->lcd_power;
> - fbi->backlight_power = pdata->backlight_power;
> -
> - for (i = 0, m = &pdata->mode[0]; i < pdata->num_modes; i++, m++)
> - info->fix.smem_len = max_t(size_t, info->fix.smem_len,
> - m->mode.xres * m->mode.yres * m->bpp / 8);
> + if (pdata) {
> + info->var.grayscale = pdata->cmap_greyscale;
> + fbi->cmap_inverse = pdata->cmap_inverse;
> + fbi->cmap_static = pdata->cmap_static;
> + fbi->lscr1 = pdata->lscr1;
> + fbi->dmacr = pdata->dmacr;
> + fbi->pwmr = pdata->pwmr;
> + fbi->lcd_power = pdata->lcd_power;
> + fbi->backlight_power = pdata->backlight_power;
> + } else {
> + np = pdev->dev.of_node;
> + info->var.grayscale = of_property_read_bool(np,
> + "cmap-greyscale");
> + fbi->cmap_inverse = of_property_read_bool(np, "cmap-inverse");
> + fbi->cmap_static = of_property_read_bool(np, "cmap-static");
> +
> + fbi->lscr1 = IMXFB_LSCR1_DEFAULT;
> + of_property_read_u32(np, "fsl,lscr1", &fbi->lscr1);
> +
> + of_property_read_u32(np, "fsl,dmacr", &fbi->dmacr);
> +
> + /* These two function pointers could be used by some specific
> + * platforms. */
> + fbi->lcd_power = NULL;
> + fbi->backlight_power = NULL;
> + }
> +
> + return 0;
> +}
> +
> +static int imxfb_of_read_mode(struct device *dev, struct device_node *np,
> + struct imx_fb_videomode *imxfb_mode)
> +{
> + int ret;
> + struct fb_videomode *of_mode = &imxfb_mode->mode;
> + u32 bpp;
> + u32 pcr;
> +
> + ret = of_property_read_string(np, "model", &of_mode->name);
> + if (ret)
> + of_mode->name = NULL;
> +
> + ret = of_get_fb_videomode(np, of_mode, OF_USE_NATIVE_MODE);
> + if (ret) {
> + dev_err(dev, "Failed to get videomode from DT\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(np, "bits-per-pixel", &bpp);
> + ret |= of_property_read_u32(np, "fsl,pcr", &pcr);
> +
> + if (ret) {
> + dev_err(dev, "Failed to read bpp and pcr from DT\n");
> + return -EINVAL;
> + }
> +
> + if (bpp < 1 || bpp > 255) {
> + dev_err(dev, "Bits per pixel have to be between 1 and 255\n");
> + return -EINVAL;
> + }
> +
> + imxfb_mode->bpp = bpp;
> + imxfb_mode->pcr = pcr;
>
> return 0;
> }
>
> -static int __init imxfb_probe(struct platform_device *pdev)
> +static int imxfb_probe(struct platform_device *pdev)
> {
> struct imxfb_info *fbi;
> struct fb_info *info;
> struct imx_fb_platform_data *pdata;
> struct resource *res;
> + struct imx_fb_videomode *m;
> + const struct of_device_id *of_id;
> int ret, i;
> + int bytes_per_pixel;
>
> dev_info(&pdev->dev, "i.MX Framebuffer driver\n");
>
> + of_id = of_match_device(imxfb_of_dev_id, &pdev->dev);
> + if (of_id)
> + pdev->id_entry = of_id->data;
> +
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> if (!res)
> return -ENODEV;
>
> pdata = pdev->dev.platform_data;
> - if (!pdata) {
> - dev_err(&pdev->dev,"No platform_data available\n");
> - return -ENOMEM;
> - }
>
> info = framebuffer_alloc(sizeof(struct imxfb_info), &pdev->dev);
> if (!info)
> @@ -802,15 +885,55 @@ static int __init imxfb_probe(struct platform_device *pdev)
>
> fbi = info->par;
>
> - if (!fb_mode)
> - fb_mode = pdata->mode[0].mode.name;
> -
> platform_set_drvdata(pdev, info);
>
> ret = imxfb_init_fbinfo(pdev);
> if (ret < 0)
> goto failed_init;
>
> + if (pdata) {
> + if (!fb_mode)
> + fb_mode = pdata->mode[0].mode.name;
> +
> + fbi->mode = pdata->mode;
> + fbi->num_modes = pdata->num_modes;
> + } else {
> + struct device_node *display_np;
> + fb_mode = NULL;
> +
> + display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
> + if (!display_np) {
> + dev_err(&pdev->dev, "No display defined in devicetree\n");
> + ret = -EINVAL;
> + goto failed_of_parse;
> + }
> +
> + /*
> + * imxfb does not support more modes, we choose only the native
> + * mode.
> + */
> + fbi->num_modes = 1;
> +
> + fbi->mode = devm_kzalloc(&pdev->dev,
> + sizeof(struct imx_fb_videomode), GFP_KERNEL);
> + if (!fbi->mode) {
> + ret = -ENOMEM;
> + goto failed_of_parse;
> + }
> +
> + ret = imxfb_of_read_mode(&pdev->dev, display_np, fbi->mode);
> + if (ret)
> + goto failed_of_parse;
> + }
> +
> + /* Calculate maximum bytes used per pixel. In most cases this should
> + * be the same as m->bpp/8 */
> + m = &fbi->mode[0];
> + bytes_per_pixel = (m->bpp + 7) / 8;
> + for (i = 0; i < fbi->num_modes; i++, m++)
> + info->fix.smem_len = max_t(size_t, info->fix.smem_len,
> + m->mode.xres * m->mode.yres * bytes_per_pixel);
> +
> res = request_mem_region(res->start, resource_size(res),
> DRIVER_NAME);
> if (!res) {
> @@ -843,7 +966,8 @@ static int __init imxfb_probe(struct platform_device *pdev)
> goto failed_ioremap;
> }
>
> - if (!pdata->fixed_screen_cpu) {
> + /* Seems not being used by anyone, so no support for oftree */
> + if (!pdata || !pdata->fixed_screen_cpu) {
> fbi->map_size = PAGE_ALIGN(info->fix.smem_len);
> fbi->map_cpu = dma_alloc_writecombine(&pdev->dev,
> fbi->map_size, &fbi->map_dma, GFP_KERNEL);
> @@ -868,18 +992,16 @@ static int __init imxfb_probe(struct platform_device *pdev)
> info->fix.smem_start = fbi->screen_dma;
> }
>
> - if (pdata->init) {
> + if (pdata && pdata->init) {
> ret = pdata->init(fbi->pdev);
> if (ret)
> goto failed_platform_init;
> }
>
> - fbi->mode = pdata->mode;
> - fbi->num_modes = pdata->num_modes;
>
> INIT_LIST_HEAD(&info->modelist);
> - for (i = 0; i < pdata->num_modes; i++)
> - fb_add_videomode(&pdata->mode[i].mode, &info->modelist);
> + for (i = 0; i < fbi->num_modes; i++)
> + fb_add_videomode(&fbi->mode[i].mode, &info->modelist);
>
> /*
> * This makes sure that our colour bitfield
> @@ -909,10 +1031,10 @@ static int __init imxfb_probe(struct platform_device *pdev)
> failed_register:
> fb_dealloc_cmap(&info->cmap);
> failed_cmap:
> - if (pdata->exit)
> + if (pdata && pdata->exit)
> pdata->exit(fbi->pdev);
> failed_platform_init:
> - if (!pdata->fixed_screen_cpu)
> + if (pdata && !pdata->fixed_screen_cpu)
> dma_free_writecombine(&pdev->dev,fbi->map_size,fbi->map_cpu,
> fbi->map_dma);
> failed_map:
> @@ -921,6 +1043,7 @@ failed_ioremap:
> failed_getclock:
> release_mem_region(res->start, resource_size(res));
> failed_req:
> +failed_of_parse:
> kfree(info->pseudo_palette);
> failed_init:
> platform_set_drvdata(pdev, NULL);
> @@ -945,7 +1068,7 @@ static int imxfb_remove(struct platform_device *pdev)
> unregister_framebuffer(info);
>
> pdata = pdev->dev.platform_data;
> - if (pdata->exit)
> + if (pdata && pdata->exit)
> pdata->exit(fbi->pdev);
>
> fb_dealloc_cmap(&info->cmap);
> @@ -974,6 +1097,7 @@ static struct platform_driver imxfb_driver = {
> .shutdown = imxfb_shutdown,
> .driver = {
> .name = DRIVER_NAME,
> + .of_match_table = imxfb_of_dev_id,
> },
> .id_table = imxfb_devtype,
> };
> --
> 1.8.2.1
>
^ permalink raw reply
* Re: fb_monspecs and device tree
From: Jean-Christophe PLAGNIOL-VILLARD @ 2013-05-28 10:47 UTC (permalink / raw)
To: Richard Genoud
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Steffen Trumtrar,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat
In-Reply-To: <CACQ1gAg=nyZrXMKk2g+cFvn5g7kHnRKMQ9R2G7ufAqs07dkYHg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 11:41 Tue 28 May , Richard Genoud wrote:
> Hi !
>
> I've been using the display-timings device tree node (great job !) on
> a sam9x5 soc.
>
> I was wondering if there's a planned support for struct fb_monspecs in
> device tree, (or if it is irrelevant), if someone is already working
> on that etc...
I'm working on this but not on fbdev on DRI/KMS
so we can have a full support of the overlay & co
but I'm going to change complely the dt binding with the one present on
github
beawre the lcdc is already done on fbdev with display-timings
but I'll be off the first week on June so wait a few for sam9x5
Best Regards,
J.
>
>
> Regards,
> Richard.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Introduce a new helper framework for buffer synchronization
From: Daniel Vetter @ 2013-05-28 10:32 UTC (permalink / raw)
To: Inki Dae
Cc: 'Rob Clark', 'Maarten Lankhorst',
'Daniel Vetter', 'linux-fbdev',
'YoungJun Cho', 'Kyungmin Park',
'myungjoo.ham', 'DRI mailing list',
linux-arm-kernel, linux-media
In-Reply-To: <005701ce5b57$667c7d40$337577c0$%dae@samsung.com>
On Tue, May 28, 2013 at 12:56:57PM +0900, Inki Dae wrote:
>
>
> > -----Original Message-----
> > From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> > owner@vger.kernel.org] On Behalf Of Rob Clark
> > Sent: Tuesday, May 28, 2013 12:48 AM
> > To: Inki Dae
> > Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho; Kyungmin
> > Park; myungjoo.ham; DRI mailing list;
> linux-arm-kernel@lists.infradead.org;
> > linux-media@vger.kernel.org
> > Subject: Re: Introduce a new helper framework for buffer synchronization
> >
> > On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com> wrote:
> > > Hi all,
> > >
> > > I have been removed previous branch and added new one with more cleanup.
> > > This time, the fence helper doesn't include user side interfaces and
> > cache
> > > operation relevant codes anymore because not only we are not sure that
> > > coupling those two things, synchronizing caches and buffer access
> > between
> > > CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
> > a
> > > good idea yet but also existing codes for user side have problems with
> > badly
> > > behaved or crashing userspace. So this could be more discussed later.
> > >
> > > The below is a new branch,
> > >
> > > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> > exynos.git/?h=dma-f
> > > ence-helper
> > >
> > > And fence helper codes,
> > >
> > > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> > exynos.git/commit/?
> > > h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
> > >
> > > And example codes for device driver,
> > >
> > > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> > exynos.git/commit/?
> > > h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
> > >
> > > I think the time is not yet ripe for RFC posting: maybe existing dma
> > fence
> > > and reservation need more review and addition work. So I'd glad for
> > somebody
> > > giving other opinions and advices in advance before RFC posting.
> >
> > thoughts from a *really* quick, pre-coffee, first look:
> > * any sort of helper to simplify single-buffer sort of use-cases (v4l)
> > probably wouldn't want to bake in assumption that seqno_fence is used.
> > * I guess g2d is probably not actually a simple use case, since I
> > expect you can submit blits involving multiple buffers :-P
>
> I don't think so. One and more buffers can be used: seqno_fence also has
> only one buffer. Actually, we have already applied this approach to most
> devices; multimedia, gpu and display controller. And this approach shows
> more performance; reduced power consumption against traditional way. And g2d
> example is just to show you how to apply my approach to device driver.
Note that seqno_fence is an implementation pattern for a certain type of
direct hw->hw synchronization which uses a shared dma_buf to exchange the
sync cookie. The dma_buf attached to the seqno_fence has _nothing_ to do
with the dma_buf the fence actually coordinates access to.
I think that confusing is a large reason for why Maarten&I don't
understand what you want to achieve with your fence helpers. Currently
they're using the seqno_fence, but totally not in a way the seqno_fence
was meant to be used.
Note that with the current code there is only a pointer from dma_bufs to
the fence. The fence itself has _no_ pointer to the dma_buf it syncs. This
shouldn't be a problem since the fence fastpath for already signalled
fences is completely barrier&lock free (it's just a load+bit-test), and
fences are meant to be embedded into whatever dma tracking structure you
already have, so no overhead there. The only ugly part is the fence
refcounting, but I don't think we can drop that.
Note that you completely reinvent this part of Maarten's fence patches by
adding new r/w_complete completions to the reservation object, which
completely replaces the fence stuff.
Also note that a list of reservation entries is again meant to be used
only when submitting the dma to the gpu. With your patches you seem to
hang onto that list until dma completes. This has the ugly side-effect
that you need to allocate these reservation entries with kmalloc, whereas
if you just use them in the execbuf ioctl to construct the dma you can
usually embed it into something else you need already anyway. At least
i915 and ttm based drivers can work that way.
Furthermore fences are specifically constructed as frankenstein-monsters
between completion/waitqueues and callbacks. All the different use-cases
need the different aspects:
- busy/idle checks and bo retiring need the completion semantics
- callbacks (in interrupt context) are used for hybrid hw->irq handler->hw
sync approaches
>
> > * otherwise, you probably don't want to depend on dmabuf, which is why
> > reservation/fence is split out the way it is.. you want to be able to
> > use a single reservation/fence mechanism within your driver without
> > having to care about which buffers are exported to dmabuf's and which
> > are not. Creating a dmabuf for every GEM bo is too heavyweight.
>
> Right. But I think we should dealt with this separately. Actually, we are
> trying to use reservation for gpu pipe line synchronization such as sgx sync
> object and this approach is used without dmabuf. In order words, some device
> can use only reservation for such pipe line synchronization and at the same
> time, fence helper or similar thing with dmabuf for buffer synchronization.
I think a quick overview of the different pieces would be in order.
- ww_mutex: This is just the locking primite which allows you to grab
multiple mutexes without the need to check for ordering (and so fear
deadlocks).
- fence: This is just a fancy kind of completion with a bit of support for
hw->hw completion events. The fences themselve have no tie-in with
buffers, ww_mutexes or anything else.
- reservation: This ties together an object (doesn't need to be a buffer,
could be any other gpu resource - see the drm/vmwgfx driver if you want
your mind blown) with fences. Note that there's no connection the other
way round, i.e. with the current patches you can't see which
reservations are using which fences. Also note that other objects than
reservations could point at fences, e.g. when the provide
shared/exclusive semantics are not good enough for your needs.
The ww_mutex in the reservation is simply the (fancy) lock which
protects each reservation. The reason we need something fancy is that
you need to lock all objects which are synced by the same fence
toghether, otherwise you can race and construct deadlocks in the hw->hw
sync part of fences.
- dma_buf integration: This is simply a pointer to the reservation object
of the underlying buffer object. We need a pointer here since otherwise
you can construct deadlocks between dma_buf objects and native buffer
objects.
The crux is also that dma_buf integration comes last - before you can do
that you need to have your driver converted over to use
ww_mutexes/fences/reservations.
I hope that helps to unconfuse things a bit and help you understand the
different pieces of the fence/reservation/ww_mutex patches floating
around.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* fb_monspecs and device tree
From: Richard Genoud @ 2013-05-28 9:41 UTC (permalink / raw)
To: Steffen Trumtrar
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat
Hi !
I've been using the display-timings device tree node (great job !) on
a sam9x5 soc.
I was wondering if there's a planned support for struct fb_monspecs in
device tree, (or if it is irrelevant), if someone is already working
on that etc...
Regards,
Richard.
^ permalink raw reply
* Re: Introduce a new helper framework for buffer synchronization
From: Maarten Lankhorst @ 2013-05-28 7:23 UTC (permalink / raw)
To: Inki Dae
Cc: 'Daniel Vetter', 'Rob Clark',
'linux-fbdev', 'YoungJun Cho',
'Kyungmin Park', 'myungjoo.ham',
'DRI mailing list', linux-arm-kernel, linux-media
In-Reply-To: <005601ce5b4d$f95e0160$ec1a0420$%dae@samsung.com>
Hey,
Op 28-05-13 04:49, Inki Dae schreef:
>
>> -----Original Message-----
>> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
>> Sent: Tuesday, May 28, 2013 12:23 AM
>> To: Inki Dae
>> Cc: 'Daniel Vetter'; 'Rob Clark'; 'linux-fbdev'; 'YoungJun Cho'; 'Kyungmin
>> Park'; 'myungjoo.ham'; 'DRI mailing list'; linux-arm-
>> kernel@lists.infradead.org; linux-media@vger.kernel.org
>> Subject: Re: Introduce a new helper framework for buffer synchronization
>>
>> Hey,
>>
>> Op 27-05-13 12:38, Inki Dae schreef:
>>> Hi all,
>>>
>>> I have been removed previous branch and added new one with more cleanup.
>>> This time, the fence helper doesn't include user side interfaces and
>> cache
>>> operation relevant codes anymore because not only we are not sure that
>>> coupling those two things, synchronizing caches and buffer access
>> between
>>> CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
>> a
>>> good idea yet but also existing codes for user side have problems with
>> badly
>>> behaved or crashing userspace. So this could be more discussed later.
>>>
>>> The below is a new branch,
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/?h=dma-f
>>> ence-helper
>>>
>>> And fence helper codes,
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/commit/?
>>> h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
>>>
>>> And example codes for device driver,
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
>> exynos.git/commit/?
>>> h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
>>>
>>> I think the time is not yet ripe for RFC posting: maybe existing dma
>> fence
>>> and reservation need more review and addition work. So I'd glad for
>> somebody
>>> giving other opinions and advices in advance before RFC posting.
>>>
>> NAK.
>>
>> For examples for how to handle locking properly, see Documentation/ww-
>> mutex-design.txt in my recent tree.
>> I could list what I believe is wrong with your implementation, but real
>> problem is that the approach you're taking is wrong.
> I just removed ticket stubs to show my approach you guys as simple as
> possible, and I just wanted to show that we could use buffer synchronization
> mechanism without ticket stubs.
The tickets have been removed in favor of a ww_context. Moving it in as a base primitive
allows more locking abuse to be detected, and makes some other things easier too.
> Question, WW-Mutexes could be used for all devices? I guess this has
> dependence on x86 gpu: gpu has VRAM and it means different memory domain.
> And could you tell my why shared fence should have only eight objects? I
> think we could need more than eight objects for read access. Anyway I think
> I don't surely understand yet so there might be my missing point.
Yes, ww mutexes are not limited in any way to x86. They're a locking mechanism.
When you acquired the ww mutexes for all buffer objects, all it does is say at
that point in time you have exclusively acquired the locks of all bo's.
After locking everything you can read the fence pointers safely, queue waits, and set a
new fence pointer on all reservation_objects. You only need a single fence
on all those objects, so 8 is plenty. Nonetheless this was a limitation of my
earlier design, and I'll dynamically allocate fence_shared in the future.
~Maarten
^ permalink raw reply
* Re: [PATCH v2] video: simplefb: add mode parsing function
From: Alexandre Courbot @ 2013-05-28 4:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51A42ED5.2060104@wwwdotorg.org>
On Tue, May 28, 2013 at 1:13 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/26/2013 09:53 PM, Alexandre Courbot wrote:
>> The naming scheme of simplefb's mode is precise enough to allow building
>> the mode structure from it instead of using a static list of modes. This
>> patch introduces a function that does this. In case exotic modes that
>> cannot be represented from their name alone are needed, the static list
>> of modes is still available as a backup.
>>
>> It also changes the order in which colors are declared from MSB first to
>> the more standard LSB first.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>> Changes from v1:
>> - amended documentation following Stephen's suggestion
>> - allow parsing of bitfields larger than 9 bits
>> - made it clear that the parsing order of bits is changed with respect
>> to the original patch
>>
>> Andrew, since this patch introduces a (small) change in the DT bindings,
>> could we try to merge it during the -rc cycle so we don't have to come
>> with a more complex solution in the future?
>
> So, I think we shouldn't change the DT binding at all now. The original
> driver is now merged for 3.10, and I think it's far too late to take
> code that implements new features for 3.10. This means that we couldn't
> merge this patch until 3.11. However, by then, we shouldn't be changing
> the DT binding in incompatible ways. Olof has already published some
> U-Boot binaries that use the current binding, and on IRC indicated he'd
> prefer not to change the binding because of that.
>
> As such, we should either:
>
> a) Just add new entries into the format array that already exists in the
> driver. Given David's response, this might be simplest.
>
> b) Extend the DT binding in a 100% backwards-compatible way, which would
> mean defaulting the bit-order to match the existing 1 format, and adding
> a new Boolean property to indicate that the format string is specified
> in the other order.
a) is definitely simpler, so let's do that and drop this patch. Sorry
about the noise.
Alex.
^ permalink raw reply
* Re: [PATCH v2] video: simplefb: add mode parsing function
From: Stephen Warren @ 2013-05-28 4:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1369626821-28494-1-git-send-email-acourbot@nvidia.com>
On 05/26/2013 09:53 PM, Alexandre Courbot wrote:
> The naming scheme of simplefb's mode is precise enough to allow building
> the mode structure from it instead of using a static list of modes. This
> patch introduces a function that does this. In case exotic modes that
> cannot be represented from their name alone are needed, the static list
> of modes is still available as a backup.
>
> It also changes the order in which colors are declared from MSB first to
> the more standard LSB first.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> Changes from v1:
> - amended documentation following Stephen's suggestion
> - allow parsing of bitfields larger than 9 bits
> - made it clear that the parsing order of bits is changed with respect
> to the original patch
>
> Andrew, since this patch introduces a (small) change in the DT bindings,
> could we try to merge it during the -rc cycle so we don't have to come
> with a more complex solution in the future?
So, I think we shouldn't change the DT binding at all now. The original
driver is now merged for 3.10, and I think it's far too late to take
code that implements new features for 3.10. This means that we couldn't
merge this patch until 3.11. However, by then, we shouldn't be changing
the DT binding in incompatible ways. Olof has already published some
U-Boot binaries that use the current binding, and on IRC indicated he'd
prefer not to change the binding because of that.
As such, we should either:
a) Just add new entries into the format array that already exists in the
driver. Given David's response, this might be simplest.
b) Extend the DT binding in a 100% backwards-compatible way, which would
mean defaulting the bit-order to match the existing 1 format, and adding
a new Boolean property to indicate that the format string is specified
in the other order.
^ permalink raw reply
* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-28 4:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>
> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Tuesday, May 28, 2013 1:02 AM
> To: Rob Clark
> Cc: Inki Dae; Maarten Lankhorst; linux-fbdev; YoungJun Cho; Kyungmin Park;
> myungjoo.ham; DRI mailing list; linux-arm-kernel@lists.infradead.org;
> linux-media@vger.kernel.org
> Subject: Re: Introduce a new helper framework for buffer synchronization
>
> On Mon, May 27, 2013 at 5:47 PM, Rob Clark <robdclark@gmail.com> wrote:
> > On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com> wrote:
> >> Hi all,
> >>
> >> I have been removed previous branch and added new one with more
cleanup.
> >> This time, the fence helper doesn't include user side interfaces and
> cache
> >> operation relevant codes anymore because not only we are not sure that
> >> coupling those two things, synchronizing caches and buffer access
> between
> >> CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side
> is a
> >> good idea yet but also existing codes for user side have problems with
> badly
> >> behaved or crashing userspace. So this could be more discussed later.
> >>
> >> The below is a new branch,
> >>
> >> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/?h=dma-f
> >> ence-helper
> >>
> >> And fence helper codes,
> >>
> >> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/commit/?
> >> h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
> >>
> >> And example codes for device driver,
> >>
> >> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/commit/?
> >> h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
> >>
> >> I think the time is not yet ripe for RFC posting: maybe existing dma
> fence
> >> and reservation need more review and addition work. So I'd glad for
> somebody
> >> giving other opinions and advices in advance before RFC posting.
> >
> > thoughts from a *really* quick, pre-coffee, first look:
> > * any sort of helper to simplify single-buffer sort of use-cases (v4l)
> > probably wouldn't want to bake in assumption that seqno_fence is used.
>
> Yeah, which is why Maarten&I discussed ideas already for what needs to
> be improved in the current dma-buf interface code to make this Just
> Work. At least as long as a driver doesn't want to add new fences,
> which would be especially useful for all kinds of gpu access.
>
> > * I guess g2d is probably not actually a simple use case, since I
> > expect you can submit blits involving multiple buffers :-P
>
> Yeah, on a quick read the current fence helper code seems to be a bit
> limited in scope.
>
> > * otherwise, you probably don't want to depend on dmabuf, which is why
> > reservation/fence is split out the way it is.. you want to be able to
> > use a single reservation/fence mechanism within your driver without
> > having to care about which buffers are exported to dmabuf's and which
> > are not. Creating a dmabuf for every GEM bo is too heavyweight.
>
> That's pretty much the reason that reservations are free-standing from
> dma_bufs. The idea is to embed them into the gem/ttm/v4l buffer
> object. Maarten also has some helpers to keep track of multi-buffer
> ww_mutex locking and fence attaching in his reservation helpers, but I
> think we should wait with those until we have drivers using them.
>
> For now I think the priority should be to get the basic stuff in and
> ttm as the first user established. Then we can go nuts later on.
>
> > I'm not entirely sure if reservation/fence could/should be made any
> > simpler for multi-buffer users. Probably the best thing to do is just
> > get reservation/fence rolled out in a few drivers and see if some
> > common patterns emerge.
>
> I think we can make the 1 buffer per dma op (i.e. 1:1
> dma_buf->reservation : fence mapping) work fairly simple in dma_buf
> with maybe a dma_buf_attachment_start_dma/end_dma helpers. But there's
> also still the open that currently there's no way to flush cpu caches
> for dma access without unmapping the attachement (or resorting to
That was what I tried adding user interfaces to dmabuf: coupling
synchronizing caches and buffer access between CPU and CPU, CPU and DMA, and
DMA and DMA with fences in kernel side. We need something to do between
mapping and unmapping attachment.
> trick which might not work), so we have a few gaping holes in the
> interface already anyway.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-28 3:56 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>
> -----Original Message-----
> From: linux-fbdev-owner@vger.kernel.org [mailto:linux-fbdev-
> owner@vger.kernel.org] On Behalf Of Rob Clark
> Sent: Tuesday, May 28, 2013 12:48 AM
> To: Inki Dae
> Cc: Maarten Lankhorst; Daniel Vetter; linux-fbdev; YoungJun Cho; Kyungmin
> Park; myungjoo.ham; DRI mailing list;
linux-arm-kernel@lists.infradead.org;
> linux-media@vger.kernel.org
> Subject: Re: Introduce a new helper framework for buffer synchronization
>
> On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com> wrote:
> > Hi all,
> >
> > I have been removed previous branch and added new one with more cleanup.
> > This time, the fence helper doesn't include user side interfaces and
> cache
> > operation relevant codes anymore because not only we are not sure that
> > coupling those two things, synchronizing caches and buffer access
> between
> > CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
> a
> > good idea yet but also existing codes for user side have problems with
> badly
> > behaved or crashing userspace. So this could be more discussed later.
> >
> > The below is a new branch,
> >
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/?h=dma-f
> > ence-helper
> >
> > And fence helper codes,
> >
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/commit/?
> > h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
> >
> > And example codes for device driver,
> >
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/commit/?
> > h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
> >
> > I think the time is not yet ripe for RFC posting: maybe existing dma
> fence
> > and reservation need more review and addition work. So I'd glad for
> somebody
> > giving other opinions and advices in advance before RFC posting.
>
> thoughts from a *really* quick, pre-coffee, first look:
> * any sort of helper to simplify single-buffer sort of use-cases (v4l)
> probably wouldn't want to bake in assumption that seqno_fence is used.
> * I guess g2d is probably not actually a simple use case, since I
> expect you can submit blits involving multiple buffers :-P
I don't think so. One and more buffers can be used: seqno_fence also has
only one buffer. Actually, we have already applied this approach to most
devices; multimedia, gpu and display controller. And this approach shows
more performance; reduced power consumption against traditional way. And g2d
example is just to show you how to apply my approach to device driver.
> * otherwise, you probably don't want to depend on dmabuf, which is why
> reservation/fence is split out the way it is.. you want to be able to
> use a single reservation/fence mechanism within your driver without
> having to care about which buffers are exported to dmabuf's and which
> are not. Creating a dmabuf for every GEM bo is too heavyweight.
Right. But I think we should dealt with this separately. Actually, we are
trying to use reservation for gpu pipe line synchronization such as sgx sync
object and this approach is used without dmabuf. In order words, some device
can use only reservation for such pipe line synchronization and at the same
time, fence helper or similar thing with dmabuf for buffer synchronization.
>
> I'm not entirely sure if reservation/fence could/should be made any
> simpler for multi-buffer users. Probably the best thing to do is just
> get reservation/fence rolled out in a few drivers and see if some
> common patterns emerge.
>
> BR,
> -R
>
> >
> > Thanks,
> > Inki Dae
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: Introduce a new helper framework for buffer synchronization
From: Inki Dae @ 2013-05-28 2:49 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51909DB4.2060208@canonical.com>
> -----Original Message-----
> From: Maarten Lankhorst [mailto:maarten.lankhorst@canonical.com]
> Sent: Tuesday, May 28, 2013 12:23 AM
> To: Inki Dae
> Cc: 'Daniel Vetter'; 'Rob Clark'; 'linux-fbdev'; 'YoungJun Cho'; 'Kyungmin
> Park'; 'myungjoo.ham'; 'DRI mailing list'; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org
> Subject: Re: Introduce a new helper framework for buffer synchronization
>
> Hey,
>
> Op 27-05-13 12:38, Inki Dae schreef:
> > Hi all,
> >
> > I have been removed previous branch and added new one with more cleanup.
> > This time, the fence helper doesn't include user side interfaces and
> cache
> > operation relevant codes anymore because not only we are not sure that
> > coupling those two things, synchronizing caches and buffer access
> between
> > CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is
> a
> > good idea yet but also existing codes for user side have problems with
> badly
> > behaved or crashing userspace. So this could be more discussed later.
> >
> > The below is a new branch,
> >
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/?h=dma-f
> > ence-helper
> >
> > And fence helper codes,
> >
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/commit/?
> > h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
> >
> > And example codes for device driver,
> >
> > https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-
> exynos.git/commit/?
> > h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
> >
> > I think the time is not yet ripe for RFC posting: maybe existing dma
> fence
> > and reservation need more review and addition work. So I'd glad for
> somebody
> > giving other opinions and advices in advance before RFC posting.
> >
> NAK.
>
> For examples for how to handle locking properly, see Documentation/ww-
> mutex-design.txt in my recent tree.
> I could list what I believe is wrong with your implementation, but real
> problem is that the approach you're taking is wrong.
I just removed ticket stubs to show my approach you guys as simple as
possible, and I just wanted to show that we could use buffer synchronization
mechanism without ticket stubs.
Question, WW-Mutexes could be used for all devices? I guess this has
dependence on x86 gpu: gpu has VRAM and it means different memory domain.
And could you tell my why shared fence should have only eight objects? I
think we could need more than eight objects for read access. Anyway I think
I don't surely understand yet so there might be my missing point.
Thanks,
Inki Dae
>
> ~Maarten
^ permalink raw reply
* Re: [PATCH v2] video: simplefb: add mode parsing function
From: Alexandre Courbot @ 2013-05-28 2:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CANq1E4Qa6g-zrA6o_u3w6mpVcUk8-ZPPR+edu6EXgpk-kGQULQ@mail.gmail.com>
Hi David,
On Tue, May 28, 2013 at 3:22 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> If we apply this patch it will get very hard to support this with a
> KMS driver. So any reason why we cannot use the DRM FOURCC constants
> instead?
I don't see any. Maybe Stephen can confirm.
> The dvbe proposal is available here: (also see the two following patches)
> http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/00274.html
>
> it provides a simple DRM driver that uses VESA / VBE. I am currently
> trying to rework it to "SimpleDRM" which can support arbitrary
> firmware framebuffers.
Ok. I am personally fine with using FOURCC codes for describing video
modes. Maybe we can just drop this patch if it gets in your way.
Alex.
^ permalink raw reply
* Re: [PATCH v2] video: simplefb: add mode parsing function
From: David Herrmann @ 2013-05-27 18:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1369626821-28494-1-git-send-email-acourbot@nvidia.com>
Hi
On Mon, May 27, 2013 at 5:53 AM, Alexandre Courbot <acourbot@nvidia.com> wrote:
> The naming scheme of simplefb's mode is precise enough to allow building
> the mode structure from it instead of using a static list of modes. This
> patch introduces a function that does this. In case exotic modes that
> cannot be represented from their name alone are needed, the static list
> of modes is still available as a backup.
>
> It also changes the order in which colors are declared from MSB first to
> the more standard LSB first.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
> Changes from v1:
> - amended documentation following Stephen's suggestion
> - allow parsing of bitfields larger than 9 bits
> - made it clear that the parsing order of bits is changed with respect
> to the original patch
>
> Andrew, since this patch introduces a (small) change in the DT bindings,
> could we try to merge it during the -rc cycle so we don't have to come
> with a more complex solution in the future?
>
> .../bindings/video/simple-framebuffer.txt | 12 +++-
> drivers/video/simplefb.c | 72 +++++++++++++++++++++-
> 2 files changed, 80 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> index 3ea4605..18d03e2 100644
> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
> @@ -10,8 +10,16 @@ Required properties:
> - width: The width of the framebuffer in pixels.
> - height: The height of the framebuffer in pixels.
> - stride: The number of bytes in each line of the framebuffer.
> -- format: The format of the framebuffer surface. Valid values are:
> - - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
> +- format: The format of the framebuffer surface. Described as a sequence of
> + channel/num-bits pairs, where each pair describes how many bits are used
> + by a given color channel. Value channels are "r" (red), "g" (green),
> + "b" (blue), "a" (alpha) and "x" (unused). Channels are listed in bit
> + order, starting from the LSB. For instance, a format named "r5g6b5"
> + describes a 16-bit format where red is encoded in the 5 less significant
> + bits, green in the 6 following ones, and blue in the 5 last:
> + BBBBBGGG GGGRRRRR
> + ^ ^
> + MSB LSB
Is there a specific reason why we need arbitrary RGB formats? I have a
KMS/DRM driver based on dvbe that can provide the exact same
functionality as this fbdev driver (including an fbdev
backwards-compat layer). However, DRM does not allow arbitrary formats
but rather provides a huge list of supported formats.
If we apply this patch it will get very hard to support this with a
KMS driver. So any reason why we cannot use the DRM FOURCC constants
instead?
The dvbe proposal is available here: (also see the two following patches)
http://lkml.indiana.edu/hypermail/linux/kernel/1302.2/00274.html
it provides a simple DRM driver that uses VESA / VBE. I am currently
trying to rework it to "SimpleDRM" which can support arbitrary
firmware framebuffers.
Cheers
David
> Example:
>
> diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
> index e2e9e3e..1430752 100644
> --- a/drivers/video/simplefb.c
> +++ b/drivers/video/simplefb.c
> @@ -21,6 +21,7 @@
> */
>
> #include <linux/errno.h>
> +#include <linux/ctype.h>
> #include <linux/fb.h>
> #include <linux/io.h>
> #include <linux/module.h>
> @@ -82,8 +83,72 @@ struct simplefb_format {
> struct fb_bitfield transp;
> };
>
> +static struct simplefb_format *simplefb_parse_format(struct device *dev,
> + const char *str)
> +{
> + struct simplefb_format *format;
> + unsigned int offset = 0;
> + unsigned int i = 0;
> +
> + format = devm_kzalloc(dev, sizeof(*format), GFP_KERNEL);
> + if (!format)
> + return ERR_PTR(-ENOMEM);
> +
> + while (str[i] != 0) {
> + struct fb_bitfield *field = NULL;
> + int length = 0;
> +
> + switch (str[i++]) {
> + case 'r':
> + case 'R':
> + field = &format->red;
> + break;
> + case 'g':
> + case 'G':
> + field = &format->green;
> + break;
> + case 'b':
> + case 'B':
> + field = &format->blue;
> + break;
> + case 'a':
> + case 'A':
> + field = &format->transp;
> + break;
> + case 'x':
> + case 'X':
> + break;
> + default:
> + goto error;
> + }
> +
> + if (!isdigit(str[i]))
> + goto error;
> +
> + while (isdigit(str[i])) {
> + length = length * 10 + (str[i++] - '0');
> + }
> +
> + if (field) {
> + field->offset = offset;
> + field->length = length;
> + }
> +
> + offset += length;
> + }
> +
> + format->bits_per_pixel = (offset + 7) & ~0x7;
> + format->name = str;
> + return format;
> +
> +error:
> + dev_err(dev, "Invalid format string\n");
> + return ERR_PTR(-EINVAL);
> +}
> +
> +/* if you use exotic modes that simplefb_parse_format cannot decode, you can
> + specify them here. */
> static struct simplefb_format simplefb_formats[] = {
> - { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> };
>
> struct simplefb_params {
> @@ -131,7 +196,10 @@ static int simplefb_parse_dt(struct platform_device *pdev,
> params->format = &simplefb_formats[i];
> break;
> }
> - if (!params->format) {
> + if (!params->format)
> + params->format = simplefb_parse_format(&pdev->dev, format);
> +
> + if (IS_ERR(params->format)) {
> dev_err(&pdev->dev, "Invalid format value\n");
> return -EINVAL;
> }
> --
> 1.8.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: Introduce a new helper framework for buffer synchronization
From: Daniel Vetter @ 2013-05-27 16:02 UTC (permalink / raw)
To: Rob Clark
Cc: Inki Dae, Maarten Lankhorst, linux-fbdev, YoungJun Cho,
Kyungmin Park, myungjoo.ham, DRI mailing list,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <CAF6AEGvGv539Ktdeg03n783nD+HofDamcJCLX93rzzKGOCV8_Q@mail.gmail.com>
On Mon, May 27, 2013 at 5:47 PM, Rob Clark <robdclark@gmail.com> wrote:
> On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> Hi all,
>>
>> I have been removed previous branch and added new one with more cleanup.
>> This time, the fence helper doesn't include user side interfaces and cache
>> operation relevant codes anymore because not only we are not sure that
>> coupling those two things, synchronizing caches and buffer access between
>> CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is a
>> good idea yet but also existing codes for user side have problems with badly
>> behaved or crashing userspace. So this could be more discussed later.
>>
>> The below is a new branch,
>>
>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/?h=dma-f
>> ence-helper
>>
>> And fence helper codes,
>>
>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?
>> h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
>>
>> And example codes for device driver,
>>
>> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?
>> h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
>>
>> I think the time is not yet ripe for RFC posting: maybe existing dma fence
>> and reservation need more review and addition work. So I'd glad for somebody
>> giving other opinions and advices in advance before RFC posting.
>
> thoughts from a *really* quick, pre-coffee, first look:
> * any sort of helper to simplify single-buffer sort of use-cases (v4l)
> probably wouldn't want to bake in assumption that seqno_fence is used.
Yeah, which is why Maarten&I discussed ideas already for what needs to
be improved in the current dma-buf interface code to make this Just
Work. At least as long as a driver doesn't want to add new fences,
which would be especially useful for all kinds of gpu access.
> * I guess g2d is probably not actually a simple use case, since I
> expect you can submit blits involving multiple buffers :-P
Yeah, on a quick read the current fence helper code seems to be a bit
limited in scope.
> * otherwise, you probably don't want to depend on dmabuf, which is why
> reservation/fence is split out the way it is.. you want to be able to
> use a single reservation/fence mechanism within your driver without
> having to care about which buffers are exported to dmabuf's and which
> are not. Creating a dmabuf for every GEM bo is too heavyweight.
That's pretty much the reason that reservations are free-standing from
dma_bufs. The idea is to embed them into the gem/ttm/v4l buffer
object. Maarten also has some helpers to keep track of multi-buffer
ww_mutex locking and fence attaching in his reservation helpers, but I
think we should wait with those until we have drivers using them.
For now I think the priority should be to get the basic stuff in and
ttm as the first user established. Then we can go nuts later on.
> I'm not entirely sure if reservation/fence could/should be made any
> simpler for multi-buffer users. Probably the best thing to do is just
> get reservation/fence rolled out in a few drivers and see if some
> common patterns emerge.
I think we can make the 1 buffer per dma op (i.e. 1:1
dma_buf->reservation : fence mapping) work fairly simple in dma_buf
with maybe a dma_buf_attachment_start_dma/end_dma helpers. But there's
also still the open that currently there's no way to flush cpu caches
for dma access without unmapping the attachement (or resorting to
trick which might not work), so we have a few gaping holes in the
interface already anyway.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply
* Re: Introduce a new helper framework for buffer synchronization
From: Rob Clark @ 2013-05-27 15:47 UTC (permalink / raw)
To: Inki Dae
Cc: Maarten Lankhorst, Daniel Vetter, linux-fbdev, YoungJun Cho,
Kyungmin Park, myungjoo.ham, DRI mailing list, linux-arm-kernel,
linux-media
In-Reply-To: <014501ce5ac6$511a8500$f34f8f00$%dae@samsung.com>
On Mon, May 27, 2013 at 6:38 AM, Inki Dae <inki.dae@samsung.com> wrote:
> Hi all,
>
> I have been removed previous branch and added new one with more cleanup.
> This time, the fence helper doesn't include user side interfaces and cache
> operation relevant codes anymore because not only we are not sure that
> coupling those two things, synchronizing caches and buffer access between
> CPU and CPU, CPU and DMA, and DMA and DMA with fences, in kernel side is a
> good idea yet but also existing codes for user side have problems with badly
> behaved or crashing userspace. So this could be more discussed later.
>
> The below is a new branch,
>
> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/?h=dma-f
> ence-helper
>
> And fence helper codes,
>
> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?
> h=dma-fence-helper&idcbc0fe7e285ce866e5816e5e21443dcce01005
>
> And example codes for device driver,
>
> https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/commit/?
> h=dma-fence-helper&idÒce7af23835789602a99d0ccef1f53cdd5caaae
>
> I think the time is not yet ripe for RFC posting: maybe existing dma fence
> and reservation need more review and addition work. So I'd glad for somebody
> giving other opinions and advices in advance before RFC posting.
thoughts from a *really* quick, pre-coffee, first look:
* any sort of helper to simplify single-buffer sort of use-cases (v4l)
probably wouldn't want to bake in assumption that seqno_fence is used.
* I guess g2d is probably not actually a simple use case, since I
expect you can submit blits involving multiple buffers :-P
* otherwise, you probably don't want to depend on dmabuf, which is why
reservation/fence is split out the way it is.. you want to be able to
use a single reservation/fence mechanism within your driver without
having to care about which buffers are exported to dmabuf's and which
are not. Creating a dmabuf for every GEM bo is too heavyweight.
I'm not entirely sure if reservation/fence could/should be made any
simpler for multi-buffer users. Probably the best thing to do is just
get reservation/fence rolled out in a few drivers and see if some
common patterns emerge.
BR,
-R
>
> Thanks,
> Inki Dae
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox