| Message ID | 20201009150756.3397-5-sakari.ailus@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | a86cf9b29e8b12811cf53c4970eefe0c1d290476 |
| Headers | show |
| Series | ipu3-cio2 format fixes | expand |
On Fri, Oct 9, 2020 at 6:10 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Validate media bus code, width and height when setting the subdev format. > > This effectively reworks how setting subdev format is implemented in the > driver. Does it make it dependent on patch 3/5? Or maybe you can use min_t() and update to min() in a separate patch? > 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 | 28 ++++++++++++++++-------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 9c7b527a8800..9726091c9985 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1257,6 +1257,9 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_format *fmt) > { > struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); > + struct v4l2_mbus_framefmt *mbus; > + u32 mbus_code = fmt->format.code; > + unsigned int i; > > /* > * Only allow setting sink pad format; > @@ -1265,18 +1268,25 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > if (fmt->pad == CIO2_PAD_SOURCE) > return cio2_subdev_get_fmt(sd, cfg, fmt); > > - mutex_lock(&q->subdev_lock); > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > + mbus = v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > + else > + mbus = &q->subdev_fmt; > > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > - *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > - } else { > - /* 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; > - fmt->format = q->subdev_fmt; > + fmt->format.code = formats[0].mbus_code; > + > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > + if (formats[i].mbus_code == fmt->format.code) { > + fmt->format.code = mbus_code; > + break; > + } > } > > + fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH); > + fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH); > + > + mutex_lock(&q->subdev_lock); > + *mbus = fmt->format; > mutex_unlock(&q->subdev_lock); > > return 0; > -- > 2.27.0 > -- With Best Regards, Andy Shevchenko
Hi Sakari, Thank you for the patch. On Fri, Oct 09, 2020 at 06:07:55PM +0300, Sakari Ailus wrote: > Validate media bus code, width and height when setting the subdev format. > > This effectively reworks how setting subdev format is implemented in the > driver. > > 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 | 28 ++++++++++++++++-------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 9c7b527a8800..9726091c9985 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1257,6 +1257,9 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_format *fmt) > { > struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); > + struct v4l2_mbus_framefmt *mbus; > + u32 mbus_code = fmt->format.code; > + unsigned int i; > > /* > * Only allow setting sink pad format; > @@ -1265,18 +1268,25 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > if (fmt->pad == CIO2_PAD_SOURCE) > return cio2_subdev_get_fmt(sd, cfg, fmt); > > - mutex_lock(&q->subdev_lock); > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > + mbus = v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > + else > + mbus = &q->subdev_fmt; > > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > - *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > - } else { > - /* 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; > - fmt->format = q->subdev_fmt; > + fmt->format.code = formats[0].mbus_code; > + > + for (i = 0; i < ARRAY_SIZE(formats); i++) { > + if (formats[i].mbus_code == fmt->format.code) { > + fmt->format.code = mbus_code; > + break; > + } > } I really, really wish C had a for...else construct as in Python :-( > > + fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH); > + fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH); This looks good, but it would be worth renaming CIO2_IMAGE_MAX_LENGTH to CIO2_IMAGE_MAX_HEIGHT in a separate patch. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > + mutex_lock(&q->subdev_lock); > + *mbus = fmt->format; > mutex_unlock(&q->subdev_lock); > > return 0; -- Regards, Laurent Pinchart
On Fri, Oct 09, 2020 at 07:22:38PM +0300, Andy Shevchenko wrote: > On Fri, Oct 9, 2020 at 6:10 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Validate media bus code, width and height when setting the subdev format. > > > > This effectively reworks how setting subdev format is implemented in the > > driver. > > Does it make it dependent on patch 3/5? Good question. They're in the same set but these probably should be backported. If fact, I think I should add Cc: stable to most of these patches. > Or maybe you can use min_t() and update to min() in a separate patch? I would have said that's overkill but considering the stable tree it's not. I'll do that for v3. Thanks for the review!
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 9c7b527a8800..9726091c9985 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1257,6 +1257,9 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_format *fmt) { struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); + struct v4l2_mbus_framefmt *mbus; + u32 mbus_code = fmt->format.code; + unsigned int i; /* * Only allow setting sink pad format; @@ -1265,18 +1268,25 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, if (fmt->pad == CIO2_PAD_SOURCE) return cio2_subdev_get_fmt(sd, cfg, fmt); - mutex_lock(&q->subdev_lock); + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) + mbus = v4l2_subdev_get_try_format(sd, cfg, fmt->pad); + else + mbus = &q->subdev_fmt; - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { - *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; - } else { - /* 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; - fmt->format = q->subdev_fmt; + fmt->format.code = formats[0].mbus_code; + + for (i = 0; i < ARRAY_SIZE(formats); i++) { + if (formats[i].mbus_code == fmt->format.code) { + fmt->format.code = mbus_code; + break; + } } + fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH); + fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH); + + mutex_lock(&q->subdev_lock); + *mbus = fmt->format; mutex_unlock(&q->subdev_lock); return 0;
Validate media bus code, width and height when setting the subdev format. This effectively reworks how setting subdev format is implemented in the driver. 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 | 28 ++++++++++++++++-------- 1 file changed, 19 insertions(+), 9 deletions(-)