Re: [PATCH v2 02/23] media: entity: Modify default behavior of media_entity_get_fwnode_pad

[Steve Longerbeam <slongerbeam@gmail.com>]

Hi Jacopo,

On 11/28/19 4:04 AM, Jacopo Mondi wrote:
> Hi Steve,
>
> On Sun, Nov 24, 2019 at 11:06:42AM -0800, Steve Longerbeam wrote:
>> Modify the default behavior of media_entity_get_fwnode_pad() (when the
>> entity does not provide the get_fwnode_pad op) to first assume the entity
>> implements a 1:1 mapping between fwnode port number and media pad index.
>>
>> If the 1:1 mapping is not valid, e.g. the port number falls outside the
>> entity's pad index range, or the pad at that port number doesn't match the
>> given direction_flags, fall-back to the previous behavior that searches
>> the entity for the first pad that matches the given direction_flags.
>>
>> The previous default behavior can choose the wrong pad for entities with
>> multiple sink or source pads. With this change the function will choose
>> the correct pad index if the entity implements a 1:1 port to pad mapping
>> at that port.
>>
>> Add some comments to the @get_fwnode_pad operation to make it more clear
>> under what conditions entities must implement the operation.
>>
> I understand the rationale behind this, but this is not better than
> assuming the first pad with a matching direction flag is the right
> one imho.
>
> This should help for subdevices with multiple sink/sources, but they
> should really implement get_fwnode_pad() instead of relying on yet another
> assumption as we had "the first pad with matching direction is the right
> one" and now we also have "the pad at index endpoint.port is the right
> one".

I don't see the second assumption as really an assumption. If the given 
endpoint's port number falls within the entity's pad index range (it's a 
valid pad index), and the pad at that index matches the requested 
direction, then it _must_ be the correct pad.


>   As you've been testing it, how many of the current mailine
> supported devices actually need this and could not:
> 1) Implement get_fwnode_pad() as you are doing for adv748x
> 2) Keep assuming the first pad with a matching flag is the correct one

Well, this patch reduces the number of devices that need to implement 
get_fwnode_pad():

Before: devices do NOT need implement the op if they have just one pad 
of a given direction (one sink pad, one source pad, or one of each).

After: devices do NOT need to implement the op if they have just one pad 
of a given direction, OR all of their pad indexes match their fwnode 
port numbers.

Steve

>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>>   drivers/media/mc/mc-entity.c | 25 ++++++++++++++++++++-----
>>   include/media/media-entity.h | 21 +++++++++++++++------
>>   2 files changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index c333320f790a..47a39d9383d8 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -370,22 +370,37 @@ int media_entity_get_fwnode_pad(struct media_entity *entity,
>>   				unsigned long direction_flags)
>>   {
>>   	struct fwnode_endpoint endpoint;
>> -	unsigned int i;
>>   	int ret;
>>
>> +	ret = fwnode_graph_parse_endpoint(fwnode, &endpoint);
>> +	if (ret)
>> +		return ret;
>> +
>>   	if (!entity->ops || !entity->ops->get_fwnode_pad) {
>> +		unsigned int i;
>> +
>> +		/*
>> +		 * for the default case, first try a 1:1 mapping between
>> +		 * fwnode port number and pad index.
>> +		 */
>> +		ret = endpoint.port;
>> +		if (ret < entity->num_pads &&
>> +		    (entity->pads[ret].flags & direction_flags))
>> +			return ret;
>> +
>> +		/*
>> +		 * if that didn't work search the entity for the first
>> +		 * pad that matches the @direction_flags.
>> +		 */
>>   		for (i = 0; i < entity->num_pads; i++) {
>>   			if (entity->pads[i].flags & direction_flags)
>>   				return i;
>>   		}
>>
>> +		/* else fail */
>>   		return -ENXIO;
>>   	}
>>
>> -	ret = fwnode_graph_parse_endpoint(fwnode, &endpoint);
>> -	if (ret)
>> -		return ret;
>> -
>>   	ret = entity->ops->get_fwnode_pad(entity, &endpoint);
>>   	if (ret < 0)
>>   		return ret;
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index cde80ad029b7..ed00adb4313b 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -199,6 +199,12 @@ struct media_pad {
>>    * @get_fwnode_pad:	Return the pad number based on a fwnode endpoint or
>>    *			a negative value on error. This operation can be used
>>    *			to map a fwnode to a media pad number. Optional.
>> + *			Entities do not need to implement this operation
>> + *			unless two conditions are met:
>> + *			- the entity has more than one sink and/or source
>> + *			  pad, _and_
>> + *			- the entity does not implement a 1:1 mapping of
>> + *			  fwnode port numbers to pad indexes.
>>    * @link_setup:		Notify the entity of link changes. The operation can
>>    *			return an error, in which case link setup will be
>>    *			cancelled. Optional.
>> @@ -858,21 +864,24 @@ struct media_link *media_entity_find_link(struct media_pad *source,
>>   struct media_pad *media_entity_remote_pad(const struct media_pad *pad);
>>
>>   /**
>> - * media_entity_get_fwnode_pad - Get pad number from fwnode
>> + * media_entity_get_fwnode_pad - Get pad number from an endpoint fwnode
>>    *
>>    * @entity: The entity
>> - * @fwnode: Pointer to the fwnode_handle which should be used to find the pad
>> + * @fwnode: Pointer to the endpoint fwnode_handle which should be used to
>> + *          find the pad
>>    * @direction_flags: Expected direction of the pad, as defined in
>>    *		     :ref:`include/uapi/linux/media.h <media_header>`
>>    *		     (seek for ``MEDIA_PAD_FL_*``)
>>    *
>>    * This function can be used to resolve the media pad number from
>> - * a fwnode. This is useful for devices which use more complex
>> - * mappings of media pads.
>> + * an endpoint fwnode. This is useful for devices which use more
>> + * complex mappings of media pads.
>>    *
>>    * If the entity does not implement the get_fwnode_pad() operation
>> - * then this function searches the entity for the first pad that
>> - * matches the @direction_flags.
>> + * then this function first assumes the entity implements a 1:1 mapping
>> + * between fwnode port number and media pad index. If the 1:1 mapping
>> + * is not valid, then the function searches the entity for the first pad
>> + * that matches the @direction_flags.
>>    *
>>    * Return: returns the pad number on success or a negative error code.
>>    */
>> --
>> 2.17.1
>>