Re: [PATCH 17/23] v4l: Implement v4l2_subdev_link_validate()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <sakari.ailus@xxxxxx>
> > > ---
> > > 
> > >  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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux