Em Wed, 6 Jan 2016 13:26:50 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

From: Mauro Carvalho Chehab mchehab@osg.samsung.com

Declare the interface types to be used on alsa for the new G_TOPOLOGY ioctl.

Signed-off-by: Mauro Carvalho Chehab mchehab@osg.samsung.com

drivers/media/media-entity.c | 12 ++++++++++++ include/uapi/linux/media.h | 8 ++++++++ 2 files changed, 20 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index eb38bc3..6e02d19 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -65,6 +65,18 @@ static inline const char *intf_type(struct media_interface *intf) return "v4l2-subdev"; case MEDIA_INTF_T_V4L_SWRADIO: return "swradio";

• case MEDIA_INTF_T_ALSA_PCM_CAPTURE:

• return "pcm-capture";
  

• case MEDIA_INTF_T_ALSA_PCM_PLAYBACK:

• return "pcm-playback";
  

• case MEDIA_INTF_T_ALSA_CONTROL:

• return "alsa-control";
  

• case MEDIA_INTF_T_ALSA_COMPRESS:

• return "compress";
  

• case MEDIA_INTF_T_ALSA_RAWMIDI:

• return "rawmidi";
  

• case MEDIA_INTF_T_ALSA_HWDEP:

• return "hwdep";
  

  default: return "unknown-intf"; }

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h index cacfceb..75cbe92 100644 --- a/include/uapi/linux/media.h +++ b/include/uapi/linux/media.h @@ -252,6 +252,7 @@ struct media_links_enum {

#define MEDIA_INTF_T_DVB_BASE 0x00000100 #define MEDIA_INTF_T_V4L_BASE 0x00000200 +#define MEDIA_INTF_T_ALSA_BASE 0x00000300

/* Interface types */

@@ -267,6 +268,13 @@ struct media_links_enum { #define MEDIA_INTF_T_V4L_SUBDEV (MEDIA_INTF_T_V4L_BASE + 3) #define MEDIA_INTF_T_V4L_SWRADIO (MEDIA_INTF_T_V4L_BASE + 4)

+#define MEDIA_INTF_T_ALSA_PCM_CAPTURE (MEDIA_INTF_T_ALSA_BASE) +#define MEDIA_INTF_T_ALSA_PCM_PLAYBACK (MEDIA_INTF_T_ALSA_BASE + 1) +#define MEDIA_INTF_T_ALSA_CONTROL (MEDIA_INTF_T_ALSA_BASE + 2) +#define MEDIA_INTF_T_ALSA_COMPRESS (MEDIA_INTF_T_ALSA_BASE + 3) +#define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_INTF_T_ALSA_BASE + 4) +#define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_INTF_T_ALSA_BASE + 5)

Patch looks ok, but please document the new media interfaces at KernelDoc documentation.

/*

• MC next gen API definitions

On Mon, 01 Feb 2016 17:07:51 +0100, Shuah Khan wrote:

On 01/28/2016 07:59 AM, Mauro Carvalho Chehab wrote:

...

Em Wed, 6 Jan 2016 13:26:50 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

From: Mauro Carvalho Chehab mchehab@osg.samsung.com

Declare the interface types to be used on alsa for the new G_TOPOLOGY ioctl.

Signed-off-by: Mauro Carvalho Chehab mchehab@osg.samsung.com

drivers/media/media-entity.c | 12 ++++++++++++ include/uapi/linux/media.h | 8 ++++++++ 2 files changed, 20 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index eb38bc3..6e02d19 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -65,6 +65,18 @@ static inline const char *intf_type(struct media_interface *intf) return "v4l2-subdev"; case MEDIA_INTF_T_V4L_SWRADIO: return "swradio";

• case MEDIA_INTF_T_ALSA_PCM_CAPTURE:

• return "pcm-capture";
  

• case MEDIA_INTF_T_ALSA_PCM_PLAYBACK:

• return "pcm-playback";
  

• case MEDIA_INTF_T_ALSA_CONTROL:

• return "alsa-control";
  

• case MEDIA_INTF_T_ALSA_COMPRESS:

• return "compress";
  

• case MEDIA_INTF_T_ALSA_RAWMIDI:

• return "rawmidi";
  

• case MEDIA_INTF_T_ALSA_HWDEP:

• return "hwdep";
  

  default: return "unknown-intf"; }

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h index cacfceb..75cbe92 100644 --- a/include/uapi/linux/media.h +++ b/include/uapi/linux/media.h @@ -252,6 +252,7 @@ struct media_links_enum {

#define MEDIA_INTF_T_DVB_BASE 0x00000100 #define MEDIA_INTF_T_V4L_BASE 0x00000200 +#define MEDIA_INTF_T_ALSA_BASE 0x00000300

/* Interface types */

@@ -267,6 +268,13 @@ struct media_links_enum { #define MEDIA_INTF_T_V4L_SUBDEV (MEDIA_INTF_T_V4L_BASE + 3) #define MEDIA_INTF_T_V4L_SWRADIO (MEDIA_INTF_T_V4L_BASE + 4)

+#define MEDIA_INTF_T_ALSA_PCM_CAPTURE (MEDIA_INTF_T_ALSA_BASE) +#define MEDIA_INTF_T_ALSA_PCM_PLAYBACK (MEDIA_INTF_T_ALSA_BASE + 1) +#define MEDIA_INTF_T_ALSA_CONTROL (MEDIA_INTF_T_ALSA_BASE + 2) +#define MEDIA_INTF_T_ALSA_COMPRESS (MEDIA_INTF_T_ALSA_BASE + 3) +#define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_INTF_T_ALSA_BASE + 4) +#define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_INTF_T_ALSA_BASE + 5)

Patch looks ok, but please document the new media interfaces at KernelDoc documentation.

Hi Takashi,

If you are okay with these changes, could you please Ack this patch. I am addressing documentation comment from Mauro.

Well, the available ALSA devices are:

CONTROL SEQUENCER TIMER COMPRESS HWDEP RAWMIDI PCM_PLAYBACK PCM_CAPTURE

as found in sound/minors.h.

Any reason not to define for some of them?

Takashi

On 01/28/2016 08:13 AM, Mauro Carvalho Chehab wrote:

Em Wed, 6 Jan 2016 13:26:52 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Add new interfaces to register and unregister entity_notify hook to media device to allow drivers to take appropriate actions when as new entities get added to the shared media device.When a new entity is registered, all registered entity_notify hooks are invoked to allow drivers or modules that registered hook to take appropriate action. For example, ALSA driver registers an entity_notify hook to parse the list of registered entities to determine if decoder has been linked to ALSA entity. au0828 bridge driver registers an entity_notify hook to create media graph for the device.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/media-device.c | 61 ++++++++++++++++++++++++++++++++++++++++++++ include/media/media-device.h | 25 ++++++++++++++++++ 2 files changed, 86 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index b786b10..20c85a9 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -536,6 +536,7 @@ static void media_device_release(struct media_devnode *mdev) int __must_check media_device_register_entity(struct media_device *mdev, struct media_entity *entity) {

• struct media_entity_notify *notify, *next; unsigned int i; int ret;

@@ -575,6 +576,11 @@ int __must_check media_device_register_entity(struct media_device *mdev, media_gobj_create(mdev, MEDIA_GRAPH_PAD, &entity->pads[i].graph_obj);

• /* invoke entity_notify callbacks */

• list_for_each_entry_safe(notify, next, &mdev->entity_notify, list) {

• (notify)->notify(entity, notify->notify_data);
  

• }

• spin_unlock(&mdev->lock);

  return 0;

@@ -608,6 +614,8 @@ static void __media_device_unregister_entity(struct media_entity *entity) /* Remove the entity */ media_gobj_destroy(&entity->graph_obj);

• /* invoke entity_notify callbacks to handle entity removal?? */
• entity->graph_obj.mdev = NULL;

}

@@ -633,6 +641,7 @@ int __must_check media_device_init(struct media_device *mdev) INIT_LIST_HEAD(&mdev->interfaces); INIT_LIST_HEAD(&mdev->pads); INIT_LIST_HEAD(&mdev->links);

• INIT_LIST_HEAD(&mdev->entity_notify); spin_lock_init(&mdev->lock); mutex_init(&mdev->graph_mutex); ida_init(&mdev->entity_internal_idx);

@@ -680,11 +689,59 @@ int __must_check __media_device_register(struct media_device *mdev, } EXPORT_SYMBOL_GPL(__media_device_register);

+/**

• • media_device_register_entity_notify - Register a media entity notify
• • callback with a media device. When a new entity is registered, all
• • the registered media_entity_notify callbacks are invoked.
• • @mdev: The media device
• • @nptr: The media_entity_notify
• */

Please put the documentation only at the header files. Please mention that it locks the addition using the mdev->lock spinlock.

Also, please follow the format that we're using with other KernelDoc tags:

/**

• function() - short description
• @foo: foo description
• @bar: bar description
• detailed description

*/

In case of doubts, please check Documentation/kernel-doc-nano-HOWTO.txt.

Don't forget to check if the documentation looks sane by compiling it with: make cleandocs make DOCBOOKS=device-drivers.xml htmldocs

Same applies to other documentation below and on other patches in this series.

...

+int __must_check media_device_register_entity_notify(struct media_device *mdev,

• 			struct media_entity_notify *nptr)
  

+{

• spin_lock(&mdev->lock);
• list_add_tail(&nptr->list, &mdev->entity_notify);
• spin_unlock(&mdev->lock);
• return 0;

+} +EXPORT_SYMBOL_GPL(media_device_register_entity_notify);

+/**

• • __media_device_unregister_entity_notify - Unregister a media entity notify
• • callback with a media device. When a new entity is registered, all
• • the registered media_entity_notify callbacks are invoked.
• • @mdev: The media device
• • @nptr: The media_entity_notify
• • Non-locking version. Should be called with mdev->lock held.
• */

+static void __media_device_unregister_entity_notify(struct media_device *mdev,

• 			struct media_entity_notify *nptr)
  

__must_check

Thanks for the catch. Will fix it.

...

+{

• list_del(&nptr->list);

+}

+/**

• • media_device_unregister_entity_notify - Unregister a media entity notify
• • callback with a media device. When a new entity is registered, all
• • the registered media_entity_notify callbacks are invoked.
• • @mdev: The media device
• • @nptr: The media_entity_notify
• */

+void media_device_unregister_entity_notify(struct media_device *mdev,

• 			struct media_entity_notify *nptr)
  

+{

• spin_lock(&mdev->lock);
• __media_device_unregister_entity_notify(mdev, nptr);
• spin_unlock(&mdev->lock);

+} +EXPORT_SYMBOL_GPL(media_device_unregister_entity_notify);

void media_device_unregister(struct media_device *mdev) { struct media_entity *entity; struct media_entity *next; struct media_interface *intf, *tmp_intf;

• struct media_entity_notify *notify, *nextp;

  if (mdev == NULL) return;

@@ -701,6 +758,10 @@ void media_device_unregister(struct media_device *mdev) list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list) __media_device_unregister_entity(entity);

• /* Remove all entity_notify callbacks from the media device */
• list_for_each_entry_safe(notify, nextp, &mdev->entity_notify, list)

• __media_device_unregister_entity_notify(mdev, notify);
  

• /* Remove all interfaces from the media device */ list_for_each_entry_safe(intf, tmp_intf, &mdev->interfaces, graph_obj.list) {

diff --git a/include/media/media-device.h b/include/media/media-device.h index 122963a..6520d1c 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -264,6 +264,12 @@ struct ida; struct device;

+struct media_entity_notify {

• struct list_head list;
• void *notify_data;
• void (*notify)(struct media_entity *entity, void *notify_data);

+};

Please document the structure.

...

/**

• struct media_device - Media device
• @dev: Parent device

@@ -319,6 +325,9 @@ struct media_device { struct list_head pads; struct list_head links;

• /* notify callback list invoked when a new entity is registered */
• struct list_head entity_notify;

Please document the new field at the Kernel-doc header.

...

/* Protects the graph objects creation/removal */ spinlock_t lock; /* Serializes graph operations. */ @@ -497,6 +506,11 @@ int __must_check media_device_register_entity(struct media_device *mdev, */ void media_device_unregister_entity(struct media_entity *entity);

+int __must_check media_device_register_entity_notify(struct media_device *mdev,

• 			struct media_entity_notify *nptr);
  

+void media_device_unregister_entity_notify(struct media_device *mdev,

• 			struct media_entity_notify *nptr);
  

Documentation?

I would like to add documentation as an add-on to the series, as I explained in response to [PATCH 04/31]

Hope that is okay for this patch as well.

thanks, -- Shuah

...

/**

• media_device_get_devres() - get media device as device resource

• 		creates if one doesn't exist
  

@@ -552,6 +566,17 @@ static inline int media_device_register_entity(struct media_device *mdev, static inline void media_device_unregister_entity(struct media_entity *entity) { } +static inline int media_device_register_entity_notify(

• 			struct media_device *mdev,
  

• 			struct media_entity_notify *nptr)
  

+{

• return 0;

+} +static inline void media_device_unregister_entity_notify(

• 			struct media_device *mdev,
  

• 			struct media_entity_notify *nptr)
  

+{ +} static inline struct media_device *media_device_get_devres(struct device *dev) { return NULL;

Documentation?

Regards, Mauro

Em Wed, 6 Jan 2016 13:26:53 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Add new fields to struct media_device to add enable_source, and disable_source handlers, and source_priv to stash driver private data that is need to run these handlers. The enable_source handler finds source entity for the passed in entity and check if it is available, and activate the link using __media_entity_setup_link() interface. Bridge driver is expected to implement and set these handlers and private data when media_device is registered or when bridge driver finds the media_device during probe. This is to enable the use-case to find tuner entity connected to the decoder entity and check if it is available, and activate it and start pipeline between the source and the entity. The disable_source handler deactivates the link and stops the pipeline. This handler can be invoked from the media core (v4l-core, dvb-core) as well as other drivers such as ALSA that control the media device.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

include/media/media-device.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)

diff --git a/include/media/media-device.h b/include/media/media-device.h index 6520d1c..04b6c2e 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -333,6 +333,25 @@ struct media_device { /* Serializes graph operations. */ struct mutex graph_mutex;

• /* Handlers to find source entity for the sink entity and

• * check if it is available, and activate the link using
  

• * media_entity_setup_link() interface and start pipeline
  

• * from the source to the entity.
  

• * Bridge driver is expected to implement and set the
  

• * handler when media_device is registered or when
  

• * bridge driver finds the media_device during probe.
  

• * Bridge driver sets source_priv with information
  

• * necessary to run enable/disable source handlers.
  

• *
  

• * Use-case: find tuner entity connected to the decoder
  

• * entity and check if it is available, and activate the
  

• * using media_entity_setup_link() if it is available.
  

• */
• void *source_priv;
• int (*enable_source)(struct media_entity *entity,

• 	     struct media_pipeline *pipe);
  

• void (*disable_source)(struct media_entity *entity);

Please document the new fields at the right place (Kernel-doc comment declared before the struct).

Is this used by the media core? If so, please but the implementation here, to make it clearer why we need those things.

• int (*link_notify)(struct media_link *link, u32 flags, unsigned int notification);

};

Em Thu, 28 Jan 2016 09:28:37 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

On 01/28/2016 08:19 AM, Mauro Carvalho Chehab wrote:

...

Em Wed, 6 Jan 2016 13:26:53 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Add new fields to struct media_device to add enable_source, and disable_source handlers, and source_priv to stash driver private data that is need to run these handlers. The enable_source handler finds source entity for the passed in entity and check if it is available, and activate the link using __media_entity_setup_link() interface. Bridge driver is expected to implement and set these handlers and private data when media_device is registered or when bridge driver finds the media_device during probe. This is to enable the use-case to find tuner entity connected to the decoder entity and check if it is available, and activate it and start pipeline between the source and the entity. The disable_source handler deactivates the link and stops the pipeline. This handler can be invoked from the media core (v4l-core, dvb-core) as well as other drivers such as ALSA that control the media device.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

include/media/media-device.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)

diff --git a/include/media/media-device.h b/include/media/media-device.h index 6520d1c..04b6c2e 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -333,6 +333,25 @@ struct media_device { /* Serializes graph operations. */ struct mutex graph_mutex;

• /* Handlers to find source entity for the sink entity and

• * check if it is available, and activate the link using
  

• * media_entity_setup_link() interface and start pipeline
  

• * from the source to the entity.
  

• * Bridge driver is expected to implement and set the
  

• * handler when media_device is registered or when
  

• * bridge driver finds the media_device during probe.
  

• * Bridge driver sets source_priv with information
  

• * necessary to run enable/disable source handlers.
  

• *
  

• * Use-case: find tuner entity connected to the decoder
  

• * entity and check if it is available, and activate the
  

• * using media_entity_setup_link() if it is available.
  

• */
• void *source_priv;
• int (*enable_source)(struct media_entity *entity,

• 	     struct media_pipeline *pipe);
  

• void (*disable_source)(struct media_entity *entity);

Please document the new fields at the right place (Kernel-doc comment declared before the struct).

Is this used by the media core? If so, please but the implementation here, to make it clearer why we need those things.

...

• int (*link_notify)(struct media_link *link, u32 flags, unsigned int notification);

};

Hi Mauro,

I don't have any problems adding documentation. I would like to add documentation in a add-on to the patch series. The main reason is once I add documentation to this patch, the rest of the patches on this file don't apply and require rebase and rework. I went though a couple of rounds of this while you were adding documentation to the interfaces you added.

How about I add the documentation patches at the end of the patch series? I am concerned that rebasing for the documentation changes will introduce bugs. Are you okay with this proposal?

I'm ok with that for those inlined kernel-doc stuff. On the changes at the uapi/linux/media.h, I would prefer if you could add the documentation together with the patch, as it makes clearer for the reviewers. As you only touched it on one or two patches, this won't cause any breakages on the remaining patches.

Regards, Mauro

thanks, -- Shuah

Em Wed, 6 Jan 2016 13:26:57 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Add a new interfaces to be used by v4l-core to invoke enable source and disable_source handlers in the media_device. The enable_source helper function invokes the enable_source handler to find tuner entity connected to the decoder and check is it is available or busy. If tuner is available, link is activated and pipeline is started. The disable_source helper function invokes the disable_source handler to deactivate and stop the pipeline.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/v4l2-core/v4l2-dev.c | 27 +++++++++++++++++++++++++++ include/media/v4l2-dev.h | 4 ++++ 2 files changed, 31 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index d8e5994..f06da6e 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -233,6 +233,33 @@ struct video_device *video_devdata(struct file *file) } EXPORT_SYMBOL(video_devdata);

+int v4l_enable_media_tuner(struct video_device *vdev)

IMHO, the better is to put those MC ancillary routines on a separate file. Hans suggested to add them at v4l2-mc.h and v4l2-mc.h.

+{ +#ifdef CONFIG_MEDIA_CONTROLLER

• struct media_device *mdev = vdev->entity.graph_obj.mdev;
• int ret;
• if (!mdev || !mdev->enable_source)

• return 0;
  

• ret = mdev->enable_source(&vdev->entity, &vdev->pipe);
• if (ret)

• return -EBUSY;
  

• return 0;

+#endif /* CONFIG_MEDIA_CONTROLLER */

• return 0;

+} +EXPORT_SYMBOL_GPL(v4l_enable_media_tuner);

+void v4l_disable_media_tuner(struct video_device *vdev) +{ +#ifdef CONFIG_MEDIA_CONTROLLER

• struct media_device *mdev = vdev->entity.graph_obj.mdev;
• if (mdev && mdev->disable_source)

• mdev->disable_source(&vdev->entity);
  

+#endif /* CONFIG_MEDIA_CONTROLLER */ +} +EXPORT_SYMBOL_GPL(v4l_disable_media_tuner);

/* Priority handling */

diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h index eeabf20..68999a3 100644 --- a/include/media/v4l2-dev.h +++ b/include/media/v4l2-dev.h @@ -87,6 +87,7 @@ struct video_device #if defined(CONFIG_MEDIA_CONTROLLER) struct media_entity entity; struct media_intf_devnode *intf_devnode;

• struct media_pipeline pipe;

#endif /* device ops */ const struct v4l2_file_operations *fops; @@ -176,6 +177,9 @@ void video_unregister_device(struct video_device *vdev); latter can also be used for video_device->release(). */ struct video_device * __must_check video_device_alloc(void);

+int v4l_enable_media_tuner(struct video_device *vdev); +void v4l_disable_media_tuner(struct video_device *vdev);

Documentation?

/* this release function frees the vdev pointer */ void video_device_release(struct video_device *vdev);

On 01/28/2016 08:29 AM, Mauro Carvalho Chehab wrote:

Em Wed, 6 Jan 2016 13:26:58 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Add a new v4l_vb2q_enable_media_tuner() wrapper function to be called from v4l2-core to enable the media tuner with videobuf2 queue, when the calling frunction has the videobuf2 queue and doesn't have the struct video_device associated with the queue handy as in the case of vb2_core_streamon(). This function simply calls v4l_enable_media_tuner() passing in the pointer to struct video_device.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/v4l2-core/v4l2-dev.c | 21 +++++++++++++++++++++ include/media/v4l2-dev.h | 1 + 2 files changed, 22 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c index f06da6e..9ef675a 100644 --- a/drivers/media/v4l2-core/v4l2-dev.c +++ b/drivers/media/v4l2-core/v4l2-dev.c @@ -30,6 +30,7 @@ #include <media/v4l2-common.h> #include <media/v4l2-device.h> #include <media/v4l2-ioctl.h> +#include <media/videobuf2-core.h>

#define VIDEO_NUM_DEVICES 256 #define VIDEO_NAME "video4linux" @@ -261,6 +262,26 @@ void v4l_disable_media_tuner(struct video_device *vdev) } EXPORT_SYMBOL_GPL(v4l_disable_media_tuner);

+/**

• • v4l_vb2q_enable_media_tuner - Wrapper for v4l_enable_media_tuner()
• • @q: videobuf2 queue
• • This function is intended to be called from v4l2-core
• • to enable the media tuner with videobuf2 queue, when
• • the calling frunction has the videobuf2 queue and doesn't

typo: function

ok thanks

...

• • have the struct video_device associated with the
• • queue handy as in the case of vb2_core_streamon(). This
• • function simply calls v4l_enable_media_tuner() passing
• • in the pointer to struct video_device.

The hole description seems confusing. I'm not seeing the light about why this is needed.

Sorry if the description isn't clear. During videobuf2 work, owner field in struct vb2_queue is changed from struct v4l2_fh * to void. Prior to this work, I could call v4l_enable_media_tuner().

+ ret = v4l_enable_media_tuner(q->owner->vdev); + if (ret) + return ret;

As you can see with the videobuf2, to be able to call v4l_enable_media_tuner() from vb2_core_streamon(), I have had to this wrapper to maintain the abstraction introduced in videobuf2 work.

Hope this helps the need for this wrapper.

thanks, -- Shuah

...

• */

+int v4l_vb2q_enable_media_tuner(struct vb2_queue *q) +{

• struct v4l2_fh *fh = q->owner;
• return v4l_enable_media_tuner(fh->vdev);

+} +EXPORT_SYMBOL_GPL(v4l_vb2q_enable_media_tuner);

/* Priority handling */

static inline bool prio_is_valid(enum v4l2_priority prio) diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h index 68999a3..1948097 100644 --- a/include/media/v4l2-dev.h +++ b/include/media/v4l2-dev.h @@ -179,6 +179,7 @@ struct video_device * __must_check video_device_alloc(void);

int v4l_enable_media_tuner(struct video_device *vdev); void v4l_disable_media_tuner(struct video_device *vdev); +int v4l_vb2q_enable_media_tuner(struct vb2_queue *q);

Documentation?

...

/* this release function frees the vdev pointer */ void video_device_release(struct video_device *vdev);

Em Wed, 6 Jan 2016 13:27:02 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

au0828_create_media_graph() doesn't do any checks to determine, if vbi_dev, vdev, and input entities have been registered prior to creating pad links. Checking graph_obj.mdev field works as the graph_obj.mdev field gets initialized in the entity register interface. Fix it to check graph_obj.mdev field before creating pad links.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index f46fb43..8ef7c71 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -291,20 +291,27 @@ static int au0828_create_media_graph(struct au0828_dev *dev) if (ret) return ret; }

• ret = media_create_pad_link(decoder, AU8522_PAD_VID_OUT,

• 		    &dev->vdev.entity, 0,
  

• 		    MEDIA_LNK_FL_ENABLED);
  

• if (ret)

• return ret;
  

• ret = media_create_pad_link(decoder, AU8522_PAD_VBI_OUT,

• 		    &dev->vbi_dev.entity, 0,
  

• 		    MEDIA_LNK_FL_ENABLED);
  

• if (ret)

• return ret;
  

• if (dev->vdev.entity.graph_obj.mdev) {

• ret = media_create_pad_link(decoder, AU8522_PAD_VID_OUT,
  

• 			    &dev->vdev.entity, 0,
  

• 			    MEDIA_LNK_FL_ENABLED);
  

• if (ret)
  

• 	return ret;
  

• }

Those new if() doesn't look right. We can't continue if the entities weren't registered, as the graph would have troubles. The logic should ensure that the entities will always be created before running au0828_create_media_graph(). If this is not the case, some async logic is needed to ensure that.

• if (dev->vbi_dev.entity.graph_obj.mdev) {

• ret = media_create_pad_link(decoder, AU8522_PAD_VBI_OUT,
  

• 			    &dev->vbi_dev.entity, 0,
  

• 			    MEDIA_LNK_FL_ENABLED);
  

• if (ret)
  

• 	return ret;
  

• }

  for (i = 0; i < AU0828_MAX_INPUT; i++) { struct media_entity *ent = &dev->input_ent[i];

• if (!ent->graph_obj.mdev)
  

• 	continue;
  

• if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED) break;

Em Wed, 6 Jan 2016 13:27:04 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Checking for tuner availability from frontend thread start disrupts video stream. Change to check for tuner and start pipeline from frontend open instead and stop pipeline from frontend release.

That's wrong, as DVB can be opened on read-only mode, where it won't be changing anything.

Also, I don't think POSIX allows to return an error like EBUSY on open: http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

In addition, make a change to invoke enable_source and disable_source handlers to check for tuner availability. The enable_source handler finds tuner entity connected to the decoder and check is it is available or busy. If tuner is available, link is activated and pipeline is started. The disable_source handler to deactivate and stop the pipeline. dvb_enable_media_tuner() is removed as it is no longer necessary with dvb invoking enable_source and disable_source handlers. pipe_start_entity field is removed and pipe field is moved to dvb_frontend from dvb_frontend_private.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/dvb-core/dvb_frontend.c | 139 +++++----------------------------- drivers/media/dvb-core/dvb_frontend.h | 3 + 2 files changed, 24 insertions(+), 118 deletions(-)

diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 4008064..c15f3d8 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -131,11 +131,6 @@ struct dvb_frontend_private { int quality; unsigned int check_wrapped; enum dvbfe_search algo_status;

-#if defined(CONFIG_MEDIA_CONTROLLER_DVB)

• struct media_pipeline pipe;
• struct media_entity *pipe_start_entity;

-#endif };

static void dvb_frontend_wakeup(struct dvb_frontend *fe); @@ -596,104 +591,12 @@ static void dvb_frontend_wakeup(struct dvb_frontend *fe) wake_up_interruptible(&fepriv->wait_queue); }

-/**

• • dvb_enable_media_tuner() - tries to enable the DVB tuner
• • @fe: struct dvb_frontend pointer
• • This function ensures that just one media tuner is enabled for a given
• • frontend. It has two different behaviors:
• • • For trivial devices with just one tuner:
• • it just enables the existing tuner->fe link
• • • For devices with more than one tuner:
• • It is up to the driver to implement the logic that will enable one tuner
• • and disable the other ones. However, if more than one tuner is enabled for
• • the same frontend, it will print an error message and return -EINVAL.
• • At return, it will return the error code returned by media_entity_setup_link,
• • or 0 if everything is OK, if no tuner is linked to the frontend or if the
• • mdev is NULL.
• */

-#ifdef CONFIG_MEDIA_CONTROLLER_DVB -static int dvb_enable_media_tuner(struct dvb_frontend *fe) -{

• struct dvb_frontend_private *fepriv = fe->frontend_priv;
• struct dvb_adapter *adapter = fe->dvb;
• struct media_device *mdev = adapter->mdev;
• struct media_entity *entity, *source;
• struct media_link *link, *found_link = NULL;
• int ret, n_links = 0, active_links = 0;
• fepriv->pipe_start_entity = NULL;
• if (!mdev)

• return 0;
  

• entity = fepriv->dvbdev->entity;
• fepriv->pipe_start_entity = entity;
• list_for_each_entry(link, &entity->links, list) {

• if (link->sink->entity == entity) {
  

• 	found_link = link;
  

• 	n_links++;
  

• 	if (link->flags & MEDIA_LNK_FL_ENABLED)
  

• 		active_links++;
  

• }
  

• }
• if (!n_links || active_links == 1 || !found_link)

• return 0;
  

• /*

• * If a frontend has more than one tuner linked, it is up to the driver
  

• * to select with one will be the active one, as the frontend core can't
  

• * guess. If the driver doesn't do that, it is a bug.
  

• */
  

• if (n_links > 1 && active_links != 1) {

• dev_err(fe->dvb->device,
  

• 	"WARNING: there are %d active links among %d tuners. This is a driver's bug!\n",
  

• 	active_links, n_links);
  

• return -EINVAL;
  

• }
• source = found_link->source->entity;
• fepriv->pipe_start_entity = source;
• list_for_each_entry(link, &source->links, list) {

• struct media_entity *sink;
  

• int flags = 0;
  

• sink = link->sink->entity;
  

• if (sink == entity)
  

• 	flags = MEDIA_LNK_FL_ENABLED;
  

• ret = media_entity_setup_link(link, flags);
  

• if (ret) {
  

• 	dev_err(fe->dvb->device,
  

• 		"Couldn't change link %s->%s to %s. Error %d\n",
  

• 		source->name, sink->name,
  

• 		flags ? "enabled" : "disabled",
  

• 		ret);
  

• 	return ret;
  

• } else
  

• 	dev_dbg(fe->dvb->device,
  

• 		"link %s->%s was %s\n",
  

• 		source->name, sink->name,
  

• 		flags ? "ENABLED" : "disabled");
  

• }
• return 0;

-} -#endif

static int dvb_frontend_thread(void *data) { struct dvb_frontend *fe = data; struct dvb_frontend_private *fepriv = fe->frontend_priv; enum fe_status s; enum dvbfe_algo algo; -#ifdef CONFIG_MEDIA_CONTROLLER_DVB

• int ret;

-#endif

• bool re_tune = false; bool semheld = false;

@@ -706,20 +609,6 @@ static int dvb_frontend_thread(void *data) fepriv->wakeup = 0; fepriv->reinitialise = 0;

-#ifdef CONFIG_MEDIA_CONTROLLER_DVB

• ret = dvb_enable_media_tuner(fe);
• if (ret) {

• /* FIXME: return an error if it fails */
  

• dev_info(fe->dvb->device,
  

• 	"proceeding with FE task\n");
  

• } else if (fepriv->pipe_start_entity) {

• ret = media_entity_pipeline_start(fepriv->pipe_start_entity,
  

• 				  &fepriv->pipe);
  

• if (ret)
  

• 	return ret;
  

• }

-#endif

• dvb_frontend_init(fe);

  set_freezable();

@@ -829,12 +718,6 @@ restart: } }

-#ifdef CONFIG_MEDIA_CONTROLLER_DVB

• if (fepriv->pipe_start_entity)

• media_entity_pipeline_stop(fepriv->pipe_start_entity);
  

• fepriv->pipe_start_entity = NULL;

-#endif

• if (dvb_powerdown_on_sleep) { if (fe->ops.set_voltage) fe->ops.set_voltage(fe, SEC_VOLTAGE_OFF);

@@ -2612,9 +2495,20 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) fepriv->tone = -1; fepriv->voltage = -1;

+#ifdef CONFIG_MEDIA_CONTROLLER_DVB

• if (fe->dvb->mdev && fe->dvb->mdev->enable_source) {
  

• 	ret = fe->dvb->mdev->enable_source(dvbdev->entity,
  

• 					   &fe->pipe);
  

• 	if (ret) {
  

• 		dev_err(fe->dvb->device,
  

• 			"Tuner is busy. Error %d\n", ret);
  

• 		goto err2;
  

• 	}
  

• }
  

+#endif ret = dvb_frontend_start (fe); if (ret)

• 	goto err2;
  

• 	goto err3;
  
  

  /* empty event queue */ fepriv->events.eventr = fepriv->events.eventw = 0;

@@ -2624,7 +2518,12 @@ static int dvb_frontend_open(struct inode *inode, struct file *file) mutex_unlock (&adapter->mfe_lock); return ret;

+err3: +#ifdef CONFIG_MEDIA_CONTROLLER_DVB

• if (fe->dvb->mdev && fe->dvb->mdev->disable_source)

• fe->dvb->mdev->disable_source(dvbdev->entity);
  

err2: +#endif dvb_generic_release(inode, file); err1: if (dvbdev->users == -1 && fe->ops.ts_bus_ctrl) @@ -2653,6 +2552,10 @@ static int dvb_frontend_release(struct inode *inode, struct file *file)

if (dvbdev->users == -1) { wake_up(&fepriv->wait_queue); +#ifdef CONFIG_MEDIA_CONTROLLER_DVB

• if (fe->dvb->mdev && fe->dvb->mdev->disable_source)
  

• 	fe->dvb->mdev->disable_source(dvbdev->entity);
  

+#endif if (fe->exit != DVB_FE_NO_EXIT) wake_up(&dvbdev->wait_queue); if (fe->ops.ts_bus_ctrl) diff --git a/drivers/media/dvb-core/dvb_frontend.h b/drivers/media/dvb-core/dvb_frontend.h index 458bcce..9466906 100644 --- a/drivers/media/dvb-core/dvb_frontend.h +++ b/drivers/media/dvb-core/dvb_frontend.h @@ -686,6 +686,9 @@ struct dvb_frontend { int (*callback)(void *adapter_priv, int component, int cmd, int arg); int id; unsigned int exit; +#if defined(CONFIG_MEDIA_CONTROLLER_DVB)

• struct media_pipeline pipe;

+#endif };

/**

Em Wed, 6 Jan 2016 13:27:06 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

au0828 is changed to use v4l_enable_media_tuner() to check for tuner availability from vidioc_g_tuner(), and au0828_v4l2_close(), before changing tuner settings. If tuner isn't free, return busy condition from vidioc_g_tuner() and in au0828_v4l2_close() tuner is left untouched without powering down to save energy.

Did you test the code when the input is not a tuner, but, instead, Composite or S-Video connector, as shown at: https://mchehab.fedorapeople.org/mc-next-gen/au0828.png

I guess calling it v4l-enable_media_tuner() is not right, specially since there are hybrid devices that have DTV (via DVB API) and S-Video and/or Composite/RCA capture via V4L2 API.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-video.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c index 32bcc56..ed3ba05 100644 --- a/drivers/media/usb/au0828/au0828-video.c +++ b/drivers/media/usb/au0828/au0828-video.c @@ -1010,8 +1010,12 @@ static int au0828_v4l2_close(struct file *filp) goto end;

if (dev->users == 1) {

• /* Save some power by putting tuner to sleep */
  

• v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 0);
  

• /* Save some power by putting tuner to sleep, if it is free */
  

• /* What happens when radio is using tuner?? */
  

• ret = v4l_enable_media_tuner(vdev);
  

• if (ret == 0)
  

• 	v4l2_device_call_all(&dev->v4l2_dev, 0, core,
  

• 			     s_power, 0);
  

  dev->std_set_in_tuner_core = 0;

  /* When close the device, set the usb intf0 into alt0 to free

@@ -1412,10 +1416,16 @@ static int vidioc_s_audio(struct file *file, void *priv, const struct v4l2_audio static int vidioc_g_tuner(struct file *file, void *priv, struct v4l2_tuner *t) { struct au0828_dev *dev = video_drvdata(file);

• struct video_device *vfd = video_devdata(file);

• int ret;

  if (t->index != 0) return -EINVAL;

• ret = v4l_enable_media_tuner(vfd);

• if (ret)

• return ret;
  

• dprintk(1, "%s called std_set %d dev_state %d\n", __func__, dev->std_set_in_tuner_core, dev->dev_state);

Em Wed, 6 Jan 2016 13:27:09 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

au0828 registers entity_notify hook to create media graph for the device. This handler runs whenvere a new entity gets added

typo: whenever.

to the media device. It creates necessary links from video, vbi, and ALSA entities to decoder and links tuner and decoder entities. As this handler runs as entities get added, it has to maintain state on the links it already created. New fields are added to au0828_dev to keep this state information. entity_notify gets unregistered before media_device unregister.

Bty, please avoid long paragraphs at the patch description, and please try to be clearer on your patch descriptions... That makes boring to read everything. :-;

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 104 +++++++++++++++++++++++---------- drivers/media/usb/au0828/au0828.h | 6 ++ 2 files changed, 78 insertions(+), 32 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 6ef177c..a381660 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -137,6 +137,8 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) #ifdef CONFIG_MEDIA_CONTROLLER if (dev->media_dev && media_devnode_is_registered(&dev->media_dev->devnode)) {

• media_device_unregister_entity_notify(dev->media_dev,
  

• 				      &dev->entity_notify);
  

  media_device_unregister(dev->media_dev); media_device_cleanup(dev->media_dev); dev->media_dev = NULL;

@@ -263,11 +265,16 @@ static int au0828_create_media_graph(struct au0828_dev *dev) struct media_device *mdev = dev->media_dev; struct media_entity *entity; struct media_entity *tuner = NULL, *decoder = NULL;

• struct media_entity *audio_capture = NULL; int i, ret;

  if (!mdev) return 0;

• if (dev->tuner_linked && dev->vdev_linked && dev->vbi_linked &&

•    dev->audio_capture_linked)
  

• return 0;
  

• media_device_for_each_entity(entity, mdev) { switch (entity->function) { case MEDIA_ENT_F_TUNER:

@@ -276,6 +283,9 @@ static int au0828_create_media_graph(struct au0828_dev *dev) case MEDIA_ENT_F_ATV_DECODER: decoder = entity; break;

• case MEDIA_ENT_F_AUDIO_CAPTURE:
  

• 	audio_capture = entity;
  

• 	break;
  

  } }

@@ -285,60 +295,77 @@ static int au0828_create_media_graph(struct au0828_dev *dev) if (!decoder) return -EINVAL;

• if (tuner) {

• if (tuner && !dev->tuner_linked) {

• dev->tuner = tuner;
  

  ret = media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0, MEDIA_LNK_FL_ENABLED); if (ret) return ret;

• dev->tuner_linked = 1;
  

  }

• if (dev->vdev.entity.graph_obj.mdev) {

• if (dev->vdev.entity.graph_obj.mdev && !dev->vdev_linked) { ret = media_create_pad_link(decoder, AU8522_PAD_VID_OUT, &dev->vdev.entity, 0, MEDIA_LNK_FL_ENABLED); if (ret) return ret;

• dev->vdev_linked = 1;
  

  }

• if (dev->vbi_dev.entity.graph_obj.mdev) {

• if (dev->vbi_dev.entity.graph_obj.mdev && !dev->vbi_linked) { ret = media_create_pad_link(decoder, AU8522_PAD_VBI_OUT, &dev->vbi_dev.entity, 0, MEDIA_LNK_FL_ENABLED); if (ret) return ret;

• }
• for (i = 0; i < AU0828_MAX_INPUT; i++) {

• struct media_entity *ent = &dev->input_ent[i];
  

• dev->vbi_linked = 1;
  
  

• if (!ent->graph_obj.mdev)
  

• 	continue;
  

• /*
  

•  * Input entities are registered before vbi entity,
  

•  * create graph nodes for them after vbi is created
  

• */
  

• for (i = 0; i < AU0828_MAX_INPUT; i++) {
  

• 	struct media_entity *ent = &dev->input_ent[i];
  
  

• if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
  

• 	break;
  

• 	if (!ent->graph_obj.mdev)
  

• 		continue;
  
  

• switch (AUVI_INPUT(i).type) {
  

• case AU0828_VMUX_CABLE:
  

• case AU0828_VMUX_TELEVISION:
  

• case AU0828_VMUX_DVB:
  

• 	if (!tuner)
  

• 	if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
  	break;
  
  

• 	ret = media_create_pad_link(ent, 0, tuner,
  

• 				    TUNER_PAD_RF_INPUT,
  

• 				    MEDIA_LNK_FL_ENABLED);
  

• 	if (ret)
  

• 		return ret;
  

• 	break;
  

• case AU0828_VMUX_COMPOSITE:
  

• case AU0828_VMUX_SVIDEO:
  

• default: /* AU0828_VMUX_DEBUG */
  

• 	/* FIXME: fix the decoder PAD */
  

• 	ret = media_create_pad_link(ent, 0, decoder, 0, 0);
  

• 	if (ret)
  

• 		return ret;
  

• 	break;
  

• 	switch (AUVI_INPUT(i).type) {
  

• 	case AU0828_VMUX_CABLE:
  

• 	case AU0828_VMUX_TELEVISION:
  

• 	case AU0828_VMUX_DVB:
  

• 		if (!tuner)
  

• 			break;
  

• 		ret = media_create_pad_link(ent, 0, tuner,
  

• 					TUNER_PAD_RF_INPUT,
  

• 					MEDIA_LNK_FL_ENABLED);
  

• 		if (ret)
  

• 			return ret;
  

• 		break;
  

• 	case AU0828_VMUX_COMPOSITE:
  

• 	case AU0828_VMUX_SVIDEO:
  

• 	default: /* AU0828_VMUX_DEBUG */
  

• 		/* FIXME: fix the decoder PAD */
  

• 		ret = media_create_pad_link(ent, 0, decoder,
  

• 					    0, 0);
  

• 		if (ret)
  

• 			return ret;
  

• 		break;
  

• 	}
  

  } }
• if (audio_capture && !dev->audio_capture_linked) {

• ret = media_create_pad_link(decoder, AU8522_PAD_AUDIO_OUT,
  

• 			    audio_capture, 0,
  

• 			    MEDIA_LNK_FL_ENABLED);
  

• if (ret)
  

• 	return ret;
  

• dev->audio_capture_linked = 1;
  

• }

#endif return 0; } @@ -349,8 +376,10 @@ static int au0828_media_device_register(struct au0828_dev *dev, #ifdef CONFIG_MEDIA_CONTROLLER int ret;

• if (dev->media_dev &&

• !media_devnode_is_registered(&dev->media_dev->devnode)) {
  

• if (!dev->media_dev)

• return 0;
  

• if (!media_devnode_is_registered(&dev->media_dev->devnode)) {

  /* register media device */ ret = media_device_register(dev->media_dev);

@@ -360,6 +389,17 @@ static int au0828_media_device_register(struct au0828_dev *dev, return ret; } }

• /* register entity_notify callback */
• dev->entity_notify.notify_data = (void *) dev;
• dev->entity_notify.notify = (void *) au0828_create_media_graph;
• ret = media_device_register_entity_notify(dev->media_dev,

• 				  &dev->entity_notify);
  

• if (ret) {

• dev_err(&udev->dev,
  

• 	"Media Device register entity_notify Error: %d\n",
  

• 	ret);
  

• return ret;
  

• }

#endif return 0; } diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h index 8276072..cfb6d58 100644 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -283,6 +283,12 @@ struct au0828_dev { struct media_entity *decoder; struct media_entity input_ent[AU0828_MAX_INPUT]; struct media_pad input_pad[AU0828_MAX_INPUT];

• struct media_entity_notify entity_notify;
• struct media_entity *tuner;
• bool tuner_linked;
• bool vdev_linked;
• bool vbi_linked;
• bool audio_capture_linked;

Hmm... now I understood why you did the changes on patch 13/31.

I see what you're doing, but not sure if this is a good idea to have one bool for each possible device. On au0828, the topology is actually simpler than on other devices, as it currently supports a very few set of I2C devices, but on other drivers, things can be messier.

See, for example, two graphs for em28xx-based devices: https://mchehab.fedorapeople.org/mc-next-gen/wintv_usb2.png https://mchehab.fedorapeople.org/mc-next-gen/hvr_950.png

On the first graph, the tuner is not connected directly to the analog demod, but, instead, to two other elements: - tda9887 - for video - msp3400 - for audio

IMHO, the best way to handle graph setup is that each driver should handle the links that belong only to them synchronously, after creating/registering all the entities.

So, only the links between two drivers would be asynchronously created. So, In the case of au0828:

- au0828 core will create the connector entities;

- I2C drivers will create their own entities;

- DVB core will create the DVB entities/interfaces;

- V4L core will create V4L interfaces and I/O entities;

- au0828 V4L driver will create all V4L links, after ensuring that the needed I2C drivers were bound;

- snd-usb-audio will create all ALSA-specific entities links;

The V4L->ALSA links will either be created by au0828-core, via the notification handler.

With that in mind, I don't see any need to touch at au0828_create_media_graph(). It will need an extra function to handle the notification when ALSA gets registered (or when the entities there are added, whatever works best).

Regards, Mauro

#endif };

Em Thu, 28 Jan 2016 13:05:35 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

On 01/28/2016 09:36 AM, Mauro Carvalho Chehab wrote:

...

Em Wed, 6 Jan 2016 13:27:09 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

au0828 registers entity_notify hook to create media graph for the device. This handler runs whenvere a new entity gets added

typo: whenever.

ok

...

...

to the media device. It creates necessary links from video, vbi, and ALSA entities to decoder and links tuner and decoder entities. As this handler runs as entities get added, it has to maintain state on the links it already created. New fields are added to au0828_dev to keep this state information. entity_notify gets unregistered before media_device unregister.

Bty, please avoid long paragraphs at the patch description, and please try to be clearer on your patch descriptions... That makes boring to read everything. :-;

Yeah I like to add details I consider relevant to the log. I will try to be concise.

It is not a matter of removing details, but better organizing the ideas. A single huge paragraph with lots of contents mixed inside doesn't make easier for the readers ;)

...

...

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 104 +++++++++++++++++++++++---------- drivers/media/usb/au0828/au0828.h | 6 ++ 2 files changed, 78 insertions(+), 32 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 6ef177c..a381660 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -137,6 +137,8 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) #ifdef CONFIG_MEDIA_CONTROLLER if (dev->media_dev && media_devnode_is_registered(&dev->media_dev->devnode)) {

• media_device_unregister_entity_notify(dev->media_dev,
  

• 				      &dev->entity_notify);
  

  media_device_unregister(dev->media_dev); media_device_cleanup(dev->media_dev); dev->media_dev = NULL;

@@ -263,11 +265,16 @@ static int au0828_create_media_graph(struct au0828_dev *dev) struct media_device *mdev = dev->media_dev; struct media_entity *entity; struct media_entity *tuner = NULL, *decoder = NULL;

• struct media_entity *audio_capture = NULL; int i, ret;

  if (!mdev) return 0;

• if (dev->tuner_linked && dev->vdev_linked && dev->vbi_linked &&

•    dev->audio_capture_linked)
  

• return 0;
  

• media_device_for_each_entity(entity, mdev) { switch (entity->function) { case MEDIA_ENT_F_TUNER:

@@ -276,6 +283,9 @@ static int au0828_create_media_graph(struct au0828_dev *dev) case MEDIA_ENT_F_ATV_DECODER: decoder = entity; break;

• case MEDIA_ENT_F_AUDIO_CAPTURE:
  

• 	audio_capture = entity;
  

• 	break;
  

  } }

@@ -285,60 +295,77 @@ static int au0828_create_media_graph(struct au0828_dev *dev) if (!decoder) return -EINVAL;

• if (tuner) {

• if (tuner && !dev->tuner_linked) {

• dev->tuner = tuner;
  

  ret = media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT, decoder, 0, MEDIA_LNK_FL_ENABLED); if (ret) return ret;

• dev->tuner_linked = 1;
  

  }

• if (dev->vdev.entity.graph_obj.mdev) {

• if (dev->vdev.entity.graph_obj.mdev && !dev->vdev_linked) { ret = media_create_pad_link(decoder, AU8522_PAD_VID_OUT, &dev->vdev.entity, 0, MEDIA_LNK_FL_ENABLED); if (ret) return ret;

• dev->vdev_linked = 1;
  

  }

• if (dev->vbi_dev.entity.graph_obj.mdev) {

• if (dev->vbi_dev.entity.graph_obj.mdev && !dev->vbi_linked) { ret = media_create_pad_link(decoder, AU8522_PAD_VBI_OUT, &dev->vbi_dev.entity, 0, MEDIA_LNK_FL_ENABLED); if (ret) return ret;

• }
• for (i = 0; i < AU0828_MAX_INPUT; i++) {

• struct media_entity *ent = &dev->input_ent[i];
  

• dev->vbi_linked = 1;
  
  

• if (!ent->graph_obj.mdev)
  

• 	continue;
  

• /*
  

•  * Input entities are registered before vbi entity,
  

•  * create graph nodes for them after vbi is created
  

• */
  

• for (i = 0; i < AU0828_MAX_INPUT; i++) {
  

• 	struct media_entity *ent = &dev->input_ent[i];
  
  

• if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
  

• 	break;
  

• 	if (!ent->graph_obj.mdev)
  

• 		continue;
  
  

• switch (AUVI_INPUT(i).type) {
  

• case AU0828_VMUX_CABLE:
  

• case AU0828_VMUX_TELEVISION:
  

• case AU0828_VMUX_DVB:
  

• 	if (!tuner)
  

• 	if (AUVI_INPUT(i).type == AU0828_VMUX_UNDEFINED)
  	break;
  
  

• 	ret = media_create_pad_link(ent, 0, tuner,
  

• 				    TUNER_PAD_RF_INPUT,
  

• 				    MEDIA_LNK_FL_ENABLED);
  

• 	if (ret)
  

• 		return ret;
  

• 	break;
  

• case AU0828_VMUX_COMPOSITE:
  

• case AU0828_VMUX_SVIDEO:
  

• default: /* AU0828_VMUX_DEBUG */
  

• 	/* FIXME: fix the decoder PAD */
  

• 	ret = media_create_pad_link(ent, 0, decoder, 0, 0);
  

• 	if (ret)
  

• 		return ret;
  

• 	break;
  

• 	switch (AUVI_INPUT(i).type) {
  

• 	case AU0828_VMUX_CABLE:
  

• 	case AU0828_VMUX_TELEVISION:
  

• 	case AU0828_VMUX_DVB:
  

• 		if (!tuner)
  

• 			break;
  

• 		ret = media_create_pad_link(ent, 0, tuner,
  

• 					TUNER_PAD_RF_INPUT,
  

• 					MEDIA_LNK_FL_ENABLED);
  

• 		if (ret)
  

• 			return ret;
  

• 		break;
  

• 	case AU0828_VMUX_COMPOSITE:
  

• 	case AU0828_VMUX_SVIDEO:
  

• 	default: /* AU0828_VMUX_DEBUG */
  

• 		/* FIXME: fix the decoder PAD */
  

• 		ret = media_create_pad_link(ent, 0, decoder,
  

• 					    0, 0);
  

• 		if (ret)
  

• 			return ret;
  

• 		break;
  

• 	}
  

  } }
• if (audio_capture && !dev->audio_capture_linked) {

• ret = media_create_pad_link(decoder, AU8522_PAD_AUDIO_OUT,
  

• 			    audio_capture, 0,
  

• 			    MEDIA_LNK_FL_ENABLED);
  

• if (ret)
  

• 	return ret;
  

• dev->audio_capture_linked = 1;
  

• }

#endif return 0; } @@ -349,8 +376,10 @@ static int au0828_media_device_register(struct au0828_dev *dev, #ifdef CONFIG_MEDIA_CONTROLLER int ret;

• if (dev->media_dev &&

• !media_devnode_is_registered(&dev->media_dev->devnode)) {
  

• if (!dev->media_dev)

• return 0;
  

• if (!media_devnode_is_registered(&dev->media_dev->devnode)) {

  /* register media device */ ret = media_device_register(dev->media_dev);

@@ -360,6 +389,17 @@ static int au0828_media_device_register(struct au0828_dev *dev, return ret; } }

• /* register entity_notify callback */
• dev->entity_notify.notify_data = (void *) dev;
• dev->entity_notify.notify = (void *) au0828_create_media_graph;
• ret = media_device_register_entity_notify(dev->media_dev,

• 				  &dev->entity_notify);
  

• if (ret) {

• dev_err(&udev->dev,
  

• 	"Media Device register entity_notify Error: %d\n",
  

• 	ret);
  

• return ret;
  

• }

#endif return 0; } diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h index 8276072..cfb6d58 100644 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -283,6 +283,12 @@ struct au0828_dev { struct media_entity *decoder; struct media_entity input_ent[AU0828_MAX_INPUT]; struct media_pad input_pad[AU0828_MAX_INPUT];

• struct media_entity_notify entity_notify;
• struct media_entity *tuner;
• bool tuner_linked;
• bool vdev_linked;
• bool vbi_linked;
• bool audio_capture_linked;

Hmm... now I understood why you did the changes on patch 13/31.

I see what you're doing, but not sure if this is a good idea to have one bool for each possible device. On au0828, the topology is actually simpler than on other devices, as it currently supports a very few set of I2C devices, but on other drivers, things can be messier.

See, for example, two graphs for em28xx-based devices: https://mchehab.fedorapeople.org/mc-next-gen/wintv_usb2.png https://mchehab.fedorapeople.org/mc-next-gen/hvr_950.png

On the first graph, the tuner is not connected directly to the analog demod, but, instead, to two other elements:

• tda9887 - for video
• msp3400 - for audio

IMHO, the best way to handle graph setup is that each driver should handle the links that belong only to them synchronously, after creating/registering all the entities.

Except for the ALSA part of the graph, we are close to drivers creating their graphs. You are right that au0828 graph creation routine shouldn't need to create links from Alsa mixer to Alsa capture nodes.

...

So, only the links between two drivers would be asynchronously created. So, In the case of au0828:

• au0828 core will create the connector entities;

• I2C drivers will create their own entities;

• DVB core will create the DVB entities/interfaces;

• V4L core will create V4L interfaces and I/O entities;

• au0828 V4L driver will create all V4L links, after ensuring that the needed I2C drivers were bound;

• snd-usb-audio will create all ALSA-specific entities links;

The V4L->ALSA links will either be created by au0828-core, via the notification handler.

Right - that is part of what au0828_create_media_graph() is doing now.

...

With that in mind, I don't see any need to touch at au0828_create_media_graph(). It will need an extra function to handle the notification when ALSA gets registered (or when the entities there are added, whatever works best).

If I understand correctly, what you are saying is:

Don't add async handling that creates V4L->ALSA to au0828_create_media_graph() and keep that in an async handler.

I will make changes to address this.

Yes, instead of modifying the existing routine to make it async and harder to read, add a separate function that will handle just the things that need to be async there. That should reduce the code complexity and the possibility of having potencial hidden race conditions.

It also allows more complex logic on the sync routine when it is needed, like the one needed to identify if the tuner is alone, or if it has multiple IF and/or video processors (with is common on some kinds of hardware). See such logic at: https://git.linuxtv.org/mchehab/experimental.git/commit/?h=mc_em28xx&id=...

on em28xx_v4l2_create_media_graph() where the tuner logic needs to check for both audio and video IF-PLL hardware.

Regards, Mauro

thanks, -- Shuah

...

...

#endif };

On 01/28/2016 09:38 AM, Mauro Carvalho Chehab wrote:

Em Wed, 6 Jan 2016 13:27:11 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Create tuner to demod pad link in disabled state to help avoid disable step when tuner resource is requested by video or audio.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/dvb-core/dvbdev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index b56e008..1d10fa6 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -593,8 +593,9 @@ int dvb_create_media_graph(struct dvb_adapter *adap) }

if (tuner && demod) {

• /* create tuner to demod link deactivated */
  

  ret = media_create_pad_link(tuner, TUNER_PAD_IF_OUTPUT,

• 			    demod, 0, MEDIA_LNK_FL_ENABLED);
  

• 			    demod, 0, 0);
  

This is not right, as it makes no sense for DVB-only drivers.

Right. Not a good change for DVB only drivers. But does make sense on hybrid. I will make sure it gets done only in hyrbid cases.

thanks, -- Shuah

...

if (ret)
	return ret;

}

Em Wed, 6 Jan 2016 13:27:12 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Implements enable_source and disable_source handlers for other drivers (v4l2-core, dvb-core, and ALSA) to use to check for tuner connected to the decoder and activate the link if tuner is free, and deactivate and free the tuner when it is no longer needed.

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 148 +++++++++++++++++++++++++++++++++ drivers/media/usb/au0828/au0828.h | 2 + 2 files changed, 150 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index a15a61a..f8d2db3 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -370,6 +370,150 @@ static int au0828_create_media_graph(struct au0828_dev *dev) return 0; }

+static int au0828_enable_source(struct media_entity *entity,

• 		struct media_pipeline *pipe)
  

+{

The best would be to put those enable source stuff at the core, in a way that other drivers could share it.

Not sure about the implementation, as this requires testing ;) Did you consider the cases where the source connector is S-Video or Composite?

+#ifdef CONFIG_MEDIA_CONTROLLER

• struct media_entity *source;
• struct media_entity *sink;
• struct media_link *link, *found_link = NULL;
• int ret = 0;
• struct media_device *mdev = entity->graph_obj.mdev;
• struct au0828_dev *dev;
• if (!mdev)

• return -ENODEV;
  

• /* for Audio and Video entities, source is the decoder */
• mutex_lock(&mdev->graph_mutex);
• dev = mdev->source_priv;
• if (!dev->tuner || !dev->decoder) {

• ret = -ENODEV;
  

• goto end;
  

• }
• /*

• * For Audio and V4L2 entity, find the link to which decoder
  

• * is the sink. Look for an active link between decoder and
  

• * tuner, if one exists, nothing to do. If not, look for any
  

• * active links between tuner and any other entity. If one
  

• * exists, tuner is busy. If tuner is free, setup link and
  

• * start pipeline from source (tuner).
  

• * For DVB FE entity, the source for the link is the tuner.
  

• * Check if tuner is available and setup link and start
  

• * pipeline.
  

• */
• if (entity->function != MEDIA_ENT_F_DTV_DEMOD)

• sink = dev->decoder;
  

• else

• sink = entity;
  

• /* Is an active link between sink and tuner */
• if (dev->active_link) {

• if (dev->active_link->sink->entity == sink &&
  

•     dev->active_link->source->entity == dev->tuner) {
  

• 	ret = 0;
  

• 	goto end;
  

• } else {
  

• 	ret = -EBUSY;
  

• 	goto end;
  

• }
  

• }
• list_for_each_entry(link, &sink->links, list) {

• /* Check sink, and source */
  

• if (link->sink->entity == sink &&
  

•     link->source->entity == dev->tuner) {
  

• 	found_link = link;
  

• 	break;
  

• }
  

• }
• if (!found_link) {

• ret = -ENODEV;
  

• goto end;
  

• }
• /* activate link between source and sink and start pipeline */
• source = found_link->source->entity;
• ret = __media_entity_setup_link(found_link, MEDIA_LNK_FL_ENABLED);
• if (ret) {

• pr_err(
  

• 	"Activate tuner link %s->%s. Error %d\n",
  

• 	source->name, sink->name, ret);
  

• goto end;
  

• }
• ret = __media_entity_pipeline_start(entity, pipe);
• if (ret) {

• pr_err("Start Pipeline: %s->%s Error %d\n",
  

• 	source->name, entity->name, ret);
  

• ret = __media_entity_setup_link(found_link, 0);
  

• pr_err("Deactive link Error %d\n", ret);
  

• goto end;
  

• }
• /*

• * save active link and active link owner to avoid audio
  

• * deactivating video owned link from disable_source and
  

• * vice versa
  

• */
• dev->active_link = found_link;
• dev->active_link_owner = entity;

+end:

• mutex_unlock(&mdev->graph_mutex);
• pr_debug("au0828_enable_source() end %s %d %d\n",

• entity->name, entity->function, ret);
  

• return ret;

+#endif

• return 0;

+}

+static void au0828_disable_source(struct media_entity *entity) +{ +#ifdef CONFIG_MEDIA_CONTROLLER

• struct media_entity *sink;
• int ret = 0;
• struct media_device *mdev = entity->graph_obj.mdev;
• struct au0828_dev *dev;
• if (!mdev)

• return;
  

• mutex_lock(&mdev->graph_mutex);
• dev = mdev->source_priv;
• if (!dev->tuner || !dev->decoder || !dev->active_link) {

• ret = -ENODEV;
  

• goto end;
  

• }
• if (entity->function != MEDIA_ENT_F_DTV_DEMOD)

• sink = dev->decoder;
  

• else

• sink = entity;
  

• /* link is active - stop pipeline from source (tuner) */
• if (dev->active_link && dev->active_link->sink->entity == sink &&

•    dev->active_link->source->entity == dev->tuner) {
  

• /*
  

•  * prevent video from deactivating link when audio
  

•  * has active pipeline
  

• */
  

• if (dev->active_link_owner != entity)
  

• 	goto end;
  

• __media_entity_pipeline_stop(entity);
  

• ret = __media_entity_setup_link(dev->active_link, 0);
  

• if (ret)
  

• 	pr_err("Deactive link Error %d\n", ret);
  

• dev->active_link = NULL;
  

• dev->active_link_owner = NULL;
  

• }

+end:

• mutex_unlock(&mdev->graph_mutex);

+#endif +}

static int au0828_media_device_register(struct au0828_dev *dev, struct usb_device *udev) { @@ -400,6 +544,10 @@ static int au0828_media_device_register(struct au0828_dev *dev, ret); return ret; }

• /* set enable_source */
• dev->media_dev->source_priv = (void *) dev;
• dev->media_dev->enable_source = au0828_enable_source;
• dev->media_dev->disable_source = au0828_disable_source;

#endif return 0; } diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h index cfb6d58..3707664 100644 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -289,6 +289,8 @@ struct au0828_dev { bool vdev_linked; bool vbi_linked; bool audio_capture_linked;

• struct media_link *active_link;
• struct media_entity *active_link_owner;

#endif };

On 01/28/2016 09:44 AM, Mauro Carvalho Chehab wrote:

Em Wed, 6 Jan 2016 13:27:13 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Add a new wrapper function to au0828_create_media_graph() to be called as an entity_notify function to fix null pointer dereference. A rebasing mistake resulted in registering au0828_create_media_graph() without the correct parameters which lead to the following null pointer dereference:

[ 69.006164] Call Trace: [ 69.006169] [<ffffffff81a9a1b0>] dump_stack+0x44/0x64 [ 69.006175] [<ffffffff81503af9>] print_trailer+0xf9/0x150 [ 69.006180] [<ffffffff81509284>] object_err+0x34/0x40 [ 69.006185] [<ffffffff815063c4>] ? ___slab_alloc+0x4c4/0x4e0 [ 69.006190] [<ffffffff8150b732>] kasan_report_error+0x212/0x520 [ 69.006196] [<ffffffff815063c4>] ? ___slab_alloc+0x4c4/0x4e0 [ 69.006201] [<ffffffff8150ba83>] __asan_report_load1_noabort+0x43/0x50 [ 69.006208] [<ffffffffa0d30991>] ? au0828_create_media_graph+0x641/0x730 [au0828] [ 69.006215] [<ffffffffa0d30991>] au0828_create_media_graph+0x641/0x730 [au0828] [ 69.006221] [<ffffffff82245c3d>] media_device_register_entity+0x33d/0x4f0 [ 69.006234] [<ffffffffa0ebeb1c>] media_stream_init+0x2ac/0x610 [snd_usb_audio] [ 69.006247] [<ffffffffa0ea9a70>] snd_usb_pcm_open+0xcd0/0x1280 [snd_usb_audio]

Please merge the fix with the patch that caused the regression.

ok will merge it.

thanks, -- Shuah

...

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index f8d2db3..9497ad1 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -370,6 +370,20 @@ static int au0828_create_media_graph(struct au0828_dev *dev) return 0; }

+void au0828_create_media_graph_notify(struct media_entity *new,

• 		      void *notify_data)
  

+{ +#ifdef CONFIG_MEDIA_CONTROLLER

• struct au0828_dev *dev = (struct au0828_dev *) notify_data;
• int ret;
• ret = au0828_create_media_graph(dev);
• if (ret)

• pr_err("%s() media graph create failed for new entity %s\n",
  

•        __func__, new->name);
  

+#endif +}

static int au0828_enable_source(struct media_entity *entity, struct media_pipeline *pipe) { @@ -535,7 +549,7 @@ static int au0828_media_device_register(struct au0828_dev *dev, } /* register entity_notify callback */ dev->entity_notify.notify_data = (void *) dev;

• dev->entity_notify.notify = (void *) au0828_create_media_graph;

• dev->entity_notify.notify = au0828_create_media_graph_notify; ret = media_device_register_entity_notify(dev->media_dev, &dev->entity_notify); if (ret) {

Em Wed, 6 Jan 2016 13:27:17 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Change au0828_create_media_graph() to create pad link between MEDIA_ENT_F_AUDIO_MIXER entity and decoder's AU8522_PAD_AUDIO_OUT. With mixer entity now linked to decoder, change to link MEDIA_ENT_F_AUDIO_CAPTURE to mixer's source pad.

See my comments about doing only this asynchronously on the previous patches.

Regards, Mauro

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 17 ++++++++++++++--- drivers/media/usb/au0828/au0828.h | 1 + 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 722e073..886fb28 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -264,6 +264,7 @@ static int au0828_create_media_graph(struct au0828_dev *dev) struct media_entity *entity; struct media_entity *tuner = NULL, *decoder = NULL; struct media_entity *audio_capture = NULL;

• struct media_entity *mixer = NULL; int i, ret;

  if (!mdev)

@@ -284,6 +285,9 @@ static int au0828_create_media_graph(struct au0828_dev *dev) case MEDIA_ENT_F_AUDIO_CAPTURE: audio_capture = entity; break;

• case MEDIA_ENT_F_AUDIO_MIXER:
  

• 	mixer = entity;
  

• 	break;
  

  } }

@@ -356,14 +360,21 @@ static int au0828_create_media_graph(struct au0828_dev *dev) } } }

• if (audio_capture && !dev->audio_capture_linked) {

• ret = media_create_pad_link(decoder, AU8522_PAD_AUDIO_OUT,
  

• 			    audio_capture, 0,
  

• if (mixer && audio_capture && !dev->audio_capture_linked) {

• ret = media_create_pad_link(mixer, 1, audio_capture, 0,
  		    MEDIA_LNK_FL_ENABLED);
  

  if (ret) return ret; dev->audio_capture_linked = 1; }
• if (mixer && !dev->mixer_linked) {

• ret = media_create_pad_link(decoder, AU8522_PAD_AUDIO_OUT,
  

• 			    mixer, 0,
  

• 			    MEDIA_LNK_FL_ENABLED);
  

• if (ret)
  

• 	return ret;
  

• dev->mixer_linked = 1;
  

• }

#endif return 0; } diff --git a/drivers/media/usb/au0828/au0828.h b/drivers/media/usb/au0828/au0828.h index 3707664..b9aa74f 100644 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -289,6 +289,7 @@ struct au0828_dev { bool vdev_linked; bool vbi_linked; bool audio_capture_linked;

• bool mixer_linked; struct media_link *active_link; struct media_entity *active_link_owner;

#endif

On 01/28/2016 10:01 AM, Mauro Carvalho Chehab wrote:

Em Wed, 6 Jan 2016 13:27:18 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Add support to track media device unregister in progress state to prevent more than one driver entering unregister. This enables fixing the general protection faults while snd-usb-audio was cleaning up media resources for pcm streams and mixers. In this patch a new interface is added to return the unregister in progress state. Subsequent patches to snd-usb-audio and au0828-core use this interface to avoid entering unregister and attempting to unregister entities and remove devnodes while unregister is in progress. Media device unregister removes entities and interface nodes.

Hmm... isn't the spinlock enough to serialize it? It seems weird the need of an extra bool here to warrant that this is really serialized.

The spinlock and check for media_devnode_is_registered(&mdev->devnode) aren't enough to ensure only one driver enters the unregister. Please note that the devnode isn't marked unregistered until the end in media_device_unregister().

...

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/media-device.c | 5 ++++- include/media/media-device.h | 17 +++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 20c85a9..1bb9a5f 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -749,10 +749,13 @@ void media_device_unregister(struct media_device *mdev) spin_lock(&mdev->lock);

/* Check if mdev was ever registered at all */

• if (!media_devnode_is_registered(&mdev->devnode)) {

• /* check if unregister is in progress */

• if (!media_devnode_is_registered(&mdev->devnode) ||

•    mdev->unregister_in_progress) {
  

  spin_unlock(&mdev->lock); return; }

• mdev->unregister_in_progress = true;

  /* Remove all entities from the media device */ list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)

diff --git a/include/media/media-device.h b/include/media/media-device.h index 04b6c2e..0807292 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -332,6 +332,10 @@ struct media_device { spinlock_t lock; /* Serializes graph operations. */ struct mutex graph_mutex;

• /* Tracks unregister in progress state to prevent

• * more than one driver entering unregister
  

• */

• bool unregister_in_progress;

  /* Handlers to find source entity for the sink entity and

  • check if it is available, and activate the link using

@@ -365,6 +369,7 @@ struct media_device { /* media_devnode to media_device */ #define to_media_device(node) container_of(node, struct media_device, devnode)

/**

• media_entity_enum_init - Initialise an entity enumeration

@@ -553,6 +558,12 @@ struct media_device *media_device_get_devres(struct device *dev);

• @dev: pointer to struct &device.

*/ struct media_device *media_device_find_devres(struct device *dev); +/* return unregister in progress state */ +static inline bool media_device_is_unregister_in_progress(

• 			struct media_device *mdev)
  

+{

• return mdev->unregister_in_progress;

+}

/* Iterate over all entities. */ #define media_device_for_each_entity(entity, mdev) \ @@ -569,6 +580,7 @@ struct media_device *media_device_find_devres(struct device *dev); /* Iterate over all links. */ #define media_device_for_each_link(link, mdev) \ list_for_each_entry(link, &(mdev)->links, graph_obj.list)

#else static inline int media_device_register(struct media_device *mdev) { @@ -604,5 +616,10 @@ static inline struct media_device *media_device_find_devres(struct device *dev) { return NULL; } +static inline bool media_device_is_unregister_in_progress(

• 			struct media_device *mdev)
  

+{

• return false;

+} #endif /* CONFIG_MEDIA_CONTROLLER */ #endif

Em Wed, 6 Jan 2016 13:27:20 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Change au0828_unregister_media_device() to check media device media device unregister is in progress and avoid calling media_device_unregister() and other cleanup done in au0828_unregister_media_device().

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 886fb28..de357a2 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -136,7 +136,9 @@ static void au0828_unregister_media_device(struct au0828_dev *dev)

#ifdef CONFIG_MEDIA_CONTROLLER if (dev->media_dev &&

• media_devnode_is_registered(&dev->media_dev->devnode)) {
  

• media_devnode_is_registered(&dev->media_dev->devnode) &&
  

• !media_device_is_unregister_in_progress(dev->media_dev)) {
  

A kref would likely work better here.

media_device_unregister(dev->media_dev);
media_device_cleanup(dev->media_dev);
dev->media_dev = NULL;

On 01/28/2016 09:45 AM, Mauro Carvalho Chehab wrote:

Em Wed, 6 Jan 2016 14:01:54 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

entity_notify handlers are removed from media_device_unregister(). There is no need to call media_device_unregister_entity_notify() to do that right before calling media_device_unregister().

Please merge with the patch that added it.

ok

...

Signed-off-by: Shuah Khan shuahkh@osg.samsung.com

drivers/media/usb/au0828/au0828-core.c | 2 -- 1 file changed, 2 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 9497ad1..722e073 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -137,8 +137,6 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) #ifdef CONFIG_MEDIA_CONTROLLER if (dev->media_dev && media_devnode_is_registered(&dev->media_dev->devnode)) {

• media_device_unregister_entity_notify(dev->media_dev,
  

• 				      &dev->entity_notify);
  

  media_device_unregister(dev->media_dev); media_device_cleanup(dev->media_dev); dev->media_dev = NULL;

On Wed, 06 Jan 2016 22:05:35 +0100, Shuah Khan wrote:

diff --git a/sound/usb/Makefile b/sound/usb/Makefile index 2d2d122..665fdd9 100644 --- a/sound/usb/Makefile +++ b/sound/usb/Makefile @@ -2,6 +2,18 @@ # Makefile for ALSA #

+# Media Controller +ifeq ($(CONFIG_MEDIA_CONTROLLER),y)

• ifeq ($(CONFIG_MEDIA_SUPPORT),y)

•    KBUILD_CFLAGS += -DUSE_MEDIA_CONTROLLER
  

• endif
• ifeq ($(CONFIG_MEDIA_SUPPORT_MODULE),y)
• ifeq ($(MODULE),y)

•      KBUILD_CFLAGS += -DUSE_MEDIA_CONTROLLER
  

• endif
• endif

+endif

Can't we define this rather via Kconfig? Doing this in Makefile is way too tricky, and it's unclear to users whether MC is actually enabled or not.

diff --git a/sound/usb/media.c b/sound/usb/media.c new file mode 100644 index 0000000..747a66a --- /dev/null +++ b/sound/usb/media.c @@ -0,0 +1,214 @@ +/*

• • media.c - Media Controller specific ALSA driver code
• • Copyright (c) 2015 Shuah Khan shuahkh@osg.samsung.com
• • Copyright (c) 2015 Samsung Electronics Co., Ltd.
• • This file is released under the GPLv2.
• */

+/*

• • This file adds Media Controller support to ALSA driver
• • to use the Media Controller API to share tuner with DVB
• • and V4L2 drivers that control media device. Media device
• • is created based on existing quirks framework. Using this
• • approach, the media controller API usage can be added for
• • a specific device.

+*/

+#include <linux/init.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/string.h> +#include <linux/ctype.h> +#include <linux/usb.h> +#include <linux/moduleparam.h> +#include <linux/mutex.h> +#include <linux/usb/audio.h> +#include <linux/usb/audio-v2.h> +#include <linux/module.h>

+#include <sound/control.h> +#include <sound/core.h> +#include <sound/info.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/initval.h>

+#include "usbaudio.h" +#include "card.h" +#include "midi.h" +#include "mixer.h" +#include "proc.h" +#include "quirks.h" +#include "endpoint.h" +#include "helper.h" +#include "debug.h" +#include "pcm.h" +#include "format.h" +#include "power.h" +#include "stream.h" +#include "media.h"

I believe we can get rid of many include files just for MC support...

+#ifdef USE_MEDIA_CONTROLLER

This ifdef can be removed once if we build this object file conditionally in Makefile.

@@ -1232,7 +1244,10 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction) subs->dsd_dop.channel = 0; subs->dsd_dop.marker = 1;

• return setup_hw_info(runtime, subs);

• ret = setup_hw_info(runtime, subs);
• if (ret == 0)

• ret = media_stream_init(subs, as->pcm, direction);
  

Need to call snd_usb_autosuspend() in the error path.

--- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -544,13 +545,19 @@ int snd_usb_create_quirk(struct snd_usb_audio *chip, [QUIRK_AUDIO_ALIGN_TRANSFER] = create_align_transfer_quirk, [QUIRK_AUDIO_STANDARD_MIXER] = create_standard_mixer_quirk, };

• int ret;

• if (quirk->media_device) {

• /* don't want to fail when media_device_create() fails */
  

• media_device_create(chip, iface);
  

• }

So far, so good...

if (quirk->type < QUIRK_TYPE_COUNT) {

• return quirk_funcs[quirk->type](chip, iface, driver, quirk);
  

• ret = quirk_funcs[quirk->type](chip, iface, driver, quirk);
  

  } else { usb_audio_err(chip, "invalid quirk type %d\n", quirk->type); return -ENXIO; }
• return ret;

Any reason to change this?

Takashi