Re: [PATCH v3 06/22] media: Media Controller enable/disable source handler API

[Shuah Khan]

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.
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)
shuahkh@xxxxxxxxxxxxxxx | (970) 217-8978