Re: [PATCH] media: vsp1: cleanup a false positive warning

[Mauro Carvalho Chehab]

Em Mon, 07 May 2018 17:05:24 +0300
Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> On Friday, 4 May 2018 15:13:58 EEST Mauro Carvalho Chehab wrote:
> > With the new vsp1 code changes introduced by changeset
> > f81f9adc4ee1 ("media: v4l: vsp1: Assign BRU and BRS to pipelines
> > dynamically"), smatch complains with:
> > drivers/media/platform/vsp1/vsp1_drm.c:262 vsp1_du_pipeline_setup_bru()
> > error: we previously assumed 'pipe->bru' could be null (see line 180)
> > 
> > This is a false positive, as, if pipe->bru is NULL, the brx
> > var will be different, with ends by calling a code that will
> > set pipe->bru to another value.
> > 
> > Yet, cleaning this false positive is as easy as adding an explicit
> > check if pipe->bru is NULL.  
> 
> It's not very difficult indeed, but it really is a false positive. I think 
> the 
> proposed change decreases readability, the condition currently reads as "if 
> (new brx != old brx)", why does smatch even flag that as an error ?

I've no idea. Never studied smatch code. If you don't think that
this is a fix for it, do you have an alternative patch (either to
smatch or to vsp1)?

Regards,
Mauro

> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>
> > ---
> >  drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > b/drivers/media/platform/vsp1/vsp1_drm.c index 095dc48aa25a..cb6b60843400
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device
> > *vsp1, brx = >brs->entity;
> > 
> > /* Switch BRx if needed. */
> > -   if (brx != pipe->brx) {
> > +   if (brx != pipe->brx || !pipe->brx) {
> > struct vsp1_entity *released_brx = NULL;
> > 
> > /* Release our BRx if we have one. */  
> 



Thanks,
Mauro

Re: [PATCH] media: vsp1: cleanup a false positive warning

[Mauro Carvalho Chehab]

Em Fri, 4 May 2018 16:37:23 +0200
Geert Uytterhoeven <ge...@linux-m68k.org> escreveu:

> Hi Mauro,
> 
> On Fri, May 4, 2018 at 2:13 PM, Mauro Carvalho Chehab
> <mchehab+sams...@kernel.org> wrote:
> > With the new vsp1 code changes introduced by changeset
> > f81f9adc4ee1 ("media: v4l: vsp1: Assign BRU and BRS to pipelines 
> > dynamically"),
> > smatch complains with:
> > drivers/media/platform/vsp1/vsp1_drm.c:262 
> > vsp1_du_pipeline_setup_bru() error: we previously assumed 'pipe->bru' could 
> > be null (see line 180)
> >
> > This is a false positive, as, if pipe->bru is NULL, the brx
> > var will be different, with ends by calling a code that will
> > set pipe->bru to another value.
> >
> > Yet, cleaning this false positive is as easy as adding an explicit
> > check if pipe->bru is NULL.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>  
> 
> Thanks for your patch!
> 
> s/bru/brx/

Hah, yeah... there was a rename from bru->brx... 
I guess that confused me, as I saw this error before the renaming patch
(even though I wrote it to be applied after them) :-)

> 
> > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > @@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(struct 
> > vsp1_device *vsp1,
> > brx = >brs->entity;
> >
> > /* Switch BRx if needed. */
> > -   if (brx != pipe->brx) {
> > +   if (brx != pipe->brx || !pipe->brx) {
> > struct vsp1_entity *released_brx = NULL;
> >
> > /* Release our BRx if we have one. */  
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 



Thanks,
Mauro

[PATCH] media: vsp1: cleanup a false positive warning

[Mauro Carvalho Chehab]

With the new vsp1 code changes introduced by changeset
f81f9adc4ee1 ("media: v4l: vsp1: Assign BRU and BRS to pipelines dynamically"),
smatch complains with:
drivers/media/platform/vsp1/vsp1_drm.c:262 vsp1_du_pipeline_setup_bru() 
error: we previously assumed 'pipe->bru' could be null (see line 180)

This is a false positive, as, if pipe->bru is NULL, the brx
var will be different, with ends by calling a code that will
set pipe->bru to another value.

Yet, cleaning this false positive is as easy as adding an explicit
check if pipe->bru is NULL.

Signed-off-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 095dc48aa25a..cb6b60843400 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -185,7 +185,7 @@ static int vsp1_du_pipeline_setup_brx(struct vsp1_device 
*vsp1,
brx = >brs->entity;
 
/* Switch BRx if needed. */
-   if (brx != pipe->brx) {
+   if (brx != pipe->brx || !pipe->brx) {
struct vsp1_entity *released_brx = NULL;
 
/* Release our BRx if we have one. */
-- 
2.17.0

Re: [PATCH] media: platform: Drop OF dependency of VIDEO_RENESAS_VSP1

[Mauro Carvalho Chehab]

Em Tue, 06 Mar 2018 18:35:32 +0200
Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday, 6 March 2018 18:25:15 EET Mauro Carvalho Chehab wrote:
> > Em Mon, 26 Feb 2018 19:09:10 +0100 Geert Uytterhoeven escreveu:  
> > > VIDEO_RENESAS_VSP1 depends on ARCH_RENESAS && OF.
> > > As ARCH_RENESAS implies OF, the latter can be dropped.
> > > 
> > > Signed-off-by: Geert Uytterhoeven <geert+rene...@glider.be>
> > > ---
> > > 
> > >  drivers/media/platform/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/platform/Kconfig
> > > b/drivers/media/platform/Kconfig index 614fbef08ddcabb0..2b8b1ad0edd9eb31
> > > 100644
> > > --- a/drivers/media/platform/Kconfig
> > > +++ b/drivers/media/platform/Kconfig
> > > @@ -448,7 +448,7 @@ config VIDEO_RENESAS_FCP
> > > 
> > >  config VIDEO_RENESAS_VSP1
> > >  
> > >   tristate "Renesas VSP1 Video Processing Engine"
> > >   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && HAS_DMA
> > > 
> > > - depends on (ARCH_RENESAS && OF) || COMPILE_TEST
> > > + depends on ARCH_RENESAS || COMPILE_TEST  
> > 
> > That is not correct!
> > 
> > COMPILE_TEST doesn't depend on OF. With this patch, it will likely
> > cause build failures with randconfigs.  
> 
> ARCH_RENESAS implies OF, so replacing (ARCH_RENESAS && OF) with ARCH_RENESAS 
> doesn't change anything. The driver can be compiled with COMPILE_TEST and !OF 
> both before and after this patch.

OK!
> 
> > >   depends on (!ARM64 && !VIDEO_RENESAS_FCP) || VIDEO_RENESAS_FCP
> > >   select VIDEOBUF2_DMA_CONTIG
> > >   select VIDEOBUF2_VMALLOC  
> 



Thanks,
Mauro

Re: [PATCH] media: platform: Drop OF dependency of VIDEO_RENESAS_VSP1

[Mauro Carvalho Chehab]

Em Mon, 26 Feb 2018 19:09:10 +0100
Geert Uytterhoeven  escreveu:

> VIDEO_RENESAS_VSP1 depends on ARCH_RENESAS && OF.
> As ARCH_RENESAS implies OF, the latter can be dropped.
> 
> Signed-off-by: Geert Uytterhoeven 
> ---
>  drivers/media/platform/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 614fbef08ddcabb0..2b8b1ad0edd9eb31 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -448,7 +448,7 @@ config VIDEO_RENESAS_FCP
>  config VIDEO_RENESAS_VSP1
>   tristate "Renesas VSP1 Video Processing Engine"
>   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && HAS_DMA
> - depends on (ARCH_RENESAS && OF) || COMPILE_TEST
> + depends on ARCH_RENESAS || COMPILE_TEST

That is not correct!

COMPILE_TEST doesn't depend on OF. With this patch, it will likely
cause build failures with randconfigs.

>   depends on (!ARM64 && !VIDEO_RENESAS_FCP) || VIDEO_RENESAS_FCP
>   select VIDEOBUF2_DMA_CONTIG
>   select VIDEOBUF2_VMALLOC



Thanks,
Mauro

Re: [PATCH] v4l2-dev.h: fix symbol collision in media_entity_to_video_device()

[Mauro Carvalho Chehab]

Hi Niklas,

Em Thu, 25 Jan 2018 01:34:30 +0100
Niklas Söderlund  escreveu:

> A recent change to the media_entity_to_video_device() macro breaks some
> use-cases for the macro due to a symbol collision. Before the change
> this worked:
> 
> vdev = media_entity_to_video_device(link->sink->entity);
> 
> While after the change it results in a compiler error "error: 'struct
> video_device' has no member named 'link'; did you mean 'lock'?". While
> the following still works after the change.
> 
> struct media_entity *entity = link->sink->entity;
> vdev = media_entity_to_video_device(entity);
> 
> Fix the collision by renaming the macro argument to 'media_entity'.
> 
> Signed-off-by: Niklas Söderlund 
> ---
>  include/media/v4l2-dev.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Hi Mauro,
> 
> As the offending commit is not yet upstream and I'm not sure if the 
> commit ids in the media-tree are stable. If they are please attach the 
> following fixes tag.

They are. 

> 
> Fixes: 69b925c5fc36d8f1 ("media: v4l2-dev.h: add kernel-doc to two macros")
> 
> Regards,
> // Niklas
> 
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 267fd2bed17bd3c1..f0fc1ebda47244b3 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -298,10 +298,10 @@ struct video_device
>   * media_entity_to_video_device - Returns a  video_device from
>   *   the  media_entity embedded on it.
>   *
> - * @entity: pointer to  media_entity
> + * @media_entity: pointer to  media_entity
>   */
> -#define media_entity_to_video_device(entity) \
> - container_of(entity, struct video_device, entity)
> +#define media_entity_to_video_device(media_entity) \
> + container_of(media_entity, struct video_device, entity)

Instead, the best would be to use __entity (or __media_entity).
That would very likely prevent future conflicts.

>  
>  /**
>   * to_video_device - Returns a  video_device from the




Cheers,
Mauro

Re: [PATCH v3 01/34] clk_ops: change round_rate() to return unsigned long

[Mauro Carvalho Chehab]

Em Mon,  1 Jan 2018 19:42:40 +
Bryan O'Donoghue <pure.lo...@nexus-software.ie> escreveu:

> Right now it is not possible to return a value larger than LONG_MAX on 32
> bit systems. You can pass a rate of ULONG_MAX but can't return anything
> past LONG_MAX due to the fact both the rounded_rate and negative error
> codes are represented in the return value of round_rate().
> 
> Most implementations either return zero on error or don't return error
> codes at all. A minority of implementations do return a negative number -
> typically -EINVAL or -ENODEV.
> 
> At the higher level then callers of round_rate() typically and rightly
> check for a value of <= 0.
> 
> It is possible then to convert round_rate() to an unsigned long return
> value and change error code indication for the minority from -ERRORCODE to
> a simple 0.
> 
> This patch is the first step in making it possible to scale round_rate past
> LONG_MAX, later patches will change the previously mentioned minority of
> round_rate() implementations to return zero only on error if those
> implementations currently return a negative error number. Implementations
> that do not return an error code of < 0 will be left as-is.
> 

>  drivers/media/platform/omap3isp/isp.c   |  4 ++--

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

[PATCH v2 5/8] media: v4l2-mediabus: convert flags to enums and document them

[Mauro Carvalho Chehab]

There is a mess with media bus flags: there are two sets of
flags, one used by parallel and ITU-R BT.656 outputs,
and another one for CSI2.

Depending on the type, the same bit has different meanings.

That's very confusing, and counter-intuitive. So, split them
into two sets of flags, inside an enum.

This way, it becomes clearer that there are two separate sets
of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
need a different set of flags.

As a side effect, enums can be documented via kernel-docs,
so there will be an improvement at flags documentation.

Unfortunately, soc_camera and pxa_camera do a mess with
the flags, using either one set of flags without proper
checks about the type. That could be fixed, but, as both drivers
are obsolete and in the process of cleanings, I opted to just
keep the behavior, using an unsigned int inside those two
drivers.

Acked-by: Hans Verkuil <hans.verk...@cisco.com>
Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/i2c/adv7180.c|  10 +-
 drivers/media/i2c/ml86v7667.c  |   5 +-
 drivers/media/i2c/mt9m111.c|   8 +-
 drivers/media/i2c/ov6650.c |  19 +--
 drivers/media/i2c/soc_camera/imx074.c  |   6 +-
 drivers/media/i2c/soc_camera/mt9m001.c |  10 +-
 drivers/media/i2c/soc_camera/mt9t031.c |  11 +-
 drivers/media/i2c/soc_camera/mt9t112.c |  11 +-
 drivers/media/i2c/soc_camera/mt9v022.c |  16 ++-
 drivers/media/i2c/soc_camera/ov5642.c  |   5 +-
 drivers/media/i2c/soc_camera/ov772x.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9640.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9740.c  |  10 +-
 drivers/media/i2c/soc_camera/rj54n1cb0c.c  |  12 +-
 drivers/media/i2c/soc_camera/tw9910.c  |  13 +-
 drivers/media/i2c/tc358743.c   |  10 +-
 drivers/media/i2c/tvp5150.c|   6 +-
 drivers/media/platform/pxa_camera.c|   8 +-
 drivers/media/platform/rcar-vin/rcar-core.c|   4 +-
 drivers/media/platform/rcar-vin/rcar-dma.c |   4 +-
 .../platform/soc_camera/sh_mobile_ceu_camera.c |   2 +-
 drivers/media/platform/soc_camera/soc_camera.c |   3 +-
 .../platform/soc_camera/soc_camera_platform.c  |   2 +-
 drivers/media/platform/soc_camera/soc_mediabus.c   |   2 +-
 drivers/media/v4l2-core/v4l2-fwnode.c  |   5 +-
 drivers/staging/media/imx/imx-media-csi.c  |   7 +-
 include/media/v4l2-fwnode.h|   4 +-
 include/media/v4l2-mediabus.h  | 145 ++---
 28 files changed, 222 insertions(+), 136 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 25d24a3f10a7..4bf25a72ef4f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -743,16 +743,16 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 
if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
cfg->type = V4L2_MBUS_CSI2;
-   cfg->flags = V4L2_MBUS_CSI2_1_LANE |
-   V4L2_MBUS_CSI2_CHANNEL_0 |
-   V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->csi2_flags = V4L2_MBUS_CSI2_1_LANE
+ | V4L2_MBUS_CSI2_CHANNEL_0
+ | V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
} else {
/*
 * The ADV7180 sensor supports BT.601/656 output modes.
 * The BT.656 is default and not yet configurable by s/w.
 */
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
}
 
diff --git a/drivers/media/i2c/ml86v7667.c b/drivers/media/i2c/ml86v7667.c
index 57ef901edb06..a25114d0c31f 100644
--- a/drivers/media/i2c/ml86v7667.c
+++ b/drivers/media/i2c/ml86v7667.c
@@ -226,8 +226,9 @@ static int ml86v7667_fill_fmt(struct v4l2_subdev *sd,
 static int ml86v7667_g_mbus_config(struct v4l2_subdev *sd,
   struct v4l2_mbus_config *cfg)
 {
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER
+   | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
 
return 0;
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index b1665d97e0fd..d9698b535080 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/me

[PATCH v2 3/8] media: v4l2-async: simplify v4l2_async_subdev structure

[Mauro Carvalho Chehab]

The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
match criteria requires just a device name.

So, it doesn't make sense to enclose those into structs,
as the criteria can go directly into the union.

That makes easier to document it, as we don't need to document
weird senseless structs.

At drivers, this makes even clearer about the match criteria.

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>
Acked-by: Benoit Parrot <bpar...@ti.com>
Acked-by: Alexandre Belloni <alexandre.bell...@free-electrons.com>
Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
Acked-by: Philipp Zabel <p.za...@pengutronix.de>
Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c|  6 +++---
 drivers/media/platform/atmel/atmel-isc.c   |  2 +-
 drivers/media/platform/atmel/atmel-isi.c   |  2 +-
 drivers/media/platform/davinci/vpif_capture.c  |  4 ++--
 drivers/media/platform/exynos4-is/media-dev.c  |  4 ++--
 drivers/media/platform/pxa_camera.c|  2 +-
 drivers/media/platform/qcom/camss-8x16/camss.c |  2 +-
 drivers/media/platform/rcar-vin/rcar-core.c|  2 +-
 drivers/media/platform/rcar_drif.c |  4 ++--
 drivers/media/platform/soc_camera/soc_camera.c |  2 +-
 drivers/media/platform/stm32/stm32-dcmi.c  |  2 +-
 drivers/media/platform/ti-vpe/cal.c|  2 +-
 drivers/media/platform/xilinx/xilinx-vipp.c|  2 +-
 drivers/media/v4l2-core/v4l2-async.c   | 16 
 drivers/media/v4l2-core/v4l2-fwnode.c  | 10 +-
 drivers/staging/media/imx/imx-media-dev.c  |  4 ++--
 include/media/v4l2-async.h |  8 ++--
 17 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c 
b/drivers/media/platform/am437x/am437x-vpfe.c
index 0997c640191d..601ae6487617 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2304,8 +2304,8 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
 
for (i = 0; i < ARRAY_SIZE(vpfe->cfg->asd); i++) {
-   if (vpfe->cfg->asd[i]->match.fwnode.fwnode ==
-   asd[i].match.fwnode.fwnode) {
+   if (vpfe->cfg->asd[i]->match.fwnode ==
+   asd[i].match.fwnode) {
sdinfo = >cfg->sub_devs[i];
vpfe->sd[i] = subdev;
vpfe->sd[i]->grp_id = sdinfo->grp_id;
@@ -2510,7 +2510,7 @@ vpfe_get_pdata(struct platform_device *pdev)
}
 
pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
-   pdata->asd[i]->match.fwnode.fwnode = of_fwnode_handle(rem);
+   pdata->asd[i]->match.fwnode = of_fwnode_handle(rem);
of_node_put(rem);
}
 
diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 0c2635647f69..34676409ca08 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2088,7 +2088,7 @@ static int isc_parse_dt(struct device *dev, struct 
isc_device *isc)
subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
 
subdev_entity->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
-   subdev_entity->asd->match.fwnode.fwnode =
+   subdev_entity->asd->match.fwnode =
of_fwnode_handle(rem);
list_add_tail(_entity->list, >subdev_entities);
}
diff --git a/drivers/media/platform/atmel/atmel-isi.c 
b/drivers/media/platform/atmel/atmel-isi.c
index e900995143a3..9958918e2449 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1128,7 +1128,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct 
device_node *node)
/* Remote node to connect */
isi->entity.node = remote;
isi->entity.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-   isi->entity.asd.match.fwnode.fwnode = of_fwnode_handle(remote);
+   isi->entity.asd.match.fwnode = of_fwnode_handle(remote);
return 0;
}
 }
diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index e45916f69def..e1c273c8b9a6 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1390,7 +1390,7 @@ static int vpif_async_bound(struct v4l2_async_notifier 
*notifier,
 
for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
struct v4l2_async_subdev *_asd = vpif_obj.config->as

Re: [PATCH 5/8] media: v4l2-mediabus: convert flags to enums and document them

[Mauro Carvalho Chehab]

Em Tue, 19 Dec 2017 10:30:15 +0100
Philipp Zabel <p.za...@pengutronix.de> escreveu:

> Hi Mauro,
> 
> On Mon, 2017-12-18 at 17:53 -0200, Mauro Carvalho Chehab wrote:
> > There is a mess with media bus flags: there are two sets of
> > flags, one used by parallel and ITU-R BT.656 outputs,
> > and another one for CSI2.
> > 
> > Depending on the type, the same bit has different meanings.
> > 
> > That's very confusing, and counter-intuitive. So, split them
> > into two sets of flags, inside an enum.
> > 
> > This way, it becomes clearer that there are two separate sets
> > of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
> > need a different set of flags.
> > 
> > As a side effect, enums can be documented via kernel-docs,
> > so there will be an improvement at flags documentation.
> > 
> > Unfortunately, soc_camera and pxa_camera do a mess with
> > the flags, using either one set of flags without proper
> > checks about the type. That could be fixed, but, as both drivers
> > are obsolete and in the process of cleanings, I opted to just
> > keep the behavior, using an unsigned int inside those two
> > drivers.
> > 
> > Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>  
> 
> If I am not mistaken this is missing a conversion of
> drivers/staging/media/imx/imx-media-csi.c:
> 
> 8<
> diff --git a/drivers/staging/media/imx/imx-media-csi.c 
> b/drivers/staging/media/imx/imx-media-csi.c
> index eb7be5093a9d5..b1daac3a537d9 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -636,9 +636,10 @@ static int csi_setup(struct csi_priv *priv)
>  
>   /* compose mbus_config from the upstream endpoint */
>   mbus_cfg.type = priv->upstream_ep.bus_type;
> - mbus_cfg.flags = (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2) ?
> - priv->upstream_ep.bus.mipi_csi2.flags :
> - priv->upstream_ep.bus.parallel.flags;
> + if (priv->upstream_ep.bus_type == V4L2_MBUS_CSI2)
> + mbus_cfg.csi2_flags = priv->upstream_ep.bus.mipi_csi2.flags;
> + else
> + mbus_cfg.pb_flags = priv->upstream_ep.bus.parallel.flags;
>  
>   /*
>    * we need to pass input frame to CSI interface, but


Oh, thanks for noticing! I really hate having drivers that don't
build with COMPILE_TEST, as that makes a lot harder to check if
something broke.


Thanks,
Mauro

[PATCH 3/8] media: v4l2-async: simplify v4l2_async_subdev structure

[Mauro Carvalho Chehab]

The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
match criteria requires just a device name.

So, it doesn't make sense to enclose those into structs,
as the criteria can go directly into the union.

That makes easier to document it, as we don't need to document
weird senseless structs.

At drivers, this makes even clearer about the match criteria.

Acked-by: Sylwester Nawrocki <s.nawro...@samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/platform/am437x/am437x-vpfe.c|  6 +++---
 drivers/media/platform/atmel/atmel-isc.c   |  2 +-
 drivers/media/platform/atmel/atmel-isi.c   |  2 +-
 drivers/media/platform/davinci/vpif_capture.c  |  4 ++--
 drivers/media/platform/exynos4-is/media-dev.c  |  4 ++--
 drivers/media/platform/pxa_camera.c|  2 +-
 drivers/media/platform/qcom/camss-8x16/camss.c |  2 +-
 drivers/media/platform/rcar-vin/rcar-core.c|  2 +-
 drivers/media/platform/rcar_drif.c |  4 ++--
 drivers/media/platform/soc_camera/soc_camera.c |  2 +-
 drivers/media/platform/stm32/stm32-dcmi.c  |  2 +-
 drivers/media/platform/ti-vpe/cal.c|  2 +-
 drivers/media/platform/xilinx/xilinx-vipp.c|  2 +-
 drivers/media/v4l2-core/v4l2-async.c   | 16 
 drivers/media/v4l2-core/v4l2-fwnode.c  | 10 +-
 drivers/staging/media/imx/imx-media-dev.c  |  4 ++--
 include/media/v4l2-async.h |  8 ++--
 17 files changed, 35 insertions(+), 39 deletions(-)

diff --git a/drivers/media/platform/am437x/am437x-vpfe.c 
b/drivers/media/platform/am437x/am437x-vpfe.c
index 0997c640191d..601ae6487617 100644
--- a/drivers/media/platform/am437x/am437x-vpfe.c
+++ b/drivers/media/platform/am437x/am437x-vpfe.c
@@ -2304,8 +2304,8 @@ vpfe_async_bound(struct v4l2_async_notifier *notifier,
vpfe_dbg(1, vpfe, "vpfe_async_bound\n");
 
for (i = 0; i < ARRAY_SIZE(vpfe->cfg->asd); i++) {
-   if (vpfe->cfg->asd[i]->match.fwnode.fwnode ==
-   asd[i].match.fwnode.fwnode) {
+   if (vpfe->cfg->asd[i]->match.fwnode ==
+   asd[i].match.fwnode) {
sdinfo = >cfg->sub_devs[i];
vpfe->sd[i] = subdev;
vpfe->sd[i]->grp_id = sdinfo->grp_id;
@@ -2510,7 +2510,7 @@ vpfe_get_pdata(struct platform_device *pdev)
}
 
pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
-   pdata->asd[i]->match.fwnode.fwnode = of_fwnode_handle(rem);
+   pdata->asd[i]->match.fwnode = of_fwnode_handle(rem);
of_node_put(rem);
}
 
diff --git a/drivers/media/platform/atmel/atmel-isc.c 
b/drivers/media/platform/atmel/atmel-isc.c
index 0c2635647f69..34676409ca08 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -2088,7 +2088,7 @@ static int isc_parse_dt(struct device *dev, struct 
isc_device *isc)
subdev_entity->pfe_cfg0 |= ISC_PFE_CFG0_PPOL_LOW;
 
subdev_entity->asd->match_type = V4L2_ASYNC_MATCH_FWNODE;
-   subdev_entity->asd->match.fwnode.fwnode =
+   subdev_entity->asd->match.fwnode =
of_fwnode_handle(rem);
list_add_tail(_entity->list, >subdev_entities);
}
diff --git a/drivers/media/platform/atmel/atmel-isi.c 
b/drivers/media/platform/atmel/atmel-isi.c
index e900995143a3..9958918e2449 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -1128,7 +1128,7 @@ static int isi_graph_parse(struct atmel_isi *isi, struct 
device_node *node)
/* Remote node to connect */
isi->entity.node = remote;
isi->entity.asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
-   isi->entity.asd.match.fwnode.fwnode = of_fwnode_handle(remote);
+   isi->entity.asd.match.fwnode = of_fwnode_handle(remote);
return 0;
}
 }
diff --git a/drivers/media/platform/davinci/vpif_capture.c 
b/drivers/media/platform/davinci/vpif_capture.c
index e45916f69def..e1c273c8b9a6 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1390,7 +1390,7 @@ static int vpif_async_bound(struct v4l2_async_notifier 
*notifier,
 
for (i = 0; i < vpif_obj.config->asd_sizes[0]; i++) {
struct v4l2_async_subdev *_asd = vpif_obj.config->asd[i];
-   const struct fwnode_handle *fwnode = _asd->match.fwnode.fwnode;
+   const struct fwnode_handle *fwnode = _asd->match.fwnode;
 
if (fwnode == subdev->fwnode) {
vpif_o

[PATCH 5/8] media: v4l2-mediabus: convert flags to enums and document them

[Mauro Carvalho Chehab]

There is a mess with media bus flags: there are two sets of
flags, one used by parallel and ITU-R BT.656 outputs,
and another one for CSI2.

Depending on the type, the same bit has different meanings.

That's very confusing, and counter-intuitive. So, split them
into two sets of flags, inside an enum.

This way, it becomes clearer that there are two separate sets
of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
need a different set of flags.

As a side effect, enums can be documented via kernel-docs,
so there will be an improvement at flags documentation.

Unfortunately, soc_camera and pxa_camera do a mess with
the flags, using either one set of flags without proper
checks about the type. That could be fixed, but, as both drivers
are obsolete and in the process of cleanings, I opted to just
keep the behavior, using an unsigned int inside those two
drivers.

Acked-by: Hans Verkuil <hans.verk...@cisco.com>
Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/i2c/adv7180.c|  10 +-
 drivers/media/i2c/ml86v7667.c  |   5 +-
 drivers/media/i2c/mt9m111.c|   8 +-
 drivers/media/i2c/ov6650.c |  19 +--
 drivers/media/i2c/soc_camera/imx074.c  |   6 +-
 drivers/media/i2c/soc_camera/mt9m001.c |  10 +-
 drivers/media/i2c/soc_camera/mt9t031.c |  11 +-
 drivers/media/i2c/soc_camera/mt9t112.c |  11 +-
 drivers/media/i2c/soc_camera/mt9v022.c |  16 ++-
 drivers/media/i2c/soc_camera/ov5642.c  |   5 +-
 drivers/media/i2c/soc_camera/ov772x.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9640.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9740.c  |  10 +-
 drivers/media/i2c/soc_camera/rj54n1cb0c.c  |  12 +-
 drivers/media/i2c/soc_camera/tw9910.c  |  13 +-
 drivers/media/i2c/tc358743.c   |  10 +-
 drivers/media/i2c/tvp5150.c|   6 +-
 drivers/media/platform/pxa_camera.c|   8 +-
 drivers/media/platform/rcar-vin/rcar-core.c|   4 +-
 drivers/media/platform/rcar-vin/rcar-dma.c |   4 +-
 .../platform/soc_camera/sh_mobile_ceu_camera.c |   2 +-
 drivers/media/platform/soc_camera/soc_camera.c |   3 +-
 .../platform/soc_camera/soc_camera_platform.c  |   2 +-
 drivers/media/platform/soc_camera/soc_mediabus.c   |   2 +-
 drivers/media/v4l2-core/v4l2-fwnode.c  |   5 +-
 include/media/v4l2-fwnode.h|   4 +-
 include/media/v4l2-mediabus.h  | 145 ++---
 27 files changed, 218 insertions(+), 133 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 25d24a3f10a7..4bf25a72ef4f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -743,16 +743,16 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 
if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
cfg->type = V4L2_MBUS_CSI2;
-   cfg->flags = V4L2_MBUS_CSI2_1_LANE |
-   V4L2_MBUS_CSI2_CHANNEL_0 |
-   V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->csi2_flags = V4L2_MBUS_CSI2_1_LANE
+ | V4L2_MBUS_CSI2_CHANNEL_0
+ | V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
} else {
/*
 * The ADV7180 sensor supports BT.601/656 output modes.
 * The BT.656 is default and not yet configurable by s/w.
 */
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
}
 
diff --git a/drivers/media/i2c/ml86v7667.c b/drivers/media/i2c/ml86v7667.c
index 57ef901edb06..a25114d0c31f 100644
--- a/drivers/media/i2c/ml86v7667.c
+++ b/drivers/media/i2c/ml86v7667.c
@@ -226,8 +226,9 @@ static int ml86v7667_fill_fmt(struct v4l2_subdev *sd,
 static int ml86v7667_g_mbus_config(struct v4l2_subdev *sd,
   struct v4l2_mbus_config *cfg)
 {
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER
+   | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
 
return 0;
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index b1665d97e0fd..d9698b535080 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -857,9 +857,11 @@ static int mt9m111_e

Re: [PATCH v2 13/17] media: v4l2-async: simplify v4l2_async_subdev structure

[Mauro Carvalho Chehab]

Em Sat, 30 Sep 2017 01:05:24 +0300
Sakari Ailus <sakari.ai...@iki.fi> escreveu:

> Hi Mauro,
> 
> (Removing the non-list recipients.)
> 
> On Fri, Sep 29, 2017 at 06:27:13AM -0300, Mauro Carvalho Chehab wrote:
> > Em Thu, 28 Sep 2017 15:09:21 +0300
> > Sakari Ailus <sakari.ai...@iki.fi> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > On Wed, Sep 27, 2017 at 06:46:56PM -0300, Mauro Carvalho Chehab wrote:  
> > > > The V4L2_ASYNC_MATCH_FWNODE match criteria requires just one
> > > > struct to be filled (struct fwnode_handle). The V4L2_ASYNC_MATCH_DEVNAME
> > > > match criteria requires just a device name.
> > > > 
> > > > So, it doesn't make sense to enclose those into structs,
> > > > as the criteria can go directly into the union.
> > > > 
> > > > That makes easier to document it, as we don't need to document
> > > > weird senseless structs.
> > > 
> > > The idea is that in the union, there's a struct which is specific to the
> > > match_type field. I wouldn't call it senseless.  
> > 
> > Why a struct for each specific match_type is **needed**? It it is not
> > needed, then it is senseless per definition :-) 
> > 
> > In the specific case of fwnode, there's already a named struct
> > for fwnode_handle. The only thing is that it is declared outside
> > enum v4l2_async_match_type. So, I don't see any reason to do things
> > like:
> > 
> > struct {
> > struct fwnode_handle *fwnode;
> > } fwnode;
> > 
> > If you're in doubt about that, think on how would you document
> > both fwnode structs. Both fwnode structs specify the match
> > criteria if %V4L2_ASYNC_MATCH_FWNODE.
> > 
> > The same applies to this:
> > 
> > struct {
> > const char *name;
> > } device_name;
> > 
> > Both device_name and name specifies the match criteria if
> > %V4L2_ASYNC_MATCH_DEVNAME.
> >   
> > > 
> > > In the two cases there's just a single field in the containing struct. You
> > > could remove the struct in that case as you do in this patch, and just use
> > > the field. But I think the result is less clean and so I wouldn't make 
> > > this
> > > change.  
> > 
> > It is actually cleaner without the stucts.
> > 
> > Without the useless struct, if one wants to match a firmware node, it
> > should be doing:
> > 
> > pdata->asd[i]->match_type = V4L2_ASYNC_MATCH_FWNODE;
> > pdata->asd[i]->match.fwnode = of_fwnode_handle(rem);  
> 
> This code should be and will be moved out of drivers. See:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg122688.html>
> 
> So there are going to be quite a bit fewer instances of it, and none should
> remain in drivers. I frankly don't have a strong opinion on this; there are
> arguments for and against. I just don't see a reason to change it.

There are still a few occurrences on drivers. Just rebased it.
I'll post it in a few, inside a new patch series.

Simplifying the name of the match rules makes easier to understand
what's going on.


Thanks,
Mauro

Re: [PATCH 04/24] media: v4l2-mediabus: convert flags to enums and document them

[Mauro Carvalho Chehab]

Em Wed, 11 Oct 2017 23:26:44 +0200
Pavel Machek <pa...@ucw.cz> escreveu:

> On Mon 2017-10-09 07:19:10, Mauro Carvalho Chehab wrote:
> > There is a mess with media bus flags: there are two sets of
> > flags, one used by parallel and ITU-R BT.656 outputs,
> > and another one for CSI2.
> > 
> > Depending on the type, the same bit has different meanings.
> >   
> 
> > @@ -86,11 +125,22 @@ enum v4l2_mbus_type {
> >  /**
> >   * struct v4l2_mbus_config - media bus configuration
> >   * @type:  in: interface type
> > - * @flags: in / out: configuration flags, depending on @type
> > + * @pb_flags:  in / out: configuration flags, if @type is
> > + * %V4L2_MBUS_PARALLEL or %V4L2_MBUS_BT656.
> > + * @csi2_flags:in / out: configuration flags, if @type is
> > + * %V4L2_MBUS_CSI2.
> > + * @flag:  access flags, no matter the @type.
> > + * Used just to avoid needing to rewrite the logic inside
> > + * soc_camera and pxa_camera drivers. Don't use on newer
> > + * drivers!
> >   */
> >  struct v4l2_mbus_config {
> > enum v4l2_mbus_type type;
> > -   unsigned int flags;
> > +   union {
> > +   enum v4l2_mbus_parallel_and_bt656_flags pb_flags;
> > +   enum v4l2_mbus_csi2_flags   csi2_flags;
> > +   unsigned intflag;
> > +   };
> >  };
> >  
> >  static inline void v4l2_fill_pix_format(struct v4l2_pix_format
> >  *pix_fmt,  
> 
> The flags->flag conversion is quite subtle, and "flag" is confusing
> because there is more than one inside. What about something like
> __legacy_flags?

Good idea.

> 
>   Pavel

Thanks,
Mauro

[PATCH] media: v4l2-mediabus: convert flags to enums and document them

There is a mess with media bus flags: there are two sets of
flags, one used by parallel and ITU-R BT.656 outputs,
and another one for CSI2.

Depending on the type, the same bit has different meanings.

That's very confusing, and counter-intuitive. So, split them
into two sets of flags, inside an enum.

This way, it becomes clearer that there are two separate sets
of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
need a different set of flags.

As a side effect, enums can be documented via kernel-docs,
so there will be an improvement at flags documentation.

Unfortunately, soc_camera and pxa_camera do a mess with
the flags, using either one set of flags without proper
checks about the type. That could be fixed, but, as both drivers
are obsolete and in the process of cleanings, I opted to just
keep the behavior, using an unsigned int inside those two
drivers.

Acked-by: Hans Verkuil <hans.verk...@cisco.com>
Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 25d24a3f10a7..4bf25a72ef4f 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -743,16 +743,16 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 
if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
cfg->type = V4L2_MBUS_CSI2;
-   cfg->flags = V4L2_MBUS_CSI2_1_LANE |
-   V4L2_MBUS_CSI2_CHANNEL_0 |
-   V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->csi2_flags = V4L2_MBUS_CSI2_1_LANE
+ | V4L2_MBUS_CSI2_CHANNEL_0
+ | V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
} else {
/*
 * The ADV7180 sensor supports BT.601/656 output modes.
 * The BT.656 is default and not yet configurable by s/w.
 */
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
}
 
diff --git a/drivers/media/i2c/ml86v7667.c b/drivers/media/i2c/ml86v7667.c
index 57ef901edb06..a25114d0c31f 100644
--- a/drivers/media/i2c/ml86v7667.c
+++ b/drivers/media/i2c/ml86v7667.c
@@ -226,8 +226,9 @@ static int ml86v7667_fill_fmt(struct v4l2_subdev *sd,
 static int ml86v7667_g_mbus_config(struct v4l2_subdev *sd,
   struct v4l2_mbus_config *cfg)
 {
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER
+   | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA

Re: [PATCH 00/24] V4L2 kAPI cleanups and documentation improvements part 2

[Mauro Carvalho Chehab]

Em Mon,  9 Oct 2017 07:19:06 -0300
Mauro Carvalho Chehab <mche...@s-opensource.com> escreveu:

> That's the second part of my V4L2 kAPI documentation improvements.
> It is meant to reduce the gap between the kAPI media headers
> and documentation, at least with regards to kernel-doc markups.
> 
> We should likely write more things at the ReST files under Documentation/
> to better describe some of those APIs (VB2 being likely the first candidate),
> but at least let's be sure that all V4L2 bits have kernel-doc markups.

I'm also applying the patches from this series that nobody commented,
or whose comments were fully addressed.

Thanks,
Mauro

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

[Mauro Carvalho Chehab]

Em Thu, 23 Nov 2017 15:21:01 +0100
Greg Kroah-Hartman <gre...@linuxfoundation.org> escreveu:

> On Thu, Nov 23, 2017 at 11:07:51AM -0200, Mauro Carvalho Chehab wrote:
> > Hi Laurent,
> > 
> > Em Thu, 16 Nov 2017 02:33:48 +0200
> > Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> escreveu:
> >   
> > > Device unplug being asynchronous, it naturally races with operations
> > > performed by userspace through ioctls or other file operations on video
> > > device nodes.
> > > 
> > > This leads to potential access to freed memory or to other resources
> > > during device access if unplug occurs during device access. To solve
> > > this, we need to wait until all device access completes when unplugging
> > > the device, and block all further access when the device is being
> > > unplugged.
> > > 
> > > Three new functions are introduced. The video_device_enter() and
> > > video_device_exit() functions must be used to mark entry and exit from
> > > all code sections where the device can be accessed. The
> > > video_device_unplug() function is then used in the unplug handler to
> > > mark the device as being unplugged and wait for all access to complete.
> > > 
> > > As an example mark the ioctl handler as a device access section. Other
> > > file operations need to be protected too, and blocking ioctls (such as
> > > VIDIOC_DQBUF) need to be handled as well.
> > > 
> > > Signed-off-by: Laurent Pinchart 
> > > <laurent.pinchart+rene...@ideasonboard.com>
> > > ---
> > >  drivers/media/v4l2-core/v4l2-dev.c | 57 
> > > ++
> > >  include/media/v4l2-dev.h   | 47 +++
> > >  2 files changed, 104 insertions(+)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> > > b/drivers/media/v4l2-core/v4l2-dev.c
> > > index c647ba648805..c73c6d49e7cf 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -156,6 +156,52 @@ void video_device_release_empty(struct video_device 
> > > *vdev)
> > >  }
> > >  EXPORT_SYMBOL(video_device_release_empty);
> > >  
> > > +int video_device_enter(struct video_device *vdev)
> > > +{
> > > + bool unplugged;
> > > +
> > > + spin_lock(>unplug_lock);
> > > + unplugged = vdev->unplugged;
> > > + if (!unplugged)
> > > + vdev->access_refcount++;
> > > + spin_unlock(>unplug_lock);
> > > +
> > > + return unplugged ? -ENODEV : 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_enter);
> > > +
> > > +void video_device_exit(struct video_device *vdev)
> > > +{
> > > + bool wake_up;
> > > +
> > > + spin_lock(>unplug_lock);
> > > + WARN_ON(--vdev->access_refcount < 0);
> > > + wake_up = vdev->access_refcount == 0;
> > > + spin_unlock(>unplug_lock);
> > > +
> > > + if (wake_up)
> > > + wake_up(>unplug_wait);
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_exit);
> > > +
> > > +void video_device_unplug(struct video_device *vdev)
> > > +{
> > > + bool unplug_blocked;
> > > +
> > > + spin_lock(>unplug_lock);
> > > + unplug_blocked = vdev->access_refcount > 0;
> > > + vdev->unplugged = true;
> > > + spin_unlock(>unplug_lock);
> > > +
> > > + if (!unplug_blocked)
> > > + return;
> > > +
> > > + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> > > + msecs_to_jiffies(15)))
> > > + WARN(1, "Timeout waiting for device access to complete\n");
> > > +}
> > > +EXPORT_SYMBOL_GPL(video_device_unplug);
> > > +
> > >  static inline void video_get(struct video_device *vdev)
> > >  {
> > >   get_device(>dev);
> > > @@ -351,6 +397,10 @@ static long v4l2_ioctl(struct file *filp, unsigned 
> > > int cmd, unsigned long arg)
> > >   struct video_device *vdev = video_devdata(filp);
> > >   int ret = -ENODEV;
> > >  
> > > + ret = video_device_enter(vdev);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > >   if (vdev->fops->unlocked_ioctl) {
> > >   struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);

[PATCH 19/22] media: drivers: remove "/**" from non-kernel-doc comments

[Mauro Carvalho Chehab]

Several comments are wrongly tagged as kernel-doc, causing
those warnings:

  drivers/media/rc/st_rc.c:98: warning: No description found for parameter 'irq'
  drivers/media/rc/st_rc.c:98: warning: No description found for parameter 
'data'
  drivers/media/pci/solo6x10/solo6x10-enc.c:183: warning: No description found 
for parameter 'solo_dev'
  drivers/media/pci/solo6x10/solo6x10-enc.c:183: warning: No description found 
for parameter 'ch'
  drivers/media/pci/solo6x10/solo6x10-enc.c:183: warning: No description found 
for parameter 'qp'
  drivers/media/usb/pwc/pwc-dec23.c:652: warning: Cannot understand  *
   on line 652 - I thought it was a doc line
  drivers/media/usb/dvb-usb/cinergyT2-fe.c:40: warning: No description found 
for parameter 'op'
  drivers/media/usb/dvb-usb/friio-fe.c:301: warning: Cannot understand  * (reg, 
val) commad list to initialize this module.
   on line 301 - I thought it was a doc line
  drivers/media/rc/streamzap.c:201: warning: No description found for parameter 
'urb'
  drivers/media/rc/streamzap.c:333: warning: No description found for parameter 
'intf'
  drivers/media/rc/streamzap.c:333: warning: No description found for parameter 
'id'
  drivers/media/rc/streamzap.c:464: warning: No description found for parameter 
'interface'
  drivers/media/i2c/ov5647.c:432: warning: Cannot understand  * @short Subdev 
core operations registration
   on line 432 - I thought it was a doc line
  drivers/media/usb/dvb-usb/friio.c:35: warning: No description found for 
parameter 'd'
  drivers/media/usb/dvb-usb/friio.c:35: warning: No description found for 
parameter 'addr'
  drivers/media/usb/dvb-usb/friio.c:35: warning: No description found for 
parameter 'wbuf'
  drivers/media/usb/dvb-usb/friio.c:35: warning: No description found for 
parameter 'wlen'
  drivers/media/usb/dvb-usb/friio.c:35: warning: No description found for 
parameter 'rbuf'
  drivers/media/usb/dvb-usb/friio.c:35: warning: No description found for 
parameter 'rlen'
  drivers/media/platform/vim2m.c:350: warning: No description found for 
parameter 'priv'
  drivers/media/dvb-frontends/tua6100.c:34: warning: cannot understand function 
prototype: 'struct tua6100_priv '
  drivers/media/platform/sti/hva/hva-h264.c:140: warning: cannot understand 
function prototype: 'struct hva_h264_stereo_video_sei '
  drivers/media/platform/sti/hva/hva-h264.c:150: warning: Cannot understand  * 
@frame_width: width in pixels of the buffer containing the input frame
   on line 150 - I thought it was a doc line
  drivers/media/platform/sti/hva/hva-h264.c:356: warning: Cannot understand  * 
@ slice_size: slice size
   on line 356 - I thought it was a doc line
  drivers/media/platform/sti/hva/hva-h264.c:369: warning: Cannot understand  * 
@ bitstream_size: bitstream size
   on line 369 - I thought it was a doc line
  drivers/media/platform/sti/hva/hva-h264.c:395: warning: Cannot understand  * 
@seq_info:  sequence information buffer
   on line 395 - I thought it was a doc line
  drivers/media/dvb-frontends/sp887x.c:137: warning: No description found for 
parameter 'fe'
  drivers/media/dvb-frontends/sp887x.c:137: warning: No description found for 
parameter 'fw'
  drivers/media/dvb-frontends/sp887x.c:287: warning: No description found for 
parameter 'n'
  drivers/media/dvb-frontends/sp887x.c:287: warning: No description found for 
parameter 'd'
  drivers/media/dvb-frontends/sp887x.c:287: warning: No description found for 
parameter 'quotient_i'
  drivers/media/dvb-frontends/sp887x.c:287: warning: No description found for 
parameter 'quotient_f'
  drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c:83: warning: cannot 
understand function prototype: 'struct ttusb '
  drivers/media/platform/sh_veu.c:277: warning: No description found for 
parameter 'priv'
  drivers/media/dvb-frontends/zl10036.c:33: warning: cannot understand function 
prototype: 'int zl10036_debug; '
  drivers/media/dvb-frontends/zl10036.c:179: warning: No description found for 
parameter 'state'
  drivers/media/dvb-frontends/zl10036.c:179: warning: No description found for 
parameter 'frequency'
  drivers/media/platform/rcar_fdp1.c:1139: warning: No description found for 
parameter 'priv'
  drivers/media/platform/ti-vpe/vpe.c:933: warning: No description found for 
parameter 'priv'
  drivers/media/usb/gspca/ov519.c:36: warning: No description found for 
parameter 'fmt'
  drivers/media/usb/dvb-usb/dib0700_devices.c:3367: warning: No description 
found for parameter 'adap'

Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/dvb-frontends/sp887x.c  |  6 +++---
 drivers/media/dvb-frontends/tua6100.c |  2 +-
 drivers/media/dvb-frontends/zl10036.c |  8 
 drivers/media/i2c/ov5647.c|  4 ++--
 drivers/media/pci/solo6x10/solo6x10-enc.c |  2 +-
 drivers/media/platform/rcar_fdp1.c|  2 +-
 drivers/media/platform/sh_veu.c   |  2 +-
 drivers/media/platfo

[PATCH 16/22] media: vsp1: add a missing kernel-doc parameter

[Mauro Carvalho Chehab]

Fix this warning:
drivers/media/platform/vsp1/vsp1_dl.c:87: warning: No description found 
for parameter 'has_chain'

Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/platform/vsp1/vsp1_dl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 8b5cbb6b7a70..4257451f1bd8 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -70,6 +70,7 @@ struct vsp1_dl_body {
  * @dma: DMA address for the header
  * @body0: first display list body
  * @fragments: list of extra display list bodies
+ * @has_chain: if true, indicates that there's a partition chain
  * @chain: entry in the display list partition chain
  */
 struct vsp1_dl_list {
-- 
2.14.3

Re: [PATCH/RFC 1/2] v4l: v4l2-dev: Add infrastructure to protect device unplug race

[Mauro Carvalho Chehab]

Hi Laurent,

Em Thu, 16 Nov 2017 02:33:48 +0200
Laurent Pinchart  escreveu:

> Device unplug being asynchronous, it naturally races with operations
> performed by userspace through ioctls or other file operations on video
> device nodes.
> 
> This leads to potential access to freed memory or to other resources
> during device access if unplug occurs during device access. To solve
> this, we need to wait until all device access completes when unplugging
> the device, and block all further access when the device is being
> unplugged.
> 
> Three new functions are introduced. The video_device_enter() and
> video_device_exit() functions must be used to mark entry and exit from
> all code sections where the device can be accessed. The
> video_device_unplug() function is then used in the unplug handler to
> mark the device as being unplugged and wait for all access to complete.
> 
> As an example mark the ioctl handler as a device access section. Other
> file operations need to be protected too, and blocking ioctls (such as
> VIDIOC_DQBUF) need to be handled as well.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/media/v4l2-core/v4l2-dev.c | 57 
> ++
>  include/media/v4l2-dev.h   | 47 +++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c 
> b/drivers/media/v4l2-core/v4l2-dev.c
> index c647ba648805..c73c6d49e7cf 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -156,6 +156,52 @@ void video_device_release_empty(struct video_device 
> *vdev)
>  }
>  EXPORT_SYMBOL(video_device_release_empty);
>  
> +int video_device_enter(struct video_device *vdev)
> +{
> + bool unplugged;
> +
> + spin_lock(>unplug_lock);
> + unplugged = vdev->unplugged;
> + if (!unplugged)
> + vdev->access_refcount++;
> + spin_unlock(>unplug_lock);
> +
> + return unplugged ? -ENODEV : 0;
> +}
> +EXPORT_SYMBOL_GPL(video_device_enter);
> +
> +void video_device_exit(struct video_device *vdev)
> +{
> + bool wake_up;
> +
> + spin_lock(>unplug_lock);
> + WARN_ON(--vdev->access_refcount < 0);
> + wake_up = vdev->access_refcount == 0;
> + spin_unlock(>unplug_lock);
> +
> + if (wake_up)
> + wake_up(>unplug_wait);
> +}
> +EXPORT_SYMBOL_GPL(video_device_exit);
> +
> +void video_device_unplug(struct video_device *vdev)
> +{
> + bool unplug_blocked;
> +
> + spin_lock(>unplug_lock);
> + unplug_blocked = vdev->access_refcount > 0;
> + vdev->unplugged = true;
> + spin_unlock(>unplug_lock);
> +
> + if (!unplug_blocked)
> + return;
> +
> + if (!wait_event_timeout(vdev->unplug_wait, !vdev->access_refcount,
> + msecs_to_jiffies(15)))
> + WARN(1, "Timeout waiting for device access to complete\n");
> +}
> +EXPORT_SYMBOL_GPL(video_device_unplug);
> +
>  static inline void video_get(struct video_device *vdev)
>  {
>   get_device(>dev);
> @@ -351,6 +397,10 @@ static long v4l2_ioctl(struct file *filp, unsigned int 
> cmd, unsigned long arg)
>   struct video_device *vdev = video_devdata(filp);
>   int ret = -ENODEV;
>  
> + ret = video_device_enter(vdev);
> + if (ret < 0)
> + return ret;
> +
>   if (vdev->fops->unlocked_ioctl) {
>   struct mutex *lock = v4l2_ioctl_get_lock(vdev, cmd);
>  
> @@ -358,11 +408,14 @@ static long v4l2_ioctl(struct file *filp, unsigned int 
> cmd, unsigned long arg)
>   return -ERESTARTSYS;
>   if (video_is_registered(vdev))
>   ret = vdev->fops->unlocked_ioctl(filp, cmd, arg);
> + else
> + ret = -ENODEV;
>   if (lock)
>   mutex_unlock(lock);
>   } else
>   ret = -ENOTTY;
>  
> + video_device_exit(vdev);
>   return ret;
>  }
>  
> @@ -841,6 +894,10 @@ int __video_register_device(struct video_device *vdev, 
> int type, int nr,
>   if (WARN_ON(!vdev->v4l2_dev))
>   return -EINVAL;
>  
> + /* unplug support */
> + spin_lock_init(>unplug_lock);
> + init_waitqueue_head(>unplug_wait);
> +

I'm c/c Greg here, as I don't think, that, the way it is, it
belongs at V4L2 core.

I mean: if this is a problem that affects all drivers, it would should, 
instead, be sitting at the driver's core.

If, otherwise, this is specific to rcar-vin (and other platform drivers),
that's likely should be inside the drivers that require it.

That's said, I remember we had to add some things in the past for
USB drivers hot unplug to happen softly. I don't remember the specifics
anymore, but it was solved by both a V4L2 core and changes at USB
drivers.

One of the things that it was added, on that time, was this patch:

commit 

[PATCH v2 13/26] media: rcar: fix a debug printk

[Mauro Carvalho Chehab]

Two orthogonal changesets caused a breakage at a printk
inside rcar. Changeset 859969b38e2e
("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API")
made davinci to use struct fwnode_handle instead of
struct device_node. Changeset 68d9c47b1679
("media: Convert to using %pOF instead of full_name")
changed the printk to not use ->full_name, but, instead,
to rely on %pOF.

With both patches applied, the Kernel will do the wrong
thing, as warned by smatch:
drivers/media/platform/rcar-vin/rcar-core.c:189 
rvin_digital_graph_init() error: '%pOF' expects argument of type 'struct 
device_node*', argument 4 has type 'void*'

So, change the logic to actually print the device name
that was obtained before the print logic.

Fixes: 68d9c47b1679 ("media: Convert to using %pOF instead of full_name")
Fixes: 859969b38e2e ("[media] v4l: Switch from V4L2 OF not V4L2 fwnode API")
Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c 
b/drivers/media/platform/rcar-vin/rcar-core.c
index 108d776f3265..ce5914f7a056 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -186,8 +186,8 @@ static int rvin_digital_graph_init(struct rvin_dev *vin)
if (!vin->digital)
return -ENODEV;
 
-   vin_dbg(vin, "Found digital subdevice %pOF\n",
-   to_of_node(vin->digital->asd.match.fwnode.fwnode));
+   vin_dbg(vin, "Found digital subdevice %s\n",
+   to_of_node(vin->digital->asd.match.fwnode.fwnode)->full_name);
 
vin->notifier.ops = _digital_notify_ops;
ret = v4l2_async_notifier_register(>v4l2_dev, >notifier);
-- 
2.13.6

Re: [PATCH 00/24] V4L2 kAPI cleanups and documentation improvements part 2

[Mauro Carvalho Chehab]

Em Mon,  9 Oct 2017 07:19:06 -0300
Mauro Carvalho Chehab <mche...@s-opensource.com> escreveu:

> That's the second part of my V4L2 kAPI documentation improvements.
> It is meant to reduce the gap between the kAPI media headers
> and documentation, at least with regards to kernel-doc markups.
> 
> We should likely write more things at the ReST files under Documentation/
> to better describe some of those APIs (VB2 being likely the first candidate),
> but at least let's be sure that all V4L2 bits have kernel-doc markups.

Forgot to mention: 

This series, together with the part1 one is on this tree:

https://git.linuxtv.org/mchehab/experimental.git/log/?h=v4l-docs-part2-v1

As I may not have time to address the remaining kAPI bits for
a while, let me document what kAPIs at include/media main dir
are still out of sync:

- cec-pin.h: struct cec_pin;
- cec.h: several structs;
- media-device.h, media-devnode.h: a couple of defines;
- rc-core.h: ancillary functions;
- soc-camera.h, v4l2-clk.h, videobuf-*.h - probably not
  worth the efforts, as those are obsolete kAPI;
- v4l2-common.h: print macros. IMHO, we should try to get
  rid of them, and use dev_foo() everywhere;
- v4l2-fwnode.h: In this specific case, Sakari has a patch
  series addressing it, moving existing documentation from
  v4l2-fwnode.c;
- v4l2-mem2mem.h: v4l2 ioctl helpers aren't documented;
- videobuf2-*.h: This series contain several documentation
  improvements there for VB2 core and V4L2, with are the
  most important ones - as they're used by the drivers.
  The other headers are either undocumented or barely
  documented.

PS.: It should be noticed that there are other files under
 include/media and inside their sub-directories that aren't
 part of the media kAPI, but are driver-specific stuff,
 like, for example, imx.h, vsp1.h and i2c/ subdir.


Thanks,
Mauro

[PATCH 00/24] V4L2 kAPI cleanups and documentation improvements part 2

[Mauro Carvalho Chehab]

That's the second part of my V4L2 kAPI documentation improvements.
It is meant to reduce the gap between the kAPI media headers
and documentation, at least with regards to kernel-doc markups.

We should likely write more things at the ReST files under Documentation/
to better describe some of those APIs (VB2 being likely the first candidate),
but at least let's be sure that all V4L2 bits have kernel-doc markups.

Mauro Carvalho Chehab (24):
  media: v4l2-dev.h: add kernel-doc to two macros
  media: v4l2-flash-led-class.h: add kernel-doc to two ancillary funcs
  media: v4l2-mediabus: use BIT() macro for flags
  media: v4l2-mediabus: convert flags to enums and document them
  media: v4l2-dev: convert VFL_TYPE_* into an enum
  media: i2c-addr.h: get rid of now unused defines
  media: get rid of i2c-addr.h
  media: v4l2-dev: document VFL_DIR_* direction defines
  media: v4l2-dev: document video_device flags
  media: v4l2-subdev: use kernel-doc markups to document subdev flags
  media: v4l2-subdev: create cross-references for ioctls
  media: v4l2-subdev: fix description of tuner.s_radio ops
  media: v4l2-subdev: better document IO pin configuration flags
  media: v4l2-subdev: convert frame description to enum
  media: v4l2-subdev: get rid of __V4L2_SUBDEV_MK_GET_TRY() macro
  media: v4l2-subdev: document remaining undocumented functions
  media: v4l2-subdev: fix a typo
  media: vb2-core: use bitops for bits
  media: vb2-core: Improve kernel-doc markups
  media: vb2-core: document remaining functions
  media: vb2-core: fix descriptions for VB2-only functions
  media: vb2: add cross references at memops and v4l2 kernel-doc markups
  media: v4l2-tpg*.h: move headers to include/media/tpg and merge them
  media: v4l2-tpg.h: rename color structs

 Documentation/media/kapi/v4l2-dev.rst  |  17 +-
 drivers/media/common/v4l2-tpg/v4l2-tpg-colors.c|   8 +-
 drivers/media/common/v4l2-tpg/v4l2-tpg-core.c  |   2 +-
 drivers/media/i2c/adv7180.c|  10 +-
 drivers/media/i2c/ml86v7667.c  |   5 +-
 drivers/media/i2c/mt9m111.c|   8 +-
 drivers/media/i2c/ov6650.c |  19 +-
 drivers/media/i2c/soc_camera/imx074.c  |   6 +-
 drivers/media/i2c/soc_camera/mt9m001.c |  10 +-
 drivers/media/i2c/soc_camera/mt9t031.c |  11 +-
 drivers/media/i2c/soc_camera/mt9t112.c |  11 +-
 drivers/media/i2c/soc_camera/mt9v022.c |  16 +-
 drivers/media/i2c/soc_camera/ov5642.c  |   5 +-
 drivers/media/i2c/soc_camera/ov772x.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9640.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9740.c  |  10 +-
 drivers/media/i2c/soc_camera/rj54n1cb0c.c  |  12 +-
 drivers/media/i2c/soc_camera/tw9910.c  |  13 +-
 drivers/media/i2c/tc358743.c   |  10 +-
 drivers/media/i2c/tda7432.c|   1 -
 drivers/media/i2c/tvaudio.c|   2 -
 drivers/media/i2c/tvp5150.c|   6 +-
 drivers/media/pci/bt8xx/bttv-cards.c   |   7 +
 drivers/media/pci/bt8xx/bttv.h |   1 -
 drivers/media/pci/cx88/cx88-blackbird.c|   3 +-
 drivers/media/pci/cx88/cx88-video.c|  10 +-
 drivers/media/pci/cx88/cx88.h  |   4 +-
 drivers/media/pci/saa7134/saa7134-video.c  |   2 +
 drivers/media/platform/pxa_camera.c|   8 +-
 drivers/media/platform/rcar-vin/rcar-core.c|   4 +-
 drivers/media/platform/rcar-vin/rcar-dma.c |   4 +-
 .../platform/soc_camera/sh_mobile_ceu_camera.c |   2 +-
 drivers/media/platform/soc_camera/soc_camera.c |   3 +-
 .../platform/soc_camera/soc_camera_platform.c  |   2 +-
 drivers/media/platform/soc_camera/soc_mediabus.c   |   2 +-
 drivers/media/platform/vimc/vimc-sensor.c  |   2 +-
 drivers/media/platform/vivid/vivid-core.h  |   2 +-
 drivers/media/usb/cx231xx/cx231xx-video.c  |   2 +
 drivers/media/usb/em28xx/em28xx-cards.c|   1 -
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c   |   2 +
 drivers/media/usb/tm6000/tm6000-cards.c|   1 -
 drivers/media/usb/tm6000/tm6000-video.c|   2 +
 drivers/media/v4l2-core/v4l2-dev.c |  10 +-
 drivers/media/v4l2-core/v4l2-fwnode.c  |   5 +-
 include/media/i2c-addr.h   |  42 --
 include/media/i2c/tvaudio.h|  17 +-
 include/media/{ => tpg}/v4l2-tpg.h |  45 +-
 include/media/v4l2-dev.h   | 124 --
 include/media/v4l2-flash-led-class.h   |  12 +
 include/media/v4l2-fwnode.h|   4 +-
 include/media/v4l2-mediabus.h  | 176 ++--
 include/media/v4l2-subdev.h| 293 +
 include/media/v4l2-tpg-color

[PATCH 04/24] media: v4l2-mediabus: convert flags to enums and document them

[Mauro Carvalho Chehab]

There is a mess with media bus flags: there are two sets of
flags, one used by parallel and ITU-R BT.656 outputs,
and another one for CSI2.

Depending on the type, the same bit has different meanings.

That's very confusing, and counter-intuitive. So, split them
into two sets of flags, inside an enum.

This way, it becomes clearer that there are two separate sets
of flags. It also makes easier if CSI1, CSP, CSI3, etc. would
need a different set of flags.

As a side effect, enums can be documented via kernel-docs,
so there will be an improvement at flags documentation.

Unfortunately, soc_camera and pxa_camera do a mess with
the flags, using either one set of flags without proper
checks about the type. That could be fixed, but, as both drivers
are obsolete and in the process of cleanings, I opted to just
keep the behavior, using an unsigned int inside those two
drivers.

Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/i2c/adv7180.c|  10 +-
 drivers/media/i2c/ml86v7667.c  |   5 +-
 drivers/media/i2c/mt9m111.c|   8 +-
 drivers/media/i2c/ov6650.c |  19 +--
 drivers/media/i2c/soc_camera/imx074.c  |   6 +-
 drivers/media/i2c/soc_camera/mt9m001.c |  10 +-
 drivers/media/i2c/soc_camera/mt9t031.c |  11 +-
 drivers/media/i2c/soc_camera/mt9t112.c |  11 +-
 drivers/media/i2c/soc_camera/mt9v022.c |  16 ++-
 drivers/media/i2c/soc_camera/ov5642.c  |   5 +-
 drivers/media/i2c/soc_camera/ov772x.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9640.c  |  10 +-
 drivers/media/i2c/soc_camera/ov9740.c  |  10 +-
 drivers/media/i2c/soc_camera/rj54n1cb0c.c  |  12 +-
 drivers/media/i2c/soc_camera/tw9910.c  |  13 +-
 drivers/media/i2c/tc358743.c   |  10 +-
 drivers/media/i2c/tvp5150.c|   6 +-
 drivers/media/platform/pxa_camera.c|   8 +-
 drivers/media/platform/rcar-vin/rcar-core.c|   4 +-
 drivers/media/platform/rcar-vin/rcar-dma.c |   4 +-
 .../platform/soc_camera/sh_mobile_ceu_camera.c |   2 +-
 drivers/media/platform/soc_camera/soc_camera.c |   3 +-
 .../platform/soc_camera/soc_camera_platform.c  |   2 +-
 drivers/media/platform/soc_camera/soc_mediabus.c   |   2 +-
 drivers/media/v4l2-core/v4l2-fwnode.c  |   5 +-
 include/media/v4l2-fwnode.h|   4 +-
 include/media/v4l2-mediabus.h  | 144 ++---
 27 files changed, 217 insertions(+), 133 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index 3df28f2f9b38..c220504049de 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -743,16 +743,16 @@ static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
 
if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
cfg->type = V4L2_MBUS_CSI2;
-   cfg->flags = V4L2_MBUS_CSI2_1_LANE |
-   V4L2_MBUS_CSI2_CHANNEL_0 |
-   V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
+   cfg->csi2_flags = V4L2_MBUS_CSI2_1_LANE
+ | V4L2_MBUS_CSI2_CHANNEL_0
+ | V4L2_MBUS_CSI2_CONTINUOUS_CLOCK;
} else {
/*
 * The ADV7180 sensor supports BT.601/656 output modes.
 * The BT.656 is default and not yet configurable by s/w.
 */
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
}
 
diff --git a/drivers/media/i2c/ml86v7667.c b/drivers/media/i2c/ml86v7667.c
index 57ef901edb06..a25114d0c31f 100644
--- a/drivers/media/i2c/ml86v7667.c
+++ b/drivers/media/i2c/ml86v7667.c
@@ -226,8 +226,9 @@ static int ml86v7667_fill_fmt(struct v4l2_subdev *sd,
 static int ml86v7667_g_mbus_config(struct v4l2_subdev *sd,
   struct v4l2_mbus_config *cfg)
 {
-   cfg->flags = V4L2_MBUS_MASTER | V4L2_MBUS_PCLK_SAMPLE_RISING |
-V4L2_MBUS_DATA_ACTIVE_HIGH;
+   cfg->pb_flags = V4L2_MBUS_MASTER
+   | V4L2_MBUS_PCLK_SAMPLE_RISING
+   | V4L2_MBUS_DATA_ACTIVE_HIGH;
cfg->type = V4L2_MBUS_BT656;
 
return 0;
diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
index 99b992e46702..53c406f87aa7 100644
--- a/drivers/media/i2c/mt9m111.c
+++ b/drivers/media/i2c/mt9m111.c
@@ -857,9 +857,11 @@ static int mt9m111_enum_mbus_code(struct v4l2_subdev *sd,
 static int m

Re: [RFC PATCH v5 3/6] i2c: add docs to clarify DMA handling

[Mauro Carvalho Chehab]

Em Wed, 20 Sep 2017 20:59:53 +0200
Wolfram Sang <wsa+rene...@sang-engineering.com> escreveu:

> Signed-off-by: Wolfram Sang <wsa+rene...@sang-engineering.com>

Documentation looks OK on my eyes. So:

Reviewed-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

> ---
>  Documentation/i2c/DMA-considerations | 58 
> 
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/i2c/DMA-considerations
> 
> diff --git a/Documentation/i2c/DMA-considerations 
> b/Documentation/i2c/DMA-considerations
> new file mode 100644
> index 00..5a63355c6a9b6f
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,58 @@
> +=
> +Linux I2C and DMA
> +=
> +
> +Given that I2C is a low-speed bus where largely small messages are 
> transferred,
> +it is not considered a prime user of DMA access. At this time of writing, 
> only
> +10% of I2C bus master drivers have DMA support implemented. And the vast
> +majority of transactions are so small that setting up DMA for it will likely
> +add more overhead than a plain PIO transfer.
> +
> +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA 
> safe.
> +It does not seem reasonable to apply additional burdens when the feature is 
> so
> +rarely used. However, it is recommended to use a DMA-safe buffer if your
> +message size is likely applicable for DMA. Most drivers have this threshold
> +around 8 bytes (as of today, this is mostly an educated guess, however). For
> +any message of 16 byte or larger, it is probably a really good idea. Please
> +note that other subsystems you use might add requirements. E.g., if your
> +I2C bus master driver is using USB as a bridge, then you need to have DMA
> +safe buffers always, because USB requires it.
> +
> +For clients, if you use a DMA safe buffer in i2c_msg, set the I2C_M_DMA_SAFE
> +flag with it. Then, the I2C core and drivers know they can safely operate DMA
> +on it. Note that using this flag is optional. I2C host drivers which are not
> +updated to use this flag will work like before. And like before, they risk
> +using an unsafe DMA buffer. To improve this situation, using I2C_M_DMA_SAFE 
> in
> +more and more clients and host drivers is the planned way forward. Note also
> +that setting this flag makes only sense in kernel space. User space data is
> +copied into kernel space anyhow. The I2C core makes sure the destination
> +buffers in kernel space are always DMA capable.
> +
> +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers 
> for i2c_smbus_xfer_emulated.
> +
> +Drivers wishing to implement safe DMA can use helper functions from the I2C
> +core. One gives you a DMA-safe buffer for a given i2c_msg as long as a 
> certain
> +threshold is met::
> +
> + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);
> +
> +If a buffer is returned, it is either msg->buf for the I2C_M_DMA_SAFE case 
> or a
> +bounce buffer. But you don't need to care about that detail, just use the
> +returned buffer. If NULL is returned, the threshold was not met or a bounce
> +buffer could not be allocated. Fall back to PIO in that case.
> +
> +In any case, a buffer obtained from above needs to be released. It ensures 
> data
> +is copied back to the message and a potentially used bounce buffer is freed::
> +
> + i2c_release_dma_safe_msg_buf(msg, dma_buf);
> +
> +The bounce buffer handling from the core is generic and simple. It will 
> always
> +allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
> +reusing pre-allocated buffers), you are free to implement your own.
> +
> +Please also check the in-kernel documentation for details. The i2c-sh_mobile
> +driver can be used as a reference example how to use the above helpers.
> +
> +Final note: If you plan to use DMA with I2C (or with anything else, actually)
> +make sure you have CONFIG_DMA_API_DEBUG enabled during development. It can 
> help
> +you find various issues which can be complex to debug otherwise.



Thanks,
Mauro

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Mauro Carvalho Chehab]

Em Wed, 20 Sep 2017 19:18:40 +0200
Wolfram Sang  escreveu:

> Hi Mauro,
> 
> > > +Linux I2C and DMA
> > > +-  
> > 
> > I would use, instead:
> > 
> > =
> > Linux I2C and DMA
> > =
> > 
> > As this is the way we're starting document titles, after converted to
> > ReST. So, better to have it already using the right format, as one day  
> 
> I did this.
> 
> > There are also a couple of things here that Sphinx would complain.  
> 
> The only complaint I got was
> 
>   WARNING: document isn't included in any toctree
> 
> which makes sense because I renamed it only temporarily to *.rst

Yeah, that is expected.

> > So, it could be worth to rename it to *.rst, while you're writing
> > it, and see what:
> > make htmldocs
> > will complain and how it will look in html.  
> 
> So, no complaints from Sphinx and the HTML output looks good IMO. Was
> there anything specific you had in mind when saying that Sphinx would
> complain?

Perhaps my comments weren't clear enough. Sorry! I didn't actually 
tried to parse it with Sphinx. Just wanted to hint you about that,
as testing the docs with Sphinx could be useful when writing
documentation. 

Usually, things like function declarations produce warnings if they
contain pointers, e. g. something like:

foo(void *bar);

as asterisks mean italics. It would complain about the lack of
an end asterisk.

In order to avoid that, and to place them into a box using monotonic fonts,
I usually add "::" at the preceding line, e. g.:

::

foo(void *bar);

or:

some description::

foo(void *bar)

on all functions (even the ones that don't use asterisks, as the
html output looks nicer.

I double-checked this patch: it doesn't contain anything that would
cause warnings or parse errors. Still, I would prefer to use
**not** instead of *not*, and would add the "::", but that's my
personal taste.

Thanks,
Mauro


pgpy9sivKUuHI.pgp
Description: Assinatura digital OpenPGP

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Mauro Carvalho Chehab]

Em Sat, 9 Sep 2017 17:27:06 +0200
Wolfram Sang  escreveu:

> > Yes, but the statistics that 10% of I2C bus master drivers
> > are DMA-safe is not true, if you take those into account ;-)
> > 
> > Perhaps you could write it as (or something similar):
> > 
> > At this time of writing, only +10% of I2C bus master
> > drivers for non-remote boards have DMA support implemented.
> 
> No, this is confusing IMO. Being remote has nothing to with DMA. What
> has to do with DMA is that these are using USB as communication channel.
> And USB clearly needs DMA-safe buffers. The encapsulation needs DMA-safe
> buffers. So, I think the new sentence "other subsystems might impose"
> mentions that.
> 
> Let me recap what this series is for:
> 
> a) It makes clear that I2C subsystem does not require DMA-safety because
> for the reasons given in the textfile.
> 
> If I2C is encapsulated over USB, then DMA-safety is of course required,
> but this comes from USB. So, those USB-I2C master drivers need to do
> something to make sure DMA-safety is guaranteed because i2c_msg buffers
> don't need to be DMA safe because of a). They could use the newly
> introduced features, but they don't have to.
> 
> b) a flag for DMA safe i2c_msgs is added
> 
> So, for messages we know to be DMA safe, we can flag that. Drivers
> could check that and use a bounce buffer if the flag is not set.
> However, this is all optional. Your drivers can still choose to not
> check the flag and everything will stay as before. Check patch 5 for a
> use case.
> 
> c) helper functions for bounce buffers are added
> 
> These are *helper* functions for drivers wishing to do DMA. A super
> simple bounce buffer support. Check patch 4 for a use case. Again, this
> is optional. Drivers can implement their own bounce buffer support. Or,
> as in your case, if you know that your buffers are good, then don't use
> any of this and things will keep going.
> 
> 
> This all is to allow bus master drivers to opt-in for DMA safety if they
> want to do DMA directly. Because currently, with a lot of i2c_msgs on
> stack, this is more or less working accidently.
> 
> And, yes, I know I have to add this new flag to a few central places in
> client drivers. Otherwise, master drivers checking for DMA safety will
> initially have a performance regression. This is scheduled for V5 and
> mentioned in this series.
> 
> > In the past, on lots of drivers, the i2c_xfer logic just used to assume
> > that the I2C client driver allocated a DMA-safe buffer, as it just used to
> > pass whatever buffer it receives directly to USB core. We did an effort to
> > change it, as it can be messy, but I'm not sure if this is solved 
> > everywhere.  
> 
> Good, I can imagine this being some effort. But again, this is because
> USB needs the DMA-safety, not I2C. AFAICS, most i2c_msgs are register
> accesses and thus, small messages.
> 
> > The usage of I2C at the media subsystem predates the I2C subsystem.
> > at V4L2 drivers, a great effort was done to port it to fully use the
> > I2C subsystem when it was added upstream, but there were some problems
> > a the initial implementation of the i2c subsystem that prevented its
> > usage for the DVB drivers. By the time such restrictions got removed,
> > it was too late, as there were already a large amount of drivers relying
> > on i2c "low level" functions.  
> 
> Didn't know that, but this is good to know! Thanks.
> 
> > Even on the drivers that use i2c_add_adapter(), the usage of DMA can't
> > be get by the above grep, as the DMA usage is actually at drivers/usb.  
> 
> Ok. But as said before, what works now will continue to work. 

OK, good!

> This series is about clarifying that i2c_msg buffers need not to be DMA safe
> and offering some helpers for those bus master drivers wanting to opt-in
> to be sure.
> 
> Clearer now?

Yeah, it is clearer for me. Thanks!

Anyway, it doesn't hurt if you could improve the documentation patch to
mention the above somewhow, as I want to avoid patches trying to make
existing media USB drivers to use the new flag without changing the
already existing DMA-safe logic there (causing double-buffering), or, 
even worse, making the I2C bus DMA-unsafe because someone misread
this document.


Regards,
Mauro

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Mauro Carvalho Chehab]

Em Fri, 8 Sep 2017 10:56:40 +0200
Wolfram Sang  escreveu:

> Hi Mauro,
> 
> thanks for your comments. Much appreciated!
> 
> > There are also a couple of things here that Sphinx would complain.
> > So, it could be worth to rename it to *.rst, while you're writing
> > it, and see what:
> > make htmldocs
> > will complain and how it will look in html.  
> 
> OK, I'll check that.

Ok.

> > > +Given that I2C is a low-speed bus where largely small messages are 
> > > transferred,
> > > +it is not considered a prime user of DMA access. At this time of 
> > > writing, only
> > > +10% of I2C bus master drivers have DMA support implemented.  
> > 
> > Are you sure about that? I'd say that, on media, at least half of the
> > drivers use DMA for I2C bus access, as the I2C bus is on a remote
> > board that talks with CPU via USB, using DMA, and all communication
> > with USB should be DMA-safe.  
> 
> Well, the DMA-safe requirement comes then from the USB subsystem,
> doesn't it?

Yes, but the statistics that 10% of I2C bus master drivers
are DMA-safe is not true, if you take those into account ;-)

Perhaps you could write it as (or something similar):

At this time of writing, only +10% of I2C bus master
drivers for non-remote boards have DMA support implemented.  


> Or do you really use DMA on the remote board to transfer
> data via I2C to an I2C client?
> 
> > I guess what you really wanted to say on most of this section is
> > about SoC (and other CPUs) where the I2C bus master is is at the
> > mainboard, and not on some peripheral.  
> 
> I might be biased to that, yes. So, it is good talking about it.
> 
> > > And the vast
> > > +majority of transactions are so small that setting up DMA for it will 
> > > likely
> > > +add more overhead than a plain PIO transfer.
> > > +
> > > +Therefore, it is *not* mandatory that the buffer of an I2C message is 
> > > DMA safe.  
> > 
> > Again, that may not be true on media boards. The code that implements the
> > I2C transfers there, on most boards, have to be DMA safe, as it won't
> > otherwise send/receive commands from the chips that are after the USB
> > bridge.  
> 
> That still sounds to me like the DMA-safe requirement comes from USB
> (which is fine, of course.). In any case, a sentence like "Other
> subsystem you might use for bridging I2C might impose other DMA
> requirements" sounds like to nice to have.

Agreed.

> 
> > > +Drivers wishing to implement DMA can use helper functions from the I2C 
> > > core.
> > > +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain
> > > +threshold is met.
> > > +
> > > + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);  
> > 
> > I'm concerned about the new bits added by this call. Right now,
> > USB drivers just use kalloc() for transfer buffers used to send and
> > receive URB control messages for both setting the main device and
> > for I2C messages. Before this changeset, buffers allocated this
> > way are DMA save and have been working for years.  
> 
> Can you give me a pointer to a driver doing this? I glimpsed around in
> drivers/media/usb and found that most drivers are having their i2c_msg
> buffers on the stack. Which is clearly not DMA-safe.

The way it is implemented depends on the driver, and usually envolves
double-buffering, e. g., on some place of the driver, a buffer that
may not be DMA-save is copied into a DMA safe buffer. 

On most cases, like on this driver:
drivers/media/usb/dvb-usb-v2/az6007.c

The i2c_xfer logic (or the read/write functions) is the one responsible
for double-buffering.

In this specific example, the DVB usbv2 core allocates the device's "state"
struct using kmalloc (it uses .size_of_priv field to know the size of
the "state" buffer).

On struct az6007_device_state, there is a "data" buffer with 4096 bytes.

At the i2c transfer function, it retrieves and use it:

struct az6007_device_state *st = d_to_priv(d)
...
ret = __az6007_read(d->udev, req, value, index, st->data, length);
...
ret =  __az6007_write(d->udev, req, value, index, st->data, length);


In the past, on lots of drivers, the i2c_xfer logic just used to assume
that the I2C client driver allocated a DMA-safe buffer, as it just used to
pass whatever buffer it receives directly to USB core. We did an effort to
change it, as it can be messy, but I'm not sure if this is solved everywhere.

The __az6007_read() and __az6007_write() indirectly do DMA (for most USB
host drivers), when they call usb_control_msg().

> 
> > When you add some flags that would make the I2C subsystem aware
> > that a buffer is now DMA safe, I guess you could be breaking
> > those drivers, as a DMA safe buffer will now be considered as
> > DMA-unsafe.  
> 
> Well, this flag is only relevant for i2c master drivers wishing to do
> DMA. So, grepping in the above directory
> 
>   grep dma $(grep -rl i2c_add_adapter *)

The usage of I2C at the media 

Re: [RFC PATCH v4 3/6] i2c: add docs to clarify DMA handling

[Mauro Carvalho Chehab]

Em Thu, 17 Aug 2017 16:14:46 +0200
Wolfram Sang  escreveu:

> Signed-off-by: Wolfram Sang 
> ---
>  Documentation/i2c/DMA-considerations | 50 
> 
>  1 file changed, 50 insertions(+)
>  create mode 100644 Documentation/i2c/DMA-considerations
> 
> diff --git a/Documentation/i2c/DMA-considerations 
> b/Documentation/i2c/DMA-considerations
> new file mode 100644
> index 00..a4b4a107102452
> --- /dev/null
> +++ b/Documentation/i2c/DMA-considerations
> @@ -0,0 +1,50 @@
> +Linux I2C and DMA
> +-

I would use, instead:

=
Linux I2C and DMA
=

As this is the way we're starting document titles, after converted to
ReST. So, better to have it already using the right format, as one day
someone will convert i2c documentation to ReST. So, it would be
really cool if this document could be just renamed without needing
to patch it during such conversion :-)

There are also a couple of things here that Sphinx would complain.
So, it could be worth to rename it to *.rst, while you're writing
it, and see what:
make htmldocs
will complain and how it will look in html.

> +
> +Given that I2C is a low-speed bus where largely small messages are 
> transferred,
> +it is not considered a prime user of DMA access. At this time of writing, 
> only
> +10% of I2C bus master drivers have DMA support implemented.

Are you sure about that? I'd say that, on media, at least half of the
drivers use DMA for I2C bus access, as the I2C bus is on a remote
board that talks with CPU via USB, using DMA, and all communication
with USB should be DMA-safe.

I guess what you really wanted to say on most of this section is
about SoC (and other CPUs) where the I2C bus master is is at the
mainboard, and not on some peripheral.

> And the vast
> +majority of transactions are so small that setting up DMA for it will likely
> +add more overhead than a plain PIO transfer.
> +
> +Therefore, it is *not* mandatory that the buffer of an I2C message is DMA 
> safe.

Again, that may not be true on media boards. The code that implements the
I2C transfers there, on most boards, have to be DMA safe, as it won't
otherwise send/receive commands from the chips that are after the USB
bridge.

> +It does not seem reasonable to apply additional burdens when the feature is 
> so
> +rarely used. However, it is recommended to use a DMA-safe buffer if your
> +message size is likely applicable for DMA. Most drivers have this threshold
> +around 8 bytes (as of today, this is mostly an educated guess, however). For
> +any message of 16 byte or larger, it is probably a really good idea.
> +
> +If you use such a buffer in a i2c_msg, set the I2C_M_DMA_SAFE flag with it.
> +Then, the I2C core and drivers know they can safely operate DMA on it. Note
> +that setting this flag makes only sense in kernel space. User space data is
> +copied into kernel space anyhow. The I2C core makes sure the destination
> +buffers in kernel space are always DMA capable.
> +
> +FIXME: Need to implement i2c_master_{send|receive}_dma and proper buffers 
> for i2c_smbus_xfer_emulated.
> +
> +Drivers wishing to implement DMA can use helper functions from the I2C core.
> +One gives you a DMA-safe buffer for a given i2c_msg as long as a certain
> +threshold is met.
> +
> + dma_buf = i2c_get_dma_safe_msg_buf(msg, threshold_in_byte);

I'm concerned about the new bits added by this call. Right now,
USB drivers just use kalloc() for transfer buffers used to send and
receive URB control messages for both setting the main device and
for I2C messages. Before this changeset, buffers allocated this
way are DMA save and have been working for years.

When you add some flags that would make the I2C subsystem aware
that a buffer is now DMA safe, I guess you could be breaking
those drivers, as a DMA safe buffer will now be considered as
DMA-unsafe.

So, you'll likely need to patch all media USB drivers to fix it,
or come up with some other solution.

> +
> +If a buffer is returned, it either msg->buf for the I2C_M_DMA_SAFE case or a
> +bounce buffer. But you don't need to care about that detail. If NULL is
> +returned, the threshold was not met or a bounce buffer could not be 
> allocated.
> +Fall back to PIO in that case.
> +
> +In any case, a buffer obtained from above needs to be released. It ensures 
> data
> +is copied back to the message and a potentially used bounce buffer is freed.
> +
> + i2c_release_dma_safe_msg_buf(msg, dma_buf);
> +
> +The bounce buffer handling from the core is generic and simple. It will 
> always
> +allocate a new bounce buffer. If you want a more sophisticated handling (e.g.
> +reusing pre-allocated buffers), you are free to implement your own.
> +
> +Please also check the in-kernel documentation for details. The i2c-sh_mobile
> +driver can be used as a reference example how to use the above helpers.
> +
> +Final 

[PATCH 0/3] document types of hardware control for V4L2

[Mauro Carvalho Chehab]

On 2010, we introduced a new way to control complex V4L2 devices used
on embedded systems, but this was never documented, nor it is possible
for an userspace applicatin to detect the kind of control a device supports.

This series fill the gap.

Mauro Carvalho Chehab (3):
  media: open.rst: document devnode-centric and mc-centric types
  media: videodev2: add a flag for vdev-centric devices
  media: add V4L2_CAP_VDEV_CENTERED flag on vdev-centric drivers

 Documentation/media/uapi/v4l/open.rst| 56 
 Documentation/media/uapi/v4l/vidioc-querycap.rst |  4 ++
 drivers/media/pci/bt8xx/bttv-driver.c|  4 +-
 drivers/media/pci/cobalt/cobalt-v4l2.c   |  3 +-
 drivers/media/pci/cx18/cx18-ioctl.c  |  4 +-
 drivers/media/pci/cx23885/cx23885-417.c  |  2 +-
 drivers/media/pci/cx23885/cx23885-video.c|  3 +-
 drivers/media/pci/cx25821/cx25821-video.c|  6 ++-
 drivers/media/pci/cx88/cx88-video.c  |  3 +-
 drivers/media/pci/dt3155/dt3155.c|  3 +-
 drivers/media/pci/ivtv/ivtv-ioctl.c  |  5 ++-
 drivers/media/pci/meye/meye.c|  2 +-
 drivers/media/pci/saa7134/saa7134-video.c|  3 +-
 drivers/media/pci/saa7164/saa7164-encoder.c  |  3 +-
 drivers/media/pci/saa7164/saa7164-vbi.c  |  3 +-
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c   |  3 +-
 drivers/media/pci/solo6x10/solo6x10-v4l2.c   |  3 +-
 drivers/media/pci/sta2x11/sta2x11_vip.c  |  3 +-
 drivers/media/pci/tw5864/tw5864-video.c  |  2 +-
 drivers/media/pci/tw68/tw68-video.c  |  3 +-
 drivers/media/pci/tw686x/tw686x-video.c  |  2 +-
 drivers/media/pci/zoran/zoran_driver.c   |  3 +-
 drivers/media/platform/rcar_drif.c   |  3 +-
 drivers/media/platform/vivid/vivid-core.c|  2 +-
 drivers/media/usb/airspy/airspy.c|  3 +-
 drivers/media/usb/au0828/au0828-video.c  |  3 +-
 drivers/media/usb/cpia2/cpia2_v4l.c  |  5 ++-
 drivers/media/usb/cx231xx/cx231xx-video.c|  5 ++-
 drivers/media/usb/em28xx/em28xx-video.c  | 11 +++--
 drivers/media/usb/go7007/go7007-v4l2.c   |  2 +-
 drivers/media/usb/gspca/gspca.c  |  3 +-
 drivers/media/usb/hackrf/hackrf.c|  8 ++--
 drivers/media/usb/hdpvr/hdpvr-video.c|  2 +-
 drivers/media/usb/msi2500/msi2500.c  |  3 +-
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c |  6 ++-
 drivers/media/usb/pwc/pwc-v4l.c  |  2 +-
 drivers/media/usb/s2255/s2255drv.c   |  2 +-
 drivers/media/usb/stk1160/stk1160-v4l.c  |  3 +-
 drivers/media/usb/stkwebcam/stk-webcam.c |  3 +-
 drivers/media/usb/tm6000/tm6000-video.c  |  5 ++-
 drivers/media/usb/usbtv/usbtv-video.c|  2 +-
 drivers/media/usb/usbvision/usbvision-video.c|  5 ++-
 drivers/media/usb/uvc/uvc_v4l2.c |  8 ++--
 drivers/media/usb/zr364xx/zr364xx.c  |  5 ++-
 include/uapi/linux/videodev2.h   |  2 +
 45 files changed, 158 insertions(+), 58 deletions(-)

-- 
2.13.3

[PATCH 3/3] media: add V4L2_CAP_VDEV_CENTERED flag on vdev-centric drivers

[Mauro Carvalho Chehab]

From: Mauro Carvalho Chehab <mche...@osg.samsung.com>

Those devices are controlled via their V4L2 device. Add a
flag to indicate them as such.

Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/pci/bt8xx/bttv-driver.c  |  4 +++-
 drivers/media/pci/cobalt/cobalt-v4l2.c |  3 ++-
 drivers/media/pci/cx18/cx18-ioctl.c|  4 ++--
 drivers/media/pci/cx23885/cx23885-417.c|  2 +-
 drivers/media/pci/cx23885/cx23885-video.c  |  3 ++-
 drivers/media/pci/cx25821/cx25821-video.c  |  6 --
 drivers/media/pci/cx88/cx88-video.c|  3 ++-
 drivers/media/pci/dt3155/dt3155.c  |  3 ++-
 drivers/media/pci/ivtv/ivtv-ioctl.c|  5 +++--
 drivers/media/pci/meye/meye.c  |  2 +-
 drivers/media/pci/saa7134/saa7134-video.c  |  3 ++-
 drivers/media/pci/saa7164/saa7164-encoder.c|  3 ++-
 drivers/media/pci/saa7164/saa7164-vbi.c|  3 ++-
 drivers/media/pci/solo6x10/solo6x10-v4l2-enc.c |  3 ++-
 drivers/media/pci/solo6x10/solo6x10-v4l2.c |  3 ++-
 drivers/media/pci/sta2x11/sta2x11_vip.c|  3 ++-
 drivers/media/pci/tw5864/tw5864-video.c|  2 +-
 drivers/media/pci/tw68/tw68-video.c|  3 ++-
 drivers/media/pci/tw686x/tw686x-video.c|  2 +-
 drivers/media/pci/zoran/zoran_driver.c |  3 ++-
 drivers/media/platform/rcar_drif.c |  3 ++-
 drivers/media/platform/vivid/vivid-core.c  |  2 +-
 drivers/media/usb/airspy/airspy.c  |  3 ++-
 drivers/media/usb/au0828/au0828-video.c|  3 ++-
 drivers/media/usb/cpia2/cpia2_v4l.c|  5 +++--
 drivers/media/usb/cx231xx/cx231xx-video.c  |  5 +++--
 drivers/media/usb/em28xx/em28xx-video.c| 11 +++
 drivers/media/usb/go7007/go7007-v4l2.c |  2 +-
 drivers/media/usb/gspca/gspca.c|  3 ++-
 drivers/media/usb/hackrf/hackrf.c  |  8 +---
 drivers/media/usb/hdpvr/hdpvr-video.c  |  2 +-
 drivers/media/usb/msi2500/msi2500.c|  3 ++-
 drivers/media/usb/pvrusb2/pvrusb2-v4l2.c   |  6 --
 drivers/media/usb/pwc/pwc-v4l.c|  2 +-
 drivers/media/usb/s2255/s2255drv.c |  2 +-
 drivers/media/usb/stk1160/stk1160-v4l.c|  3 ++-
 drivers/media/usb/stkwebcam/stk-webcam.c   |  3 ++-
 drivers/media/usb/tm6000/tm6000-video.c|  5 +++--
 drivers/media/usb/usbtv/usbtv-video.c  |  2 +-
 drivers/media/usb/usbvision/usbvision-video.c  |  5 +++--
 drivers/media/usb/uvc/uvc_v4l2.c   |  8 +---
 drivers/media/usb/zr364xx/zr364xx.c|  5 +++--
 42 files changed, 96 insertions(+), 58 deletions(-)

diff --git a/drivers/media/pci/bt8xx/bttv-driver.c 
b/drivers/media/pci/bt8xx/bttv-driver.c
index 40110be4e986..382cc76b954b 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -2481,6 +2481,7 @@ static int bttv_querycap(struct file *file, void  *priv,
V4L2_CAP_VIDEO_CAPTURE |
V4L2_CAP_READWRITE |
V4L2_CAP_STREAMING |
+   V4L2_CAP_VDEV_CENTERED |
V4L2_CAP_DEVICE_CAPS;
if (no_overlay <= 0)
cap->capabilities |= V4L2_CAP_VIDEO_OVERLAY;
@@ -2511,7 +2512,8 @@ static int bttv_querycap(struct file *file, void  *priv,
 V4L2_CAP_STREAMING |
 V4L2_CAP_TUNER);
else {
-   cap->device_caps = V4L2_CAP_RADIO | V4L2_CAP_TUNER;
+   cap->device_caps = V4L2_CAP_RADIO | V4L2_CAP_TUNER |
+  V4L2_CAP_VDEV_CENTERED;
if (btv->has_saa6588)
cap->device_caps |= V4L2_CAP_READWRITE |
V4L2_CAP_RDS_CAPTURE;
diff --git a/drivers/media/pci/cobalt/cobalt-v4l2.c 
b/drivers/media/pci/cobalt/cobalt-v4l2.c
index def4a3b37084..803a9cf09a9f 100644
--- a/drivers/media/pci/cobalt/cobalt-v4l2.c
+++ b/drivers/media/pci/cobalt/cobalt-v4l2.c
@@ -495,7 +495,8 @@ static int cobalt_querycap(struct file *file, void *priv_fh,
strlcpy(vcap->card, "cobalt", sizeof(vcap->card));
snprintf(vcap->bus_info, sizeof(vcap->bus_info),
 "PCIe:%s", pci_name(cobalt->pci_dev));
-   vcap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_READWRITE;
+   vcap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_READWRITE |
+   V4L2_CAP_VDEV_CENTERED;
if (s->is_output)
vcap->device_caps |= V4L2_CAP_VIDEO_OUTPUT;
else
diff --git a/drivers/media/pci/cx18/cx18-ioctl.c 
b/drivers/media/pci/cx18/cx18-ioctl.c
index 80b902b12a78..3e258fdc6685 100644
--- a/drivers/media/pci/cx18/cx18-ioctl.c
+++ b/drivers/media/pci/cx18/cx18-ioctl.c
@@ -402,8 +402,8 @@ static int cx18_querycap(

Re: [PATCH v2 3/3] drm: rcar-du: Repair vblank for DRM page flips using the VSP

[Mauro Carvalho Chehab]

Em Wed, 12 Jul 2017 01:29:42 +0300
Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> escreveu:

> From: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> 
> The driver recently switched from handling page flip completion in the
> DU vertical blanking handler to the VSP frame end handler to fix a race
> condition. This unfortunately resulted in incorrect timestamps in the
> vertical blanking events sent to userspace as vertical blanking is now
> handled after sending the event.
> 
> To fix this we must reverse the order of the two operations. The easiest
> way is to handle vertical blanking in the VSP frame end handler before
> sending the event. The VSP frame end interrupt occurs approximately 50µs
> earlier than the DU frame end interrupt, but this should not cause any
> undue harm.
> 
> As we need to handle vertical blanking even when page flip completion is
> delayed, the VSP driver now needs to call the frame end completion
> callback unconditionally, with a new argument to report whether page
> flip has completed.
> 
> With this new scheme the DU vertical blanking interrupt isn't needed
> anymore, so we can stop enabling it.
> 
> Fixes: d503a43ac06a ("drm: rcar-du: Register a completion callback with VSP1")
> Signed-off-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 11/14] v4l: vsp1: Add support for header display lists in continuous mode

[Mauro Carvalho Chehab]

Em Mon, 26 Jun 2017 21:12:23 +0300
Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> escreveu:

> The VSP supports both header and headerless display lists. The latter is
> easier to use when the VSP feeds data directly to the DU in continuous
> mode, and the driver thus uses headerless display lists for DU operation
> and header display lists otherwise.
> 
> Headerless display lists are only available on WPF.0. This has never
> been an issue so far, as only WPF.0 is connected to the DU. However, on
> H3 ES2.0, the VSP-DL instance has both WPF.0 and WPF.1 connected to the
> DU. We thus can't use headerless display lists unconditionally for DU
> operation.
> 
> Implement support for continuous mode with header display lists, and use
> it for DU operation on WPF outputs that don't support headerless mode.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 10/14] v4l: vsp1: Add support for multiple DRM pipelines

[Mauro Carvalho Chehab]

Em Mon, 26 Jun 2017 21:12:22 +0300
Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> escreveu:

> The R-Car H3 ES2.0 VSP-DL instance has two LIF entities and can drive
> two display pipelines at the same time. Refactor the VSP DRM code to
> support that by introducing a vsp_drm_pipeline object that models one
> display pipeline.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 09/14] v4l: vsp1: Add support for multiple LIF instances

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 18:57:40 +0100
Kieran Bingham <kieran.bing...@ideasonboard.com> escreveu:

> Hi Laurent,
> 
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > The VSP2-DL instance (present in the H3 ES2.0 and M3-N SoCs) has two LIF
> > instances. Adapt the driver infrastructure to support multiple LIFs.
> > Support for multiple display pipelines will be added separately.
> > 
> > The change to the entity routing table removes the ability to connect
> > the LIF output to the HGO or HGT histogram generators. This feature is
> > only available on Gen2 hardware, isn't supported by the rest of the
> > driver, and has no known use case, so this isn't an issue.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> 
> >  
> This looks good.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>


Thanks,
Mauro

Re: [PATCH v2.1 08/14] v4l: vsp1: Add support for new VSP2-BS, VSP2-DL and VSP2-D instances

[Mauro Carvalho Chehab]

Em Fri, 14 Jul 2017 03:35:57 +0300
Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> escreveu:

> New Gen3 SoCs come with two new VSP2 variants names VSP2-BS and VSP2-DL,
> as well as a new VSP2-D variant on V3M and V3H SoCs. Add new entries for
> them in the VSP device info table.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 07/14] v4l: vsp1: Add support for the BRS entity

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 14:38:40 +0100
Kieran Bingham <kieran.bingham+rene...@ideasonboard.com> escreveu:

> On 26/06/17 19:12, Laurent Pinchart wrote:
> > The Blend/ROP Sub Unit (BRS) is a stripped-down version of the BRU found
> > in several VSP2 instances. Compared to a regular BRU, it supports two
> > inputs only, and thus has no ROP unit.
> > 
> > Add support for the BRS by modeling it as a new entity type, but reuse  
> 
> s/modeling/modelling/
> 
> 
> > the vsp1_bru object underneath. Chaining the BRU and BRS entities seems
> > to be supported by the hardware but isn't implemented yet as it isn't
> > the primary use case for the BRS.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> 
> >  
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 06/14] v4l: vsp1: Add pipe index argument to the VSP-DU API

[Mauro Carvalho Chehab]

Em Fri, 14 Jul 2017 02:04:06 +0300
Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:

> On Thursday 13 Jul 2017 14:16:03 Kieran Bingham wrote:
> > On 26/06/17 19:12, Laurent Pinchart wrote:  
> > > In the H3 ES2.0 SoC the VSP2-DL instance has two connections to DU
> > > channels that need to be configured independently. Extend the VSP-DU API
> > > with a pipeline index to identify which pipeline the caller wants to
> > > operate on.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+rene...@ideasonboard.com>  
> > 
> > A bit of comment merge between this and the next patch but it's minor and
> > not worth the effort to change that ... so I'll happily ignore it if you do
> > :)
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> >   
> > > ---
> > > 
> > >  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  | 12 ++--
> > >  drivers/media/platform/vsp1/vsp1_drm.c | 32 +++
> > >  include/media/vsp1.h   | 10 ++
> > >  3 files changed, 34 insertions(+), 20 deletions(-)  
> 
> [snip]
> 
> > > diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> > > b/drivers/media/platform/vsp1/vsp1_drm.c index c72d021ff820..daaafe7885fa
> > > 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_drm.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> > > @@ -58,21 +58,26 @@ EXPORT_SYMBOL_GPL(vsp1_du_init);
> > >  /**
> > >   * vsp1_du_setup_lif - Setup the output part of the VSP pipeline
> > >   * @dev: the VSP device
> > > + * @pipe_index: the DRM pipeline index
> > >   * @cfg: the LIF configuration
> > >   *
> > >   * Configure the output part of VSP DRM pipeline for the given frame
> > >   @cfg.width
> > > - * and @cfg.height. This sets up formats on the BRU source pad, the WPF0
> > > sink
> > > - * and source pads, and the LIF sink pad.
> > > + * and @cfg.height. This sets up formats on the blend unit (BRU or BRS)
> > > source
> > > + * pad, the WPF sink and source pads, and the LIF sink pad.
> > >   *
> > > - * As the media bus code on the BRU source pad is conditioned by the
> > > - * configuration of the BRU sink 0 pad, we also set up the formats on all
> > > BRU
> > > + * The @pipe_index argument selects which DRM pipeline to setup. The
> > > number of
> > > + * available pipelines depend on the VSP instance.
> > > + *
> > > + * As the media bus code on the blend unit source pad is conditioned by
> > > the
> > > + * configuration of its sink 0 pad, we also set up the formats on all
> > > blend unit
> > >   * sinks, even if the configuration will be overwritten later by
> > > - * vsp1_du_setup_rpf(). This ensures that the BRU configuration is set to
> > > a well
> > > - * defined state.
> > > + * vsp1_du_setup_rpf(). This ensures that the blend unit configuration is
> > > set to
> > > + * a well defined state.  
> > 
> > I presume those comment updates for the BRU/ blend-unit configuration are
> > actually for the next patch - but I don't think it matters here - and isn't
> > worth the effort to move the hunks.  
> 
> Too late, I've fixed it already :-) Thanks for pointing it out.

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>


Thanks,
Mauro

Re: [PATCH v2 05/14] v4l: vsp1: Don't create links for DRM pipeline

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 14:06:04 +0100
Kieran Bingham <kieran.bingham+rene...@ideasonboard.com> escreveu:

> On 26/06/17 19:12, Laurent Pinchart wrote:
> > When the VSP1 is used in a DRM pipeline the driver doesn't register the
> > media device. Links between entities are not exposed to userspace, but
> > are still used internally for the sole purpose of setting up internal
> > source to sink pointers through the link setup handler.
> > 
> > Instead of going through this complex procedure, remove link creation
> > and set the sink pointers directly.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> 
> >  
> 
> A whole function removed ... always love code removal...
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 04/14] v4l: vsp1: Store source and sink pointers as vsp1_entity

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 14:00:31 +0100
Kieran Bingham <kieran.bingham+rene...@ideasonboard.com> escreveu:

> Hi Laurent,
> 
> This looks like a good simplification/removal of obfuscation to me!
> 
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > The internal VSP entity source and sink pointers are stored as
> > media_entity pointers, which are then cast to a vsp1_entity. As all
> > sources and sinks are vsp1_entity instances, we can store the
> > vsp1_entity pointers directly.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> 
> >  
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 03/14] v4l: vsp1: Don't set WPF sink pointer

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 13:50:20 +0100
Kieran Bingham <kieran.bingham+rene...@ideasonboard.com> escreveu:

> On 26/06/17 19:12, Laurent Pinchart wrote:
> > The sink pointer is used to configure routing inside the VSP, and as
> > such must point to the next VSP entity in the pipeline. The WPF being a
> > pipeline terminal sink, its output route can't be configured. The
> > routing configuration code already handles this correctly without
> > referring to the sink pointer, which thus doesn't need to be set.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> 
> >  
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>


Thanks,
Mauro

Re: [PATCH v2 02/14] v4l: vsp1: Don't recycle active list at display start

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 18:02:20 +0100
Kieran Bingham <kieran.bing...@ideasonboard.com> escreveu:

> Hi Laurent,
> 
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > When the display start interrupt occurs, we know that the hardware has
> > finished loading the active display list. The driver then proceeds to
> > recycle the list, assuming it won't be needed anymore.
> > 
> > This assumption holds true for headerless display lists, as the VSP
> > doesn't reload the list for the next frame if it hasn't changed.
> > However, this isn't true anymore for header display lists, as they are
> > loaded at every frame start regardless of whether they have been
> > updated.
> > 
> > To prepare for header display lists usage in display pipelines, we need
> > to postpone recycling the list until it gets replaced by a new one
> > through a page flip. The driver already does so in the frame end
> > interrupt handler, so all we need is to skip list recycling in the
> > display start interrupt handler.
> > 
> > While the active list can be recycled at display start for headerless
> > display lists, there's no real harm in postponing that to the frame end
> > interrupt handler in all cases. This simplifies interrupt handling as we
> > don't need to process the display start interrupt anymore.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> 
> >  
> 
> Ok, I had skipped this one as I was concerned about its effects in relation to
> 11/14 but I see how that's working now.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH v2 01/14] v4l: vsp1: Fill display list headers without holding dlm spinlock

[Mauro Carvalho Chehab]

Em Thu, 13 Jul 2017 13:48:40 +0100
Kieran Bingham <kieranbing...@gmail.com> escreveu:

> Hi Laurent,
> 
> Starts easy ... (I haven't gone through these in numerical order of course :D)
> 
> On 26/06/17 19:12, Laurent Pinchart wrote:
> > The display list headers are filled using information from the display
> > list only. Lower the display list manager spinlock contention by filling
> > the headers without holding the lock.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> 
> >  
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

Thanks,
Mauro

Re: [PATCH] media: Convert to using %pOF instead of full_name

[Mauro Carvalho Chehab]

Em Wed, 19 Jul 2017 11:02:01 -0500
Rob Herring  escreveu:

> On Wed, Jul 19, 2017 at 4:41 AM, Sylwester Nawrocki
>  wrote:
> > On 07/18/2017 11:43 PM, Rob Herring wrote:  
> >> Now that we have a custom printf format specifier, convert users of
> >> full_name to use %pOF instead. This is preparation to remove storing
> >> of the full path string for each node.
> >>
> >> Signed-off-by: Rob Herring   
> >  
> >> ---
> >>   drivers/media/i2c/s5c73m3/s5c73m3-core.c   |  3 +-
> >>   drivers/media/i2c/s5k5baf.c|  7 ++--
> >>   drivers/media/platform/am437x/am437x-vpfe.c|  4 +-
> >>   drivers/media/platform/atmel/atmel-isc.c   |  4 +-
> >>   drivers/media/platform/davinci/vpif_capture.c  | 16 
> >>   drivers/media/platform/exynos4-is/fimc-is.c|  8 ++--
> >>   drivers/media/platform/exynos4-is/fimc-lite.c  |  3 +-
> >>   drivers/media/platform/exynos4-is/media-dev.c  |  8 ++--
> >>   drivers/media/platform/exynos4-is/mipi-csis.c  |  4 +-
> >>   drivers/media/platform/mtk-mdp/mtk_mdp_comp.c  |  6 +--
> >>   drivers/media/platform/mtk-mdp/mtk_mdp_core.c  |  8 ++--
> >>   drivers/media/platform/omap3isp/isp.c  |  8 ++--
> >>   drivers/media/platform/pxa_camera.c|  2 +-
> >>   drivers/media/platform/rcar-vin/rcar-core.c|  4 +-
> >>   drivers/media/platform/soc_camera/soc_camera.c |  6 +--
> >>   drivers/media/platform/xilinx/xilinx-vipp.c| 52 
> >> +-
> >>   drivers/media/v4l2-core/v4l2-async.c   |  4 +-
> >>   drivers/media/v4l2-core/v4l2-clk.c |  3 +-
> >>   include/media/v4l2-clk.h   |  4 +-
> >>   19 files changed, 71 insertions(+), 83 deletions(-)  
> >  
> >> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c 
> >> b/drivers/media/platform/xilinx/xilinx-vipp.c
> >> index ac4704388920..9233ad0b1b6b 100644
> >> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
> >> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c  
> >  
> >> @@ -144,9 +144,8 @@ static int xvip_graph_build_one(struct 
> >> xvip_composite_device *xdev,
> >>   remote = ent->entity;
> >>
> >>   if (link.remote_port >= remote->num_pads) {
> >> - dev_err(xdev->dev, "invalid port number %u on %s\n",
> >> - link.remote_port,
> >> - to_of_node(link.remote_node)->full_name);
> >> + dev_err(xdev->dev, "invalid port number %u on 
> >> %pOF\n",
> >> + link.remote_port, link.remote_node);  
> >
> > Shouldn't there be to_of_node(link.remote_node) instead of link.remote_node 
> > ?  
> 
> Humm, yes. I thought I fixed that.

After such fix, I'm OK with this patch.

Are you planning to apply it on your tree, or via the media one?

I guess it is probably better to apply via media, in order to avoid
conflicts with other changes.

Thanks,
Mauro

Re: [PATCH v4] v4l: subdev: tolerate null in media_entity_to_v4l2_subdev

[Mauro Carvalho Chehab]

Em Wed,  7 Jun 2017 10:52:07 +0100
Kieran Bingham  escreveu:

> From: Kieran Bingham 
> 
> Return NULL, if a null entity is parsed for it's v4l2_subdev
> 
> Signed-off-by: Kieran Bingham 

Could you please improve this patch description?

I'm unsure if this is a bug fix, or some sort of feature...

On what situations would a null entity be passed to this function?

Regards,
Mauro

> 
> ---
> Not sure if this patch ever made it out of my mailbox:
> 
> Here's the respin with the parameter evaluated only once.
> 
> v4:
>  - Improve macro usage to evaluate ent only once
> 
>  include/media/v4l2-subdev.h | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index a40760174797..0f92ebd2d710 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -826,8 +826,15 @@ struct v4l2_subdev {
>   struct v4l2_subdev_platform_data *pdata;
>  };
>  
> -#define media_entity_to_v4l2_subdev(ent) \
> - container_of(ent, struct v4l2_subdev, entity)
> +#define media_entity_to_v4l2_subdev(ent) \
> +({   \
> + typeof(ent) __me_sd_ent = (ent);\
> + \
> + __me_sd_ent ?   \
> + container_of(__me_sd_ent, struct v4l2_subdev, entity) : \
> + NULL;   \
> +})
> +
>  #define vdev_to_v4l2_subdev(vdev) \
>   ((struct v4l2_subdev *)video_get_drvdata(vdev))
>  



Thanks,
Mauro

Re: [PATCH 6/7] [media] soc_camera: rcar_vin: use proper name for the R-Car SoC

[Mauro Carvalho Chehab]

Em Mon, 29 May 2017 14:02:02 +0200
Wolfram Sang  escreveu:

> >Why "soc_camera:" in the subject?  
> 
> I used 'git log $file' and copied the most recent entry. Do I need to
> resend?
> 

No need to resend. I'll fix it when applying the patch.

Btw, I'm seeing only patches 6 and 7 here at media ML (and patchwork).
As those are trivial changes, I'll just apply what I have.

Thanks,
Mauro

Re: [PATCH v6 3/7] media: i2c: max2175: Add MAX2175 support

[Mauro Carvalho Chehab]

Em Thu, 8 Jun 2017 09:42:43 +
Ramesh Shanmugasundaram  escreveu:

> > Subject: Re: [PATCH v6 3/7] media: i2c: max2175: Add MAX2175 support
> > 
> > Em Wed, 31 May 2017 09:44:53 +0100
> > Ramesh Shanmugasundaram  escreveu:
> >   
> > > +++ b/Documentation/media/v4l-drivers/max2175.rst
> > > @@ -0,0 +1,60 @@
> > > +Maxim Integrated MAX2175 RF to bits tuner driver
> > > +
> > > +
> > > +The MAX2175 driver implements the following driver-specific controls:
> > > +
> > > +``V4L2_CID_MAX2175_I2S_ENABLE``
> > > +---
> > > +Enable/Disable I2S output of the tuner.
> > > +
> > > +.. flat-table::
> > > +:header-rows:  0
> > > +:stub-columns: 0
> > > +:widths:   1 4
> > > +
> > > +* - ``(0)``
> > > +  - I2S output is disabled.
> > > +* - ``(1)``
> > > +  - I2S output is enabled.  
> > 
> > Hmm... There are other drivers at the subsystem that use I2S (for audio -
> > not for SDR - but I guess the issue is similar).
> > 
> > On such drivers, the bridge driver controls it directly, being sure that
> > I2S is enabled when it is expecting some data coming from the I2S bus.
> > 
> > On some drivers, there are both I2S and A/D inputs at the bridge chipset.
> > On such drivers, enabling/disabling I2S is done via VIDIOC_S_INPUT (and
> > optionally via ALSA mixer), being transparent to the user if the stream
> > comes from a tuner via I2S or from a directly connected A/D input.
> > 
> > I don't think it is a good idea to enable it via a control, as, if the
> > bridge driver is expecting data via I2S, disabling it will cause timeouts
> > at the videobuf handling.  
> 
> The MAX2175 device is exposed as a v4l2 subdev with tuner ops and can 
> interface with an SDR device. When the tuner is configured, the I2S output is 
> enabled by default. From an independent tuner device perspective, this 
> default behaviour is enough and this control may not be needed/used.
> 
> However, for the use case here, the R-Car DRIF device acts as the main SDR 
> device and the Maxim MAX2175 provides a sub-dev interface with tuner ops.
> 
> +-++-+
> | |-SCK--->|CLK  |   
> |   Master|-SS>|SYNC  DRIFn (slave)  |
> |  (MAX2175)  |-SD0--->|D0   |   
> | |-SD1--->|D1   |   
> +-++-+
> 
> The DRIF device design is such that it involves separate register writes to 
> enable Rx on each of the data line. To keep both the data lines in sync it 
> expects the master device to enable output after both the data line Rx are 
> enabled.
> 
> This level of control is exposed as a feature in the MAX2175 using this 
> control. When interfaced with DRIF this control is used to achieve the 
> desired functionality. When not interfaced with DRIF, the MAX2175 default 
> behaviour does not have to change because of DRIF and hence this I2S control 
> may be unused. Like MAX2175, DRIF is also an independent device and can 
> interface with a different third party tuner. 
> 
> Hence, this I2S enable/disable is exposed as a user control. The end user 
> application (knowing both these devices) is expected to use these controls 
> appropriately. Please let me know if I need to explain anything in further 
> detail.


The usecase is clear. That's exactly what other drivers with I2S do,
except that, on those other drivers, they pass I2S control info via
platform_data (they're not platform drivers).

With those drivers, generic applications work as-is via the standard
video, radio or sdr devnodes, without knowing about I2S.

The main difference here is that you're requiring an specialized
application for this device to work, as a generic one won't be
aware of this device-specific control, and may end by exposing this
"internal" control to the end user. That is OK for embedded usage,
but, as soon as this is used on some non-embedded usecase (with
is likely, as there are several PC consumer products using other
chips from Maxim), we'll have problems.

I guess the solution here is to make such control visible only via the
subdev interface.

Thanks,
Mauro

Re: [PATCH v2 1/2] v4l: vsp1: Add support for colorkey alpha blending

[Mauro Carvalho Chehab]

Em Sun, 7 May 2017 13:13:26 +0300
Alexandru Gheorghe <alexandru_gheor...@mentor.com> escreveu:

> The vsp2 hw supports changing of the alpha of pixels that match a color
> key, this patch adds support for this feature in order to be used by
> the rcar-du driver.
> The colorkey is interpreted different depending of the pixel format:
>   * RGB   - all color components have to match.
>   * YCbCr - only the Y component has to match.
> 
> Signed-off-by: Alexandru Gheorghe <alexandru_gheor...@mentor.com>

As most of the changes on this series are for DRM, from my side,
feel free to merge this via DRM tree.

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>

> ---
>  drivers/media/platform/vsp1/vsp1_drm.c  |  3 +++
>  drivers/media/platform/vsp1/vsp1_rpf.c  | 10 --
>  drivers/media/platform/vsp1/vsp1_rwpf.h |  3 +++
>  include/media/vsp1.h|  3 +++
>  4 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
> b/drivers/media/platform/vsp1/vsp1_drm.c
> index 3627f08..a4d0aee 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -393,6 +393,9 @@ int vsp1_du_atomic_update(struct device *dev, unsigned 
> int rpf_index,
>   else
>   rpf->format.plane_fmt[1].bytesperline = cfg->pitch;
>   rpf->alpha = cfg->alpha;
> + rpf->colorkey = cfg->colorkey;
> + rpf->colorkey_en = cfg->colorkey_en;
> + rpf->colorkey_alpha = cfg->colorkey_alpha;
>   rpf->interlaced = cfg->interlaced;
>  
>   if (soc_device_match(r8a7795es1) && rpf->interlaced) {
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c 
> b/drivers/media/platform/vsp1/vsp1_rpf.c
> index a12d6f9..91f2a9f 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -356,8 +356,14 @@ static void rpf_configure(struct vsp1_entity *entity,
>   }
>  
>   vsp1_rpf_write(rpf, dl, VI6_RPF_MSK_CTRL, 0);
> - vsp1_rpf_write(rpf, dl, VI6_RPF_CKEY_CTRL, 0);
> -
> + if (rpf->colorkey_en) {
> + vsp1_rpf_write(rpf, dl, VI6_RPF_CKEY_SET0,
> +(rpf->colorkey_alpha << 24) | rpf->colorkey);
> + vsp1_rpf_write(rpf, dl, VI6_RPF_CKEY_CTRL,
> +VI6_RPF_CKEY_CTRL_SAPE0);
> + } else {
> + vsp1_rpf_write(rpf, dl, VI6_RPF_CKEY_CTRL, 0);
> + }
>  }
>  
>  static const struct vsp1_entity_operations rpf_entity_ops = {
> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.h 
> b/drivers/media/platform/vsp1/vsp1_rwpf.h
> index fbe6aa6..2d7f4b9 100644
> --- a/drivers/media/platform/vsp1/vsp1_rwpf.h
> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.h
> @@ -51,6 +51,9 @@ struct vsp1_rwpf {
>   unsigned int brs_input;
>  
>   unsigned int alpha;
> + u32 colorkey;
> + bool colorkey_en;
> + u32 colorkey_alpha;
>  
>   u32 mult_alpha;
>   u32 outfmt;
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 97265f7..65e3934 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -32,6 +32,9 @@ struct vsp1_du_atomic_config {
>   struct v4l2_rect dst;
>   unsigned int alpha;
>   unsigned int zpos;
> + u32 colorkey;
> + u32 colorkey_alpha;
> + bool colorkey_en;
>   bool interlaced;
>  };
>  



Thanks,
Mauro

Re: [PATCH v6 3/7] media: i2c: max2175: Add MAX2175 support

[Mauro Carvalho Chehab]

Em Wed, 31 May 2017 09:44:53 +0100
Ramesh Shanmugasundaram  escreveu:

> +++ b/Documentation/media/v4l-drivers/max2175.rst
> @@ -0,0 +1,60 @@
> +Maxim Integrated MAX2175 RF to bits tuner driver
> +
> +
> +The MAX2175 driver implements the following driver-specific controls:
> +
> +``V4L2_CID_MAX2175_I2S_ENABLE``
> +---
> +Enable/Disable I2S output of the tuner.
> +
> +.. flat-table::
> +:header-rows:  0
> +:stub-columns: 0
> +:widths:   1 4
> +
> +* - ``(0)``
> +  - I2S output is disabled.
> +* - ``(1)``
> +  - I2S output is enabled.

Hmm... There are other drivers at the subsystem that use I2S
(for audio - not for SDR - but I guess the issue is similar).

On such drivers, the bridge driver controls it directly, being sure
that I2S is enabled when it is expecting some data coming from the
I2S bus.

On some drivers, there are both I2S and A/D inputs at the
bridge chipset. On such drivers, enabling/disabling I2S is
done via VIDIOC_S_INPUT (and optionally via ALSA mixer), being
transparent to the user if the stream comes from a tuner via I2S
or from a directly connected A/D input.

I don't think it is a good idea to enable it via a control, as,
if the bridge driver is expecting data via I2S, disabling it will
cause timeouts at the videobuf handling.

Thanks,
Mauro

Re: [PATCH v4 4/4] drm: rcar-du: Register a completion callback with VSP1

[Mauro Carvalho Chehab]

Em Fri,  5 May 2017 16:21:10 +0100
Kieran Bingham <kieran.bingham+rene...@ideasonboard.com> escreveu:

> Currently we process page flip events on every display interrupt,
> however this does not take into consideration the processing time needed
> by the VSP1 utilised in the pipeline.
> 
> Register a callback with the VSP driver to obtain completion events, and
> track them so that we only perform page flips when the full display
> pipeline has completed for the frame.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

For all parts of this series that touch drivers/media:

Acked-by: Mauro Carvalho Chehab <mche...@s-opensource.com>


> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  8 ++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_vsp.c  |  9 +
>  3 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 5f0664bcd12d..345eff72f581 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -378,7 +378,7 @@ static void rcar_du_crtc_update_planes(struct 
> rcar_du_crtc *rcrtc)
>   * Page Flip
>   */
>  
> -static void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc)
>  {
>   struct drm_pending_vblank_event *event;
>   struct drm_device *dev = rcrtc->crtc.dev;
> @@ -650,6 +650,7 @@ static const struct drm_crtc_funcs crtc_funcs = {
>  static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>  {
>   struct rcar_du_crtc *rcrtc = arg;
> + struct rcar_du_device *rcdu = rcrtc->group->dev;
>   irqreturn_t ret = IRQ_NONE;
>   u32 status;
>  
> @@ -658,7 +659,10 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg)
>  
>   if (status & DSSR_FRM) {
>   drm_crtc_handle_vblank(>crtc);
> - rcar_du_crtc_finish_page_flip(rcrtc);
> +
> + if (rcdu->info->gen < 3)
> + rcar_du_crtc_finish_page_flip(rcrtc);
> +
>   ret = IRQ_HANDLED;
>   }
>  
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h 
> b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 15871fae7445..b199ed5adf36 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -73,5 +73,6 @@ void rcar_du_crtc_resume(struct rcar_du_crtc *rcrtc);
>  
>  void rcar_du_crtc_route_output(struct drm_crtc *crtc,
>  enum rcar_du_output output);
> +void rcar_du_crtc_finish_page_flip(struct rcar_du_crtc *rcrtc);
>  
>  #endif /* __RCAR_DU_CRTC_H__ */
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c 
> b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> index b0ff304ce3dc..c7bb96fbfab1 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> @@ -28,6 +28,13 @@
>  #include "rcar_du_kms.h"
>  #include "rcar_du_vsp.h"
>  
> +static void rcar_du_vsp_complete(void *private)
> +{
> + struct rcar_du_crtc *crtc = private;
> +
> + rcar_du_crtc_finish_page_flip(crtc);
> +}
> +
>  void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>  {
>   const struct drm_display_mode *mode = >crtc.state->adjusted_mode;
> @@ -35,6 +42,8 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
>   struct vsp1_du_lif_config cfg = {
>   .width = mode->hdisplay,
>   .height = mode->vdisplay,
> + .callback = rcar_du_vsp_complete,
> + .callback_data = crtc,
>   };
>   struct rcar_du_plane_state state = {
>   .state = {



Thanks,
Mauro

Re: [PATCH 0/2] media: entity: add operation to help map DT node to media pad

[Mauro Carvalho Chehab]

Hi Niklas,

Em Fri, 28 Apr 2017 00:33:21 +0200
Niklas Söderlund  escreveu:

> Hi,
> 
> This small series add a new entity operation which will aid capture 
> drivers to map a port/endpoint in DT to a media graph pad. I looked 
> around and in my experience most drivers assume the DT port number is 
> the same as the media pad number.
> 
> This might be true for most devices but there are cases where this 
> mapping do not hold true. This series is implemented support to the 
> ongoing ADV748x work by Kieran Bingham, [1]. In his work he have a 
> driver which registers more then one subdevice. So when a driver finds 
> this subdevice it must be able to ask the subdevice itself which pad 
> number correspond to the DT endpoint the driver used to bind subdevice 
> in the first place.
> 
> I have updated my R-Car CSI-2 patch series to use this new function to 
> ask it's subdevice to resolve the media pad.

The problem of finding a PAD is not specific for DT-based devices.
So, what we need is a generic way to find a pad.

The non-DT based drivers usually don't implement subdev API. So, they
need to build the pipelines themselves. On such devices, there are
hundreds of different combinations of devices, and the main driver
needs to seek the hardware connected into it. Based on such
runtime knowledge, setup the pipelines.

One such example is em28xx with can use a wide range of different 
tuners, analog TV decoders and digital TV frontends.

The I2C devices like tuners and decoders have pads with different
signals:
- RF 
- digital video (encoded with ITU-R BT.656 or similar)
- audio IF signal
- chroma IF signal
- baseband signal
- luminance IF signal
- digital audio (using I2S)
- composite video
- ...

Right now, this is "solved" by using enums at include/media/v4l2-mc.h,
like this one:

enum tuner_pad_index {
TUNER_PAD_RF_INPUT,
TUNER_PAD_OUTPUT,
TUNER_PAD_AUD_OUT,
TUNER_NUM_PADS
};

That's not optimal, as even tuners that don't provide, for example,
an audio output pad need to have an unconnected TUNER_PAD_AUD_OUT
pad [1].

[1] With the current model, we're using TUNER_PAD_AUD_OUT for both
IF and digital audio - as currently - drivers don't need to distinguish
and we didn't want to have an excessive number of unconnected PADs.

So, what we really need is a way to represent a set of properties
associated with pads, and a function that would seek for a PAD that
matches a property set.

There is a proposal from Sakari to have a properties API that would
allow such kind of association (among others) and would even let
export such properties to userspace, but he never had time to send
us patches adding such functionality.

- 

IMHO, what we should do, instead of the approach you took, would be
to create a list of properties associated with each PAD (or, actually,
to any graph object, as we may want later to have properties also for
entities, interfaces and links). Something like:

enum media_property_type {
MEDIA_PROP_PAD_DT_PORT_REG, // not sure if this is the best name
MEDIA_PROP_PAD_DT_REG,  // not sure if this is the best name
MEDIA_PROP_PAD_SIGNAL_TYPE, // that's for the above example of 
identifying a pad based on the signal it carries: I2S, RF, IF, ...
...
};

struct media_properties {
enum media_property_type type;
int value;

struct list_head *list;
};

struct media_graph {
struct {
struct media_entity *entity;
struct list_head *link;
} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];

struct media_entity_enum ent_enum;
int top;

struct list_head *props; /* head for struct media_properties */
};

and a generic media_find_property() function that would allow a
driver to seek for an specific set of properties, e. g.:

int find_find_property(struct media_properties *props, struct media_graph 
*gobj);

This way, if someone would need to seek for an specific set of
properties (like on your DT case), he could use a helper function like
(untested):

find_dt_reg(int _port_reg, int _reg, struct media_graph *gobj)
{
struct media_properties port_reg, reg;

port_reg.type = MEDIA_PROP_DT_PORT_REG;
port_reg.value = _port_reg;

reg.type = MEDIA_PROP_DT_REG;
reg.value = _reg;

INIT_LIST_HEAD(_reg->list);
list_add_tail(>list, );

find_find_property(_reg, gobj);
}


Thanks,
Mauro

Re: [PATCH v3 1/8] v4l: Add metadata buffer type and format

[Mauro Carvalho Chehab]

Em Mon, 10 Apr 2017 19:58:46 +0200
Hans Verkuil <hverk...@xs4all.nl> escreveu:

> On 04/10/2017 07:23 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 28 Feb 2017 17:56:41 +0200
> > Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com> escreveu:
> >   
> >> The metadata buffer type is used to transfer metadata between userspace
> >> and kernelspace through a V4L2 buffers queue. It comes with a new
> >> metadata capture capability and format description.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>
> >> Tested-by: Guennadi Liakhovetski <guennadi.liakhovet...@intel.com>
> >> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> >> Acked-by: Hans Verkuil <hans.verk...@cisco.com>
> >> ---
> >>  Documentation/media/uapi/v4l/buffer.rst  |  3 ++
> >>  Documentation/media/uapi/v4l/dev-meta.rst| 62 
> >> 
> >>  Documentation/media/uapi/v4l/devices.rst |  1 +
> >>  Documentation/media/uapi/v4l/vidioc-querycap.rst |  3 ++
> >>  Documentation/media/videodev2.h.rst.exceptions   |  2 +
> >>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c| 19 
> >>  drivers/media/v4l2-core/v4l2-dev.c   | 16 +++---
> >>  drivers/media/v4l2-core/v4l2-ioctl.c | 34 +
> >>  drivers/media/v4l2-core/videobuf2-v4l2.c |  3 ++
> >>  include/media/v4l2-ioctl.h   | 17 +++
> >>  include/trace/events/v4l2.h  |  1 +
> >>  include/uapi/linux/videodev2.h   | 13 +
> >>  12 files changed, 168 insertions(+), 6 deletions(-)
> >>  create mode 100644 Documentation/media/uapi/v4l/dev-meta.rst
> >>
> >> diff --git a/Documentation/media/uapi/v4l/buffer.rst 
> >> b/Documentation/media/uapi/v4l/buffer.rst
> >> index 5c58db98ab7a..02834ce7fa4d 100644
> >> --- a/Documentation/media/uapi/v4l/buffer.rst
> >> +++ b/Documentation/media/uapi/v4l/buffer.rst
> >> @@ -418,6 +418,9 @@ enum v4l2_buf_type
> >>- 12
> >>- Buffer for Software Defined Radio (SDR) output stream, see
> >>:ref:`sdr`.
> >> +* - ``V4L2_BUF_TYPE_META_CAPTURE``
> >> +  - 13
> >> +  - Buffer for metadata capture, see :ref:`metadata`.
> >>  
> >>  
> >>  
> >> diff --git a/Documentation/media/uapi/v4l/dev-meta.rst 
> >> b/Documentation/media/uapi/v4l/dev-meta.rst
> >> new file mode 100644
> >> index ..b6044c54082a
> >> --- /dev/null
> >> +++ b/Documentation/media/uapi/v4l/dev-meta.rst
> >> @@ -0,0 +1,62 @@
> >> +.. -*- coding: utf-8; mode: rst -*-
> >> +
> >> +.. _metadata:
> >> +
> >> +**
> >> +Metadata Interface
> >> +**
> >> +
> >> +Metadata refers to any non-image data that supplements video frames with
> >> +additional information. This may include statistics computed over the 
> >> image
> >> +or frame capture parameters supplied by the image source. This interface 
> >> is
> >> +intended for transfer of metadata to userspace and control of that 
> >> operation.
> >> +
> >> +The metadata interface is implemented on video capture device nodes. The 
> >> device
> >> +can be dedicated to metadata or can implement both video and metadata 
> >> capture
> >> +as specified in its reported capabilities.
> >> +
> >> +.. note::
> >> +
> >> +This is an :ref:`experimental` interface and may
> >> +change in the future.  
> > 
> > While I'm ok with this comment, in practice, this comment is bogus. Once we
> > merge it, it is unlikely that we'll be able to change it.
> > 
> > That would just add a task on our TODO list that we'll need to remove this
> > comment some day.  
> 
> I'll remove this. These notes were all removed some time ago. This patch was 
> most
> likely made when these notes were still in use.
> 
> >   
> >> +
> >> +Querying Capabilities
> >> +=
> >> +
> >> +Device nodes supporting the metadata interface set the 
> >> ``V4L2_CAP_META_CAPTURE``
> >> +flag in the ``device_caps`` field of the
> >> +:c:type:`v4l2_capability` structure returned by the 
> >> :c:func:`VIDIOC_QUERYCAP`
> >> +ioctl. That flag means the device can capture metad

Re: [PATCH v3 1/8] v4l: Add metadata buffer type and format

[Mauro Carvalho Chehab]

Em Tue, 28 Feb 2017 17:56:41 +0200
Laurent Pinchart  escreveu:

> The metadata buffer type is used to transfer metadata between userspace
> and kernelspace through a V4L2 buffers queue. It comes with a new
> metadata capture capability and format description.
> 
> Signed-off-by: Laurent Pinchart 
> Tested-by: Guennadi Liakhovetski 
> Acked-by: Sakari Ailus 
> Acked-by: Hans Verkuil 
> ---
>  Documentation/media/uapi/v4l/buffer.rst  |  3 ++
>  Documentation/media/uapi/v4l/dev-meta.rst| 62 
> 
>  Documentation/media/uapi/v4l/devices.rst |  1 +
>  Documentation/media/uapi/v4l/vidioc-querycap.rst |  3 ++
>  Documentation/media/videodev2.h.rst.exceptions   |  2 +
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c| 19 
>  drivers/media/v4l2-core/v4l2-dev.c   | 16 +++---
>  drivers/media/v4l2-core/v4l2-ioctl.c | 34 +
>  drivers/media/v4l2-core/videobuf2-v4l2.c |  3 ++
>  include/media/v4l2-ioctl.h   | 17 +++
>  include/trace/events/v4l2.h  |  1 +
>  include/uapi/linux/videodev2.h   | 13 +
>  12 files changed, 168 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/media/uapi/v4l/dev-meta.rst
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst 
> b/Documentation/media/uapi/v4l/buffer.rst
> index 5c58db98ab7a..02834ce7fa4d 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -418,6 +418,9 @@ enum v4l2_buf_type
>- 12
>- Buffer for Software Defined Radio (SDR) output stream, see
>   :ref:`sdr`.
> +* - ``V4L2_BUF_TYPE_META_CAPTURE``
> +  - 13
> +  - Buffer for metadata capture, see :ref:`metadata`.
>  
>  
>  
> diff --git a/Documentation/media/uapi/v4l/dev-meta.rst 
> b/Documentation/media/uapi/v4l/dev-meta.rst
> new file mode 100644
> index ..b6044c54082a
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/dev-meta.rst
> @@ -0,0 +1,62 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _metadata:
> +
> +**
> +Metadata Interface
> +**
> +
> +Metadata refers to any non-image data that supplements video frames with
> +additional information. This may include statistics computed over the image
> +or frame capture parameters supplied by the image source. This interface is
> +intended for transfer of metadata to userspace and control of that operation.
> +
> +The metadata interface is implemented on video capture device nodes. The 
> device
> +can be dedicated to metadata or can implement both video and metadata capture
> +as specified in its reported capabilities.
> +
> +.. note::
> +
> +This is an :ref:`experimental` interface and may
> +change in the future.

While I'm ok with this comment, in practice, this comment is bogus. Once we
merge it, it is unlikely that we'll be able to change it.

That would just add a task on our TODO list that we'll need to remove this
comment some day.

> +
> +Querying Capabilities
> +=
> +
> +Device nodes supporting the metadata interface set the 
> ``V4L2_CAP_META_CAPTURE``
> +flag in the ``device_caps`` field of the
> +:c:type:`v4l2_capability` structure returned by the :c:func:`VIDIOC_QUERYCAP`
> +ioctl. That flag means the device can capture metadata to memory.
> +
> +At least one of the read/write or streaming I/O methods must be supported.
> +
> +
> +Data Format Negotiation
> +===
> +
> +The metadata device uses the :ref:`format` ioctls to select the capture 
> format.
> +The metadata buffer content format is bound to that selected format. In 
> addition
> +to the basic :ref:`format` ioctls, the :c:func:`VIDIOC_ENUM_FMT` ioctl must 
> be
> +supported as well.
> +
> +To use the :ref:`format` ioctls applications set the ``type`` field of the
> +:c:type:`v4l2_format` structure to ``V4L2_BUF_TYPE_META_CAPTURE`` and use the
> +:c:type:`v4l2_meta_format` ``meta`` member of the ``fmt`` union as needed per
> +the desired operation. Both drivers and applications must set the remainder 
> of
> +the :c:type:`v4l2_format` structure to 0.
> +
> +.. _v4l2-meta-format:

Better to add an space after the label. My experience with random versions
of Sphinx is that it doesn't like to have different types of paragraph
without at least one blank line between them.

> +.. flat-table:: struct v4l2_meta_format
> +:header-rows:  0
> +:stub-columns: 0
> +:widths:   1 1 2
> +
> +* - __u32
> +  - ``dataformat``
> +  - The data format, set by the application. This is a little endian
> +:ref:`four character code `. V4L2 defines metadata 
> formats
> +in :ref:`meta-formats`.
> +* - __u32
> +  - ``buffersize``
> +  - Maximum 

Re: [PATCH v2.3] v4l: Clearly document interactions between formats, controls and buffers

[Mauro Carvalho Chehab]

Em Mon,  6 Mar 2017 16:14:41 +0200
Laurent Pinchart  escreveu:

> V4L2 exposes parameters that influence buffers sizes through the format
> ioctls (VIDIOC_G_FMT, VIDIOC_TRY_FMT, VIDIOC_S_FMT, and possibly
> VIDIOC_G_SELECTION and VIDIOC_S_SELECTION). Other parameters not part of
> the format structure may also influence buffer sizes or buffer layout in
> general. One existing such parameter is rotation, which is implemented
> by the V4L2_CID_ROTATE control and thus exposed through the V4L2 control
> ioctls.
> 
> The interaction between those parameters and buffers is currently only
> partially specified by the V4L2 API. In particular interactions between
> controls and buffers isn't specified at all. The behaviour of the
> VIDIOC_S_FMT and VIDIOC_S_SELECTION ioctls when buffers are allocated is
> also not fully specified.
> 
> This patch clearly defines and documents the interactions between
> formats, selections, controls and buffers.
> 
> The preparatory discussions for the documentation change considered
> completely disallowing controls that change the buffer size or layout,
> in favour of extending the format API with a new ioctl that would bundle
> those controls with format information. The idea has been rejected, as
> this would essentially be a restricted version of the upcoming request
> API that wouldn't bring any additional value.
> 
> Another option we have considered was to mandate the use of the request
> API to modify controls that influence buffer size or layout. This has
> also been rejected on the grounds that requiring the request API to
> change rotation even when streaming is stopped would significantly
> complicate implementation of drivers and usage of the V4L2 API for
> applications.
> 
> Applications will however be required to use the upcoming request API to
> change at runtime formats or controls that influence the buffer size or
> layout, because of the need to synchronize buffers with the formats and
> controls. Otherwise there would be no way to interpret the content of a
> buffer correctly.
> 
> Signed-off-by: Laurent Pinchart 
> Acked-by: Sakari Ailus 
> ---
> Changes since v2.2:
> 
> - Describe the simple option first
> - Fix error codes
> 
> Changes since v2.1:
> 
> - Fixed small issues in commit message
> - Simplified wording of one sentence in the documentation
> 
> Changes since v2:
> 
> - Document the interaction with ioctls that can affect formats
>   (VIDIOC_S_SELECTION, VIDIOC_S_INPUT, VIDIOC_S_OUTPUT, VIDIOC_S_STD and
>   VIDIOC_S_DV_TIMINGS)
> - Clarify the format/control change order
> ---
>  Documentation/media/uapi/v4l/buffer.rst | 110 
> 
>  1 file changed, 110 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst 
> b/Documentation/media/uapi/v4l/buffer.rst
> index ac58966ccb9b..d1e0d55dc219 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst

...

> +.. note::
> +
> +   The API doesn't mandate the above order for control (3.) and format (4.)
> +   changes. Format and controls can be set in a different order, or even
> +   interleaved, depending on the device and use case. For instance some
> +   controls might behave differently for different pixel formats, in which
> +   case the format might need to be set first.
> +
> +When reallocation is required, any attempt to modify format or controls that
> +influences the buffer size while buffers are allocated shall cause the format
> +or control set ioctl to return the ``EBUSY`` error. Any attempt to queue a
> +buffer too small for the current format or controls shall cause the
> +:c:func:`VIDIOC_QBUF` ioctl to return a ``EINVAL`` error.

This can be problematic. As I just implemented support for controls
this weekend at Zbar, I'm now talking as an userspace app developer's
hat.

The real problem here is that applications must be aware of what
controls change the buffer layout. Blindly changing controls without
such check would cause the stream to fail with -EINVAL errors at
QBUF.

So, applications will need to to have a "black list" of controls that may
influence the buffer size  (like V4L2_CID_ROTATE), in order to know
if, for such particular control, the stream should be stopped, in
order to reallocate buffers.

If such "black list" is not updated as newer controls are added, the
final result is that, if the user changes such control, the 
application will receive EINVAL, causing it to fail streaming.

Instead of that, the best is to add control flag to be returned via
VIDIOC_QUERY_CTRL/VIDIOC_QUERY_EXT_CTRL indicating when a control modifies 
the buffer layout, e. g., something like:

#define V4L2_CTRL_FLAG_MODIFY_BUF_LAYOUT0x0400

Such flag shall be set for V4L2_CID_ROTATE (and other controls) if,
for a particular driver, the buffer layout is modified.

This way, userspace can 

Re: [PATCH] [media] vsp1: fix CodingStyle violations on multi-line comments

[Mauro Carvalho Chehab]

Em Mon, 19 Sep 2016 21:35:36 +0300
Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> Thank you for the patch.
> 
> 
> On Monday 19 Sep 2016 15:26:19 Mauro Carvalho Chehab wrote:
> > Several multi-line comments added at the vsp1 patch series
> > violate the Kernel CodingStyle. Fix them.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>  
> 
> I prefer the current style but that seems to be a hopeless battle :-) I have 
> a 
> small comment, please see below.
> 
> > ---
> >  drivers/media/platform/vsp1/vsp1_bru.c|  3 ++-
> >  drivers/media/platform/vsp1/vsp1_clu.c|  3 ++-
> >  drivers/media/platform/vsp1/vsp1_dl.c | 21 ++---
> >  drivers/media/platform/vsp1/vsp1_drm.c|  3 ++-
> >  drivers/media/platform/vsp1/vsp1_entity.h |  2 +-
> >  drivers/media/platform/vsp1/vsp1_pipe.c   |  2 +-
> >  drivers/media/platform/vsp1/vsp1_rpf.c|  9 ++---
> >  drivers/media/platform/vsp1/vsp1_rwpf.c   |  6 --
> >  drivers/media/platform/vsp1/vsp1_video.c  | 20 +---
> >  drivers/media/platform/vsp1/vsp1_wpf.c|  9 ++---
> >  10 files changed, 51 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_bru.c
> > b/drivers/media/platform/vsp1/vsp1_bru.c index 2f5788c1a5be..ee8355c28f94
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_bru.c
> > +++ b/drivers/media/platform/vsp1/vsp1_bru.c
> > @@ -242,7 +242,8 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
> > goto done;
> > }
> > 
> > -   /* The compose rectangle top left corner must be inside the output
> > +   /*
> > +* The compose rectangle top left corner must be inside the output
> >  * frame.
> >  */
> > format = vsp1_entity_get_pad_format(>entity, config,
> > diff --git a/drivers/media/platform/vsp1/vsp1_clu.c
> > b/drivers/media/platform/vsp1/vsp1_clu.c index f052abd05166..f2fb26e5ab4e
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_clu.c
> > +++ b/drivers/media/platform/vsp1/vsp1_clu.c
> > @@ -224,7 +224,8 @@ static void clu_configure(struct vsp1_entity *entity,
> > 
> > switch (params) {
> > case VSP1_ENTITY_PARAMS_INIT: {
> > -   /* The format can't be changed during streaming, only verify   
> it
> > +   /*
> > +* The format can't be changed during streaming, only verify   
> it
> >  * at setup time and store the information internally for   
> future
> >  * runtime configuration calls.
> >  */
> > diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> > b/drivers/media/platform/vsp1/vsp1_dl.c index 0af3e8fdc714..ad545aff4e35
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_dl.c
> > +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> > @@ -296,7 +296,8 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct
> > vsp1_dl_manager *dlm) dl = list_first_entry(>free, struct
> > vsp1_dl_list, list);
> > list_del(>list);
> > 
> > -   /* The display list chain must be initialised to ensure every
> > +   /*
> > +* The display list chain must be initialised to ensure every
> >  * display list can assert list_empty() if it is not in a   
> chain.
> >  */
> > INIT_LIST_HEAD(>chain);
> > @@ -315,7 +316,8 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
> > if (!dl)
> > return;
> > 
> > -   /* Release any linked display-lists which were chained for a single
> > +   /*
> > +* Release any linked display-lists which were chained for a single
> >  * hardware operation.
> >  */
> > if (dl->has_chain) {
> > @@ -325,7 +327,8 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
> > 
> > dl->has_chain = false;
> > 
> > -   /* We can't free fragments here as DMA memory can only be freed in
> > +   /*
> > +* We can't free fragments here as DMA memory can only be freed in
> >  * interruptible context. Move all fragments to the display list
> >  * manager's list of fragments to be freed, they will be
> >  * garbage-collected by the work queue.
> > @@ -437,7 +440,8 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list
> > *dl, bool is_last) struct vsp1_dl_body *dlb;
> > unsigned int num_lists = 0;
> > 
> > -   /* Fill the header with the display list bodies addresses and sizes.   
> The
> > + 

Re: [PATCH 06/13] v4l: vsp1: Disable cropping on WPF sink pad

[Mauro Carvalho Chehab]

Em Mon, 19 Sep 2016 21:33:13 +0300
Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday 19 Sep 2016 15:26:15 Mauro Carvalho Chehab wrote:
> > Em Mon, 19 Sep 2016 20:59:56 +0300 Laurent Pinchart escreveu:  
> > > On Monday 19 Sep 2016 14:55:43 Mauro Carvalho Chehab wrote:  
> > >> Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart escreveu:  
> > >>> Cropping on the WPF sink pad restricts the left and top coordinates to
> > >>> 0-255. The same result can be obtained by cropping on the RPF without
> > >>> any such restriction, this feature isn't useful. Disable it.
> > >>> 
> > >>> Signed-off-by: Laurent Pinchart
> > >>> <laurent.pinchart+rene...@ideasonboard.com>
> > >>> ---
> > >>> 
> > >>>  drivers/media/platform/vsp1/vsp1_rwpf.c | 37 
> > >>>  drivers/media/platform/vsp1/vsp1_wpf.c  | 18 +++-
> > >>>  2 files changed, 26 insertions(+), 29 deletions(-)
> > >>> 
> > >>> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c
> > >>> b/drivers/media/platform/vsp1/vsp1_rwpf.c index
> > >>> 8cb87e96b78b..a3ace8df7f4d 100644
> > >>> --- a/drivers/media/platform/vsp1/vsp1_rwpf.c
> > >>> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c  
> 
> [snip]
> 
> > >>> @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct
> > >>> v4l2_subdev *subdev,
> > >>> struct v4l2_mbus_framefmt *format;
> > >>> int ret = 0;
> > >>> 
> > >>> -   /* Cropping is implemented on the sink pad. */
> > >>> -   if (sel->pad != RWPF_PAD_SINK)
> > >>> +   /* Cropping is only supported on the RPF and is implemented on
> > >>> the sink
> > >>> +* pad.
> > >>> +*/  
> > >> 
> > >> Please read CodingStyle and run checkpatch before sending stuff
> > >> upstream.
> > >> 
> > >> This violates the CodingStyle: it should be, instead:
> > >>  /*
> > >>   * foo
> > >>   * bar
> > >>   */  
> > > 
> > > But it's consistent with the coding style of this driver. I'm OK fixing
> > > it, but it should be done globally in that case.  
> > 
> > There are inconsistencies inside the driver too on multi-line
> > comments even without fixing the ones introduced on this series,
> > as, on several places, multi-line comments are correct:
> > 
> > drivers/media/platform/vsp1/vsp1_bru.c:/*
> > drivers/media/platform/vsp1/vsp1_bru.c- * The BRU can't perform format
> > conversion, all sink and source formats must be
> > drivers/media/platform/vsp1/vsp1_bru.c- * identical. We pick the format on
> > the first sink pad (pad 0) and propagate it
> > drivers/media/platform/vsp1/vsp1_bru.c- * to all other pads.
> > drivers/media/platform/vsp1/vsp1_bru.c- */
> > 
> > drivers/media/platform/vsp1/vsp1_dl.c:/*
> > drivers/media/platform/vsp1/vsp1_dl.c- * Initialize a display list body
> > object and allocate DMA memory for the body
> > drivers/media/platform/vsp1/vsp1_dl.c- * data. The display list body object
> > is expected to have been initialized to
> > drivers/media/platform/vsp1/vsp1_dl.c- * 0 when allocated.
> > drivers/media/platform/vsp1/vsp1_dl.c- */
> > 
> > ...
> > 
> > I'll address the ones only the CodingStyle violation introduced by this
> > series. I'll leave for the vsp1 maintainers/developers.  
> 
> OK, I'll address that.
> 
> > Btw, there are several kernel-doc tags that use just:
> > /*
> >  ...
> >  */
> > 
> > instead of:
> > 
> > /**
> >  ...
> >  */
> > 
> > I suggest you to add the files/headers with kernel-doc markups on
> > a Documentation/media/v4l-drivers/vsp1.rst file, to be created.
> > 
> > This way, you can validate that such documentation is correct,
> > and produce an auto-generated documentation for this driver.  
> 
> I've thought about it, but I don't think those comments should become part of 
> the kernel documentation. They're really about driver internals, and meant 
> for 
> the driver developers. In particular only a subset of the driver is 
> documented 
> that way, when I've considered that the code or structures were complex 
> enough 
> to need proper documentation. A generated doc would then be quite in

[PATCH] [media] vsp1: fix CodingStyle violations on multi-line comments

[Mauro Carvalho Chehab]

Several multi-line comments added at the vsp1 patch series
violate the Kernel CodingStyle. Fix them.

Signed-off-by: Mauro Carvalho Chehab <mche...@s-opensource.com>
---
 drivers/media/platform/vsp1/vsp1_bru.c|  3 ++-
 drivers/media/platform/vsp1/vsp1_clu.c|  3 ++-
 drivers/media/platform/vsp1/vsp1_dl.c | 21 ++---
 drivers/media/platform/vsp1/vsp1_drm.c|  3 ++-
 drivers/media/platform/vsp1/vsp1_entity.h |  2 +-
 drivers/media/platform/vsp1/vsp1_pipe.c   |  2 +-
 drivers/media/platform/vsp1/vsp1_rpf.c|  9 ++---
 drivers/media/platform/vsp1/vsp1_rwpf.c   |  6 --
 drivers/media/platform/vsp1/vsp1_video.c  | 20 +---
 drivers/media/platform/vsp1/vsp1_wpf.c|  9 ++---
 10 files changed, 51 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_bru.c 
b/drivers/media/platform/vsp1/vsp1_bru.c
index 2f5788c1a5be..ee8355c28f94 100644
--- a/drivers/media/platform/vsp1/vsp1_bru.c
+++ b/drivers/media/platform/vsp1/vsp1_bru.c
@@ -242,7 +242,8 @@ static int bru_set_selection(struct v4l2_subdev *subdev,
goto done;
}
 
-   /* The compose rectangle top left corner must be inside the output
+   /*
+* The compose rectangle top left corner must be inside the output
 * frame.
 */
format = vsp1_entity_get_pad_format(>entity, config,
diff --git a/drivers/media/platform/vsp1/vsp1_clu.c 
b/drivers/media/platform/vsp1/vsp1_clu.c
index f052abd05166..f2fb26e5ab4e 100644
--- a/drivers/media/platform/vsp1/vsp1_clu.c
+++ b/drivers/media/platform/vsp1/vsp1_clu.c
@@ -224,7 +224,8 @@ static void clu_configure(struct vsp1_entity *entity,
 
switch (params) {
case VSP1_ENTITY_PARAMS_INIT: {
-   /* The format can't be changed during streaming, only verify it
+   /*
+* The format can't be changed during streaming, only verify it
 * at setup time and store the information internally for future
 * runtime configuration calls.
 */
diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
b/drivers/media/platform/vsp1/vsp1_dl.c
index 0af3e8fdc714..ad545aff4e35 100644
--- a/drivers/media/platform/vsp1/vsp1_dl.c
+++ b/drivers/media/platform/vsp1/vsp1_dl.c
@@ -296,7 +296,8 @@ struct vsp1_dl_list *vsp1_dl_list_get(struct 
vsp1_dl_manager *dlm)
dl = list_first_entry(>free, struct vsp1_dl_list, list);
list_del(>list);
 
-   /* The display list chain must be initialised to ensure every
+   /*
+* The display list chain must be initialised to ensure every
 * display list can assert list_empty() if it is not in a chain.
 */
INIT_LIST_HEAD(>chain);
@@ -315,7 +316,8 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
if (!dl)
return;
 
-   /* Release any linked display-lists which were chained for a single
+   /*
+* Release any linked display-lists which were chained for a single
 * hardware operation.
 */
if (dl->has_chain) {
@@ -325,7 +327,8 @@ static void __vsp1_dl_list_put(struct vsp1_dl_list *dl)
 
dl->has_chain = false;
 
-   /* We can't free fragments here as DMA memory can only be freed in
+   /*
+* We can't free fragments here as DMA memory can only be freed in
 * interruptible context. Move all fragments to the display list
 * manager's list of fragments to be freed, they will be
 * garbage-collected by the work queue.
@@ -437,7 +440,8 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list 
*dl, bool is_last)
struct vsp1_dl_body *dlb;
unsigned int num_lists = 0;
 
-   /* Fill the header with the display list bodies addresses and sizes. The
+   /*
+* Fill the header with the display list bodies addresses and sizes. The
 * address of the first body has already been filled when the display
 * list was allocated.
 */
@@ -456,7 +460,8 @@ static void vsp1_dl_list_fill_header(struct vsp1_dl_list 
*dl, bool is_last)
 
dl->header->num_lists = num_lists;
 
-   /* If this display list's chain is not empty, we are on a list, where
+   /*
+* If this display list's chain is not empty, we are on a list, where
 * the next item in the list is the display list entity which should be
 * automatically queued by the hardware.
 */
@@ -482,7 +487,8 @@ void vsp1_dl_list_commit(struct vsp1_dl_list *dl)
if (dl->dlm->mode == VSP1_DL_MODE_HEADER) {
struct vsp1_dl_list *dl_child;
 
-   /* In header mode the caller guarantees that the hardware is
+   /*
+* In header mode the caller guarantees that the hardware is
 * idle at this point.
 */
 
@@ -495,7 +501,

Re: [PATCH 06/13] v4l: vsp1: Disable cropping on WPF sink pad

[Mauro Carvalho Chehab]

Em Mon, 19 Sep 2016 20:59:56 +0300
Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Monday 19 Sep 2016 14:55:43 Mauro Carvalho Chehab wrote:
> > Em Wed, 14 Sep 2016 02:16:59 +0300 Laurent Pinchart escreveu:  
> > > Cropping on the WPF sink pad restricts the left and top coordinates to
> > > 0-255. The same result can be obtained by cropping on the RPF without
> > > any such restriction, this feature isn't useful. Disable it.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+rene...@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/platform/vsp1/vsp1_rwpf.c | 37
> > >  + drivers/media/platform/vsp1/vsp1_wpf.c
> > >   | 18 +++-
> > >  2 files changed, 26 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c
> > > b/drivers/media/platform/vsp1/vsp1_rwpf.c index
> > > 8cb87e96b78b..a3ace8df7f4d 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_rwpf.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
> > > @@ -66,7 +66,6 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev  
> > > *subdev,>   
> > >   struct vsp1_rwpf *rwpf = to_rwpf(subdev);
> > >   struct v4l2_subdev_pad_config *config;
> > >   struct v4l2_mbus_framefmt *format;
> > > 
> > > - struct v4l2_rect *crop;
> > > 
> > >   int ret = 0;
> > >   
> > >   mutex_lock(>entity.lock);
> > > 
> > > @@ -103,12 +102,16 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev  
> > > *subdev,>   
> > >   fmt->format = *format;
> > > 
> > > - /* Update the sink crop rectangle. */
> > > - crop = vsp1_rwpf_get_crop(rwpf, config);
> > > - crop->left = 0;
> > > - crop->top = 0;
> > > - crop->width = fmt->format.width;
> > > - crop->height = fmt->format.height;
> > > + if (rwpf->entity.type == VSP1_ENTITY_RPF) {
> > > + struct v4l2_rect *crop;
> > > +
> > > + /* Update the sink crop rectangle. */
> > > + crop = vsp1_rwpf_get_crop(rwpf, config);
> > > + crop->left = 0;
> > > + crop->top = 0;
> > > + crop->width = fmt->format.width;
> > > + crop->height = fmt->format.height;
> > > + }
> > > 
> > >   /* Propagate the format to the source pad. */
> > >   format = vsp1_entity_get_pad_format(>entity, config,
> > > 
> > > @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct 
> > > v4l2_subdev  
> > > *subdev,>   
> > >   struct v4l2_mbus_framefmt *format;
> > >   int ret = 0;
> > > 
> > > - /* Cropping is implemented on the sink pad. */
> > > - if (sel->pad != RWPF_PAD_SINK)
> > > + /* Cropping is only supported on the RPF and is implemented on the   
> sink
> > > +  * pad.
> > > +  */  
> > 
> > Please read CodingStyle and run checkpatch before sending stuff upstream.
> > 
> > This violates the CodingStyle: it should be, instead:
> > /*
> >  * foo
> >  * bar
> >  */  
> 
> But it's consistent with the coding style of this driver. I'm OK fixing it, 
> but it should be done globally in that case.

There are inconsistencies inside the driver too on multi-line
comments even without fixing the ones introduced on this series,
as, on several places, multi-line comments are correct:

drivers/media/platform/vsp1/vsp1_bru.c:/*
drivers/media/platform/vsp1/vsp1_bru.c- * The BRU can't perform format 
conversion, all sink and source formats must be
drivers/media/platform/vsp1/vsp1_bru.c- * identical. We pick the format on the 
first sink pad (pad 0) and propagate it
drivers/media/platform/vsp1/vsp1_bru.c- * to all other pads.
drivers/media/platform/vsp1/vsp1_bru.c- */

drivers/media/platform/vsp1/vsp1_dl.c:/*
drivers/media/platform/vsp1/vsp1_dl.c- * Initialize a display list body object 
and allocate DMA memory for the body
drivers/media/platform/vsp1/vsp1_dl.c- * data. The display list body object is 
expected to have been initialized to
drivers/media/platform/vsp1/vsp1_dl.c- * 0 when allocated.
drivers/media/platform/vsp1/vsp1_dl.c- */

...

I'll address the ones only the CodingStyle violation introduced by this
series. I'll leave for the vsp1 maintainers/developers.

Btw, there are several kernel-doc tags that use just:
/*
 ...
 */

instead of:

/**
 ...
 */

I suggest you to add the files/headers with kernel-doc markups on
a Documentation/media/v4l-drivers/vsp1.rst file, to be created.

This way, you can validate that such documentation is correct,
and produce an auto-generated documentation for this driver.

Regards,
Mauro

Re: [PATCH 06/13] v4l: vsp1: Disable cropping on WPF sink pad

[Mauro Carvalho Chehab]

Em Wed, 14 Sep 2016 02:16:59 +0300
Laurent Pinchart  escreveu:

> Cropping on the WPF sink pad restricts the left and top coordinates to
> 0-255. The same result can be obtained by cropping on the RPF without
> any such restriction, this feature isn't useful. Disable it.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/media/platform/vsp1/vsp1_rwpf.c | 37 
> +
>  drivers/media/platform/vsp1/vsp1_wpf.c  | 18 +++-
>  2 files changed, 26 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_rwpf.c 
> b/drivers/media/platform/vsp1/vsp1_rwpf.c
> index 8cb87e96b78b..a3ace8df7f4d 100644
> --- a/drivers/media/platform/vsp1/vsp1_rwpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rwpf.c
> @@ -66,7 +66,6 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev *subdev,
>   struct vsp1_rwpf *rwpf = to_rwpf(subdev);
>   struct v4l2_subdev_pad_config *config;
>   struct v4l2_mbus_framefmt *format;
> - struct v4l2_rect *crop;
>   int ret = 0;
>  
>   mutex_lock(>entity.lock);
> @@ -103,12 +102,16 @@ static int vsp1_rwpf_set_format(struct v4l2_subdev 
> *subdev,
>  
>   fmt->format = *format;
>  
> - /* Update the sink crop rectangle. */
> - crop = vsp1_rwpf_get_crop(rwpf, config);
> - crop->left = 0;
> - crop->top = 0;
> - crop->width = fmt->format.width;
> - crop->height = fmt->format.height;
> + if (rwpf->entity.type == VSP1_ENTITY_RPF) {
> + struct v4l2_rect *crop;
> +
> + /* Update the sink crop rectangle. */
> + crop = vsp1_rwpf_get_crop(rwpf, config);
> + crop->left = 0;
> + crop->top = 0;
> + crop->width = fmt->format.width;
> + crop->height = fmt->format.height;
> + }
>  
>   /* Propagate the format to the source pad. */
>   format = vsp1_entity_get_pad_format(>entity, config,
> @@ -129,8 +132,10 @@ static int vsp1_rwpf_get_selection(struct v4l2_subdev 
> *subdev,
>   struct v4l2_mbus_framefmt *format;
>   int ret = 0;
>  
> - /* Cropping is implemented on the sink pad. */
> - if (sel->pad != RWPF_PAD_SINK)
> + /* Cropping is only supported on the RPF and is implemented on the sink
> +  * pad.
> +  */

Please read CodingStyle and run checkpatch before sending stuff upstream.

This violates the CodingStyle: it should be, instead:
/*
 * foo
 * bar
 */

This time, I'll fix it, but next time I might not have enough time, and
need to reject the patch series.

Thanks,
Mauro

Re: [PATCH v2 2/4] v4l: Define a pixel format for the R-Car VSP1 1-D histogram engine

[Mauro Carvalho Chehab]

Em Tue, 06 Sep 2016 21:11:10 +0300
Laurent Pinchart <laurent.pinch...@ideasonboard.com> escreveu:

> Hi Mauro,
> 
> On Tuesday 06 Sep 2016 14:06:51 Mauro Carvalho Chehab wrote:
> > Em Wed, 17 Aug 2016 15:20:28 +0300 Laurent Pinchart escreveu:  
> > > The format is used on the R-Car VSP1 video queues that carry
> > > 1-D histogram statistics data.
> > > 
> > > Signed-off-by: Laurent Pinchart
> > > <laurent.pinchart+rene...@ideasonboard.com>
> > > Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> > > ---
> > > Changes since v1:
> > > 
> > > - Rebased on top of the DocBook to reST conversion
> > > 
> > >  Documentation/media/uapi/v4l/meta-formats.rst  |  15 ++
> > >  .../media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst| 170 
> > >  Documentation/media/uapi/v4l/pixfmt.rst|   1 +
> > >  drivers/media/v4l2-core/v4l2-ioctl.c   |   1 +
> > >  include/uapi/linux/videodev2.h |   3 +
> > >  5 files changed, 190 insertions(+)
> > >  create mode 100644 Documentation/media/uapi/v4l/meta-formats.rst
> > >  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> > > 
> > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> > > b/Documentation/media/uapi/v4l/meta-formats.rst new file mode 100644
> > > index ..05ab91e12f10
> > > --- /dev/null
> > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> > > @@ -0,0 +1,15 @@
> > > +.. -*- coding: utf-8; mode: rst -*-
> > > +
> > > +.. _meta-formats:
> > > +
> > > +
> > > +Metadata Formats
> > > +
> > > +
> > > +These formats are used for the :ref:`metadata` interface only.
> > > +
> > > +
> > > +.. toctree::
> > > +:maxdepth: 1
> > > +
> > > +pixfmt-meta-vsp1-hgo
> > > diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> > > b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst new file mode
> > > 100644
> > > index ..e935e4525b10
> > > --- /dev/null
> > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> > > @@ -0,0 +1,170 @@
> > > +.. -*- coding: utf-8; mode: rst -*-
> > > +
> > > +.. _v4l2-meta-fmt-vsp1-hgo:
> > > +
> > > +***
> > > +V4L2_META_FMT_VSP1_HGO ('VSPH')
> > > +***
> > > +
> > > +*man V4L2_META_FMT_VSP1_HGO(2)*  
> > 
> > Just remove it. This is some trash that came from the conversions.
> > I have a set of patches removing it on the existing man pages.  
> 
> Sure, I will do.

Thanks!

> > > +
> > > +Renesas R-Car VSP1 1-D Histogram Data
> > > +
> > > +
> > > +Description
> > > +===
> > > +
> > > +This format describes histogram data generated by the Renesas R-Car VSP1
> > > 1-D +Histogram (HGO) engine.
> > > +
> > > +The VSP1 HGO is a histogram computation engine that can operate on RGB,
> > > YCrCb +or HSV data. It operates on a possibly cropped and subsampled
> > > input image and +computes the minimum, maximum and sum of all pixels as
> > > well as per-channel +histograms.
> > > +
> > > +The HGO can compute histograms independently per channel, on the maximum
> > > of the +three channels (RGB data only) or on the Y channel only (YCbCr
> > > only). It can +additionally output the histogram with 64 or 256 bins,
> > > resulting in four +possible modes of operation.
> > > +
> > > +- In *64 bins normal mode*, the HGO operates on the three channels
> > > independently +  to compute three 64-bins histograms. RGB, YCbCr and HSV
> > > image formats are +  supported.
> > > +- In *64 bins maximum mode*, the HGO operates on the maximum of the (R,
> > > G, B) +  channels to compute a single 64-bins histogram. Only the RGB
> > > image format is +  supported.
> > > +- In *256 bins normal mode*, the HGO operates on the Y channel to compute
> > > a +  single 256-bins histogram. Only the YCbCr image format is supported.
> > > +- In *256 bins maximum mode*, the HGO operates on the maximum of the (R,
> > > G, B) +  channels to compute a single 256-bins histogram. Only the RGB
> > > image format is +  supported.  
> > 
> > As they all share the same FOURCC format, according with the documentation,
> > how the user is supposed to switch between those modes? Or is it depend on
> > the video format? In any case, please add some explanation, and cross-refs
> > if needed.  
> 
> The modes are selected using controls, they don't depend on the video format. 
> Do you think it would make sense to cross-reference between formats and 
> controls ?

It probably makes a way more sense to use enum_fmt/s_fmt/g_fmt ioctls and
add one different fourcc per format.

Using a control instead of *fmt to select the format seems really weird,
as it is not what it is expected for the fourcc formats.


Thanks,
Mauro

Re: [PATCH v2 2/4] v4l: Define a pixel format for the R-Car VSP1 1-D histogram engine

[Mauro Carvalho Chehab]

Hi Laurent,

Em Wed, 17 Aug 2016 15:20:28 +0300
Laurent Pinchart  escreveu:

> The format is used on the R-Car VSP1 video queues that carry
> 1-D histogram statistics data.
> 
> Signed-off-by: Laurent Pinchart 
> Acked-by: Sakari Ailus 
> ---
> Changes since v1:
> 
> - Rebased on top of the DocBook to reST conversion
> 
>  Documentation/media/uapi/v4l/meta-formats.rst  |  15 ++
>  .../media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst| 170 
> +
>  Documentation/media/uapi/v4l/pixfmt.rst|   1 +
>  drivers/media/v4l2-core/v4l2-ioctl.c   |   1 +
>  include/uapi/linux/videodev2.h |   3 +
>  5 files changed, 190 insertions(+)
>  create mode 100644 Documentation/media/uapi/v4l/meta-formats.rst
>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> 
> diff --git a/Documentation/media/uapi/v4l/meta-formats.rst 
> b/Documentation/media/uapi/v4l/meta-formats.rst
> new file mode 100644
> index ..05ab91e12f10
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> @@ -0,0 +1,15 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _meta-formats:
> +
> +
> +Metadata Formats
> +
> +
> +These formats are used for the :ref:`metadata` interface only.
> +
> +
> +.. toctree::
> +:maxdepth: 1
> +
> +pixfmt-meta-vsp1-hgo
> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst 
> b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> new file mode 100644
> index ..e935e4525b10
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgo.rst
> @@ -0,0 +1,170 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-vsp1-hgo:
> +
> +***
> +V4L2_META_FMT_VSP1_HGO ('VSPH')
> +***
> +
> +*man V4L2_META_FMT_VSP1_HGO(2)*

Just remove it. This is some trash that came from the conversions.
I have a set of patches removing it on the existing man pages.

> +
> +Renesas R-Car VSP1 1-D Histogram Data
> +
> +
> +Description
> +===
> +
> +This format describes histogram data generated by the Renesas R-Car VSP1 1-D
> +Histogram (HGO) engine.
> +
> +The VSP1 HGO is a histogram computation engine that can operate on RGB, YCrCb
> +or HSV data. It operates on a possibly cropped and subsampled input image and
> +computes the minimum, maximum and sum of all pixels as well as per-channel
> +histograms.
> +
> +The HGO can compute histograms independently per channel, on the maximum of 
> the
> +three channels (RGB data only) or on the Y channel only (YCbCr only). It can
> +additionally output the histogram with 64 or 256 bins, resulting in four
> +possible modes of operation.
> +
> +- In *64 bins normal mode*, the HGO operates on the three channels 
> independently
> +  to compute three 64-bins histograms. RGB, YCbCr and HSV image formats are
> +  supported.
> +- In *64 bins maximum mode*, the HGO operates on the maximum of the (R, G, B)
> +  channels to compute a single 64-bins histogram. Only the RGB image format 
> is
> +  supported.
> +- In *256 bins normal mode*, the HGO operates on the Y channel to compute a
> +  single 256-bins histogram. Only the YCbCr image format is supported.
> +- In *256 bins maximum mode*, the HGO operates on the maximum of the (R, G, 
> B)
> +  channels to compute a single 256-bins histogram. Only the RGB image format 
> is
> +  supported.

As they all share the same FOURCC format, according with the documentation,
how the user is supposed to switch between those modes? Or is it depend on
the video format? In any case, please add some explanation, and cross-refs
if needed.

> +
> +**Byte Order.**
> +All data is stored in memory in little endian format. Each cell in the tables
> +contains one byte.
> +
> +.. flat-table:: VSP1 HGO Data - 64 Bins, Normal Mode (792 bytes)
> +:header-rows:  2
> +:stub-columns: 0
> +
> +* - Offset
> +  - :cspan:`4` Memory
> +* -
> +  - [31:24]
> +  - [23:16]
> +  - [15:8]
> +  - [7:0]
> +* - 0
> +  - -
> +  - R/Cr/H max [7:0]
> +  - -
> +  - R/Cr/H min [7:0]
> +* - 4
> +  - -
> +  - G/Y/S max [7:0]
> +  - -
> +  - G/Y/S min [7:0]
> +* - 8
> +  - -
> +  - B/Cb/V max [7:0]
> +  - -
> +  - B/Cb/V min [7:0]
> +* - 12
> +  - :cspan:`4` R/Cr/H sum [31:0]
> +* - 16
> +  - :cspan:`4` G/Y/S sum [31:0]
> +* - 20
> +  - :cspan:`4` B/Cb/V sum [31:0]
> +* - 24
> +  - :cspan:`4` R/Cr/H bin 0 [31:0]
> +* -
> +  - :cspan:`4` ...
> +* - 276
> +  - :cspan:`4` R/Cr/H bin 63 [31:0]
> +* - 280
> +  - :cspan:`4` G/Y/S bin 0 [31:0]
> +* -
> +  - :cspan:`4` ...
> +* - 532
> +  - :cspan:`4` G/Y/S bin 63 [31:0]
> +* - 536
> +  - :cspan:`4` B/Cb/V bin 0 [31:0]
> +* 

Re: [PATCH v4 3/8] media: rcar_vin: Use correct pad number in try_fmt

[Mauro Carvalho Chehab]

Em Wed, 11 May 2016 16:02:51 +0200
Ulrich Hecht  escreveu:

> Fix rcar_vin_try_fmt's use of an inappropriate pad number when calling
> the subdev set_fmt function - for the ADV7612, IDs should be non-zero.
> 
> Signed-off-by: William Towle 
> Reviewed-by: Rob Taylor 
> Acked-by: Hans Verkuil 
> [uli: adapted to rcar-vin rewrite]

Please use [email@domain: some revierwer note], as stated at 
Documentation/SubmittingPatches.

> Signed-off-by: Ulrich Hecht 

This patch breaks compilation:

drivers/media/platform/rcar-vin/rcar-v4l2.c: In function 
'__rvin_try_format_source':
drivers/media/platform/rcar-vin/rcar-v4l2.c:115:18: error: 'struct rvin_dev' 
has no member named 'src_pad_idx'
  format.pad = vin->src_pad_idx;
  ^~



> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 14 +++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c 
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 0bc4487..42dbd35 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -98,7 +98,7 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
>   struct rvin_source_fmt *source)
>  {
>   struct v4l2_subdev *sd;
> - struct v4l2_subdev_pad_config pad_cfg;
> + struct v4l2_subdev_pad_config *pad_cfg;
>   struct v4l2_subdev_format format = {
>   .which = which,
>   };
> @@ -108,10 +108,16 @@ static int __rvin_try_format_source(struct rvin_dev 
> *vin,
>  
>   v4l2_fill_mbus_format(, pix, vin->source.code);
>  
> + pad_cfg = v4l2_subdev_alloc_pad_config(sd);
> + if (pad_cfg == NULL)
> + return -ENOMEM;
> +
> + format.pad = vin->src_pad_idx;
> +
>   ret = v4l2_device_call_until_err(sd->v4l2_dev, 0, pad, set_fmt,
> -  _cfg, );
> +  pad_cfg, );
>   if (ret < 0)
> - return ret;
> + goto cleanup;
>  
>   v4l2_fill_pix_format(pix, );
>  
> @@ -121,6 +127,8 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
>   vin_dbg(vin, "Source resolution: %ux%u\n", source->width,
>   source->height);
>  
> +cleanup:
> + v4l2_subdev_free_pad_config(pad_cfg);
>   return 0;
>  }
>  



Thanks,
Mauro

Re: [PATCH v2 05/13] v4l: vsp1: Add FCP support

[Mauro Carvalho Chehab]

Em Tue, 26 Apr 2016 00:36:30 +0300
Laurent Pinchart  escreveu:

> On some platforms the VSP performs memory accesses through an FCP. When
> that's the case get a reference to the FCP from the VSP DT node and
> enable/disable it at runtime as needed.
> 
> Signed-off-by: Laurent Pinchart 
> ---
>  .../devicetree/bindings/media/renesas,vsp1.txt  |  5 +
>  drivers/media/platform/Kconfig  |  1 +
>  drivers/media/platform/vsp1/vsp1.h  |  2 ++
>  drivers/media/platform/vsp1/vsp1_drv.c  | 21 
> -
>  4 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,vsp1.txt 
> b/Documentation/devicetree/bindings/media/renesas,vsp1.txt
> index 627405abd144..9b695bcbf219 100644
> --- a/Documentation/devicetree/bindings/media/renesas,vsp1.txt
> +++ b/Documentation/devicetree/bindings/media/renesas,vsp1.txt
> @@ -14,6 +14,11 @@ Required properties:
>- interrupts: VSP interrupt specifier.
>- clocks: A phandle + clock-specifier pair for the VSP functional clock.
>  
> +Optional properties:
> +
> +  - renesas,fcp: A phandle referencing the FCP that handles memory accesses
> + for the VSP. Not needed on Gen2, mandatory on Gen3.
> +
>  
>  Example: R8A7790 (R-Car H2) VSP1-S node
>  
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index f453910050be..a3304466e628 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -264,6 +264,7 @@ config VIDEO_RENESAS_VSP1
>   tristate "Renesas VSP1 Video Processing Engine"
>   depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && HAS_DMA
>   depends on (ARCH_RENESAS && OF) || COMPILE_TEST
> + depends on !ARM64 || VIDEO_RENESAS_FCP

It sounds that this will break compile-test on ARM64 for no good reason.

Thanks,
Mauro

Re: V4L2 SDR compliance test?

[Mauro Carvalho Chehab]

Em Mon, 9 May 2016 13:59:52 +
Ramesh Shanmugasundaram  escreveu:

> Hi Mauro,
> 
> Thanks for the clarifications.
> 
> > Ramesh Shanmugasundaram  escreveu:
> >   
> > > Hi Maintainers, All,
> > >
> > > Renesas R-Car SoCs have Digital Radio Interface (DRIF) controllers.
> > > They are receive only SPI slave modules that support I2S format. The 
> > > targeted application of these controllers is SDR.  
> [...]
> > > Two possible cases:
> > > ---
> > > 1) Third party tuner driver
> > >   - The framework does provide support to register tuner as a subdev 
> > > and can be bound to the SDR device using notifier callbacks  
> > 
> > Tuner is usually an i2c subdev, visible by the main SDR driver. No 
> > problem
> > here: the main driver should use the subdev callbacks to direct the 
> > tuner- specific ioctls to the tuner subdev.  
> 
> Yes. The main SDR driver can have code to direct tuner subdev.
>  
> >   
> > > 2) User space Tuner app
> > >   - We also have cases where the tuner s/w logic can be a vendor 
> > > supplied user space application or library that talks to the chip 
> > > using generic system calls - like i2c read/writes.  
> > 
> > Well, an userspace tuner app is out of the Kernel tree, so I can't see 
> > how it would affect it: it can have whatever API it needs (or even no 
> > API at all). Of course, in this case, the performance will very likely 
> > be worse, as the SDR data should also be handled in userspace.
> > 
> > If, for performance issues, it would require a faster I/O, then such 
> > tuner app should be converted to be a Kernel driver. Usually, it is 
> > not hard to convert those drivers to Kernelspace, as typically it is 
> > just a bunch of registers that should be set to make it to tune into 
> > an specific frequency and to adjust the tuner filters to the desired 
> > bandwidth and modulation type, in order to improve noise rejection.  
> 
> Yes, this is possible. However, this is Tuner chip vendor's decision (NDA, 
> license issues etc.) and it is not in our control.

True, but an independent tuner driver very likely can be written with very
little effort. All it requires is to monitor the traffic at the I2C bus
and to write a driver that reproduces it, identifying what part of the
I2C messages contain the frequency and enable/disable the filters.

We have several such drivers in the Kernel, and that's the procedure used
when the chipset vendor doesn't see the value of having his chipset used
by a Linux-based device.

> As mentioned above, we can have the main SDR (DRIF) driver code to direct 
> tuner subdev if present. However, when we want to upstream the DRIF driver we 
> may not have a real Tuner driver/device to get the compliance test results. 
> Should we run the compliance tests with a dummy stubbed tuner subdev? 

No, an upstream requirement is to have everything tested on real hardware.
What I recommend you is to either convince the tuner provider to send
upstream a driver (or allow you to do so) or to switch to some other vendor
whose driver is already in the Kernel or sees the value of having his
chipset used on Linux.
 
> Please do suggest how to proceed in this case? 
> 

Regards,
Mauro

[PATCH] [media] vsp1: make vsp1_drm_frame_end static

[Mauro Carvalho Chehab]

As reported by smatch:
drivers/media/platform/vsp1/vsp1_drm.c:39:6: warning: no previous 
prototype for 'vsp1_drm_frame_end' [-Wmissing-prototypes]
 void vsp1_drm_frame_end(struct vsp1_pipeline *pipe)

Fixes: ef9621bcd664 ("[media] v4l: vsp1: Store the display list manager in the 
WPF")
Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c 
b/drivers/media/platform/vsp1/vsp1_drm.c
index 22f67360b750..1f08da4b933b 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -36,7 +36,7 @@ void vsp1_drm_display_start(struct vsp1_device *vsp1)
vsp1_dlm_irq_display_start(vsp1->drm->pipe.output->dlm);
 }
 
-void vsp1_drm_frame_end(struct vsp1_pipeline *pipe)
+static void vsp1_drm_frame_end(struct vsp1_pipeline *pipe)
 {
vsp1_dlm_irq_frame_end(pipe->output->dlm);
 }
-- 
2.5.5

Re: [PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Mauro Carvalho Chehab]

Em Wed, 16 Mar 2016 11:03:38 -0300
Javier Martinez Canillas <jav...@dowhile0.org> escreveu:

> Hello Mauro,
> 
> The patch looks mostly good to me, I just have a question below:
> 
> On Wed, Mar 16, 2016 at 9:04 AM, Mauro Carvalho Chehab
> <mche...@osg.samsung.com> wrote:
> 
> [snip]
> 
> >
> > -void media_device_cleanup(struct media_device *mdev)
> > +static void media_device_cleanup(struct media_device *mdev)
> >  {
> > ida_destroy(>entity_internal_idx);
> > mdev->entity_internal_idx_max = 0;
> > media_entity_graph_walk_cleanup(>pm_count_walk);
> > -   mutex_destroy(>graph_mutex);  
> 
> Any reason why this is removed? mutex_init() is still called in
> media_device_init() so I believe the destroy should be kept here.

This code is now called only at do_media_device_unregister, with
is protected by the mutex. If we keep the mutex_destroy there,
the mutex will be destroyed in lock state, causing an error if
mutex debug is enabled.

Btw, the standard implementation for the mutex is:
include/linux/mutex.h:static inline void mutex_destroy(struct mutex 
*lock) {}

Only when mutex debug is enabled, it becomes something else:
include/linux/mutex-debug.h:extern void mutex_destroy(struct mutex 
*lock);

With produces a warning if the mutex_destroy() is called with the
mutex is hold. This can never happen with the current implementation, as
the logic is:
mutex_lock(>graph_mutex);
kref_put(>kref, do_media_device_unregister);
mutex_unlock(>graph_mutex);

We could eventually move the mutex lock to
do_media_device_unregister() and then add there the mutex_destroy()

Regards,
Mauro

> 
> Reviewed-by: Javier Martinez Canillas <jav...@osg.samsung.com>
> 
> Best regards,
> Javier


-- 
Thanks,
Mauro

[PATCH 5/5] [media] media-device: make media_device_cleanup() static

[Mauro Carvalho Chehab]

When multiple drivers are sharing the media_device struct,
one driver cannot know the right moment to cleanup the
media_device struct, because it can happen only when the
struct is not used by the other drivers.

So, let's call media_device_cleanup() internally, and
ensure that media_device_unregister() will do the right thing,
if the media device is not fully initialized.

Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
---
 drivers/media/common/siano/smsdvb-main.c  |  1 -
 drivers/media/media-device.c  | 13 ++---
 drivers/media/pci/saa7134/saa7134-core.c  |  1 -
 drivers/media/platform/exynos4-is/media-dev.c |  3 +--
 drivers/media/platform/omap3isp/isp.c |  1 -
 drivers/media/platform/s3c-camif/camif-core.c |  2 --
 drivers/media/platform/vsp1/vsp1_drv.c|  1 -
 drivers/media/platform/xilinx/xilinx-vipp.c   |  3 +--
 drivers/media/usb/au0828/au0828-core.c|  1 -
 drivers/media/usb/cx231xx/cx231xx-cards.c |  1 -
 drivers/media/usb/dvb-usb-v2/dvb_usb_core.c   |  1 -
 drivers/media/usb/dvb-usb/dvb-usb-dvb.c   |  1 -
 drivers/media/usb/em28xx/em28xx-cards.c   |  1 -
 drivers/media/usb/siano/smsusb.c  |  2 +-
 drivers/media/usb/uvc/uvc_driver.c|  4 +---
 include/media/media-device.h  | 10 --
 16 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/drivers/media/common/siano/smsdvb-main.c 
b/drivers/media/common/siano/smsdvb-main.c
index 9148e14c9d07..711c389c05e3 100644
--- a/drivers/media/common/siano/smsdvb-main.c
+++ b/drivers/media/common/siano/smsdvb-main.c
@@ -617,7 +617,6 @@ static void smsdvb_media_device_unregister(struct 
smsdvb_client_t *client)
if (!coredev->media_dev)
return;
media_device_unregister(coredev->media_dev);
-   media_device_cleanup(coredev->media_dev);
kfree(coredev->media_dev);
coredev->media_dev = NULL;
 #endif
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 38e6c319fe6e..0c7371eeda84 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -707,14 +707,12 @@ void media_device_init(struct media_device *mdev)
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
-void media_device_cleanup(struct media_device *mdev)
+static void media_device_cleanup(struct media_device *mdev)
 {
ida_destroy(>entity_internal_idx);
mdev->entity_internal_idx_max = 0;
media_entity_graph_walk_cleanup(>pm_count_walk);
-   mutex_destroy(>graph_mutex);
 }
-EXPORT_SYMBOL_GPL(media_device_cleanup);
 
 int __must_check __media_device_register(struct media_device *mdev,
 struct module *owner)
@@ -812,11 +810,12 @@ static void do_media_device_unregister(struct kref *kref)
}
 
/* Check if mdev devnode was registered */
-   if (!media_devnode_is_registered(>devnode))
-   return;
+   if (media_devnode_is_registered(>devnode)) {
+   device_remove_file(>devnode.dev, _attr_model);
+   media_devnode_unregister(>devnode);
+   }
 
-   device_remove_file(>devnode.dev, _attr_model);
-   media_devnode_unregister(>devnode);
+   media_device_cleanup(mdev);
 
dev_dbg(mdev->dev, "Media device unregistered\n");
 }
diff --git a/drivers/media/pci/saa7134/saa7134-core.c 
b/drivers/media/pci/saa7134/saa7134-core.c
index c0e1780ec831..213dc71f09eb 100644
--- a/drivers/media/pci/saa7134/saa7134-core.c
+++ b/drivers/media/pci/saa7134/saa7134-core.c
@@ -813,7 +813,6 @@ static void saa7134_unregister_media_device(struct 
saa7134_dev *dev)
if (!dev->media_dev)
return;
media_device_unregister(dev->media_dev);
-   media_device_cleanup(dev->media_dev);
kfree(dev->media_dev);
dev->media_dev = NULL;
 #endif
diff --git a/drivers/media/platform/exynos4-is/media-dev.c 
b/drivers/media/platform/exynos4-is/media-dev.c
index feb521f28e14..8c106fda7b84 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1501,7 +1501,7 @@ err_clk:
 err_m_ent:
fimc_md_unregister_entities(fmd);
 err_md:
-   media_device_cleanup(>media_dev);
+   media_device_unregister(>media_dev);
v4l2_device_unregister(>v4l2_dev);
return ret;
 }
@@ -1521,7 +1521,6 @@ static int fimc_md_remove(struct platform_device *pdev)
fimc_md_unregister_entities(fmd);
fimc_md_pipelines_free(fmd);
media_device_unregister(>media_dev);
-   media_device_cleanup(>media_dev);
fimc_md_put_clocks(fmd);
 
return 0;
diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 5d54e2c6c16b..cd67edcad8d3 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/om

Re: [PATCH] [media] media: rename media unregister function

[Mauro Carvalho Chehab]

Em Fri, 18 Mar 2016 16:20:19 +0200
Sakari Ailus <sakari.ai...@linux.intel.com> escreveu:

> Shuah Khan wrote:
> > On 03/18/2016 08:12 AM, Javier Martinez Canillas wrote:  
> >> Hello Shuah,
> >>
> >> On 03/18/2016 11:01 AM, Shuah Khan wrote:  
> >>> On 03/18/2016 07:05 AM, Mauro Carvalho Chehab wrote:  
> >>>> Now that media_device_unregister() also does a cleanup, rename it
> >>>> to media_device_unregister_cleanup().
> >>>>
> >>>> Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>  
> >>>
> >>> I think adding cleanup is redundant. media_device_unregister()
> >>> would imply that there has to be some cleanup releasing resources.
> >>> I wouldn't make this change.
> >>>  
> >>
> >> Problem is that there is a media_device_init() and media_device_register(),
> >> so having both unregister and cleanup in this function will make very clear
> >> that a single function is the counter part of the previous two operations.
> >>
> > 
> > Yes. I realized that this change is motivated by the fact that there is
> > the media_device_init() and we had the counterpart media_device_cleanup()
> > as an exported function. I still think there is no need to make the change
> > to add _cleanup() at the end of media_device_unregister(). It can be handled
> > in API documentation that it does both.  
> 
> I think that's a bad idea. People will only read the documentation when
> something doesn't work. In this case it's easy to miss that.

After thinking about that, I guess the best is to use kref only
if the media_device_*devres functions are used. With this, we don't
need to touch at media_device_cleanup().

Just the patch to the ML:
https://patchwork.linuxtv.org/patch/33533/

It was tested with HVR-950Q.

Regards,
Mauro

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Wed, 16 Mar 2016 10:28:35 +0200
Sakari Ailus <sakari.ai...@iki.fi> escreveu:

> Hi Mauro,
> 
> On Tue, Mar 15, 2016 at 12:55:35PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 14:09:09 +0200
> > Sakari Ailus <sakari.ai...@iki.fi> escreveu:
> >   
> > > Hi Mauro,
> > > 

...

> > > Notify callbacks, perhaps not, but the list is still protected by the
> > > spinlock. It perhaps is not likely that another process would change it 
> > > but
> > > I don't think we can rely on that.  
> > 
> > I can see only 2 risks protected by the lock:
> > 
> > 1) mdev gets freed while an entity is being created. This is a problem
> >with the current memory protection schema we're using. I guess the
> >only way to fix it is to use kref for 
> > mdev/entities/interfaces/links/pads.
> >This change doesn't make it better or worse.
> >Also, I don't think we have such risk with the current devices.
> > 
> > 2) a notifier may be inserted or removed by another driver, while the
> >loop is running.
> > 
> > To avoid (2), I see 3 alternatives:
> > 
> > a) keep the loop as proposed on this patch. As the list is navigated using 
> > list_for_each_entry_safe(), I guess[1] it should be safe to remove/add
> > new notify callbacks there while the loop is running by some other process. 
> >   
> 
> list_for_each_entry_safe() does not protect against concurrent access, only
> against adding and removing list entries by the same user. List access
> serialisation is still needed, whether you use _safe() functions or not.
> 
> > 
> > [1] It *is* safe if the change were done inside the loop - but I'm not
> > 100% sure that it is safe if some other CPU touches the notify list.  
> 
> Indeed.
> 
> > 
> > b) Unlock/relock the spinlock every time:
> > 
> > /* previous code that locks mdev->lock spinlock */
> > 
> > /* invoke entity_notify callbacks */
> > list_for_each_entry_safe(notify, next, >entity_notify, list) {
> > spin_unlock(>lock);
> > (notify)->notify(entity, notify->notify_data);
> > spin_lock(>lock);
> > }
> >  
> > spin_unlock(>lock);
> > 
> > c) use a separate lock for the notify list -this seems to be an overkill.
> > 
> > d) Protect it with the graph traversal mutex. That sounds the worse idea,
> >IMHO, as we'll be abusing the lock.  
> 
> I'd simply replace the spinlock with a mutex here. As we want to get rid of
> the graph mutex anyway in the long run, let's not mix the two as they're
> well separated now. As long as the mutex users do not sleep (i.e. the
> notify() callback) the mutex is about as fast to use as the spinlock.

It could work. I added such patch on an experimental branch, where
I'm addressing a few troubles with au0828 unbind logic:

https://git.linuxtv.org/mchehab/experimental.git/log/?h=au0828-unbind-fixes

The patch itself is at:

https://git.linuxtv.org/mchehab/experimental.git/commit/?h=au0828-unbind-fixes=dba4d41bdfa6bb8dc51cb0f692102919b2b7c8b4

At least for au0828, it seems to work fine.

Regards,
Mauro

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Mon, 14 Mar 2016 14:09:09 +0200
Sakari Ailus <sakari.ai...@iki.fi> escreveu:

> Hi Mauro,
> 
> On Mon, Mar 14, 2016 at 08:46:33AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 12:52:54 +0200
> > Sakari Ailus <sakari.ai...@iki.fi> escreveu:
> >   
> > > Hi Mauro,
> > > 
> > > On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:  
> > > > Em Mon, 14 Mar 2016 09:22:37 +0200
> > > > Sakari Ailus <sakari.ai...@iki.fi> escreveu:
> > > > 
> > > > > Hi Shuah,
> > > > > 
> > > > > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > > > > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > > > > media_devnode_create(), and media_add_link() that could get called
> > > > > > in atomic context to allow callers to pass in the right flags for
> > > > > > memory allocation.
> > > > > > 
> > > > > > tree-wide driver changes for media_*() GFP flags change:
> > > > > > Change drivers to add gfpflags to interffaces, 
> > > > > > media_create_pad_link(),
> > > > > > media_create_intf_link() and media_devnode_create().
> > > > > > 
> > > > > > Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
> > > > > > Suggested-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>  
> > > > > 
> > > > > What's the use case for calling the above functions in an atomic 
> > > > > context?
> > > > > 
> > > > 
> > > > ALSA code seems to do a lot of stuff at atomic context. That's what
> > > > happens on my test machine when au0828 gets probed before
> > > > snd-usb-audio:
> > > > http://pastebin.com/LEX5LD5K
> > > > 
> > > > It seems that ALSA USB probe routine (usb_audio_probe) happens in
> > > > atomic context.
> > > 
> > > usb_audio_probe() grabs a mutex (register_mutex) on its own. It certainly
> > > cannot be called in atomic context.
> > > 
> > > In the above log, what I did notice, though, was that because *we* grab
> > > mdev->lock spinlock in media_device_register_entity(), we may not sleep
> > > which is what the notify() callback implementation in au0828 driver does
> > > (for memory allocation).  
> > 
> > True. After looking into the code, the problem is that the notify
> > callbacks are called with the spinlock hold. I don't see any reason
> > to do that.  
> 
> Notify callbacks, perhaps not, but the list is still protected by the
> spinlock. It perhaps is not likely that another process would change it but
> I don't think we can rely on that.

I can see only 2 risks protected by the lock:

1) mdev gets freed while an entity is being created. This is a problem
   with the current memory protection schema we're using. I guess the
   only way to fix it is to use kref for mdev/entities/interfaces/links/pads.
   This change doesn't make it better or worse.
   Also, I don't think we have such risk with the current devices.

2) a notifier may be inserted or removed by another driver, while the
   loop is running.

To avoid (2), I see 3 alternatives:

a) keep the loop as proposed on this patch. As the list is navigated using 
list_for_each_entry_safe(), I guess[1] it should be safe to remove/add
new notify callbacks there while the loop is running by some other process. 

[1] It *is* safe if the change were done inside the loop - but I'm not
100% sure that it is safe if some other CPU touches the notify list.

b) Unlock/relock the spinlock every time:

/* previous code that locks mdev->lock spinlock */

/* invoke entity_notify callbacks */
list_for_each_entry_safe(notify, next, >entity_notify, list) {
spin_unlock(>lock);
(notify)->notify(entity, notify->notify_data);
spin_lock(>lock);
}
 
spin_unlock(>lock);

c) use a separate lock for the notify list -this seems to be an overkill.

d) Protect it with the graph traversal mutex. That sounds the worse idea,
   IMHO, as we'll be abusing the lock.

> 
> >   
> > > Could we instead replace mdev->lock by a mutex?  
> > 
> > We changed the code to use a spinlock for a reason: this fixed some
> > troubles in the past with the code locking (can't remember the problem,
> > but this was documented at the kernel logs and at the ML). Yet, the code
> > under the spinlock never sleeps, so this is fine.  
>

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Mon, 14 Mar 2016 12:52:54 +0200
Sakari Ailus <sakari.ai...@iki.fi> escreveu:

> Hi Mauro,
> 
> On Mon, Mar 14, 2016 at 07:13:58AM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 14 Mar 2016 09:22:37 +0200
> > Sakari Ailus <sakari.ai...@iki.fi> escreveu:
> >   
> > > Hi Shuah,
> > > 
> > > On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:  
> > > > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > > > media_devnode_create(), and media_add_link() that could get called
> > > > in atomic context to allow callers to pass in the right flags for
> > > > memory allocation.
> > > > 
> > > > tree-wide driver changes for media_*() GFP flags change:
> > > > Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> > > > media_create_intf_link() and media_devnode_create().
> > > > 
> > > > Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
> > > > Suggested-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
> > > 
> > > What's the use case for calling the above functions in an atomic context?
> > >   
> > 
> > ALSA code seems to do a lot of stuff at atomic context. That's what
> > happens on my test machine when au0828 gets probed before
> > snd-usb-audio:
> > http://pastebin.com/LEX5LD5K
> > 
> > It seems that ALSA USB probe routine (usb_audio_probe) happens in
> > atomic context.  
> 
> usb_audio_probe() grabs a mutex (register_mutex) on its own. It certainly
> cannot be called in atomic context.
> 
> In the above log, what I did notice, though, was that because *we* grab
> mdev->lock spinlock in media_device_register_entity(), we may not sleep
> which is what the notify() callback implementation in au0828 driver does
> (for memory allocation).

True. After looking into the code, the problem is that the notify
callbacks are called with the spinlock hold. I don't see any reason
to do that.

> Could we instead replace mdev->lock by a mutex?

We changed the code to use a spinlock for a reason: this fixed some
troubles in the past with the code locking (can't remember the problem,
but this was documented at the kernel logs and at the ML). Yet, the code
under the spinlock never sleeps, so this is fine.

Yet, in the future, we'll need to do a review of all the locking schema,
in order to better support dynamic graph changes.

> If there is no pressing need to implement atomic memory allocation I simply
> wouldn't do it, especially in device initialisation where an allocation
> failure will lead to probe failure as well.

The fix for this issue should be simple. See the enclosed code. Btw.
it probably makes sense to add some code here to avoid starving the
stack, as a notify callback could try to create an entity, with,
in turn, would call the notify callback again.

I'll run some tests here to double check if it fixes the issue.

---

[media] media-device: Don't call notify callbacks holding a spinlock

The notify routines may sleep. So, they can't be called in spinlock
context. Also, they may want to call other media routines that might
be spinning the spinlock, like creating a link.

This fixes the following bug:

[ 1839.510587] BUG: sleeping function called from invalid context at 
mm/slub.c:1289
[ 1839.510881] in_atomic(): 1, irqs_disabled(): 0, pid: 3479, name: modprobe
[ 1839.511157] 4 locks held by modprobe/3479:
[ 1839.511415]  #0:  (>mutex){..}, at: [] 
__driver_attach+0xa3/0x160
[ 1839.512381]  #1:  (>mutex){..}, at: [] 
__driver_attach+0xb1/0x160
[ 1839.512388]  #2:  (register_mutex#5){+.+.+.}, at: [] 
usb_audio_probe+0x257/0x1c90 [snd_usb_audio]
[ 1839.512401]  #3:  (&(>lock)->rlock){+.+.+.}, at: [] 
media_device_register_entity+0x1cb/0x700 [media]
[ 1839.512412] CPU: 2 PID: 3479 Comm: modprobe Not tainted 4.5.0-rc3+ #49
[ 1839.512415] Hardware name:  /NUC5i7RYB, BIOS 
RYBDWi35.86A.0350.2015.0812.1722 08/12/2015
[ 1839.512417]   8803b3f6f288 81933901 
8803c4bae000
[ 1839.512422]  8803c4bae5c8 8803b3f6f2b0 811c6af5 
8803c4bae000
[ 1839.512427]  8285d7f6 0509 8803b3f6f2f0 
811c6ce5
[ 1839.512432] Call Trace:
[ 1839.512436]  [] dump_stack+0x85/0xc4
[ 1839.512440]  [] ___might_sleep+0x245/0x3a0
[ 1839.512443]  [] __might_sleep+0x95/0x1a0
[ 1839.512446]  [] kmem_cache_alloc_trace+0x20e/0x300
[ 1839.512451]  [] ? media_add_link+0x4d/0x140 [media]
[ 1839.512455]  [] media_add_link+0x4d/0x140 [media]
[ 1839.512459]  [] media_create_pad_link+0xa1/0x600 [media]
[ 1839.512463]  [] au0828_media_graph_notify+0x173/0x360 
[au0828]
[ 1839.512467]  [] ? media_gobj_create+0x1ba/0x480 [media]
[ 1839.512471]  [] media

Re: [PATCH] media: add GFP flag to media_*() that could get called in atomic context

[Mauro Carvalho Chehab]

Em Mon, 14 Mar 2016 09:22:37 +0200
Sakari Ailus <sakari.ai...@iki.fi> escreveu:

> Hi Shuah,
> 
> On Sat, Mar 12, 2016 at 06:48:09PM -0700, Shuah Khan wrote:
> > Add GFP flags to media_create_pad_link(), media_create_intf_link(),
> > media_devnode_create(), and media_add_link() that could get called
> > in atomic context to allow callers to pass in the right flags for
> > memory allocation.
> > 
> > tree-wide driver changes for media_*() GFP flags change:
> > Change drivers to add gfpflags to interffaces, media_create_pad_link(),
> > media_create_intf_link() and media_devnode_create().
> > 
> > Signed-off-by: Shuah Khan <shua...@osg.samsung.com>
> > Suggested-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>  
> 
> What's the use case for calling the above functions in an atomic context?
> 

ALSA code seems to do a lot of stuff at atomic context. That's what
happens on my test machine when au0828 gets probed before
snd-usb-audio:
http://pastebin.com/LEX5LD5K

It seems that ALSA USB probe routine (usb_audio_probe) happens in
atomic context.

Regards,
Mauro

[PATCH] [media] vsp1_drm.h: add missing prototypes

[Mauro Carvalho Chehab]

drivers/media/platform/vsp1/vsp1_drm.c:47:5: warning: no previous prototype for 
'vsp1_du_init' [-Wmissing-prototypes]
 int vsp1_du_init(struct device *dev)
 ^
drivers/media/platform/vsp1/vsp1_drm.c:76:5: warning: no previous prototype for 
'vsp1_du_setup_lif' [-Wmissing-prototypes]
 int vsp1_du_setup_lif(struct device *dev, unsigned int width,
 ^
drivers/media/platform/vsp1/vsp1_drm.c:221:6: warning: no previous prototype 
for 'vsp1_du_atomic_begin' [-Wmissing-prototypes]
 void vsp1_du_atomic_begin(struct device *dev)
  ^
drivers/media/platform/vsp1/vsp1_drm.c:273:5: warning: no previous prototype 
for 'vsp1_du_atomic_update' [-Wmissing-prototypes]
 int vsp1_du_atomic_update(struct device *dev, unsigned int rpf_index,
 ^
drivers/media/platform/vsp1/vsp1_drm.c:451:6: warning: no previous prototype 
for 'vsp1_du_atomic_flush' [-Wmissing-prototypes]
 void vsp1_du_atomic_flush(struct device *dev)
  ^

Signed-off-by: Mauro Carvalho Chehab <mche...@osg.samsung.com>
---
 drivers/media/platform/vsp1/vsp1_drm.h | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.h 
b/drivers/media/platform/vsp1/vsp1_drm.h
index 7704038c3add..f68056838319 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.h
+++ b/drivers/media/platform/vsp1/vsp1_drm.h
@@ -35,4 +35,15 @@ int vsp1_drm_init(struct vsp1_device *vsp1);
 void vsp1_drm_cleanup(struct vsp1_device *vsp1);
 int vsp1_drm_create_links(struct vsp1_device *vsp1);
 
+int vsp1_du_init(struct device *dev);
+int vsp1_du_setup_lif(struct device *dev, unsigned int width,
+ unsigned int height);
+void vsp1_du_atomic_begin(struct device *dev);
+int vsp1_du_atomic_update(struct device *dev, unsigned int rpf_index,
+ u32 pixelformat, unsigned int pitch,
+ dma_addr_t mem[2], const struct v4l2_rect *src,
+ const struct v4l2_rect *dst);
+void vsp1_du_atomic_flush(struct device *dev);
+
+
 #endif /* __VSP1_DRM_H__ */
-- 
2.5.0