| Message ID | 20191124190703.12138-3-slongerbeam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Create media links in bound notifiers | expand |
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". 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 Thanks j > 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 >
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 >>
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. */
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. 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(-)