Em Wed, 03 Feb 2016 21:03:33 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

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

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

drivers/media/media-entity.c | 16 ++++++++++++++++ include/uapi/linux/media.h | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index f2e4360..6179543 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -65,6 +65,22 @@ 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";
  

• case MEDIA_INTF_T_ALSA_SEQUENCER:

• return "sequencer";
  

• case MEDIA_INTF_T_ALSA_TIMER:

• return "timer";
  

  default: return "unknown-intf"; }

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h index c9eb42a..ee020e8 100644 --- a/include/uapi/linux/media.h +++ b/include/uapi/linux/media.h @@ -265,6 +265,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 */

@@ -280,6 +281,27 @@ 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)

+/**

• • DOC: Media Controller Next Generation ALSA Interface Types
• • MEDIA_INTF_T_ALSA_PCM_CAPTURE - PCM Capture Interface (pcm-capture)
• • MEDIA_INTF_T_ALSA_PCM_PLAYBACK - PCM Playback Interface (pcm-playback)
• • MEDIA_INTF_T_ALSA_CONTROL - ALSA Control Interface (alsa-control)
• • MEDIA_INTF_T_ALSA_COMPRESS - ALSA Compression Interface (compress)
• • MEDIA_INTF_T_ALSA_RAWMIDI - ALSA Raw MIDI Interface (rawmidi)
• • MEDIA_INTF_T_ALSA_HWDEP - ALSA Hardware Dependent Interface (hwdep)
• • MEDIA_INTF_T_ALSA_SEQUENCER - ALSA Sequencer (sequencer)
• • MEDIA_INTF_T_ALSA_TIMER - ALSA Timer (timer)
• */

We don't document the userspace API using kernel-doc, as it is too poor for that. Also, we migrated the uAPI documentation from LaTex (at DVB side) and from a separate DocBook document. Migrating those to kernel-doc would need some rich documentation markup language, and someone with lots of spare time.

Instead, we document them at a separate DocBook volume: Documentation/DocBook/media_api.tmpl

The actual DocBook documents are at: Documentation/DocBook/media/dvb - for the DVB side Documentation/DocBook/media/v4l - for V4L2, RC and Media Controller

In the specific case of the Media Controller, the description of those defines are at: Documentation/DocBook/media/v4l/media-types.xml

Just edit it with some text editor and add the new fields there at the right places.

Please test if the documentation is producing the right data, by using the enclosed small script. The extra xmllint lines validate the syntax, helping to identify hidden missing tags. The last line will produce a single html file, instead of one html file per page (with is the default for make htmldocs).

Regards, Mauro

#!/bin/bash LC_ALL=en_US.UTF-8 make cleanmediadocs make DOCBOOKS=media_api.xml htmldocs 2>&1 | grep -v "element.*: validity error : ID .* already defined" xmllint --noent --postvalid "$PWD/Documentation/DocBook/media_api.xml" >/tmp/x.xml 2>/dev/null xmllint --noent --postvalid --noout /tmp/x.xml xmlto html-nochunks -m ./Documentation/DocBook/stylesheet.xsl -o Documentation/DocBook/media Documentation/DocBook/media_api.xml >/dev/null 2>&1

Em Wed, 03 Feb 2016 21:03:34 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Add ALSA Media Controller capture, playback, and mixer function entity defines.

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

include/uapi/linux/media.h | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h index ee020e8..7d50480 100644 --- a/include/uapi/linux/media.h +++ b/include/uapi/linux/media.h @@ -98,6 +98,17 @@ struct media_device_info { #define MEDIA_ENT_F_IF_AUD_DECODER (MEDIA_ENT_F_BASE + 42)

/*

• • DOC: Media Controller Next Generation ALSA Function Entities
• • MEDIA_ENT_F_AUDIO_CAPTURE - Audio Capture Function
• • MEDIA_ENT_F_AUDIO_PLAYBACK - Audio Play Back Function
• • MEDIA_ENT_F_AUDIO_MIXER - Audio Mixer Function

+*/

Wrong way to document it. Please see my comments to patch 01/22.

+#define MEDIA_ENT_F_AUDIO_CAPTURE (MEDIA_ENT_F_BASE + 200) +#define MEDIA_ENT_F_AUDIO_PLAYBACK (MEDIA_ENT_F_BASE + 201) +#define MEDIA_ENT_F_AUDIO_MIXER (MEDIA_ENT_F_BASE + 202)

+/*

• Don't touch on those. The ranges MEDIA_ENT_F_OLD_BASE and
• MEDIA_ENT_F_OLD_SUBDEV_BASE are kept to keep backward compatibility
• with the legacy v1 API.The number range is out of range by purpose:

Em Wed, 03 Feb 2016 21:03:35 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Add new interfaces to register and unregister entity_notify hook to media device. These interfaces allow drivers to add hooks to take appropriate actions when new entities get added to a shared media device. For example, au0828 bridge driver registers an entity_notify hook to create links as needed between media graph nodes.

Shuah,

It seems you didn't address the documentation issues I pointed on the last review. While not ideal, I'll accept, in this specific case, although this requires an extra time for me (as I want to read the documentation *before* actually reviewing the patches), I'm OK if you send the documentation a the end of the series, but please send it *together* with the patch series.

That affect not only me, but others that build documentation and gets lots of errors if something is not documented at the right place.

See below for some notes. I won't be commenting documentation on the rest of this patch series.

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 4d1c13d..1f5d67e 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;

}

@@ -640,6 +648,7 @@ void 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);

@@ -685,11 +694,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 split the comments into a short description and a detailed description, and add blank lines to make it easier to be read by mortals.

The short description appears at the index of the html page: https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/ch06s04.html

So, we want it to be short ;)

The detailed description appears at the end: https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-dvb-creat...

Btw, in the above comment, you don't have actually a detailed description, but, instead, a note. So, the best would be do to it as:

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

The "NOTE: foo" will appear as: https://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/API-media-dev...

P.S.: Don't forget to test if everything is OK with: rm Documentation/DocBook/device-drivers.aux.xml Documentation/DocBook/device-drivers.xml LC_ALL=en_US.UTF-8 make DOCBOOKS=device-drivers.xml htmldocs

The same note applied to the other DocBook tags you added.

Btw, add the documentation *only* at the .h.

+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)
  

+{

• 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;

@@ -706,6 +763,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 d385589..bad8242a 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);

+};

Documentation is missing.

/**

• 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;

Documentation is missing. You should have noticed if you tried to generate the docbook, as it would produce an error like:

include/media/media-device.h:357: warning: No description found for parameter 'entity_notify'

as reported by kbuild test robot.

/* 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 is missing.

/**

• 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)
  

+{ +}

Documentation is missing.

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

Em Wed, 03 Feb 2016 21:03:38 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Add non-locking __media_entity_pipeline_start/stop() interfaces to be called from code paths that hold the graph_mutex. For this change, the media_entity_pipeline_start() routine is renamed to __media_entity_pipeline_start() minus the graph_mutex lock and unlock. media_entity_pipeline_start() now calls the non-locking __media_entity_pipeline_start() holding the graph_lock. The stop interface, media_entity_pipeline_stop() routine is renamed to __media_entity_pipeline_stop() minus the graph_mutex lock and unlock. media_entity_pipeline_stop() now calls the non-locking __media_entity_pipeline_stop() holding the graph_lock.

Please write a better patch descriptions. The above could easily be split into 3 or 4 paragraphs, with would make it easier to read.

Also, too much details, written on a spaghetti style;)

It is obvious by anyone that looked at the patch that "media_entity_pipeline_start() routine is renamed to __media_entity_pipeline_start() minus the graph_mutex lock and unlock."

I read exactly the same thing at the C code, *and* at the subject of the e-mail. Patch subject/description should tell what, why and how (but you should not repeat what can easily be inferred from looking at the diff).

I would describe it as:

Currently, media pipeline graph traversal routines take the graph_mutex lock.

While this works for the current code, we'll need unlocked versions of them because (some explanation *why* you need it, with is the main thing that should be at the description).

So, add non-locking media_entity_pipeline_start/stop() variations.

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

drivers/media/media-entity.c | 34 ++++++++++++++++++++++++---------- include/media/media-entity.h | 12 ++++++++++++ 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c index 9b4d712..b78e4c2 100644 --- a/drivers/media/media-entity.c +++ b/drivers/media/media-entity.c @@ -365,8 +365,8 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_next);

• Pipeline management

*/

-__must_check int media_entity_pipeline_start(struct media_entity *entity,

• 			     struct media_pipeline *pipe)
  

+__must_check int __media_entity_pipeline_start(struct media_entity *entity,

• 			       struct media_pipeline *pipe)
  

{ struct media_device *mdev = entity->graph_obj.mdev; struct media_entity_graph *graph = &pipe->graph; @@ -374,8 +374,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity, struct media_link *link; int ret;

• mutex_lock(&mdev->graph_mutex);
• if (!pipe->streaming_count++) { ret = media_entity_graph_walk_init(&pipe->graph, mdev); if (ret)

@@ -456,8 +454,6 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity, } }

• mutex_unlock(&mdev->graph_mutex);
• return 0;

error: @@ -487,19 +483,28 @@ error_graph_walk_start: if (!--pipe->streaming_count) media_entity_graph_walk_cleanup(graph);

• mutex_unlock(&mdev->graph_mutex);

• return ret;

+} +EXPORT_SYMBOL_GPL(__media_entity_pipeline_start);

+__must_check int media_entity_pipeline_start(struct media_entity *entity,

• 			     struct media_pipeline *pipe)
  

+{

• struct media_device *mdev = entity->graph_obj.mdev;
• int ret;
• mutex_lock(&mdev->graph_mutex);
• ret = __media_entity_pipeline_start(entity, pipe);
• mutex_unlock(&mdev->graph_mutex); return ret;

} EXPORT_SYMBOL_GPL(media_entity_pipeline_start);

-void media_entity_pipeline_stop(struct media_entity *entity) +void __media_entity_pipeline_stop(struct media_entity *entity) {

• struct media_device *mdev = entity->graph_obj.mdev; struct media_entity_graph *graph = &entity->pipe->graph; struct media_pipeline *pipe = entity->pipe;

• mutex_lock(&mdev->graph_mutex);

  WARN_ON(!pipe->streaming_count); media_entity_graph_walk_start(graph, entity);

@@ -516,6 +521,15 @@ void media_entity_pipeline_stop(struct media_entity *entity) if (!--pipe->streaming_count) media_entity_graph_walk_cleanup(graph);

+} +EXPORT_SYMBOL_GPL(__media_entity_pipeline_stop);

+void media_entity_pipeline_stop(struct media_entity *entity) +{

• struct media_device *mdev = entity->graph_obj.mdev;
• mutex_lock(&mdev->graph_mutex);
• __media_entity_pipeline_stop(entity); mutex_unlock(&mdev->graph_mutex);

} EXPORT_SYMBOL_GPL(media_entity_pipeline_stop); diff --git a/include/media/media-entity.h b/include/media/media-entity.h index fe485d3..c7583db 100644 --- a/include/media/media-entity.h +++ b/include/media/media-entity.h @@ -832,6 +832,12 @@ media_entity_graph_walk_next(struct media_entity_graph *graph); */ __must_check int media_entity_pipeline_start(struct media_entity *entity, struct media_pipeline *pipe); +/**

• • non-locking __media_entity_pipeline_start() can be called from
• • code paths that hold the graph_mutex
• */

+__must_check int __media_entity_pipeline_start(struct media_entity *entity,

• 			       struct media_pipeline *pipe);
  
  

/**

• media_entity_pipeline_stop - Mark a pipeline as not streaming

@@ -848,6 +854,12 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity, void media_entity_pipeline_stop(struct media_entity *entity);

/**

• • non-locking __media_entity_pipeline_stop() can be called from
• • code paths that hold the graph_mutex
• */

+void __media_entity_pipeline_stop(struct media_entity *entity);

+/**

• media_devnode_create() - creates and initializes a device node interface
• @mdev: pointer to struct &media_device

Em Wed, 03 Feb 2016 21:03:43 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Change dvb frontend to check if tuner is free when device opened in RW mode. Call to enable_source handler either returns with an active pipeline to tuner or error if tuner is busy. Tuner is released when frontend is released calling the disable_source handler.

This patch seems too early in the series, as I'm not seeing any patch providing a replacement for the removed code yet.

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 03cc508..2b17e8b 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

Why moving this to the non-private part of the data?

};

/**

On 02/04/2016 02:37 AM, Mauro Carvalho Chehab wrote:

Em Wed, 03 Feb 2016 21:03:44 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Remove au0828_enable_analog_tuner() as it is no longer needed because v4l2-core implements common interfaces to check for media source availability.

I didn't see such code at v4l2-core yet. Missing patch?

Removed linux-api from cc:

Please see Patch 10/22 for v4l2-core changes. [PATCH v2 10/22] media: Change v4l-core to check if source is free

thanks, -- Shuah

...

In addition, queue_setup() no longer needs the tuner availability check since v4l2-core does it.

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

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

diff --git a/drivers/media/usb/au0828/au0828-video.c b/drivers/media/usb/au0828/au0828-video.c index 8c54fd2..81952c8 100644 --- a/drivers/media/usb/au0828/au0828-video.c +++ b/drivers/media/usb/au0828/au0828-video.c @@ -638,64 +638,6 @@ static inline int au0828_isoc_copy(struct au0828_dev *dev, struct urb *urb) return rc; }

-static int au0828_enable_analog_tuner(struct au0828_dev *dev) -{ -#ifdef CONFIG_MEDIA_CONTROLLER

• struct media_device *mdev = dev->media_dev;
• struct media_entity *source;
• struct media_link *link, *found_link = NULL;
• int ret, active_links = 0;
• if (!mdev || !dev->decoder)

• return 0;
  

• /*

• * This will find the tuner that is connected into the decoder.
  

• * Technically, this is not 100% correct, as the device may be
  

• * using an analog input instead of the tuner. However, as we can't
  

• * do DVB streaming while the DMA engine is being used for V4L2,
  

• * this should be enough for the actual needs.
  

• */
  

• list_for_each_entry(link, &dev->decoder->links, list) {

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

• 	found_link = link;
  

• 	if (link->flags & MEDIA_LNK_FL_ENABLED)
  

• 		active_links++;
  

• 	break;
  

• }
  

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

• return 0;
  

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

• struct media_entity *sink;
  

• int flags = 0;
  

• sink = link->sink->entity;
  

• if (sink == dev->decoder)
  

• 	flags = MEDIA_LNK_FL_ENABLED;
  

• ret = media_entity_setup_link(link, flags);
  

• if (ret) {
  

• 	pr_err(
  

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

• 		source->name, sink->name,
  

• 		flags ? "enabled" : "disabled",
  

• 		ret);
  

• 	return ret;
  

• } else
  

• 	au0828_isocdbg(
  

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

• 		source->name, sink->name,
  

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

• }

-#endif

• return 0;

-}

static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers, unsigned int *nplanes, unsigned int sizes[], void *alloc_ctxs[]) @@ -707,9 +649,6 @@ static int queue_setup(struct vb2_queue *vq, return sizes[0] < size ? -EINVAL : 0; *nplanes = 1; sizes[0] = size;

• au0828_enable_analog_tuner(dev);
• return 0;

}

-- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html

Em Wed, 03 Feb 2016 21:03:45 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Change au0828 to check if tuner is free or not before changing tuner configuration.

vidioc_g_tuner(), and au0828_v4l2_close() now call v4l-core interface v4l_enable_media_source() before changing tuner configuration.

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 81952c8..8087215 100644 --- a/drivers/media/usb/au0828/au0828-video.c +++ b/drivers/media/usb/au0828/au0828-video.c @@ -1006,8 +1006,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_source(vdev);
  

• if (ret == 0)
  

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

• 			     s_power, 0);
  

Actually, for most USB devices (and this is the case of au0828), the tuner can safely be disabled here if ALSA is not streaming, as, except for some very old USB tuner models (like em28xx-based WinTV USB2) have a separate audio output jack.

Btw, those devices that have a separate audio output jack have analog tuners, like Philips FM1236. Those devices are always on, so the s_power callback are silently ignored.

So, the best logic here would be to disable the tuner if: - ALSA is not streaming; - the last V4L2 file handler is closed.

Additionally, media source can be disabled if the above conditions are met and if the device doesn't have a separate audio out plug (or doesn't use a silicon tuner like xc2028/3028/4000/5000).

Of course, as ALSA may stream anytime, a callback would be needed to enable the media source and power on the tuner, for radio to work.

I understand that the above would require some other non-trivial changes, with would be best addressed on a separate patch series.

So, for now, I would add a FIXME there explaining the above.

dev->std_set_in_tuner_core = 0;

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

@@ -1408,10 +1412,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_source(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, 03 Feb 2016 21:03:46 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Change au0828 to use Managed Media Controller API to share media device and coordinate creating/deleting the shared media device with the snd-usb-audio driver. The shared media device is created as device resource of the parent usb device of the two drivers.

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

drivers/media/usb/au0828/au0828-core.c | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index df2bc3f..b8c4bdd 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -134,10 +134,10 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) {

#ifdef CONFIG_MEDIA_CONTROLLER

• if (dev->media_dev) {

• if (dev->media_dev &&

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

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

• kfree(dev->media_dev);
  

  dev->media_dev = NULL; }

#endif @@ -223,23 +223,24 @@ static int au0828_media_device_init(struct au0828_dev *dev, #ifdef CONFIG_MEDIA_CONTROLLER struct media_device *mdev;

• mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);

• mdev = media_device_get_devres(&udev->dev); if (!mdev) return -ENOMEM;

• mdev->dev = &udev->dev;

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

• mdev->dev = &udev->dev;
  
  

• if (!dev->board.name)

• strlcpy(mdev->model, "unknown au0828", sizeof(mdev->model));
  

• else

• strlcpy(mdev->model, dev->board.name, sizeof(mdev->model));
  

• if (udev->serial)

• strlcpy(mdev->serial, udev->serial, sizeof(mdev->serial));
  

• strcpy(mdev->bus_info, udev->devpath);
• mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
• mdev->driver_version = LINUX_VERSION_CODE;

• if (udev->product)
  

• 	strlcpy(mdev->model, udev->product,
  

• 		sizeof(mdev->model));
  

Why did you change that? On some boards, udev->product doesn't reflect the brand name, but have just some random generic data.

Also, as the other logs associated with the device uses dev->board.name, we want the media controller to use the same name here. Ok, if this is null, we could use udev->product as a replacement.

• if (udev->serial)
  

• 	strlcpy(mdev->serial, udev->serial,
  

• 		sizeof(mdev->serial));
  

• strcpy(mdev->bus_info, udev->devpath);
  

• mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
  
  

• media_device_init(mdev);

• media_device_init(mdev);
  

• }

  dev->media_dev = mdev;

#endif

Em Wed, 03 Feb 2016 21:03:52 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Add enable_source and disable_source handlers. The enable source handler is called from v4l2-core, dvb-core, and ALSA drivers to check if the shared media source is free. The disable source handler is called to release the shared media source.

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

drivers/media/usb/au0828/au0828-core.c | 149 +++++++++++++++++++++++++++++++++ drivers/media/usb/au0828/au0828.h | 3 + 2 files changed, 152 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 4c90f28..fd2265c 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -282,6 +282,7 @@ static int au0828_create_media_graph(struct au0828_dev *dev) return -EINVAL;

if (tuner) {

• dev->tuner = tuner;
  

  /* create tuner to decoder link in deactivated state */ ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT, decoder, 0, 0);

@@ -373,6 +374,150 @@ void au0828_media_graph_notify(struct media_entity *new, void *notify_data) #endif }

+static int au0828_enable_source(struct media_entity *entity,

• 		struct media_pipeline *pipe)
  

+{ +#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;
  

• }

This is wrong. There are devices without tuner (capture devices) and without analog decoder (pure DVB devices).

In the case of pure DVB devices (e. g. no dev->decoder), it should just enable the DVB path.

In the case of devices without tuner, it should use the same logic needed to handle the S-Video/Composite connector inputs.

Btw, I'm not seeing how this logic would do the right thing if the user selects either S-Video or Composite connectors.

• /*

• * 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;
  

• }

Hmm... isn't it to early to activate the pipeline here? My original guess is that, on the analog side, this should happen only at the stream on code. Wouldn't this break apps like mythTV?

• /*

• * 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;
  

• }

Same note as before.

• 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;
  

• }

Most code here looks like the one at au0828_enable_source(). Wouldn't be simpler to merge those code and add a "bool enable" to the function parameters?

+end:

• mutex_unlock(&mdev->graph_mutex);

+#endif +}

static int au0828_media_device_register(struct au0828_dev *dev, struct usb_device *udev) { @@ -403,6 +548,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 54379ec..a7c88a1 100644 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -284,6 +284,9 @@ struct au0828_dev { 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;
• struct media_link *active_link;
• struct media_entity *active_link_owner;

#endif };

On 02/08/2016 03:42 PM, Shuah Khan wrote:

On 02/04/2016 03:26 AM, Mauro Carvalho Chehab wrote:

...

Em Wed, 03 Feb 2016 21:03:52 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Add enable_source and disable_source handlers. The enable source handler is called from v4l2-core, dvb-core, and ALSA drivers to check if the shared media source is free. The disable source handler is called to release the shared media source.

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

drivers/media/usb/au0828/au0828-core.c | 149 +++++++++++++++++++++++++++++++++ drivers/media/usb/au0828/au0828.h | 3 + 2 files changed, 152 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 4c90f28..fd2265c 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -282,6 +282,7 @@ static int au0828_create_media_graph(struct au0828_dev *dev) return -EINVAL;

if (tuner) {

• dev->tuner = tuner;
  

  /* create tuner to decoder link in deactivated state */ ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT, decoder, 0, 0);

@@ -373,6 +374,150 @@ void au0828_media_graph_notify(struct media_entity *new, void *notify_data) #endif }

+static int au0828_enable_source(struct media_entity *entity,

• 		struct media_pipeline *pipe)
  

+{ +#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;
  

• }

This is wrong. There are devices without tuner (capture devices) and without analog decoder (pure DVB devices).

Removed linux-api from the list.

Yes this logic is making an assumption that both decoder and tuner are present. Based on your comment here, is the following check for decoder in au0828_create_media_graph() incorrect? When decoder is null, au0828_usb_probe() bails out. Please see au0828_create_media_graph() return handling in au0828_usb_probe()?

   /* Something bad happened! */
    if (!decoder)
            return -EINVAL;

...

In the case of pure DVB devices (e. g. no dev->decoder), it should just enable the DVB path.

In the case of devices without tuner, it should use the same logic needed to handle the S-Video/Composite connector inputs.

Btw, I'm not seeing how this logic would do the right thing if the user selects either S-Video or Composite connectors.

...

• /*

• * 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;
  

• }

Hmm... isn't it to early to activate the pipeline here? My original guess is that, on the analog side, this should happen only at the stream on code. Wouldn't this break apps like mythTV?

On analog side, there are a few ioctls that change the configuration on the tuner way before stream on step. Is there a reason to separate the setup_link() and pipeline_start() steps? I can separate these two steps, but I am not really seeing the reason for that.

...

...

• /*

• * 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;
  

• }

Same note as before.

Same comment as before here about au0828_create_media_graph() and au0828_usb_probe() handling.

...

...

• 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;
  

• }

Most code here looks like the one at au0828_enable_source(). Wouldn't be simpler to merge those code and add a "bool enable" to the function parameters?

I would rather keep these separate. A very short section is common really.

thanks, -- Shuah

...

...

+end:

• mutex_unlock(&mdev->graph_mutex);

+#endif +}

static int au0828_media_device_register(struct au0828_dev *dev, struct usb_device *udev) { @@ -403,6 +548,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 54379ec..a7c88a1 100644 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -284,6 +284,9 @@ struct au0828_dev { 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;
• struct media_link *active_link;
• struct media_entity *active_link_owner;

#endif };

Em Mon, 08 Feb 2016 15:42:15 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

On 02/04/2016 03:26 AM, Mauro Carvalho Chehab wrote:

...

Em Wed, 03 Feb 2016 21:03:52 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

...

Add enable_source and disable_source handlers. The enable source handler is called from v4l2-core, dvb-core, and ALSA drivers to check if the shared media source is free. The disable source handler is called to release the shared media source.

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

drivers/media/usb/au0828/au0828-core.c | 149 +++++++++++++++++++++++++++++++++ drivers/media/usb/au0828/au0828.h | 3 + 2 files changed, 152 insertions(+)

diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 4c90f28..fd2265c 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -282,6 +282,7 @@ static int au0828_create_media_graph(struct au0828_dev *dev) return -EINVAL;

if (tuner) {

• dev->tuner = tuner;
  

  /* create tuner to decoder link in deactivated state */ ret = media_create_pad_link(tuner, TUNER_PAD_OUTPUT, decoder, 0, 0);

@@ -373,6 +374,150 @@ void au0828_media_graph_notify(struct media_entity *new, void *notify_data) #endif }

+static int au0828_enable_source(struct media_entity *entity,

• 		struct media_pipeline *pipe)
  

+{ +#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;
  

• }

This is wrong. There are devices without tuner (capture devices) and without analog decoder (pure DVB devices).

Removed linux-api from the list.

Yes this logic is making an assumption that both decoder and tuner are present. Based on your comment here, is the following check for decoder in au0828_create_media_graph() incorrect? When decoder is null, au0828_usb_probe() bails out. Please see au0828_create_media_graph() return handling in au0828_usb_probe()?

It seems so. at its current state, au0828 always register a V4L2 node, except if !CONFIG_VIDEO_AU0828_V4L2. So, it is missing a

#ifdef CONFIG_VIDEO_AU0828_V4L2

inside it. I'll write such fixup.

   /* Something bad happened! */
    if (!decoder)
            return -EINVAL;

...

In the case of pure DVB devices (e. g. no dev->decoder), it should just enable the DVB path.

In the case of devices without tuner, it should use the same logic needed to handle the S-Video/Composite connector inputs.

Btw, I'm not seeing how this logic would do the right thing if the user selects either S-Video or Composite connectors.

...

• /*

• * 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;
  

• }

Hmm... isn't it to early to activate the pipeline here? My original guess is that, on the analog side, this should happen only at the stream on code. Wouldn't this break apps like mythTV?

...

• /*

• * 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;
  

• }

Same note as before.

Same comment as before here about au0828_create_media_graph() and au0828_usb_probe() handling.

...

...

• 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;
  

• }

Most code here looks like the one at au0828_enable_source(). Wouldn't be simpler to merge those code and add a "bool enable" to the function parameters?

I would rather keep these separate. A very short section is common really.

thanks, -- Shuah

...

...

+end:

• mutex_unlock(&mdev->graph_mutex);

+#endif +}

static int au0828_media_device_register(struct au0828_dev *dev, struct usb_device *udev) { @@ -403,6 +548,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 54379ec..a7c88a1 100644 --- a/drivers/media/usb/au0828/au0828.h +++ b/drivers/media/usb/au0828/au0828.h @@ -284,6 +284,9 @@ struct au0828_dev { 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;
• struct media_link *active_link;
• struct media_entity *active_link_owner;

#endif };

Em Wed, 03 Feb 2016 21:03:53 -0700 Shuah Khan shuahkh@osg.samsung.com escreveu:

Change ALSA driver to use Media Controller API to share media resources with DVB and V4L2 drivers on a AU0828 media device. Media Controller specific initialization is done after sound card is registered. ALSA creates Media interface and entity function graph nodes for Control, Mixer, PCM Playback, and PCM Capture devices.

snd_usb_hw_params() will call Media Controller enable source handler interface to request the media resource. If resource request is granted, it will release it from snd_usb_hw_free(). If resource is busy, -EBUSY is returned.

Media specific cleanup is done in usb_audio_disconnect().

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

sound/usb/Kconfig | 4 + sound/usb/Makefile | 2 + sound/usb/card.c | 14 +++ sound/usb/card.h | 1 + sound/usb/media.c | 319 +++++++++++++++++++++++++++++++++++++++++++++++ sound/usb/media.h | 72 +++++++++++ sound/usb/mixer.h | 1 + sound/usb/pcm.c | 28 ++++- sound/usb/quirks-table.h | 1 + sound/usb/stream.c | 2 + sound/usb/usbaudio.h | 3 + 11 files changed, 442 insertions(+), 5 deletions(-) create mode 100644 sound/usb/media.c create mode 100644 sound/usb/media.h

diff --git a/sound/usb/Kconfig b/sound/usb/Kconfig index a452ad7..1a71d93 100644 --- a/sound/usb/Kconfig +++ b/sound/usb/Kconfig @@ -15,6 +15,7 @@ config SND_USB_AUDIO select SND_RAWMIDI select SND_PCM select BITREVERSE

• select SND_USB_AUDIO_USE_MEDIA_CONTROLLER if MEDIA_CONTROLLER help Say Y here to include support for USB audio and USB MIDI devices.

@@ -22,6 +23,9 @@ config SND_USB_AUDIO To compile this driver as a module, choose M here: the module will be called snd-usb-audio.

+config SND_USB_AUDIO_USE_MEDIA_CONTROLLER

• bool

config SND_USB_UA101 tristate "Edirol UA-101/UA-1000 driver" select SND_PCM diff --git a/sound/usb/Makefile b/sound/usb/Makefile index 2d2d122..8dca3c4 100644 --- a/sound/usb/Makefile +++ b/sound/usb/Makefile @@ -15,6 +15,8 @@ snd-usb-audio-objs := card.o \ quirks.o \ stream.o

+snd-usb-audio-$(CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER) += media.o

snd-usbmidi-lib-objs := midi.o

# Toplevel Module Dependency diff --git a/sound/usb/card.c b/sound/usb/card.c index 1f09d95..35fe256 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -66,6 +66,7 @@ #include "format.h" #include "power.h" #include "stream.h" +#include "media.h"

MODULE_AUTHOR("Takashi Iwai tiwai@suse.de"); MODULE_DESCRIPTION("USB Audio"); @@ -561,6 +562,11 @@ static int usb_audio_probe(struct usb_interface *intf, if (err < 0) goto __error;

• if (quirk->media_device) {

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

• media_device_create(chip, intf);
  

• }
• usb_chip[chip->index] = chip; chip->num_interfaces++; usb_set_intfdata(intf, chip);

@@ -617,6 +623,14 @@ static void usb_audio_disconnect(struct usb_interface *intf) list_for_each(p, &chip->midi_list) { snd_usbmidi_disconnect(p); }

• /*
  

•  * Nice to check quirk && quirk->media_device
  

•  * need some special handlings. Doesn't look like
  

•  * we have access to quirk here
  

•  * Acceses mixer_list
  

• */
  

• media_device_delete(chip);
  

• /* release mixer resources */ list_for_each_entry(mixer, &chip->mixer_list, list) { snd_usb_mixer_disconnect(mixer);

diff --git a/sound/usb/card.h b/sound/usb/card.h index 71778ca..c15a03c 100644 --- a/sound/usb/card.h +++ b/sound/usb/card.h @@ -156,6 +156,7 @@ struct snd_usb_substream { } dsd_dop;

bool trigger_tstamp_pending_update; /* trigger timestamp being updated from initial estimate */

• void *media_ctl;

};

This is Takashi's call, but I would prefer to avoid using a void * here. Fortunately, GCC is smart enough to handle struct pointers even if the header doesn't have a full declaration of the struct.

So, I would change the above to:

struct media_device;

struct snd_usb_substream { ... struct media_device *media_ctl; };

This way, we ensure strong typecast checks when compiling the code under sound/usb/media.c, while not needing to include media_device.h header here.

struct snd_usb_stream { diff --git a/sound/usb/media.c b/sound/usb/media.c new file mode 100644 index 0000000..8fef48e --- /dev/null +++ b/sound/usb/media.c @@ -0,0 +1,319 @@ +/*

• • 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/mutex.h> +#include <linux/slab.h> +#include <linux/usb.h>

+#include <sound/pcm.h> +#include <sound/core.h>

+#include "usbaudio.h" +#include "card.h" +#include "mixer.h" +#include "media.h"

+static int media_enable_source(struct media_ctl *mctl) +{

• if (mctl && mctl->media_dev->enable_source)

• return mctl->media_dev->enable_source(&mctl->media_entity,
  

• 				      &mctl->media_pipe);
  

• return 0;

+}

+static void media_disable_source(struct media_ctl *mctl) +{

• if (mctl && mctl->media_dev->disable_source)

• mctl->media_dev->disable_source(&mctl->media_entity);
  

+}

+int media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm,

• 	int stream)
  

+{

• struct media_device *mdev;
• struct media_ctl *mctl;
• struct device *pcm_dev = &pcm->streams[stream].dev;
• u32 intf_type;
• int ret = 0;
• u16 mixer_pad;
• struct media_entity *entity;
• mdev = subs->stream->chip->media_dev;
• if (!mdev)

• return -ENODEV;
  

• if (subs->media_ctl)

• return 0;
  

• /* allocate media_ctl */
• mctl = kzalloc(sizeof(*mctl), GFP_KERNEL);
• if (!mctl)

• return -ENOMEM;
  

• mctl->media_dev = mdev;
• if (stream == SNDRV_PCM_STREAM_PLAYBACK) {

• intf_type = MEDIA_INTF_T_ALSA_PCM_PLAYBACK;
  

• mctl->media_entity.function = MEDIA_ENT_F_AUDIO_PLAYBACK;
  

• mctl->media_pad.flags = MEDIA_PAD_FL_SOURCE;
  

• mixer_pad = 1;
  

• } else {

• intf_type = MEDIA_INTF_T_ALSA_PCM_CAPTURE;
  

• mctl->media_entity.function = MEDIA_ENT_F_AUDIO_CAPTURE;
  

• mctl->media_pad.flags = MEDIA_PAD_FL_SINK;
  

• mixer_pad = 2;
  

• }
• mctl->media_entity.name = pcm->name;
• media_entity_pads_init(&mctl->media_entity, 1, &mctl->media_pad);
• ret = media_device_register_entity(mctl->media_dev,

• 			    &mctl->media_entity);
  

• if (ret)

• goto err1;
  

• mctl->intf_devnode = media_devnode_create(mdev, intf_type, 0,

• 				  MAJOR(pcm_dev->devt),
  

• 				  MINOR(pcm_dev->devt));
  

• if (!mctl->intf_devnode) {

• ret = -ENOMEM;
  

• goto err2;
  

• }
• mctl->intf_link = media_create_intf_link(&mctl->media_entity,

• 				 &mctl->intf_devnode->intf,
  

• 				 MEDIA_LNK_FL_ENABLED);
  

• if (!mctl->intf_link) {

• ret = -ENOMEM;
  

• goto err3;
  

• }
• /* create link between mixer and audio */
• media_device_for_each_entity(entity, mdev) {

• switch (entity->function) {
  

• case MEDIA_ENT_F_AUDIO_MIXER:
  

• 	ret = media_create_pad_link(entity, mixer_pad,
  

• 				    &mctl->media_entity, 0,
  

• 				    MEDIA_LNK_FL_ENABLED);
  

• 	if (ret)
  

• 		goto err4;
  

• 	break;
  

• }
  

• }
• subs->media_ctl = mctl;
• return 0;

+err4:

• media_remove_intf_link(mctl->intf_link);

+err3:

• media_devnode_remove(mctl->intf_devnode);

+err2:

• media_device_unregister_entity(&mctl->media_entity);

+err1:

• kfree(mctl);
• return ret;

+}

+void media_stream_delete(struct snd_usb_substream *subs) +{

• struct media_ctl *mctl = subs->media_ctl;
• if (mctl && mctl->media_dev) {

• struct media_device *mdev;
  

• mdev = subs->stream->chip->media_dev;
  

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

• 	media_devnode_remove(mctl->intf_devnode);
  

• 	media_device_unregister_entity(&mctl->media_entity);
  

• 	media_entity_cleanup(&mctl->media_entity);
  

• }
  

• kfree(mctl);
  

• subs->media_ctl = NULL;
  

• }

+}

+int media_start_pipeline(struct snd_usb_substream *subs) +{

• struct media_ctl *mctl = subs->media_ctl;
• if (mctl)

• return media_enable_source(mctl);
  

• return 0;

+}

+void media_stop_pipeline(struct snd_usb_substream *subs) +{

• struct media_ctl *mctl = subs->media_ctl;
• if (mctl)

• media_disable_source(mctl);
  

+}

+int media_mixer_init(struct snd_usb_audio *chip) +{

• struct device *ctl_dev = &chip->card->ctl_dev;
• struct media_intf_devnode *ctl_intf;
• struct usb_mixer_interface *mixer;
• struct media_device *mdev = chip->media_dev;
• struct media_mixer_ctl *mctl;
• u32 intf_type = MEDIA_INTF_T_ALSA_CONTROL;
• int ret;
• if (!mdev)

• return -ENODEV;
  

• ctl_intf = chip->ctl_intf_media_devnode;
• if (!ctl_intf) {

• ctl_intf = (void *) media_devnode_create(mdev,
  

• 					 intf_type, 0,
  

• 					 MAJOR(ctl_dev->devt),
  

• 					 MINOR(ctl_dev->devt));
  

• if (!ctl_intf)
  

• 	return -ENOMEM;
  

• chip->ctl_intf_media_devnode = ctl_intf;
  

• }
• list_for_each_entry(mixer, &chip->mixer_list, list) {

• if (mixer->media_mixer_ctl)
  

• 	continue;
  

• /* allocate media_mixer_ctl */
  

• mctl = kzalloc(sizeof(*mctl), GFP_KERNEL);
  

• if (!mctl)
  

• 	return -ENOMEM;
  

• mctl->media_dev = mdev;
  

• mctl->media_entity.function = MEDIA_ENT_F_AUDIO_MIXER;
  

• mctl->media_entity.name = chip->card->mixername;
  

• mctl->media_pad[0].flags = MEDIA_PAD_FL_SINK;
  

• mctl->media_pad[1].flags = MEDIA_PAD_FL_SOURCE;
  

• mctl->media_pad[2].flags = MEDIA_PAD_FL_SOURCE;
  

• media_entity_pads_init(&mctl->media_entity, MEDIA_MIXER_PAD_MAX,
  

• 		  mctl->media_pad);
  

• ret =  media_device_register_entity(mctl->media_dev,
  

• 				    &mctl->media_entity);
  

• if (ret) {
  

• 	kfree(mctl);
  

• 	return ret;
  

• }
  

• mctl->intf_link = media_create_intf_link(&mctl->media_entity,
  

• 					 &ctl_intf->intf,
  

• 					 MEDIA_LNK_FL_ENABLED);
  

• if (!mctl->intf_link) {
  

• 	media_device_unregister_entity(&mctl->media_entity);
  

• 	media_entity_cleanup(&mctl->media_entity);
  

• 	kfree(mctl);
  

• 	return -ENOMEM;
  

• }
  

• mctl->intf_devnode = ctl_intf;
  

• mixer->media_mixer_ctl = mctl;
  

• }
• return 0;

+}

+static void media_mixer_delete(struct snd_usb_audio *chip) +{

• struct usb_mixer_interface *mixer;
• struct media_device *mdev = chip->media_dev;
• if (!mdev)

• return;
  

• list_for_each_entry(mixer, &chip->mixer_list, list) {

• struct media_mixer_ctl *mctl;
  

• mctl = mixer->media_mixer_ctl;
  

• if (!mixer->media_mixer_ctl)
  

• 	continue;
  

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

• 	media_device_unregister_entity(&mctl->media_entity);
  

• 	media_entity_cleanup(&mctl->media_entity);
  

• }
  

• kfree(mctl);
  

• mixer->media_mixer_ctl = NULL;
  

• }
• if (media_devnode_is_registered(&mdev->devnode))

• media_devnode_remove(chip->ctl_intf_media_devnode);
  

• chip->ctl_intf_media_devnode = NULL;

+}

+int media_device_create(struct snd_usb_audio *chip,

• 	struct usb_interface *iface)
  

+{

• struct media_device *mdev;
• struct usb_device *usbdev = interface_to_usbdev(iface);
• int ret;
• mdev = media_device_get_devres(&usbdev->dev);
• if (!mdev)

• return -ENOMEM;
  

• if (!mdev->dev) {

• /* register media device */
  

• mdev->dev = &usbdev->dev;
  

• if (usbdev->product)
  

• 	strlcpy(mdev->model, usbdev->product,
  

• 		sizeof(mdev->model));
  

• if (usbdev->serial)
  

• 	strlcpy(mdev->serial, usbdev->serial,
  

• 		sizeof(mdev->serial));
  

• strcpy(mdev->bus_info, usbdev->devpath);
  

• mdev->hw_revision = le16_to_cpu(usbdev->descriptor.bcdDevice);
  

• media_device_init(mdev);
  

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

• ret = media_device_register(mdev);
  

• if (ret) {
  

• 	dev_err(&usbdev->dev,
  

• 		"Couldn't register media device. Error: %d\n",
  

• 		ret);
  

• 	return ret;
  

• }
  

• }
• /* save media device - avoid lookups */
• chip->media_dev = mdev;
• /* Create media entities for mixer and control dev */
• ret = media_mixer_init(chip);
• if (ret) {

• dev_err(&usbdev->dev,
  

• 	"Couldn't create media mixer entities. Error: %d\n",
  

• 	ret);
  

• /* clear saved media_dev */
  

• chip->media_dev = NULL;
  

• return ret;
  

• }
• return 0;

+}

+void media_device_delete(struct snd_usb_audio *chip) +{

• struct media_device *mdev = chip->media_dev;
• media_mixer_delete(chip);
• if (mdev) {

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

• 	media_device_unregister(mdev);
  

• chip->media_dev = NULL;
  

• }

+} diff --git a/sound/usb/media.h b/sound/usb/media.h new file mode 100644 index 0000000..b5f451a --- /dev/null +++ b/sound/usb/media.h @@ -0,0 +1,72 @@ +/*

• • media.h - 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.

+*/ +#ifndef __MEDIA_H

+#ifdef CONFIG_SND_USB_AUDIO_USE_MEDIA_CONTROLLER

+#include <media/media-device.h> +#include <media/media-entity.h> +#include <sound/asound.h>

+struct media_ctl {

• struct media_device *media_dev;
• struct media_entity media_entity;
• struct media_intf_devnode *intf_devnode;
• struct media_link *intf_link;
• struct media_pad media_pad;
• struct media_pipeline media_pipe;

+};

+/*

• • One source pad each for SNDRV_PCM_STREAM_CAPTURE and
• • SNDRV_PCM_STREAM_PLAYBACK. One for sink pad to link
• • to AUDIO Source

+*/ +#define MEDIA_MIXER_PAD_MAX (SNDRV_PCM_STREAM_LAST + 2)

+struct media_mixer_ctl {

• struct media_device *media_dev;
• struct media_entity media_entity;
• struct media_intf_devnode *intf_devnode;
• struct media_link *intf_link;
• struct media_pad media_pad[MEDIA_MIXER_PAD_MAX];
• struct media_pipeline media_pipe;

+};

+int media_device_create(struct snd_usb_audio *chip,

• 	struct usb_interface *iface);
  

+void media_device_delete(struct snd_usb_audio *chip); +int media_stream_init(struct snd_usb_substream *subs, struct snd_pcm *pcm,

• 	int stream);
  

+void media_stream_delete(struct snd_usb_substream *subs); +int media_start_pipeline(struct snd_usb_substream *subs); +void media_stop_pipeline(struct snd_usb_substream *subs); +#else +static inline int media_device_create(struct snd_usb_audio *chip,

• 		      struct usb_interface *iface)
  

• 				{ return 0; }
  

+static inline void media_device_delete(struct snd_usb_audio *chip) { } +static inline int media_stream_init(struct snd_usb_substream *subs,

• 			struct snd_pcm *pcm, int stream)
  

• 				{ return 0; }
  

+static inline void media_stream_delete(struct snd_usb_substream *subs) { } +static inline int media_start_pipeline(struct snd_usb_substream *subs)

• 			{ return 0; }
  

+static inline void media_stop_pipeline(struct snd_usb_substream *subs) { } +#endif +#endif /* __MEDIA_H */ diff --git a/sound/usb/mixer.h b/sound/usb/mixer.h index 3417ef3..787b352 100644 --- a/sound/usb/mixer.h +++ b/sound/usb/mixer.h @@ -22,6 +22,7 @@ struct usb_mixer_interface { struct urb *rc_urb; struct usb_ctrlrequest *rc_setup_packet; u8 rc_buffer[6];

• void *media_mixer_ctl;

};

#define MAX_CHANNELS 16 /* max logical channels */ diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c index 9245f52..c3b8486 100644 --- a/sound/usb/pcm.c +++ b/sound/usb/pcm.c @@ -35,6 +35,7 @@ #include "pcm.h" #include "clock.h" #include "power.h" +#include "media.h"

#define SUBSTREAM_FLAG_DATA_EP_STARTED 0 #define SUBSTREAM_FLAG_SYNC_EP_STARTED 1 @@ -715,10 +716,14 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, struct audioformat *fmt; int ret;

• ret = media_start_pipeline(subs);
• if (ret)

• return ret;
  

• ret = snd_pcm_lib_alloc_vmalloc_buffer(substream, params_buffer_bytes(hw_params)); if (ret < 0)

• return ret;
  

• goto err_ret;
  
  

  subs->pcm_format = params_format(hw_params); subs->period_bytes = params_period_bytes(hw_params);

@@ -732,22 +737,27 @@ static int snd_usb_hw_params(struct snd_pcm_substream *substream, dev_dbg(&subs->dev->dev, "cannot set format: format = %#x, rate = %d, channels = %d\n", subs->pcm_format, subs->cur_rate, subs->channels);

• return -EINVAL;
  

• ret = -EINVAL;
  

• goto err_ret;
  

  }

  ret = snd_usb_lock_shutdown(subs->stream->chip); if (ret < 0)

• return ret;
  

• goto err_ret;
  

  ret = set_format(subs, fmt); snd_usb_unlock_shutdown(subs->stream->chip); if (ret < 0)

• return ret;
  

• goto err_ret;
  
  

  subs->interface = fmt->iface; subs->altset_idx = fmt->altset_idx; subs->need_setup_ep = true;

  return 0;

+err_ret:

• media_stop_pipeline(subs);
• return ret;

}

/* @@ -759,6 +769,7 @@ static int snd_usb_hw_free(struct snd_pcm_substream *substream) { struct snd_usb_substream *subs = substream->runtime->private_data;

• media_stop_pipeline(subs); subs->cur_audiofmt = NULL; subs->cur_rate = 0; subs->period_bytes = 0;

@@ -1219,6 +1230,7 @@ static int snd_usb_pcm_open(struct snd_pcm_substream *substream, int direction) struct snd_usb_stream *as = snd_pcm_substream_chip(substream); struct snd_pcm_runtime *runtime = substream->runtime; struct snd_usb_substream *subs = &as->substream[direction];

• int ret;

  subs->interface = -1; subs->altset_idx = 0;

@@ -1232,7 +1244,12 @@ 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);
  

• if (ret)

• snd_usb_autosuspend(subs->stream->chip);
  

• return ret;

}

static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) @@ -1241,6 +1258,7 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction) struct snd_usb_substream *subs = &as->substream[direction];

stop_endpoints(subs, true);

• media_stop_pipeline(subs);

  if (subs->interface >= 0 && !snd_usb_lock_shutdown(subs->stream->chip)) {

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h index c60a776..9d087b1 100644 --- a/sound/usb/quirks-table.h +++ b/sound/usb/quirks-table.h @@ -2886,6 +2886,7 @@ YAMAHA_DEVICE(0x7010, "UB99"), .product_name = pname, \ .ifnum = QUIRK_ANY_INTERFACE, \ .type = QUIRK_AUDIO_ALIGN_TRANSFER, \

• .media_device = 1, \
  

  } \

}

diff --git a/sound/usb/stream.c b/sound/usb/stream.c index c4dc577..6c7f5c1 100644 --- a/sound/usb/stream.c +++ b/sound/usb/stream.c @@ -36,6 +36,7 @@ #include "format.h" #include "clock.h" #include "stream.h" +#include "media.h"

/*

• free a substream

@@ -52,6 +53,7 @@ static void free_substream(struct snd_usb_substream *subs) kfree(fp); } kfree(subs->rate_list.list);

• media_stream_delete(subs);

}

diff --git a/sound/usb/usbaudio.h b/sound/usb/usbaudio.h index b665d85..0c0a86c 100644 --- a/sound/usb/usbaudio.h +++ b/sound/usb/usbaudio.h @@ -60,6 +60,8 @@ struct snd_usb_audio { bool autoclock; /* from the 'autoclock' module param */

struct usb_host_interface *ctrl_intf; /* the audio control interface */

• void *media_dev;
• void *ctl_intf_media_devnode;

};

#define usb_audio_err(chip, fmt, args...) \ @@ -110,6 +112,7 @@ struct snd_usb_audio_quirk { const char *product_name; int16_t ifnum; uint16_t type;

• bool media_device; const void *data;

};