Hi Laurent,
Thanks for your feedback.
On 2018-03-02 11:59:14 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Friday, 2 March 2018 03:57:41 EET Niklas Söderlund wrote:
> > When the VIN driver is running in media centric mode (on Gen3) the
> > colorspace is not retrieved from the video source instead the user is
> > expected to set it as part of the format. There is no way for the VIN
> > driver to validated the colorspace requested by user-space, this creates
> > a problem where validation tools fail. Until the user requested
> > colorspace can be validated lets force it to the driver default.
>
> The problem here is that the V4L2 specification clearly documents the
> colorspace fields as being set by drivers for capture devices. Using the
> values supplied by userspace thus wouldn't comply with the API. The API has
> to
> be updated to allow us to implement this feature, but until then Hans wants
> the userspace to be set by the driver to a fixed value. Could you update the
> commit message accordingly, as well as the comment below ?
Yes, your description of the issue is better I will rephrase my commit
message and comment.
>
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+rene...@ragnatech.se>
> > ---
> > drivers/media/platform/rcar-vin/rcar-v4l2.c | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > 8d92710efffa7276..02f3100ed30db63c 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -675,12 +675,24 @@ static const struct v4l2_ioctl_ops rvin_ioctl_ops = {
> > * V4L2 Media Controller
> > */
> >
> > +static int rvin_mc_try_format(struct rvin_dev *vin, struct v4l2_pix_format
> > *pix) +{
> > + /*
> > + * There is no way to validate the colorspace provided by the
> > + * user. Until it can be validated force colorspace to the
> > + * driver default.
> > + */
> > + pix->colorspace = RVIN_DEFAULT_COLORSPACE;
>
> Should you also set the xfer_func, quantization and ycbcr_enc ?
You are correct, I will set these fields as well.
>
> Apart from that,
>
> Reviewed-by: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
Thank you. As I have rewritten the commit message and set more fields
then just the colorspace I have not picked up this tag for the next
version.
>
> > +
> > + return rvin_format_align(vin, pix);
> > +}
> > +
> > static int rvin_mc_try_fmt_vid_cap(struct file *file, void *priv,
> > struct v4l2_format *f)
> > {
> > struct rvin_dev *vin = video_drvdata(file);
> >
> > - return rvin_format_align(vin, &f->fmt.pix);
> > + return rvin_mc_try_format(vin, &f->fmt.pix);
> > }
> >
> > static int rvin_mc_s_fmt_vid_cap(struct file *file, void *priv,
> > @@ -692,7 +704,7 @@ static int rvin_mc_s_fmt_vid_cap(struct file *file, void
> > *priv, if (vb2_is_busy(&vin->queue))
> > return -EBUSY;
> >
> > - ret = rvin_format_align(vin, &f->fmt.pix);
> > + ret = rvin_mc_try_format(vin, &f->fmt.pix);
> > if (ret)
> > return ret;
>
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Regards,
Niklas Söderlund
