* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Russell King - ARM Linux @ 2013-06-18 9:38 UTC (permalink / raw)
To: Inki Dae
Cc: 'Maarten Lankhorst', 'linux-fbdev',
'Kyungmin Park', 'DRI mailing list',
'Rob Clark', 'myungjoo.ham',
'YoungJun Cho', 'Daniel Vetter', linux-arm-kernel,
linux-media
In-Reply-To: <008a01ce6c02$e00a9f50$a01fddf0$%dae@samsung.com>
On Tue, Jun 18, 2013 at 06:04:44PM +0900, Inki Dae wrote:
>
>
> > -----Original Message-----
> > From: Russell King - ARM Linux [mailto:linux@arm.linux.org.uk]
> > Sent: Tuesday, June 18, 2013 5:43 PM
> > To: Inki Dae
> > Cc: 'Maarten Lankhorst'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI mailing
> > list'; 'Rob Clark'; 'myungjoo.ham'; 'YoungJun Cho'; 'Daniel Vetter';
> > linux-arm-kernel@lists.infradead.org; linux-media@vger.kernel.org
> > Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization
> > framework
> >
> > On Tue, Jun 18, 2013 at 02:27:40PM +0900, Inki Dae wrote:
> > > So I'd like to ask for other DRM maintainers. How do you think about it?
> > it
> > > seems like that Intel DRM (maintained by Daniel), OMAP DRM (maintained
> > by
> > > Rob) and GEM CMA helper also have same issue Russell pointed out. I
> > think
> > > not only the above approach but also the performance is very important.
> >
> > CMA uses coherent memory to back their buffers, though that might not be
> > true of memory obtained from other drivers via dma_buf. Plus, there is
> > no support in the CMA helper for exporting or importng these buffers.
> >
>
> It's not so. Please see Dave's drm next. recently dmabuf support for the CMA
> helper has been merged to there.
The point stands: CMA is DMA coherent memory. It doesn't need and must
never be dma-map-sg'd or dma-sync'd or dma-unmap'd.
^ permalink raw reply
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Lucas Stach @ 2013-06-18 9:47 UTC (permalink / raw)
To: Inki Dae
Cc: 'Russell King - ARM Linux', 'linux-fbdev',
'Kyungmin Park', 'DRI mailing list',
'myungjoo.ham', 'YoungJun Cho', linux-arm-kernel,
linux-media
In-Reply-To: <008a01ce6c02$e00a9f50$a01fddf0$%dae@samsung.com>
Am Dienstag, den 18.06.2013, 18:04 +0900 schrieb Inki Dae:
[...]
>
> > a display device driver. It shouldn't be used within a single driver
> > as a means of passing buffers between userspace and kernel space.
>
> What I try to do is not really such ugly thing. What I try to do is to
> notify that, when CPU tries to access a buffer , to kernel side through
> dmabuf interface. So it's not really to send the buffer to kernel.
>
> Thanks,
> Inki Dae
>
The most basic question about why you are trying to implement this sort
of thing in the dma_buf framework still stands.
Once you imported a dma_buf into your DRM driver it's a GEM object and
you can and should use the native DRM ioctls to prepare/end a CPU access
to this BO. Then internally to your driver you can use the dma_buf
reservation/fence stuff to provide the necessary cross-device sync.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Russell King - ARM Linux @ 2013-06-18 10:46 UTC (permalink / raw)
To: Daniel Vetter
Cc: Inki Dae, Maarten Lankhorst, linux-fbdev, Kyungmin Park,
DRI mailing list, Rob Clark, myungjoo.ham, YoungJun Cho,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <CAKMK7uECS=eqNB-Ud6s=NvdYMPfxgJaGPbUx9hANfP6kb02j_w@mail.gmail.com>
On Tue, Jun 18, 2013 at 09:00:16AM +0200, Daniel Vetter wrote:
> On Mon, Jun 17, 2013 at 04:42:37PM +0100, Russell King - ARM Linux wrote:
> > What we need is something along the lines of:
> > (a) dma_buf_map_attachment() _not_ to map the scatterlist for DMA.
> > or
> > (b) drm_gem_prime_import() not to call dma_buf_map_attachment() at all.
>
> I strongly vote for (b) (plus adding a dma_buf_map_sync interface to allow
> syncing to other devices/cpu whithout dropping the dma mappings). At least
> that's been the idea behind things, but currently all (x86-based) drm
> drivers cut corners here massively.
>
> Aside: The entire reason behind hiding the dma map step in the dma-buf
> attachment is to allow drivers to expose crazy iommus (e.g. the tiler unit
> on omap) to other devices. So imo (a) isn't the right choice.
That's why I also talk below about adding the dma_buf_map/sync callbacks
below, so that a dma_buf implementation can do whatever is necessary with
the sg at the point of use.
However, you are correct that this should be unnecessary, as DRM should
only be calling dma_buf_map_attachment() when it needs to know about the
memory behind the object. The problem is that people will cache that
information - it also may be expensive to setup for the dma_buf - as it
involves counting the number of pages, memory allocation, and maybe
obtaining the set of pages from shmem.
With (a) plus the callbacks below, it means that step is only performed
once, and then the DMA API part is entirely separate - we can have our
cake and eat it in that regard.
> > Note: the existing stuff does have the nice side effect of being able
> > to pass buffers which do not have a struct page * associated with them
> > through the dma_buf API - I think we can still preserve that by having
> > dma_buf provide a couple of new APIs to do the SG list map/sync/unmap,
> > but in any case we need to fix the existing API so that:
> >
> > dma_buf_map_attachment() becomes dma_buf_get_sg()
> > dma_buf_unmap_attachment() becomes dma_buf_put_sg()
> >
> > both getting rid of the DMA direction argument, and then we have four
> > new dma_buf calls:
> >
> > dma_buf_map_sg()
> > dma_buf_unmap_sg()
> > dma_buf_sync_sg_for_cpu()
> > dma_buf_sync_sg_for_device()
> >
> > which do the actual sg map/unmap via the DMA API *at the appropriate
> > time for DMA*.
>
> Hm, my idea was to just add a dma_buf_sync_attchment for the device side
> syncing, since the cpu access stuff is already bracketed with the
> begin/end cpu access stuff. We might need a sync_for_cpu or so for mmap,
> but imo mmap support for dma_buf is a bit insane anyway, so I don't care
> too much about it.
>
> Since such dma mappings would be really longstanding in most cases anyway
> drivers could just map with BIDIRECTIONAL and do all the real flushing
> with the new sync stuff.
Note that the DMA API debug doesn't allow you to change the direction
argument on an existing mapping (neither should it, again this is
documented in the DMA API stuff in Documentation/). This is where you
would need the complete set of four functions I mention above which
reflect the functionality of the DMA API.
The dma_buf implementation doesn't _have_ to implement them if they
are no-ops - for example, your dma_buf sg array contains DMA pointers,
and the memory is already coherent in some way (for example, it's a
separate chunk of memory which isn't part of system RAM.)
^ permalink raw reply
* [PATCH 1/2] video: fbmem: Use 'const char' in fb_get_options()
From: Fabio Estevam @ 2013-06-18 13:26 UTC (permalink / raw)
To: linux-fbdev
Commit d20d31748 (drm: Constify the pretty-print functions) causes the following
build warning:
drivers/gpu/drm/drm_fb_helper.c:127:3: warning: passing argument 1 of 'fb_get_options' discards 'const' qualifier from pointer target type [enabled by default]
Let's change the first argument of fb_get_options from 'char *' to 'const char *'
so that we can get rid of this warning.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
drivers/video/fbmem.c | 2 +-
include/linux/fb.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 098bfc6..d8d5779 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1881,7 +1881,7 @@ static int ofonly __read_mostly;
*
* NOTE: Needed to maintain backwards compatibility
*/
-int fb_get_options(char *name, char **option)
+int fb_get_options(const char *name, char **option)
{
char *opt, *options = NULL;
int retval = 0;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index d49c60f..ffac70a 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -624,7 +624,7 @@ extern void fb_pad_aligned_buffer(u8 *dst, u32 d_pitch, u8 *src, u32 s_pitch, u3
extern void fb_set_suspend(struct fb_info *info, int state);
extern int fb_get_color_depth(struct fb_var_screeninfo *var,
struct fb_fix_screeninfo *fix);
-extern int fb_get_options(char *name, char **option);
+extern int fb_get_options(const char *name, char **option);
extern int fb_new_modelist(struct fb_info *info);
extern struct fb_info *registered_fb[FB_MAX];
--
1.8.1.2
^ permalink raw reply related
* [PATCH 2/2] i915: i915_gem: Print a size_t with %z
From: Fabio Estevam @ 2013-06-18 13:26 UTC (permalink / raw)
To: plagnioj
Cc: Fabio Estevam, linux-fbdev, Daniel Vetter, intel-gfx, Dave Airlie,
festevam
In-Reply-To: <1371561988-30989-1-git-send-email-fabio.estevam@freescale.com>
Fix the following build warning:
drivers/gpu/drm/i915/i915_gem.c:3129:3: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'size_t' [-Wformat]
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: <intel-gfx@lists.freedesktop.org>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
drivers/gpu/drm/i915/i915_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6e469e6..d497ca5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3126,7 +3126,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
* before evicting everything in a vain attempt to find space.
*/
if (obj->base.size > gtt_max) {
- DRM_ERROR("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%ld\n",
+ DRM_ERROR("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%zd\n",
obj->base.size,
map_and_fenceable ? "mappable" : "total",
gtt_max);
--
1.8.1.2
^ permalink raw reply related
* [PATCH linux-next] fb: make fp_get_options name argument const
From: Vincent Stehlé @ 2013-06-18 14:23 UTC (permalink / raw)
To: linux-next, linux-fbdev
Cc: Vincent Stehlé, Jean-Christophe Plagniol-Villard,
Tomi Valkeinen, Dave Airlie, trivial
drm_get_connector_name now returns a const value, which causes the following
compilation warning:
drivers/gpu/drm/drm_fb_helper.c: In function ‘drm_fb_helper_parse_command_line’:
drivers/gpu/drm/drm_fb_helper.c:127:3: warning: passing argument 1 of ‘fb_get_options’ discards ‘const’ qualifier from pointer target type [enabled by default]
In file included from drivers/gpu/drm/drm_fb_helper.c:35:0:
include/linux/fb.h:627:12: note: expected ‘char *’ but argument is of type ‘const char *’
As fb_get_options uses its name argument as read only, make it const. This
fixes the aforementioned compilation warning.
Signed-off-by: Vincent Stehlé <vincent.stehle@freescale.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: trivial@kernel.org
---
Hi,
I remarked this warning while building linux-next with imx_v6_v7_defconfig.
Is changing fb_get_options prototype "permitted", please?
Best regards,
V.
drivers/video/fbmem.c | 2 +-
include/linux/fb.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 098bfc6..d8d5779 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1881,7 +1881,7 @@ static int ofonly __read_mostly;
*
* NOTE: Needed to maintain backwards compatibility
*/
-int fb_get_options(char *name, char **option)
+int fb_get_options(const char *name, char **option)
{
char *opt, *options = NULL;
int retval = 0;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index d49c60f..ffac70a 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -624,7 +624,7 @@ extern void fb_pad_aligned_buffer(u8 *dst, u32 d_pitch, u8 *src, u32 s_pitch, u3
extern void fb_set_suspend(struct fb_info *info, int state);
extern int fb_get_color_depth(struct fb_var_screeninfo *var,
struct fb_fix_screeninfo *fix);
-extern int fb_get_options(char *name, char **option);
+extern int fb_get_options(const char *name, char **option);
extern int fb_new_modelist(struct fb_info *info);
extern struct fb_info *registered_fb[FB_MAX];
--
1.7.10.4
^ permalink raw reply related
* Re: [PATCH 2/2] i915: i915_gem: Print a size_t with %z
From: Geert Uytterhoeven @ 2013-06-18 14:53 UTC (permalink / raw)
To: Fabio Estevam
Cc: Linux Fbdev development list, Jean-Christophe PLAGNIOL-VILLARD,
Intel Graphics Development, Daniel Vetter, Dave Airlie,
Fabio Estevam
In-Reply-To: <1371561988-30989-2-git-send-email-fabio.estevam@freescale.com>
On Tue, Jun 18, 2013 at 3:26 PM, Fabio Estevam
<fabio.estevam@freescale.com> wrote:
> drivers/gpu/drm/i915/i915_gem.c:3129:3: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'size_t' [-Wformat]
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3126,7 +3126,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
> * before evicting everything in a vain attempt to find space.
> */
> if (obj->base.size > gtt_max) {
> - DRM_ERROR("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%ld\n",
> + DRM_ERROR("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%zd\n",
size_t is unsigned, so you better change it to %zu.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH v2 1/2] video: fbmem: Use 'const char' in fb_get_options()
From: Fabio Estevam @ 2013-06-18 15:18 UTC (permalink / raw)
To: linux-fbdev
Commit d20d31748 (drm: Constify the pretty-print functions) causes the following
build warning:
drivers/gpu/drm/drm_fb_helper.c:127:3: warning: passing argument 1 of 'fb_get_options' discards 'const' qualifier from pointer target type [enabled by default]
Let's change the first argument of fb_get_options from 'char *' to 'const char *'
so that we can get rid of this warning.
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- None
drivers/video/fbmem.c | 2 +-
include/linux/fb.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 098bfc6..d8d5779 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1881,7 +1881,7 @@ static int ofonly __read_mostly;
*
* NOTE: Needed to maintain backwards compatibility
*/
-int fb_get_options(char *name, char **option)
+int fb_get_options(const char *name, char **option)
{
char *opt, *options = NULL;
int retval = 0;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index d49c60f..ffac70a 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -624,7 +624,7 @@ extern void fb_pad_aligned_buffer(u8 *dst, u32 d_pitch, u8 *src, u32 s_pitch, u3
extern void fb_set_suspend(struct fb_info *info, int state);
extern int fb_get_color_depth(struct fb_var_screeninfo *var,
struct fb_fix_screeninfo *fix);
-extern int fb_get_options(char *name, char **option);
+extern int fb_get_options(const char *name, char **option);
extern int fb_new_modelist(struct fb_info *info);
extern struct fb_info *registered_fb[FB_MAX];
--
1.8.1.2
^ permalink raw reply related
* [PATCH v2 2/2] i915: i915_gem: Print a size_t with %z
From: Fabio Estevam @ 2013-06-18 15:18 UTC (permalink / raw)
To: plagnioj
Cc: Fabio Estevam, linux-fbdev, Daniel Vetter, intel-gfx, geert,
Dave Airlie, festevam
In-Reply-To: <1371568723-27486-1-git-send-email-fabio.estevam@freescale.com>
Fix the following build warning:
drivers/gpu/drm/i915/i915_gem.c:3129:3: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'size_t' [-Wformat]
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Dave Airlie <airlied@redhat.com>
Cc: <intel-gfx@lists.freedesktop.org>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
Changes since v1:
- Use %zu instead of %zd, since size_t is unsigned
drivers/gpu/drm/i915/i915_gem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6e469e6..d497ca5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3126,7 +3126,7 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
* before evicting everything in a vain attempt to find space.
*/
if (obj->base.size > gtt_max) {
- DRM_ERROR("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%ld\n",
+ DRM_ERROR("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%zu\n",
obj->base.size,
map_and_fenceable ? "mappable" : "total",
gtt_max);
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] build some drivers only when compile-testing
From: Greg Kroah-Hartman @ 2013-06-18 16:04 UTC (permalink / raw)
To: Jiri Slaby
Cc: Jeff Mahoney, jirislaby, linux-kernel, Andrew Morton,
Linus Torvalds, Alexander Shishkin, linux-usb,
Florian Tobias Schandinat, linux-geode, linux-fbdev,
Richard Cochran, netdev, Ben Hutchings, Keller, Jacob E,
Michal Marek, tomi.valkeinen
In-Reply-To: <51BF6BFF.7050705@suse.cz>
On Mon, Jun 17, 2013 at 10:05:19PM +0200, Jiri Slaby wrote:
> On 05/23/2013 05:09 AM, Jeff Mahoney wrote:
> > On 5/22/13 10:23 PM, Greg Kroah-Hartman wrote:
> >> On Wed, May 22, 2013 at 11:18:46AM +0200, Jiri Slaby wrote:
> >>> Some drivers can be built on more platforms than they run on. This
> >>> causes users and distributors packaging burden when they have to
> >>> manually deselect some drivers from their allmodconfigs. Or sometimes
> >>> it is even impossible to disable the drivers without patching the
> >>> kernel.
> >>>
> >>> Introduce a new config option COMPILE_TEST and make all those drivers
> >>> to depend on the platform they run on, or on the COMPILE_TEST option.
> >>> Now, when users/distributors choose COMPILE_TEST=n they will not have
> >>> the drivers in their allmodconfig setups, but developers still can
> >>> compile-test them with COMPILE_TEST=y.
> >>
> >> I understand the urge, and it's getting hard for distros to handle these
> >> drivers that just don't work on other architectures, but it's really
> >> valuable to ensure that they build properly, for those of us that don't
> >> have many/any cross compilers set up.
>
> But this is exactly what COMPILE_TEST will give us when set to "y", or
> am I missing something?
>
> >>> Now the drivers where we use this new option:
> >>> * PTP_1588_CLOCK_PCH: The PCH EG20T is only compatible with Intel Atom
> >>> processors so it should depend on x86.
> >>> * FB_GEODE: Geode is 32-bit only so only enable it for X86_32.
> >>> * USB_CHIPIDEA_IMX: The OF_DEVICE dependency will be met on powerpc
> >>> systems -- which do not actually support the hardware via that
> >>> method.
> >>
> >> This seems ripe to start to get really messy, really quickly. Shouldn't
> >> "default configs" handle if this "should" be enabled for a platform or
> >> not, and let the rest of us just build them with no problems?
> >
> > If every time a new Kconfig option is added, corresponding default
> > config updates come with it, sure. I just don't see that happening,
> > especially when it can be done much more clearly in the Kconfig while
> > the developer is writing the driver.
> >
> >> What problems is this causing you? Are you running out of space in
> >> kernel packages with drivers that will never be actually used?
> >
> > Wasted build resources. Wasted disk space on /every/ system the kernel
> > package is installed on. We're all trying to pare down the kernel
> > packages to eliminate wasted space and doing it manually means a bunch
> > of research, sometimes with incorrect assumptions about the results,
> > needs to be done by someone not usually associated with that code. That
> > research gets repeated by people maintaining kernel packages for pretty
> > much every distro.
>
> I second all the above.
>
> >>> +config COMPILE_TEST
> >>> + bool "Compile also drivers which will not load" if EXPERT
> >>
> >> EXPERT is getting to be the "let's hide it here" option, isn't it...
> >>
> >> I don't know, if no one else strongly objects, I can be convinced that
> >> this is needed, but so far, I don't see why it really is, or what this
> >> is going to help with.
> >
> > I'm not convinced adding a || COMPILE_TEST option to every driver that
> > may be arch specific is the best way to go either. Perhaps adding a new
> > Kconfig verb called "archdepends on" or something that will evaluate as
> > true if COMPILE_TEST is enabled but will evaluate the conditional if
> > not. *waves hands*
>
> Sam Ravnborg (the kconfig ex-maintainer) once wrote that he doesn't want
> to extend the kconfig language for this purpose (which I support). That
> a config option is fine and sufficient in this case [1]. Except he
> called the config option "SHOW_ALL_DRIVERS". Adding the current
> maintainer to CCs ;).
>
> [1] http://comments.gmane.org/gmane.linux.kbuild.devel/9829
>
> The last point I inclined to the Greg's argument to remove the EXPERT
> dependency.
>
> So currently I have what is attached... Comments?
Looks good to me, want me to queue it up through my char/misc driver
tree for 3.11?
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 1/2] video: fbmem: Use 'const char' in fb_get_options()
From: David Airlie @ 2013-06-18 23:00 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1371568723-27486-1-git-send-email-fabio.estevam@freescale.com>
----- Original Message -----
> From: "Fabio Estevam" <fabio.estevam@freescale.com>
> To: plagnioj@jcrosoft.com
> Cc: linux-fbdev@vger.kernel.org, festevam@gmail.com, geert@linux-m68k.org, "Fabio Estevam"
> <fabio.estevam@freescale.com>, "Ville Syrjälä" <ville.syrjala@linux.intel.com>, "Daniel Vetter"
> <daniel.vetter@ffwll.ch>, "Dave Airlie" <airlied@redhat.com>
> Sent: Wednesday, 19 June, 2013 1:18:42 AM
> Subject: [PATCH v2 1/2] video: fbmem: Use 'const char' in fb_get_options()
>
> Commit d20d31748 (drm: Constify the pretty-print functions) causes the
> following
> build warning:
This patch should already be merged into the fbdev tree, at least the maintainer said it was, hence why I didn't merge it into
my drm-next tree.
Is the fbdev queue not in the drm-next?
Dave.
^ permalink raw reply
* RE: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Inki Dae @ 2013-06-19 5:45 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1371467722-665-1-git-send-email-inki.dae@samsung.com>
> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Tuesday, June 18, 2013 6:47 PM
> To: Inki Dae
> Cc: 'Russell King - ARM Linux'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI
> mailing list'; 'myungjoo.ham'; 'YoungJun Cho'; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org
> Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization
> framework
>
> Am Dienstag, den 18.06.2013, 18:04 +0900 schrieb Inki Dae:
> [...]
> >
> > > a display device driver. It shouldn't be used within a single driver
> > > as a means of passing buffers between userspace and kernel space.
> >
> > What I try to do is not really such ugly thing. What I try to do is to
> > notify that, when CPU tries to access a buffer , to kernel side through
> > dmabuf interface. So it's not really to send the buffer to kernel.
> >
> > Thanks,
> > Inki Dae
> >
> The most basic question about why you are trying to implement this sort
> of thing in the dma_buf framework still stands.
>
> Once you imported a dma_buf into your DRM driver it's a GEM object and
> you can and should use the native DRM ioctls to prepare/end a CPU access
> to this BO. Then internally to your driver you can use the dma_buf
> reservation/fence stuff to provide the necessary cross-device sync.
>
I don't really want that is used only for DRM drivers. We really need it for all other DMA devices; i.e., v4l2 based drivers. That is what I try to do. And my approach uses reservation to use dma-buf resources but not dma fence stuff anymore. However, I'm looking into Radeon DRM driver for why we need dma fence stuff, and how we can use it if needed.
Thanks,
Inki Dae
> Regards,
> Lucas
> --
> Pengutronix e.K. | Lucas Stach |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH] build some drivers only when compile-testing
From: Jiri Slaby @ 2013-06-19 6:50 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jeff Mahoney, jirislaby, linux-kernel, Andrew Morton,
Linus Torvalds, Alexander Shishkin, linux-usb,
Florian Tobias Schandinat, linux-geode, linux-fbdev,
Richard Cochran, netdev, Ben Hutchings, Keller, Jacob E,
Michal Marek, tomi.valkeinen
In-Reply-To: <20130618160427.GE28961@kroah.com>
On 06/18/2013 06:04 PM, Greg Kroah-Hartman wrote:
>> So currently I have what is attached... Comments?
>
> Looks good to me, want me to queue it up through my char/misc driver
> tree for 3.11?
If there are no objections... Whoever picks that up, I would be happy 8-).
--
js
suse labs
^ permalink raw reply
* Re: [PATCH] build some drivers only when compile-testing
From: Tomi Valkeinen @ 2013-06-19 7:10 UTC (permalink / raw)
To: Jiri Slaby
Cc: Jeff Mahoney, Greg Kroah-Hartman, jirislaby, linux-kernel,
Andrew Morton, Linus Torvalds, Alexander Shishkin, linux-usb,
Florian Tobias Schandinat, linux-geode, linux-fbdev,
Richard Cochran, netdev, Ben Hutchings, Keller, Jacob E,
Michal Marek
In-Reply-To: <51BF6BFF.7050705@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 446 bytes --]
On 17/06/13 23:05, Jiri Slaby wrote:
> The last point I inclined to the Greg's argument to remove the EXPERT
> dependency.
>
> So currently I have what is attached... Comments?
The patch looks a bit odd with the USB_CHIPIDEA_IMX parts. You're not
adding COMPILE_TEST there, but you're adding a totally new config
option, and also changing the Makefile.
Maybe that's ok, but there's no mention about this in the desc.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH] build some drivers only when compile-testing
From: Jiri Slaby @ 2013-06-19 7:12 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jeff Mahoney, Greg Kroah-Hartman, jirislaby, linux-kernel,
Andrew Morton, Linus Torvalds, Alexander Shishkin, linux-usb,
Florian Tobias Schandinat, linux-geode, linux-fbdev,
Richard Cochran, netdev, Ben Hutchings, Keller, Jacob E,
Michal Marek
In-Reply-To: <51C1595D.60706@ti.com>
On 06/19/2013 09:10 AM, Tomi Valkeinen wrote:
> On 17/06/13 23:05, Jiri Slaby wrote:
>
>> The last point I inclined to the Greg's argument to remove the
>> EXPERT dependency.
>>
>> So currently I have what is attached... Comments?
>
> The patch looks a bit odd with the USB_CHIPIDEA_IMX parts. You're
> not adding COMPILE_TEST there, but you're adding a totally new
> config option, and also changing the Makefile.
Look:
+config USB_CHIPIDEA_IMX
+ bool "ChipIdea IMX support"
+ depends on OF_DEVICE && (ARM || COMPILE_TEST)
COMPILE_TEST added here ----------------^^^^^^^^^^^^
--
js
suse labs
^ permalink raw reply
* Re: [PATCH] build some drivers only when compile-testing
From: Tomi Valkeinen @ 2013-06-19 7:19 UTC (permalink / raw)
To: Jiri Slaby
Cc: Jeff Mahoney, Greg Kroah-Hartman, jirislaby, linux-kernel,
Andrew Morton, Linus Torvalds, Alexander Shishkin, linux-usb,
Florian Tobias Schandinat, linux-geode, linux-fbdev,
Richard Cochran, netdev, Ben Hutchings, Keller, Jacob E,
Michal Marek
In-Reply-To: <51C159F8.8090604@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]
On 19/06/13 10:12, Jiri Slaby wrote:
> On 06/19/2013 09:10 AM, Tomi Valkeinen wrote:
>> On 17/06/13 23:05, Jiri Slaby wrote:
>>
>>> The last point I inclined to the Greg's argument to remove the
>>> EXPERT dependency.
>>>
>>> So currently I have what is attached... Comments?
>>
>> The patch looks a bit odd with the USB_CHIPIDEA_IMX parts. You're
>> not adding COMPILE_TEST there, but you're adding a totally new
>> config option, and also changing the Makefile.
>
> Look:
>
> +config USB_CHIPIDEA_IMX
> + bool "ChipIdea IMX support"
> + depends on OF_DEVICE && (ARM || COMPILE_TEST)
>
> COMPILE_TEST added here ----------------^^^^^^^^^^^^
My point was that you're not adding COMPILE_TEST to an existing config
option, you're creating a totally new config option.
As I said, that's probably ok, but it'd be nice to mention extra things
like that in the desc. The pedantic approach would be to do the makefile
and Kconfig change in an earlier patch, and then add only COMPILE_TEST.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] video: fbmem: Use 'const char' in fb_get_options()
From: Ville Syrjälä @ 2013-06-19 7:33 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1371561988-30989-1-git-send-email-fabio.estevam@freescale.com>
On Tue, Jun 18, 2013 at 10:26:27AM -0300, Fabio Estevam wrote:
> Commit d20d31748 (drm: Constify the pretty-print functions) causes the following
> build warning:
>
> drivers/gpu/drm/drm_fb_helper.c:127:3: warning: passing argument 1 of 'fb_get_options' discards 'const' qualifier from pointer target type [enabled by default]
>
> Let's change the first argument of fb_get_options from 'char *' to 'const char *'
> so that we can get rid of this warning.
I already sent the same patch alongside the drm constify patches. It
just needs to trickle back into the drm tree via the fbdev folks.
>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Dave Airlie <airlied@redhat.com>
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> drivers/video/fbmem.c | 2 +-
> include/linux/fb.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 098bfc6..d8d5779 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1881,7 +1881,7 @@ static int ofonly __read_mostly;
> *
> * NOTE: Needed to maintain backwards compatibility
> */
> -int fb_get_options(char *name, char **option)
> +int fb_get_options(const char *name, char **option)
> {
> char *opt, *options = NULL;
> int retval = 0;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index d49c60f..ffac70a 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -624,7 +624,7 @@ extern void fb_pad_aligned_buffer(u8 *dst, u32 d_pitch, u8 *src, u32 s_pitch, u3
> extern void fb_set_suspend(struct fb_info *info, int state);
> extern int fb_get_color_depth(struct fb_var_screeninfo *var,
> struct fb_fix_screeninfo *fix);
> -extern int fb_get_options(char *name, char **option);
> +extern int fb_get_options(const char *name, char **option);
> extern int fb_new_modelist(struct fb_info *info);
>
> extern struct fb_info *registered_fb[FB_MAX];
> --
> 1.8.1.2
>
--
Ville Syrjälä
Intel OTC
^ permalink raw reply
* DRM/KMS/FB: HDMI hotplug displays 1024x768 resolution instead of native mode
From: Jiada Wang @ 2013-06-19 7:37 UTC (permalink / raw)
To: linux-arm-kernel, airlied, linux-fbdev-devel; +Cc: Jiada Wang
Hi,
I am implementing HDMI hotplug feature for i.MX6 platform,
the basic function has already been done and works
But there is is an "issue",
When bootup system without HDMI cable attached, fbcon is initialised
with fbsize: 1024x768, after plug-in HDMI cable, I can see in debug log,
display's valid video_mode has been read via DDC, but in
drm_fb_helper_hotplug_event(), only size lower than
fb_helper->fb->width/fb_helper->fb->height, will be accepted,
So instead of the display's native mode, 1024x768 is displayed.
Is it correct DRM/fbdev behaviour?
Thanks,
Jiada
^ permalink raw reply
* [RFC PATCH v3] dmabuf-sync: Introduce buffer synchronization framework
From: Inki Dae @ 2013-06-19 9:10 UTC (permalink / raw)
To: dri-devel, linux-fbdev, linux-arm-kernel, linux-media
Cc: maarten.lankhorst, daniel, robdclark, kyungmin.park, myungjoo.ham,
yj44.cho, Inki Dae
In-Reply-To: <1371467722-665-1-git-send-email-inki.dae@samsung.com>
This patch adds a buffer synchronization framework based on DMA BUF[1]
and reservation[2] to use dma-buf resource, and based on ww-mutexes[3]
for lock mechanism.
The purpose of this framework is to provide not only buffer access control
to CPU and DMA but also easy-to-use interfaces for device drivers and
potentially user application (not implemented for user applications, yet).
And this framework can be used for all dma devices using system memory as
dma buffer, especially for most ARM based SoCs.
Changelog v3:
- remove cache operation relevant codes and update document file.
Changelog v2:
- use atomic_add_unless to avoid potential bug.
- add a macro for checking valid access type.
- code clean.
The mechanism of this framework has the following steps,
1. Register dmabufs to a sync object - A task gets a new sync object and
can add one or more dmabufs that the task wants to access.
This registering should be performed when a device context or an event
context such as a page flip event is created or before CPU accesses a shared
buffer.
dma_buf_sync_get(a sync object, a dmabuf);
2. Lock a sync object - A task tries to lock all dmabufs added in its own
sync object. Basically, the lock mechanism uses ww-mutex[1] to avoid dead
lock issue and for race condition between CPU and CPU, CPU and DMA, and DMA
and DMA. Taking a lock means that others cannot access all locked dmabufs
until the task that locked the corresponding dmabufs, unlocks all the locked
dmabufs.
This locking should be performed before DMA or CPU accesses these dmabufs.
dma_buf_sync_lock(a sync object);
3. Unlock a sync object - The task unlocks all dmabufs added in its own sync
object. The unlock means that the DMA or CPU accesses to the dmabufs have
been completed so that others may access them.
This unlocking should be performed after DMA or CPU has completed accesses
to the dmabufs.
dma_buf_sync_unlock(a sync object);
4. Unregister one or all dmabufs from a sync object - A task unregisters
the given dmabufs from the sync object. This means that the task dosen't
want to lock the dmabufs.
The unregistering should be performed after DMA or CPU has completed
accesses to the dmabufs or when dma_buf_sync_lock() is failed.
dma_buf_sync_put(a sync object, a dmabuf);
dma_buf_sync_put_all(a sync object);
The described steps may be summarized as:
get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
This framework includes the following two features.
1. read (shared) and write (exclusive) locks - A task is required to declare
the access type when the task tries to register a dmabuf;
READ, WRITE, READ DMA, or WRITE DMA.
The below is example codes,
struct dmabuf_sync *sync;
sync = dmabuf_sync_init(NULL, "test sync");
dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_R);
...
And the below can be used as access types:
DMA_BUF_ACCESS_R - CPU will access a buffer for read.
DMA_BUF_ACCESS_W - CPU will access a buffer for read or write.
DMA_BUF_ACCESS_DMA_R - DMA will access a buffer for read
DMA_BUF_ACCESS_DMA_W - DMA will access a buffer for read or
write.
2. Mandatory resource releasing - a task cannot hold a lock indefinitely.
A task may never try to unlock a buffer after taking a lock to the buffer.
In this case, a timer handler to the corresponding sync object is called
in five (default) seconds and then the timed-out buffer is unlocked by work
queue handler to avoid lockups and to enforce resources of the buffer.
The below is how to use:
1. Allocate and Initialize a sync object:
struct dmabuf_sync *sync;
sync = dmabuf_sync_init(NULL, "test sync");
...
2. Add a dmabuf to the sync object when setting up dma buffer relevant
registers:
dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
...
3. Lock all dmabufs of the sync object before DMA or CPU accesses
the dmabufs:
dmabuf_sync_lock(sync);
...
4. Now CPU or DMA can access all dmabufs locked in step 3.
5. Unlock all dmabufs added in a sync object after DMA or CPU access
to these dmabufs is completed:
dmabuf_sync_unlock(sync);
And call the following functions to release all resources,
dmabuf_sync_put_all(sync);
dmabuf_sync_fini(sync);
You can refer to actual example codes:
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
commit/?h=dmabuf-sync&id@30bdee9bab5841ad32faade528d04cc0c5fc94
https://git.kernel.org/cgit/linux/kernel/git/daeinki/drm-exynos.git/
commit/?h=dmabuf-sync&idla548e9ea9e865592719ef6b1cde58366af9f5c
[1] http://lwn.net/Articles/470339/
[2] http://lwn.net/Articles/532616/
[3] https://patchwork.kernel.org/patch/2625361/
Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Documentation/dma-buf-sync.txt | 199 ++++++++++++++++
drivers/base/Kconfig | 7 +
drivers/base/Makefile | 1 +
drivers/base/dmabuf-sync.c | 501 ++++++++++++++++++++++++++++++++++++++++
include/linux/dma-buf.h | 14 ++
include/linux/dmabuf-sync.h | 115 +++++++++
include/linux/reservation.h | 7 +
7 files changed, 844 insertions(+), 0 deletions(-)
create mode 100644 Documentation/dma-buf-sync.txt
create mode 100644 drivers/base/dmabuf-sync.c
create mode 100644 include/linux/dmabuf-sync.h
diff --git a/Documentation/dma-buf-sync.txt b/Documentation/dma-buf-sync.txt
new file mode 100644
index 0000000..134de7b
--- /dev/null
+++ b/Documentation/dma-buf-sync.txt
@@ -0,0 +1,199 @@
+ DMA Buffer Synchronization Framework
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+ Inki Dae
+ <inki dot dae at samsung dot com>
+ <daeinki at gmail dot com>
+
+This document is a guide for device-driver writers describing the DMA buffer
+synchronization API. This document also describes how to use the API to
+use buffer synchronization mechanism between DMA and DMA, CPU and DMA, and
+CPU and CPU.
+
+The DMA Buffer synchronization API provides buffer synchronization mechanism;
+i.e., buffer access control to CPU and DMA, and easy-to-use interfaces for
+device drivers and potentially user application (not implemented for user
+applications, yet). And this API can be used for all dma devices using system
+memory as dma buffer, especially for most ARM based SoCs.
+
+
+Motivation
+----------
+
+Buffer synchronization issue between DMA and DMA:
+ Sharing a buffer, a device cannot be aware of when the other device
+ will access the shared buffer: a device may access a buffer containing
+ wrong data if the device accesses the shared buffer while another
+ device is still accessing the shared buffer.
+ Therefore, a user process should have waited for the completion of DMA
+ access by another device before a device tries to access the shared
+ buffer.
+
+Buffer synchronization issue between CPU and DMA:
+ A user process should consider that when having to send a buffer, filled
+ by CPU, to a device driver for the device driver to access the buffer as
+ a input buffer while CPU and DMA are sharing the buffer.
+ This means that the user process needs to understand how the device
+ driver is worked. Hence, the conventional mechanism not only makes
+ user application complicated but also incurs performance overhead.
+
+Buffer synchronization issue between CPU and CPU:
+ In case that two processes share one buffer; shared with DMA also,
+ they may need some mechanism to allow process B to access the shared
+ buffer after the completion of CPU access by process A.
+ Therefore, process B should have waited for the completion of CPU access
+ by process A using the mechanism before trying to access the shared
+ buffer.
+
+What is the best way to solve these buffer synchronization issues?
+ We may need a common object that a device driver and a user process
+ notify the common object of when they try to access a shared buffer.
+ That way we could decide when we have to allow or not to allow for CPU
+ or DMA to access the shared buffer through the common object.
+ If so, what could become the common object? Right, that's a dma-buf[1].
+ Now we have already been using the dma-buf to share one buffer with
+ other drivers.
+
+
+Basic concept
+-------------
+
+The mechanism of this framework has the following steps,
+ 1. Register dmabufs to a sync object - A task gets a new sync object and
+ can add one or more dmabufs that the task wants to access.
+ This registering should be performed when a device context or an event
+ context such as a page flip event is created or before CPU accesses a shared
+ buffer.
+
+ dma_buf_sync_get(a sync object, a dmabuf);
+
+ 2. Lock a sync object - A task tries to lock all dmabufs added in its own
+ sync object. Basically, the lock mechanism uses ww-mutexes[2] to avoid dead
+ lock issue and for race condition between CPU and CPU, CPU and DMA, and DMA
+ and DMA. Taking a lock means that others cannot access all locked dmabufs
+ until the task that locked the corresponding dmabufs, unlocks all the locked
+ dmabufs.
+ This locking should be performed before DMA or CPU accesses these dmabufs.
+
+ dma_buf_sync_lock(a sync object);
+
+ 3. Unlock a sync object - The task unlocks all dmabufs added in its own sync
+ object. The unlock means that the DMA or CPU accesses to the dmabufs have
+ been completed so that others may access them.
+ This unlocking should be performed after DMA or CPU has completed accesses
+ to the dmabufs.
+
+ dma_buf_sync_unlock(a sync object);
+
+ 4. Unregister one or all dmabufs from a sync object - A task unregisters
+ the given dmabufs from the sync object. This means that the task dosen't
+ want to lock the dmabufs.
+ The unregistering should be performed after DMA or CPU has completed
+ accesses to the dmabufs or when dma_buf_sync_lock() is failed.
+
+ dma_buf_sync_put(a sync object, a dmabuf);
+ dma_buf_sync_put_all(a sync object);
+
+ The described steps may be summarized as:
+ get -> lock -> CPU or DMA access to a buffer/s -> unlock -> put
+
+This framework includes the following two features.
+ 1. read (shared) and write (exclusive) locks - A task is required to declare
+ the access type when the task tries to register a dmabuf;
+ READ, WRITE, READ DMA, or WRITE DMA.
+
+ The below is example codes,
+ struct dmabuf_sync *sync;
+
+ sync = dmabuf_sync_init(NULL, "test sync");
+
+ dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_R);
+ ...
+
+ 2. Mandatory resource releasing - a task cannot hold a lock indefinitely.
+ A task may never try to unlock a buffer after taking a lock to the buffer.
+ In this case, a timer handler to the corresponding sync object is called
+ in five (default) seconds and then the timed-out buffer is unlocked by work
+ queue handler to avoid lockups and to enforce resources of the buffer.
+
+
+Access types
+------------
+
+DMA_BUF_ACCESS_R - CPU will access a buffer for read.
+DMA_BUF_ACCESS_W - CPU will access a buffer for read or write.
+DMA_BUF_ACCESS_DMA_R - DMA will access a buffer for read
+DMA_BUF_ACCESS_DMA_W - DMA will access a buffer for read or write.
+
+
+API set
+-------
+
+bool is_dmabuf_sync_supported(void)
+ - Check if dmabuf sync is supported or not.
+
+struct dmabuf_sync *dmabuf_sync_init(void *priv, const char *name)
+ - Allocate and initialize a new sync object. The caller can get a new
+ sync object for buffer synchronization. priv is used to set caller's
+ private data and name is the name of sync object.
+
+void dmabuf_sync_fini(struct dmabuf_sync *sync)
+ - Release all resources to the sync object.
+
+int dmabuf_sync_get(struct dmabuf_sync *sync, void *sync_buf,
+ unsigned int type)
+ - Add a dmabuf to a sync object. The caller can group multiple dmabufs
+ by calling this function several times. Internally, this function also
+ takes a reference to a dmabuf.
+
+void dmabuf_sync_put(struct dmabuf_sync *sync, struct dma_buf *dmabuf)
+ - Remove a given dmabuf from a sync object. Internally, this function
+ also release every reference to the given dmabuf.
+
+void dmabuf_sync_put_all(struct dmabuf_sync *sync)
+ - Remove all dmabufs added in a sync object. Internally, this function
+ also release every reference to the dmabufs of the sync object.
+
+int dmabuf_sync_lock(struct dmabuf_sync *sync)
+ - Lock all dmabufs added in a sync object. The caller should call this
+ function prior to CPU or DMA access to the dmabufs so that others can
+ not access the dmabufs. Internally, this function avoids dead lock
+ issue with ww-mutexes.
+
+int dmabuf_sync_unlock(struct dmabuf_sync *sync)
+ - Unlock all dmabufs added in a sync object. The caller should call
+ this function after CPU or DMA access to the dmabufs is completed so
+ that others can access the dmabufs.
+
+
+Tutorial
+--------
+
+1. Allocate and Initialize a sync object:
+ struct dmabuf_sync *sync;
+
+ sync = dmabuf_sync_init(NULL, "test sync");
+ ...
+
+2. Add a dmabuf to the sync object when setting up dma buffer relevant registers:
+ dmabuf_sync_get(sync, dmabuf, DMA_BUF_ACCESS_READ);
+ ...
+
+3. Lock all dmabufs of the sync object before DMA or CPU accesses the dmabufs:
+ dmabuf_sync_lock(sync);
+ ...
+
+4. Now CPU or DMA can access all dmabufs locked in step 3.
+
+5. Unlock all dmabufs added in a sync object after DMA or CPU access to these
+ dmabufs is completed:
+ dmabuf_sync_unlock(sync);
+
+ And call the following functions to release all resources,
+ dmabuf_sync_put_all(sync);
+ dmabuf_sync_fini(sync);
+
+
+References:
+[1] http://lwn.net/Articles/470339/
+[2] https://patchwork.kernel.org/patch/2625361/
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 5ccf182..54a1d5a 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -212,6 +212,13 @@ config FENCE_TRACE
lockup related problems for dma-buffers shared across multiple
devices.
+config DMABUF_SYNC
+ bool "DMABUF Synchronization Framework"
+ depends on DMA_SHARED_BUFFER
+ help
+ This option enables dmabuf sync framework for buffer synchronization between
+ DMA and DMA, CPU and DMA, and CPU and CPU.
+
config CMA
bool "Contiguous Memory Allocator"
depends on HAVE_DMA_CONTIGUOUS && HAVE_MEMBLOCK
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 8a55cb9..599f6c90 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -11,6 +11,7 @@ obj-y += power/
obj-$(CONFIG_HAS_DMA) += dma-mapping.o
obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
obj-$(CONFIG_DMA_SHARED_BUFFER) += dma-buf.o fence.o reservation.o
+obj-$(CONFIG_DMABUF_SYNC) += dmabuf-sync.o
obj-$(CONFIG_ISA) += isa.o
obj-$(CONFIG_FW_LOADER) += firmware_class.o
obj-$(CONFIG_NUMA) += node.o
diff --git a/drivers/base/dmabuf-sync.c b/drivers/base/dmabuf-sync.c
new file mode 100644
index 0000000..7f3ac81
--- /dev/null
+++ b/drivers/base/dmabuf-sync.c
@@ -0,0 +1,501 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ * Authors:
+ * Inki Dae <inki.dae@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+
+#include <linux/dmabuf-sync.h>
+
+#define MAX_SYNC_TIMEOUT 5 /* Second. */
+
+int dmabuf_sync_enabled = 1;
+
+MODULE_PARM_DESC(enabled, "Check if dmabuf sync is supported or not");
+module_param_named(enabled, dmabuf_sync_enabled, int, 0444);
+
+static void dmabuf_sync_timeout_worker(struct work_struct *work)
+{
+ struct dmabuf_sync *sync = container_of(work, struct dmabuf_sync, work);
+ struct dmabuf_sync_object *sobj;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (WARN_ON(!sobj->robj))
+ continue;
+
+ printk(KERN_WARNING "%s: timeout = 0x%x [type = %d, " \
+ "refcnt = %d, locked = %d]\n",
+ sync->name, (u32)sobj->dmabuf,
+ sobj->access_type,
+ atomic_read(&sobj->robj->shared_cnt),
+ sobj->robj->locked);
+
+ /* unlock only valid sync object. */
+ if (!sobj->robj->locked)
+ continue;
+
+ if (sobj->robj->shared &&
+ atomic_add_unless(&sobj->robj->shared_cnt, -1, 1))
+ continue;
+
+ ww_mutex_unlock(&sobj->robj->lock);
+
+ if (sobj->access_type & DMA_BUF_ACCESS_R)
+ printk(KERN_WARNING "%s: r-unlocked = 0x%x\n",
+ sync->name, (u32)sobj->dmabuf);
+ else
+ printk(KERN_WARNING "%s: w-unlocked = 0x%x\n",
+ sync->name, (u32)sobj->dmabuf);
+ }
+
+ sync->status = 0;
+ mutex_unlock(&sync->lock);
+
+ dmabuf_sync_put_all(sync);
+ dmabuf_sync_fini(sync);
+}
+
+static void dmabuf_sync_lock_timeout(unsigned long arg)
+{
+ struct dmabuf_sync *sync = (struct dmabuf_sync *)arg;
+
+ schedule_work(&sync->work);
+}
+
+static int dmabuf_sync_lock_objs(struct dmabuf_sync *sync,
+ struct ww_acquire_ctx *ctx)
+{
+ struct dmabuf_sync_object *contended_sobj = NULL;
+ struct dmabuf_sync_object *res_sobj = NULL;
+ struct dmabuf_sync_object *sobj = NULL;
+ int ret;
+
+ if (ctx)
+ ww_acquire_init(ctx, &reservation_ww_class);
+
+retry:
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (WARN_ON(!sobj->robj))
+ continue;
+
+ /* Don't lock in case of read and read. */
+ if (sobj->robj->accessed_type & DMA_BUF_ACCESS_R &&
+ sobj->access_type & DMA_BUF_ACCESS_R) {
+ atomic_inc(&sobj->robj->shared_cnt);
+ sobj->robj->shared = true;
+ continue;
+ }
+
+ if (sobj = res_sobj) {
+ res_sobj = NULL;
+ continue;
+ }
+
+ ret = ww_mutex_lock(&sobj->robj->lock, ctx);
+ if (ret < 0) {
+ contended_sobj = sobj;
+
+ if (ret = -EDEADLK)
+ printk(KERN_WARNING"%s: deadlock = 0x%x\n",
+ sync->name, (u32)sobj->dmabuf);
+ goto err;
+ }
+
+ sobj->robj->locked = true;
+ }
+
+ if (ctx)
+ ww_acquire_done(ctx);
+
+ init_timer(&sync->timer);
+
+ sync->timer.data = (unsigned long)sync;
+ sync->timer.function = dmabuf_sync_lock_timeout;
+ sync->timer.expires = jiffies + (HZ * MAX_SYNC_TIMEOUT);
+
+ add_timer(&sync->timer);
+
+ return 0;
+
+err:
+ list_for_each_entry_continue_reverse(sobj, &sync->syncs, head) {
+ /* Don't need to unlock in case of read and read. */
+ if (atomic_add_unless(&sobj->robj->shared_cnt, -1, 1))
+ continue;
+
+ ww_mutex_unlock(&sobj->robj->lock);
+ sobj->robj->locked = false;
+ }
+
+ if (res_sobj) {
+ if (!atomic_add_unless(&res_sobj->robj->shared_cnt, -1, 1)) {
+ ww_mutex_unlock(&res_sobj->robj->lock);
+ res_sobj->robj->locked = false;
+ }
+ }
+
+ if (ret = -EDEADLK) {
+ ww_mutex_lock_slow(&contended_sobj->robj->lock, ctx);
+ res_sobj = contended_sobj;
+
+ goto retry;
+ }
+
+ if (ctx)
+ ww_acquire_fini(ctx);
+
+ return ret;
+}
+
+static void dmabuf_sync_unlock_objs(struct dmabuf_sync *sync,
+ struct ww_acquire_ctx *ctx)
+{
+ struct dmabuf_sync_object *sobj;
+
+ if (list_empty(&sync->syncs))
+ return;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (sobj->robj->shared) {
+ if (atomic_add_unless(&sobj->robj->shared_cnt, -1 , 1))
+ continue;
+
+ ww_mutex_unlock(&sobj->robj->lock);
+ sobj->robj->shared = false;
+ sobj->robj->locked = false;
+ } else {
+ ww_mutex_unlock(&sobj->robj->lock);
+ sobj->robj->locked = false;
+ }
+ }
+
+ mutex_unlock(&sync->lock);
+
+ if (ctx)
+ ww_acquire_fini(ctx);
+
+ del_timer(&sync->timer);
+}
+
+/**
+ * is_dmabuf_sync_supported - Check if dmabuf sync is supported or not.
+ */
+bool is_dmabuf_sync_supported(void)
+{
+ return dmabuf_sync_enabled = 1;
+}
+EXPORT_SYMBOL(is_dmabuf_sync_supported);
+
+/**
+ * dmabuf_sync_init - Allocate and initialize a dmabuf sync.
+ *
+ * @priv: A device private data.
+ * @name: A sync object name.
+ *
+ * This function should be called when a device context or an event
+ * context such as a page flip event is created. And the created
+ * dmabuf_sync object should be set to the context.
+ * The caller can get a new sync object for buffer synchronization
+ * through this function.
+ */
+struct dmabuf_sync *dmabuf_sync_init(void *priv, const char *name)
+{
+ struct dmabuf_sync *sync;
+
+ sync = kzalloc(sizeof(*sync), GFP_KERNEL);
+ if (!sync)
+ return ERR_PTR(-ENOMEM);
+
+ strncpy(sync->name, name, ARRAY_SIZE(sync->name) - 1);
+
+ sync->priv = priv;
+ INIT_LIST_HEAD(&sync->syncs);
+ mutex_init(&sync->lock);
+ INIT_WORK(&sync->work, dmabuf_sync_timeout_worker);
+
+ return sync;
+}
+EXPORT_SYMBOL(dmabuf_sync_init);
+
+/**
+ * dmabuf_sync_fini - Release a given dmabuf sync.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_init call to release relevant resources, and after
+ * dmabuf_sync_unlock function is called.
+ */
+void dmabuf_sync_fini(struct dmabuf_sync *sync)
+{
+ if (WARN_ON(!sync))
+ return;
+
+ kfree(sync);
+}
+EXPORT_SYMBOL(dmabuf_sync_fini);
+
+/*
+ * dmabuf_sync_get_obj - Add a given object to syncs list.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ * @dmabuf: An object to dma_buf structure.
+ * @type: A access type to a dma buf.
+ * The DMA_BUF_ACCESS_R means that this dmabuf could be accessed by
+ * others for read access. On the other hand, the DMA_BUF_ACCESS_W
+ * means that this dmabuf couldn't be accessed by others but would be
+ * accessed by caller's dma exclusively. And the DMA_BUF_ACCESS_DMA can be
+ * combined.
+ *
+ * This function creates and initializes a new dmabuf sync object and it adds
+ * the dmabuf sync object to syncs list to track and manage all dmabufs.
+ */
+static int dmabuf_sync_get_obj(struct dmabuf_sync *sync, struct dma_buf *dmabuf,
+ unsigned int type)
+{
+ struct dmabuf_sync_object *sobj;
+
+ if (!dmabuf->resv) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ if (!IS_VALID_DMA_BUF_ACCESS_TYPE(type))
+ return -EINVAL;
+
+ if ((type & DMA_BUF_ACCESS_RW) = DMA_BUF_ACCESS_RW)
+ type &= ~DMA_BUF_ACCESS_R;
+
+ sobj = kzalloc(sizeof(*sobj), GFP_KERNEL);
+ if (!sobj) {
+ WARN_ON(1);
+ return -ENOMEM;
+ }
+
+ sobj->dmabuf = dmabuf;
+ sobj->robj = dmabuf->resv;
+
+ mutex_lock(&sync->lock);
+ list_add_tail(&sobj->head, &sync->syncs);
+ mutex_unlock(&sync->lock);
+
+ get_dma_buf(dmabuf);
+
+ sobj->access_type = type;
+
+ return 0;
+}
+
+/*
+ * dmabuf_sync_put_obj - Release a given sync object.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get_obj call to release a given sync object.
+ */
+static void dmabuf_sync_put_obj(struct dmabuf_sync *sync,
+ struct dma_buf *dmabuf)
+{
+ struct dmabuf_sync_object *sobj;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry(sobj, &sync->syncs, head) {
+ if (sobj->dmabuf != dmabuf)
+ continue;
+
+ dma_buf_put(sobj->dmabuf);
+
+ list_del_init(&sobj->head);
+ kfree(sobj);
+ break;
+ }
+
+ if (list_empty(&sync->syncs))
+ sync->status = 0;
+
+ mutex_unlock(&sync->lock);
+}
+
+/*
+ * dmabuf_sync_put_objs - Release all sync objects of dmabuf_sync.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get_obj call to release all sync objects.
+ */
+static void dmabuf_sync_put_objs(struct dmabuf_sync *sync)
+{
+ struct dmabuf_sync_object *sobj, *next;
+
+ mutex_lock(&sync->lock);
+
+ list_for_each_entry_safe(sobj, next, &sync->syncs, head) {
+ dma_buf_put(sobj->dmabuf);
+
+ list_del_init(&sobj->head);
+ kfree(sobj);
+ }
+
+ mutex_unlock(&sync->lock);
+
+ sync->status = 0;
+}
+
+/**
+ * dmabuf_sync_lock - lock all dmabufs added to syncs list.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * The caller should call this function prior to CPU or DMA access to
+ * the dmabufs so that others can not access the dmabufs.
+ * Internally, this function avoids dead lock issue with ww-mutex.
+ */
+int dmabuf_sync_lock(struct dmabuf_sync *sync)
+{
+ int ret;
+
+ if (!sync) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ if (list_empty(&sync->syncs))
+ return -EINVAL;
+
+ if (sync->status != DMABUF_SYNC_GOT)
+ return -EINVAL;
+
+ ret = dmabuf_sync_lock_objs(sync, &sync->ctx);
+ if (ret < 0) {
+ WARN_ON(1);
+ return ret;
+ }
+
+ sync->status = DMABUF_SYNC_LOCKED;
+
+ return ret;
+}
+EXPORT_SYMBOL(dmabuf_sync_lock);
+
+/**
+ * dmabuf_sync_unlock - unlock all objects added to syncs list.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * The caller should call this function after CPU or DMA access to
+ * the dmabufs is completed so that others can access the dmabufs.
+ */
+int dmabuf_sync_unlock(struct dmabuf_sync *sync)
+{
+ if (!sync) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ /* If current dmabuf sync object wasn't reserved then just return. */
+ if (sync->status != DMABUF_SYNC_LOCKED)
+ return -EAGAIN;
+
+ dmabuf_sync_unlock_objs(sync, &sync->ctx);
+
+ return 0;
+}
+EXPORT_SYMBOL(dmabuf_sync_unlock);
+
+/**
+ * dmabuf_sync_get - initialize reservation entry and update
+ * dmabuf sync.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ * @sync_buf: A dma_buf object pointer that we want to be synchronized
+ * with others.
+ *
+ * This function should be called after dmabuf_sync_init function is called.
+ * The caller can group multiple dmabufs by calling this function several
+ * times. Internally, this function also takes a reference to a dmabuf.
+ */
+int dmabuf_sync_get(struct dmabuf_sync *sync, void *sync_buf, unsigned int type)
+{
+ int ret;
+
+ if (!sync || !sync_buf) {
+ WARN_ON(1);
+ return -EFAULT;
+ }
+
+ ret = dmabuf_sync_get_obj(sync, sync_buf, type);
+ if (ret < 0) {
+ WARN_ON(1);
+ return ret;
+ }
+
+ sync->status = DMABUF_SYNC_GOT;
+
+ return 0;
+}
+EXPORT_SYMBOL(dmabuf_sync_get);
+
+/**
+ * dmabuf_sync_put - Release a given dmabuf.
+ *
+ * @sync: An object to dmabuf_sync structure.
+ * @dmabuf: An object to dma_buf structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get function is called to release the dmabuf, or
+ * dmabuf_sync_unlock function is called.
+ */
+void dmabuf_sync_put(struct dmabuf_sync *sync, struct dma_buf *dmabuf)
+{
+ if (!sync || !dmabuf) {
+ WARN_ON(1);
+ return;
+ }
+
+ if (list_empty(&sync->syncs))
+ return;
+
+ dmabuf_sync_put_obj(sync, dmabuf);
+}
+EXPORT_SYMBOL(dmabuf_sync_put);
+
+/**
+ * dmabuf_sync_put_all - Release all sync objects
+ *
+ * @sync: An object to dmabuf_sync structure.
+ *
+ * This function should be called if some operation is failed after
+ * dmabuf_sync_get function is called to release all sync objects, or
+ * dmabuf_sync_unlock function is called.
+ */
+void dmabuf_sync_put_all(struct dmabuf_sync *sync)
+{
+ if (!sync) {
+ WARN_ON(1);
+ return;
+ }
+
+ if (list_empty(&sync->syncs))
+ return;
+
+ dmabuf_sync_put_objs(sync);
+}
+EXPORT_SYMBOL(dmabuf_sync_put_all);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index 34cfbac..ae0c87b 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -150,6 +150,20 @@ struct dma_buf_attachment {
void *priv;
};
+#define DMA_BUF_ACCESS_R 0x1
+#define DMA_BUF_ACCESS_W 0x2
+#define DMA_BUF_ACCESS_DMA 0x4
+#define DMA_BUF_ACCESS_RW (DMA_BUF_ACCESS_R | DMA_BUF_ACCESS_W)
+#define DMA_BUF_ACCESS_DMA_R (DMA_BUF_ACCESS_R | DMA_BUF_ACCESS_DMA)
+#define DMA_BUF_ACCESS_DMA_W (DMA_BUF_ACCESS_W | DMA_BUF_ACCESS_DMA)
+#define DMA_BUF_ACCESS_DMA_RW (DMA_BUF_ACCESS_DMA_R | DMA_BUF_ACCESS_DMA_W)
+#define IS_VALID_DMA_BUF_ACCESS_TYPE(t) (t = DMA_BUF_ACCESS_R || \
+ t = DMA_BUF_ACCESS_W || \
+ t = DMA_BUF_ACCESS_DMA_R || \
+ t = DMA_BUF_ACCESS_DMA_W || \
+ t = DMA_BUF_ACCESS_RW || \
+ t = DMA_BUF_ACCESS_DMA_RW)
+
/**
* get_dma_buf - convenience wrapper for get_file.
* @dmabuf: [in] pointer to dma_buf
diff --git a/include/linux/dmabuf-sync.h b/include/linux/dmabuf-sync.h
new file mode 100644
index 0000000..44c37de
--- /dev/null
+++ b/include/linux/dmabuf-sync.h
@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2013 Samsung Electronics Co.Ltd
+ * Authors:
+ * Inki Dae <inki.dae@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.
+ *
+ */
+
+#include <linux/mutex.h>
+#include <linux/sched.h>
+#include <linux/dma-buf.h>
+#include <linux/reservation.h>
+
+enum dmabuf_sync_status {
+ DMABUF_SYNC_GOT = 1,
+ DMABUF_SYNC_LOCKED,
+};
+
+/*
+ * A structure for dmabuf_sync_object.
+ *
+ * @head: A list head to be added to syncs list.
+ * @robj: A reservation_object object.
+ * @dma_buf: A dma_buf object.
+ * @access_type: Indicate how a current task tries to access
+ * a given buffer.
+ */
+struct dmabuf_sync_object {
+ struct list_head head;
+ struct reservation_object *robj;
+ struct dma_buf *dmabuf;
+ unsigned int access_type;
+};
+
+/*
+ * A structure for dmabuf_sync.
+ *
+ * @syncs: A list head to sync object and this is global to system.
+ * @list: A list entry used as committed list node
+ * @lock: A mutex lock to current sync object.
+ * @ctx: A current context for ww mutex.
+ * @work: A work struct to release resources at timeout.
+ * @priv: A private data.
+ * @name: A string to dmabuf sync owner.
+ * @timer: A timer list to avoid lockup and release resources.
+ * @status: Indicate current status (DMABUF_SYNC_GOT or DMABUF_SYNC_LOCKED).
+ */
+struct dmabuf_sync {
+ struct list_head syncs;
+ struct list_head list;
+ struct mutex lock;
+ struct ww_acquire_ctx ctx;
+ struct work_struct work;
+ void *priv;
+ char name[64];
+ struct timer_list timer;
+ unsigned int status;
+};
+
+#ifdef CONFIG_DMABUF_SYNC
+extern bool is_dmabuf_sync_supported(void);
+
+extern struct dmabuf_sync *dmabuf_sync_init(void *priv, const char *name);
+
+extern void dmabuf_sync_fini(struct dmabuf_sync *sync);
+
+extern int dmabuf_sync_lock(struct dmabuf_sync *sync);
+
+extern int dmabuf_sync_unlock(struct dmabuf_sync *sync);
+
+extern int dmabuf_sync_get(struct dmabuf_sync *sync, void *sync_buf,
+ unsigned int type);
+
+extern void dmabuf_sync_put(struct dmabuf_sync *sync, struct dma_buf *dmabuf);
+
+extern void dmabuf_sync_put_all(struct dmabuf_sync *sync);
+
+#else
+static inline bool is_dmabuf_sync_supported(void) { return false; }
+
+static inline struct dmabuf_sync *dmabuf_sync_init(void *priv,
+ const char *names)
+{
+ return ERR_PTR(0);
+}
+
+static inline void dmabuf_sync_fini(struct dmabuf_sync *sync) { }
+
+static inline int dmabuf_sync_lock(struct dmabuf_sync *sync)
+{
+ return 0;
+}
+
+static inline int dmabuf_sync_unlock(struct dmabuf_sync *sync)
+{
+ return 0;
+}
+
+static inline int dmabuf_sync_get(struct dmabuf_sync *sync,
+ void *sync_buf,
+ unsigned int type)
+{
+ return 0;
+}
+
+static inline void dmabuf_sync_put(struct dmabuf_sync *sync,
+ struct dma_buf *dmabuf) { }
+
+static inline void dmabuf_sync_put_all(struct dmabuf_sync *sync) { }
+
+#endif
diff --git a/include/linux/reservation.h b/include/linux/reservation.h
index 80050e2..4310192 100644
--- a/include/linux/reservation.h
+++ b/include/linux/reservation.h
@@ -50,6 +50,11 @@ struct reservation_object {
struct fence *fence_excl;
struct fence **fence_shared;
u32 fence_shared_count, fence_shared_max;
+
+ atomic_t shared_cnt;
+ unsigned int accessed_type;
+ unsigned int shared;
+ unsigned int locked;
};
static inline void
@@ -60,6 +65,8 @@ reservation_object_init(struct reservation_object *obj)
obj->fence_shared_count = obj->fence_shared_max = 0;
obj->fence_shared = NULL;
obj->fence_excl = NULL;
+
+ atomic_set(&obj->shared_cnt, 1);
}
static inline void
--
1.7.5.4
^ permalink raw reply related
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Lucas Stach @ 2013-06-19 10:22 UTC (permalink / raw)
To: Inki Dae
Cc: 'Russell King - ARM Linux', 'linux-fbdev',
'Kyungmin Park', 'DRI mailing list',
'myungjoo.ham', 'YoungJun Cho', linux-arm-kernel,
linux-media
In-Reply-To: <008601ce6cb0$2c8cec40$85a6c4c0$%dae@samsung.com>
Am Mittwoch, den 19.06.2013, 14:45 +0900 schrieb Inki Dae:
>
> > -----Original Message-----
> > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Tuesday, June 18, 2013 6:47 PM
> > To: Inki Dae
> > Cc: 'Russell King - ARM Linux'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI
> > mailing list'; 'myungjoo.ham'; 'YoungJun Cho'; linux-arm-
> > kernel@lists.infradead.org; linux-media@vger.kernel.org
> > Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization
> > framework
> >
> > Am Dienstag, den 18.06.2013, 18:04 +0900 schrieb Inki Dae:
> > [...]
> > >
> > > > a display device driver. It shouldn't be used within a single driver
> > > > as a means of passing buffers between userspace and kernel space.
> > >
> > > What I try to do is not really such ugly thing. What I try to do is to
> > > notify that, when CPU tries to access a buffer , to kernel side through
> > > dmabuf interface. So it's not really to send the buffer to kernel.
> > >
> > > Thanks,
> > > Inki Dae
> > >
> > The most basic question about why you are trying to implement this sort
> > of thing in the dma_buf framework still stands.
> >
> > Once you imported a dma_buf into your DRM driver it's a GEM object and
> > you can and should use the native DRM ioctls to prepare/end a CPU access
> > to this BO. Then internally to your driver you can use the dma_buf
> > reservation/fence stuff to provide the necessary cross-device sync.
> >
>
> I don't really want that is used only for DRM drivers. We really need
> it for all other DMA devices; i.e., v4l2 based drivers. That is what I
> try to do. And my approach uses reservation to use dma-buf resources
> but not dma fence stuff anymore. However, I'm looking into Radeon DRM
> driver for why we need dma fence stuff, and how we can use it if
> needed.
>
Still I don't see the point why you need syncpoints above dma-buf. In
both the DRM and the V4L2 world we have defined points in the API where
a buffer is allowed to change domain from device to CPU and vice versa.
In DRM if you want to access a buffer with the CPU you do a cpu_prepare.
The buffer changes back to GPU domain once you do the execbuf
validation, queue a pageflip to the buffer or similar things.
In V4L2 the syncpoints for cache operations are the queue/dequeue API
entry points. Those are also the exact points to synchronize with other
hardware thus using dma-buf reserve/fence.
In all this I can't see any need for a new syncpoint primitive slapped
on top of dma-buf.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* RE: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Inki Dae @ 2013-06-19 10:44 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1371467722-665-1-git-send-email-inki.dae@samsung.com>
> -----Original Message-----
> From: Lucas Stach [mailto:l.stach@pengutronix.de]
> Sent: Wednesday, June 19, 2013 7:22 PM
> To: Inki Dae
> Cc: 'Russell King - ARM Linux'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI
> mailing list'; 'myungjoo.ham'; 'YoungJun Cho'; linux-arm-
> kernel@lists.infradead.org; linux-media@vger.kernel.org
> Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization
> framework
>
> Am Mittwoch, den 19.06.2013, 14:45 +0900 schrieb Inki Dae:
> >
> > > -----Original Message-----
> > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > Sent: Tuesday, June 18, 2013 6:47 PM
> > > To: Inki Dae
> > > Cc: 'Russell King - ARM Linux'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI
> > > mailing list'; 'myungjoo.ham'; 'YoungJun Cho'; linux-arm-
> > > kernel@lists.infradead.org; linux-media@vger.kernel.org
> > > Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer
> synchronization
> > > framework
> > >
> > > Am Dienstag, den 18.06.2013, 18:04 +0900 schrieb Inki Dae:
> > > [...]
> > > >
> > > > > a display device driver. It shouldn't be used within a single
> driver
> > > > > as a means of passing buffers between userspace and kernel space.
> > > >
> > > > What I try to do is not really such ugly thing. What I try to do is
> to
> > > > notify that, when CPU tries to access a buffer , to kernel side
> through
> > > > dmabuf interface. So it's not really to send the buffer to kernel.
> > > >
> > > > Thanks,
> > > > Inki Dae
> > > >
> > > The most basic question about why you are trying to implement this
> sort
> > > of thing in the dma_buf framework still stands.
> > >
> > > Once you imported a dma_buf into your DRM driver it's a GEM object and
> > > you can and should use the native DRM ioctls to prepare/end a CPU
> access
> > > to this BO. Then internally to your driver you can use the dma_buf
> > > reservation/fence stuff to provide the necessary cross-device sync.
> > >
> >
> > I don't really want that is used only for DRM drivers. We really need
> > it for all other DMA devices; i.e., v4l2 based drivers. That is what I
> > try to do. And my approach uses reservation to use dma-buf resources
> > but not dma fence stuff anymore. However, I'm looking into Radeon DRM
> > driver for why we need dma fence stuff, and how we can use it if
> > needed.
> >
>
> Still I don't see the point why you need syncpoints above dma-buf. In
> both the DRM and the V4L2 world we have defined points in the API where
> a buffer is allowed to change domain from device to CPU and vice versa.
>
> In DRM if you want to access a buffer with the CPU you do a cpu_prepare.
> The buffer changes back to GPU domain once you do the execbuf
> validation, queue a pageflip to the buffer or similar things.
>
> In V4L2 the syncpoints for cache operations are the queue/dequeue API
> entry points. Those are also the exact points to synchronize with other
> hardware thus using dma-buf reserve/fence.
If so, what if we want to access a buffer with the CPU _in V4L2_? We should open a drm device node, and then do a cpu_prepare?
Thanks,
Inki Dae
>
> In all this I can't see any need for a new syncpoint primitive slapped
> on top of dma-buf.
>
> Regards,
> Lucas
> --
> Pengutronix e.K. | Lucas Stach |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH 8/8] at91/avr32/atmel_lcdfb: prepare clk before calling enable
From: Boris BREZILLON @ 2013-06-19 11:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1371640263-9268-1-git-send-email-b.brezillon@overkiz.com>
Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
avoid common clk framework warnings.
Signed-off-by: Boris BREZILLON <b.brezillon@overkiz.com>
---
drivers/video/atmel_lcdfb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index 540909d..8525457 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -893,14 +893,14 @@ static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo)
static void atmel_lcdfb_start_clock(struct atmel_lcdfb_info *sinfo)
{
- clk_enable(sinfo->bus_clk);
- clk_enable(sinfo->lcdc_clk);
+ clk_prepare_enable(sinfo->bus_clk);
+ clk_prepare_enable(sinfo->lcdc_clk);
}
static void atmel_lcdfb_stop_clock(struct atmel_lcdfb_info *sinfo)
{
- clk_disable(sinfo->bus_clk);
- clk_disable(sinfo->lcdc_clk);
+ clk_disable_unprepare(sinfo->bus_clk);
+ clk_disable_unprepare(sinfo->lcdc_clk);
}
--
1.7.9.5
^ permalink raw reply related
* Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization framework
From: Lucas Stach @ 2013-06-19 12:34 UTC (permalink / raw)
To: Inki Dae
Cc: 'Russell King - ARM Linux', 'linux-fbdev',
'Kyungmin Park', 'DRI mailing list',
'myungjoo.ham', 'YoungJun Cho', linux-arm-kernel,
linux-media
In-Reply-To: <00ae01ce6cd9$f4834630$dd89d290$%dae@samsung.com>
Am Mittwoch, den 19.06.2013, 19:44 +0900 schrieb Inki Dae:
>
> > -----Original Message-----
> > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > Sent: Wednesday, June 19, 2013 7:22 PM
> > To: Inki Dae
> > Cc: 'Russell King - ARM Linux'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI
> > mailing list'; 'myungjoo.ham'; 'YoungJun Cho'; linux-arm-
> > kernel@lists.infradead.org; linux-media@vger.kernel.org
> > Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer synchronization
> > framework
> >
> > Am Mittwoch, den 19.06.2013, 14:45 +0900 schrieb Inki Dae:
> > >
> > > > -----Original Message-----
> > > > From: Lucas Stach [mailto:l.stach@pengutronix.de]
> > > > Sent: Tuesday, June 18, 2013 6:47 PM
> > > > To: Inki Dae
> > > > Cc: 'Russell King - ARM Linux'; 'linux-fbdev'; 'Kyungmin Park'; 'DRI
> > > > mailing list'; 'myungjoo.ham'; 'YoungJun Cho'; linux-arm-
> > > > kernel@lists.infradead.org; linux-media@vger.kernel.org
> > > > Subject: Re: [RFC PATCH v2] dmabuf-sync: Introduce buffer
> > synchronization
> > > > framework
> > > >
> > > > Am Dienstag, den 18.06.2013, 18:04 +0900 schrieb Inki Dae:
> > > > [...]
> > > > >
> > > > > > a display device driver. It shouldn't be used within a single
> > driver
> > > > > > as a means of passing buffers between userspace and kernel space.
> > > > >
> > > > > What I try to do is not really such ugly thing. What I try to do is
> > to
> > > > > notify that, when CPU tries to access a buffer , to kernel side
> > through
> > > > > dmabuf interface. So it's not really to send the buffer to kernel.
> > > > >
> > > > > Thanks,
> > > > > Inki Dae
> > > > >
> > > > The most basic question about why you are trying to implement this
> > sort
> > > > of thing in the dma_buf framework still stands.
> > > >
> > > > Once you imported a dma_buf into your DRM driver it's a GEM object and
> > > > you can and should use the native DRM ioctls to prepare/end a CPU
> > access
> > > > to this BO. Then internally to your driver you can use the dma_buf
> > > > reservation/fence stuff to provide the necessary cross-device sync.
> > > >
> > >
> > > I don't really want that is used only for DRM drivers. We really need
> > > it for all other DMA devices; i.e., v4l2 based drivers. That is what I
> > > try to do. And my approach uses reservation to use dma-buf resources
> > > but not dma fence stuff anymore. However, I'm looking into Radeon DRM
> > > driver for why we need dma fence stuff, and how we can use it if
> > > needed.
> > >
> >
> > Still I don't see the point why you need syncpoints above dma-buf. In
> > both the DRM and the V4L2 world we have defined points in the API where
> > a buffer is allowed to change domain from device to CPU and vice versa.
> >
> > In DRM if you want to access a buffer with the CPU you do a cpu_prepare.
> > The buffer changes back to GPU domain once you do the execbuf
> > validation, queue a pageflip to the buffer or similar things.
> >
> > In V4L2 the syncpoints for cache operations are the queue/dequeue API
> > entry points. Those are also the exact points to synchronize with other
> > hardware thus using dma-buf reserve/fence.
>
>
> If so, what if we want to access a buffer with the CPU _in V4L2_? We
> should open a drm device node, and then do a cpu_prepare?
>
Not at all. As I said the syncpoints are the queue/dequeue operations.
When dequeueing a buffer you are explicitly dragging the buffer domain
back from device into userspace and thus CPU domain.
If you are operating on an mmap of a V4L2 processed buffer it's either
before or after it got processed by the hardware and therefore all DMA
operations on the buffer are bracketed by the V4L2 qbug/dqbuf ioctls.
That is where cache operations and synchronization should happen. The
V4L2 driver shouldn't allow you to dequeue a buffer and thus dragging it
back into CPU domain while there is still DMA ongoing. Equally the queue
ioctrl should make sure caches are properly written back to memory. The
results of reading/writing to the mmap of a V4L2 buffer while it is
enqueued to the hardware is simply undefined and there is nothing
suggesting that this is a valid usecase.
Regards,
Lucas
--
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* Re: [PATCH] build some drivers only when compile-testing
From: Greg Kroah-Hartman @ 2013-06-19 14:27 UTC (permalink / raw)
To: Jiri Slaby
Cc: Tomi Valkeinen, Jeff Mahoney, jirislaby, linux-kernel,
Andrew Morton, Linus Torvalds, Alexander Shishkin, linux-usb,
Florian Tobias Schandinat, linux-geode, linux-fbdev,
Richard Cochran, netdev, Ben Hutchings, Keller, Jacob E,
Michal Marek
In-Reply-To: <51C159F8.8090604@suse.cz>
On Wed, Jun 19, 2013 at 09:12:56AM +0200, Jiri Slaby wrote:
> On 06/19/2013 09:10 AM, Tomi Valkeinen wrote:
> > On 17/06/13 23:05, Jiri Slaby wrote:
> >
> >> The last point I inclined to the Greg's argument to remove the
> >> EXPERT dependency.
> >>
> >> So currently I have what is attached... Comments?
> >
> > The patch looks a bit odd with the USB_CHIPIDEA_IMX parts. You're
> > not adding COMPILE_TEST there, but you're adding a totally new
> > config option, and also changing the Makefile.
>
> Look:
>
> +config USB_CHIPIDEA_IMX
> + bool "ChipIdea IMX support"
> + depends on OF_DEVICE && (ARM || COMPILE_TEST)
Ah, what about all of the chipidea chips on x86 boxes (yes, they are
rare, but they are present, so you can't just turn them off for that
whole platform here.
I'd leave chipidea alone, it's getting more prelevant, not on a desktop
or server, but in SoC systems, which is x86 for a lot of devices.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC PATCH 1/5] phy: Add driver for Exynos MIPI CSIS/DSIM DPHYs
From: Sylwester Nawrocki @ 2013-06-19 16:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3575249.Vg82ll65tN@flatron>
Hi Tomasz,
On 06/16/2013 11:11 PM, Tomasz Figa wrote:
> Hi Sylwester,
>
> Looks good, but I added some nitpicks inline.
>
> On Friday 14 of June 2013 19:45:47 Sylwester Nawrocki wrote:
>> Add a PHY provider driver for the Samsung S5P/Exynos SoC MIPI CSI-2
>> receiver and MIPI DSI transmitter DPHYs.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> .../bindings/phy/exynos-video-mipi-phy.txt | 16 ++
>> drivers/phy/Kconfig | 10 ++
>> drivers/phy/Makefile | 3 +-
>> drivers/phy/exynos_video_mipi_phy.c | 166
>> ++++++++++++++++++++ 4 files changed, 194 insertions(+), 1 deletion(-)
>> create mode 100644
>> Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt create
>> mode 100644 drivers/phy/exynos_video_mipi_phy.c
>>
>> diff --git
>> a/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
>> b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt new
>> file mode 100644
>> index 0000000..32311c89
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/exynos-video-mipi-phy.txt
>> @@ -0,0 +1,16 @@
>> +Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY
>> +-------------------------------------------------
>> +
>> +Required properties:
>> +- compatible : "samsung,<soc_name>-video-phy", currently most SoCs can
>
> I don't like this <soc_name> here. It sounds like any SoC name can be put
> here. IMHO just listing all supported compatible values should be enough.
Hmm, OK, I'll simply put there the one compatible string supported now.
>> claim + compatibility with the S5PV210 MIPI CSIS/DSIM PHY and thus
>> should use + "samsung,s5pv210-video-phy";
>> +- reg : offset and length of the MIPI DPHY register set;
>> +- #phy-cells : from the generic phy bindings, must be 1;
>> +
>> +For "samsung,s5pv210-video-phy" compatible DPHYs the second cell in the
>> PHY +specifier identifies the DPHY and its meaning is as follows:
>> + 0 - MIPI CSIS 0,
>> + 1 - MIPI DSIM 0,
>> + 2 - MIPI CSIS 1,
>> + 3 - MIPI DSIM 1.
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 0764a54..d234e99 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -11,3 +11,13 @@ menuconfig GENERIC_PHY
>> devices present in the kernel. This layer will have the generic
>> API by which phy drivers can create PHY using the phy framework
> and
>> phy users can obtain reference to the PHY.
>> +
>> +if GENERIC_PHY
>> +
>> +config EXYNOS_VIDEO_MIPI_PHY
>> + bool "S5P/EXYNOS MIPI CSI-2/DSI PHY driver"
>> + depends on OF
>
> Hmm. Is this driver designed only for OF-enabled boards?
Yes, there seems currently to be no users of MIPI CSIS/DSIM in the mainline
kernel among the non-dt platforms, so I initially focused on DT only. I will
rework this driver to make it usable on non-dt platforms, but I currently
have not way to fully test it. I believe S5PV210 will get migrated to device
tree sooner than anyone needs the functionality this driver provides on
non-dt, and S5PC100 seems to be forgotten anyway.
>> + help
>> + Support for MIPI CSI-2 and MIPI DSI DPHY found on Samsung
>> + S5P and EXYNOS SoCs.
>> +endif
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 9e9560f..b16f2c1 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -2,4 +2,5 @@
>> # Makefile for the phy drivers.
>> #
>>
>> -obj-$(CONFIG_GENERIC_PHY) += phy-core.o
>> +obj-$(CONFIG_GENERIC_PHY) += phy-core.o
>> +obj-$(CONFIG_EXYNOS_VIDEO_MIPI_PHY) += exynos_video_mipi_phy.o
>> diff --git a/drivers/phy/exynos_video_mipi_phy.c
>> b/drivers/phy/exynos_video_mipi_phy.c new file mode 100644
>> index 0000000..8d4976f
>> --- /dev/null
>> +++ b/drivers/phy/exynos_video_mipi_phy.c
>> @@ -0,0 +1,166 @@
>> +/*
>> + * Samsung S5P/EXYNOS SoC series MIPI CSIS/DSIM DPHY driver
>> + *
>> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
>> + * Author: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as +
>> * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +
>> +/* MIPI_PHYn_CONTROL register bit definitions */
>> +#define EXYNOS_MIPI_PHY_ENABLE (1 << 0)
>> +#define EXYNOS_MIPI_PHY_SRESETN (1 << 1)
>> +#define EXYNOS_MIPI_PHY_MRESETN (1 << 2)
>> +#define EXYNOS_MIPI_PHY_RESET_MASK (3 << 1)
>> +
>> +#define EXYNOS_MAX_VIDEO_PHYS 4
>> +
>> +struct exynos_video_phy {
>> + spinlock_t slock;
>> + struct phy *phys[EXYNOS_MAX_VIDEO_PHYS];
>> + void __iomem *regs;
>> +};
>> +
>> +/*
>> + * The @id argument specifies MIPI CSIS or DSIM PHY as follows:
>> + * 0 - MIPI CSIS 0
>> + * 1 - MIPI DSIM 0
>> + * 2 - MIPI CSIS 1
>> + * 3 - MIPI DSIM 1
>> + */
>> +static int set_phy_state(struct exynos_video_phy *state,
>> + unsigned int id, int on)
>> +{
>> + void __iomem *addr = id < 2 ? state->regs : state->regs + 4;
>
> I don't find this statement too readable. What about:
>
> void __iomem *addr = state->regs;
>
> and below:
>
> /* CSIS 1 and DSIM 1 PHYs have separate register */
> if (id >= 2)
> addr += 4;
OK, thanks for the suggestion. I've addressed this in v2.
>> + unsigned long flags;
>> + u32 reg, reset;
>> +
>> + pr_debug("%s(): id: %d, on: %d, addr: %#x, base: %#x\n",
>> + __func__, id, on, (u32)addr, (u32)state->regs);
>> +
>> + if (WARN_ON(id > EXYNOS_MAX_VIDEO_PHYS))
>> + return -EINVAL;
>> +
>> + if (id & 1)
>
> Nice trick ;), but not very readable. What about creating an enum of PHYs
> and using those defined values here:
>
> if (id = PHY_DSI0 || id = PHY_DSI1)
Yeah, good point. Fixed.
>> + reset = EXYNOS_MIPI_PHY_MRESETN;
>> + else
>> + reset = EXYNOS_MIPI_PHY_SRESETN;
>> +
>> + spin_lock_irqsave(&state->slock, flags);
>> +
>> + reg = readl(addr);
>> + if (on)
>> + reg |= reset;
>> + else
>> + reg &= ~reset;
>> + writel(reg, addr);
>> +
>> + if (on)
>> + reg |= EXYNOS_MIPI_PHY_ENABLE;
>
> I believe this is a kind of reference counting, but a comment here would
> be nice.
Indeed, I've added a comment.
>> + else if (!(reg & EXYNOS_MIPI_PHY_RESET_MASK))
>> + reg &= ~EXYNOS_MIPI_PHY_ENABLE;
>> +
>> + writel(reg, addr);
>> +
>> + spin_unlock_irqrestore(&state->slock, flags);
>> + return 0;
>> +}
>> +
>> +static int exynos_video_phy_power_on(struct phy *phy)
>> +{
>> + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
>> + return set_phy_state(state, phy->id, 1);
>> +}
>> +
>> +static int exynos_video_phy_power_off(struct phy *phy)
>> +{
>> + struct exynos_video_phy *state = dev_get_drvdata(&phy->dev);
>> + return set_phy_state(state, phy->id, 0);
>> +}
>> +
>> +static struct phy *exynos_video_phy_xlate(struct device *dev,
>> + struct of_phandle_args *args)
>> +{
>> + struct exynos_video_phy *state = dev_get_drvdata(dev);
>> +
>> + if (WARN_ON(args->args[0] > EXYNOS_MAX_VIDEO_PHYS))
>> + return NULL;
>> +
>> + return state->phys[args->args[0]];
>> +}
>> +
>> +static struct phy_ops exynos_video_phy_ops = {
>> + .power_on = exynos_video_phy_power_on,
>> + .power_off = exynos_video_phy_power_off,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +static int exynos_video_phy_probe(struct platform_device *pdev)
>> +{
>> + struct exynos_video_phy *state;
>> + struct device *dev = &pdev->dev;
>> + struct resource res;
>> + struct phy_provider *phy_provider;
>> + int ret, i;
>> +
>> + state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
>> + if (!state)
>> + return -ENOMEM;
>> +
>> + ret = of_address_to_resource(dev->of_node, 0, &res);
>> + if (ret < 0)
>> + return ret;
>
> You can use platform_get_resource() here to get a resource generated for
> you by of_platform_populate().
>
> In addition you don't need to check the pointer returned by
> platform_get_resource() because it is checked in devm_ioremap_resource().
Fixed.
>> +
>> + state->regs = devm_ioremap_resource(dev, &res);
>> + if (IS_ERR(state->regs))
>> + return PTR_ERR(state->regs);
>> +
>> + dev_set_drvdata(dev, state);
>> +
>> + phy_provider = devm_of_phy_provider_register(dev, THIS_MODULE,
>> + exynos_video_phy_xlate);
>> + if (IS_ERR(phy_provider))
>> + return PTR_ERR(phy_provider);
>> +
>> + for (i = 0; i < EXYNOS_MAX_VIDEO_PHYS; i++) {
>> + state->phys[i] = devm_phy_create(dev, i,
> &exynos_video_phy_ops,
>> +
> state);
>> + if (IS_ERR(state->phys[i])) {
>> + dev_err(dev, "failed to create PHY %d\n", i);
>> + return PTR_ERR(state->phys[i]);
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id exynos_video_phy_of_match[] = {
>> + { .compatible = "samsung,s5pv210-video-phy" },
>> + { },
>> +};
>
> IMHO a MODULE_DEVICE_TABLE should be added here.
True, added after modifying the Kconfig entry to allow this driver
to be built as a module.
Regards,
Sylwester
^ 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