[PATCH 00/31] Sharing media resources across ALSA and au0828 drivers

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

Em Wed,  6 Jan 2016 13:26:50 -0700
Shuah Khan [off-list ref] escreveu:

quoted

From: Mauro Carvalho Chehab <redacted>

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

Signed-off-by: Mauro Carvalho Chehab <redacted>
---
 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.

thanks,
-- Shuah

quoted

+
 /*
  * MC next gen API definitions
  *

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org | (970) 217-8978

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

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

quoted

Em Wed,  6 Jan 2016 13:26:50 -0700
Shuah Khan [off-list ref] escreveu:

quoted

From: Mauro Carvalho Chehab <redacted>

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

Signed-off-by: Mauro Carvalho Chehab <redacted>
---
 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 02/01/2016 09:18 AM, Takashi Iwai wrote:

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

quoted

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

quoted

Em Wed,  6 Jan 2016 13:26:50 -0700
Shuah Khan [off-list ref] escreveu:

quoted

From: Mauro Carvalho Chehab <redacted>

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

Signed-off-by: Mauro Carvalho Chehab <redacted>
---
 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?

Looks like SEQUENCER and TIMER are missing in
the MC defines. I can see how SEQUENCER could
be relevant as an MC node, however not sure
about TIMER use-case. That said, I don't see
any reason for not including it.

I will add Media Interface Types for both
of these missing ones.

thanks,
-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

On 01/28/2016 01:42 PM, Shuah Khan wrote:

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

quoted

Em Thu, 28 Jan 2016 10:04:24 -0700
Shuah Khan [off-list ref] escreveu:

quoted

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

quoted

Em Wed,  6 Jan 2016 13:27:18 -0700
Shuah Khan [off-list ref] escreveu:
  

quoted

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().

I guess the call to:
	device_remove_file(&mdev->devnode.dev, &dev_attr_model);

IMO, This should be, instead, at media_devnode_unregister().

Then, we can change the logic at media_devnode_unregister() to:

void media_devnode_unregister(struct media_devnode *mdev)
{
	mutex_lock(&media_devnode_lock);

	/* Check if mdev was ever registered at all */
	if (!media_devnode_is_registered(mdev)) {
		mutex_unlock(&media_devnode_lock);
		return;
	}

	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
	mutex_unlock(&media_devnode_lock);
	device_remove_file(&mdev->devnode.dev, &dev_attr_model);
	device_unregister(&mdev->dev);
}

This sounds enough to avoid device_unregister() or device_remove_file()
to be called twice.

I can give it a try. There might other problems that could
result from media device being a devres in this case. The
last put_device on the usbdev parent device (media device
is created as devres for this), all device resources get
released. That might have to be solved in a different way.

For now I will see if your solution works.

Hi Mauro,

Moving device_remove_file() won't be easy without
making more changes. The file is created in
media_device_regsiter() and all the attributes are
handled in media-device.c

One solution I can think of is clearing the
MEDIA_FLAG_REGISTERED bit very early in
media_device_unregister()

--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c

@@ -759,6 +759,9 @@ void media_device_unregister(struct media_device *mdev)
                return;
        }
 
+       /* Protect unregister path - clear MEDIA_FLAG_REGISTERED */
+       clear_bit(MEDIA_FLAG_REGISTERED, &mdev->devnode.flags);
+
        /* Remove all entities from the media device */
        list_for_each_entry_safe(entity, next, &mdev->entities, graph_obj.list)
                __media_device_unregister_entity(entity);

and changing media_devnode_unregister() to simply call
device_unregister(&mdev->dev);

Again clearing MEDIA_FLAG_REGISTERED bit in
media_device_unregister() some problems.
For one thing clearing this bit should be
done holding media_devnode_lock. It can be
done cleanly if we do the following:

How about if we split media_devnode_unregister()
into twp ohases:

media_devnode_start_unregister()
to clear this bit. It can do:

media_devnode_start_unregister()
{
	mutex_lock(&media_devnode_lock);
	if (!media_devnode_is_registered(mdev)) {
		mutex_unlock(&media_devnode_lock);
		return;
	}
	clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
	mutex_unlock(&media_devnode_lock);
}

then:media_device_unregister(struct media_device *mdev)
will call this first thing and then hold mdev->lock
do the rest and the call media_devnode_unregister()
and which will be changed to as follows:

--- a/drivers/media/media-devnode.c
+++ b/drivers/media/media-devnode.c

@@ -274,13 +274,6 @@ error:
 
 void media_devnode_unregister(struct media_devnode *mdev)
 {
-       /* Check if mdev was ever registered at all */
-       if (!media_devnode_is_registered(mdev))
-               return;
-
-       mutex_lock(&media_devnode_lock);
-       clear_bit(MEDIA_FLAG_REGISTERED, &mdev->flags);
-       mutex_unlock(&media_devnode_lock);
        device_unregister(&mdev->dev);
 }

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978