cron job: media_tree daily build: WARNINGS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Sat Aug 22 04:00:16 CEST 2015 git branch: test git hash: 3a6b0605c73d1d695f6d4e49289deaa3fa3e73ee gcc version:i686-linux-gcc (GCC) 5.1.0 sparse version: v0.5.0-51-ga53cea2 smatch version: 0.4.1-3153-g7d56ab3 host hardware: x86_64 host os:4.0.0-3.slh.1-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: OK linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin-bf561: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: WARNINGS linux-2.6.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12.23-i686: OK linux-3.13.11-i686: OK linux-3.14.9-i686: OK linux-3.15.2-i686: OK linux-3.16.7-i686: OK linux-3.17.8-i686: OK linux-3.18.7-i686: OK linux-3.19-i686: OK linux-4.0-i686: OK linux-4.1.1-i686: OK linux-4.2-rc1-i686: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12.23-x86_64: OK linux-3.13.11-x86_64: OK linux-3.14.9-x86_64: OK linux-3.15.2-x86_64: OK linux-3.16.7-x86_64: OK linux-3.17.8-x86_64: OK linux-3.18.7-x86_64: OK linux-3.19-x86_64: OK linux-4.0-x86_64: OK linux-4.1.1-x86_64: OK linux-4.2-rc1-x86_64: OK apps: OK spec-git: OK sparse: WARNINGS smatch: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/10] media/usb/pvrusb2: Support for V4L2_CTRL_WHICH_DEF_VAL
The code you've added is carefully checking the return pointer from
pvr2_hdw_get_ctrl_v4l() yet the original code did not operate this way.
The result is that now there's this "unbalanced" effect where it appears
that the validity of the pvr2_ctrl instance is only checked on one side
of the if-statement. I would recommend instead to elevate the call to
pvr2_hdw_get_ctrl_v4l() out of the if-statement - since in both cases
it's being called the same way both times. Then do the validity check
in that one spot and that simplifies the if-statement all the way down
to choosing between pvr2_ctrl_get_value() vs pvr2_ctrl_get_def().
It's not a correctness comment; what you have should work fine. So I'm
ack'ing this in any case:
Acked-By: Mike Isely
But you can do the above pretty easily & safely, and simplify it a bit
further.
-Mike
On Fri, 21 Aug 2015, Ricardo Ribalda Delgado wrote:
> This driver does not use the control infrastructure.
> Add support for the new field which on structure
> v4l2_ext_controls
>
> Signed-off-by: Ricardo Ribalda Delgado
> ---
> drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> index 1c5f85bf7ed4..43b2f2214798 100644
> --- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> +++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
> @@ -628,6 +628,7 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
> struct pvr2_v4l2_fh *fh = file->private_data;
> struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
> struct v4l2_ext_control *ctrl;
> + struct pvr2_ctrl *cptr;
> unsigned int idx;
> int val;
> int ret;
> @@ -635,8 +636,18 @@ static int pvr2_g_ext_ctrls(struct file *file, void
> *priv,
> ret = 0;
> for (idx = 0; idx < ctls->count; idx++) {
> ctrl = ctls->controls + idx;
> - ret = pvr2_ctrl_get_value(
> + if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL) {
> + cptr = pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id);
> + if (cptr)
> + pvr2_ctrl_get_def(cptr, &val);
> + else
> + ret = -EINVAL;
> +
> +
> + } else
> + ret = pvr2_ctrl_get_value(
> pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id), &val);
> +
> if (ret) {
> ctls->error_idx = idx;
> return ret;
> @@ -658,6 +669,10 @@ static int pvr2_s_ext_ctrls(struct file *file, void
> *priv,
> unsigned int idx;
> int ret;
>
> + /* Default value cannot be changed */
> + if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL)
> + return -EINVAL;
> +
> ret = 0;
> for (idx = 0; idx < ctls->count; idx++) {
> ctrl = ctls->controls + idx;
>
--
Mike Isely
isely @ isely (dot) net
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 7/8] [media] media: add a debug message to warn about gobj creation/removal
Hi Mauro, On Friday 21 August 2015 18:09:31 Mauro Carvalho Chehab wrote: > Em Fri, 21 Aug 2015 20:54:29 +0300 Laurent Pinchart escreveu: > > On Friday 21 August 2015 07:19:21 Mauro Carvalho Chehab wrote: > >> Em Fri, 21 Aug 2015 04:32:51 +0300 Laurent Pinchart escreveu: > >>> On Wednesday 19 August 2015 08:01:54 Mauro Carvalho Chehab wrote: > It helps to check if the media controller is doing the > right thing with the object creation and removal. > > No extra code/data will be produced if DEBUG or > CONFIG_DYNAMIC_DEBUG is not enabled. > >>> > >>> CONFIG_DYNAMIC_DEBUG is often enabled. > >> > >> True, but once a driver/core is properly debugged, images without DEBUG > >> could be used in production, if the amount of memory constraints are > >> too tight. > >> > >> > You're more or less adding function call tracing in this patch, isn't > >> > that something that ftrace is supposed to do ? > >> > >> Ftrace is a great infrastructure and helps a lot when we need to > >> identify bottlenecks and other performance related stuff, but it > >> doesn't replace debug functions. > >> > >> There are some fundamental differences on what you could do with ftrace > >> and what you can't. > >> > >> At least on this stage, what I need is something that will provide > >> output via serial console when the driver gets loaded, and that provides > >> a synchronous output with the other Kernel messages. > >> > >> This is the only way to debug certain OOPSes that are happening during > >> the development of the patches. > >> > >> This is something you cannot do with ftrace, but dynamic DEBUG works > >> like a charm. > > > > I understand the need for debug messages during development of a patch > > series, but I don't think this level of debugging belongs to mainline. > > Debug messages for function call tracing, even more in patch 6/8 and 7/8, > > is frowned upon in the kernel. > > > > Or maybe I got it wrong and patches 6/8 and 7/8 are only for development > > and you don't plan to get them in mainline ? > > As we've agreed, the first phase won't have dynamic support. Both patches > 6/8 and 7/8 are important until then. Why are they more important with dynamic support ? > So, they should reach mainline together with the first MC new gen series. Patch 6/8 states in its commit message that "We can only free the media device after being sure that no graph object is used. In order to help tracking it, let's add debug messages that will print when the media controller gets registered or unregistered." Instead of debug messages that need to be enabled and tracked manually, why not detecting the condition and issuing a WARN_ON() ? > Patch 6/8 can be reverted after we finish implementing dynamic support. > > I think patch 7/8 will still be a good debug feature, but we can discuss > about that after implementing dynamic support. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/8] [media] media: use media_gobj inside entities
Hi Mauro, On Friday 21 August 2015 18:01:31 Mauro Carvalho Chehab wrote: > Em Fri, 21 Aug 2015 20:51:10 +0300 Laurent Pinchart escreveu: > > On Friday 21 August 2015 07:09:44 Mauro Carvalho Chehab wrote: > >> Em Fri, 21 Aug 2015 04:10:03 +0300 Laurent Pinchart escreveu: > >>> On Wednesday 19 August 2015 08:01:50 Mauro Carvalho Chehab wrote: > As entities are graph elements, let's embed media_gobj > on it. That ensures an unique ID for entities that can be > global along the entire media controller. > > For now, we'll keep the already existing entity ID. Such > field need to be dropped at some point, but for now, let's > not do this, to avoid needing to review all drivers and > the userspace apps. > > Signed-off-by: Mauro Carvalho Chehab > Acked-by: Hans Verkuil > Signed-off-by: Mauro Carvalho Chehab > > diff --git a/drivers/media/media-device.c > b/drivers/media/media-device.c > index e429605ca2c3..81d6a130efef 100644 > --- a/drivers/media/media-device.c > +++ b/drivers/media/media-device.c > @@ -379,7 +379,6 @@ int __must_check __media_device_register(struct > media_device *mdev, > if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0)) > return -EINVAL; > > -mdev->entity_id = 1; > INIT_LIST_HEAD(&mdev->entities); > spin_lock_init(&mdev->lock); > mutex_init(&mdev->graph_mutex); > @@ -433,10 +432,8 @@ int __must_check > media_device_register_entity(struct media_device *mdev, > > entity->parent = mdev; > > spin_lock(&mdev->lock); > > -if (entity->id == 0) > -entity->id = mdev->entity_id++; > -else > -mdev->entity_id = max(entity->id + 1, mdev->entity_id); > +/* Initialize media_gobj embedded at the entity */ > +media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj); > >>> > >>> Graph object initialization should be moved to media_entity_init() to > >>> keep initialization separate from registration. > >> > >> Won't work. I tried. My first RFC patches were doing that. > >> > >> The problem is that media_entity_init() is currently called too early > >> at the V4L2 drivers, before having mdev assigned. > > > > That looks like a problem that should be fixed in the drivers then. The > > initialization of media devices and entities hasn't been thought of > > correctly, let's not carry mistakes forward. > > In this particular case, calling media_gobj_init() during > media_entity_register() won't cause any troubles, and moving it latter > to media_entity_init() is a two lines patch, once drivers got fixed > by the drivers maintainers. It's the "once drivers got fixed by the drivers maintainers" that worries me. We all know it unfortunately doesn't happen by itself, and I believe it will become more and more difficult as time goes by and more drivers use the MC framework. We're trying to refactor, clean up and extend MC, both in-kernel and towards userspace, to correct design and implementation mistakes. I'm worried that we'll make different but similarly painful mistakes if we don't take a bit of time to do things properly now. And while it will require a couple more patches, I don't think we're looking at months of work either. It might be that assigning an ID to objects can only be done at at registration time, especially for standalone subdev drivers (I'm thinking about the I2C camera sensors in particular). Still, calling media_gobj_init() in media_device_register_entity() doesn't sound right. Maybe the solution is to use media_gobj_init() for init-time initialization, and creating a media_gobj_assign_id() (or similarly-named) function to call at registration time. What I'd really like to see is clear explicit rules regarding how init/cleanup and register/unregister interact and how they should be used by drivers. The current mess is partly caused by not having thought this out properly to start with. I also have a feeling that we'll realize changes are required when implementing support for dynamic changes. > >> Also, objects without PADs don't call media_entity_init(). At long > >> term, this function may even disappear, as it only exists today to > >> allocate arrays for pads/links. If we get rid of the pads array, it > >> will make no sense to keep it. > > > > I wouldn't do that, as if we later add a field to media_entity that > > requires an initialization function to be called we would need to go and > > patch all the code that instantiates media_entity to add init calls. I'd > > prefer keeping the media_entity_init function even if all it does is call > > media_gobj_init. > > OK. > > list_add_tail(&entity->list, &mdev->entities); > spin_unlock(&mdev->lock); > -- Regards, Laurent Pinchart -- To unsubs
Re: [PATCH] v4l: omap3isp: Enable driver compilation with COMPILE_TEST
Hi Mauro, On Friday 21 August 2015 17:45:15 Mauro Carvalho Chehab wrote: > Em Fri, 21 Aug 2015 21:09:17 +0300 Laurent Pinchart escreveu: > > The omap3isp driver can't be compiled on non-ARM platforms but has no > > compile-time dependency on OMAP. Drop the OMAP dependency when > > COMPILE_TEST is set. > > > > Signed-off-by: Laurent Pinchart > > --- > > > > drivers/media/platform/Kconfig | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/Kconfig > > b/drivers/media/platform/Kconfig index 484038185ae3..95f0f6e6bbc8 100644 > > --- a/drivers/media/platform/Kconfig > > +++ b/drivers/media/platform/Kconfig > > @@ -85,7 +85,9 @@ config VIDEO_M32R_AR_M64278 > > > > config VIDEO_OMAP3 > > tristate "OMAP 3 Camera support" > > - depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3 > > + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API > > + depends on ARCH_OMAP3 || COMPILE_TEST > > + depends on ARM > > depends on HAS_DMA && OF > > depends on OMAP_IOMMU > > select ARM_DMA_USE_IOMMU > > Sorry, but this doesn't make sense. > > We can only add COMPILE_TEST after getting rid of those > depends on OMAP_IOMMU > select ARM_DMA_USE_IOMMU > > The COMPILE_TEST flag was added to support building drivers with > allyesconfig/allmodconfig for all archs. Selecting a sub-arch > specific configuration doesn't help at all (or make any difference, > as if such subarch is already selected, a make allmodconfig/allyesconfig > will build the driver anyway). As explained in the commit message, the driver currently explicitly depends on the OMAP IOMMU API (as well as the ARM DMA mapping API). That's something that will be removed at some point in the future (it "only" requires finding time to clean the code). COMPILE_TEST will then be useful and will need to be enabled by a patch similar to this one. I thus don't see a big reason not to enable COMPILE_TEST support now. > One of the main reasons why this is interesting is to support the > Coverity Scan community license, used by the Kernel janitors. This > tool runs only on x86. Not all drivers can be compiled on x86, some dependencies on ARM APIs can be valid. COMPILE_TEST with a dependency on ARM is already useful as it gives a much wider compile coverage than depending on a particular ARM platform. I would even not be surprised if the Linux kernel was compiled for ARM more often that x86 nowadays. As for coverity, how does running on x86 doesn't preclude analyzing source code written for ARM platforms ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/21] ARM: pxa: magician: Add OV9640 camera support
Dne 21.8.2015 v 19:36 Robert Jarzmik napsal(a): > I shrunk a bit the mailing list. > > Arnd Bergmann writes: > >> On Friday 21 August 2015 00:39:30 Petr Cvek wrote: >>> Dne 20.8.2015 v 22:26 Arnd Bergmann napsal(a): On Thursday 20 August 2015 21:48:20 Robert Jarzmik wrote: > Petr Cvek writes: >>> Datasheet says: >>> >>> tS:RESETSetting time after software/hardware reset 1 ms >>> >>> So at least one ~1 ms should be left there. Are msleep less than 20ms >>> valid? >>> >>> (checkpatch: msleep < 20ms can sleep for up to 20ms) >> >> On most kernels, an msleep(1) will sleep somewhere between 1 and 3 >> milliseconds >> (but could be much longer), while an mdelay(1) tries to sleep around one >> millisecond, more or less. > > I have rethought of that a bit more. I'm convinced a delay of "at least 1ms" > is > necessary according to the specification, it can also be more. > > Moreover, I came to the conclusion this reset sequence is not something that > is > "magician" specific (see palmz72_camera_reset() in > .../mach-pxa/palmz72.c). Actually it's not even mach-pxa specific, it's > "ov9640" > specific. > > Now add this to the fact that it would be good to have a solution working for > devicetree as well, and on any board, and the conclusion I came to was that > this > handling deserves to be in ov9640 driver (please correct me if I'm wrong). > > The idea behind it is have the reset handled in ov9640, and the gpio provided > by > platform data or devicetree. > > So Guennadi, is it possible to add a gpio through platform data to ov9640 > driver, does it make sense, and would you accept to have the reset handled > there > ? And if yes, would you, Petr, accept to revamp your patch to have the reset > and > power handled in ov9640 ? > OK, why not, so power and reset gpio with polarity settings? > Please note that it is a proposal, I'm not forcing anybody, I'd like to > choose a > path that agrees with the future push to remove as many machine files as > possible. Anyway I'm planning to send patch for OV9640 in future too (color correction matrix is not complete and some registers too). > > Cheers. > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 7/8] [media] media: add a debug message to warn about gobj creation/removal
Em Fri, 21 Aug 2015 20:54:29 +0300 Laurent Pinchart escreveu: > Hi Mauro, > > On Friday 21 August 2015 07:19:21 Mauro Carvalho Chehab wrote: > > Em Fri, 21 Aug 2015 04:32:51 +0300 Laurent Pinchart escreveu: > > > On Wednesday 19 August 2015 08:01:54 Mauro Carvalho Chehab wrote: > > > > It helps to check if the media controller is doing the > > > > right thing with the object creation and removal. > > > > > > > > No extra code/data will be produced if DEBUG or > > > > CONFIG_DYNAMIC_DEBUG is not enabled. > > > > > > CONFIG_DYNAMIC_DEBUG is often enabled. > > > > True, but once a driver/core is properly debugged, images without DEBUG > > could be used in production, if the amount of memory constraints are > > too tight. > > > > > You're more or less adding function call tracing in this patch, isn't that > > > something that ftrace is supposed to do ? > > > > Ftrace is a great infrastructure and helps a lot when we need to > > identify bottlenecks and other performance related stuff, but it > > doesn't replace debug functions. > > > > There are some fundamental differences on what you could do with ftrace > > and what you can't. > > > > At least on this stage, what I need is something that will provide > > output via serial console when the driver gets loaded, and that provides > > a synchronous output with the other Kernel messages. > > > > This is the only way to debug certain OOPSes that are happening during > > the development of the patches. > > > > This is something you cannot do with ftrace, but dynamic DEBUG works > > like a charm. > > I understand the need for debug messages during development of a patch > series, > but I don't think this level of debugging belongs to mainline. Debug messages > for function call tracing, even more in patch 6/8 and 7/8, is frowned upon in > the kernel. > > Or maybe I got it wrong and patches 6/8 and 7/8 are only for development and > you don't plan to get them in mainline ? As we've agreed, the first phase won't have dynamic support. Both patches 6/8 and 7/8 are important until then. So, they should reach mainline together with the first MC new gen series. Patch 6/8 can be reverted after we finish implementing dynamic support. I think patch 7/8 will still be a good debug feature, but we can discuss about that after implementing dynamic support. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/8] [media] media: use media_gobj inside entities
Em Fri, 21 Aug 2015 20:51:10 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> On Friday 21 August 2015 07:09:44 Mauro Carvalho Chehab wrote:
> > Em Fri, 21 Aug 2015 04:10:03 +0300 Laurent Pinchart escreveu:
> > > On Wednesday 19 August 2015 08:01:50 Mauro Carvalho Chehab wrote:
> > >> As entities are graph elements, let's embed media_gobj
> > >> on it. That ensures an unique ID for entities that can be
> > >> global along the entire media controller.
> > >>
> > >> For now, we'll keep the already existing entity ID. Such
> > >> field need to be dropped at some point, but for now, let's
> > >> not do this, to avoid needing to review all drivers and
> > >> the userspace apps.
> > >>
> > >> Signed-off-by: Mauro Carvalho Chehab
> > >> Acked-by: Hans Verkuil
> > >> Signed-off-by: Mauro Carvalho Chehab
> > >>
> > >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > >> index e429605ca2c3..81d6a130efef 100644
> > >> --- a/drivers/media/media-device.c
> > >> +++ b/drivers/media/media-device.c
> > >> @@ -379,7 +379,6 @@ int __must_check __media_device_register(struct
> > >> media_device *mdev,
> > >> if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
> > >> return -EINVAL;
> > >>
> > >> -mdev->entity_id = 1;
> > >> INIT_LIST_HEAD(&mdev->entities);
> > >> spin_lock_init(&mdev->lock);
> > >> mutex_init(&mdev->graph_mutex);
> > >> @@ -433,10 +432,8 @@ int __must_check media_device_register_entity(struct
> > >> media_device *mdev,
> > >> entity->parent = mdev;
> > >> spin_lock(&mdev->lock);
> > >>
> > >> -if (entity->id == 0)
> > >> -entity->id = mdev->entity_id++;
> > >> -else
> > >> -mdev->entity_id = max(entity->id + 1, mdev->entity_id);
> > >> +/* Initialize media_gobj embedded at the entity */
> > >> +media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
> > >
> > > Graph object initialization should be moved to media_entity_init() to keep
> > > initialization separate from registration.
> >
> > Won't work. I tried. My first RFC patches were doing that.
> >
> > The problem is that media_entity_init() is currently called too early
> > at the V4L2 drivers, before having mdev assigned.
>
> That looks like a problem that should be fixed in the drivers then. The
> initialization of media devices and entities hasn't been thought of
> correctly,
> let's not carry mistakes forward.
In this particular case, calling media_gobj_init() during
media_entity_register() won't cause any troubles, and moving it latter
to media_entity_init() is a two lines patch, once drivers got fixed
by the drivers maintainers.
> > Also, objects without PADs don't call media_entity_init(). At long
> > term, this function may even disappear, as it only exists today to
> > allocate arrays for pads/links. If we get rid of the pads array, it
> > will make no sense to keep it.
>
> I wouldn't do that, as if we later add a field to media_entity that requires
> an initialization function to be called we would need to go and patch all the
> code that instantiates media_entity to add init calls. I'd prefer keeping the
> media_entity_init function even if all it does is call media_gobj_init.
OK.
>
> > >> list_add_tail(&entity->list, &mdev->entities);
> > >> spin_unlock(&mdev->lock);
> > >>
> > >> @@ -459,6 +456,7 @@ void media_device_unregister_entity(struct
> > >> media_entity *entity)
> > >> return;
> > >>
> > >> spin_lock(&mdev->lock);
> > >> +media_gobj_remove(&entity->graph_obj);
> > >> list_del(&entity->list);
> > >> spin_unlock(&mdev->lock);
> > >> entity->parent = NULL;
> > >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > >> index 4834172bf6f8..888cb88e19bf 100644
> > >> --- a/drivers/media/media-entity.c
> > >> +++ b/drivers/media/media-entity.c
> > >> @@ -43,7 +43,12 @@ void media_gobj_init(struct media_device *mdev,
> > >> enum media_gobj_type type,
> > >> struct media_gobj *gobj)
> > >> {
> > >> -/* For now, nothing to do */
> > >> +/* Create a per-type unique object ID */
> > >> +switch (type) {
> > >> +case MEDIA_GRAPH_ENTITY:
> > >> +gobj->id = media_gobj_gen_id(type, ++mdev->entity_id);
> > >> +break;
> > >> +}
> > >> }
> > >>
> > >> /**
> > >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> > >> index a44f18fdf321..f6deef6e5820 100644
> > >> --- a/include/media/media-device.h
> > >> +++ b/include/media/media-device.h
> > >> @@ -41,7 +41,7 @@ struct device;
> > >> * @bus_info: Unique and stable device location identifier
> > >> * @hw_revision: Hardware device revision
> > >> * @driver_version: Device driver version
> > >> - * @entity_id: ID of the next entity to be registered
> > >> + *
Re: [PATCH v6 1/8] [media] media: create a macro to get entity ID
Em Fri, 21 Aug 2015 21:11:57 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> On Friday 21 August 2015 14:45:35 Mauro Carvalho Chehab wrote:
> > Em Fri, 21 Aug 2015 20:27:19 +0300 Laurent Pinchart escreveu:
> > > On Friday 21 August 2015 05:42:29 Mauro Carvalho Chehab wrote:
> > >> Em Fri, 21 Aug 2015 03:40:48 +0300 Laurent Pinchart escreveu:
> > >> > On Wednesday 19 August 2015 08:01:48 Mauro Carvalho Chehab wrote:
> > Instead of accessing directly entity.id, let's create a macro,
> > as this field will be moved into a common struct later on.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > Acked-by: Hans Verkuil
> > Signed-off-by: Mauro Carvalho Chehab
> > >
> > > [snip]
> > >
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> > b/drivers/media/platform/vsp1/vsp1_video.c index
> > 17f08973f835..debe4e539df6
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -352,10 +352,10 @@ static int
> > vsp1_pipeline_validate_branch(struct
> > vsp1_pipeline *pipe,
> > break;
> >
> > /* Ensure the branch has no loop. */
> > - if (entities & (1 << entity->subdev.entity.id))
> > + if (entities & (1 <<
> > media_entity_id(&entity->subdevntity)))
> > return -EPIPE;
> >
> > - entities |= 1 << entity->subdev.entity.id;
> > + entities |= 1 <<
> > media_entity_id(&entity->subdev.entity);
> >
> > /* UDS can't be chained. */
> > if (entity->type == VSP1_ENTITY_UDS) {
> > >>>
> > >>> I would move the modification of the vsp1 driver to Javier's patch
> > >>> that modifies the OMAP3 and OMAP4 drivers. Alternatively you could
> > >>> squash them into this patch, but I believe having a first patch that
> > >>> adds the inline function and a second patch that modifies all drivers
> > >>> to use it would be better.
> > >>
> > >> Squashing will lose Javier's authorship. I guess the better is have a
> > >> first patch with the inline, then my paches and Javier's ones, and
> > >> latter on the patch removing entity->id.
> > >
> > > What I meant is
> > >
> > > 1. This patch without the VSP1 chunk, with your authorship
> > > 2. Javier's patches for OMAP3 and OMAP4 + the VSP1 chunk squashed in a
> > > single patch, with Javier's authorship
> > > 3. Javier's patch removing entity->id, with Javier's authorship
> >
> > Actually, the removal of entity->id is at the first patch, with my
> > authorship, but I got the idea ;)
>
> I'm not sure to follow you. The first patch is this one, and it doesn't
> remove
> the id field from struct media_entity.
Sorry, I meant to say "the first patch series I submitted after the workshop",
e. .g RFC v1.
> > Btw, this was noticed because Javier is testing the MC new gen on OMAP3.
> > We should really enforce that all all drivers should compile with
> > COMPILE_TEST, as otherwise we'll keep having troubles like that.
>
> I agree with that. I've just sent a patch to enable compilation of the
> omap3isp driver with COMPILE_TEST. There's still a compile-time dependency on
> ARM, as well as a dependency on OMAP_IOMMU which currently depends on OMAP,
> but that can be fixed independently.
See the comments for the patch.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] v4l: omap3isp: Enable driver compilation with COMPILE_TEST
Em Fri, 21 Aug 2015 21:09:17 +0300 Laurent Pinchart escreveu: > The omap3isp driver can't be compiled on non-ARM platforms but has no > compile-time dependency on OMAP. Drop the OMAP dependency when > COMPILE_TEST is set. > > Signed-off-by: Laurent Pinchart > --- > drivers/media/platform/Kconfig | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > index 484038185ae3..95f0f6e6bbc8 100644 > --- a/drivers/media/platform/Kconfig > +++ b/drivers/media/platform/Kconfig > @@ -85,7 +85,9 @@ config VIDEO_M32R_AR_M64278 > > config VIDEO_OMAP3 > tristate "OMAP 3 Camera support" > - depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3 > + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API > + depends on ARCH_OMAP3 || COMPILE_TEST > + depends on ARM > depends on HAS_DMA && OF > depends on OMAP_IOMMU > select ARM_DMA_USE_IOMMU Sorry, but this doesn't make sense. We can only add COMPILE_TEST after getting rid of those depends on OMAP_IOMMU select ARM_DMA_USE_IOMMU The COMPILE_TEST flag was added to support building drivers with allyesconfig/allmodconfig for all archs. Selecting a sub-arch specific configuration doesn't help at all (or make any difference, as if such subarch is already selected, a make allmodconfig/allyesconfig will build the driver anyway). One of the main reasons why this is interesting is to support the Coverity Scan community license, used by the Kernel janitors. This tool runs only on x86. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] i2c: fix platform_no_drv_owner.cocci warnings
drivers/media/i2c/tc358743.c:1960:3-8: No need to set .owner here. The core
will do it.
Remove .owner field if calls are used which set it automatically
Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci
CC: Geert Uytterhoeven
Signed-off-by: Fengguang Wu
---
tc358743.c |1 -
1 file changed, 1 deletion(-)
--- a/drivers/media/i2c/tc358743.c
+++ b/drivers/media/i2c/tc358743.c
@@ -1957,7 +1957,6 @@ MODULE_DEVICE_TABLE(i2c, tc358743_id);
static struct i2c_driver tc358743_driver = {
.driver = {
- .owner = THIS_MODULE,
.name = "tc358743",
},
.probe = tc358743_probe,
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[linux-next:master 7080/10086] drivers/media/i2c/tc358743.c:1960:3-8: No need to set .owner here. The core will do it.
tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master head: 1ef981bcd18de26dc78bc79f092d6f4bb25e0e8f commit: 5cc596c66fe7c725ec049ae2093e7242034c97d6 [7080/10086] [media] i2c: Make all i2c devices visible if COMPILE_TEST=y coccinelle warnings: (new ones prefixed by >>) >> drivers/media/i2c/tc358743.c:1960:3-8: No need to set .owner here. The core >> will do it. Please review and possibly fold the followup patch. --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/3] media: atmel-isi: add sanity check for supported formats in try/set_fmt()
Hi Josh,
Thank you for the patch.
On Friday 21 August 2015 16:08:14 Josh Wu wrote:
> After adding the format check in try_fmt()/set_fmt(), we don't need any
> format check in configure_geometry(). So make configure_geometry() as
> void type.
>
> Signed-off-by: Josh Wu
> ---
>
> Changes in v3:
> - check the whether format is supported, if no then return a default
> format.
> - misc changes according to Laurent's feedback.
>
> Changes in v2:
> - new added patch
>
> drivers/media/platform/soc_camera/atmel-isi.c | 37 ++--
> 1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index fe9247a..84c91d3
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -102,17 +102,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
> return readl(isi->regs + reg);
> }
>
> -static int configure_geometry(struct atmel_isi *isi, u32 width,
> +static void configure_geometry(struct atmel_isi *isi, u32 width,
> u32 height, u32 code)
> {
> u32 cfg2;
>
> /* According to sensor's output format to set cfg2 */
> switch (code) {
> - /* YUV, including grey */
> + default:
> + /* Grey */
> case MEDIA_BUS_FMT_Y8_1X8:
> cfg2 = ISI_CFG2_GRAYSCALE;
> break;
> + /* YUV */
> case MEDIA_BUS_FMT_VYUY8_2X8:
> cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
> break;
> @@ -126,8 +128,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
> width, cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
> break;
> /* RGB, TODO */
> - default:
> - return -EINVAL;
> }
>
> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> @@ -138,8 +138,23 @@ static int configure_geometry(struct atmel_isi *isi,
> u32 width, cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
> & ISI_CFG2_IM_VSIZE_MASK;
> isi_writel(isi, ISI_CFG2, cfg2);
> +}
>
> - return 0;
> +static bool is_supported(struct soc_camera_device *icd,
> + const u32 pixformat)
> +{
> + switch (pixformat) {
> + /* YUV, including grey */
> + case V4L2_PIX_FMT_GREY:
> + case V4L2_PIX_FMT_YUYV:
> + case V4L2_PIX_FMT_UYVY:
> + case V4L2_PIX_FMT_YVYU:
> + case V4L2_PIX_FMT_VYUY:
> + return true;
> + /* RGB, TODO */
> + default:
> + return false;
> + }
> }
>
> static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -390,10 +405,8 @@ static int start_streaming(struct vb2_queue *vq,
> unsigned int count) /* Disable all interrupts */
> isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>
> - ret = configure_geometry(isi, icd->user_width, icd->user_height,
> + configure_geometry(isi, icd->user_width, icd->user_height,
> icd->current_fmt->code);
> - if (ret < 0)
> - return ret;
>
> spin_lock_irq(&isi->lock);
> /* Clear any pending interrupt */
> @@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
> *icd, struct v4l2_mbus_framefmt *mf = &format.format;
> int ret;
>
> + /* check with atmel-isi support format, if not support use UYVY */
> + if (!is_supported(icd, pix->pixelformat))
> + pix->pixelformat = V4L2_PIX_FMT_YUYV;
The comment mentions UYVY and the code uses YUYV.
> +
> xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
> if (!xlate) {
> dev_warn(icd->parent, "Format %x not found\n",
Can this still happen ?
> @@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
> *icd, u32 pixfmt = pix->pixelformat;
> int ret;
>
> + /* check with atmel-isi support format, if not support use UYVY */
> + if (!is_supported(icd, pix->pixelformat))
> + pix->pixelformat = V4L2_PIX_FMT_YUYV;
> +
> xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
> if (pixfmt && !xlate) {
> dev_warn(icd->parent, "Format %x not found\n", pixfmt);
Same comment here.
I wonder whether most of the content of isi_camera_set_fmt() and
isi_camera_try_fmt() could be factorized out into a shared function.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/8] [media] media: create a macro to get entity ID
Hi Mauro,
On Friday 21 August 2015 14:45:35 Mauro Carvalho Chehab wrote:
> Em Fri, 21 Aug 2015 20:27:19 +0300 Laurent Pinchart escreveu:
> > On Friday 21 August 2015 05:42:29 Mauro Carvalho Chehab wrote:
> >> Em Fri, 21 Aug 2015 03:40:48 +0300 Laurent Pinchart escreveu:
> >> > On Wednesday 19 August 2015 08:01:48 Mauro Carvalho Chehab wrote:
> Instead of accessing directly entity.id, let's create a macro,
> as this field will be moved into a common struct later on.
>
> Signed-off-by: Mauro Carvalho Chehab
> Acked-by: Hans Verkuil
> Signed-off-by: Mauro Carvalho Chehab
> >
> > [snip]
> >
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index
> 17f08973f835..debe4e539df6
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -352,10 +352,10 @@ static int
> vsp1_pipeline_validate_branch(struct
> vsp1_pipeline *pipe,
> break;
>
> /* Ensure the branch has no loop. */
> -if (entities & (1 << entity->subdev.entity.id))
> +if (entities & (1 <<
> media_entity_id(&entity->subdevntity)))
> return -EPIPE;
>
> -entities |= 1 << entity->subdev.entity.id;
> +entities |= 1 <<
> media_entity_id(&entity->subdev.entity);
>
> /* UDS can't be chained. */
> if (entity->type == VSP1_ENTITY_UDS) {
> >>>
> >>> I would move the modification of the vsp1 driver to Javier's patch
> >>> that modifies the OMAP3 and OMAP4 drivers. Alternatively you could
> >>> squash them into this patch, but I believe having a first patch that
> >>> adds the inline function and a second patch that modifies all drivers
> >>> to use it would be better.
> >>
> >> Squashing will lose Javier's authorship. I guess the better is have a
> >> first patch with the inline, then my paches and Javier's ones, and
> >> latter on the patch removing entity->id.
> >
> > What I meant is
> >
> > 1. This patch without the VSP1 chunk, with your authorship
> > 2. Javier's patches for OMAP3 and OMAP4 + the VSP1 chunk squashed in a
> > single patch, with Javier's authorship
> > 3. Javier's patch removing entity->id, with Javier's authorship
>
> Actually, the removal of entity->id is at the first patch, with my
> authorship, but I got the idea ;)
I'm not sure to follow you. The first patch is this one, and it doesn't remove
the id field from struct media_entity.
> Btw, this was noticed because Javier is testing the MC new gen on OMAP3.
> We should really enforce that all all drivers should compile with
> COMPILE_TEST, as otherwise we'll keep having troubles like that.
I agree with that. I've just sent a patch to enable compilation of the
omap3isp driver with COMPILE_TEST. There's still a compile-time dependency on
ARM, as well as a dependency on OMAP_IOMMU which currently depends on OMAP,
but that can be fixed independently.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] v4l: omap3isp: Enable driver compilation with COMPILE_TEST
The omap3isp driver can't be compiled on non-ARM platforms but has no compile-time dependency on OMAP. Drop the OMAP dependency when COMPILE_TEST is set. Signed-off-by: Laurent Pinchart --- drivers/media/platform/Kconfig | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig index 484038185ae3..95f0f6e6bbc8 100644 --- a/drivers/media/platform/Kconfig +++ b/drivers/media/platform/Kconfig @@ -85,7 +85,9 @@ config VIDEO_M32R_AR_M64278 config VIDEO_OMAP3 tristate "OMAP 3 Camera support" - depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3 + depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API + depends on ARCH_OMAP3 || COMPILE_TEST + depends on ARM depends on HAS_DMA && OF depends on OMAP_IOMMU select ARM_DMA_USE_IOMMU -- 2.4.6 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 7/8] [media] media: add a debug message to warn about gobj creation/removal
Hi Mauro, On Friday 21 August 2015 07:19:21 Mauro Carvalho Chehab wrote: > Em Fri, 21 Aug 2015 04:32:51 +0300 Laurent Pinchart escreveu: > > On Wednesday 19 August 2015 08:01:54 Mauro Carvalho Chehab wrote: > > > It helps to check if the media controller is doing the > > > right thing with the object creation and removal. > > > > > > No extra code/data will be produced if DEBUG or > > > CONFIG_DYNAMIC_DEBUG is not enabled. > > > > CONFIG_DYNAMIC_DEBUG is often enabled. > > True, but once a driver/core is properly debugged, images without DEBUG > could be used in production, if the amount of memory constraints are > too tight. > > > You're more or less adding function call tracing in this patch, isn't that > > something that ftrace is supposed to do ? > > Ftrace is a great infrastructure and helps a lot when we need to > identify bottlenecks and other performance related stuff, but it > doesn't replace debug functions. > > There are some fundamental differences on what you could do with ftrace > and what you can't. > > At least on this stage, what I need is something that will provide > output via serial console when the driver gets loaded, and that provides > a synchronous output with the other Kernel messages. > > This is the only way to debug certain OOPSes that are happening during > the development of the patches. > > This is something you cannot do with ftrace, but dynamic DEBUG works > like a charm. I understand the need for debug messages during development of a patch series, but I don't think this level of debugging belongs to mainline. Debug messages for function call tracing, even more in patch 6/8 and 7/8, is frowned upon in the kernel. Or maybe I got it wrong and patches 6/8 and 7/8 are only for development and you don't plan to get them in mainline ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/8] [media] media: use media_gobj inside entities
Hi Mauro,
On Friday 21 August 2015 07:09:44 Mauro Carvalho Chehab wrote:
> Em Fri, 21 Aug 2015 04:10:03 +0300 Laurent Pinchart escreveu:
> > On Wednesday 19 August 2015 08:01:50 Mauro Carvalho Chehab wrote:
> >> As entities are graph elements, let's embed media_gobj
> >> on it. That ensures an unique ID for entities that can be
> >> global along the entire media controller.
> >>
> >> For now, we'll keep the already existing entity ID. Such
> >> field need to be dropped at some point, but for now, let's
> >> not do this, to avoid needing to review all drivers and
> >> the userspace apps.
> >>
> >> Signed-off-by: Mauro Carvalho Chehab
> >> Acked-by: Hans Verkuil
> >> Signed-off-by: Mauro Carvalho Chehab
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index e429605ca2c3..81d6a130efef 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -379,7 +379,6 @@ int __must_check __media_device_register(struct
> >> media_device *mdev,
> >>if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
> >>return -EINVAL;
> >>
> >> - mdev->entity_id = 1;
> >>INIT_LIST_HEAD(&mdev->entities);
> >>spin_lock_init(&mdev->lock);
> >>mutex_init(&mdev->graph_mutex);
> >> @@ -433,10 +432,8 @@ int __must_check media_device_register_entity(struct
> >> media_device *mdev,
> >>entity->parent = mdev;
> >>spin_lock(&mdev->lock);
> >>
> >> - if (entity->id == 0)
> >> - entity->id = mdev->entity_id++;
> >> - else
> >> - mdev->entity_id = max(entity->id + 1, mdev->entity_id);
> >> + /* Initialize media_gobj embedded at the entity */
> >> + media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
> >
> > Graph object initialization should be moved to media_entity_init() to keep
> > initialization separate from registration.
>
> Won't work. I tried. My first RFC patches were doing that.
>
> The problem is that media_entity_init() is currently called too early
> at the V4L2 drivers, before having mdev assigned.
That looks like a problem that should be fixed in the drivers then. The
initialization of media devices and entities hasn't been thought of correctly,
let's not carry mistakes forward.
> Also, objects without PADs don't call media_entity_init(). At long
> term, this function may even disappear, as it only exists today to
> allocate arrays for pads/links. If we get rid of the pads array, it
> will make no sense to keep it.
I wouldn't do that, as if we later add a field to media_entity that requires
an initialization function to be called we would need to go and patch all the
code that instantiates media_entity to add init calls. I'd prefer keeping the
media_entity_init function even if all it does is call media_gobj_init.
> >>list_add_tail(&entity->list, &mdev->entities);
> >>spin_unlock(&mdev->lock);
> >>
> >> @@ -459,6 +456,7 @@ void media_device_unregister_entity(struct
> >> media_entity *entity)
> >>return;
> >>
> >>spin_lock(&mdev->lock);
> >> + media_gobj_remove(&entity->graph_obj);
> >>list_del(&entity->list);
> >>spin_unlock(&mdev->lock);
> >>entity->parent = NULL;
> >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> >> index 4834172bf6f8..888cb88e19bf 100644
> >> --- a/drivers/media/media-entity.c
> >> +++ b/drivers/media/media-entity.c
> >> @@ -43,7 +43,12 @@ void media_gobj_init(struct media_device *mdev,
> >> enum media_gobj_type type,
> >> struct media_gobj *gobj)
> >> {
> >> - /* For now, nothing to do */
> >> + /* Create a per-type unique object ID */
> >> + switch (type) {
> >> + case MEDIA_GRAPH_ENTITY:
> >> + gobj->id = media_gobj_gen_id(type, ++mdev->entity_id);
> >> + break;
> >> + }
> >> }
> >>
> >> /**
> >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> >> index a44f18fdf321..f6deef6e5820 100644
> >> --- a/include/media/media-device.h
> >> +++ b/include/media/media-device.h
> >> @@ -41,7 +41,7 @@ struct device;
> >> * @bus_info: Unique and stable device location identifier
> >> * @hw_revision: Hardware device revision
> >> * @driver_version: Device driver version
> >> - * @entity_id:ID of the next entity to be registered
> >> + * @entity_id:Unique ID used on the last entity registered
> >> * @entities: List of registered entities
> >> * @lock: Entities list lock
> >> * @graph_mutex: Entities graph operation lock
> >> @@ -69,6 +69,7 @@ struct media_device {
> >>u32 driver_version;
> >>
> >>u32 entity_id;
> >> +
> >>struct list_head entities;
> >>
> >>/* Protects the entities list */
> >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> >> index c1cd4fba051d..9ca366334bcf 100644
> >> --- a/include/media/media-entity.h
> >> +++ b/include/media/media-entity.h
> >> @@ -33,10 +33,10 @@
> >> /**
> >> * enum media_gobj_type
Re: [PATCH v6 1/8] [media] media: create a macro to get entity ID
Em Fri, 21 Aug 2015 20:27:19 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> On Friday 21 August 2015 05:42:29 Mauro Carvalho Chehab wrote:
> > Em Fri, 21 Aug 2015 03:40:48 +0300 Laurent Pinchart escreveu:
> > > On Wednesday 19 August 2015 08:01:48 Mauro Carvalho Chehab wrote:
> > > > Instead of accessing directly entity.id, let's create a macro,
> > > > as this field will be moved into a common struct later on.
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab
> > > > Acked-by: Hans Verkuil
> > > > Signed-off-by: Mauro Carvalho Chehab
>
> [snip]
>
> > > > diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> > > > b/drivers/media/platform/vsp1/vsp1_video.c index
> > > > 17f08973f835..debe4e539df6
> > > > 100644
> > > > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > > > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > > > @@ -352,10 +352,10 @@ static int vsp1_pipeline_validate_branch(struct
> > > > vsp1_pipeline *pipe,
> > > > break;
> > > >
> > > > /* Ensure the branch has no loop. */
> > > > - if (entities & (1 << entity->subdev.entity.id))
> > > > + if (entities & (1 <<
> > > > media_entity_id(&entity->subdev.entity)))
> > > > return -EPIPE;
> > > >
> > > > - entities |= 1 << entity->subdev.entity.id;
> > > > + entities |= 1 <<
> > > > media_entity_id(&entity->subdev.entity);
> > > >
> > > > /* UDS can't be chained. */
> > > > if (entity->type == VSP1_ENTITY_UDS) {
> > >
> > > I would move the modification of the vsp1 driver to Javier's patch that
> > > modifies the OMAP3 and OMAP4 drivers. Alternatively you could squash them
> > > into this patch, but I believe having a first patch that adds the inline
> > > function and a second patch that modifies all drivers to use it would be
> > > better.
> >
> > Squashing will lose Javier's authorship. I guess the better is have a
> > first patch with the inline, then my paches and Javier's ones, and
> > latter on the patch removing entity->id.
>
> What I meant is
>
> 1. This patch without the VSP1 chunk, with your authorship
> 2. Javier's patches for OMAP3 and OMAP4 + the VSP1 chunk squashed in a single
> patch, with Javier's authorship
> 3. Javier's patch removing entity->id, with Javier's authorship
Actually, the removal of entity->id is at the first patch, with my
authorship, but I got the idea ;)
Btw, this was noticed because Javier is testing the MC new gen on OMAP3.
We should really enforce that all all drivers should compile with
COMPILE_TEST, as otherwise we'll keep having troubles like that.
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2] DocBook/device-drivers: Add drivers/media core stuff
There are lots of docbook marks at the media subsystem, but those aren't used. Add the core headers/code in order to start generating docs. --- WARNING: Please notice that, while this doesn't cause any error, still there are lots of warnings to be fixed: http://pastebin.com/Ld916dFi And that's because a lot of media core stuff got commented! It would be good if someone could address those. Signed-off-by: Mauro Carvalho Chehab diff --git a/Documentation/DocBook/device-drivers.tmpl b/Documentation/DocBook/device-drivers.tmpl index faf09d4a0ea8..e3e0f4880770 100644 --- a/Documentation/DocBook/device-drivers.tmpl +++ b/Documentation/DocBook/device-drivers.tmpl @@ -216,6 +216,36 @@ X!Isound/sound_firmware.c --> + + Media Devices +!Iinclude/media/media-device.h +!Iinclude/media/media-devnode.h +!Iinclude/media/media-entity.h +!Iinclude/media/v4l2-async.h +!Iinclude/media/v4l2-flash-led-class.h +!Iinclude/media/v4l2-mem2mem.h +!Iinclude/media/v4l2-of.h +!Iinclude/media/v4l2-subdev.h +!Iinclude/media/rc-core.h + + + + 16x50 UART Driver !Edrivers/tty/serial/serial_core.c diff --git a/drivers/media/dvb-core/dvb_math.h b/drivers/media/dvb-core/dvb_math.h index aecc867e9404..f586aa001ede 100644 --- a/drivers/media/dvb-core/dvb_math.h +++ b/drivers/media/dvb-core/dvb_math.h @@ -30,9 +30,10 @@ * to use rational values you can use the following method: * intlog2(value) = intlog2(value * 2^x) - x * 2^24 * - * example: intlog2(8) will give 3 << 24 = 3 * 2^24 - * example: intlog2(9) will give 3 << 24 + ... = 3.16... * 2^24 - * example: intlog2(1.5) = intlog2(3) - 2^24 = 0.584... * 2^24 + * Some usecase examples: + * intlog2(8) will give 3 << 24 = 3 * 2^24 + * intlog2(9) will give 3 << 24 + ... = 3.16... * 2^24 + * intlog2(1.5) = intlog2(3) - 2^24 = 0.584... * 2^24 * * @param value The value (must be != 0) * @return log2(value) * 2^24 @@ -45,7 +46,8 @@ extern unsigned int intlog2(u32 value); * to use rational values you can use the following method: * intlog10(value) = intlog10(value * 10^x) - x * 2^24 * - * example: intlog10(1000) will give 3 << 24 = 3 * 2^24 + * An usecase example: + * intlog10(1000) will give 3 << 24 = 3 * 2^24 * due to the implementation intlog10(1000) might be not exactly 3 * 2^24 * * look at intlog2 for similar examples -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/8] [media] media: create a macro to get entity ID
Hi Mauro,
On Friday 21 August 2015 05:42:29 Mauro Carvalho Chehab wrote:
> Em Fri, 21 Aug 2015 03:40:48 +0300 Laurent Pinchart escreveu:
> > On Wednesday 19 August 2015 08:01:48 Mauro Carvalho Chehab wrote:
> > > Instead of accessing directly entity.id, let's create a macro,
> > > as this field will be moved into a common struct later on.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > > Acked-by: Hans Verkuil
> > > Signed-off-by: Mauro Carvalho Chehab
[snip]
> > > diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> > > b/drivers/media/platform/vsp1/vsp1_video.c index
> > > 17f08973f835..debe4e539df6
> > > 100644
> > > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > > @@ -352,10 +352,10 @@ static int vsp1_pipeline_validate_branch(struct
> > > vsp1_pipeline *pipe,
> > > break;
> > >
> > > /* Ensure the branch has no loop. */
> > > - if (entities & (1 << entity->subdev.entity.id))
> > > + if (entities & (1 << media_entity_id(&entity->subdev.entity)))
> > > return -EPIPE;
> > >
> > > - entities |= 1 << entity->subdev.entity.id;
> > > + entities |= 1 << media_entity_id(&entity->subdev.entity);
> > >
> > > /* UDS can't be chained. */
> > > if (entity->type == VSP1_ENTITY_UDS) {
> >
> > I would move the modification of the vsp1 driver to Javier's patch that
> > modifies the OMAP3 and OMAP4 drivers. Alternatively you could squash them
> > into this patch, but I believe having a first patch that adds the inline
> > function and a second patch that modifies all drivers to use it would be
> > better.
>
> Squashing will lose Javier's authorship. I guess the better is have a
> first patch with the inline, then my paches and Javier's ones, and
> latter on the patch removing entity->id.
What I meant is
1. This patch without the VSP1 chunk, with your authorship
2. Javier's patches for OMAP3 and OMAP4 + the VSP1 chunk squashed in a single
patch, with Javier's authorship
3. Javier's patch removing entity->id, with Javier's authorship
I don't see a need to split usage of media_entity_id() into per-driver
patches.
> I'll do that on the next patch series.
>
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index 8b21a4d920d9..0a66fc225559 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -113,6 +113,11 @@ static inline u32 media_entity_subtype(struct
> > > media_entity *entity) return entity->type & MEDIA_ENT_SUBTYPE_MASK;
> > >
> > > }
> > >
> > > +static inline u32 media_entity_id(struct media_entity *entity)
> > > +{
> > > + return entity->id;
> > > +}
> > > +
> > >
> > > #define MEDIA_ENTITY_ENUM_MAX_DEPTH 16
> > > #define MEDIA_ENTITY_ENUM_MAX_ID 64
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RFC] DocBook/device-drivers: Add drivers/media core stuff
There are lots of docbook marks at the media subsystem, but those aren't used. Add the core headers/code in order to start generating docs. === WARNING: === Please notice that the output is currently broken: http://pastebin.com/JcXsqcVH Before applying this one, we need to fix the stuff there! Signed-off-by: Mauro Carvalho Chehab diff --git a/Documentation/DocBook/device-drivers.tmpl b/Documentation/DocBook/device-drivers.tmpl index faf09d4a0ea8..a273c4fd774e 100644 --- a/Documentation/DocBook/device-drivers.tmpl +++ b/Documentation/DocBook/device-drivers.tmpl @@ -216,6 +216,34 @@ X!Isound/sound_firmware.c --> + + Media Devices +!Iinclude/media/media-device.h +!Iinclude/media/media-devnode.h +!Iinclude/media/media-entity.h +!Iinclude/media/v4l2-async.h +!Iinclude/media/v4l2-ctrls.h +!Iinclude/media/v4l2-dv-timings.h +!Iinclude/media/v4l2-event.h +!Iinclude/media/v4l2-flash-led-class.h +!Iinclude/media/v4l2-mediabus.h +!Iinclude/media/v4l2-mem2mem.h +!Iinclude/media/v4l2-of.h +!Iinclude/media/v4l2-subdev.h +!Iinclude/media/videobuf2-core.h +!Iinclude/media/videobuf2-memops.h +!Iinclude/media/rc-core.h +!Iinclude/media/lirc.h +!Idrivers/media/dvb-core/dvb_ca_en50221.h +!Edrivers/media/dvb-core/dvb_demux.c +!Idrivers/media/dvb-core/dvb_frontend.h +!Idrivers/media/dvb-core/dvb_math.h +!Edrivers/media/dvb-core/dvb_net.c +!Idrivers/media/dvb-core/dvb_ringbuffer.h +!Idrivers/media/dvb-core/dvbdev.h + + + 16x50 UART Driver !Edrivers/tty/serial/serial_core.c -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[GIT PULL for v4.2] media fixes
Hi Linus, Please pull from: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media tags/media/v4.2-3 For some fixes: - a regression fix at the videobuf2 core driver; - Fix error handling at mantis probing code; - Revert the IR encode patches, as the API is not mature enough. So, better to postpone the changes to a latter Kernel; - Fix Kconfig breakages on some randconfig scenarios. Thanks! Mauro PS.: Sorry for the mess on my previous pull request. Yeah, the script I was using to get patches from patchwork was broken and were mangling the subject when importing e-mails with git Revert subjects. - The following changes since commit faebbd8f134f0c054f372982c8ddd1bbcc41b440: [media] lmedm04: fix the range for relative measurements (2015-06-24 08:38:30 -0300) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media tags/media/v4.2-3 for you to fetch changes up to 02387b5f25bdba668c7fe2618697bae24f973667: [media] mantis: Fix error handling in mantis_dma_init() (2015-08-19 07:04:55 -0300) media fixes for v4.2-rc8 David Härdeman (7): Revert "[media] rc: nuvoton-cir: Add support for writing wakeup samples via sysfs filter callback" Revert "[media] rc: rc-loopback: Add loopback of filter scancodes" Revert "[media] rc: rc-core: Add support for encode_wakeup drivers" Revert "[media] rc: ir-rc6-decoder: Add encode capability" Revert "[media] rc: ir-rc5-decoder: Add encode capability" Revert "[media] rc: rc-ir-raw: Add Manchester encoder (phase encoder) helper" Revert "[media] rc: rc-ir-raw: Add scancode encoder callback" Fabio Estevam (1): [media] mantis: Fix error handling in mantis_dma_init() Laurent Pinchart (1): [media] vb2: Fix compilation breakage when !CONFIG_BUG Randy Dunlap (2): [media] media/dvb: fix ts2020.c Kconfig and build [media] media/pci/cobalt: fix Kconfig and build when SND is not enabled Sakari Ailus (1): [media] vb2: Only requeue buffers immediately once streaming is started drivers/media/dvb-frontends/Kconfig | 2 +- drivers/media/pci/cobalt/Kconfig | 1 + drivers/media/pci/cobalt/cobalt-irq.c| 2 +- drivers/media/pci/mantis/mantis_dma.c| 5 +- drivers/media/rc/ir-rc5-decoder.c| 116 - drivers/media/rc/ir-rc6-decoder.c| 122 - drivers/media/rc/nuvoton-cir.c | 127 - drivers/media/rc/nuvoton-cir.h | 1 - drivers/media/rc/rc-core-priv.h | 36 - drivers/media/rc/rc-ir-raw.c | 139 -- drivers/media/rc/rc-loopback.c | 36 - drivers/media/rc/rc-main.c | 7 +- drivers/media/v4l2-core/videobuf2-core.c | 40 +- include/media/rc-core.h | 7 - include/media/videobuf2-core.h | 2 + 15 files changed, 34 insertions(+), 609 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] [media] DocBook: fix an unbalanced tag
The got lost on some change at the DVB docbook, making the tag unbalanced and causing a warning. Fix it. Signed-off-by: Mauro Carvalho Chehab diff --git a/Documentation/DocBook/media/dvb/intro.xml b/Documentation/DocBook/media/dvb/intro.xml index d30751338493..51db15648099 100644 --- a/Documentation/DocBook/media/dvb/intro.xml +++ b/Documentation/DocBook/media/dvb/intro.xml @@ -164,7 +164,7 @@ are called: from 0, and M enumerates the devices of each type within each adapter, starting from 0, too. We will omit the “ /dev/dvb/adapterN/” in the further discussion -of these devices. +of these devices. More details about the data structures and function calls of all the devices are described in the following chapters. -- 2.4.3 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Bugs reporting
Hi, can you point me where is usually reported the bugs with the cards firmware and/or drivers. The combination is Cine S2 v6.5 with Duoflex S2 V4. The miniDiSEqC (AB) is not working on the Duoflex S2 V4. Is not board fault, same behavior have all cards i tested - about 20 Duoflex and 8 Cine modules. For the protocol i tested: Cine S2 v6.5 + Duoflex S2 V4: - ports 0 and 1 (Ports on Cine card) - miniDiSEqC is working - ports 0 and 1 (Ports on Cine card) - DiSEqC is working - ports 2 and up (Ports on Duflex cards) - DiSEqC is working - ports 2 and up (Ports on Duflex cards) - miniDiSEqC is NOT WORKING Cine S2 v6.2 + Duoflex S2 older versions: - all ports - both Cine and Duoflex - miniDiSEqC is working - all ports - both Cine and Duoflex - DiSEqC is working Cine S2 v6.5 + Duoflex S2 older version: - Not tested, will test soon Cine S2 v6.2 + Duoflex S2 V4: - Not tested, will test when purchase next batch. Also there is tons of errors on i2c bus to Duoflex S2 V4 tabs. This was tested with latest drivers, also was tested with stock drivers with several kernel versions from 3.18.11 to 4.2 rc2 - same behavior -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/10] media/v4l2-core: struct struct v4l2_ext_controls param which
Support for new field which on v4l2_ext_controls, used to get the
default value of one or more controls.
Signed-off-by: Ricardo Ribalda Delgado
---
I missed a ;... sorry about that
drivers/media/v4l2-core/v4l2-ctrls.c | 34 +-
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 580098ba5c0b..0caae655968e 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1489,6 +1489,17 @@ static int new_to_user(struct v4l2_ext_control *c,
return ptr_to_user(c, ctrl, ctrl->p_new);
}
+/* Helper function: copy the initial control value back to the caller */
+static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
+{
+ int idx;
+
+ for (idx = 0; idx < ctrl->elems; idx++)
+ ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
+
+ return ptr_to_user(c, ctrl, ctrl->p_new);
+}
+
/* Helper function: copy the caller-provider value to the given control value
*/
static int user_to_ptr(struct v4l2_ext_control *c,
struct v4l2_ctrl *ctrl,
@@ -2708,7 +2719,9 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
*hdl,
cs->error_idx = i;
- if (cs->which && V4L2_CTRL_ID2WHICH(id) != cs->which)
+ if (cs->which &&
+ cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
+ V4L2_CTRL_ID2WHICH(id) != cs->which)
return -EINVAL;
/* Old-style private controls are not allowed for
@@ -2787,7 +2800,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
*hdl,
whether there are any controls at all. */
static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
{
- if (!which)
+ if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
}
@@ -2801,6 +2814,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
struct v4l2_ctrl_helper *helpers = helper;
int ret;
int i, j;
+ bool def_value;
+
+ def_value = (cs->which == V4L2_CTRL_WHICH_DEF_VAL);
cs->error_idx = cs->count;
cs->which = V4L2_CTRL_ID2WHICH(cs->which);
@@ -2827,9 +2843,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
for (i = 0; !ret && i < cs->count; i++) {
int (*ctrl_to_user)(struct v4l2_ext_control *c,
- struct v4l2_ctrl *ctrl) = cur_to_user;
+ struct v4l2_ctrl *ctrl);
struct v4l2_ctrl *master;
+ ctrl_to_user = def_value ? def_to_user : cur_to_user;
+
if (helpers[i].mref == NULL)
continue;
@@ -2839,8 +2857,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
v4l2_ctrl_lock(master);
/* g_volatile_ctrl will update the new control values */
- if ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
- (master->has_volatiles && !is_cur_manual(master))) {
+ if (!def_value &&
+ ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
+ (master->has_volatiles && !is_cur_manual(master {
for (j = 0; j < master->ncontrols; j++)
cur_to_new(master->cluster[j]);
ret = call_op(master, g_volatile_ctrl);
@@ -3062,6 +3081,11 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct
v4l2_ctrl_handler *hdl,
int ret;
cs->error_idx = cs->count;
+
+ /* Default value cannot be changed */
+ if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
cs->which = V4L2_CTRL_ID2WHICH(cs->which);
if (hdl == NULL)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] saa7164: convert to the control framework
Convert this driver to the control framework. Note that the VBI device
nodes have no controls as there is nothing to control.
Signed-off-by: Hans Verkuil
---
Steve, can you test this patch? For some reason my saa7164 isn't seen anymore
on the PCIe bus, so I can't test it myself :-(
This patch greatly simplifies this driver w.r.t. control handling and it will
now be fully compliant to the spec as well.
Thanks,
Hans
---
drivers/media/pci/saa7164/saa7164-encoder.c | 464 +---
drivers/media/pci/saa7164/saa7164-vbi.c | 359 -
drivers/media/pci/saa7164/saa7164.h | 2 +
3 files changed, 84 insertions(+), 741 deletions(-)
diff --git a/drivers/media/pci/saa7164/saa7164-encoder.c
b/drivers/media/pci/saa7164/saa7164-encoder.c
index 4434e0f..f5e1236 100644
--- a/drivers/media/pci/saa7164/saa7164-encoder.c
+++ b/drivers/media/pci/saa7164/saa7164-encoder.c
@@ -35,24 +35,6 @@ static struct saa7164_tvnorm saa7164_tvnorms[] = {
}
};
-static const u32 saa7164_v4l2_ctrls[] = {
- V4L2_CID_BRIGHTNESS,
- V4L2_CID_CONTRAST,
- V4L2_CID_SATURATION,
- V4L2_CID_HUE,
- V4L2_CID_AUDIO_VOLUME,
- V4L2_CID_SHARPNESS,
- V4L2_CID_MPEG_STREAM_TYPE,
- V4L2_CID_MPEG_VIDEO_ASPECT,
- V4L2_CID_MPEG_VIDEO_B_FRAMES,
- V4L2_CID_MPEG_VIDEO_GOP_SIZE,
- V4L2_CID_MPEG_AUDIO_MUTE,
- V4L2_CID_MPEG_VIDEO_BITRATE_MODE,
- V4L2_CID_MPEG_VIDEO_BITRATE,
- V4L2_CID_MPEG_VIDEO_BITRATE_PEAK,
- 0
-};
-
/* Take the encoder configuration form the port struct and
* flush it to the hardware.
*/
@@ -396,253 +378,47 @@ static int vidioc_s_frequency(struct file *file, void
*priv,
return 0;
}
-static int vidioc_g_ctrl(struct file *file, void *priv,
- struct v4l2_control *ctl)
+static int saa7164_s_ctrl(struct v4l2_ctrl *ctrl)
{
- struct saa7164_encoder_fh *fh = file->private_data;
- struct saa7164_port *port = fh->port;
- struct saa7164_dev *dev = port->dev;
-
- dprintk(DBGLVL_ENC, "%s(id=%d, value=%d)\n", __func__,
- ctl->id, ctl->value);
-
- switch (ctl->id) {
- case V4L2_CID_BRIGHTNESS:
- ctl->value = port->ctl_brightness;
- break;
- case V4L2_CID_CONTRAST:
- ctl->value = port->ctl_contrast;
- break;
- case V4L2_CID_SATURATION:
- ctl->value = port->ctl_saturation;
- break;
- case V4L2_CID_HUE:
- ctl->value = port->ctl_hue;
- break;
- case V4L2_CID_SHARPNESS:
- ctl->value = port->ctl_sharpness;
- break;
- case V4L2_CID_AUDIO_VOLUME:
- ctl->value = port->ctl_volume;
- break;
- default:
- return -EINVAL;
- }
-
- return 0;
-}
-
-static int vidioc_s_ctrl(struct file *file, void *priv,
- struct v4l2_control *ctl)
-{
- struct saa7164_encoder_fh *fh = file->private_data;
- struct saa7164_port *port = fh->port;
+ struct saa7164_port *port =
+ container_of(ctrl->handler, struct saa7164_port, ctrl_handler);
+ struct saa7164_encoder_params *params = &port->encoder_params;
struct saa7164_dev *dev = port->dev;
int ret = 0;
- dprintk(DBGLVL_ENC, "%s(id=%d, value=%d)\n", __func__,
- ctl->id, ctl->value);
-
- switch (ctl->id) {
+ switch (ctrl->id) {
case V4L2_CID_BRIGHTNESS:
- if ((ctl->value >= 0) && (ctl->value <= 255)) {
- port->ctl_brightness = ctl->value;
- saa7164_api_set_usercontrol(port,
- PU_BRIGHTNESS_CONTROL);
- } else
- ret = -EINVAL;
+ port->ctl_brightness = ctrl->val;
+ saa7164_api_set_usercontrol(port, PU_BRIGHTNESS_CONTROL);
break;
case V4L2_CID_CONTRAST:
- if ((ctl->value >= 0) && (ctl->value <= 255)) {
- port->ctl_contrast = ctl->value;
- saa7164_api_set_usercontrol(port, PU_CONTRAST_CONTROL);
- } else
- ret = -EINVAL;
+ port->ctl_contrast = ctrl->val;
+ saa7164_api_set_usercontrol(port, PU_CONTRAST_CONTROL);
break;
case V4L2_CID_SATURATION:
- if ((ctl->value >= 0) && (ctl->value <= 255)) {
- port->ctl_saturation = ctl->value;
- saa7164_api_set_usercontrol(port,
- PU_SATURATION_CONTROL);
- } else
- ret = -EINVAL;
+ port->ctl_saturation = ctrl->val;
+ saa7164_api_set_usercontrol(port, PU_SATURATION_CONTROL);
break;
case V4L2_CID_HUE:
- if ((ctl->value >= 0) && (ctl->value <= 255)) {
-
Re: WARNING: CPU: 1 PID: 813 at kernel/module.c:291 module_assert_mutex_or_preempt+0x49/0x90()
On 08/21/2015 03:21 AM, poma wrote: On 10.08.2015 23:26, poma wrote: [ cut here ] WARNING: CPU: 1 PID: 813 at kernel/module.c:291 module_assert_mutex_or_preempt+0x49/0x90() Modules linked in: mxl5007t af9013 ... dvb_usb_af9015(+) ... dvb_usb_v2 dvb_core rc_core ... CPU: 1 PID: 813 Comm: systemd-udevd Not tainted 4.2.0-0.rc6.git0.1.fc24.x86_64+debug #1 ... Call Trace: [] dump_stack+0x4c/0x65 [] warn_slowpath_common+0x86/0xc0 [] ? af9013_read_ucblocks+0x20/0x20 [af9013] [] ? af9013_read_ucblocks+0x20/0x20 [af9013] [] warn_slowpath_null+0x1a/0x20 [] module_assert_mutex_or_preempt+0x49/0x90 [] __module_address+0x32/0x150 [] ? af9013_read_ucblocks+0x20/0x20 [af9013] [] ? af9013_read_ucblocks+0x20/0x20 [af9013] [] __module_text_address+0x16/0x70 [] ? af9013_read_ucblocks+0x20/0x20 [af9013] [] ? af9013_read_ucblocks+0x20/0x20 [af9013] [] symbol_put_addr+0x29/0x40 [] dvb_frontend_detach+0x7d/0x90 [dvb_core] [] dvb_usbv2_probe+0xc85/0x11a0 [dvb_usb_v2] [] af9015_probe+0x84/0xf0 [dvb_usb_af9015] [] usb_probe_interface+0x1bb/0x2e0 [] driver_probe_device+0x1f6/0x450 [] __driver_attach+0x94/0xa0 [] ? driver_probe_device+0x450/0x450 [] bus_for_each_dev+0x73/0xc0 [] driver_attach+0x1e/0x20 [] bus_add_driver+0x1ee/0x280 [] driver_register+0x60/0xe0 [] usb_register_driver+0xad/0x160 [] ? 0xa0567000 [] af9015_usb_driver_init+0x1e/0x1000 [dvb_usb_af9015] [] do_one_initcall+0xb3/0x200 [] ? kmem_cache_alloc_trace+0x355/0x380 [] ? do_init_module+0x28/0x1e9 [] do_init_module+0x60/0x1e9 [] load_module+0x21f7/0x28d0 [] ? m_show+0x1b0/0x1b0 [] ? sched_clock+0x9/0x10 [] ? local_clock+0x1c/0x20 [] SyS_init_module+0x178/0x1c0 [] entry_SYSCALL_64_fastpath+0x12/0x76 ---[ end trace 31a9dd90d4f559f5 ]--- Ref. https://bugzilla.redhat.com/show_bug.cgi?id=1252167 https://bugzilla.kernel.org/show_bug.cgi?id=102631 You guys are really something. First of all, as is evident here, I am the original reporter, not Laura Abbott . -All- your comments including the final patch on this issue, are just plain wrong.[1] This patch only hides the actual problem with this particular device - the dual tuner combination driven by dvb_usb_af9015 & mxl5007t, broken by design since day one. Read the entire https://bugzilla.redhat.com/show_bug.cgi?id=1252167 and stop this nonsense. NACK "module: Fix locking in symbol_put_addr()" [1] http://www.gossamer-threads.com/lists/linux/kernel/2241154 Sorry, I've been delayed with conferences and have been avoiding e-mails and bugzilla. I missed sending out the Tested-by tag. You are welcome to drop the reported-by if you don't think it is appropriate. Laura -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver
On 08/21/2015 03:03 PM, Thierry Reding wrote:
> On Thu, Aug 20, 2015 at 05:51:39PM -0700, Bryan Wu wrote:
>> NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
>> controller which can support up to 6 MIPI CSI camera sensors.
>>
>> This patch adds a V4L2 media controller and capture driver to support
>> Tegra VI hardware. It's verified with Tegra built-in test pattern
>> generator.
>
> Hi Bryan,
>
> I've been looking forward to seeing this posted. I don't know the VI
> hardware in very much detail, nor am I an expert on the media framework,
> so I will primarily comment on architectural or SoC-specific things.
>
> By the way, please always Cc linux-te...@vger.kernel.org on all patches
> relating to Tegra. That way people not explicitly Cc'ed but still
> interested in Tegra will see this code, even if they aren't subscribed
> to the linux-media mailing list.
>
>> Signed-off-by: Bryan Wu
>> Reviewed-by: Hans Verkuil
>> ---
>> drivers/media/platform/Kconfig |1 +
>> drivers/media/platform/Makefile |2 +
>> drivers/media/platform/tegra/Kconfig |9 +
>> drivers/media/platform/tegra/Makefile|3 +
>> drivers/media/platform/tegra/tegra-channel.c | 1074
>> ++
>> drivers/media/platform/tegra/tegra-core.c| 295 +++
>> drivers/media/platform/tegra/tegra-core.h| 134
>> drivers/media/platform/tegra/tegra-vi.c | 585 ++
>> drivers/media/platform/tegra/tegra-vi.h | 224 ++
>> include/dt-bindings/media/tegra-vi.h | 35 +
>> 10 files changed, 2362 insertions(+)
>> create mode 100644 drivers/media/platform/tegra/Kconfig
>> create mode 100644 drivers/media/platform/tegra/Makefile
>> create mode 100644 drivers/media/platform/tegra/tegra-channel.c
>> create mode 100644 drivers/media/platform/tegra/tegra-core.c
>> create mode 100644 drivers/media/platform/tegra/tegra-core.h
>> create mode 100644 drivers/media/platform/tegra/tegra-vi.c
>> create mode 100644 drivers/media/platform/tegra/tegra-vi.h
>> create mode 100644 include/dt-bindings/media/tegra-vi.h
>
> I can't spot a device tree binding document for this, but we'll need one
> to properly review this driver.
>
>> diff --git a/drivers/media/platform/tegra/Kconfig
>> b/drivers/media/platform/tegra/Kconfig
>> new file mode 100644
>> index 000..a69d1b2
>> --- /dev/null
>> +++ b/drivers/media/platform/tegra/Kconfig
>> @@ -0,0 +1,9 @@
>> +config VIDEO_TEGRA
>> +tristate "NVIDIA Tegra Video Input Driver (EXPERIMENTAL)"
>
> I don't think the (EXPERIMENTAL) is warranted. Either the driver works
> or it doesn't. And I assume you already tested that it works, even if
> only using the TPG.
>
>> +depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF
>
> This seems to be missing a couple of dependencies. For example I would
> expect at least TEGRA_HOST1X to be listed here to make sure people can't
> select this when the host1x API isn't available. I would also expect
> some sort of architecture dependency because it really makes no sense to
> build this if Tegra isn't supported.
>
> If you are concerned about compile coverage you can make that explicit
> using a COMPILE_TEST alternative such as:
>
> depends on ARCH_TEGRA || (ARM && COMPILE_TEST)
>
> Note that the ARM dependency in there makes sure that HAVE_IOMEM is
> selected, so this could also be:
>
> depends on ARCH_TEGRA || (HAVE_IOMEM && COMPILE_TEST)
>
> though that'd still leave open the possibility of build breakage because
> of some missing support.
>
> If you add the dependency on TEGRA_HOST1X that I mentioned above you
> shouldn't need any architecture dependency because TEGRA_HOST1X implies
> those already.
>
>> +select VIDEOBUF2_DMA_CONTIG
>> +---help---
>> + Driver for Video Input (VI) device controller in NVIDIA Tegra SoC.
>
> I'd reword this slightly as:
>
> Driver for the Video Input (VI) controller found on NVIDIA Tegra
> SoCs.
>
>> +
>> + TO compile this driver as a module, choose M here: the module will be
>
> s/TO/To/.
>
>> + called tegra-video.
>
>> diff --git a/drivers/media/platform/tegra/Makefile
>> b/drivers/media/platform/tegra/Makefile
>> new file mode 100644
>> index 000..c8eff0b
>> --- /dev/null
>> +++ b/drivers/media/platform/tegra/Makefile
>> @@ -0,0 +1,3 @@
>> +tegra-video-objs += tegra-core.o tegra-vi.o tegra-channel.o
>
> I'd personally leave out the redundant tegra- prefix here, because the
> files are in a tegra/ subdirectory already.
This is actually consistent with the other media drivers, so please keep the
prefix. One can debate whether the prefix makes sense or not, but changing
that would be a subsystem-wide change.
>> +static int tegra_channel_capture_frame(struct tegra_channel *chan)
>> +{
>> +struct tegra_channel_buffer *buf = chan->active;
>> +struct vb2_buffer *vb = &buf->buf;
>> +int err = 0;
>> +u32 thresh, valu
[PATCH v2 04/10] media/core: Replace ctrl_class with which
Replace the obsolete field ctrl_class with "which".
Make sure it not used in future modules by commenting out the field with
ifndef __KERNEL_ .
The field cannot be simply removed because that would be change on the
kenel API to the userspace (and we don't like that).
Signed-off-by: Ricardo Ribalda Delgado
---
.../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 6 +++---
drivers/media/pci/saa7164/saa7164-encoder.c| 6 +++---
drivers/media/pci/saa7164/saa7164-vbi.c| 6 +++---
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 2 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 6 +++---
drivers/media/v4l2-core/v4l2-ctrls.c | 24 +++---
drivers/media/v4l2-core/v4l2-ioctl.c | 14 ++---
include/uapi/linux/videodev2.h | 5 +
9 files changed, 38 insertions(+), 33 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
index c5bdbfcc42b3..8f4eb395665d 100644
--- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
@@ -61,7 +61,7 @@ must belong to the same control class.
Applications must always fill in the
count,
-ctrl_class,
+which,
controls and
reserved fields of &v4l2-ext-controls;, and
initialize the &v4l2-ext-control; array pointed to by the
@@ -109,7 +109,7 @@ the driver whether wrong values are automatically adjusted
to a valid
value or if an error is returned.
When the id or
-ctrl_class is invalid drivers return an
+which is invalid drivers return an
&EINVAL;. When the value is out of bounds drivers can choose to take
the closest valid value or return an &ERANGE;, whatever seems more
appropriate. In the first case the new value is set in
@@ -383,7 +383,7 @@ These controls are described in .
The &v4l2-ext-control; id
is invalid, the &v4l2-ext-controls;
-ctrl_class is invalid, or the &v4l2-ext-control;
+which is invalid, or the &v4l2-ext-control;
value was inappropriate (e.g. the given menu
index is not supported by the driver). This error code is
also returned by the VIDIOC_S_EXT_CTRLS and
diff --git a/drivers/media/pci/saa7164/saa7164-encoder.c
b/drivers/media/pci/saa7164/saa7164-encoder.c
index 4434e0f28c26..e0ec64ed06fc 100644
--- a/drivers/media/pci/saa7164/saa7164-encoder.c
+++ b/drivers/media/pci/saa7164/saa7164-encoder.c
@@ -538,7 +538,7 @@ static int vidioc_g_ext_ctrls(struct file *file, void *priv,
struct saa7164_port *port = fh->port;
int i, err = 0;
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+ if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
for (i = 0; i < ctrls->count; i++) {
struct v4l2_ext_control *ctrl = ctrls->controls + i;
@@ -612,7 +612,7 @@ static int vidioc_try_ext_ctrls(struct file *file, void
*priv,
{
int i, err = 0;
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+ if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
for (i = 0; i < ctrls->count; i++) {
struct v4l2_ext_control *ctrl = ctrls->controls + i;
@@ -687,7 +687,7 @@ static int vidioc_s_ext_ctrls(struct file *file, void *priv,
struct saa7164_port *port = fh->port;
int i, err = 0;
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+ if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
for (i = 0; i < ctrls->count; i++) {
struct v4l2_ext_control *ctrl = ctrls->controls + i;
diff --git a/drivers/media/pci/saa7164/saa7164-vbi.c
b/drivers/media/pci/saa7164/saa7164-vbi.c
index 859fd03d82f9..08f23b6d9cef 100644
--- a/drivers/media/pci/saa7164/saa7164-vbi.c
+++ b/drivers/media/pci/saa7164/saa7164-vbi.c
@@ -501,7 +501,7 @@ static int vidioc_g_ext_ctrls(struct file *file, void *priv,
struct saa7164_port *port = fh->port;
int i, err = 0;
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+ if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
for (i = 0; i < ctrls->count; i++) {
struct v4l2_ext_control *ctrl = ctrls->controls + i;
@@ -560,7 +560,7 @@ static int vidioc_try_ext_ctrls(struct file *file, void
*priv,
{
int i, err = 0;
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+ if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
for (i = 0; i < ctrls->count; i++) {
struct v4l2_ext_control *ctrl = ctrls->controls + i;
@@ -626,7 +626,7 @@ static int vidioc_s_ext_ctrls(struct file *file, void *priv,
struct saa7164_port *port = fh->port;
int i, err = 0;
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
+ if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
for (i = 0; i < ctrls->count; i++) {
stru
[PATCH v2 03/10] media/v4l2-compat-ioctl32: Simple stylechecks
The next patches on the serie need this modifications to pass clean
checkpath.pl.
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index af635430524e..5416806609a8 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -609,11 +609,11 @@ static inline int put_v4l2_input32(struct v4l2_input *kp,
struct v4l2_input32 __
}
struct v4l2_ext_controls32 {
- __u32 ctrl_class;
- __u32 count;
- __u32 error_idx;
- __u32 reserved[2];
- compat_caddr_t controls; /* actually struct v4l2_ext_control32 * */
+ __u32 ctrl_class;
+ __u32 count;
+ __u32 error_idx;
+ __u32 reserved[2];
+ compat_caddr_t controls; /* actually struct v4l2_ext_control32 * */
};
struct v4l2_ext_control32 {
@@ -655,7 +655,8 @@ static int get_v4l2_ext_controls32(struct v4l2_ext_controls
*kp, struct v4l2_ext
get_user(kp->ctrl_class, &up->ctrl_class) ||
get_user(kp->count, &up->count) ||
get_user(kp->error_idx, &up->error_idx) ||
- copy_from_user(kp->reserved, up->reserved,
sizeof(kp->reserved)))
+ copy_from_user(kp->reserved, up->reserved,
+ sizeof(kp->reserved)))
return -EFAULT;
n = kp->count;
if (n == 0) {
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 00/10] Support getting default values from any control
Integer controls provide a way to get their default/initial value, but
any other control (p_u32, p_u8.) provide no other way to get the
initial value than unloading the module and loading it back.
*What is the actual problem?
I have a custom control with WIDTH integer values. Every value
represents the calibrated FPN (fixed pattern noise) correction value for that
column
-Application A changes the FPN correction value
-Application B wants to restore the calibrated value but it cant :(
*What is the proposed solution?
(Kudos to Hans Verkuil!!!)
The key change is in struct v4l2_ext_controls where the __u32 ctrl_class field
is changed to:
union {
__u32 ctrl_class;
__u32 which;
};
And two new defines are added:
#define V4L2_CTRL_WHICH_CUR_VAL0
#define V4L2_CTRL_WHICH_DEF_VAL0x0f00
The 'which' field tells you which controls are get/set/tried.
V4L2_CTRL_WHICH_CUR_VAL: the current value of the controls
V4L2_CTRL_WHICH_DEF_VAL: the default value of the controls
V4L2_CTRL_CLASS_*: the current value of the controls belonging to the specified
class.
Note: this is deprecated usage and is only there for backwards
compatibility.
Which is also why I don't think there is a need to add V4L2_CTRL_WHICH_
aliases for these defines.
I have posted a copy of my working tree to
https://github.com/ribalda/linux/tree/which_def_v2
Changelog v2:
Suggested by Hans Verkuil
Replace ctrls_class with which on all the codebase
Changes in Documentation
(Thanks!)
Ricardo Ribalda Delgado (10):
videodev2.h: Fix typo in comment
videodev2.h: Extend struct v4l2_ext_controls
media/v4l2-compat-ioctl32: Simple stylechecks
media/core: Replace ctrl_class with which
media/v4l2-core: struct struct v4l2_ext_controls param which
usb/uvc: Support for V4L2_CTRL_WHICH_DEF_VAL
media/usb/pvrusb2: Support for V4L2_CTRL_WHICH_DEF_VAL
media/pci/saa7164-encoder Support for V4L2_CTRL_WHICH_DEF_VAL
media/pci/saa7164-vbi Support for V4L2_CTRL_WHICH_DEF_VAL
Docbook: media: Document changes on struct v4l2_ext_controls
Documentation/DocBook/media/v4l/v4l2.xml | 9
.../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 28 --
drivers/media/pci/saa7164/saa7164-encoder.c| 59 -
drivers/media/pci/saa7164/saa7164-vbi.c| 61 +-
drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 2 +-
drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 2 +-
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 17 +-
drivers/media/usb/uvc/uvc_v4l2.c | 14 -
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 17 +++---
drivers/media/v4l2-core/v4l2-ctrls.c | 54 +--
drivers/media/v4l2-core/v4l2-ioctl.c | 14 ++---
include/uapi/linux/videodev2.h | 14 -
12 files changed, 200 insertions(+), 91 deletions(-)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 01/10] videodev2.h: Fix typo in comment
Referenced file has moved
Signed-off-by: Ricardo Ribalda Delgado
---
include/uapi/linux/videodev2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 3228fbebcd63..72fa3e490e30 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2271,7 +2271,7 @@ struct v4l2_create_buffers {
#define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
/* Reminder: when adding new ioctls please add support for them to
- drivers/media/video/v4l2-compat-ioctl32.c as well! */
+ drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
#define BASE_VIDIOC_PRIVATE192 /* 192-255 are private */
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 06/10] usb/uvc: Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/usb/uvc/uvc_v4l2.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 2764f43607c1..e6d3a1bcfa2f 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -980,6 +980,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void
*fh,
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
struct v4l2_ext_control *ctrl = ctrls->controls;
+ struct v4l2_queryctrl qc;
unsigned int i;
int ret;
@@ -988,7 +989,14 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void
*fh,
return ret;
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
- ret = uvc_ctrl_get(chain, ctrl);
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ qc.id = ctrl->id;
+ ret = uvc_query_v4l2_ctrl(chain, &qc);
+ if (!ret)
+ ctrl->value = qc.default_value;
+ } else
+ ret = uvc_ctrl_get(chain, ctrl);
+
if (ret < 0) {
uvc_ctrl_rollback(handle);
ctrls->error_idx = i;
@@ -1010,6 +1018,10 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh
*handle,
unsigned int i;
int ret;
+ /* Default value cannot be changed */
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
ret = uvc_ctrl_begin(chain);
if (ret < 0)
return ret;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 07/10] media/usb/pvrusb2: Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index 1c5f85bf7ed4..43b2f2214798 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -628,6 +628,7 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
struct pvr2_v4l2_fh *fh = file->private_data;
struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
struct v4l2_ext_control *ctrl;
+ struct pvr2_ctrl *cptr;
unsigned int idx;
int val;
int ret;
@@ -635,8 +636,18 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
ret = 0;
for (idx = 0; idx < ctls->count; idx++) {
ctrl = ctls->controls + idx;
- ret = pvr2_ctrl_get_value(
+ if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ cptr = pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id);
+ if (cptr)
+ pvr2_ctrl_get_def(cptr, &val);
+ else
+ ret = -EINVAL;
+
+
+ } else
+ ret = pvr2_ctrl_get_value(
pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id), &val);
+
if (ret) {
ctls->error_idx = idx;
return ret;
@@ -658,6 +669,10 @@ static int pvr2_s_ext_ctrls(struct file *file, void *priv,
unsigned int idx;
int ret;
+ /* Default value cannot be changed */
+ if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
ret = 0;
for (idx = 0; idx < ctls->count; idx++) {
ctrl = ctls->controls + idx;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/10] media/v4l2-core: struct struct v4l2_ext_controls param which
Support for new field which on v4l2_ext_controls, used to get the
default value of one or more controls.
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 34 +-
1 file changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index 580098ba5c0b..8f1a5d3cbf7f 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1489,6 +1489,17 @@ static int new_to_user(struct v4l2_ext_control *c,
return ptr_to_user(c, ctrl, ctrl->p_new);
}
+/* Helper function: copy the initial control value back to the caller */
+static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
+{
+ int idx;
+
+ for (idx = 0; idx < ctrl->elems; idx++)
+ ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
+
+ return ptr_to_user(c, ctrl, ctrl->p_new);
+}
+
/* Helper function: copy the caller-provider value to the given control value
*/
static int user_to_ptr(struct v4l2_ext_control *c,
struct v4l2_ctrl *ctrl,
@@ -2708,7 +2719,9 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
*hdl,
cs->error_idx = i;
- if (cs->which && V4L2_CTRL_ID2WHICH(id) != cs->which)
+ if (cs->which &&
+ cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
+ V4L2_CTRL_ID2WHICH(id) != cs->which)
return -EINVAL;
/* Old-style private controls are not allowed for
@@ -2787,7 +2800,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
*hdl,
whether there are any controls at all. */
static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
{
- if (!which)
+ if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
}
@@ -2801,6 +2814,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
struct v4l2_ctrl_helper *helpers = helper;
int ret;
int i, j;
+ bool def_value;
+
+ def_value = (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
cs->error_idx = cs->count;
cs->which = V4L2_CTRL_ID2WHICH(cs->which);
@@ -2827,9 +2843,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
for (i = 0; !ret && i < cs->count; i++) {
int (*ctrl_to_user)(struct v4l2_ext_control *c,
- struct v4l2_ctrl *ctrl) = cur_to_user;
+ struct v4l2_ctrl *ctrl);
struct v4l2_ctrl *master;
+ ctrl_to_user = def_value ? def_to_user : cur_to_user;
+
if (helpers[i].mref == NULL)
continue;
@@ -2839,8 +2857,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
v4l2_ctrl_lock(master);
/* g_volatile_ctrl will update the new control values */
- if ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
- (master->has_volatiles && !is_cur_manual(master))) {
+ if (!def_value &&
+ ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
+ (master->has_volatiles && !is_cur_manual(master {
for (j = 0; j < master->ncontrols; j++)
cur_to_new(master->cluster[j]);
ret = call_op(master, g_volatile_ctrl);
@@ -3062,6 +3081,11 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct
v4l2_ctrl_handler *hdl,
int ret;
cs->error_idx = cs->count;
+
+ /* Default value cannot be changed */
+ if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
cs->which = V4L2_CTRL_ID2WHICH(cs->which);
if (hdl == NULL)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 08/10] media/pci/saa7164-encoder Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/pci/saa7164/saa7164-encoder.c | 55 -
1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/drivers/media/pci/saa7164/saa7164-encoder.c
b/drivers/media/pci/saa7164/saa7164-encoder.c
index e0ec64ed06fc..4a5c1ee3fb34 100644
--- a/drivers/media/pci/saa7164/saa7164-encoder.c
+++ b/drivers/media/pci/saa7164/saa7164-encoder.c
@@ -531,30 +531,6 @@ static int saa7164_get_ctrl(struct saa7164_port *port,
return 0;
}
-static int vidioc_g_ext_ctrls(struct file *file, void *priv,
- struct v4l2_ext_controls *ctrls)
-{
- struct saa7164_encoder_fh *fh = file->private_data;
- struct saa7164_port *port = fh->port;
- int i, err = 0;
-
- if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
- for (i = 0; i < ctrls->count; i++) {
- struct v4l2_ext_control *ctrl = ctrls->controls + i;
-
- err = saa7164_get_ctrl(port, ctrl);
- if (err) {
- ctrls->error_idx = i;
- break;
- }
- }
- return err;
-
- }
-
- return -EINVAL;
-}
-
static int saa7164_try_ctrl(struct v4l2_ext_control *ctrl, int ac3)
{
int ret = -EINVAL;
@@ -884,6 +860,37 @@ static int vidioc_queryctrl(struct file *file, void *priv,
return -EINVAL;
}
+static int vidioc_g_ext_ctrls(struct file *file, void *priv,
+ struct v4l2_ext_controls *ctrls)
+{
+ struct saa7164_encoder_fh *fh = file->private_data;
+ struct saa7164_port *port = fh->port;
+ int i, err = 0;
+
+ if (ctrls->which != V4L2_CTRL_CLASS_MPEG &&
+ ctrls->which != V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
+ for (i = 0; i < ctrls->count; i++) {
+ struct v4l2_ext_control *ctrl = ctrls->controls + i;
+
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ struct v4l2_queryctrl q;
+
+ err = fill_queryctrl(&port->encoder_params, &q);
+ if (!err)
+ ctrl->value = q.default_value;
+ } else
+ err = saa7164_get_ctrl(port, ctrl);
+
+ if (err) {
+ ctrls->error_idx = i;
+ break;
+ }
+ }
+ return err;
+}
+
static int saa7164_encoder_stop_port(struct saa7164_port *port)
{
struct saa7164_dev *dev = port->dev;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 10/10] Docbook: media: Document changes on struct v4l2_ext_controls
Vidioc-g-ext-ctrls can now be used to get the default value of the controls. Signed-off-by: Ricardo Ribalda Delgado --- Documentation/DocBook/media/v4l/v4l2.xml | 9 + .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 22 ++ 2 files changed, 31 insertions(+) diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml index e98caa1c39bd..be52bd2fb335 100644 --- a/Documentation/DocBook/media/v4l/v4l2.xml +++ b/Documentation/DocBook/media/v4l/v4l2.xml @@ -153,6 +153,15 @@ structs, ioctls) must be noted in more detail in the history chapter applications. --> + 4.4 + 2015-08-20 + rr + Extend vidioc-g-ext-ctrls;. Replace ctrl_class with a new +union with ctrl_class and which. Which is used to select the current value of +the control or the default value. + + + 3.21 2015-02-13 mcc diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index 8f4eb395665d..3b7bf68a8250 100644 --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml @@ -216,7 +216,12 @@ Valid if V4L2_CTRL_FLAG_HAS_PAYLOAD is set for this control &cs-str; + + union + (anonymous) + + __u32 ctrl_class The control class to which all controls belong, see @@ -228,6 +233,23 @@ with a count of 0. If that succeeds, then the driver supports this feature. + + __u32 + which + Which value of the control to get/set/try. V4L2_CTRL_WHICH_CUR_VAL +will return the current value of the control and V4L2_CTRL_WHICH_DEF_VAL will +return the default value of the control. Please note that you can only get the default value of the +control, you cannot set or try it. +For backwards compatibility you can also use a control class here (see +). In that case all controls have to belong to that +control class. This usage is deprecated, instead just use V4L2_CTRL_WHICH_CUR_VAL. +There are some very old drivers that do not yet support V4L2_CTRL_WHICH_CUR_VAL +and that require a control class here. You can test for such drivers by setting ctrl_class to +V4L2_CTRL_WHICH_CUR_VAL and calling VIDIOC_TRY_EXT_CTRLS with a count of 0. +If that fails, then the driver does not support V4L2_CTRL_WHICH_CUR_VAL. + + + __u32 count The number of controls in the controls array. May -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 09/10] media/pci/saa7164-vbi Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/pci/saa7164/saa7164-vbi.c | 57 +++--
1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/drivers/media/pci/saa7164/saa7164-vbi.c
b/drivers/media/pci/saa7164/saa7164-vbi.c
index 08f23b6d9cef..4c2d8ec5b4dd 100644
--- a/drivers/media/pci/saa7164/saa7164-vbi.c
+++ b/drivers/media/pci/saa7164/saa7164-vbi.c
@@ -494,30 +494,6 @@ static int saa7164_get_ctrl(struct saa7164_port *port,
return 0;
}
-static int vidioc_g_ext_ctrls(struct file *file, void *priv,
- struct v4l2_ext_controls *ctrls)
-{
- struct saa7164_vbi_fh *fh = file->private_data;
- struct saa7164_port *port = fh->port;
- int i, err = 0;
-
- if (ctrls->which == V4L2_CTRL_CLASS_MPEG) {
- for (i = 0; i < ctrls->count; i++) {
- struct v4l2_ext_control *ctrl = ctrls->controls + i;
-
- err = saa7164_get_ctrl(port, ctrl);
- if (err) {
- ctrls->error_idx = i;
- break;
- }
- }
- return err;
-
- }
-
- return -EINVAL;
-}
-
static int saa7164_try_ctrl(struct v4l2_ext_control *ctrl, int ac3)
{
int ret = -EINVAL;
@@ -810,6 +786,39 @@ static int vidioc_queryctrl(struct file *file, void *priv,
return -EINVAL;
}
+static int vidioc_g_ext_ctrls(struct file *file, void *priv,
+ struct v4l2_ext_controls *ctrls)
+{
+ struct saa7164_vbi_fh *fh = file->private_data;
+ struct saa7164_port *port = fh->port;
+ int i, err = 0;
+
+ if (ctrls->which != V4L2_CTRL_CLASS_MPEG &&
+ ctrls->which != V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
+ for (i = 0; i < ctrls->count; i++) {
+ struct v4l2_ext_control *ctrl = ctrls->controls + i;
+
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ struct v4l2_queryctrl q;
+
+ err = fill_queryctrl(&port->vbi_params, &q);
+ if (!err)
+ ctrl->value = q.default_value;
+ } else
+ err = saa7164_get_ctrl(port, ctrl);
+
+ if (err) {
+ ctrls->error_idx = i;
+ break;
+ }
+ }
+
+ return err;
+}
+
+
static int saa7164_vbi_stop_port(struct saa7164_port *port)
{
struct saa7164_dev *dev = port->dev;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/10] videodev2.h: Extend struct v4l2_ext_controls
So it can be used to get the default value of a control.
Without this change it is not possible to get get the
default value of array controls.
Signed-off-by: Ricardo Ribalda Delgado
---
include/uapi/linux/videodev2.h | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 72fa3e490e30..2e857b19a155 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1461,7 +1461,10 @@ struct v4l2_ext_control {
} __attribute__ ((packed));
struct v4l2_ext_controls {
- __u32 ctrl_class;
+ union {
+ __u32 ctrl_class;
+ __u32 which;
+ };
__u32 count;
__u32 error_idx;
__u32 reserved[2];
@@ -1472,6 +1475,8 @@ struct v4l2_ext_controls {
#define V4L2_CTRL_ID2CLASS(id)((id) & 0x0fffUL)
#define V4L2_CTRL_DRIVER_PRIV(id) (((id) & 0x) >= 0x1000)
#define V4L2_CTRL_MAX_DIMS (4)
+#define V4L2_CTRL_WHICH_CUR_VAL 0
+#define V4L2_CTRL_WHICH_DEF_VAL 0x0f00
enum v4l2_ctrl_type {
V4L2_CTRL_TYPE_INTEGER = 1,
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver
On Thu, Aug 20, 2015 at 05:51:39PM -0700, Bryan Wu wrote: > NVIDIA Tegra processor contains a powerful Video Input (VI) hardware > controller which can support up to 6 MIPI CSI camera sensors. > > This patch adds a V4L2 media controller and capture driver to support > Tegra VI hardware. It's verified with Tegra built-in test pattern > generator. Hi Bryan, I've been looking forward to seeing this posted. I don't know the VI hardware in very much detail, nor am I an expert on the media framework, so I will primarily comment on architectural or SoC-specific things. By the way, please always Cc linux-te...@vger.kernel.org on all patches relating to Tegra. That way people not explicitly Cc'ed but still interested in Tegra will see this code, even if they aren't subscribed to the linux-media mailing list. > Signed-off-by: Bryan Wu > Reviewed-by: Hans Verkuil > --- > drivers/media/platform/Kconfig |1 + > drivers/media/platform/Makefile |2 + > drivers/media/platform/tegra/Kconfig |9 + > drivers/media/platform/tegra/Makefile|3 + > drivers/media/platform/tegra/tegra-channel.c | 1074 > ++ > drivers/media/platform/tegra/tegra-core.c| 295 +++ > drivers/media/platform/tegra/tegra-core.h| 134 > drivers/media/platform/tegra/tegra-vi.c | 585 ++ > drivers/media/platform/tegra/tegra-vi.h | 224 ++ > include/dt-bindings/media/tegra-vi.h | 35 + > 10 files changed, 2362 insertions(+) > create mode 100644 drivers/media/platform/tegra/Kconfig > create mode 100644 drivers/media/platform/tegra/Makefile > create mode 100644 drivers/media/platform/tegra/tegra-channel.c > create mode 100644 drivers/media/platform/tegra/tegra-core.c > create mode 100644 drivers/media/platform/tegra/tegra-core.h > create mode 100644 drivers/media/platform/tegra/tegra-vi.c > create mode 100644 drivers/media/platform/tegra/tegra-vi.h > create mode 100644 include/dt-bindings/media/tegra-vi.h I can't spot a device tree binding document for this, but we'll need one to properly review this driver. > diff --git a/drivers/media/platform/tegra/Kconfig > b/drivers/media/platform/tegra/Kconfig > new file mode 100644 > index 000..a69d1b2 > --- /dev/null > +++ b/drivers/media/platform/tegra/Kconfig > @@ -0,0 +1,9 @@ > +config VIDEO_TEGRA > + tristate "NVIDIA Tegra Video Input Driver (EXPERIMENTAL)" I don't think the (EXPERIMENTAL) is warranted. Either the driver works or it doesn't. And I assume you already tested that it works, even if only using the TPG. > + depends on VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API && OF This seems to be missing a couple of dependencies. For example I would expect at least TEGRA_HOST1X to be listed here to make sure people can't select this when the host1x API isn't available. I would also expect some sort of architecture dependency because it really makes no sense to build this if Tegra isn't supported. If you are concerned about compile coverage you can make that explicit using a COMPILE_TEST alternative such as: depends on ARCH_TEGRA || (ARM && COMPILE_TEST) Note that the ARM dependency in there makes sure that HAVE_IOMEM is selected, so this could also be: depends on ARCH_TEGRA || (HAVE_IOMEM && COMPILE_TEST) though that'd still leave open the possibility of build breakage because of some missing support. If you add the dependency on TEGRA_HOST1X that I mentioned above you shouldn't need any architecture dependency because TEGRA_HOST1X implies those already. > + select VIDEOBUF2_DMA_CONTIG > + ---help--- > + Driver for Video Input (VI) device controller in NVIDIA Tegra SoC. I'd reword this slightly as: Driver for the Video Input (VI) controller found on NVIDIA Tegra SoCs. > + > + TO compile this driver as a module, choose M here: the module will be s/TO/To/. > + called tegra-video. > diff --git a/drivers/media/platform/tegra/Makefile > b/drivers/media/platform/tegra/Makefile > new file mode 100644 > index 000..c8eff0b > --- /dev/null > +++ b/drivers/media/platform/tegra/Makefile > @@ -0,0 +1,3 @@ > +tegra-video-objs += tegra-core.o tegra-vi.o tegra-channel.o I'd personally leave out the redundant tegra- prefix here, because the files are in a tegra/ subdirectory already. > +obj-$(CONFIG_VIDEO_TEGRA) += tegra-video.o > diff --git a/drivers/media/platform/tegra/tegra-channel.c > b/drivers/media/platform/tegra/tegra-channel.c > new file mode 100644 > index 000..b0063d2 > --- /dev/null > +++ b/drivers/media/platform/tegra/tegra-channel.c > @@ -0,0 +1,1074 @@ > +/* > + * NVIDIA Tegra Video Input Device > + * > + * Copyright (c) 2015, NVIDIA CORPORATION. All rights reserved. > + * > + * Author: Bryan Wu > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the
[linux-next:master 7080/10086] drivers/media/i2c/s5k4ecgx.c:346:2: warning: format '%zu' expects argument of type 'size_t', but argument 4 has type 'unsigned int'
tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: 1ef981bcd18de26dc78bc79f092d6f4bb25e0e8f
commit: 5cc596c66fe7c725ec049ae2093e7242034c97d6 [7080/10086] [media] i2c: Make
all i2c devices visible if COMPILE_TEST=y
config: mn10300-allyesconfig (attached as .config)
reproduce:
wget
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 5cc596c66fe7c725ec049ae2093e7242034c97d6
# save the attached .config to linux build tree
make.cross ARCH=mn10300
All warnings (new ones prefixed by >>):
drivers/media/i2c/s5k4ecgx.c: In function 's5k4ecgx_load_firmware':
>> drivers/media/i2c/s5k4ecgx.c:346:2: warning: format '%zu' expects argument
>> of type 'size_t', but argument 4 has type 'unsigned int' [-Wformat=]
v4l2_dbg(3, debug, sd, "FW: %s size %zu register sets %d\n",
^
--
drivers/media/i2c/s5k5baf.c: In function 's5k5baf_fw_parse':
>> drivers/media/i2c/s5k5baf.c:363:3: warning: format '%zu' expects argument of
>> type 'size_t', but argument 3 has type 'unsigned int' [-Wformat=]
dev_err(dev, "firmware file too short (%zu)\n", count);
^
drivers/media/i2c/s5k5baf.c:386:4: warning: format '%zu' expects argument of
type 'size_t', but argument 4 has type 'unsigned int' [-Wformat=]
f->count, 2 * (count + S5K5BAG_FW_TAG_LEN));
^
--
drivers/media/i2c/s5c73m3/s5c73m3-core.c: In function 's5c73m3_load_fw':
>> drivers/media/i2c/s5c73m3/s5c73m3-core.c:365:2: warning: format '%zu'
>> expects argument of type 'size_t', but argument 4 has type 'unsigned int'
>> [-Wformat=]
v4l2_info(sd, "Loading firmware (%s, %zu B)\n", fw_name, fw->size);
^
vim +346 drivers/media/i2c/s5k4ecgx.c
8b99312b Sangwook Lee 2012-09-13 330 static int s5k4ecgx_load_firmware(struct
v4l2_subdev *sd)
8b99312b Sangwook Lee 2012-09-13 331 {
8b99312b Sangwook Lee 2012-09-13 332 struct i2c_client *client =
v4l2_get_subdevdata(sd);
8b99312b Sangwook Lee 2012-09-13 333 const struct firmware *fw;
8b99312b Sangwook Lee 2012-09-13 334 const u8 *ptr;
8b99312b Sangwook Lee 2012-09-13 335 int err, i, regs_num;
8b99312b Sangwook Lee 2012-09-13 336 u32 addr, crc, crc_file, addr_inc = 0;
8b99312b Sangwook Lee 2012-09-13 337 u16 val;
8b99312b Sangwook Lee 2012-09-13 338
8b99312b Sangwook Lee 2012-09-13 339 err = request_firmware(&fw,
S5K4ECGX_FIRMWARE, sd->v4l2_dev->dev);
8b99312b Sangwook Lee 2012-09-13 340 if (err) {
8b99312b Sangwook Lee 2012-09-13 341 v4l2_err(sd, "Failed to read
firmware %s\n", S5K4ECGX_FIRMWARE);
8b99312b Sangwook Lee 2012-09-13 342 return err;
8b99312b Sangwook Lee 2012-09-13 343 }
b75f2d16 Hans Verkuil 2014-12-13 344 regs_num = get_unaligned_le32(fw->data);
8b99312b Sangwook Lee 2012-09-13 345
107376ba Randy Dunlap 2012-10-16 @346 v4l2_dbg(3, debug, sd, "FW: %s size %zu
register sets %d\n",
8b99312b Sangwook Lee 2012-09-13 347S5K4ECGX_FIRMWARE, fw->size,
regs_num);
8b99312b Sangwook Lee 2012-09-13 348
8b99312b Sangwook Lee 2012-09-13 349 regs_num++; /* Add header */
8b99312b Sangwook Lee 2012-09-13 350 if (fw->size != regs_num *
FW_RECORD_SIZE + FW_CRC_SIZE) {
8b99312b Sangwook Lee 2012-09-13 351 err = -EINVAL;
8b99312b Sangwook Lee 2012-09-13 352 goto fw_out;
8b99312b Sangwook Lee 2012-09-13 353 }
b75f2d16 Hans Verkuil 2014-12-13 354 crc_file = get_unaligned_le32(fw->data
+ regs_num * FW_RECORD_SIZE);
:: The code at line 346 was first introduced by commit
:: 107376ba4ef1ecd4249427a9f9c1922fc1d66bb4 [media] i2c/s5k4ecgx: fix
printk format warning
:: TO: Randy Dunlap
:: CC: Mauro Carvalho Chehab
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
#
# Automatically generated file; DO NOT EDIT.
# Linux/mn10300 4.2.0-rc2 Kernel Configuration
#
CONFIG_MN10300=y
CONFIG_AM33_2=y
# CONFIG_AM33_3 is not set
# CONFIG_AM34_2 is not set
CONFIG_MMU=y
# CONFIG_HIGHMEM is not set
# CONFIG_NUMA is not set
CONFIG_UID16=y
CONFIG_RWSEM_GENERIC_SPINLOCK=y
CONFIG_GENERIC_CALIBRATE_DELAY=y
CONFIG_GENERIC_HWEIGHT=y
CONFIG_GENERIC_BUG=y
CONFIG_QUICKLIST=y
CONFIG_ARCH_HAS_ILOG2_U32=y
# CONFIG_HOTPLUG_CPU is not set
CONFIG_DEFCONFIG_LIST="/lib/modules/$UNAME_RELEASE/.config"
CONFIG_CONSTRUCTORS=y
CONFIG_IRQ_WORK=y
#
# General setup
#
CONFIG_BROKEN_ON_SMP=y
CONFIG_INIT_ENV_ARG_LIMIT=32
CONFIG_CROSS_COMPILE=""
CONFIG_COMPILE_TEST=y
CONFIG_LOCALVERSION=""
CONFIG_LOCALVERSION_AUTO=y
CONFIG_DEFAULT_HOSTNAME="(none)"
CONFIG_SWAP=y
CONFIG_SYSVIPC=y
CONFIG_SYSVIPC_SYSCTL=y
CONFIG_POSIX_MQUEUE=y
CONFIG_POSIX_MQUEUE_SYSCTL=y
CONFIG_CROSS_MEMORY_ATTACH=y
CONFIG_FHANDLE=y
CONFIG_USELIB=y
CONFIG_AUDIT=y
#
# IRQ subsystem
#
CONFIG_GENERIC_IRQ_SHOW=y
CONFIG_IRQ_DOMAIN=y
CONFIG_IRQ_DOMAIN_DEBUG=y
CONFIG_GENERIC_CLOCKEVENTS=y
CONFIG_GENERIC
Re: [PATCH] v4l2-compat-ioctl32: fix struct v4l2_event32 alignment
On 08/21/2015 02:03 PM, Andrzej Hajda wrote:
> Union v4l2_event::u is aligned to 8 bytes on arm32. On arm64 v4l2_event32::u
> is aligned to 4 bytes. As a result structures v4l2_event and v4l2_event32 have
> different sizes and VIDOC_DQEVENT ioctl does not work from arm32 apps running
> on arm64 kernel. The patch fixes it. Using compat_s64 allows to retain 4 bytes
> alignment on x86 architecture.
What about v4l2_standard32 and v4l2_ext_control32? I very strongly suspect that
those will break for arm32 apps on an arm64 as well.
I prefer a patch that fixes all three...
Regards,
Hans
>
> Signed-off-by: Andrzej Hajda
> ---
> Hi Hans,
>
> Tested successfully on arm32 app / arm64 kernel.
> Thanks for help.
>
> Regards
> Andrzej
> ---
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index af63543..52afffe 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -738,6 +738,7 @@ static int put_v4l2_ext_controls32(struct
> v4l2_ext_controls *kp, struct v4l2_ext
> struct v4l2_event32 {
> __u32 type;
> union {
> + compat_s64 value64;
> __u8data[64];
> } u;
> __u32 pending;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] v4l2-compat-ioctl32: fix struct v4l2_event32 alignment
Union v4l2_event::u is aligned to 8 bytes on arm32. On arm64 v4l2_event32::u
is aligned to 4 bytes. As a result structures v4l2_event and v4l2_event32 have
different sizes and VIDOC_DQEVENT ioctl does not work from arm32 apps running
on arm64 kernel. The patch fixes it. Using compat_s64 allows to retain 4 bytes
alignment on x86 architecture.
Signed-off-by: Andrzej Hajda
---
Hi Hans,
Tested successfully on arm32 app / arm64 kernel.
Thanks for help.
Regards
Andrzej
---
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index af63543..52afffe 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -738,6 +738,7 @@ static int put_v4l2_ext_controls32(struct v4l2_ext_controls
*kp, struct v4l2_ext
struct v4l2_event32 {
__u32 type;
union {
+ compat_s64 value64;
__u8data[64];
} u;
__u32 pending;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[linux-next:master 7079/10086] drivers/media/dvb-frontends/or51132.c:140:2: warning: format '%Zd' expects argument of type 'signed size_t', but argument 2 has type 'size_t'
tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: 1ef981bcd18de26dc78bc79f092d6f4bb25e0e8f
commit: 6df34051d8bd8807c873e1bb92b905e370ff3f5b [7079/10086] [media]
dvb-frontends: Make all DVB Frontends visible if COMPILE_TEST=y
config: mn10300-allyesconfig (attached as .config)
reproduce:
wget
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 6df34051d8bd8807c873e1bb92b905e370ff3f5b
# save the attached .config to linux build tree
make.cross ARCH=mn10300
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:277:0,
from include/linux/kernel.h:13,
from drivers/media/dvb-frontends/or51211.c:35:
drivers/media/dvb-frontends/or51211.c: In function 'or51211_load_firmware':
include/linux/dynamic_debug.h:64:16: warning: format '%zu' expects argument
of type 'size_t', but argument 4 has type 'unsigned int' [-Wformat=]
static struct _ddebug __aligned(8) \
^
include/linux/dynamic_debug.h:76:2: note: in expansion of macro
'DEFINE_DYNAMIC_DEBUG_METADATA'
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
^
include/linux/printk.h:283:2: note: in expansion of macro 'dynamic_pr_debug'
dynamic_pr_debug(fmt, ##__VA_ARGS__)
^
>> drivers/media/dvb-frontends/or51211.c:49:18: note: in expansion of macro
>> 'pr_debug'
do { if (debug) pr_debug(args); } while (0)
^
>> drivers/media/dvb-frontends/or51211.c:114:2: note: in expansion of macro
>> 'dprintk'
dprintk("Firmware is %zu bytes\n", fw->size);
^
--
drivers/media/dvb-frontends/or51132.c: In function 'or51132_load_firmware':
>> drivers/media/dvb-frontends/or51132.c:140:2: warning: format '%Zd' expects
>> argument of type 'signed size_t', but argument 2 has type 'size_t'
>> [-Wformat=]
dprintk("Firmware is %Zd bytes\n",fw->size);
^
--
In file included from include/linux/printk.h:277:0,
from include/linux/kernel.h:13,
from include/linux/list.h:8,
from include/linux/preempt.h:10,
from include/linux/spinlock.h:50,
from include/linux/mmzone.h:7,
from include/linux/gfp.h:5,
from include/linux/slab.h:14,
from drivers/media/dvb-frontends/cx24117.c:27:
drivers/media/dvb-frontends/cx24117.c: In function 'cx24117_load_firmware':
include/linux/dynamic_debug.h:64:16: warning: format '%zu' expects argument
of type 'size_t', but argument 6 has type 'unsigned int' [-Wformat=]
static struct _ddebug __aligned(8) \
^
include/linux/dynamic_debug.h:84:2: note: in expansion of macro
'DEFINE_DYNAMIC_DEBUG_METADATA'
DEFINE_DYNAMIC_DEBUG_METADATA(descriptor, fmt); \
^
include/linux/device.h:1146:2: note: in expansion of macro 'dynamic_dev_dbg'
dynamic_dev_dbg(dev, format, ##__VA_ARGS__); \
^
>> drivers/media/dvb-frontends/cx24117.c:562:2: note: in expansion of macro
>> 'dev_dbg'
dev_dbg(&state->priv->i2c->dev,
^
vim +140 drivers/media/dvb-frontends/or51132.c
d9e54324 drivers/media/dvb/frontends/or51132.c Trent Piepho2007-03-02
124 if ((err = i2c_transfer(state->i2c, msg, 2)) != 2) {
d9e54324 drivers/media/dvb/frontends/or51132.c Trent Piepho2007-03-02
125 printk(KERN_WARNING "or51132: I2C error reading register %d:
%d\n",
d9e54324 drivers/media/dvb/frontends/or51132.c Trent Piepho2007-03-02
126 reg, err);
d9e54324 drivers/media/dvb/frontends/or51132.c Trent Piepho2007-03-02
127 return -EREMOTEIO;
d9e54324 drivers/media/dvb/frontends/or51132.c Trent Piepho2007-03-02
128 }
18dcd55a drivers/media/dvb/frontends/or51132.c Al Viro 2008-05-21
129 return buf[0] | (buf[1] << 8);
d9e54324 drivers/media/dvb/frontends/or51132.c Trent Piepho2007-03-02
130 }
d9e54324 drivers/media/dvb/frontends/or51132.c Trent Piepho2007-03-02
131
^1da177e drivers/media/dvb/frontends/or51132.c Linus Torvalds 2005-04-16
132 static int or51132_load_firmware (struct dvb_frontend* fe, const struct
firmware *fw)
^1da177e drivers/media/dvb/frontends/or51132.c Linus Torvalds 2005-04-16
133 {
b8742700 drivers/media/dvb/frontends/or51132.c Johannes Stezenbach 2005-05-16
134 struct or51132_state* state = fe->demodulator_priv;
cbfa6f2a drivers/media/dvb/frontends/or51132.c Tobias Klauser 2008-04-21
135 static const u8 run_buf[] = {0x7F,0x01};
0ead0918 drivers/media/dvb/frontends/or51132.c Trent Piepho2006-04-02
136 u8 rec_buf[8];
^1da177e drivers/media/dvb/frontends/or51132.c Linus Torvalds 2005-04-16
137 u32 firmwareAsize, firmwareBsize;
^1da177e drivers/media/dvb/frontends/or
Re: [PATCH 8/8] Docbook: media: Document changes on struct v4l2_ext_controls
On 08/21/2015 11:29 AM, Ricardo Ribalda Delgado wrote: > Vidioc-g-ext-ctrls can now be used to get the default value of the > controls. > > Signed-off-by: Ricardo Ribalda Delgado > --- > Documentation/DocBook/media/v4l/v4l2.xml | 9 + > Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 14 ++ > 2 files changed, 23 insertions(+) > > diff --git a/Documentation/DocBook/media/v4l/v4l2.xml > b/Documentation/DocBook/media/v4l/v4l2.xml > index e98caa1c39bd..be52bd2fb335 100644 > --- a/Documentation/DocBook/media/v4l/v4l2.xml > +++ b/Documentation/DocBook/media/v4l/v4l2.xml > @@ -153,6 +153,15 @@ structs, ioctls) must be noted in more detail in the > history chapter > applications. --> > > > + 4.4 > + 2015-08-20 > + rr > + Extend vidioc-g-ext-ctrls;. Replace ctrl_class with a new > +union with ctrl_class and which. Which is used to select the current value of > +the control or the default value. > + > + > + > 3.21 > 2015-02-13 > mcc > diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml > b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml > index c5bdbfcc42b3..224fa2bd1481 100644 > --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml > +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml > @@ -216,7 +216,12 @@ Valid if V4L2_CTRL_FLAG_HAS_PAYLOAD > is set for this control > > &cs-str; > > + > + union > + (anonymous) > + > > + > __u32 > ctrl_class > The control class to which all controls belong, see > @@ -228,6 +233,15 @@ with a count of 0. If that > succeeds, then the driver > supports this feature. All I would say here is that ctrl_class is an alias for 'which', kept for backwards compatibility. Applications should use 'which' instead. > > > + > + __u32 > + which > + Which control are get/set/tried. > V4L2_CTRL_WHICH_CUR_VAL I'd say: "Which value of the control to get/set/try." > +will return the current value of the control and > V4L2_CTRL_WHICH_DEF_VAL will > +return the default value of the control. Please note that the default value > of the control cannot > +be set or tried, only get. I'd rephrase that: "Please note that you can only get the default value of the control, you cannot set or try it." Add this: "For backwards compatibility you can also use a control class here (see . In that case all controls have to belong to that control class. This usage is deprecated, instead just use V4L2_CTRL_WHICH_CUR_VAL. There are some very old drivers that do not yet support V4L2_CTRL_WHICH_CUR_VAL and that require a control class here. You can test for such drivers by setting ctrl_class to V4L2_CTRL_WHICH_CUR_VAL and calling VIDIOC_TRY_EXT_CTRLS with a count of 0. If that fails, then the driver does not support V4L2_CTRL_WHICH_CUR_VAL." I think the only driver that still doesn't support this is saa7164. I really need to convert it to the control framework. Regards, Hans > + > + > __u32 > count > The number of controls in the controls array. May > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH] v4l2-compat-ioctl32: fix struct v4l2_event32 alignment
On 08/21/2015 11:50 AM, Andrzej Hajda wrote:
> Union v4l2_event::u is aligned to 8 bytes on arm32. On arm64 v4l2_event32::u
> is aligned to 4 bytes. As a result structures v4l2_event and v4l2_event32 have
> different sizes and VIDOC_DQEVENT ioctl does not work from arm32 apps running
> on arm64 kernel. The patch fixes it.
>
> Signed-off-by: Andrzej Hajda
> ---
> Hi Hans,
>
> It seems there is problem with VIDIOC_DQEVENT called from arm32 apps on arm64
> kernel. After tracking down the issue it seems v4l2_event32 on arm64 have
> different field alignment/size than v4l2_event on arm32. The patch fixes it.
> But i guess it can break ABI on other architectures. Simple tests shows:
>
> i386:
> sizeof(struct v4l2_event)=0x78
> offsetof(struct v4l2_event::u)=0x4
>
> amd64:
> sizeof(struct v4l2_event)=0x88
> offsetof(struct v4l2_event::u)=0x8
>
> arm:
> sizeof(struct v4l2_event)=0x80
> offsetof(struct v4l2_event::u)=0x8
>
> arm64:
> sizeof(struct v4l2_event)=0x88
> offsetof(struct v4l2_event::u)=0x8
>
> Any advices how to fix it in arch compatible way?
I noticed a compat_s64 type that knows about the 4-byte alignment on i386.
So I think this can be solved by declaring v4l2_event32 as follows:
struct v4l2_event32 {
__u32 type;
union {
compat_s64 value64;
__u8data[64];
} u;
__u32 pending;
__u32 sequence;
struct compat_timespec timestamp;
__u32 id;
__u32 reserved[8];
};
I think that this will force the correct alignment. Can you check this?
Good catch BTW.
Looking at v4l2-compat-ioctl32.c I suspect that compat_u/s64 will need to be
used in more places: v4l2_standard32 (replace the id[2] array by compat_u64 id)
and v4l2_ext_control32 are both almost certainly wrong for arm64. I would check
those two.
Regards,
Hans
>
> Regards
> Andrzej
> ---
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index af63543..a4a1856 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -739,7 +739,7 @@ struct v4l2_event32 {
> __u32 type;
> union {
> __u8data[64];
> - } u;
> + } u __attribute__((aligned(8)));
> __u32 pending;
> __u32 sequence;
> struct compat_timespec timestamp;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 6/8] [media] media: add messages when media device gets (un)registered
Em Fri, 21 Aug 2015 04:35:04 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Wednesday 19 August 2015 08:01:53 Mauro Carvalho Chehab wrote:
> > We can only free the media device after being sure that no
> > graph object is used.
>
> media_device_release() is currently broken as it should call back to the
> driver that has allocated the media_device() structure. I think we should fix
> that before adding more code on top of the problem.
Either that or move all allocations to happen via some MC function,
but this is out of the scope of this patch.
Whatever way we fix it, it is important to know when mdev
is supposed to cease to exist, and if this call happens
before or after the removal of all objects from the graph.
>
> > In order to help tracking it, let's add debug messages
> > that will print when the media controller gets registered
> > or unregistered.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 065f6f08da37..0f3844470147 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -359,6 +359,7 @@ static DEVICE_ATTR(model, S_IRUGO, show_model, NULL);
> >
> > static void media_device_release(struct media_devnode *mdev)
> > {
> > + dev_dbg(mdev->parent, "Media device released\n");
>
> As commented on patch 7/8, ftrace is a better candidate for function tracing.
Again, the problem here is to compare this with OOPs and other debug
messages, to see if the release is happening at the proper place.
Ftrace is not meant to be used for OOPS debug, nor it provides data
to the console, as it is not designed to be used for that purpose.
>
> > }
> >
> > /**
> > @@ -397,6 +398,8 @@ int __must_check __media_device_register(struct
> > media_device *mdev, return ret;
> > }
> >
> > + dev_dbg(mdev->dev, "Media device registered\n");
> > +
> > return 0;
> > }
> > EXPORT_SYMBOL_GPL(__media_device_register);
> > @@ -416,6 +419,8 @@ void media_device_unregister(struct media_device *mdev)
> >
> > device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> > media_devnode_unregister(&mdev->devnode);
> > +
> > + dev_dbg(mdev->dev, "Media device unregistered\n");
> > }
> > EXPORT_SYMBOL_GPL(media_device_unregister);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: WARNING: CPU: 1 PID: 813 at kernel/module.c:291 module_assert_mutex_or_preempt+0x49/0x90()
On 10.08.2015 23:26, poma wrote: > > [ cut here ] > WARNING: CPU: 1 PID: 813 at kernel/module.c:291 > module_assert_mutex_or_preempt+0x49/0x90() > Modules linked in: mxl5007t af9013 ... dvb_usb_af9015(+) ... dvb_usb_v2 > dvb_core rc_core ... > CPU: 1 PID: 813 Comm: systemd-udevd Not tainted > 4.2.0-0.rc6.git0.1.fc24.x86_64+debug #1 > ... > Call Trace: > [] dump_stack+0x4c/0x65 > [] warn_slowpath_common+0x86/0xc0 > [] ? af9013_read_ucblocks+0x20/0x20 [af9013] > [] ? af9013_read_ucblocks+0x20/0x20 [af9013] > [] warn_slowpath_null+0x1a/0x20 > [] module_assert_mutex_or_preempt+0x49/0x90 > [] __module_address+0x32/0x150 > [] ? af9013_read_ucblocks+0x20/0x20 [af9013] > [] ? af9013_read_ucblocks+0x20/0x20 [af9013] > [] __module_text_address+0x16/0x70 > [] ? af9013_read_ucblocks+0x20/0x20 [af9013] > [] ? af9013_read_ucblocks+0x20/0x20 [af9013] > [] symbol_put_addr+0x29/0x40 > [] dvb_frontend_detach+0x7d/0x90 [dvb_core] > [] dvb_usbv2_probe+0xc85/0x11a0 [dvb_usb_v2] > [] af9015_probe+0x84/0xf0 [dvb_usb_af9015] > [] usb_probe_interface+0x1bb/0x2e0 > [] driver_probe_device+0x1f6/0x450 > [] __driver_attach+0x94/0xa0 > [] ? driver_probe_device+0x450/0x450 > [] bus_for_each_dev+0x73/0xc0 > [] driver_attach+0x1e/0x20 > [] bus_add_driver+0x1ee/0x280 > [] driver_register+0x60/0xe0 > [] usb_register_driver+0xad/0x160 > [] ? 0xa0567000 > [] af9015_usb_driver_init+0x1e/0x1000 [dvb_usb_af9015] > [] do_one_initcall+0xb3/0x200 > [] ? kmem_cache_alloc_trace+0x355/0x380 > [] ? do_init_module+0x28/0x1e9 > [] do_init_module+0x60/0x1e9 > [] load_module+0x21f7/0x28d0 > [] ? m_show+0x1b0/0x1b0 > [] ? sched_clock+0x9/0x10 > [] ? local_clock+0x1c/0x20 > [] SyS_init_module+0x178/0x1c0 > [] entry_SYSCALL_64_fastpath+0x12/0x76 > ---[ end trace 31a9dd90d4f559f5 ]--- > > > Ref. > https://bugzilla.redhat.com/show_bug.cgi?id=1252167 > https://bugzilla.kernel.org/show_bug.cgi?id=102631 > You guys are really something. First of all, as is evident here, I am the original reporter, not Laura Abbott . -All- your comments including the final patch on this issue, are just plain wrong.[1] This patch only hides the actual problem with this particular device - the dual tuner combination driven by dvb_usb_af9015 & mxl5007t, broken by design since day one. Read the entire https://bugzilla.redhat.com/show_bug.cgi?id=1252167 and stop this nonsense. NACK "module: Fix locking in symbol_put_addr()" [1] http://www.gossamer-threads.com/lists/linux/kernel/2241154 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 7/8] [media] media: add a debug message to warn about gobj creation/removal
Em Fri, 21 Aug 2015 04:32:51 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Wednesday 19 August 2015 08:01:54 Mauro Carvalho Chehab wrote:
> > It helps to check if the media controller is doing the
> > right thing with the object creation and removal.
> >
> > No extra code/data will be produced if DEBUG or
> > CONFIG_DYNAMIC_DEBUG is not enabled.
>
> CONFIG_DYNAMIC_DEBUG is often enabled.
True, but once a driver/core is properly debugged, images without DEBUG
could be used in production, if the amount of memory constraints are
too tight.
> You're more or less adding function call tracing in this patch, isn't that
> something that ftrace is supposed to do ?
Ftrace is a great infrastructure and helps a lot when we need to
identify bottlenecks and other performance related stuff, but it
doesn't replace debug functions.
There are some fundamental differences on what you could do with ftrace
and what you can't.
At least on this stage, what I need is something that will provide
output via serial console when the driver gets loaded, and that provides
a synchronous output with the other Kernel messages.
This is the only way to debug certain OOPSes that are happening during
the development of the patches.
This is something you cannot do with ftrace, but dynamic DEBUG works
like a charm.
>
> > Signed-off-by: Mauro Carvalho Chehab
> >
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index 36d725ec5f3d..6d515e149d7f 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -27,6 +27,69 @@
> > #include
> >
> > /**
> > + * dev_dbg_obj - Prints in debug mode a change on some object
> > + *
> > + * @event_name:Name of the event to report. Could be __func__
> > + * @gobj: Pointer to the object
> > + *
> > + * Enabled only if DEBUG or CONFIG_DYNAMIC_DEBUG. Otherwise, it
> > + * won't produce any code.
> > + */
> > +static inline const char *gobj_type(enum media_gobj_type type)
> > +{
> > + switch (type) {
> > + case MEDIA_GRAPH_ENTITY:
> > + return "entity";
> > + case MEDIA_GRAPH_PAD:
> > + return "pad";
> > + case MEDIA_GRAPH_LINK:
> > + return "link";
> > + default:
> > + return "unknown";
> > + }
> > +}
> > +
> > +static void dev_dbg_obj(const char *event_name, struct media_gobj *gobj)
> > +{
> > +#if defined(DEBUG) || defined (CONFIG_DYNAMIC_DEBUG)
> > + switch (media_type(gobj)) {
> > + case MEDIA_GRAPH_ENTITY:
> > + dev_dbg(gobj->mdev->dev,
> > + "%s: id 0x%08x entity#%d: '%s'\n",
> > + event_name, gobj->id, media_localid(gobj),
> > + gobj_to_entity(gobj)->name);
> > + break;
> > + case MEDIA_GRAPH_LINK:
> > + {
> > + struct media_link *link = gobj_to_link(gobj);
> > +
> > + dev_dbg(gobj->mdev->dev,
> > + "%s: id 0x%08x link#%d: '%s' %s#%d ==> '%s' %s#%d\n",
> > + event_name, gobj->id, media_localid(gobj),
> > +
> > + link->source->entity->name,
> > + gobj_type(media_type(&link->source->graph_obj)),
> > + media_localid(&link->source->graph_obj),
> > +
> > + link->sink->entity->name,
> > + gobj_type(media_type(&link->sink->graph_obj)),
> > + media_localid(&link->sink->graph_obj));
> > + break;
> > + }
> > + case MEDIA_GRAPH_PAD:
> > + {
> > + struct media_pad *pad = gobj_to_pad(gobj);
> > +
> > + dev_dbg(gobj->mdev->dev,
> > + "%s: id 0x%08x pad#%d: '%s':%d\n",
> > + event_name, gobj->id, media_localid(gobj),
> > + pad->entity->name, pad->index);
> > + }
> > + }
> > +#endif
> > +}
> > +
> > +/**
> > * media_gobj_init - Initialize a graph object
> > *
> > * @mdev: Pointer to the media_device that contains the object
> > @@ -43,6 +106,8 @@ void media_gobj_init(struct media_device *mdev,
> >enum media_gobj_type type,
> >struct media_gobj *gobj)
> > {
> > + gobj->mdev = mdev;
> > +
> > /* Create a per-type unique object ID */
> > switch (type) {
> > case MEDIA_GRAPH_ENTITY:
> > @@ -55,6 +120,7 @@ void media_gobj_init(struct media_device *mdev,
> > gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
> > break;
> > }
> > + dev_dbg_obj(__func__, gobj);
> > }
> >
> > /**
> > @@ -66,7 +132,7 @@ void media_gobj_init(struct media_device *mdev,
> > */
> > void media_gobj_remove(struct media_gobj *gobj)
> > {
> > - /* For now, nothing to do */
> > + dev_dbg_obj(__func__, gobj);
> > }
> >
> > /**
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 749b46c91217..bdd5e565ae76 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -61,6 +6
Re: [PATCH 3/8] media/v4l2-core: struct struct v4l2_ext_controls param which
On 08/21/2015 11:29 AM, Ricardo Ribalda Delgado wrote:
> Support for new field which on v4l2_ext_controls, used to get the
> default value of one or more controls.
>
> Signed-off-by: Ricardo Ribalda Delgado
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 35 ++-
> 1 file changed, 30 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b6b7dcc1b77d..23a69f637f6d 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1489,6 +1489,17 @@ static int new_to_user(struct v4l2_ext_control *c,
> return ptr_to_user(c, ctrl, ctrl->p_new);
> }
>
> +/* Helper function: copy the initial control value back to the caller */
> +static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
> +{
> + int idx;
> +
> + for (idx = 0; idx < ctrl->elems; idx++)
> + ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> +
> + return ptr_to_user(c, ctrl, ctrl->p_new);
> +}
> +
> /* Helper function: copy the caller-provider value to the given control
> value */
> static int user_to_ptr(struct v4l2_ext_control *c,
> struct v4l2_ctrl *ctrl,
> @@ -2708,7 +2719,9 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
> *hdl,
>
> cs->error_idx = i;
>
> - if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
> + if (cs->ctrl_class &&
> + cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
> + V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
> return -EINVAL;
>
> /* Old-style private controls are not allowed for
> @@ -2787,7 +2800,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
> *hdl,
> whether there are any controls at all. */
> static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
> {
> - if (ctrl_class == 0)
> + if (ctrl_class == 0 || ctrl_class == V4L2_CTRL_WHICH_DEF_VAL)
> return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
> return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
> }
> @@ -2801,10 +2814,14 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> struct v4l2_ext_controls *cs
> struct v4l2_ctrl_helper *helpers = helper;
> int ret;
> int i, j;
> + bool def_value = false;
>
> cs->error_idx = cs->count;
> cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
>
> + if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
> + def_value = true;
> +
Ah, this is confusing. First assigning to ctrl_class, then checking 'which'.
I would add a patch after patch 2/8 that replaces all occurrences of ctrl_class
in
by 'which'. It's only used in v4l2-core, the saa7164 driver and in
Documentation.
The old 'ctrl_class' shouldn't be used in the kernel anymore.
It is probably a good idea to put #ifndef __KERNEL__ around the ctrl_class
field in
the header. That way it isn't visible in the kernel at all.
I would also rename V4L2_CTRL_ID2CLASS to V4L2_CTRL_ID2WHICH (and keep the old
define
as #define V4L2_CTRL_ID2CLASS V4L2_CTRL_ID2WHICH under #ifndef __KERNEL__).
> if (hdl == NULL)
> return -EINVAL;
>
> @@ -2827,9 +2844,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> struct v4l2_ext_controls *cs
>
> for (i = 0; !ret && i < cs->count; i++) {
> int (*ctrl_to_user)(struct v4l2_ext_control *c,
> - struct v4l2_ctrl *ctrl) = cur_to_user;
> + struct v4l2_ctrl *ctrl);
> struct v4l2_ctrl *master;
>
> + ctrl_to_user = def_value ? def_to_user : cur_to_user;
> +
> if (helpers[i].mref == NULL)
> continue;
>
> @@ -2839,8 +2858,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
> struct v4l2_ext_controls *cs
> v4l2_ctrl_lock(master);
>
> /* g_volatile_ctrl will update the new control values */
> - if ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
> - (master->has_volatiles && !is_cur_manual(master))) {
> + if (!def_value &&
> + ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
> + (master->has_volatiles && !is_cur_manual(master {
> for (j = 0; j < master->ncontrols; j++)
> cur_to_new(master->cluster[j]);
> ret = call_op(master, g_volatile_ctrl);
> @@ -3062,6 +3082,11 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
> struct v4l2_ctrl_handler *hdl,
> int ret;
>
> cs->error_idx = cs->count;
> +
> + /* Default value cannot be changed */
> + if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
> + return -EINVAL;
> +
> cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
>
> if (hdl == NULL)
>
Regards,
Hans
--
Re: [PATCH v6 3/8] [media] media: use media_gobj inside entities
Em Fri, 21 Aug 2015 04:10:03 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Wednesday 19 August 2015 08:01:50 Mauro Carvalho Chehab wrote:
> > As entities are graph elements, let's embed media_gobj
> > on it. That ensures an unique ID for entities that can be
> > global along the entire media controller.
> >
> > For now, we'll keep the already existing entity ID. Such
> > field need to be dropped at some point, but for now, let's
> > not do this, to avoid needing to review all drivers and
> > the userspace apps.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > Acked-by: Hans Verkuil
> > Signed-off-by: Mauro Carvalho Chehab
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index e429605ca2c3..81d6a130efef 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -379,7 +379,6 @@ int __must_check __media_device_register(struct
> > media_device *mdev, if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
> > return -EINVAL;
> >
> > - mdev->entity_id = 1;
> > INIT_LIST_HEAD(&mdev->entities);
> > spin_lock_init(&mdev->lock);
> > mutex_init(&mdev->graph_mutex);
> > @@ -433,10 +432,8 @@ int __must_check media_device_register_entity(struct
> > media_device *mdev, entity->parent = mdev;
> >
> > spin_lock(&mdev->lock);
> > - if (entity->id == 0)
> > - entity->id = mdev->entity_id++;
> > - else
> > - mdev->entity_id = max(entity->id + 1, mdev->entity_id);
> > + /* Initialize media_gobj embedded at the entity */
> > + media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
>
> Graph object initialization should be moved to media_entity_init() to keep
> initialization separate from registration.
Won't work. I tried. My first RFC patches were doing that.
The problem is that media_entity_init() is currently called too early
at the V4L2 drivers, before having mdev assigned.
Also, objects without PADs don't call media_entity_init(). At long
term, this function may even disappear, as it only exists today to
allocate arrays for pads/links. If we get rid of the pads array, it
will make no sense to keep it.
> > list_add_tail(&entity->list, &mdev->entities);
> > spin_unlock(&mdev->lock);
> >
> > @@ -459,6 +456,7 @@ void media_device_unregister_entity(struct media_entity
> > *entity) return;
> >
> > spin_lock(&mdev->lock);
> > + media_gobj_remove(&entity->graph_obj);
> > list_del(&entity->list);
> > spin_unlock(&mdev->lock);
> > entity->parent = NULL;
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index 4834172bf6f8..888cb88e19bf 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -43,7 +43,12 @@ void media_gobj_init(struct media_device *mdev,
> >enum media_gobj_type type,
> >struct media_gobj *gobj)
> > {
> > - /* For now, nothing to do */
> > + /* Create a per-type unique object ID */
> > + switch (type) {
> > + case MEDIA_GRAPH_ENTITY:
> > + gobj->id = media_gobj_gen_id(type, ++mdev->entity_id);
> > + break;
> > + }
> > }
> >
> > /**
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index a44f18fdf321..f6deef6e5820 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -41,7 +41,7 @@ struct device;
> > * @bus_info: Unique and stable device location identifier
> > * @hw_revision: Hardware device revision
> > * @driver_version: Device driver version
> > - * @entity_id: ID of the next entity to be registered
> > + * @entity_id: Unique ID used on the last entity registered
> > * @entities: List of registered entities
> > * @lock: Entities list lock
> > * @graph_mutex: Entities graph operation lock
> > @@ -69,6 +69,7 @@ struct media_device {
> > u32 driver_version;
> >
> > u32 entity_id;
> > +
> > struct list_head entities;
> >
> > /* Protects the entities list */
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index c1cd4fba051d..9ca366334bcf 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -33,10 +33,10 @@
> > /**
> > * enum media_gobj_type - type of a graph element
> > *
> > + * @MEDIA_GRAPH_ENTITY:Identify a media entity
>
> I think we should explicitly define here what an entity is.
I'm happy to add a new text here if you have a better idea.
Anyway, for now, I'm more concerned on getting things done than to spend
lots of time with the comments. As pointed on patch 1/8, while we're
changing a lot the code, those comments tend to become obsolete very
quick. The comments there is more like a boilerplate, as, once we finish
touching at the core, it makes sense to review the comments at media *.h
files, converting all descriptions to kernel-doc-nano format
Re: [PATCH v6 2/8] [media] media: add a common struct to be embed on media graph objects
Em Fri, 21 Aug 2015 04:02:57 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Wednesday 19 August 2015 08:01:49 Mauro Carvalho Chehab wrote:
> > Due to the MC API proposed changes, we'll need to have an unique
> > object ID for all graph objects, and have some shared fields
> > that will be common on all media graph objects.
> >
> > Right now, the only common object is the object ID, but other
> > fields will be added later on.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> >
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index cb0ac4e0dfa5..4834172bf6f8 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -27,6 +27,38 @@
> > #include
> >
> > /**
> > + * media_gobj_init - Initialize a graph object
> > + *
> > + * @mdev: Pointer to the media_device that contains the object
> > + * @type: Type of the object
> > + * @gobj: Pointer to the object
> > + *
> > + * This routine initializes the embedded struct media_gobj inside a
> > + * media graph object. It is called automatically if media_*_create()
> > + * calls are used. However, if the object (entity, link, pad, interface)
> > + * is embedded on some other object, this function should be called before
> > + * registering the object at the media controller.
Actually, the comment here is a little bit outdated, from the time I was
adding a kref inside the graph_obj. That's by the way one of the problems
with patches that suffer lots of transforms: comments may need to be
adjusted to reflect what changed, but tracking such changes is harder.
The comment above should be, instead:
* This routine initializes the embedded struct media_gobj inside a
* media graph object. It is called automatically at media_*_create()
* and at media_entity_register().
> Allowing both dynamic allocation and embedding will create a more complex API
> with more opportunities for drivers to get it wrong. I'd like to try and
> standardize the expected behaviour.
Let's discuss dynamic allocation when we come with the patches. For
now, if media_obj_init is not called, objects won't have an unique
ID and will cause an OOPS if debug is enabled, with is easily tracked.
>
> > + */
> > +void media_gobj_init(struct media_device *mdev,
> > + enum media_gobj_type type,
> > + struct media_gobj *gobj)
> > +{
> > + /* For now, nothing to do */
> > +}
> > +
> > +/**
> > + * media_gobj_remove - Stop using a graph object on a media device
>
> Is this function supposed to be the counterpart of media_gobj_init ? If so it
> should be called media_gobj_cleanup instead.
*_cleanup doesn't mean anything for me. The usual Kernel way is to use either
*_init and *_remove or *_register/*_remove for a kernel "object"
creation/removal.
If you prefer, I could rename the first function to media_gobj_register.
>
> > + *
> > + * @graph_obj: Pointer to the object
>
> The parameter is called gobj. Could you compile the kerneldoc to ensure that
> such typos get caught ?
I will fix at a next version and compile kerneldoc.
>
> > + *
> > + * This should be called at media_device_unregister_*() routines
> > + */
> > +void media_gobj_remove(struct media_gobj *gobj)
> > +{
> > + /* For now, nothing to do */
> > +}
> > +
> > +/**
> > * media_entity_init - Initialize a media entity
> > *
> > * @num_pads: Total number of sink and source pads.
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 0a66fc225559..c1cd4fba051d 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -28,6 +28,39 @@
> > #include
> > #include
> >
> > +/* Enums used internally at the media controller to represent graphs */
> > +
> > +/**
> > + * enum media_gobj_type - type of a graph element
>
> Let's try to standardize the vocabulary, should it be called graph object or
> graph element ? In the first case let's document it as graph object. In the
> second case it would be more consistent to refer to it as enum
> media_gelem_type (and struct media_gelem below).
graph object. Thanks for noticing it. I'll fix.
>
> > + *
> > + */
> > +enum media_gobj_type {
> > +/* FIXME: add the types here, as we embed media_gobj */
> > + MEDIA_GRAPH_NONE
> > +};
> > +
> > +#define MEDIA_BITS_PER_TYPE8
> > +#define MEDIA_BITS_PER_LOCAL_ID(32 - MEDIA_BITS_PER_TYPE)
> > +#define MEDIA_LOCAL_ID_MASK
> > GENMASK(MEDIA_BITS_PER_LOCAL_ID - 1, 0)
> > +
> > +/* Structs to represent the objects that belong to a media graph */
> > +
> > +/**
> > + * struct media_gobj - Define a graph object.
> > + *
> > + * @id:Non-zero object ID identifier. The ID should be unique
> > + * inside a media_device, as it is composed by
> > + * MEDIA_BITS_PER_TYPE to store the type plus
> > + * MEDIA_BITS_PER_LOCAL_ID to store a per-type ID
> > +
[RFC PATCH] v4l2-compat-ioctl32: fix struct v4l2_event32 alignment
Union v4l2_event::u is aligned to 8 bytes on arm32. On arm64 v4l2_event32::u
is aligned to 4 bytes. As a result structures v4l2_event and v4l2_event32 have
different sizes and VIDOC_DQEVENT ioctl does not work from arm32 apps running
on arm64 kernel. The patch fixes it.
Signed-off-by: Andrzej Hajda
---
Hi Hans,
It seems there is problem with VIDIOC_DQEVENT called from arm32 apps on arm64
kernel. After tracking down the issue it seems v4l2_event32 on arm64 have
different field alignment/size than v4l2_event on arm32. The patch fixes it.
But i guess it can break ABI on other architectures. Simple tests shows:
i386:
sizeof(struct v4l2_event)=0x78
offsetof(struct v4l2_event::u)=0x4
amd64:
sizeof(struct v4l2_event)=0x88
offsetof(struct v4l2_event::u)=0x8
arm:
sizeof(struct v4l2_event)=0x80
offsetof(struct v4l2_event::u)=0x8
arm64:
sizeof(struct v4l2_event)=0x88
offsetof(struct v4l2_event::u)=0x8
Any advices how to fix it in arch compatible way?
Regards
Andrzej
---
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index af63543..a4a1856 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -739,7 +739,7 @@ struct v4l2_event32 {
__u32 type;
union {
__u8data[64];
- } u;
+ } u __attribute__((aligned(8)));
__u32 pending;
__u32 sequence;
struct compat_timespec timestamp;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/8] Support getting default values from any control
Integer controls provide a way to get their default/initial value, but
any other control (p_u32, p_u8.) provide no other way to get the
initial value than unloading the module and loading it back.
*What is the actual problem?
I have a custom control with WIDTH integer values. Every value
represents the calibrated FPN (fixed pattern noise) correction value for that
column
-Application A changes the FPN correction value
-Application B wants to restore the calibrated value but it cant :(
*What is the proposed solution?
(Kudos to Hans Verkuil!!!)
The key change is in struct v4l2_ext_controls where the __u32 ctrl_class field
is changed to:
union {
__u32 ctrl_class;
__u32 which;
};
And two new defines are added:
#define V4L2_CTRL_WHICH_CUR_VAL0
#define V4L2_CTRL_WHICH_DEF_VAL0x0f00
The 'which' field tells you which controls are get/set/tried.
V4L2_CTRL_WHICH_CUR_VAL: the current value of the controls
V4L2_CTRL_WHICH_DEF_VAL: the default value of the controls
V4L2_CTRL_CLASS_*: the current value of the controls belonging to the specified
class.
Note: this is deprecated usage and is only there for backwards
compatibility.
Which is also why I don't think there is a need to add V4L2_CTRL_WHICH_
aliases for these defines.
I have posted a copy of my working tree to
https://github.com/ribalda/linux/tree/which_def
Changelog v1 (compared to v5 of New ioct VIDIOC_G_DEF_EXT_CTRLS):
Suggested by Hans Verkuil
Replace ioctl implementation with a new union on the struct v4l2_ext_controls
THANKS!
Ricardo Ribalda Delgado (8):
videodev2.h: Fix typo in comment
videodev2.h: Extend struct v4l2_ext_controls
media/v4l2-core: struct struct v4l2_ext_controls param which
usb/uvc: Support for V4L2_CTRL_WHICH_DEF_VAL
media/usb/pvrusb2: Support for V4L2_CTRL_WHICH_DEF_VAL
media/pci/saa7164-encoder Support for V4L2_CTRL_WHICH_DEF_VAL
media/pci/saa7164-vbi Support for V4L2_CTRL_WHICH_DEF_VAL
Docbook: media: Document changes on struct v4l2_ext_controls
Documentation/DocBook/media/v4l/v4l2.xml | 9
.../DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 14 ++
drivers/media/pci/saa7164/saa7164-encoder.c| 55 -
drivers/media/pci/saa7164/saa7164-vbi.c| 57 +-
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 17 ++-
drivers/media/usb/uvc/uvc_v4l2.c | 14 +-
drivers/media/v4l2-core/v4l2-ctrls.c | 35 +++--
include/uapi/linux/videodev2.h | 9 +++-
8 files changed, 153 insertions(+), 57 deletions(-)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/8] videodev2.h: Extend struct v4l2_ext_controls
So it can be used to get the default value of a control.
Without this change it is not possible to get get the
default value of array controls.
Signed-off-by: Ricardo Ribalda Delgado
---
include/uapi/linux/videodev2.h | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 72fa3e490e30..2e857b19a155 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1461,7 +1461,10 @@ struct v4l2_ext_control {
} __attribute__ ((packed));
struct v4l2_ext_controls {
- __u32 ctrl_class;
+ union {
+ __u32 ctrl_class;
+ __u32 which;
+ };
__u32 count;
__u32 error_idx;
__u32 reserved[2];
@@ -1472,6 +1475,8 @@ struct v4l2_ext_controls {
#define V4L2_CTRL_ID2CLASS(id)((id) & 0x0fffUL)
#define V4L2_CTRL_DRIVER_PRIV(id) (((id) & 0x) >= 0x1000)
#define V4L2_CTRL_MAX_DIMS (4)
+#define V4L2_CTRL_WHICH_CUR_VAL 0
+#define V4L2_CTRL_WHICH_DEF_VAL 0x0f00
enum v4l2_ctrl_type {
V4L2_CTRL_TYPE_INTEGER = 1,
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/8] videodev2.h: Fix typo in comment
Referenced file has moved
Signed-off-by: Ricardo Ribalda Delgado
---
include/uapi/linux/videodev2.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 3228fbebcd63..72fa3e490e30 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2271,7 +2271,7 @@ struct v4l2_create_buffers {
#define VIDIOC_QUERY_EXT_CTRL _IOWR('V', 103, struct v4l2_query_ext_ctrl)
/* Reminder: when adding new ioctls please add support for them to
- drivers/media/video/v4l2-compat-ioctl32.c as well! */
+ drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
#define BASE_VIDIOC_PRIVATE192 /* 192-255 are private */
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 8/8] Docbook: media: Document changes on struct v4l2_ext_controls
Vidioc-g-ext-ctrls can now be used to get the default value of the controls. Signed-off-by: Ricardo Ribalda Delgado --- Documentation/DocBook/media/v4l/v4l2.xml | 9 + Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml | 14 ++ 2 files changed, 23 insertions(+) diff --git a/Documentation/DocBook/media/v4l/v4l2.xml b/Documentation/DocBook/media/v4l/v4l2.xml index e98caa1c39bd..be52bd2fb335 100644 --- a/Documentation/DocBook/media/v4l/v4l2.xml +++ b/Documentation/DocBook/media/v4l/v4l2.xml @@ -153,6 +153,15 @@ structs, ioctls) must be noted in more detail in the history chapter applications. --> + 4.4 + 2015-08-20 + rr + Extend vidioc-g-ext-ctrls;. Replace ctrl_class with a new +union with ctrl_class and which. Which is used to select the current value of +the control or the default value. + + + 3.21 2015-02-13 mcc diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml index c5bdbfcc42b3..224fa2bd1481 100644 --- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml +++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml @@ -216,7 +216,12 @@ Valid if V4L2_CTRL_FLAG_HAS_PAYLOAD is set for this control &cs-str; + + union + (anonymous) + + __u32 ctrl_class The control class to which all controls belong, see @@ -228,6 +233,15 @@ with a count of 0. If that succeeds, then the driver supports this feature. + + __u32 + which +Which control are get/set/tried. V4L2_CTRL_WHICH_CUR_VAL +will return the current value of the control and V4L2_CTRL_WHICH_DEF_VAL will +return the default value of the control. Please note that the default value of the control cannot +be set or tried, only get. + + __u32 count The number of controls in the controls array. May -- 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/8] media/v4l2-core: struct struct v4l2_ext_controls param which
Support for new field which on v4l2_ext_controls, used to get the
default value of one or more controls.
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/v4l2-core/v4l2-ctrls.c | 35 ++-
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
b/drivers/media/v4l2-core/v4l2-ctrls.c
index b6b7dcc1b77d..23a69f637f6d 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1489,6 +1489,17 @@ static int new_to_user(struct v4l2_ext_control *c,
return ptr_to_user(c, ctrl, ctrl->p_new);
}
+/* Helper function: copy the initial control value back to the caller */
+static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
+{
+ int idx;
+
+ for (idx = 0; idx < ctrl->elems; idx++)
+ ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
+
+ return ptr_to_user(c, ctrl, ctrl->p_new);
+}
+
/* Helper function: copy the caller-provider value to the given control value
*/
static int user_to_ptr(struct v4l2_ext_control *c,
struct v4l2_ctrl *ctrl,
@@ -2708,7 +2719,9 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
*hdl,
cs->error_idx = i;
- if (cs->ctrl_class && V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
+ if (cs->ctrl_class &&
+ cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
+ V4L2_CTRL_ID2CLASS(id) != cs->ctrl_class)
return -EINVAL;
/* Old-style private controls are not allowed for
@@ -2787,7 +2800,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler
*hdl,
whether there are any controls at all. */
static int class_check(struct v4l2_ctrl_handler *hdl, u32 ctrl_class)
{
- if (ctrl_class == 0)
+ if (ctrl_class == 0 || ctrl_class == V4L2_CTRL_WHICH_DEF_VAL)
return list_empty(&hdl->ctrl_refs) ? -EINVAL : 0;
return find_ref_lock(hdl, ctrl_class | 1) ? 0 : -EINVAL;
}
@@ -2801,10 +2814,14 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
struct v4l2_ctrl_helper *helpers = helper;
int ret;
int i, j;
+ bool def_value = false;
cs->error_idx = cs->count;
cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
+ if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+ def_value = true;
+
if (hdl == NULL)
return -EINVAL;
@@ -2827,9 +2844,11 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
for (i = 0; !ret && i < cs->count; i++) {
int (*ctrl_to_user)(struct v4l2_ext_control *c,
- struct v4l2_ctrl *ctrl) = cur_to_user;
+ struct v4l2_ctrl *ctrl);
struct v4l2_ctrl *master;
+ ctrl_to_user = def_value ? def_to_user : cur_to_user;
+
if (helpers[i].mref == NULL)
continue;
@@ -2839,8 +2858,9 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
struct v4l2_ext_controls *cs
v4l2_ctrl_lock(master);
/* g_volatile_ctrl will update the new control values */
- if ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
- (master->has_volatiles && !is_cur_manual(master))) {
+ if (!def_value &&
+ ((master->flags & V4L2_CTRL_FLAG_VOLATILE) ||
+ (master->has_volatiles && !is_cur_manual(master {
for (j = 0; j < master->ncontrols; j++)
cur_to_new(master->cluster[j]);
ret = call_op(master, g_volatile_ctrl);
@@ -3062,6 +3082,11 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct
v4l2_ctrl_handler *hdl,
int ret;
cs->error_idx = cs->count;
+
+ /* Default value cannot be changed */
+ if (cs->which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
cs->ctrl_class = V4L2_CTRL_ID2CLASS(cs->ctrl_class);
if (hdl == NULL)
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/8] usb/uvc: Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/usb/uvc/uvc_v4l2.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 2764f43607c1..e6d3a1bcfa2f 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -980,6 +980,7 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void
*fh,
struct uvc_fh *handle = fh;
struct uvc_video_chain *chain = handle->chain;
struct v4l2_ext_control *ctrl = ctrls->controls;
+ struct v4l2_queryctrl qc;
unsigned int i;
int ret;
@@ -988,7 +989,14 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file, void
*fh,
return ret;
for (i = 0; i < ctrls->count; ++ctrl, ++i) {
- ret = uvc_ctrl_get(chain, ctrl);
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ qc.id = ctrl->id;
+ ret = uvc_query_v4l2_ctrl(chain, &qc);
+ if (!ret)
+ ctrl->value = qc.default_value;
+ } else
+ ret = uvc_ctrl_get(chain, ctrl);
+
if (ret < 0) {
uvc_ctrl_rollback(handle);
ctrls->error_idx = i;
@@ -1010,6 +1018,10 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh
*handle,
unsigned int i;
int ret;
+ /* Default value cannot be changed */
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
ret = uvc_ctrl_begin(chain);
if (ret < 0)
return ret;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/8] media/usb/pvrusb2: Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/usb/pvrusb2/pvrusb2-v4l2.c | 17 -
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
index 1c5f85bf7ed4..43b2f2214798 100644
--- a/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
+++ b/drivers/media/usb/pvrusb2/pvrusb2-v4l2.c
@@ -628,6 +628,7 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
struct pvr2_v4l2_fh *fh = file->private_data;
struct pvr2_hdw *hdw = fh->channel.mc_head->hdw;
struct v4l2_ext_control *ctrl;
+ struct pvr2_ctrl *cptr;
unsigned int idx;
int val;
int ret;
@@ -635,8 +636,18 @@ static int pvr2_g_ext_ctrls(struct file *file, void *priv,
ret = 0;
for (idx = 0; idx < ctls->count; idx++) {
ctrl = ctls->controls + idx;
- ret = pvr2_ctrl_get_value(
+ if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ cptr = pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id);
+ if (cptr)
+ pvr2_ctrl_get_def(cptr, &val);
+ else
+ ret = -EINVAL;
+
+
+ } else
+ ret = pvr2_ctrl_get_value(
pvr2_hdw_get_ctrl_v4l(hdw, ctrl->id), &val);
+
if (ret) {
ctls->error_idx = idx;
return ret;
@@ -658,6 +669,10 @@ static int pvr2_s_ext_ctrls(struct file *file, void *priv,
unsigned int idx;
int ret;
+ /* Default value cannot be changed */
+ if (ctls->which == V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
ret = 0;
for (idx = 0; idx < ctls->count; idx++) {
ctrl = ctls->controls + idx;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 7/8] media/pci/saa7164-vbi Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/pci/saa7164/saa7164-vbi.c | 57 +++--
1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/drivers/media/pci/saa7164/saa7164-vbi.c
b/drivers/media/pci/saa7164/saa7164-vbi.c
index 859fd03d82f9..e8723102a4d2 100644
--- a/drivers/media/pci/saa7164/saa7164-vbi.c
+++ b/drivers/media/pci/saa7164/saa7164-vbi.c
@@ -494,30 +494,6 @@ static int saa7164_get_ctrl(struct saa7164_port *port,
return 0;
}
-static int vidioc_g_ext_ctrls(struct file *file, void *priv,
- struct v4l2_ext_controls *ctrls)
-{
- struct saa7164_vbi_fh *fh = file->private_data;
- struct saa7164_port *port = fh->port;
- int i, err = 0;
-
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
- for (i = 0; i < ctrls->count; i++) {
- struct v4l2_ext_control *ctrl = ctrls->controls + i;
-
- err = saa7164_get_ctrl(port, ctrl);
- if (err) {
- ctrls->error_idx = i;
- break;
- }
- }
- return err;
-
- }
-
- return -EINVAL;
-}
-
static int saa7164_try_ctrl(struct v4l2_ext_control *ctrl, int ac3)
{
int ret = -EINVAL;
@@ -810,6 +786,39 @@ static int vidioc_queryctrl(struct file *file, void *priv,
return -EINVAL;
}
+static int vidioc_g_ext_ctrls(struct file *file, void *priv,
+ struct v4l2_ext_controls *ctrls)
+{
+ struct saa7164_vbi_fh *fh = file->private_data;
+ struct saa7164_port *port = fh->port;
+ int i, err = 0;
+
+ if (ctrls->ctrl_class != V4L2_CTRL_CLASS_MPEG &&
+ ctrls->which != V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
+ for (i = 0; i < ctrls->count; i++) {
+ struct v4l2_ext_control *ctrl = ctrls->controls + i;
+
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ struct v4l2_queryctrl q;
+
+ err = fill_queryctrl(&port->vbi_params, &q);
+ if (!err)
+ ctrl->value = q.default_value;
+ } else
+ err = saa7164_get_ctrl(port, ctrl);
+
+ if (err) {
+ ctrls->error_idx = i;
+ break;
+ }
+ }
+
+ return err;
+}
+
+
static int saa7164_vbi_stop_port(struct saa7164_port *port)
{
struct saa7164_dev *dev = port->dev;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/8] media/pci/saa7164-encoder Support for V4L2_CTRL_WHICH_DEF_VAL
This driver does not use the control infrastructure.
Add support for the new field which on structure
v4l2_ext_controls
Signed-off-by: Ricardo Ribalda Delgado
---
drivers/media/pci/saa7164/saa7164-encoder.c | 55 -
1 file changed, 31 insertions(+), 24 deletions(-)
diff --git a/drivers/media/pci/saa7164/saa7164-encoder.c
b/drivers/media/pci/saa7164/saa7164-encoder.c
index 4434e0f28c26..e897b83b66dd 100644
--- a/drivers/media/pci/saa7164/saa7164-encoder.c
+++ b/drivers/media/pci/saa7164/saa7164-encoder.c
@@ -531,30 +531,6 @@ static int saa7164_get_ctrl(struct saa7164_port *port,
return 0;
}
-static int vidioc_g_ext_ctrls(struct file *file, void *priv,
- struct v4l2_ext_controls *ctrls)
-{
- struct saa7164_encoder_fh *fh = file->private_data;
- struct saa7164_port *port = fh->port;
- int i, err = 0;
-
- if (ctrls->ctrl_class == V4L2_CTRL_CLASS_MPEG) {
- for (i = 0; i < ctrls->count; i++) {
- struct v4l2_ext_control *ctrl = ctrls->controls + i;
-
- err = saa7164_get_ctrl(port, ctrl);
- if (err) {
- ctrls->error_idx = i;
- break;
- }
- }
- return err;
-
- }
-
- return -EINVAL;
-}
-
static int saa7164_try_ctrl(struct v4l2_ext_control *ctrl, int ac3)
{
int ret = -EINVAL;
@@ -884,6 +860,37 @@ static int vidioc_queryctrl(struct file *file, void *priv,
return -EINVAL;
}
+static int vidioc_g_ext_ctrls(struct file *file, void *priv,
+ struct v4l2_ext_controls *ctrls)
+{
+ struct saa7164_encoder_fh *fh = file->private_data;
+ struct saa7164_port *port = fh->port;
+ int i, err = 0;
+
+ if (ctrls->ctrl_class != V4L2_CTRL_CLASS_MPEG &&
+ ctrls->which != V4L2_CTRL_WHICH_DEF_VAL)
+ return -EINVAL;
+
+ for (i = 0; i < ctrls->count; i++) {
+ struct v4l2_ext_control *ctrl = ctrls->controls + i;
+
+ if (ctrls->which == V4L2_CTRL_WHICH_DEF_VAL) {
+ struct v4l2_queryctrl q;
+
+ err = fill_queryctrl(&port->encoder_params, &q);
+ if (!err)
+ ctrl->value = q.default_value;
+ } else
+ err = saa7164_get_ctrl(port, ctrl);
+
+ if (err) {
+ ctrls->error_idx = i;
+ break;
+ }
+ }
+ return err;
+}
+
static int saa7164_encoder_stop_port(struct saa7164_port *port)
{
struct saa7164_dev *dev = port->dev;
--
2.5.0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] [media] v4l: tegra: Add NVIDIA Tegra VI driver
Hi Bryan,
Thanks for contributing this driver, very much appreciated.
I do have some comments below, basically about the same things we discussed
privately before.
On 08/21/2015 02:51 AM, Bryan Wu wrote:
> NVIDIA Tegra processor contains a powerful Video Input (VI) hardware
> controller which can support up to 6 MIPI CSI camera sensors.
>
> This patch adds a V4L2 media controller and capture driver to support
> Tegra VI hardware. It's verified with Tegra built-in test pattern
> generator.
>
> Signed-off-by: Bryan Wu
> Reviewed-by: Hans Verkuil
> ---
> drivers/media/platform/Kconfig |1 +
> drivers/media/platform/Makefile |2 +
> drivers/media/platform/tegra/Kconfig |9 +
> drivers/media/platform/tegra/Makefile|3 +
> drivers/media/platform/tegra/tegra-channel.c | 1074
> ++
> drivers/media/platform/tegra/tegra-core.c| 295 +++
> drivers/media/platform/tegra/tegra-core.h| 134
> drivers/media/platform/tegra/tegra-vi.c | 585 ++
> drivers/media/platform/tegra/tegra-vi.h | 224 ++
> include/dt-bindings/media/tegra-vi.h | 35 +
> 10 files changed, 2362 insertions(+)
> create mode 100644 drivers/media/platform/tegra/Kconfig
> create mode 100644 drivers/media/platform/tegra/Makefile
> create mode 100644 drivers/media/platform/tegra/tegra-channel.c
> create mode 100644 drivers/media/platform/tegra/tegra-core.c
> create mode 100644 drivers/media/platform/tegra/tegra-core.h
> create mode 100644 drivers/media/platform/tegra/tegra-vi.c
> create mode 100644 drivers/media/platform/tegra/tegra-vi.h
> create mode 100644 include/dt-bindings/media/tegra-vi.h
>
> +static int tegra_channel_capture_frame(struct tegra_channel *chan)
> +{
> + struct tegra_channel_buffer *buf = chan->active;
> + struct vb2_buffer *vb = &buf->buf;
> + int err = 0;
> + u32 thresh, value, frame_start;
> + int bytes_per_line = chan->format.bytesperline;
> +
> + if (!vb2_start_streaming_called(&chan->queue) || !buf)
> + return -EINVAL;
> +
> + if (chan->bypass)
> + goto bypass_done;
> +
> + /* Program buffer address */
> + csi_write(chan,
> + TEGRA_VI_CSI_SURFACE0_OFFSET_MSB + chan->surface * 8,
> + 0x0);
> + csi_write(chan,
> + TEGRA_VI_CSI_SURFACE0_OFFSET_LSB + chan->surface * 8,
> + buf->addr);
> + csi_write(chan,
> + TEGRA_VI_CSI_SURFACE0_STRIDE + chan->surface * 4,
> + bytes_per_line);
> +
> + /* Program syncpoint */
> + frame_start = sp_bit(chan, SP_PP_FRAME_START);
> + tegra_channel_write(chan, TEGRA_VI_CFG_VI_INCR_SYNCPT,
> + frame_start | host1x_syncpt_id(chan->sp));
> +
> + csi_write(chan, TEGRA_VI_CSI_SINGLE_SHOT, 0x1);
> +
> + /* Use syncpoint to wake up */
> + thresh = host1x_syncpt_incr_max(chan->sp, 1);
> +
> + mutex_unlock(&chan->lock);
> + err = host1x_syncpt_wait(chan->sp, thresh,
> + TEGRA_VI_SYNCPT_WAIT_TIMEOUT, &value);
> + mutex_lock(&chan->lock);
> +
> + if (err) {
> + dev_err(&chan->video.dev, "frame start syncpt timeout!\n");
> + tegra_channel_capture_error(chan, err);
> + }
> +
> +bypass_done:
> + /* Captured one frame */
> + spin_lock_irq(&chan->queued_lock);
> + vb->v4l2_buf.sequence = chan->sequence++;
> + vb->v4l2_buf.field = V4L2_FIELD_NONE;
> + v4l2_get_timestamp(&vb->v4l2_buf.timestamp);
> + vb2_set_plane_payload(vb, 0, chan->format.sizeimage);
> + vb2_buffer_done(vb, err < 0 ? VB2_BUF_STATE_ERROR : VB2_BUF_STATE_DONE);
> + spin_unlock_irq(&chan->queued_lock);
> +
> + return err;
> +}
> +
> +static void tegra_channel_work(struct work_struct *work)
> +{
> + struct tegra_channel *chan =
> + container_of(work, struct tegra_channel, work);
> +
> + while (1) {
> + spin_lock_irq(&chan->queued_lock);
> + if (list_empty(&chan->capture)) {
> + chan->active = NULL;
> + spin_unlock_irq(&chan->queued_lock);
> + return;
> + }
> + chan->active = list_entry(chan->capture.next,
> + struct tegra_channel_buffer, queue);
> + list_del_init(&chan->active->queue);
> + spin_unlock_irq(&chan->queued_lock);
> +
> + mutex_lock(&chan->lock);
> + tegra_channel_capture_frame(chan);
> + mutex_unlock(&chan->lock);
> + }
> +}
> +
> +/*
> -
> + * videobuf2 queue operations
> + */
> +
> +static int
> +tegra_channel_queue_setup(struct vb2_queue *vq, const struct v4l2_format
> *fmt,
> + unsigned int *nbuffers, unsigned int *nplanes,
> + unsigned int sizes[], void *al
Re: [PATCH v6 1/8] [media] media: create a macro to get entity ID
Em Fri, 21 Aug 2015 03:40:48 +0300
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Wednesday 19 August 2015 08:01:48 Mauro Carvalho Chehab wrote:
> > Instead of accessing directly entity.id, let's create a macro,
> > as this field will be moved into a common struct later on.
> >
> > Signed-off-by: Mauro Carvalho Chehab
> > Acked-by: Hans Verkuil
> > Signed-off-by: Mauro Carvalho Chehab
> >
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index c55ab5029323..e429605ca2c3 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -77,8 +77,8 @@ static struct media_entity *find_entity(struct
> > media_device *mdev, u32 id) spin_lock(&mdev->lock);
> >
> > media_device_for_each_entity(entity, mdev) {
> > - if ((entity->id == id && !next) ||
> > - (entity->id > id && next)) {
> > + if (((media_entity_id(entity) == id) && !next) ||
> > + ((media_entity_id(entity) > id) && next)) {
> > spin_unlock(&mdev->lock);
> > return entity;
> > }
> > @@ -104,7 +104,7 @@ static long media_device_enum_entities(struct
> > media_device *mdev, if (ent == NULL)
> > return -EINVAL;
> >
> > - u_ent.id = ent->id;
> > + u_ent.id = media_entity_id(ent);
> > if (ent->name)
> > strlcpy(u_ent.name, ent->name, sizeof(u_ent.name));
> > u_ent.type = ent->type;
> > @@ -122,7 +122,7 @@ static long media_device_enum_entities(struct
> > media_device *mdev, static void media_device_kpad_to_upad(const struct
> > media_pad *kpad, struct media_pad_desc *upad)
> > {
> > - upad->entity = kpad->entity->id;
> > + upad->entity = media_entity_id(kpad->entity);
> > upad->index = kpad->index;
> > upad->flags = kpad->flags;
> > }
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index 949e5f92cbdc..cb0ac4e0dfa5 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -140,10 +140,10 @@ void media_entity_graph_walk_start(struct
> > media_entity_graph *graph, graph->stack[graph->top].entity = NULL;
> > bitmap_zero(graph->entities, MEDIA_ENTITY_ENUM_MAX_ID);
> >
> > - if (WARN_ON(entity->id >= MEDIA_ENTITY_ENUM_MAX_ID))
> > + if (WARN_ON(media_entity_id(entity) >= MEDIA_ENTITY_ENUM_MAX_ID))
> > return;
> >
> > - __set_bit(entity->id, graph->entities);
> > + __set_bit(media_entity_id(entity), graph->entities);
> > stack_push(graph, entity);
> > }
> > EXPORT_SYMBOL_GPL(media_entity_graph_walk_start);
> > @@ -184,11 +184,11 @@ media_entity_graph_walk_next(struct media_entity_graph
> > *graph)
> >
> > /* Get the entity in the other end of the link . */
> > next = media_entity_other(entity, link);
> > - if (WARN_ON(next->id >= MEDIA_ENTITY_ENUM_MAX_ID))
> > + if (WARN_ON(media_entity_id(next) >= MEDIA_ENTITY_ENUM_MAX_ID))
> > return NULL;
> >
> > /* Has the entity already been visited? */
> > - if (__test_and_set_bit(next->id, graph->entities)) {
> > + if (__test_and_set_bit(media_entity_id(next), graph->entities))
> > {
> > link_top(graph)++;
> > continue;
> > }
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> > b/drivers/media/platform/vsp1/vsp1_video.c index 17f08973f835..debe4e539df6
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -352,10 +352,10 @@ static int vsp1_pipeline_validate_branch(struct
> > vsp1_pipeline *pipe, break;
> >
> > /* Ensure the branch has no loop. */
> > - if (entities & (1 << entity->subdev.entity.id))
> > + if (entities & (1 << media_entity_id(&entity->subdev.entity)))
> > return -EPIPE;
> >
> > - entities |= 1 << entity->subdev.entity.id;
> > + entities |= 1 << media_entity_id(&entity->subdev.entity);
> >
> > /* UDS can't be chained. */
> > if (entity->type == VSP1_ENTITY_UDS) {
>
> I would move the modification of the vsp1 driver to Javier's patch that
> modifies the OMAP3 and OMAP4 drivers. Alternatively you could squash them
> into
> this patch, but I believe having a first patch that adds the inline function
> and a second patch that modifies all drivers to use it would be better.
Squashing will lose Javier's authorship. I guess the better is have a
first patch with the inline, then my paches and Javier's ones, and
latter on the patch removing entity->id.
I'll do that on the next patch series.
>
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 8b21a4d920d9..0a66fc225559 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -113,6 +113,11 @@ static inline u32 media_entity_s
Re: [PATCH v6 2/8] [media] media: add a common struct to be embed on media graph objects
On 08/21/2015 03:02 AM, Laurent Pinchart wrote:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Wednesday 19 August 2015 08:01:49 Mauro Carvalho Chehab wrote:
>> +/* Enums used internally at the media controller to represent graphs */
>> +
>> +/**
>> + * enum media_gobj_type - type of a graph element
>
> Let's try to standardize the vocabulary, should it be called graph object or
> graph element ? In the first case let's document it as graph object. In the
> second case it would be more consistent to refer to it as enum
> media_gelem_type (and struct media_gelem below).
For what it is worth, I prefer the term graph object.
>
>> + *
>> + */
>> +enum media_gobj_type {
>> + /* FIXME: add the types here, as we embed media_gobj */
>> +MEDIA_GRAPH_NONE
>> +};
>> +
>> +#define MEDIA_BITS_PER_TYPE 8
>> +#define MEDIA_BITS_PER_LOCAL_ID (32 - MEDIA_BITS_PER_TYPE)
>> +#define MEDIA_LOCAL_ID_MASK GENMASK(MEDIA_BITS_PER_LOCAL_ID - 1, 0)
>> +
>> +/* Structs to represent the objects that belong to a media graph */
>> +
>> +/**
>> + * struct media_gobj - Define a graph object.
>> + *
>> + * @id: Non-zero object ID identifier. The ID should be unique
>> + * inside a media_device, as it is composed by
>> + * MEDIA_BITS_PER_TYPE to store the type plus
>> + * MEDIA_BITS_PER_LOCAL_ID to store a per-type ID
>> + * (called as "local ID").
>
> I'd very much prefer using a single ID range and adding a type field. Abusing
> bits of the ID field to store the type will just makes IDs impractical to
> use.
> Let's do it properly.
Why is that impractical? I think it is more practical. Why waste memory on
something
that is easy to encode in the ID?
I'm not necessarily opposed to splitting this up (Mauro's initial patch series
used
a separate type field if I remember correctly), but it's not clear to me what
the
benefits are. Keeping it in a single u32 makes describing links also very easy
since
you just give it the two objects that are linked and it is immediately clear
which
object types are linked: no need to either store the types in the link struct or
look up each object to find the type.
>
>> + * All elements on the media graph should have this struct embedded
>
> All elements (objects) or only the ones that need an ID ? Or maybe we'll
> define graph element (object) as an element (object) that has an ID, making
> some "elements" not elements.
Yes, all objects have an ID. I see no reason to special-case this.
You wrote this at 3 am, so you were probably sleep-deprived when you wrote the
second sentence as I can't wrap my head around that one :-)
>
>> + */
>> +struct media_gobj {
>> +u32 id;
>> +};
>> +
>> +
>> struct media_pipeline {
>> };
>>
>> @@ -118,6 +151,26 @@ static inline u32 media_entity_id(struct media_entity
>> *entity) return entity->id;
>> }
>>
>> +static inline enum media_gobj_type media_type(struct media_gobj *gobj)
>> +{
>> +return gobj->id >> MEDIA_BITS_PER_LOCAL_ID;
>> +}
>> +
>> +static inline u32 media_localid(struct media_gobj *gobj)
>> +{
>> +return gobj->id & MEDIA_LOCAL_ID_MASK;
>> +}
>> +
>> +static inline u32 media_gobj_gen_id(enum media_gobj_type type, u32
>> local_id)
>> +{
>> +u32 id;
>> +
>> +id = type << MEDIA_BITS_PER_LOCAL_ID;
>> +id |= local_id & MEDIA_LOCAL_ID_MASK;
>> +
>> +return id;
>> +}
>> +
>> #define MEDIA_ENTITY_ENUM_MAX_DEPTH 16
>> #define MEDIA_ENTITY_ENUM_MAX_ID64
>>
>> @@ -131,6 +184,14 @@ struct media_entity_graph {
>> int top;
>> };
>>
>> +#define gobj_to_entity(gobj) \
>> +container_of(gobj, struct media_entity, graph_obj)
>
> For consistency reason would this be called media_gobj_to_entity ? I would
> also turn it into an inline function to ensure type checking.
Good one. I agree.
>
>> +
>> +void media_gobj_init(struct media_device *mdev,
>> +enum media_gobj_type type,
>> +struct media_gobj *gobj);
>> +void media_gobj_remove(struct media_gobj *gobj);
>> +
>> int media_entity_init(struct media_entity *entity, u16 num_pads,
>> struct media_pad *pads);
>> void media_entity_cleanup(struct media_entity *entity);
>
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/3] media: atmel-isi: setup the ISI_CFG2 register directly
In the function configure_geometry(), we will setup the ISI CFG2
according to the sensor output format.
It make no sense to just read back the CFG2 register and just set part
of it.
So just set up this register directly makes things simpler.
Currently only support YUV format from camera sensor.
Signed-off-by: Josh Wu
Reviewed-by: Laurent Pinchart
---
Changes in v3: None
Changes in v2:
- add Laurent's reviewed-by tag.
drivers/media/platform/soc_camera/atmel-isi.c | 20 +++-
1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index f39132c..b67da70 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -105,24 +105,25 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
static int configure_geometry(struct atmel_isi *isi, u32 width,
u32 height, u32 code)
{
- u32 cfg2, cr;
+ u32 cfg2;
+ /* According to sensor's output format to set cfg2 */
switch (code) {
/* YUV, including grey */
case MEDIA_BUS_FMT_Y8_1X8:
- cr = ISI_CFG2_GRAYSCALE;
+ cfg2 = ISI_CFG2_GRAYSCALE;
break;
case MEDIA_BUS_FMT_VYUY8_2X8:
- cr = ISI_CFG2_YCC_SWAP_MODE_3;
+ cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
break;
case MEDIA_BUS_FMT_UYVY8_2X8:
- cr = ISI_CFG2_YCC_SWAP_MODE_2;
+ cfg2 = ISI_CFG2_YCC_SWAP_MODE_2;
break;
case MEDIA_BUS_FMT_YVYU8_2X8:
- cr = ISI_CFG2_YCC_SWAP_MODE_1;
+ cfg2 = ISI_CFG2_YCC_SWAP_MODE_1;
break;
case MEDIA_BUS_FMT_YUYV8_2X8:
- cr = ISI_CFG2_YCC_SWAP_DEFAULT;
+ cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
break;
/* RGB, TODO */
default:
@@ -130,17 +131,10 @@ static int configure_geometry(struct atmel_isi *isi, u32
width,
}
isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
- cfg2 = isi_readl(isi, ISI_CFG2);
- /* Set YCC swap mode */
- cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK;
- cfg2 |= cr;
/* Set width */
- cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK);
cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) &
ISI_CFG2_IM_HSIZE_MASK;
/* Set height */
- cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK);
cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
& ISI_CFG2_IM_VSIZE_MASK;
isi_writel(isi, ISI_CFG2, cfg2);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 2/3] media: atmel-isi: move configure_geometry() to start_streaming()
As in set_fmt() function we only need to know which format is been set,
we don't need to access the ISI hardware in this moment.
So move the configure_geometry(), which access the ISI hardware, to
start_streaming() will make code more consistent and simpler.
Signed-off-by: Josh Wu
Reviewed-by: Laurent Pinchart
---
Changes in v3: None
Changes in v2:
- Add Laurent's reviewed-by tag.
drivers/media/platform/soc_camera/atmel-isi.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index b67da70..fe9247a 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned
int count)
/* Disable all interrupts */
isi_writel(isi, ISI_INTDIS, (u32)~0UL);
+ ret = configure_geometry(isi, icd->user_width, icd->user_height,
+ icd->current_fmt->code);
+ if (ret < 0)
+ return ret;
+
spin_lock_irq(&isi->lock);
/* Clear any pending interrupt */
isi_readl(isi, ISI_STATUS);
@@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
static int isi_camera_set_fmt(struct soc_camera_device *icd,
struct v4l2_format *f)
{
- struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
- struct atmel_isi *isi = ici->priv;
struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
const struct soc_camera_format_xlate *xlate;
struct v4l2_pix_format *pix = &f->fmt.pix;
@@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct soc_camera_device
*icd,
if (mf->code != xlate->code)
return -EINVAL;
- /* Enable PM and peripheral clock before operate isi registers */
- pm_runtime_get_sync(ici->v4l2_dev.dev);
-
- ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
-
- pm_runtime_put(ici->v4l2_dev.dev);
-
- if (ret < 0)
- return ret;
-
pix->width = mf->width;
pix->height = mf->height;
pix->field = mf->field;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 3/3] media: atmel-isi: add sanity check for supported formats in try/set_fmt()
After adding the format check in try_fmt()/set_fmt(), we don't need any
format check in configure_geometry(). So make configure_geometry() as
void type.
Signed-off-by: Josh Wu
---
Changes in v3:
- check the whether format is supported, if no then return a default
format.
- misc changes according to Laurent's feedback.
Changes in v2:
- new added patch
drivers/media/platform/soc_camera/atmel-isi.c | 37 +--
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index fe9247a..84c91d3 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -102,17 +102,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
return readl(isi->regs + reg);
}
-static int configure_geometry(struct atmel_isi *isi, u32 width,
+static void configure_geometry(struct atmel_isi *isi, u32 width,
u32 height, u32 code)
{
u32 cfg2;
/* According to sensor's output format to set cfg2 */
switch (code) {
- /* YUV, including grey */
+ default:
+ /* Grey */
case MEDIA_BUS_FMT_Y8_1X8:
cfg2 = ISI_CFG2_GRAYSCALE;
break;
+ /* YUV */
case MEDIA_BUS_FMT_VYUY8_2X8:
cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
break;
@@ -126,8 +128,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
width,
cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
break;
/* RGB, TODO */
- default:
- return -EINVAL;
}
isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
@@ -138,8 +138,23 @@ static int configure_geometry(struct atmel_isi *isi, u32
width,
cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
& ISI_CFG2_IM_VSIZE_MASK;
isi_writel(isi, ISI_CFG2, cfg2);
+}
- return 0;
+static bool is_supported(struct soc_camera_device *icd,
+ const u32 pixformat)
+{
+ switch (pixformat) {
+ /* YUV, including grey */
+ case V4L2_PIX_FMT_GREY:
+ case V4L2_PIX_FMT_YUYV:
+ case V4L2_PIX_FMT_UYVY:
+ case V4L2_PIX_FMT_YVYU:
+ case V4L2_PIX_FMT_VYUY:
+ return true;
+ /* RGB, TODO */
+ default:
+ return false;
+ }
}
static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -390,10 +405,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned
int count)
/* Disable all interrupts */
isi_writel(isi, ISI_INTDIS, (u32)~0UL);
- ret = configure_geometry(isi, icd->user_width, icd->user_height,
+ configure_geometry(isi, icd->user_width, icd->user_height,
icd->current_fmt->code);
- if (ret < 0)
- return ret;
spin_lock_irq(&isi->lock);
/* Clear any pending interrupt */
@@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
*icd,
struct v4l2_mbus_framefmt *mf = &format.format;
int ret;
+ /* check with atmel-isi support format, if not support use UYVY */
+ if (!is_supported(icd, pix->pixelformat))
+ pix->pixelformat = V4L2_PIX_FMT_YUYV;
+
xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
if (!xlate) {
dev_warn(icd->parent, "Format %x not found\n",
@@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
*icd,
u32 pixfmt = pix->pixelformat;
int ret;
+ /* check with atmel-isi support format, if not support use UYVY */
+ if (!is_supported(icd, pix->pixelformat))
+ pix->pixelformat = V4L2_PIX_FMT_YUYV;
+
xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
if (pixfmt && !xlate) {
dev_warn(icd->parent, "Format %x not found\n", pixfmt);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] [media] staging: omap4iss: get entity ID using media_entity_id()
On 08/21/2015 02:15 AM, Laurent Pinchart wrote: > Hi Javier, > > On Friday 21 August 2015 02:14:05 Javier Martinez Canillas wrote: >> On 08/20/2015 08:37 PM, Laurent Pinchart wrote: >>> On Wednesday 19 August 2015 17:35:19 Javier Martinez Canillas wrote: The struct media_entity does not have an .id field anymore since now the entity ID is stored in the embedded struct media_gobj. This caused the omap4iss driver fail to build. Fix by using the media_entity_id() macro to obtain the entity ID. Signed-off-by: Javier Martinez Canillas >>> >>> This looks fine to me. The patch needs to be moved between Mauro's 1/8 and >>> 2/8 patches to avoid breaking bisection with patch 3/8. I'd squash this >>> patch and 2/4 into a single "media: Use media_entity_id() in drivers" >>> patch. >> >> Yes, Hans and Mauro already mentioned it and I completely agree that >> should be squashed with Mauro's patch to maintain git bisect-ability. > > I wouldn't squash patches 1/4 and 2/4 into Mauro's 3/8 patch as Hans > proposed, > but instead squashing them together into a single patch and move the result > as > 1.5/8 in Mauro's series. > I agree with Laurent, this is a better solution. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
