Re: [PATCH 17/23] v4l: Implement v4l2_subdev_link_validate()
Hi Sakari,
On Tuesday 17 January 2012 21:21:39 Sakari Ailus wrote:
> On Mon, Jan 16, 2012 at 03:44:08PM +0100, Laurent Pinchart wrote:
> > On Wednesday 11 January 2012 22:26:54 Sakari Ailus wrote:
> > > v4l2_subdev_link_validate() is the default op for validating a link. In
> > > V4L2 subdev context, it is used to call a pad op which performs the
> > > proper link check without much extra work.
> > >
> > > Signed-off-by: Sakari Ailus
> > > ---
> > >
> > > drivers/media/video/v4l2-subdev.c | 62
> > >
> > > + include/media/v4l2-subdev.h
> > > |
> > >
> > > 10 ++
> > > 2 files changed, 72 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/media/video/v4l2-subdev.c
> > > b/drivers/media/video/v4l2-subdev.c index 836270d..4b329a0 100644
> > > --- a/drivers/media/video/v4l2-subdev.c
> > > +++ b/drivers/media/video/v4l2-subdev.c
> > > @@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops
> > > = {
> > >
> > > .poll = subdev_poll,
> > >
> > > };
> > >
> > > +#ifdef CONFIG_MEDIA_CONTROLLER
> > > +int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
> > > + struct media_link *link,
> > > + struct v4l2_subdev_format *source_fmt,
> > > + struct v4l2_subdev_format *sink_fmt)
> > > +{
> > > + if (source_fmt->format.width != sink_fmt->format.width
> > > + || source_fmt->format.height != sink_fmt->format.height
> > > + || source_fmt->format.code != sink_fmt->format.code)
> > > + return -EINVAL;
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
> >
> > What about calling this function directly from
> > v4l2_subdev_link_validate() if the pad::link_validate operation is NULL
> > ? That wouldn't require changing all subdev drivers to explicitly use
> > the default implementation.
>
> I can do that. I still want to keep the function available for those that
> want to call it explicitly to perform the above check.
>
> > > +
> > > +static struct v4l2_subdev_format
> > > +*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
> > > + struct v4l2_subdev_format *fmt)
> > > +{
> > > + int rval;
> > > +
> > > + switch (media_entity_type(pad->entity)) {
> > > + case MEDIA_ENT_T_V4L2_SUBDEV:
> > > + fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > + fmt->pad = pad->index;
> > > + rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
> > > + pad->entity),
> > > + pad, get_fmt, NULL, fmt);
> > > + if (rval < 0)
> > > + return NULL;
> > > + return fmt;
> > > + case MEDIA_ENT_T_DEVNODE_V4L:
> > > + return NULL;
> > > + default:
> > > + BUG();
> >
> > Maybe WARN() and return NULL ?
>
> It's a clear driver BUG() if this happens. If you think the correct
> response to that is WARN() and return NULL, I can do that.
You're right.
> > > + }
> > > +}
> > > +
> > > +int v4l2_subdev_link_validate(struct media_link *link)
> > > +{
> > > + struct v4l2_subdev *sink = NULL, *source = NULL;
> > > + struct v4l2_subdev_format _sink_fmt, _source_fmt;
> > > + struct v4l2_subdev_format *sink_fmt, *source_fmt;
> > > +
> > > + source_fmt = v4l2_subdev_link_validate_get_format(
> > > + link->source, &_source_fmt);
> > > + sink_fmt = v4l2_subdev_link_validate_get_format(
> > > + link->sink, &_sink_fmt);
> > > +
> > > + if (source_fmt)
> > > + source = media_entity_to_v4l2_subdev(link->source->entity);
> > > + if (sink_fmt)
> > > + sink = media_entity_to_v4l2_subdev(link->sink->entity);
> > > +
> > > + if (source_fmt && sink_fmt)
> > > + return v4l2_subdev_call(sink, pad, link_validate, link,
> > > + source_fmt, sink_fmt);
> >
> > This looks overly complex. Why don't you return 0 if one of the two
> > entities is of a type different than MEDIA_ENT_T_V4L2_SUBDEV, then
> > retrieve the formats for the two entities and return 0 if one of the two
> > operation fails, and finally call pad::link_validate ?
>
> Now that you mention that, I agree. :-) I'll fix it.
--
Regards,
Laurent Pinchart
--
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 17/23] v4l: Implement v4l2_subdev_link_validate()
Hi Laurent,
Thanks for the review!
On Mon, Jan 16, 2012 at 03:44:08PM +0100, Laurent Pinchart wrote:
> On Wednesday 11 January 2012 22:26:54 Sakari Ailus wrote:
> > v4l2_subdev_link_validate() is the default op for validating a link. In
> > V4L2 subdev context, it is used to call a pad op which performs the proper
> > link check without much extra work.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/media/video/v4l2-subdev.c | 62
> > + include/media/v4l2-subdev.h |
> > 10 ++
> > 2 files changed, 72 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/media/video/v4l2-subdev.c
> > b/drivers/media/video/v4l2-subdev.c index 836270d..4b329a0 100644
> > --- a/drivers/media/video/v4l2-subdev.c
> > +++ b/drivers/media/video/v4l2-subdev.c
> > @@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
> > .poll = subdev_poll,
> > };
> >
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
> > + struct media_link *link,
> > + struct v4l2_subdev_format *source_fmt,
> > + struct v4l2_subdev_format *sink_fmt)
> > +{
> > + if (source_fmt->format.width != sink_fmt->format.width
> > + || source_fmt->format.height != sink_fmt->format.height
> > + || source_fmt->format.code != sink_fmt->format.code)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
>
> What about calling this function directly from v4l2_subdev_link_validate() if
> the pad::link_validate operation is NULL ? That wouldn't require changing all
> subdev drivers to explicitly use the default implementation.
I can do that. I still want to keep the function available for those that
want to call it explicitly to perform the above check.
> > +
> > +static struct v4l2_subdev_format
> > +*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + int rval;
> > +
> > + switch (media_entity_type(pad->entity)) {
> > + case MEDIA_ENT_T_V4L2_SUBDEV:
> > + fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > + fmt->pad = pad->index;
> > + rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
> > + pad->entity),
> > + pad, get_fmt, NULL, fmt);
> > + if (rval < 0)
> > + return NULL;
> > + return fmt;
> > + case MEDIA_ENT_T_DEVNODE_V4L:
> > + return NULL;
> > + default:
> > + BUG();
>
> Maybe WARN() and return NULL ?
It's a clear driver BUG() if this happens. If you think the correct response
to that is WARN() and return NULL, I can do that.
> > + }
> > +}
> > +
> > +int v4l2_subdev_link_validate(struct media_link *link)
> > +{
> > + struct v4l2_subdev *sink = NULL, *source = NULL;
> > + struct v4l2_subdev_format _sink_fmt, _source_fmt;
> > + struct v4l2_subdev_format *sink_fmt, *source_fmt;
> > +
> > + source_fmt = v4l2_subdev_link_validate_get_format(
> > + link->source, &_source_fmt);
> > + sink_fmt = v4l2_subdev_link_validate_get_format(
> > + link->sink, &_sink_fmt);
> > +
> > + if (source_fmt)
> > + source = media_entity_to_v4l2_subdev(link->source->entity);
> > + if (sink_fmt)
> > + sink = media_entity_to_v4l2_subdev(link->sink->entity);
> > +
> > + if (source_fmt && sink_fmt)
> > + return v4l2_subdev_call(sink, pad, link_validate, link,
> > + source_fmt, sink_fmt);
>
> This looks overly complex. Why don't you return 0 if one of the two entities
> is of a type different than MEDIA_ENT_T_V4L2_SUBDEV, then retrieve the
> formats
> for the two entities and return 0 if one of the two operation fails, and
> finally call pad::link_validate ?
Now that you mention that, I agree. :-) I'll fix it.
Regards,
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: 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
Re: [PATCH 17/23] v4l: Implement v4l2_subdev_link_validate()
Hi Sakari,
Thanks for the patch.
On Wednesday 11 January 2012 22:26:54 Sakari Ailus wrote:
> v4l2_subdev_link_validate() is the default op for validating a link. In
> V4L2 subdev context, it is used to call a pad op which performs the proper
> link check without much extra work.
>
> Signed-off-by: Sakari Ailus
> ---
> drivers/media/video/v4l2-subdev.c | 62
> + include/media/v4l2-subdev.h |
> 10 ++
> 2 files changed, 72 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-subdev.c
> b/drivers/media/video/v4l2-subdev.c index 836270d..4b329a0 100644
> --- a/drivers/media/video/v4l2-subdev.c
> +++ b/drivers/media/video/v4l2-subdev.c
> @@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
> .poll = subdev_poll,
> };
>
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
> + struct media_link *link,
> + struct v4l2_subdev_format *source_fmt,
> + struct v4l2_subdev_format *sink_fmt)
> +{
> + if (source_fmt->format.width != sink_fmt->format.width
> + || source_fmt->format.height != sink_fmt->format.height
> + || source_fmt->format.code != sink_fmt->format.code)
> + return -EINVAL;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
What about calling this function directly from v4l2_subdev_link_validate() if
the pad::link_validate operation is NULL ? That wouldn't require changing all
subdev drivers to explicitly use the default implementation.
> +
> +static struct v4l2_subdev_format
> +*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
> + struct v4l2_subdev_format *fmt)
> +{
> + int rval;
> +
> + switch (media_entity_type(pad->entity)) {
> + case MEDIA_ENT_T_V4L2_SUBDEV:
> + fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + fmt->pad = pad->index;
> + rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
> + pad->entity),
> + pad, get_fmt, NULL, fmt);
> + if (rval < 0)
> + return NULL;
> + return fmt;
> + case MEDIA_ENT_T_DEVNODE_V4L:
> + return NULL;
> + default:
> + BUG();
Maybe WARN() and return NULL ?
> + }
> +}
> +
> +int v4l2_subdev_link_validate(struct media_link *link)
> +{
> + struct v4l2_subdev *sink = NULL, *source = NULL;
> + struct v4l2_subdev_format _sink_fmt, _source_fmt;
> + struct v4l2_subdev_format *sink_fmt, *source_fmt;
> +
> + source_fmt = v4l2_subdev_link_validate_get_format(
> + link->source, &_source_fmt);
> + sink_fmt = v4l2_subdev_link_validate_get_format(
> + link->sink, &_sink_fmt);
> +
> + if (source_fmt)
> + source = media_entity_to_v4l2_subdev(link->source->entity);
> + if (sink_fmt)
> + sink = media_entity_to_v4l2_subdev(link->sink->entity);
> +
> + if (source_fmt && sink_fmt)
> + return v4l2_subdev_call(sink, pad, link_validate, link,
> + source_fmt, sink_fmt);
This looks overly complex. Why don't you return 0 if one of the two entities
is of a type different than MEDIA_ENT_T_V4L2_SUBDEV, then retrieve the formats
for the two entities and return 0 if one of the two operation fails, and
finally call pad::link_validate ?
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +
> void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops
> *ops) {
> INIT_LIST_HEAD(&sd->list);
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index feab950..436e6f4 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -470,6 +470,11 @@ struct v4l2_subdev_pad_ops {
>struct v4l2_subdev_selection *sel);
> int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
>struct v4l2_subdev_selection *sel);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> + int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
> + struct v4l2_subdev_format *source_fmt,
> + struct v4l2_subdev_format *sink_fmt);
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> };
>
> struct v4l2_subdev_ops {
> @@ -606,6 +611,11 @@ static inline void *v4l2_get_subdev_hostdata(const
> struct v4l2_subdev *sd) return sd->host_priv;
> }
>
> +int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
> + struct media_link *link,
> + struct v4l2_subdev_format *source_fmt,
> + struct v4l2_subdev_format
[PATCH 17/23] v4l: Implement v4l2_subdev_link_validate()
v4l2_subdev_link_validate() is the default op for validating a link. In V4L2
subdev context, it is used to call a pad op which performs the proper link
check without much extra work.
Signed-off-by: Sakari Ailus
---
drivers/media/video/v4l2-subdev.c | 62 +
include/media/v4l2-subdev.h | 10 ++
2 files changed, 72 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-subdev.c
b/drivers/media/video/v4l2-subdev.c
index 836270d..4b329a0 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -367,6 +367,68 @@ const struct v4l2_file_operations v4l2_subdev_fops = {
.poll = subdev_poll,
};
+#ifdef CONFIG_MEDIA_CONTROLLER
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+ struct media_link *link,
+ struct v4l2_subdev_format *source_fmt,
+ struct v4l2_subdev_format *sink_fmt)
+{
+ if (source_fmt->format.width != sink_fmt->format.width
+ || source_fmt->format.height != sink_fmt->format.height
+ || source_fmt->format.code != sink_fmt->format.code)
+ return -EINVAL;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate_default);
+
+static struct v4l2_subdev_format
+*v4l2_subdev_link_validate_get_format(struct media_pad *pad,
+ struct v4l2_subdev_format *fmt)
+{
+ int rval;
+
+ switch (media_entity_type(pad->entity)) {
+ case MEDIA_ENT_T_V4L2_SUBDEV:
+ fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
+ fmt->pad = pad->index;
+ rval = v4l2_subdev_call(media_entity_to_v4l2_subdev(
+ pad->entity),
+ pad, get_fmt, NULL, fmt);
+ if (rval < 0)
+ return NULL;
+ return fmt;
+ case MEDIA_ENT_T_DEVNODE_V4L:
+ return NULL;
+ default:
+ BUG();
+ }
+}
+
+int v4l2_subdev_link_validate(struct media_link *link)
+{
+ struct v4l2_subdev *sink = NULL, *source = NULL;
+ struct v4l2_subdev_format _sink_fmt, _source_fmt;
+ struct v4l2_subdev_format *sink_fmt, *source_fmt;
+
+ source_fmt = v4l2_subdev_link_validate_get_format(
+ link->source, &_source_fmt);
+ sink_fmt = v4l2_subdev_link_validate_get_format(
+ link->sink, &_sink_fmt);
+
+ if (source_fmt)
+ source = media_entity_to_v4l2_subdev(link->source->entity);
+ if (sink_fmt)
+ sink = media_entity_to_v4l2_subdev(link->sink->entity);
+
+ if (source_fmt && sink_fmt)
+ return v4l2_subdev_call(sink, pad, link_validate, link,
+ source_fmt, sink_fmt);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_subdev_link_validate);
+#endif /* CONFIG_MEDIA_CONTROLLER */
+
void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops
*ops)
{
INIT_LIST_HEAD(&sd->list);
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index feab950..436e6f4 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -470,6 +470,11 @@ struct v4l2_subdev_pad_ops {
struct v4l2_subdev_selection *sel);
int (*set_selection)(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh,
struct v4l2_subdev_selection *sel);
+#ifdef CONFIG_MEDIA_CONTROLLER
+ int (*link_validate)(struct v4l2_subdev *sd, struct media_link *link,
+struct v4l2_subdev_format *source_fmt,
+struct v4l2_subdev_format *sink_fmt);
+#endif /* CONFIG_MEDIA_CONTROLLER */
};
struct v4l2_subdev_ops {
@@ -606,6 +611,11 @@ static inline void *v4l2_get_subdev_hostdata(const struct
v4l2_subdev *sd)
return sd->host_priv;
}
+int v4l2_subdev_link_validate_default(struct v4l2_subdev *sd,
+ struct media_link *link,
+ struct v4l2_subdev_format *source_fmt,
+ struct v4l2_subdev_format *sink_fmt);
+int v4l2_subdev_link_validate(struct media_link *link);
void v4l2_subdev_init(struct v4l2_subdev *sd,
const struct v4l2_subdev_ops *ops);
--
1.7.2.5
--
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
