Re: [PATCH v2] [media] move media platform data to linux/platform_data/media
On Tue, Nov 17, 2015 at 09:21:13AM -0200, Mauro Carvalho Chehab wrote: > Now that media has its own subdirectory inside platform_data, > let's move the headers that are already there to such subdir. > > After moving those files, the references were adjusted using this > script: > > MAIN_DIR="linux/platform_data/" > PREV_DIR="linux/platform_data/" > DIRS="media/" > > echo "Checking affected files" >&2 > for i in $DIRS; do > for j in $(find include/$MAIN_DIR/$i -type f -name '*.h'); do >n=`basename $j` > git grep -l $n > done > done|sort|uniq >files && ( > echo "Handling files..." >&2; > echo "for i in \$(cat files|grep -v Documentation); do cat \$i | \\"; > ( > cd include/$MAIN_DIR; > for j in $DIRS; do > for i in $(ls $j); do > echo "perl -ne 's,(include > [\\\"\\<])$PREV_DIR($i)([\\\"\\>]),\1$MAIN_DIR$j\2\3,; print \$_' |\\"; > done; > done; > echo "cat > a && mv a \$i; done"; > ); > echo "Handling documentation..." >&2; > echo "for i in MAINTAINERS \$(cat files); do cat \$i | \\"; > ( > cd include/$MAIN_DIR; > for j in $DIRS; do > for i in $(ls $j); do > echo " perl -ne > 's,include/$PREV_DIR($i)\b,include/$MAIN_DIR$j\1,; print \$_' |\\"; > done; > done; > echo "cat > a && mv a \$i; done" > ); > ) >script && . ./script > > Suggested-by: Arnd Bergmann > Signed-off-by: Mauro Carvalho Chehab Acked-by: Shawn Guo -- 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
cron job: media_tree daily build: ERRORS
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: Tue Nov 24 04:00:25 CET 2015 git branch: test git hash: 10897dacea26943dd80bd6629117f4620fc320ef gcc version:i686-linux-gcc (GCC) 5.1.0 sparse version: v0.5.0 smatch version: host hardware: x86_64 host os:4.2.0-164 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: OK linux-2.6.32.27-i686: ERRORS linux-2.6.33.7-i686: ERRORS linux-2.6.34.7-i686: ERRORS linux-2.6.35.9-i686: ERRORS linux-2.6.36.4-i686: ERRORS linux-2.6.37.6-i686: ERRORS linux-2.6.38.8-i686: ERRORS linux-2.6.39.4-i686: ERRORS linux-3.0.60-i686: ERRORS linux-3.1.10-i686: ERRORS linux-3.2.37-i686: ERRORS linux-3.3.8-i686: ERRORS linux-3.4.27-i686: ERRORS linux-3.5.7-i686: ERRORS 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-i686: OK linux-4.3-i686: OK linux-4.4-rc1-i686: OK linux-2.6.32.27-x86_64: ERRORS linux-2.6.33.7-x86_64: ERRORS linux-2.6.34.7-x86_64: ERRORS linux-2.6.35.9-x86_64: ERRORS linux-2.6.36.4-x86_64: ERRORS linux-2.6.37.6-x86_64: ERRORS linux-2.6.38.8-x86_64: ERRORS linux-2.6.39.4-x86_64: ERRORS linux-3.0.60-x86_64: ERRORS linux-3.1.10-x86_64: ERRORS linux-3.2.37-x86_64: ERRORS linux-3.3.8-x86_64: ERRORS linux-3.4.27-x86_64: ERRORS linux-3.5.7-x86_64: ERRORS 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-x86_64: OK linux-4.3-x86_64: OK linux-4.4-rc1-x86_64: OK apps: WARNINGS spec-git: OK sparse: ERRORS smatch: OK Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Tuesday.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 0/4] VSP1: Add support for lookup tables
On Mon, Nov 16, 2015 at 06:46:41AM +0200, Laurent Pinchart wrote: > Hello, > > The VSP1 includes two lookup table modules, a 1D LUT and a 3D cubic lookup > table (CLU). This patch series fixes the LUT implementation and adds support > for the CLU. > > The patches are based on top of > > git://linuxtv.org/media_tree.git master > > and have been tested on a Koelsch board. > > Laurent Pinchart (4): > v4l: vsp1: Fix LUT format setting > v4l: vsp1: Add Cubic Look Up Table (CLU) support > ARM: Renesas: r8a7790: Enable CLU support in VSPS > ARM: Renesas: r8a7791: Enable CLU support in VSPS I marked the "ARM: Renesas:" patches as deferred pending the binding being accepted. I know we are moving towards "Renesas:" but could you stick to "shmobile" for now? -- 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] dtv-scan-tables: Add all muxes currently in use for Colombia.
Works for me in conjuction with the latest dvbv5-scan. -Dan --- dvb-t/co-All | 34 ++ 1 file changed, 34 insertions(+) create mode 100644 dvb-t/co-All diff --git a/dvb-t/co-All b/dvb-t/co-All new file mode 100644 index 000..c06056a --- /dev/null +++ b/dvb-t/co-All @@ -0,0 +1,34 @@ +# initial scan file for Colombia, national level +# DVB-T2, 470-860MHz, 6MHz bandwidth +# See: https://es.wikipedia.org/wiki/Televisi%C3%B3n_Digital_Terrestre_en_Colombia +# See: https://www.crcom.gov.co/recursos_user/Documentos_CRC_2012/Actividades_Regulatorias/TDT/documentos_soporte_TDT_20120914.pdf#page=51 + +[CHANNEL] + DELIVERY_SYSTEM = DVBT2 + FREQUENCY = 47300 + BANDWIDTH_HZ = 600 + +[CHANNEL] + DELIVERY_SYSTEM = DVBT2 + FREQUENCY = 47900 + BANDWIDTH_HZ = 600 + +[CHANNEL] + DELIVERY_SYSTEM = DVBT2 + FREQUENCY = 48500 + BANDWIDTH_HZ = 600 + +[CHANNEL] + DELIVERY_SYSTEM = DVBT2 + FREQUENCY = 49100 + BANDWIDTH_HZ = 600 + +[CHANNEL] + DELIVERY_SYSTEM = DVBT2 + FREQUENCY = 55100 + BANDWIDTH_HZ = 600 + +[CHANNEL] + DELIVERY_SYSTEM = DVBT2 + FREQUENCY = 55700 + BANDWIDTH_HZ = 600 -- 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 v8 45/55] [media] media: Use a macro to interate between all interfaces
Hi Mauro,
Thank you for the patch.
In the subject line, s/interate between all/iterate over all/
On Sunday 06 September 2015 09:03:05 Mauro Carvalho Chehab wrote:
> Just like we do with entities, use a similar macro for the
> interfaces loop.
>
> Signed-off-by: Mauro Carvalho Chehab
> Acked-by: Hans Verkuil
>
> diff --git a/drivers/media/dvb-core/dvbdev.c
> b/drivers/media/dvb-core/dvbdev.c index 6babc688801b..f00f1a5f279c 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -578,9 +578,10 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
> }
>
> /* Create indirect interface links for FE->tuner, DVR->demux and CA->ca
*/
> - list_for_each_entry(intf, &mdev->interfaces, list) {
> + media_device_for_each_intf(intf, mdev) {
> if (intf->type == MEDIA_INTF_T_DVB_CA && ca)
> media_create_intf_link(ca, intf, 0);
> +
> if (intf->type == MEDIA_INTF_T_DVB_FE && tuner)
> media_create_intf_link(tuner, intf, 0);
>
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 51807efa505b..f23d686aaac6 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -113,6 +113,11 @@ struct media_device *media_device_find_devres(struct
> device *dev); #define media_device_for_each_entity(entity, mdev)
> \
> list_for_each_entry(entity, &(mdev)->entities, list)
>
> +/* Iterate over all interfaces. */
> +#define media_device_for_each_intf(intf, mdev) \
> + list_for_each_entry(intf, &(mdev)->interfaces, list)
> +
> +
One blank line should be enough.
> #else
> static inline int media_device_register(struct media_device *mdev)
> {
--
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 v8 46/55] [media] media: move mdev list init to gobj
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 09:03:06 Mauro Carvalho Chehab wrote:
> Let's control the topology changes inside the graph_object. So, move the
> addition and removal of interfaces/entities from the mdev lists to
> media_gobj_init() and media_gobj_remove().
>
> The main reason is that mdev should have lists for all object types, as
> the new MC api will require to store objects in separate places.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 134fe7510195..ec98595b8a7a 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -415,7 +415,7 @@ void media_device_unregister(struct media_device *mdev)
> struct media_entity *entity;
> struct media_entity *next;
>
> - list_for_each_entry_safe(entity, next, &mdev->entities, list)
> + list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> media_device_unregister_entity(entity);
>
> device_remove_file(&mdev->devnode.dev, &dev_attr_model);
> @@ -449,7 +449,6 @@ int __must_check media_device_register_entity(struct
> media_device *mdev, spin_lock(&mdev->lock);
> /* Initialize media_gobj embedded at the entity */
> media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
> - list_add_tail(&entity->list, &mdev->entities);
>
> /* Initialize objects at the pads */
> for (i = 0; i < entity->num_pads; i++)
> @@ -487,7 +486,6 @@ void media_device_unregister_entity(struct media_entity
> *entity) for (i = 0; i < entity->num_pads; i++)
> media_gobj_remove(&entity->pads[i].graph_obj);
> media_gobj_remove(&entity->graph_obj);
> - list_del(&entity->list);
> spin_unlock(&mdev->lock);
> entity->graph_obj.mdev = NULL;
> }
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 6ed5eef88593..cbb0604e81c1 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -170,6 +170,7 @@ void media_gobj_init(struct media_device *mdev,
> switch (type) {
> case MEDIA_GRAPH_ENTITY:
> gobj->id = media_gobj_gen_id(type, ++mdev->entity_id);
> + list_add_tail(&gobj->list, &mdev->entities);
> break;
> case MEDIA_GRAPH_PAD:
> gobj->id = media_gobj_gen_id(type, ++mdev->pad_id);
> @@ -178,6 +179,7 @@ void media_gobj_init(struct media_device *mdev,
> gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
> break;
> case MEDIA_GRAPH_INTF_DEVNODE:
> + list_add_tail(&gobj->list, &mdev->interfaces);
> gobj->id = media_gobj_gen_id(type, ++mdev->intf_devnode_id);
> break;
> }
> @@ -193,6 +195,16 @@ void media_gobj_init(struct media_device *mdev,
> */
> void media_gobj_remove(struct media_gobj *gobj)
> {
> + /* Remove the object from mdev list */
> + switch (media_type(gobj)) {
> + case MEDIA_GRAPH_ENTITY:
> + case MEDIA_GRAPH_INTF_DEVNODE:
> + list_del(&gobj->list);
> + break;
Type-specific handling in the graph object code doesn't seem right. I'd keep
the list_del calls in the type-specific remove functions. Same for the
list_add_tail calls above, unless we switch from per-type lists to a single
graph objects list as I mentioned in a reply to another patch (and the more I
think about it the more tempting it gets).
> + default:
> + break;
> + }
> +
> dev_dbg_obj(__func__, gobj);
> }
>
> @@ -864,8 +876,6 @@ static void media_interface_init(struct media_device
> *mdev, INIT_LIST_HEAD(&intf->links);
>
> media_gobj_init(mdev, gobj_type, &intf->graph_obj);
> -
> - list_add_tail(&intf->list, &mdev->interfaces);
> }
>
> /* Functions related to the media interface via device nodes */
> @@ -894,7 +904,6 @@ EXPORT_SYMBOL_GPL(media_devnode_create);
> void media_devnode_remove(struct media_intf_devnode *devnode)
> {
> media_gobj_remove(&devnode->intf.graph_obj);
> - list_del(&devnode->intf.list);
> kfree(devnode);
> }
> EXPORT_SYMBOL_GPL(media_devnode_remove);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index f23d686aaac6..85fa302047bd 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -111,11 +111,11 @@ struct media_device *media_device_find_devres(struct
> device *dev);
>
> /* Iterate over all entities. */
> #define media_device_for_each_entity(entity, mdev) \
> - list_for_each_entry(entity, &(mdev)->entities, list)
> + list_for_each_entry(entity, &(mdev)->entities, graph_obj.list)
>
> /* Iterate over all interfaces. */
> #define media_device_for_each_intf(intf, mdev) \
> - list_for_each_entry(intf, &(mdev)->interfaces, list)
> + list_for_each_entry(intf, &(mdev)->interfaces, graph_obj.list)
>
>
> #else
> d
Re: [PATCH v8 47/55] [media] media-device: add pads and links to media_device
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 09:03:07 Mauro Carvalho Chehab wrote:
> The MC next gen API sends objects to userspace grouped by
> their types.
>
> In the case of pads and links, in order to improve performance
Are we sure it really improves performances ?
> and have a simpler code, the best is to store them also on
> separate linked lists at MC.
Have you considered the approach of storing them in a single list of objects
instead of per-type lists ? I wonder if it could be helpful. What bothers me
here is that we violate layers by using the list field in the graph object
structure to store it in lists specific to the individual graph object types.
Such violations have proved to be bad ideas in most cases, even if I can't
pinpoint why right now. It could of course also be an exception.
> If we don't do that, we would need this kind of interaction
> to send data to userspace (code is in structured english):
>
> for each entity:
> for each pad:
> store pads
>
> for each entity:
> for each link:
> store link
>
> for each interface:
> for each link:
> store link
>
> With would require one nexted loop for pads and two nested
s/nexted loop/nested loop/
> loops for links. By using separate linked lists for them,
> just one loop would be enough.
>
> Signed-off-by: Mauro Carvalho Chehab
> Acked-by: Hans Verkuil
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index ec98595b8a7a..5b2c9f7fcd45 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -382,6 +382,8 @@ int __must_check __media_device_register(struct
> media_device *mdev,
>
> INIT_LIST_HEAD(&mdev->entities);
> INIT_LIST_HEAD(&mdev->interfaces);
> + INIT_LIST_HEAD(&mdev->pads);
> + INIT_LIST_HEAD(&mdev->links);
> spin_lock_init(&mdev->lock);
> mutex_init(&mdev->graph_mutex);
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index cbb0604e81c1..568553d41f5d 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -174,13 +174,15 @@ void media_gobj_init(struct media_device *mdev,
> break;
> case MEDIA_GRAPH_PAD:
> gobj->id = media_gobj_gen_id(type, ++mdev->pad_id);
> + list_add_tail(&gobj->list, &mdev->pads);
> break;
> case MEDIA_GRAPH_LINK:
> gobj->id = media_gobj_gen_id(type, ++mdev->link_id);
> + list_add_tail(&gobj->list, &mdev->links);
> break;
> case MEDIA_GRAPH_INTF_DEVNODE:
> - list_add_tail(&gobj->list, &mdev->interfaces);
> gobj->id = media_gobj_gen_id(type, ++mdev->intf_devnode_id);
> + list_add_tail(&gobj->list, &mdev->interfaces);
> break;
> }
> dev_dbg_obj(__func__, gobj);
> @@ -195,17 +197,10 @@ void media_gobj_init(struct media_device *mdev,
> */
> void media_gobj_remove(struct media_gobj *gobj)
> {
> + dev_dbg_obj(__func__, gobj);
> +
> /* Remove the object from mdev list */
> - switch (media_type(gobj)) {
> - case MEDIA_GRAPH_ENTITY:
> - case MEDIA_GRAPH_INTF_DEVNODE:
> - list_del(&gobj->list);
> - break;
> - default:
> - break;
> - }
> -
> - dev_dbg_obj(__func__, gobj);
> + list_del(&gobj->list);
> }
>
> /**
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index 85fa302047bd..0d1b9c687454 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -47,6 +47,8 @@ struct device;
> * @intf_devnode_id: Unique ID used on the last interface devnode
> registered * @entities: List of registered entities
> * @interfaces: List of registered interfaces
> + * @pads:List of registered pads
> + * @links: List of registered links
> * @lock:Entities list lock
> * @graph_mutex: Entities graph operation lock
> * @link_notify: Link state change notification callback
> @@ -79,6 +81,8 @@ struct media_device {
>
> struct list_head entities;
> struct list_head interfaces;
> + struct list_head pads;
> + struct list_head links;
>
> /* Protects the entities list */
> spinlock_t lock;
> @@ -117,6 +121,14 @@ struct media_device *media_device_find_devres(struct
> device *dev); #define media_device_for_each_intf(intf, mdev)
> \
> list_for_each_entry(intf, &(mdev)->interfaces, graph_obj.list)
>
> +/* Iterate over all pads. */
> +#define media_device_for_each_pad(pad, mdev) \
> + list_for_each_entry(pad, &(mdev)->pads, graph_obj.list)
> +
> +/* Iterate over all links. */
> +#define media_device_for_each_link(link, mdev) \
> + list_for_each_entry(link, &(mdev)->links, graph_obj.list)
> +
>
> #else
> stat
Re: [PATCH v8 48/55] [media] media_device: add a topology version field
Hi Mauro and Hans,
On Monday 31 August 2015 14:29:28 Hans Verkuil wrote:
> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote:
> > Every time a graph object is added or removed, the version
> > of the topology changes. That's a requirement for the new
> > MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know
> > that the topology has changed after a previous call to it.
> >
> > Signed-off-by: Mauro Carvalho Chehab
>
> I think this should be postponed until we actually have dynamic
> reconfigurable graphs.
>
> I would also like to reserve version 0: if 0 is returned, then the graph is
> static.
>
> In G_TOPOLOGY we'd return always 0 for now.
So would I. We need a way to group graph modifications to avoid incrementing
the version number and generating an event for every entity, link or pad added
or removed. As this patch doesn't address that I don't see a use for the
version number now other than making our life more difficult when we'll
implement dynamic updates by forcing us to consider backward compatibility
with something that we know won't do the job.
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index c89f51bc688d..c18f4af52771 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -185,6 +185,9 @@ void media_gobj_init(struct media_device *mdev,
> > list_add_tail(&gobj->list, &mdev->interfaces);
> > break;
> > }
> > +
> > + mdev->topology_version++;
> > +
> > dev_dbg_obj(__func__, gobj);
> > }
> >
> > @@ -199,6 +202,8 @@ void media_gobj_remove(struct media_gobj *gobj)
> > {
> > dev_dbg_obj(__func__, gobj);
> >
> > + gobj->mdev->topology_version++;
> > +
> > /* Remove the object from mdev list */
> > list_del(&gobj->list);
> > }
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index 0d1b9c687454..1b12774a9ab4 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -41,6 +41,8 @@ struct device;
> > * @bus_info: Unique and stable device location identifier
> > * @hw_revision: Hardware device revision
> > * @driver_version: Device driver version
> > + * @topology_version: Monotonic counter for storing the version of the
> > graph
> > + * topology. Should be incremented each time the topology changes.
> > * @entity_id: Unique ID used on the last entity registered
> > * @pad_id:Unique ID used on the last pad registered
> > * @link_id: Unique ID used on the last link registered
> > @@ -74,6 +76,8 @@ struct media_device {
> > u32 hw_revision;
> > u32 driver_version;
> >
> > + u32 topology_version;
> > +
> > u32 entity_id;
> > u32 pad_id;
> > u32 link_id;
--
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 v8 48/55] [media] media_device: add a topology version field
Hi Mauro, On Friday 04 September 2015 14:08:27 Mauro Carvalho Chehab wrote: > Em Mon, 31 Aug 2015 15:35:38 +0200 Hans Verkuil escreveu: > > On 08/31/2015 02:52 PM, Mauro Carvalho Chehab wrote: > >> Em Mon, 31 Aug 2015 14:29:28 +0200 Hans Verkuil escreveu: > >>> On 08/30/2015 05:06 AM, Mauro Carvalho Chehab wrote: > Every time a graph object is added or removed, the version > of the topology changes. That's a requirement for the new > MEDIA_IOC_G_TOPOLOGY, in order to allow userspace to know > that the topology has changed after a previous call to it. > > Signed-off-by: Mauro Carvalho Chehab > >>> > >>> I think this should be postponed until we actually have dynamic > >>> reconfigurable graphs. > >> > >> So far, we're using the term "dynamic" to mean partial graph object > >> removal. > >> > >> But even today, MC does support "dynamic" in the sense of graph object > >> additions. > >> > >> You should notice that having a topology_version is something that IMHO, > >> it is needed since the beginning, even without dynamic reconfigurable > >> graphs, because the graph may grow in runtime. > >> > >> That will happen, for example, if usb-snd-audio is blacklisted at > >> /etc/modprobe*, and someone connects an au0828. > >> > >> New entities/links will be created (after Shuah patches) if one would > >> modprobe latter snd-usb-audio. > > > > latter -> later :-) > > > > You are right, this would trigger a topology change. I hadn't thought > > about that. First of all it won't be very useful without a topology change notification, so until we have them it doesn't matter too much. Then, the code is full of race conditions when it comes to dynamic updates, and I'm afraid Shua's patches can't go in before we fix them. > >>> I would also like to reserve version 0: if 0 is returned, then the graph > >>> is static. > >> > >> Why? Implementing this would be really hard, as that would mean that > >> G_TOPOLOGY would need to be blocked until all drivers and subdevices get > >> probed. > >> > >> In order to implement that, some logic would be needed at the drivers to > >> identify if everything was set and unlock G_TOPOLOGY. > > > > That wouldn't be needed if the media device node was created last. Which I > > think is a good idea anyway. > > Creating the media device node last won't work. It should be the first thing > to be created, as all objects should be added to media_device linked lists. I disagree with that. The media_device needs to be initialized first, but can be registered with userspace last. We don't want to generate a topology update event for every new entity, link or pad during the device probe sequence. Drivers should be in control and tell when they're done with initialization. > Also, the numberspace should be local to a given media_device, as the graph > traversal algorithm relies on having the number of entities <= 64, > currently, in order to be able to detect loops. We should increase that > number, but removing an "as low as possible" entity number limit is not > trivial. > > > > What would be the gain for that? I fail to see any. > > > > It would tell userspace that it doesn't have to cope with dynamically > > changing graphs. > > > > Even though with the au0828 example you can expect to see cases like that, > > I can pretty much guarantee that no generic v4l2 applications will ever > > support dynamic changes. > > Well, my test app supports it and it is generic ;) My plan is to use it as a > basis for the library to be used on userspace for generic apps, extending it > to be used by other tools like xawtv, qv4l2 and the dvbv5 apps. > > I don't think it is hard to handle it on a generic app, I'll quote you later on that :-) > and this should be done anyway if we want dynamic support. > > The logic seems actually be simple: > > at G_TOPOLOGY, if the topology changes, reload the objects; And update everything in userspace. That's a very hard task if you don't design your applications extremely carefully. > at SETUP_LINK_V2, the topology will be sent. if the driver detects that > topology changed, it returns an error. > > The caller will then need to reload the topology and re-apply the > transaction to select the links, if the entities affected still exists. In > other words, if the user's intent were to change the pipeline to receive the > data at /dev/video2, e. g. something like: > ./yavta/yavta -f UYVY -s 720x312 -n 1 --capture=1 -F /dev/video2 > > What userspace would do is to reload everything, check if /dev/video2 still > exists and then redo the function that it is equivalent to the above > command, failing otherwise. That doesn't sound hard to implement. > > > Those that will support it will be custom-made. > > > > Being able to see that graphs can change dynamically would allow such apps > > to either refuse to use the device, or warn the user. > > The way I see is that applications that will assume that the graph > i
Re: [PATCH v8 49/55] [media] media-device: add support for MEDIA_IOC_G_TOPOLOGY ioctl
Hi Mauro,
Thank you for the patch.
Sakari and Hans have made several comments on this and the previous version of
the same patch and I generally agree with them. I'll thus review the next
version.
On Sunday 06 September 2015 09:03:09 Mauro Carvalho Chehab wrote:
> Add support for the new MEDIA_IOC_G_TOPOLOGY ioctl, according
> with the RFC for the MC next generation.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 5b2c9f7fcd45..96a476eeb16e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -232,6 +232,156 @@ static long media_device_setup_link(struct
> media_device *mdev, return ret;
> }
>
> +static long __media_device_get_topology(struct media_device *mdev,
> + struct media_v2_topology *topo)
> +{
> + struct media_entity *entity;
> + struct media_interface *intf;
> + struct media_pad *pad;
> + struct media_link *link;
> + struct media_v2_entity uentity;
> + struct media_v2_interface uintf;
> + struct media_v2_pad upad;
> + struct media_v2_link ulink;
> + int ret = 0, i;
> +
> + topo->topology_version = mdev->topology_version;
> +
> + /* Get entities and number of entities */
> + i = 0;
> + media_device_for_each_entity(entity, mdev) {
> + i++;
> +
> + if (ret || !topo->entities)
> + continue;
> +
> + if (i > topo->num_entities) {
> + ret = -ENOSPC;
> + continue;
> + }
> +
> + /* Copy fields to userspace struct if not error */
> + memset(&uentity, 0, sizeof(uentity));
> + uentity.id = entity->graph_obj.id;
> + strncpy(uentity.name, entity->name,
> + sizeof(uentity.name));
> +
> + if (copy_to_user(&topo->entities[i - 1], &uentity,
> sizeof(uentity)))
> + ret = -EFAULT;
> + }
> + topo->num_entities = i;
> +
> + /* Get interfaces and number of interfaces */
> + i = 0;
> + media_device_for_each_intf(intf, mdev) {
> + i++;
> +
> + if (ret || !topo->interfaces)
> + continue;
> +
> + if (i > topo->num_interfaces) {
> + ret = -ENOSPC;
> + continue;
> + }
> +
> + memset(&uintf, 0, sizeof(uintf));
> +
> + /* Copy intf fields to userspace struct */
> + uintf.id = intf->graph_obj.id;
> + uintf.intf_type = intf->type;
> + uintf.flags = intf->flags;
> +
> + if (media_type(&intf->graph_obj) == MEDIA_GRAPH_INTF_DEVNODE) {
> + struct media_intf_devnode *devnode;
> +
> + devnode = intf_to_devnode(intf);
> +
> + uintf.devnode.major = devnode->major;
> + uintf.devnode.minor = devnode->minor;
> + }
> +
> + if (copy_to_user(&topo->interfaces[i - 1], &uintf,
> sizeof(uintf)))
> + ret = -EFAULT;
> + }
> + topo->num_interfaces = i;
> +
> + /* Get pads and number of pads */
> + i = 0;
> + media_device_for_each_pad(pad, mdev) {
> + i++;
> +
> + if (ret || !topo->pads)
> + continue;
> +
> + if (i > topo->num_pads) {
> + ret = -ENOSPC;
> + continue;
> + }
> +
> + memset(&upad, 0, sizeof(upad));
> +
> + /* Copy pad fields to userspace struct */
> + upad.id = pad->graph_obj.id;
> + upad.entity_id = pad->entity->graph_obj.id;
> + upad.flags = pad->flags;
> +
> + if (copy_to_user(&topo->pads[i - 1], &upad, sizeof(upad)))
> + ret = -EFAULT;
> + }
> + topo->num_pads = i;
> +
> + /* Get links and number of links */
> + i = 0;
> + media_device_for_each_link(link, mdev) {
> + i++;
> +
> + if (ret || !topo->links)
> + continue;
> +
> + if (i > topo->num_links) {
> + ret = -ENOSPC;
> + continue;
> + }
> +
> + memset(&ulink, 0, sizeof(ulink));
> +
> + /* Copy link fields to userspace struct */
> + ulink.id = link->graph_obj.id;
> + ulink.source_id = link->gobj0->id;
> + ulink.sink_id = link->gobj1->id;
> + ulink.flags = link->flags;
> +
> + if (media_type(link->gobj0) != MEDIA_GRAPH_PAD)
> + ulink.flags |= MEDIA_LNK_FL_INTERFACE_LINK;
> +
> + if (copy_to_user(&topo->links[i - 1], &ulink, sizeof(ulink)))
> + ret = -EFAULT;
> + }
> + topo->num_links = i;
> +
> + return ret;
> +}
> +
> +static long media_device_get_topology(struct med
Re: [PATCH v8 50/55] [media] media-entity: unregister entity links
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 09:03:10 Mauro Carvalho Chehab wrote:
> Add functions to explicitly unregister all entity links.
> This function is called automatically when an entity
> link is destroyed.
>
> Signed-off-by: Mauro Carvalho Chehab
> Acked-by: Hans Verkuil
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 064515f2ba9b..a37ccd2edfd5 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -903,6 +903,7 @@ EXPORT_SYMBOL_GPL(media_devnode_create);
>
> void media_devnode_remove(struct media_intf_devnode *devnode)
> {
> + media_remove_intf_links(&devnode->intf);
> media_gobj_remove(&devnode->intf.graph_obj);
> kfree(devnode);
> }
> @@ -944,3 +945,25 @@ void media_remove_intf_link(struct media_link *link)
> mutex_unlock(&link->graph_obj.mdev->graph_mutex);
> }
> EXPORT_SYMBOL_GPL(media_remove_intf_link);
> +
> +void __media_remove_intf_links(struct media_interface *intf)
> +{
> + struct media_link *link, *tmp;
> +
> + list_for_each_entry_safe(link, tmp, &intf->links, list)
> + __media_remove_intf_link(link);
> +
> +}
> +EXPORT_SYMBOL_GPL(__media_remove_intf_links);
The only place where this function is used is in media_remove_intf_links()
below. How about inlining it for now ?
> +void media_remove_intf_links(struct media_interface *intf)
> +{
> + /* Do nothing if the intf is not registered. */
> + if (intf->graph_obj.mdev == NULL)
> + return;
> +
> + mutex_lock(&intf->graph_obj.mdev->graph_mutex);
> + __media_remove_intf_links(intf);
> + mutex_unlock(&intf->graph_obj.mdev->graph_mutex);
> +}
> +EXPORT_SYMBOL_GPL(media_remove_intf_links);
As this function is exported it should be documented with kerneldoc.
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index bc7eb6240795..ca4a4f23362f 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -318,6 +318,9 @@ struct media_link *media_create_intf_link(struct
> media_entity *entity, struct media_interface *intf,
> u32 flags);
> void media_remove_intf_link(struct media_link *link);
> +void __media_remove_intf_links(struct media_interface *intf);
> +void media_remove_intf_links(struct media_interface *intf);
> +
>
> #define media_entity_call(entity, operation, args...)
> \
> (((entity)->ops && (entity)->ops->operation) ? \
--
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 v8 52/55] [media] media-device: remove interfaces and interface links
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 09:03:12 Mauro Carvalho Chehab wrote:
> Just like what's done with entities, when the media controller is
> unregistered, release any interface and interface links that
> might still be there.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 7c37aeab05bb..0238885fcc74 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -574,6 +574,22 @@ void media_device_unregister(struct media_device *mdev)
> {
> struct media_entity *entity;
> struct media_entity *next;
> + struct media_link *link, *tmp_link;
> + struct media_interface *intf, *tmp_intf;
> +
> + /* Remove interface links from the media device */
> + list_for_each_entry_safe(link, tmp_link, &mdev->links,
> + graph_obj.list) {
> + media_gobj_remove(&link->graph_obj);
> + kfree(link);
> + }
> +
> + /* Remove all interfaces from the media device */
> + list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces,
> + graph_obj.list) {
> + media_gobj_remove(&intf->graph_obj);
> + kfree(intf);
> + }
>
> list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
> media_device_unregister_entity(entity);
> @@ -651,7 +667,6 @@ void media_device_unregister_entity(struct media_entity
> *entity) /* Remove all data links that belong to this entity */
> list_for_each_entry_safe(link, tmp, &entity->links, list) {
> media_gobj_remove(&link->graph_obj);
> - list_del(&link->list);
> kfree(link);
The link has already been freed in media_device_unregister(). You have access-
after-free and double-free issues here.
> }
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index a37ccd2edfd5..cd4d767644df 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -206,6 +206,10 @@ void media_gobj_remove(struct media_gobj *gobj)
>
> /* Remove the object from mdev list */
> list_del(&gobj->list);
> +
> + /* Links have their own list - we need to drop them there too */
> + if (media_type(gobj) == MEDIA_GRAPH_LINK)
> + list_del(&gobj_to_link(gobj)->list);
Please... That's a very bad layering violation. Let's not do that. Generic
graph object code should not contain any type-specific code. You can create a
media_link_remove() function for links that will remove the link from the
entity links list and call media_gobj_remove().
> }
>
> /**
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index ca4a4f23362f..fb5f0e21f137 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -153,7 +153,7 @@ struct media_entity {
> };
>
> /**
> - * struct media_intf_devnode - Define a Kernel API interface
> + * struct media_interface - Define a Kernel API interface
This belongs to a different patch ;-)
> *
> * @graph_obj: embedded graph object
> * @list:Linked list used to find other interfaces that belong
> @@ -163,6 +163,11 @@ struct media_entity {
> * uapi/media/media.h header, e. g.
> * MEDIA_INTF_T_*
> * @flags: Interface flags as defined at uapi/media/media.h
> + *
> + * NOTE: As media_device_unregister() will free the address of the
> + *media_interface, this structure should be embedded as the first
> + *element of the derived functions, in order for the address to be
> + *the same.
s/NOTE/DIRTY HACK/
Or, much better, let's fix it :-) If you want to be able to destroy graph
object without needing to know their type, you can add a destroy operation to
the graph objects and have per-type implementations. There's probably other
options as well.
> */
> struct media_interface {
> struct media_gobj graph_obj;
> @@ -179,11 +184,11 @@ struct media_interface {
> * @minor: Minor number of a device node
> */
> struct media_intf_devnode {
> - struct media_interface intf;
> + struct media_interface intf; /* must be first field in struct */
>
> /* Should match the fields at media_v2_intf_devnode */
> - u32 major;
> - u32 minor;
> + u32 major;
> + u32 minor;
This doesn't belong to this patch either.
> };
>
> static inline u32 media_entity_id(struct media_entity *entity)
--
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 v8 53/55] [media] v4l2-core: create MC interfaces for devnodes
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 09:03:13 Mauro Carvalho Chehab wrote:
> V4L2 device (and subdevice) nodes should create an interface, if the
> Media Controller support is enabled.
>
> Please notice that radio devices should not create an entity, as radio
> input/output is either via wires or via ALSA.
I'd go one step further, video nodes should not create an entity, ever. This
was one of the design principles behind the MC rework. We'll need to patch
drivers to create DMA engine entity explicitly, possibly through a helper
function (maybe a function to create and initialize a DMA engine entity based
on a struct video_device). This can of course be done on a separate patch, but
removing the entity field from struct video_device should be part of the
series.
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> b/drivers/media/v4l2-core/v4l2-dev.c index 44b330589787..07123dd569c4
> 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -194,9 +194,12 @@ static void v4l2_device_release(struct device *cd)
> mutex_unlock(&videodev_lock);
>
> #if defined(CONFIG_MEDIA_CONTROLLER)
> - if (v4l2_dev->mdev &&
> - vdev->vfl_type != VFL_TYPE_SUBDEV)
> - media_device_unregister_entity(&vdev->entity);
> + if (v4l2_dev->mdev) {
> + /* Remove interfaces and interface links */
> + media_devnode_remove(vdev->intf_devnode);
> + if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN)
> + media_device_unregister_entity(&vdev->entity);
> + }
> #endif
>
> /* Do not call v4l2_device_put if there is no release callback set.
> @@ -713,6 +716,92 @@ static void determine_valid_ioctls(struct video_device
> *vdev) BASE_VIDIOC_PRIVATE);
> }
>
> +
Extra blank line.
> +static int video_register_media_controller(struct video_device *vdev, int
> type)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + u32 intf_type;
> + int ret;
> +
> + if (!vdev->v4l2_dev->mdev)
> + return 0;
> +
> + vdev->entity.type = MEDIA_ENT_T_UNKNOWN;
> +
> + switch (type) {
> + case VFL_TYPE_GRABBER:
> + intf_type = MEDIA_INTF_T_V4L_VIDEO;
> + vdev->entity.type = MEDIA_ENT_T_V4L2_VIDEO;
> + break;
> + case VFL_TYPE_VBI:
> + intf_type = MEDIA_INTF_T_V4L_VBI;
> + vdev->entity.type = MEDIA_ENT_T_V4L2_VBI;
> + break;
> + case VFL_TYPE_SDR:
> + intf_type = MEDIA_INTF_T_V4L_SWRADIO;
> + vdev->entity.type = MEDIA_ENT_T_V4L2_SWRADIO;
> + break;
> + case VFL_TYPE_RADIO:
> + intf_type = MEDIA_INTF_T_V4L_RADIO;
> + /*
> + * Radio doesn't have an entity at the V4L2 side to represent
> + * radio input or output. Instead, the audio input/output goes
> + * via either physical wires or ALSA.
> + */
> + break;
> + case VFL_TYPE_SUBDEV:
> + intf_type = MEDIA_INTF_T_V4L_SUBDEV;
> + /* Entity will be created via v4l2_device_register_subdev() */
> + break;
> + default:
> + return 0;
> + }
> +
> + if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN) {
> + vdev->entity.name = vdev->name;
> +
> + /* Needed just for backward compatibility with legacy MC API */
> + vdev->entity.info.dev.major = VIDEO_MAJOR;
> + vdev->entity.info.dev.minor = vdev->minor;
> +
> + ret = media_device_register_entity(vdev->v4l2_dev->mdev,
> +&vdev->entity);
> + if (ret < 0) {
> + printk(KERN_WARNING
> + "%s: media_device_register_entity failed\n",
> + __func__);
> + return ret;
> + }
> + }
> +
> + vdev->intf_devnode = media_devnode_create(vdev->v4l2_dev->mdev,
> + intf_type,
> + 0, VIDEO_MAJOR,
> + vdev->minor,
> + GFP_KERNEL);
> + if (!vdev->intf_devnode) {
> + media_device_unregister_entity(&vdev->entity);
> + return -ENOMEM;
> + }
> +
> + if (vdev->entity.type != MEDIA_ENT_T_UNKNOWN) {
> + struct media_link *link;
> +
> + link = media_create_intf_link(&vdev->entity,
> + &vdev->intf_devnode->intf, 0);
> + if (!link) {
> + media_devnode_remove(vdev->intf_devnode);
> + media_device_unregister_entity(&vdev->entity);
> + return -ENOMEM;
> + }
> + }
> +
> + /* FIXME: how to create the other interface l
Re: [media-workshop] [PATCH v8.4 81/83] [media] media-entity: init pads on entity init if was registered before
Hello Laurent,
On 11/23/2015 01:20 PM, Laurent Pinchart wrote:
> Hi Javier,
>
> (Replying to linux-media instead of media-workshop, I can't find this patch
> in
> my linux-media folder)
>
> Thank you for the patch.
>
Thanks for your feedback.
> On Monday 12 October 2015 13:44:10 Mauro Carvalho Chehab wrote:
>> From: Javier Martinez Canillas
>>
>> If an entity is registered with a media device before is initialized
>> with media_device_register_entity(), the number of pads won't be set
>> so media_device_register_entity() won't create pad objects and add
>> it to the media device pads list.
>>
>> Do this at entity initialization time if the entity was registered
>> before so the graph is complete and correct regardless of the order
>> in which the entities are initialized and registered.
>
> We now have two places where the pads graph objects are initialized and that
> looks like a bug to me as media_gobj_init() allocates IDs and, even worse,
> adds the entity to the media device entities list.
>
Yes but the idea of this patch was in fact to make it less error prone and
more robust since entities could either be initialized and later registered
or first be registered and then later initialized.
> We need to standardize on where graph objects are initialized across the
> different kind of objects and document this clearly otherwise drivers will
> get
> it wrong.
>
I'm OK with having more strict rules of the order in which objects should
be initialized and registered and force all drivers to follow these docs.
>> Suggested-by: Mauro Carvalho Chehab
This patch was suggested by Mauro so I'll also let him to comment what he
prefers or if he has another opinion on this.
>> Signed-off-by: Javier Martinez Canillas
>> ---
>> drivers/media/media-entity.c | 10 ++
>> 1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index f28265864b76..2c984fb7d497 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -238,6 +238,7 @@ int
>> media_entity_init(struct media_entity *entity, u16 num_pads,
>>struct media_pad *pads)
>> {
>> +struct media_device *mdev = entity->graph_obj.mdev;
>> unsigned int i;
>>
>> entity->group_id = 0;
>> @@ -246,11 +247,20 @@ media_entity_init(struct media_entity *entity, u16
>> num_pads, entity->num_pads = num_pads;
>> entity->pads = pads;
>>
>> +if (mdev)
>> +spin_lock(&mdev->lock);
>> +
>> for (i = 0; i < num_pads; i++) {
>> pads[i].entity = entity;
>> pads[i].index = i;
>> +if (mdev)
>> +media_gobj_init(mdev, MEDIA_GRAPH_PAD,
>> +&entity->pads[i].graph_obj);
>> }
>>
>> +if (mdev)
>> +spin_unlock(&mdev->lock);
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL_GPL(media_entity_init);
>
Best regards,
--
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
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 v8 55/55] [media] media-entity.h: document all the structs
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 09:03:14 Mauro Carvalho Chehab wrote:
> Only a few structs are documented on kernel-doc-nano format
> (the ones added by the MC next gen patches).
>
> Add a documentation for all structs, and ensure that they'll
> be producing the documentation at the Kernel's device driver
> DocBook.
Very good idea ! Please see below for some spelling mistakes or other
corrections in addition to the ones pointed out by Hans.
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fb5f0e21f137..e1a89899deef 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -55,11 +55,13 @@ enum media_gobj_type {
> /**
> * struct media_gobj - Define a graph object.
> *
> + * @mdev:Pointer to the struct media_device that owns the 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").
> + * @list:Linked list associated to one of the per-type mdev object lists
I'd say "List entry stored in one of the ..." (or "List head" if you prefer).
> *
> * All objects on the media graph should have this struct embedded
> */
> @@ -73,6 +75,28 @@ struct media_gobj {
> struct media_pipeline {
> };
>
> +/**
> + * struct media_link - Define a media link graph object.
I think you can drop the "Define" and just say "A media link graph object".
Or, rather, "A media graph link object" to mean "a link object part of a media
graph".
Same comment for the other structures.
> + *
> + * @graph_obj: Embedded structure containing the media object common
> data
> + * @list:Linked list associated with an entity or an interface that
> + * owns the link.
> + * @gobj0: Part of an union. Used to get the pointer for the first
> + * graph_object of the link.
> + * @source: Part of an union. Used only if the first object (gobj0) is
> + * a pad. On such case, it represents the source pad.
> + * @intf:Part of an union. Used only if the first object (gobj0) is
> + * an interface.
> + * @gobj1: Part of an union. Used to get the pointer for the second
> + * graph_object of the link.
> + * @source: Part of an union. Used only if the second object (gobj0) is
s/gobj0/gobj1/
> + * a pad. On such case, it represents the sink pad.
> + * @entity: Part of an union. Used only if the second object (gobj0) is
Ditto.
> + * an entity.
> + * @reverse: Pointer to the link for the reverse direction of a pad to
pad
> + * link.
> + * @flags: Link flags, as defined at uapi/media.h (MEDIA_LNK_FL_*)
> + */
> struct media_link {
> struct media_gobj graph_obj;
> struct list_head list;
> @@ -86,15 +110,23 @@ struct media_link {
> struct media_pad *sink;
> struct media_entity *entity;
> };
> - struct media_link *reverse; /* Link in the reverse direction */
> - unsigned long flags;/* Link flags (MEDIA_LNK_FL_*) */
> + struct media_link *reverse;
> + unsigned long flags;
> };
>
> +/**
> + * struct media_pad - Define a media pad graph object.
> + *
> + * @graph_obj: Embedded structure containing the media object common
> data
> + * @entity: Entity where this object belongs
"Entity this pad belongs to"
> + * @index: Pad index in the entity pads array, numbered from 0 to n
> + * @flags: Pad flags, as defined at uapi/media.h (MEDIA_PAD_FL_*)
> + */
> struct media_pad {
> struct media_gobj graph_obj;/* must be first field in struct */
> - struct media_entity *entity;/* Entity this pad belongs to */
> - u16 index; /* Pad index in the entity pads array */
> - unsigned long flags;/* Pad flags (MEDIA_PAD_FL_*) */
> + struct media_entity *entity;
> + u16 index;
> + unsigned long flags;
> };
>
> /**
> @@ -113,51 +145,73 @@ struct media_entity_operations {
> int (*link_validate)(struct media_link *link);
> };
>
> +/**
> + * struct media_entity - Define a media entity graph object.
> + *
> + * @graph_obj: Embedded structure containing the media object common
> data.
> + * @name:Entity name.
> + * @type:Entity type, as defined at uapi/media.h (MEDIA_ENT_T_*)
> + * @revision:Entity revision - OBSOLETE - should be removed soon.
> + * @flags: Entity flags, as defined at uapi/media.h (MEDIA_ENT_FL_*)
> + * @group_id:Entity group ID - OBSOLETE - should be removed soon.
> + * @num_pads:Number of sink and source pads.
> + * @num_links: Number of existing links, both enabled and disabled.
I'd mention there that it includes the backlinks too as it's always c
Re: [PATCH 01/18] [media] tuner-core: add an input pad
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 14:30:44 Mauro Carvalho Chehab wrote:
> Tuners actually have at least one connector on its
> input.
>
> Add a PAD to connect it.
The patch looks fine to me, but have you checked that there are no driver
instantiating a tuner that would get confused by this additional pad, for
instance in graph traversal code ?
Additionally it should be documented somewhere that drivers instantiating
tuners are responsible for creating and linking a connector to the tuner
input.
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/dvb-core/dvbdev.c
> b/drivers/media/dvb-core/dvbdev.c index f00f1a5f279c..a8e7e2398f7a 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -34,6 +34,9 @@
> #include
> #include "dvbdev.h"
>
> +/* Due to enum tuner_pad_index */
> +#include
> +
> static DEFINE_MUTEX(dvbdev_mutex);
> static int dvbdev_debug;
>
> @@ -552,7 +555,7 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
> }
>
> if (tuner && demod)
> - media_create_pad_link(tuner, 0, demod, 0, 0);
> + media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, demod, 0, 0);
>
> if (demod && demux)
> media_create_pad_link(demod, 1, demux, 0, MEDIA_LNK_FL_ENABLED);
> diff --git a/drivers/media/usb/au0828/au0828-core.c
> b/drivers/media/usb/au0828/au0828-core.c index e28cabe65934..f54c7d10f350
> 100644
> --- a/drivers/media/usb/au0828/au0828-core.c
> +++ b/drivers/media/usb/au0828/au0828-core.c
> @@ -27,6 +27,9 @@
> #include
> #include
>
> +/* Due to enum tuner_pad_index */
> +#include
> +
> /*
> * 1 = General debug messages
> * 2 = USB handling
> @@ -260,7 +263,7 @@ static void au0828_create_media_graph(struct au0828_dev
> *dev) return;
>
> if (tuner)
> - media_create_pad_link(tuner, 0, decoder, 0,
> + media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
> MEDIA_LNK_FL_ENABLED);
> media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
> MEDIA_LNK_FL_ENABLED);
> diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c
> b/drivers/media/usb/cx231xx/cx231xx-cards.c index
> 3b5c9ae39ad3..1070d87efc65 100644
> --- a/drivers/media/usb/cx231xx/cx231xx-cards.c
> +++ b/drivers/media/usb/cx231xx/cx231xx-cards.c
> @@ -1264,7 +1264,7 @@ static void cx231xx_create_media_graph(struct cx231xx
> *dev) return;
>
> if (tuner)
> - media_create_pad_link(tuner, 0, decoder, 0,
> + media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0,
>MEDIA_LNK_FL_ENABLED);
> media_create_pad_link(decoder, 1, &dev->vdev.entity, 0,
>MEDIA_LNK_FL_ENABLED);
> diff --git a/drivers/media/v4l2-core/tuner-core.c
> b/drivers/media/v4l2-core/tuner-core.c index 100b8f069640..b90f2a52db96
> 100644
> --- a/drivers/media/v4l2-core/tuner-core.c
> +++ b/drivers/media/v4l2-core/tuner-core.c
> @@ -134,8 +134,9 @@ struct tuner {
> unsigned inttype; /* chip type id */
> void*config;
> const char *name;
> +
> #if defined(CONFIG_MEDIA_CONTROLLER)
> - struct media_padpad;
> + struct media_padpad[TUNER_NUM_PADS];
> #endif
> };
>
> @@ -695,11 +696,12 @@ static int tuner_probe(struct i2c_client *client,
> /* Should be just before return */
> register_client:
> #if defined(CONFIG_MEDIA_CONTROLLER)
> - t->pad.flags = MEDIA_PAD_FL_SOURCE;
> + t->pad[TUNER_PAD_RF_INPUT].flags = MEDIA_PAD_FL_SINK;
> + t->pad[TUNER_PAD_IF_OUTPUT].flags = MEDIA_PAD_FL_SOURCE;
> t->sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_TUNER;
> t->sd.entity.name = t->name;
>
> - ret = media_entity_init(&t->sd.entity, 1, &t->pad);
> + ret = media_entity_init(&t->sd.entity, TUNER_NUM_PADS, &t->pad[0]);
> if (ret < 0) {
> tuner_err("failed to initialize media entity!\n");
> kfree(t);
> diff --git a/include/media/tuner.h b/include/media/tuner.h
> index b46ebb48fe74..95835c8069dd 100644
> --- a/include/media/tuner.h
> +++ b/include/media/tuner.h
> @@ -25,6 +25,14 @@
>
> #include
>
> +/* Tuner PADs */
> +/* FIXME: is this the right place for it? */
> +enum tuner_pad_index {
> + TUNER_PAD_RF_INPUT,
> + TUNER_PAD_IF_OUTPUT,
> + TUNER_NUM_PADS
> +};
> +
> #define ADDR_UNSET (255)
>
> #define TUNER_TEMIC_PAL 0/* 4002 FH5 (3X 7756,
> 9483) */
--
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 04/18] [media] media-device: supress backlinks at G_TOPOLOGY ioctl
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 14:30:47 Mauro Carvalho Chehab wrote:
> Due to the graph traversal algorithm currently in usage, we
> need a copy of all data links. Those backlinks should not be
> send to userspace, as otherwise, all links there will be
> duplicated.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 0238885fcc74..97eb97d9b662 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -333,6 +333,9 @@ static long __media_device_get_topology(struct
> media_device *mdev, /* Get links and number of links */
> i = 0;
> media_device_for_each_link(link, mdev) {
> + if (link->is_backlink)
> + continue;
> +
> i++;
>
> if (ret || !topo->links)
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index cd4d767644df..4868b8269204 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -648,6 +648,7 @@ media_create_pad_link(struct media_entity *source, u16
> source_pad, backlink->source = &source->pads[source_pad];
> backlink->sink = &sink->pads[sink_pad];
> backlink->flags = flags;
> + backlink->is_backlink = true;
>
> /* Initialize graph object embedded at the new link */
> media_gobj_init(sink->graph_obj.mdev, MEDIA_GRAPH_LINK,
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index e1a89899deef..3d389f142a1d 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -96,6 +96,7 @@ struct media_pipeline {
> * @reverse: Pointer to the link for the reverse direction of a pad to
pad
> * link.
> * @flags: Link flags, as defined at uapi/media.h (MEDIA_LNK_FL_*)
> + * @is_backlink: Indicate if the link is a backlink.
> */
> struct media_link {
> struct media_gobj graph_obj;
> @@ -112,6 +113,7 @@ struct media_link {
> };
> struct media_link *reverse;
> unsigned long flags;
> + bool is_backlink;
I agree with the purpose of this patch (and as stated for other patches in the
series I believe you should squash it with the patch that introduces the
G_TOPOLOGY ioctl) but I won't whether you couldn't do with the additional
variable by adding a flag for backlinks (flag that wouldn't be shown to
userspace).
Now that I think about it an even better implementation could be to avoid
creating backlinks at all. As links are now dynamically allocated you could
have two struct list_head in the link structure, one for the source and one
for the sink. It sounds too easy to be true, I wonder if I'm overlooking
something.
> };
>
> /**
--
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 05/18] [media] media-controller: enable all interface links at init
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 14:30:48 Mauro Carvalho Chehab wrote:
> Interface links are normally enabled, meaning that the interfaces are
> bound to the entities. So, any ioctl send to the interface are reflected
s/send/sent/
> at the entities managed by the interface.
>
> However, when a device is usage, other interfaces for the same hardware
s/usage/in use/
> could be decoupled from the entities linked to them, because the
> hardware may have some parts busy.
>
> That's for example, what happens when an hybrid TV device is in usage.
s/usage/use/
> If it is streaming analog TV or capturing signals from S-Video/Composite
> connectors, typically the digital part of the hardware can't be used and
> vice-versa.
>
> This is generally due to some internal hardware or firmware limitation,
> that it is not easily mapped via data pipelines.
>
> What the Kernel drivers do internally is that they decouple the hardware
> from the interface. So, all changes, if allowed, are done only at some
> interface cache, but not physically changed at the hardware.
>
> The usage is similar to the usage of the MEDIA_LNK_FL_ENABLED on data
> links. So, let's use the same flag to indicate if ether the interface
s/ether/either/
> to entity link is bound/enabled or not.
I believe we'll need to experiment with the interface links to see what's
really needed there. As a general rule I'd like to avoid exposing too much
information to userspace without a clear indication that the information is
actually needed, as it's always easier to expose additional information later
than to remove information already exposed.
For this reason I'd like to see as a first step how we would do in userspace
without making those links dynamic. If we then realize that we're lacking
information we'll decide on the best course of action and on exactly what to
expose and how to expose it, using concrete userspace use cases.
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/dvb-core/dvbdev.c
> b/drivers/media/dvb-core/dvbdev.c index a8e7e2398f7a..5c4fb41060b4 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -396,7 +396,8 @@ static void dvb_register_media_device(struct dvb_device
> *dvbdev, if (!dvbdev->entity || !dvbdev->intf_devnode)
> return;
>
> - media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf, 0);
> + media_create_intf_link(dvbdev->entity, &dvbdev->intf_devnode->intf,
> +MEDIA_LNK_FL_ENABLED);
>
> #endif
> }
> @@ -583,20 +584,24 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
> /* Create indirect interface links for FE->tuner, DVR->demux and CA->ca
*/
> media_device_for_each_intf(intf, mdev) {
> if (intf->type == MEDIA_INTF_T_DVB_CA && ca)
> - media_create_intf_link(ca, intf, 0);
> + media_create_intf_link(ca, intf, MEDIA_LNK_FL_ENABLED);
>
> if (intf->type == MEDIA_INTF_T_DVB_FE && tuner)
> - media_create_intf_link(tuner, intf, 0);
> + media_create_intf_link(tuner, intf,
> +MEDIA_LNK_FL_ENABLED);
>
> if (intf->type == MEDIA_INTF_T_DVB_DVR && demux)
> - media_create_intf_link(demux, intf, 0);
> + media_create_intf_link(demux, intf,
> +MEDIA_LNK_FL_ENABLED);
>
> media_device_for_each_entity(entity, mdev) {
> if (entity->type == MEDIA_ENT_T_DVB_TSOUT) {
> if (!strcmp(entity->name, DVR_TSOUT))
> - media_create_intf_link(entity, intf, 0);
> + media_create_intf_link(entity, intf,
> +
> MEDIA_LNK_FL_ENABLED);
> if (!strcmp(entity->name, DEMUX_TSOUT))
> - media_create_intf_link(entity, intf, 0);
> + media_create_intf_link(entity, intf,
> +
> MEDIA_LNK_FL_ENABLED);
> break;
> }
> }
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> b/drivers/media/v4l2-core/v4l2-dev.c index 07123dd569c4..8429da66754a
> 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -788,7 +788,8 @@ static int video_register_media_controller(struct
> video_device *vdev, int type) struct media_link *link;
>
> link = media_create_intf_link(&vdev->entity,
> - &vdev->intf_devnode->intf, 0);
> + &vdev->intf_devnode->intf,
> + MEDIA_LNK_FL_
Re: [PATCH] tda10071: Fix dependency to REGMAP_I2C
Moikka! Thank you for the patch! Usually these are nowadays found by automated build test, but for some reason not that one... On 11/23/2015 09:19 PM, Matthias Schwarzott wrote: Without I get this error for by dvb-card: tda10071: Unknown symbol devm_regmap_init_i2c (err 0) cx23885_dvb_register() dvb_register failed err = -22 cx23885_dev_setup() Failed to register dvb adapters on VID_B Signed-off-by: Matthias Schwarzott Reviewed-by: Antti Palosaari --- drivers/media/dvb-frontends/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/Kconfig b/drivers/media/dvb-frontends/Kconfig index 292c947..310e4b8 100644 --- a/drivers/media/dvb-frontends/Kconfig +++ b/drivers/media/dvb-frontends/Kconfig @@ -264,7 +264,7 @@ config DVB_MB86A16 config DVB_TDA10071 tristate "NXP TDA10071" depends on DVB_CORE && I2C - select REGMAP + select REGMAP_I2C default m if !MEDIA_SUBDRV_AUTOSELECT help Say Y when you want to support this frontend. -- http://palosaari.fi/ -- 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
media_build using regmap-i2c
Hi! I noticed for a media_build installation, that tda10071 was not loadable because regmap-i2c did not exist. The only way I saw to get regmap-i2c compiled, is to enable any other kernel module that has a "select REGMAP_I2C" in Kconfig. I choose "CONFIG_SENSORS_ADS7828=m". Maybe the kernel needs a way to enable this module "for external drivers" as the "Library routines" folders. Regards Matthias -- 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] tda10071: Fix dependency to REGMAP_I2C
Without I get this error for by dvb-card: tda10071: Unknown symbol devm_regmap_init_i2c (err 0) cx23885_dvb_register() dvb_register failed err = -22 cx23885_dev_setup() Failed to register dvb adapters on VID_B Signed-off-by: Matthias Schwarzott --- drivers/media/dvb-frontends/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/dvb-frontends/Kconfig b/drivers/media/dvb-frontends/Kconfig index 292c947..310e4b8 100644 --- a/drivers/media/dvb-frontends/Kconfig +++ b/drivers/media/dvb-frontends/Kconfig @@ -264,7 +264,7 @@ config DVB_MB86A16 config DVB_TDA10071 tristate "NXP TDA10071" depends on DVB_CORE && I2C - select REGMAP + select REGMAP_I2C default m if !MEDIA_SUBDRV_AUTOSELECT help Say Y when you want to support this frontend. -- 2.6.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 06/18] [media] media.h: create connector entities for hybrid TV devices
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 14:30:49 Mauro Carvalho Chehab wrote:
> Add entities to represent the connectors that exists inside a
> hybrid TV device.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index b17f6763aff4..69433405aec2 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -61,6 +61,7 @@ struct media_device_info {
> #define MEDIA_ENT_T_DVB_BASE 0x
> #define MEDIA_ENT_T_V4L2_BASE0x0001
> #define MEDIA_ENT_T_V4L2_SUBDEV_BASE 0x0002
> +#define MEDIA_ENT_T_CONNECTOR_BASE 0x0003
>
> /*
> * V4L2 entities - Those are used for DMA (mmap/DMABUF) and
> @@ -105,6 +106,13 @@ struct media_device_info {
> #define MEDIA_ENT_T_DVB_CA (MEDIA_ENT_T_DVB_BASE + 4)
> #define MEDIA_ENT_T_DVB_NET_DECAP(MEDIA_ENT_T_DVB_BASE + 5)
>
> +/* Connectors */
> +#define MEDIA_ENT_T_CONN_RF (MEDIA_ENT_T_CONNECTOR_BASE)
> +#define MEDIA_ENT_T_CONN_SVIDEO (MEDIA_ENT_T_CONNECTOR_BASE + 1)
> +#define MEDIA_ENT_T_CONN_COMPOSITE (MEDIA_ENT_T_CONNECTOR_BASE + 2)
> + /* For internal test signal generators and other debug connectors */
No need to a \t at the beginning of the line.
> +#define MEDIA_ENT_T_CONN_TEST(MEDIA_ENT_T_CONNECTOR_BASE + 3)
I'd like to see more information about this.
When later renaming types to functions you rename this type as well, and I'm
still not convinced that we shouldn't have both types and functions.
Let's discuss these topics and the one below on the documentation patches.
> #ifndef __KERNEL__
> /* Legacy symbols used to avoid userspace compilation breakages */
> #define MEDIA_ENT_TYPE_SHIFT 16
> @@ -121,9 +129,9 @@ struct media_device_info {
> #define MEDIA_ENT_T_DEVNODE_DVB (MEDIA_ENT_T_DEVNODE + 4)
> #endif
>
> -/* Entity types */
> -
> +/* Entity flags */
> #define MEDIA_ENT_FL_DEFAULT (1 << 0)
> +#define MEDIA_ENT_FL_CONNECTOR (1 << 1)
Ditto, I'm not sure about the use cases.
> struct media_entity_desc {
> __u32 id;
--
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 12/18] [media] media-entity: must check media_create_pad_link()
Hi Mauro, Thank you for the patch. On Sunday 06 September 2015 14:30:55 Mauro Carvalho Chehab wrote: > Drivers should check if media_create_pad_link() actually > worked. > > Signed-off-by: Mauro Carvalho Chehab > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index 62f882d872b1..8bdc10dcc5e7 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -348,8 +348,9 @@ int media_entity_init(struct media_entity *entity, u16 > num_pads, struct media_pad *pads); > void media_entity_cleanup(struct media_entity *entity); > > -int media_create_pad_link(struct media_entity *source, u16 source_pad, > - struct media_entity *sink, u16 sink_pad, u32 flags); > +__must_check int media_create_pad_link(struct media_entity *source, > + u16 source_pad, struct media_entity *sink, s/,\t/, / > + u16 sink_pad, u32 flags); And it would make sense to squash this with the patch that introduces media_create_pad_link(). > void __media_entity_remove_links(struct media_entity *entity); > void media_entity_remove_links(struct media_entity *entity); -- 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 13/18] [media] media-entity.h: rename entity.type to entity.function
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 14:30:56 Mauro Carvalho Chehab wrote:
> Entities should have one or more functions. Calling it as a
> type proofed to not be correct, as an entity could eventually
s/proofed/proved/
> have more than one type.
>
> So, rename the field as function.
s/as/to/
> Please notice that this patch doesn't extend support for
> multiple function entities. Such change will happen when
> we have real case drivers using it.
>
> No functional changes.
>
> Signed-off-by: Mauro Carvalho Chehab
[snip]
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 8bdc10dcc5e7..10f7d5f0eb66 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -152,7 +152,8 @@ struct media_entity_operations {
> *
> * @graph_obj: Embedded structure containing the media object common
> data.
> * @name:Entity name.
> - * @type:Entity type, as defined at uapi/media.h (MEDIA_ENT_T_*)
> + * @function:Entity main function, as defined at uapi/media.h
> + * (MEDIA_ENT_F_*)
I would squash this patch with "uapi/media.h: Rename entities types to
functions" as they essentially touch the same lines. If you want to keep them
separate I would use MEDIA_ENT_T_* here and rename it to MEDIA_ENT_F_* in
"uapi/media.h: Rename entities types to functions".
> * @revision:Entity revision - OBSOLETE - should be removed soon.
> * @flags: Entity flags, as defined at uapi/media.h (MEDIA_ENT_FL_*)
> * @group_id:Entity group ID - OBSOLETE - should be removed soon.
[snip]
--
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 14/18] [media] media-device: export the entity function via new ioctl
Hi Mauro,
Thank you for the patch.
On Sunday 06 September 2015 14:30:57 Mauro Carvalho Chehab wrote:
> Now that entities have a main function, expose it via
> MEDIA_IOC_G_TOPOLOGY ioctl.
>
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index ccef9621d147..32090030c342 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -263,6 +263,7 @@ static long __media_device_get_topology(struct
> media_device *mdev, /* Copy fields to userspace struct if not error */
> memset(&uentity, 0, sizeof(uentity));
> uentity.id = entity->graph_obj.id;
> + uentity.function = entity->function;
> strncpy(uentity.name, entity->name,
> sizeof(uentity.name));
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 69433405aec2..d232cc680c67 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -284,7 +284,8 @@ struct media_links_enum {
> struct media_v2_entity {
> __u32 id;
> char name[64]; /* FIXME: move to a property? (RFC says so) */
> - __u16 reserved[14];
> + __u32 function; /* Main function of the entity */
Shouldn't we use kerneldoc instead of inline comments ?
Also, as this is the main function only, I'd mention that in the subject line.
The implementation itself looks fine to me, I'll discuss the API over the
documentation patch.
> + __u16 reserved[12];
> };
>
> /* Should match the specific fields at media_intf_devnode */
--
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 1/2] [media] media_entity: remove gfp_flags argument
Hi Mauro,
Thank you for the patch.
On Wednesday 09 September 2015 08:32:02 Mauro Carvalho Chehab wrote:
> We should not be creating device nodes at IRQ contexts. So,
> the only flags we'll be using will be GFP_KERNEL. Let's
> remove the gfp_flags, in order to make the interface simpler.
>
> If we ever need it, it would be easy to revert those changes.
>
> While here, remove an extra blank line.
>
> Suggested-by: Sakari Ailus
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/dvb-core/dvbdev.c
> b/drivers/media/dvb-core/dvbdev.c index e9f24c1479dd..60828a9537a0 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -379,8 +379,7 @@ static int dvb_register_media_device(struct dvb_device
> *dvbdev,
>
> dvbdev->intf_devnode = media_devnode_create(dvbdev->adapter->mdev,
> intf_type, 0,
> - DVB_MAJOR, minor,
> - GFP_KERNEL);
> + DVB_MAJOR, minor);
>
> if (!dvbdev->intf_devnode)
> return -ENOMEM;
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 4868b8269204..f28265864b76 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -887,12 +887,11 @@ static void media_interface_init(struct media_device
> *mdev,
>
> struct media_intf_devnode *media_devnode_create(struct media_device *mdev,
> u32 type, u32 flags,
> - u32 major, u32 minor,
> - gfp_t gfp_flags)
> + u32 major, u32 minor)
> {
> struct media_intf_devnode *devnode;
>
> - devnode = kzalloc(sizeof(*devnode), gfp_flags);
> + devnode = kzalloc(sizeof(*devnode), GFP_KERNEL);
> if (!devnode)
> return NULL;
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> b/drivers/media/v4l2-core/v4l2-dev.c index 430aa2330d07..982255d2063f
> 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -777,8 +777,7 @@ static int video_register_media_controller(struct
> video_device *vdev, int type) vdev->intf_devnode =
> media_devnode_create(vdev->v4l2_dev->mdev,
> intf_type,
> 0, VIDEO_MAJOR,
> - vdev->minor,
> - GFP_KERNEL);
> + vdev->minor);
> if (!vdev->intf_devnode) {
> media_device_unregister_entity(&vdev->entity);
> return -ENOMEM;
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 9cbb10079024..44ab153c37fc 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -71,7 +71,6 @@ struct media_gobj {
> struct list_headlist;
> };
>
> -
This belongs to "media: add a common struct to be embed on media graph
objects". I know that you're reluctant to rebase, split and squash patches
from fear of introducing breakages, but white space fixes should be really
harmless :-)
The rest of the patch looks good to me, but as mentioned over IRC, I still
think it would be beneficial to do a final round of rebase, split and squash
as otherwise it's pretty difficult to perform a final review and ensure
everything is right. This patch looks like a pretty safe candidate in that
regard.
> struct media_pipeline {
> };
>
> @@ -373,8 +372,7 @@ void media_entity_pipeline_stop(struct media_entity
> *entity); struct media_intf_devnode *
> __must_check media_devnode_create(struct media_device *mdev,
> u32 type, u32 flags,
> - u32 major, u32 minor,
> - gfp_t gfp_flags);
> + u32 major, u32 minor);
> void media_devnode_remove(struct media_intf_devnode *devnode);
> struct media_link *
> __must_check media_create_intf_link(struct media_entity *entity,
--
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 v8 17/55] [media] omap3isp: separate links creation from entities init
Hello Laurent,
On 11/23/2015 12:55 PM, Laurent Pinchart wrote:
> Hi Javier and Mauro,
>
> Thank you for the patch.
>
Thanks for your feedback.
> On Monday 12 October 2015 13:43:05 Mauro Carvalho Chehab wrote:
>> From: Javier Martinez Canillas
>>
>> The omap3isp driver initializes the entities and creates the pads links
>> before the entities are registered with the media device. This does not
>> work now that object IDs are used to create links so the media_device
>> has to be set.
>>
>> Split out the pads links creation from the entity initialization so are
>> made after the entities registration.
>
> Is a part of this sentence missing ?
>
The sentence is not clear indeed, I think it should be something like:
"so the links are created after the entities have been registered with
the media device"
>> Suggested-by: Mauro Carvalho Chehab
>> Signed-off-by: Javier Martinez Canillas
>>
>> Series-to: linux-ker...@vger.kernel.org
>> Series-cc: Mauro Carvalho Chehab
>> Series-cc: linux-media@vger.kernel.org
>> Series-cc: Shuah Khan
>> Series-cc: Laurent Pinchart
>>
>> Cover-letter:
>
> I don't think those are known tags. Could you rework the commit message to
> merge both part into something coherent without copying the cover letter ?
>
Yes, sorry about that. I use patman to manage and post my patches and
forgot to remove the patman tags before handling the patches to Mauro.
I mentioned to him already and he told me that would strip the tags
before pushing to the media tree or posting the patches again.
[remove left over cover letter from a different series]
>>
>> Change-Id: I44abb9b67d6378cbd54ba4e0673a5d6d5721fc77
>
> No gerrit craziness please.
>
I believe this is something similar, a left over from Mauro since he
uses gerrit to manage this patch series.
>> ---
>> drivers/media/platform/omap3isp/isp.c| 152 ++--
>> drivers/media/platform/omap3isp/ispccdc.c| 22 ++--
>> drivers/media/platform/omap3isp/ispccdc.h| 1 +
>> drivers/media/platform/omap3isp/ispccp2.c| 22 ++--
>> drivers/media/platform/omap3isp/ispccp2.h| 1 +
>> drivers/media/platform/omap3isp/ispcsi2.c| 22 ++--
>> drivers/media/platform/omap3isp/ispcsi2.h| 1 +
>> drivers/media/platform/omap3isp/isppreview.c | 33 +++---
>> drivers/media/platform/omap3isp/isppreview.h | 1 +
>> drivers/media/platform/omap3isp/ispresizer.c | 33 +++---
>> drivers/media/platform/omap3isp/ispresizer.h | 1 +
>> 11 files changed, 185 insertions(+), 104 deletions(-)
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c
>> b/drivers/media/platform/omap3isp/isp.c index aa13b17d19a0..b8f6f81d2db2
>> 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1933,6 +1933,100 @@ done:
>> return ret;
>> }
>>
>> +/*
>> + * isp_create_pads_links - Pads links creation for the subdevices
>> + * @isp : Pointer to ISP device
>
> Missing blank line here. And missing description of the function for that
> matter. You can use
>
> "This function creates all links between ISP internal and external entities."
>
>> + * return negative error code or zero on success
>
> In kerneldoc style that should be
>
> Return: A negative error code on failure or zero on success. Possible error
> codes are those returned by media_create_pad_link().
>
> Same for the other functions below, the return line should start with
> "Return:".
>
You are right, the kerneldoc is not correct. I'll fix it.
>> + */
>> +static int isp_create_pads_links(struct isp_device *isp)
>
> This should be called isp_create_pad_links() if you want the include the pad
> prefix, but I'd just name it isp_create_links() as the driver doesn't handle
> any other kind of links. Same for all the *_create_pads_links() functions
> below.
>
Ok.
>> +{
>> +int ret;
>> +
>> +ret = omap3isp_csi2_create_pads_links(isp);
>> +if (ret < 0) {
>> +dev_err(isp->dev, "CSI2 pads links creation failed\n");
>
> That's lots of error strings. You would save memory by turning the messages
> into "%s pads links creation failed\n", "CSI2" as the compiler will then
> avoid
> multiple copies of the first string.
>
> I would actually remove the messages as the only source of error is a memory
> allocation failure, which will already print a message. You could add a
> single
> dev_err() in the location where isp_create_pads_links() is called if you want
> to.
>
Agreed, I'll just remove the messages.
>> +return ret;
>> +}
>> +
>> +ret = omap3isp_ccp2_create_pads_links(isp);
>> +if (ret < 0) {
>> +dev_err(isp->dev, "CCP2 pads links creation failed\n");
>> +return ret;
>> +}
>> +
>> +ret = omap3isp_ccdc_create_pads_links(isp);
>> +if (ret < 0) {
>> +dev_err(isp->dev, "CCDC pads links creation failed\n");
>> +return ret;
>> +}
>> +
>> +ret = omap3isp_preview_create_pads_links(isp
Re: [PATCH 2/2] [media] media-device: use unsigned ints on some places
Hi Mauro,
Thank you for the patch.
On Wednesday 09 September 2015 08:32:03 Mauro Carvalho Chehab wrote:
> The entity->num_pads are defined as u16. So, better to use an
> unsigned int, as this prevents additional warnings when W=2
> (or W=1 on some architectures).
>
> The "i" counter at __media_device_get_topology() is also a
> monotonic counter that should never be below zero. So,
> make it unsigned too.
>
> Suggested-by: Sakari Ailus
> Signed-off-by: Mauro Carvalho Chehab
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 13987710e5bc..1312e93ebd6e 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -243,7 +243,8 @@ static long __media_device_get_topology(struct
> media_device *mdev, struct media_v2_interface uintf;
> struct media_v2_pad upad;
> struct media_v2_link ulink;
> - int ret = 0, i;
> + int ret = 0;
> + unsigned int i;
Very small nitpicking, I'd put the unsigned int i line before the int ret line
to match the coding style of the rest of the file (with shorter variable
declaration lines at the bottom).
>
> topo->topology_version = mdev->topology_version;
>
> @@ -613,7 +614,7 @@ EXPORT_SYMBOL_GPL(media_device_unregister);
> int __must_check media_device_register_entity(struct media_device *mdev,
> struct media_entity *entity)
> {
> - int i;
> + unsigned int i;
>
> if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
> entity->function == MEDIA_ENT_F_UNKNOWN)
> @@ -650,9 +651,9 @@ EXPORT_SYMBOL_GPL(media_device_register_entity);
> */
> void media_device_unregister_entity(struct media_entity *entity)
> {
> - int i;
> struct media_device *mdev = entity->graph_obj.mdev;
> struct media_link *link, *tmp;
> + unsigned int i;
Like you've done here, that's very good :-)
>
> if (mdev == NULL)
> return;
With that fixed,
Reviewed-by: Laurent Pinchart
--
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: [media-workshop] [PATCH v8.4 81/83] [media] media-entity: init pads on entity init if was registered before
Hi Javier,
(Replying to linux-media instead of media-workshop, I can't find this patch in
my linux-media folder)
Thank you for the patch.
On Monday 12 October 2015 13:44:10 Mauro Carvalho Chehab wrote:
> From: Javier Martinez Canillas
>
> If an entity is registered with a media device before is initialized
> with media_device_register_entity(), the number of pads won't be set
> so media_device_register_entity() won't create pad objects and add
> it to the media device pads list.
>
> Do this at entity initialization time if the entity was registered
> before so the graph is complete and correct regardless of the order
> in which the entities are initialized and registered.
We now have two places where the pads graph objects are initialized and that
looks like a bug to me as media_gobj_init() allocates IDs and, even worse,
adds the entity to the media device entities list.
We need to standardize on where graph objects are initialized across the
different kind of objects and document this clearly otherwise drivers will get
it wrong.
> Suggested-by: Mauro Carvalho Chehab
> Signed-off-by: Javier Martinez Canillas
> ---
> drivers/media/media-entity.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index f28265864b76..2c984fb7d497 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -238,6 +238,7 @@ int
> media_entity_init(struct media_entity *entity, u16 num_pads,
> struct media_pad *pads)
> {
> + struct media_device *mdev = entity->graph_obj.mdev;
> unsigned int i;
>
> entity->group_id = 0;
> @@ -246,11 +247,20 @@ media_entity_init(struct media_entity *entity, u16
> num_pads, entity->num_pads = num_pads;
> entity->pads = pads;
>
> + if (mdev)
> + spin_lock(&mdev->lock);
> +
> for (i = 0; i < num_pads; i++) {
> pads[i].entity = entity;
> pads[i].index = i;
> + if (mdev)
> + media_gobj_init(mdev, MEDIA_GRAPH_PAD,
> + &entity->pads[i].graph_obj);
> }
>
> + if (mdev)
> + spin_unlock(&mdev->lock);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(media_entity_init);
--
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 v8 17/55] [media] omap3isp: separate links creation from entities init
Hi Javier and Mauro,
Thank you for the patch.
On Monday 12 October 2015 13:43:05 Mauro Carvalho Chehab wrote:
> From: Javier Martinez Canillas
>
> The omap3isp driver initializes the entities and creates the pads links
> before the entities are registered with the media device. This does not
> work now that object IDs are used to create links so the media_device
> has to be set.
>
> Split out the pads links creation from the entity initialization so are
> made after the entities registration.
Is a part of this sentence missing ?
> Suggested-by: Mauro Carvalho Chehab
> Signed-off-by: Javier Martinez Canillas
>
> Series-to: linux-ker...@vger.kernel.org
> Series-cc: Mauro Carvalho Chehab
> Series-cc: linux-media@vger.kernel.org
> Series-cc: Shuah Khan
> Series-cc: Laurent Pinchart
>
> Cover-letter:
I don't think those are known tags. Could you rework the commit message to
merge both part into something coherent without copying the cover letter ?
> Patches to test MC next gen patches in OMAP3 ISP
> Hello,
>
> This series contains two patches that are needed to test the
> "[PATCH v7 00/44] MC next generation patches" [0] in a OMAP3
> board by using the omap3isp driver.
>
> I found two issues during testing, the first one is that the
> media_entity_cleanup() function tries to empty the pad links
> list but the list is initialized when a entity is registered
> causing a NULL pointer deference error.
>
> The second issue is that the omap3isp driver creates links
> when the entities are initialized but before the media device
> is registered causing a NULL pointer deference as well.
>
> Patch 1/1 fixes the first issue by removing the links list
> empty logic from media_entity_cleanup() since that is made
> in media_device_unregister_entity() and 2/2 fixes the second
> issue by separating the entities initialization from the pads
> links creation after the entities have been registered.
>
> Patch 1/1 was posted before [1] but forgot to add the [media]
> prefix in the subject line so I'm including in this set again.
> Sorry about that.
>
> The testing was made on an OMAP3 DM3735 IGEPv2 board and test
> that the media-ctl -p prints out the topology. More extensive
> testing will be made but I wanted to share these patches in
> order to make easier for other people that were looking at it.
>
> [0]: https://www.mail-archive.com/linux-media@vger.kernel.org/msg91528.html
> [1]: https://lkml.org/lkml/2015/8/24/649
>
> Best regards,
> Javier
> END
>
> Change-Id: I44abb9b67d6378cbd54ba4e0673a5d6d5721fc77
No gerrit craziness please.
> ---
> drivers/media/platform/omap3isp/isp.c| 152 ++--
> drivers/media/platform/omap3isp/ispccdc.c| 22 ++--
> drivers/media/platform/omap3isp/ispccdc.h| 1 +
> drivers/media/platform/omap3isp/ispccp2.c| 22 ++--
> drivers/media/platform/omap3isp/ispccp2.h| 1 +
> drivers/media/platform/omap3isp/ispcsi2.c| 22 ++--
> drivers/media/platform/omap3isp/ispcsi2.h| 1 +
> drivers/media/platform/omap3isp/isppreview.c | 33 +++---
> drivers/media/platform/omap3isp/isppreview.h | 1 +
> drivers/media/platform/omap3isp/ispresizer.c | 33 +++---
> drivers/media/platform/omap3isp/ispresizer.h | 1 +
> 11 files changed, 185 insertions(+), 104 deletions(-)
>
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index aa13b17d19a0..b8f6f81d2db2
> 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -1933,6 +1933,100 @@ done:
> return ret;
> }
>
> +/*
> + * isp_create_pads_links - Pads links creation for the subdevices
> + * @isp : Pointer to ISP device
Missing blank line here. And missing description of the function for that
matter. You can use
"This function creates all links between ISP internal and external entities."
> + * return negative error code or zero on success
In kerneldoc style that should be
Return: A negative error code on failure or zero on success. Possible error
codes are those returned by media_create_pad_link().
Same for the other functions below, the return line should start with
"Return:".
> + */
> +static int isp_create_pads_links(struct isp_device *isp)
This should be called isp_create_pad_links() if you want the include the pad
prefix, but I'd just name it isp_create_links() as the driver doesn't handle
any other kind of links. Same for all the *_create_pads_links() functions
below.
> +{
> + int ret;
> +
> + ret = omap3isp_csi2_create_pads_links(isp);
> + if (ret < 0) {
> + dev_err(isp->dev, "CSI2 pads links creation failed\n");
That's lots of error strings. You would save memory by turning the messages
into "%s pads links creation failed\n", "CSI2" as the compiler will then avoid
multiple copies of the first string.
I would actually remove the messages as the only source of error is a memory
allocation failure, which will already print a message. Y
Re: [PATCH 1/2] [media] media: don't try to empty links list in media_entity_cleanup()
Hello Javier,
(Resending as I've replied by mistake to the version of the patch you had sent
to the media workshop list only)
Thank you for the patch.
On Monday 12 October 2015 13:44:11 Mauro Carvalho Chehab wrote:
> From: Javier Martinez Canillas
>
> The media_entity_cleanup() function only cleans up the entity links list
> but this operation is already made in media_device_unregister_entity().
>
> In most cases this should be harmless (besides having duplicated code)
> since the links list would be empty so the iteration would not happen
> but the links list is initialized in media_device_register_entity() so
> if a driver fails to register an entity with a media device and clean up
> the entity in the error path, a NULL deference pointer error will happen.
>
> So don't try to empty the links list in media_entity_cleanup() since
> is either done already or haven't been initialized yet.
Does this mean that it's an invalid usage of the API to create links before
registering entities ? If so it should be clearly documented somewhere, such
as in the kerneldoc of the media_create_pad_link() function.
And yes, that means that all exported API functions need kerneldoc. Sorry for
being a killjoy
On a related note, we need to solve the userspace API race caused by
registering the MC devnode before all entities and links are created an
registered. It's not a new issue so I won't call for fixing it as part of this
patch series, but we'll need to fix that with the dynamic graph update
implementation at the latest. It will likely require reworking the
initialization and registration sequences.
> Signed-off-by: Javier Martinez Canillas
> ---
> drivers/media/media-entity.c | 7 ---
> 1 file changed, 7 deletions(-)
>
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 2c984fb7d497..eaeda2589ce5 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -268,13 +268,6 @@ EXPORT_SYMBOL_GPL(media_entity_init);
> void
> media_entity_cleanup(struct media_entity *entity)
> {
> - struct media_link *link, *tmp;
> -
> - list_for_each_entry_safe(link, tmp, &entity->links, list) {
> - media_gobj_remove(&link->graph_obj);
> - list_del(&link->list);
> - kfree(link);
> - }
> }
> EXPORT_SYMBOL_GPL(media_entity_cleanup);
As media_entity_cleanup is now empty I'd turn it into a static inline. We need
to keep the function in case cleanup ends up being needed later, but there's
no reason not to optimize the call away for now.
--
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 v8.2 19/55] [media] media: convert links from array to list
Em Mon, 23 Nov 2015 17:37:54 +0200
Laurent Pinchart escreveu:
> Hi Mauro,
>
> (Resending as I've replied by mistake to the version of the patch you had
> sent
> to the media workshop list only)
(resending my answer to your previously sent review to the WS only ML.
The e-mail contents is the same as the previously sent one)
>
> Thank you for the patch.
>
Hi Laurent,
I'll be addressing the points below on separate patches, to avoid rebasing
it and causing the need for we all (me, Shuah, Javier, Sakari) to re-test
everything after patch 24 again. This way, if a regression happens, we know
what change to blame ;)
Regards,
Mauro
Em Mon, 23 Nov 2015 15:30:36 +0200
Laurent Pinchart escreveu:
> Hi Mauro,
>
> Thank you for the patch.
>
> On Monday 12 October 2015 13:43:13 Mauro Carvalho Chehab wrote:
> > The entire logic that represent graph links were developed on a
> > time where there were no needs to dynamic remove links. So,
> > although links are created/removed one by one via some
> > functions, they're stored as an array inside the entity struct.
> >
> > As the array may grow, there's a logic inside the code that
> > checks if the amount of space is not enough to store
> > the needed links. If it isn't the core uses krealloc()
> > to change the size of the link, with is bad, as it
> > leaves the memory fragmented.
>
> I agree with the change made in this patch, but I'm not sure if fragmentation
> is really the issue. I wouldn't be surprised if we ended up with more
> fragmented memory.
That would actually depend on how things get allocated/deallocated.
If we discover that fragmentation is actually increasing, we could
change the code later to use a lookaside cache.
>
> > So, convert links into a list.
> >
> > Also, currently, both source and sink entities need the link
> > at the graph traversal logic inside media_entity. So there's
> > a logic duplicating all links. That makes it to spend
> > twice the memory needed. This is not a big deal for today's
> > usage, where the number of links are not big.
> >
> > Yet, if during the MC workshop discussions, it was said that
> > IIO graphs could have up to 4,000 entities. So, we may
> > want to remove the duplication on some future. The problem
> > is that it would require a separate linked list to store
> > the backlinks inside the entity, or to use a more complex
> > algorithm to do graph backlink traversal, with is something
> > that the current graph traversal inside the core can't cope
> > with. So, let's postpone a such change if/when it is actually
> > needed.
>
> The media_link structure uses 44 bytes on 32-bit architectures and 84 bytes
> on
> 64-bit architecture. It will thus be allocated out of the 64-bytes and 96-
> bytes pools respectively. That's a 12.5% memory waste on 64-bit architectures
> and 31.25% on 32-bit architecture. If you're concerned about memory usage
> (and
> I think we all should) a linked list is less efficient than an array in this
> case (and even more so if you take the struct list_head into account).
I doubt that the amount of memory spent at the media controller
would actually cause impact, as the size of those structs are a
way smaller than the size of video buffers. Anyway, if we found
later that this would cause troubles, we can redesign it.
>
> > Change-Id: I558e8f87f200fe5f83ddaafe5560f91f0d906b63
>
> No need to infect mainline with gerrit nonsense :-)
I'm not using gerrit ;) I'm just adding this crap because the change ID
is a good way to detect if a patch is new or not. I have some scripts
that use those IDs to detect it, when working with this 80+ patch series.
In any case, the scripts I use to pull patches at the main tree will
remove those stuff.
>
> > Signed-off-by: Mauro Carvalho Chehab
> > ---
> > drivers/media/dvb-core/dvb_frontend.c | 9 +--
> > drivers/media/media-device.c | 25 +++---
> > drivers/media/media-entity.c | 128 ---
> > drivers/media/usb/au0828/au0828-core.c| 12 ++-
> > drivers/media/usb/au0828/au0828-video.c | 8 +-
> > drivers/media/usb/cx231xx/cx231xx-video.c | 8 +-
> > include/media/media-entity.h | 10 +--
> > 7 files changed, 97 insertions(+), 103 deletions(-)
>
> [snip]
>
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 0d85c6c28004..3e649cacfc07 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -25,6 +25,7 @@
> > #include
> > #include
> > #include
> > +#include
>
> Could you please keep headers sorted alphabetically ?
Ok, I'll reorder on a later patch.
>
> > #include
> > #include
>
> [snip]
>
> > @@ -150,22 +151,21 @@ static long __media_device_enum_links(struct
> > media_device *mdev, }
> >
> > if (links->links) {
> > - struct media_link_desc __user *ulink;
> > - unsigned int l;
> > + struct media_
Re: [PATCH v8.2 19/55] [media] media: convert links from array to list
Hi Mauro,
(Resending as I've replied by mistake to the version of the patch you had sent
to the media workshop list only)
Thank you for the patch.
On Monday 12 October 2015 13:43:13 Mauro Carvalho Chehab wrote:
> The entire logic that represent graph links were developed on a
> time where there were no needs to dynamic remove links. So,
> although links are created/removed one by one via some
> functions, they're stored as an array inside the entity struct.
>
> As the array may grow, there's a logic inside the code that
> checks if the amount of space is not enough to store
> the needed links. If it isn't the core uses krealloc()
> to change the size of the link, with is bad, as it
> leaves the memory fragmented.
I agree with the change made in this patch, but I'm not sure if fragmentation
is really the issue. I wouldn't be surprised if we ended up with more
fragmented memory.
> So, convert links into a list.
>
> Also, currently, both source and sink entities need the link
> at the graph traversal logic inside media_entity. So there's
> a logic duplicating all links. That makes it to spend
> twice the memory needed. This is not a big deal for today's
> usage, where the number of links are not big.
>
> Yet, if during the MC workshop discussions, it was said that
> IIO graphs could have up to 4,000 entities. So, we may
> want to remove the duplication on some future. The problem
> is that it would require a separate linked list to store
> the backlinks inside the entity, or to use a more complex
> algorithm to do graph backlink traversal, with is something
> that the current graph traversal inside the core can't cope
> with. So, let's postpone a such change if/when it is actually
> needed.
The media_link structure uses 44 bytes on 32-bit architectures and 84 bytes on
64-bit architecture. It will thus be allocated out of the 64-bytes and 96-
bytes pools respectively. That's a 12.5% memory waste on 64-bit architectures
and 31.25% on 32-bit architecture. If you're concerned about memory usage (and
I think we all should) a linked list is less efficient than an array in this
case (and even more so if you take the struct list_head into account).
> Change-Id: I558e8f87f200fe5f83ddaafe5560f91f0d906b63
No need to infect mainline with gerrit nonsense
> Signed-off-by: Mauro Carvalho Chehab
> ---
> drivers/media/dvb-core/dvb_frontend.c | 9 +--
> drivers/media/media-device.c | 25 +++---
> drivers/media/media-entity.c | 128 ---
> drivers/media/usb/au0828/au0828-core.c| 12 ++-
> drivers/media/usb/au0828/au0828-video.c | 8 +-
> drivers/media/usb/cx231xx/cx231xx-video.c | 8 +-
> include/media/media-entity.h | 10 +--
> 7 files changed, 97 insertions(+), 103 deletions(-)
[snip]
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 0d85c6c28004..3e649cacfc07 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -25,6 +25,7 @@
> #include
> #include
> #include
> +#include
Could you please keep headers sorted alphabetically ?
> #include
> #include
[snip]
> @@ -150,22 +151,21 @@ static long __media_device_enum_links(struct
> media_device *mdev, }
>
> if (links->links) {
> - struct media_link_desc __user *ulink;
> - unsigned int l;
> + struct media_link *ent_link;
> + struct media_link_desc __user *ulink = links->links;
Might be slightly nitpicking, but I think variables would be more coherent if
they were called
struct media_link_desc __user *ulink = links->links;
struct media_link *link;
...
>
> - for (l = 0, ulink = links->links; l < entity->num_links; l++)
{
> + list_for_each_entry(ent_link, &entity->links, list) {
> struct media_link_desc link;
and
struct media_link_desc klink;
here.
> /* Ignore backlinks. */
> - if (entity->links[l].source->entity != entity)
> + if (ent_link->source->entity != entity)
> continue;
> -
> memset(&link, 0, sizeof(link));
> - media_device_kpad_to_upad(entity->links[l].source,
> + media_device_kpad_to_upad(ent_link->source,
> &link.source);
> - media_device_kpad_to_upad(entity->links[l].sink,
> + media_device_kpad_to_upad(ent_link->sink,
> &link.sink);
> - link.flags = entity->links[l].flags;
> + link.flags = ent_link->flags;
> if (copy_to_user(ulink, &link, sizeof(*ulink)))
> return -EFAULT;
> ulink++;
> @@ -437,6 +437,7 @@ int __must_check media_devi
[PATCH] [media] rc-core: Try loading modules if the kernel supports them
Always try to load modules for RC keymaps when the kernel supports
modules, not just when the RC Core is build as a module. Fixes
module autoloading issues when the core is builtin but the keymaps
are not.
Signed-off-by: Sjoerd Simons
---
drivers/media/rc/rc-main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 3f0f71a..ea1008c 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -61,7 +61,7 @@ struct rc_map *rc_map_get(const char *name)
struct rc_map_list *map;
map = seek_rc_map(name);
-#ifdef MODULE
+#ifdef CONFIG_MODULES
if (!map) {
int rc = request_module("%s", name);
if (rc < 0) {
--
2.6.2
--
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: DVBSky T330 DVB-C regression Linux 4.1.12 to 4.3
Moikka!
On 11/19/2015 01:36 AM, Stephan Eisvogel wrote:
Hey Olli, Antti,
culprit is:
http://git.linuxtv.org/cgit.cgi/linux.git/commit/drivers/media/dvb-frontends/si2168.c?id=7adf99d20ce0e96a70755f452e3a63824b14060f
I removed it like this:
/* error bit set? */
/*
if ((cmd->args[0] >> 6) & 0x01) {
ret = -EREMOTEIO;
goto err;
}
*/
With this change backed out I zapped through about a 100 channels, and
my DVB stick works
again. Of course demodulator error handling would be nice anyhow. Beyond
my time budget
for now.
Surprising finding. Init succeeded already as firmware was downloaded so
that error likely happens during si2168_set_frontend(). As set frontend
is called once for each tuning request one failure should not cause more
harm than one tuning failure. It could be nice to see which function is
failing and if it fails repeatedly.
To see that, debug messages should be enabled:
modprobe si2168 dyndbg==pmftl
or
modprobe si2168; echo -n 'module si2168 =pft' >
/sys/kernel/debug/dynamic_debug/control
You could also replace all dev_dbg with dev_info if you don't care
compile kernel with dynamic debugs enabled needed for normal debug logging.
Also, you used 4.0.19 firmware. Could you test that old one:
http://palosaari.fi/linux/v4l-dvb/firmware/Si2168/Si2168-B40/4.0.11/
Unfortunately I don't have that device at all, so I cannot do much
myself. It is more up to Olli :]
regards
Antti
--
http://palosaari.fi/
--
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: Interrupt handler responsibility
On 11/21/2015 11:20 PM, Ran Shalit wrote: > Hello, > > I am trying to understand the interrupt handler responsibility in > v4l2, also with respect to dma usage. I see that it is not defined as > part of the videobuf2 API. > > This is what I understand this far: > 1. start_streaming is responsible for getting into "streaming" state. > dma start should be trigggered at this point. Right. > 2. interrupt handler: is responsible for passing back the buffer to > user using vb2_buffer_done() call. Right. > > But what is the exact reponsibility of interrupt handler with respect > to dma usage ? Typically when the DMA has finished DMAing a frame it will generate an interrupt. The interrupt handler will then call vb2_buffer_done() handing the buffer back to the vb2 framework. Think of it as who owns the buffer: when buf_queue is called the buffer is handed from vb2 to the driver, and the driver calls vb2_buffer_done() when it is finished with the buffer (i.e. the data is DMAed into the buffer) and it hands it back to vb2. > In some of the drivers I see that the interrupt start/stop dma, but in > v4l2-pci-skeleton.c I don't see any usage of dma in the interrupt > handler, so I'm not sure. The interrupt handler is called in response of a DMA interrupt. How that interrupt is generated is hardware specific, so that's why you don't see it in the skeleton driver. 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
