Re: [PATCH v3 06/22] media: Media Controller enable/disable source handler API
Hi Shuah, I see that the patches have been merged to media-tree.git master and subsequently a pull request has been sent to Linus including them, but the remaining issues still need to be fixed. Some matters should be discussed further on the list. On Mon, Mar 14, 2016 at 09:22:12AM -0600, Shuah Khan wrote: > On 03/13/2016 02:11 PM, Sakari Ailus wrote: > > Hi Shuah, > > > > On Thu, Mar 10, 2016 at 07:29:59AM -0700, Shuah Khan wrote: > >> On 03/10/2016 12:35 AM, Sakari Ailus wrote: > >>> Hi Shuah, > >>> > >>> On Thu, Feb 11, 2016 at 04:41:22PM -0700, Shuah Khan wrote: > 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 used to run these handlers. The enable_source handler > finds source entity for the passed in entity and checks if it is > available. When link is found, it activates it. Disable source > handler deactivates the link. > > Bridge driver is expected to implement and set these handlers. > > Signed-off-by: Shuah Khan > --- > include/media/media-device.h | 30 ++ > 1 file changed, 30 insertions(+) > > diff --git a/include/media/media-device.h b/include/media/media-device.h > index 075a482..1a04644 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -302,6 +302,11 @@ struct media_entity_notify { > * @entity_notify: List of registered entity_notify callbacks > * @lock: Entities list lock > * @graph_mutex: Entities graph operation lock > + * > + * @source_priv: Driver Private data for enable/disable source handlers > + * @enable_source: Enable Source Handler function pointer > + * @disable_source: Disable Source Handler function pointer > + * > * @link_notify: Link state change notification callback > * > * This structure represents an abstract high-level media device. It > allows easy > @@ -313,6 +318,26 @@ struct media_entity_notify { > * > * @model is a descriptive model name exported through sysfs. It > doesn't have to > * be unique. > + * > + * @enable_source is a handler to find source entity for the > + * sink entity and activate the link between them if source > + * entity is free. Drivers should call this handler before > + * accessing the source. > + * > + * @disable_source is a handler to find source entity for the > + * sink entity and deactivate the link between them. Drivers > + * should call this handler to release the source. > + * > >>> > >>> Is there a particular reason you're not simply (de)activating the link, > >>> but > >>> instead add a new callback? > >> > >> These two handlers are separate for a couple of reasons: > >> > >> 1. Explicit and symmetric API is easier to use and maintain. > >>Similar what we do in other cases, register/unregister > >>get/put etc. > > > > Link state is set explicitly (enabled or disabled). This is certainly not a > > reason to create a redundant API for link setup. > > > >> 2. This is more important. Disable handler makes sure the > >>owner is releasing the resource. Otherwise, when some > >>other application does enable, the owner could loose > >>the resource, if enable and disable are the same. > >> > >>e.g: Video app is holding the resource, DVB app does > >>enable. Disable handler makes sure Video/owner is the one > >>that is asking to do the release. > > > > Based on the later patches in this set, the enable_source() callback of the > > au0828 driver performs three things: > > > > - Find the source entity, > > - Enable the link from some au0828 entity to the source and > > - Start the pipeline that begins from the I/O device node. The pipe object > > is embedded in struct video_device. > > > > disable_source() undoes this in reverse order. > > > > That's in the au0828 driver and rightly so. > > > > Then it gets murkier. enable_source() and disable_source() callbacks > > (through a few turns) will get called from v4l2-ioctl.c functions > > v4l_querycap, v4l_s_fmt, v4l_s_frequency, v4l_s_std and v4l_querystd. This > > is also performed in VB2 core vb2_core_streamon() function. > > > > I certainly have no objections when it comes to blocking other processes > > from setting the format when a process holding a file handle to a device has > > e.g. set the format. The implementation is another matter. > > > > What particularly does concern me in this patchset is: > > > > - struct media_pipe is intended to be allocated by drivers embedded in > > another struct holding information the driver needs related to the > > pipeline. Moving this struct to struct video_device prevents this, which > > translates to v4l_enable_media_source() and v4l_disable_media_source() > > functions the patch
Re: [PATCH v3 06/22] media: Media Controller enable/disable source handler API
On 03/13/2016 02:11 PM, Sakari Ailus wrote: > Hi Shuah, > > On Thu, Mar 10, 2016 at 07:29:59AM -0700, Shuah Khan wrote: >> On 03/10/2016 12:35 AM, Sakari Ailus wrote: >>> Hi Shuah, >>> >>> On Thu, Feb 11, 2016 at 04:41:22PM -0700, Shuah Khan wrote: 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 used to run these handlers. The enable_source handler finds source entity for the passed in entity and checks if it is available. When link is found, it activates it. Disable source handler deactivates the link. Bridge driver is expected to implement and set these handlers. Signed-off-by: Shuah Khan --- include/media/media-device.h | 30 ++ 1 file changed, 30 insertions(+) diff --git a/include/media/media-device.h b/include/media/media-device.h index 075a482..1a04644 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -302,6 +302,11 @@ struct media_entity_notify { * @entity_notify: List of registered entity_notify callbacks * @lock: Entities list lock * @graph_mutex: Entities graph operation lock + * + * @source_priv: Driver Private data for enable/disable source handlers + * @enable_source: Enable Source Handler function pointer + * @disable_source: Disable Source Handler function pointer + * * @link_notify: Link state change notification callback * * This structure represents an abstract high-level media device. It allows easy @@ -313,6 +318,26 @@ struct media_entity_notify { * * @model is a descriptive model name exported through sysfs. It doesn't have to * be unique. + * + * @enable_source is a handler to find source entity for the + * sink entity and activate the link between them if source + * entity is free. Drivers should call this handler before + * accessing the source. + * + * @disable_source is a handler to find source entity for the + * sink entity and deactivate the link between them. Drivers + * should call this handler to release the source. + * >>> >>> Is there a particular reason you're not simply (de)activating the link, but >>> instead add a new callback? >> >> These two handlers are separate for a couple of reasons: >> >> 1. Explicit and symmetric API is easier to use and maintain. >>Similar what we do in other cases, register/unregister >>get/put etc. > > Link state is set explicitly (enabled or disabled). This is certainly not a > reason to create a redundant API for link setup. > >> 2. This is more important. Disable handler makes sure the >>owner is releasing the resource. Otherwise, when some >>other application does enable, the owner could loose >>the resource, if enable and disable are the same. >> >>e.g: Video app is holding the resource, DVB app does >>enable. Disable handler makes sure Video/owner is the one >>that is asking to do the release. > > Based on the later patches in this set, the enable_source() callback of the > au0828 driver performs three things: > > - Find the source entity, > - Enable the link from some au0828 entity to the source and > - Start the pipeline that begins from the I/O device node. The pipe object > is embedded in struct video_device. > > disable_source() undoes this in reverse order. > > That's in the au0828 driver and rightly so. > > Then it gets murkier. enable_source() and disable_source() callbacks > (through a few turns) will get called from v4l2-ioctl.c functions > v4l_querycap, v4l_s_fmt, v4l_s_frequency, v4l_s_std and v4l_querystd. This > is also performed in VB2 core vb2_core_streamon() function. > > I certainly have no objections when it comes to blocking other processes > from setting the format when a process holding a file handle to a device has > e.g. set the format. The implementation is another matter. > > What particularly does concern me in this patchset is: > > - struct media_pipe is intended to be allocated by drivers embedded in > another struct holding information the driver needs related to the > pipeline. Moving this struct to struct video_device prevents this, which > translates to v4l_enable_media_source() and v4l_disable_media_source() > functions the patchset adds being specific to the au0828 driver. They > should be part of that driver, and may not be part of the V4L2 core. > struct media_pipe may not be added to struct video_device for the same > reason. This media_pipe is associated with struct video_device though. I don't understand your concern. I am viewing this media_pipe as part of the registered video_device. video_device struct is in the au0828 device and gets registered as a whole including the media_pipe > > - The IOCTL handlers in v4l2-ioctl.c already cal
Re: [PATCH v3 06/22] media: Media Controller enable/disable source handler API
Hi Shuah, On Thu, Mar 10, 2016 at 07:29:59AM -0700, Shuah Khan wrote: > On 03/10/2016 12:35 AM, Sakari Ailus wrote: > > Hi Shuah, > > > > On Thu, Feb 11, 2016 at 04:41:22PM -0700, Shuah Khan wrote: > >> 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 used to run these handlers. The enable_source handler > >> finds source entity for the passed in entity and checks if it is > >> available. When link is found, it activates it. Disable source > >> handler deactivates the link. > >> > >> Bridge driver is expected to implement and set these handlers. > >> > >> Signed-off-by: Shuah Khan > >> --- > >> include/media/media-device.h | 30 ++ > >> 1 file changed, 30 insertions(+) > >> > >> diff --git a/include/media/media-device.h b/include/media/media-device.h > >> index 075a482..1a04644 100644 > >> --- a/include/media/media-device.h > >> +++ b/include/media/media-device.h > >> @@ -302,6 +302,11 @@ struct media_entity_notify { > >> * @entity_notify: List of registered entity_notify callbacks > >> * @lock: Entities list lock > >> * @graph_mutex: Entities graph operation lock > >> + * > >> + * @source_priv: Driver Private data for enable/disable source handlers > >> + * @enable_source: Enable Source Handler function pointer > >> + * @disable_source: Disable Source Handler function pointer > >> + * > >> * @link_notify: Link state change notification callback > >> * > >> * This structure represents an abstract high-level media device. It > >> allows easy > >> @@ -313,6 +318,26 @@ struct media_entity_notify { > >> * > >> * @model is a descriptive model name exported through sysfs. It doesn't > >> have to > >> * be unique. > >> + * > >> + * @enable_source is a handler to find source entity for the > >> + * sink entity and activate the link between them if source > >> + * entity is free. Drivers should call this handler before > >> + * accessing the source. > >> + * > >> + * @disable_source is a handler to find source entity for the > >> + * sink entity and deactivate the link between them. Drivers > >> + * should call this handler to release the source. > >> + * > > > > Is there a particular reason you're not simply (de)activating the link, but > > instead add a new callback? > > These two handlers are separate for a couple of reasons: > > 1. Explicit and symmetric API is easier to use and maintain. >Similar what we do in other cases, register/unregister >get/put etc. Link state is set explicitly (enabled or disabled). This is certainly not a reason to create a redundant API for link setup. > 2. This is more important. Disable handler makes sure the >owner is releasing the resource. Otherwise, when some >other application does enable, the owner could loose >the resource, if enable and disable are the same. > >e.g: Video app is holding the resource, DVB app does >enable. Disable handler makes sure Video/owner is the one >that is asking to do the release. Based on the later patches in this set, the enable_source() callback of the au0828 driver performs three things: - Find the source entity, - Enable the link from some au0828 entity to the source and - Start the pipeline that begins from the I/O device node. The pipe object is embedded in struct video_device. disable_source() undoes this in reverse order. That's in the au0828 driver and rightly so. Then it gets murkier. enable_source() and disable_source() callbacks (through a few turns) will get called from v4l2-ioctl.c functions v4l_querycap, v4l_s_fmt, v4l_s_frequency, v4l_s_std and v4l_querystd. This is also performed in VB2 core vb2_core_streamon() function. I certainly have no objections when it comes to blocking other processes from setting the format when a process holding a file handle to a device has e.g. set the format. The implementation is another matter. What particularly does concern me in this patchset is: - struct media_pipe is intended to be allocated by drivers embedded in another struct holding information the driver needs related to the pipeline. Moving this struct to struct video_device prevents this, which translates to v4l_enable_media_source() and v4l_disable_media_source() functions the patchset adds being specific to the au0828 driver. They should be part of that driver, and may not be part of the V4L2 core. struct media_pipe may not be added to struct video_device for the same reason. - The IOCTL handlers in v4l2-ioctl.c already call driver-settable callbacks before this set. It looks like redundant callbacks are added there as ell. The same appears to be true for the VB2 callback. Could you use the existing callbacks instead of creating new ones? As a short term solution, I propose moving the code to the au0828 driver. Once it is there, we can see whether it could be made more generic if there is a need for it elsewhere.
Re: [PATCH v3 06/22] media: Media Controller enable/disable source handler API
On 03/10/2016 12:35 AM, Sakari Ailus wrote: > Hi Shuah, > > On Thu, Feb 11, 2016 at 04:41:22PM -0700, Shuah Khan wrote: >> 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 used to run these handlers. The enable_source handler >> finds source entity for the passed in entity and checks if it is >> available. When link is found, it activates it. Disable source >> handler deactivates the link. >> >> Bridge driver is expected to implement and set these handlers. >> >> Signed-off-by: Shuah Khan >> --- >> include/media/media-device.h | 30 ++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/include/media/media-device.h b/include/media/media-device.h >> index 075a482..1a04644 100644 >> --- a/include/media/media-device.h >> +++ b/include/media/media-device.h >> @@ -302,6 +302,11 @@ struct media_entity_notify { >> * @entity_notify: List of registered entity_notify callbacks >> * @lock: Entities list lock >> * @graph_mutex: Entities graph operation lock >> + * >> + * @source_priv: Driver Private data for enable/disable source handlers >> + * @enable_source: Enable Source Handler function pointer >> + * @disable_source: Disable Source Handler function pointer >> + * >> * @link_notify: Link state change notification callback >> * >> * This structure represents an abstract high-level media device. It allows >> easy >> @@ -313,6 +318,26 @@ struct media_entity_notify { >> * >> * @model is a descriptive model name exported through sysfs. It doesn't >> have to >> * be unique. >> + * >> + * @enable_source is a handler to find source entity for the >> + * sink entity and activate the link between them if source >> + * entity is free. Drivers should call this handler before >> + * accessing the source. >> + * >> + * @disable_source is a handler to find source entity for the >> + * sink entity and deactivate the link between them. Drivers >> + * should call this handler to release the source. >> + * > > Is there a particular reason you're not simply (de)activating the link, but > instead add a new callback? These two handlers are separate for a couple of reasons: 1. Explicit and symmetric API is easier to use and maintain. Similar what we do in other cases, register/unregister get/put etc. 2. This is more important. Disable handler makes sure the owner is releasing the resource. Otherwise, when some other application does enable, the owner could loose the resource, if enable and disable are the same. e.g: Video app is holding the resource, DVB app does enable. Disable handler makes sure Video/owner is the one that is asking to do the release. thanks, -- Shuah > >> + * Note: 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 >> + * the link between them from enable_source and deactivate >> + * from disable_source. >> */ >> struct media_device { >> /* dev->driver_data points to this struct. */ >> @@ -344,6 +369,11 @@ struct media_device { >> /* Serializes graph operations. */ >> struct mutex graph_mutex; >> >> +void *source_priv; >> +int (*enable_source)(struct media_entity *entity, >> + struct media_pipeline *pipe); >> +void (*disable_source)(struct media_entity *entity); >> + >> int (*link_notify)(struct media_link *link, u32 flags, >> unsigned int notification); >> }; > -- Shuah Khan Sr. Linux Kernel Developer Open Source Innovation Group Samsung Research America (Silicon Valley) shua...@osg.samsung.com | (970) 217-8978 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 06/22] media: Media Controller enable/disable source handler API
Hi Shuah, On Thu, Feb 11, 2016 at 04:41:22PM -0700, Shuah Khan wrote: > 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 used to run these handlers. The enable_source handler > finds source entity for the passed in entity and checks if it is > available. When link is found, it activates it. Disable source > handler deactivates the link. > > Bridge driver is expected to implement and set these handlers. > > Signed-off-by: Shuah Khan > --- > include/media/media-device.h | 30 ++ > 1 file changed, 30 insertions(+) > > diff --git a/include/media/media-device.h b/include/media/media-device.h > index 075a482..1a04644 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -302,6 +302,11 @@ struct media_entity_notify { > * @entity_notify: List of registered entity_notify callbacks > * @lock:Entities list lock > * @graph_mutex: Entities graph operation lock > + * > + * @source_priv: Driver Private data for enable/disable source handlers > + * @enable_source: Enable Source Handler function pointer > + * @disable_source: Disable Source Handler function pointer > + * > * @link_notify: Link state change notification callback > * > * This structure represents an abstract high-level media device. It allows > easy > @@ -313,6 +318,26 @@ struct media_entity_notify { > * > * @model is a descriptive model name exported through sysfs. It doesn't > have to > * be unique. > + * > + * @enable_source is a handler to find source entity for the > + * sink entity and activate the link between them if source > + * entity is free. Drivers should call this handler before > + * accessing the source. > + * > + * @disable_source is a handler to find source entity for the > + * sink entity and deactivate the link between them. Drivers > + * should call this handler to release the source. > + * Is there a particular reason you're not simply (de)activating the link, but instead add a new callback? > + * Note: 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 > + * the link between them from enable_source and deactivate > + * from disable_source. > */ > struct media_device { > /* dev->driver_data points to this struct. */ > @@ -344,6 +369,11 @@ struct media_device { > /* Serializes graph operations. */ > struct mutex graph_mutex; > > + void *source_priv; > + int (*enable_source)(struct media_entity *entity, > + struct media_pipeline *pipe); > + void (*disable_source)(struct media_entity *entity); > + > int (*link_notify)(struct media_link *link, u32 flags, > unsigned int notification); > }; -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html