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 <shuahkh@xxxxxxxxxxxxxxx>
>>>> ---
>>>> 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 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?
If I understand correctly, you are suggesting that the calls to enable
and disable source should be made from the au0828 hooks. e.g vidioc_s_std()
Calling them from v4l2-core makes it generic and works on all drivers
without driver changes. That is the rationale for making adding v4l2
common interfaces v4l_enable_media_source() and v4l_disable_media_source()
>
> 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. I believe so. Support atomic pipeline
> configuration and startup is a generic problem that requires a generic
> solution: we should have a way to construct a pipeline and prevent other
> users from messing with it before it's started. But as currently implemented
> by this patchset, it is very specific to the au0828 driver and as such may
> not be added to the V4L2 or the MC frameworks.
>
The enable and disable handlers themselves are for a good reason. These are
handlers that get called from dvb-core, v4l2-core via the common interfaces.
These also get called from sound driver from the media_device. All of this is
generic. Could you please elaborate on which part is au0828 driver specific
other than the media_pipe in struct video_device?
thanks,
-- Shuah
--
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
