Hi Helen and Mauro, On Thu, Jun 08, 2017 at 02:05:08PM -0300, Helen Koike wrote: > colorspace, ycbcr_enc, quantization and xfer_func must match > across the link. > Check if they match in v4l2_subdev_link_validate_default > unless they are set as _DEFAULT.
I think you could ignore my earlier comments on this --- the check will take place only iff both are not defaults, i.e. non-zero. And these values definitely should be zero unless explicitly set otherwise -- by the driver. I missed this on the previous review round. So I think it'd be fine to return an error on these. How about using dev_dbg() instead if dev_warn()? Using dev_warn() gives an easy way to flood the logs to the user. A debug level message is still important as it's next to impossible for the user to figure out what actually went wrong. Getting a single numeric error code from starting the pipeline isn't telling much... > > Signed-off-by: Helen Koike <helen.ko...@collabora.com> > > --- > > Hi, > > As discussed previously, I added a warn message instead of returning > error to give drivers some time to adapt. > But the problem is that: as the format is set by userspace, it is > possible that userspace will set the wrong format at pads and see these > messages when there is no error in the driver's code at all (or maybe > this is not a problem, just noise in the log). > > Helen > --- > drivers/media/v4l2-core/v4l2-subdev.c | 38 > +++++++++++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c > b/drivers/media/v4l2-core/v4l2-subdev.c > index da78497..1a642c7 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -508,6 +508,44 @@ int v4l2_subdev_link_validate_default(struct v4l2_subdev > *sd, > || source_fmt->format.code != sink_fmt->format.code) > return -EPIPE; > > + /* > + * TODO: return -EPIPE instead of printing a warning in the following > + * checks. As colorimetry properties were added after most of the > + * drivers, only a warning was added to avoid potential regressions > + */ > + > + /* colorspace match. */ > + if (source_fmt->format.colorspace != sink_fmt->format.colorspace) > + dev_warn(sd->v4l2_dev->dev, > + "colorspace doesn't match in link > \"%s\":%d->\"%s\":%d\n", > + link->source->entity->name, link->source->index, > + link->sink->entity->name, link->sink->index); > + > + /* Colorimetry must match if they are not set to DEFAULT */ > + if (source_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT > + && sink_fmt->format.ycbcr_enc != V4L2_YCBCR_ENC_DEFAULT > + && source_fmt->format.ycbcr_enc != sink_fmt->format.ycbcr_enc) > + dev_warn(sd->v4l2_dev->dev, > + "YCbCr encoding doesn't match in link > \"%s\":%d->\"%s\":%d\n", > + link->source->entity->name, link->source->index, > + link->sink->entity->name, link->sink->index); > + > + if (source_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT > + && sink_fmt->format.quantization != V4L2_QUANTIZATION_DEFAULT > + && source_fmt->format.quantization != sink_fmt->format.quantization) > + dev_warn(sd->v4l2_dev->dev, > + "quantization doesn't match in link > \"%s\":%d->\"%s\":%d\n", > + link->source->entity->name, link->source->index, > + link->sink->entity->name, link->sink->index); > + > + if (source_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT > + && sink_fmt->format.xfer_func != V4L2_XFER_FUNC_DEFAULT > + && source_fmt->format.xfer_func != sink_fmt->format.xfer_func) > + dev_warn(sd->v4l2_dev->dev, > + "transfer function doesn't match in link > \"%s\":%d->\"%s\":%d\n", > + link->source->entity->name, link->source->index, > + link->sink->entity->name, link->sink->index); > + > /* The field order must match, or the sink field order must be NONE > * to support interlaced hardware connected to bridges that support > * progressive formats only. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
