| Message ID | 20201008204747.26320-4-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] ipu3-cio2: Return actual subdev format | expand |
Hi Sakari, Thank you for the patch. On Thu, Oct 08, 2020 at 11:47:47PM +0300, Sakari Ailus wrote: > Check the mbus code provided by the user is one of those the driver > supports. Ignore the code in set_fmt otherwise. You're reading my mind :-) The code shouldn't be ignored though, when an unsupported code is set, it's best to use a fixed default code instead to make the driver behaviour as stateless as possible. > Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver") > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 9c7b527a8800..2ea6313e00b0 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1270,10 +1270,17 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > } else { > + unsigned int i; > + > /* It's the sink, allow changing frame size */ > q->subdev_fmt.width = fmt->format.width; > q->subdev_fmt.height = fmt->format.height; > - q->subdev_fmt.code = fmt->format.code; > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > + if (formats[i].mbus_code == fmt->format.code) { > + q->subdev_fmt.code = fmt->format.code; > + break; > + } > + } > fmt->format = q->subdev_fmt; This should equally apply to the TRY format, we should accept an unsupported code. I'd rework the code as follows: v4l2_mbus_framefmt *format; if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) format = v4l2_subdev_get_try_format(sd, cfg, fmt->pad); else format = &q->subdev_fmt; (this can be done outside of the mutex-protected section) and then operate on format unconditionally. Note that we should also allow changing the field and colorspace information. -- Regards, Laurent Pinchart
Hi Laurent, On Fri, Oct 09, 2020 at 03:49:49AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Thu, Oct 08, 2020 at 11:47:47PM +0300, Sakari Ailus wrote: > > Check the mbus code provided by the user is one of those the driver > > supports. Ignore the code in set_fmt otherwise. > > You're reading my mind :-) > > The code shouldn't be ignored though, when an unsupported code is set, > it's best to use a fixed default code instead to make the driver > behaviour as stateless as possible. I can change it to that, yes. > > > Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver") > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index 9c7b527a8800..2ea6313e00b0 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -1270,10 +1270,17 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > > *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > > } else { > > + unsigned int i; > > + > > /* It's the sink, allow changing frame size */ > > q->subdev_fmt.width = fmt->format.width; > > q->subdev_fmt.height = fmt->format.height; > > - q->subdev_fmt.code = fmt->format.code; > > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > > + if (formats[i].mbus_code == fmt->format.code) { > > + q->subdev_fmt.code = fmt->format.code; > > + break; > > + } > > + } > > fmt->format = q->subdev_fmt; > > This should equally apply to the TRY format, we should accept an > unsupported code. I'd rework the code as follows: > > v4l2_mbus_framefmt *format; > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > format = v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > else > format = &q->subdev_fmt; > > (this can be done outside of the mutex-protected section) and then > operate on format unconditionally. Agreed. > Note that we should also allow changing the field and colorspace > information. Indeed.
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 9c7b527a8800..2ea6313e00b0 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1270,10 +1270,17 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; } else { + unsigned int i; + /* It's the sink, allow changing frame size */ q->subdev_fmt.width = fmt->format.width; q->subdev_fmt.height = fmt->format.height; - q->subdev_fmt.code = fmt->format.code; + for (i = 0; i < ARRAY_SIZE(formats); i++) { + if (formats[i].mbus_code == fmt->format.code) { + q->subdev_fmt.code = fmt->format.code; + break; + } + } fmt->format = q->subdev_fmt; }
Check the mbus code provided by the user is one of those the driver supports. Ignore the code in set_fmt otherwise. Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)