* [PATCH 0/3] ipu3-cio2 format fixes
@ 2020-10-08 20:47 Sakari Ailus
2020-10-08 20:47 ` [PATCH 1/3] ipu3-cio2: Return actual subdev format Sakari Ailus
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Sakari Ailus @ 2020-10-08 20:47 UTC (permalink / raw)
To: linux-media; +Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu
Hello all,
This set addresses most notable subdev format related issues, namely the
sub-device sink format being unaccessible. The result of accessing it
varied from oopses to crashes.
Sakari Ailus (3):
ipu3-cio2: Return actual subdev format
ipu3-cio2: Serialise access to pad format
ipu3-cio2: Only allow setting valid mbus codes
drivers/media/pci/intel/ipu3/ipu3-cio2.c | 42 ++++++++++++------------
drivers/media/pci/intel/ipu3/ipu3-cio2.h | 1 +
2 files changed, 22 insertions(+), 21 deletions(-)
--
2.27.0
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/3] ipu3-cio2: Return actual subdev format 2020-10-08 20:47 [PATCH 0/3] ipu3-cio2 format fixes Sakari Ailus @ 2020-10-08 20:47 ` Sakari Ailus 2020-10-09 0:39 ` Laurent Pinchart 2020-10-08 20:47 ` [PATCH 2/3] ipu3-cio2: Serialise access to pad format Sakari Ailus 2020-10-08 20:47 ` [PATCH 3/3] ipu3-cio2: Only allow setting valid mbus codes Sakari Ailus 2 siblings, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2020-10-08 20:47 UTC (permalink / raw) To: linux-media; +Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu Return actual subdev format on ipu3-cio2 subdev pads. The earlier implementation was based on an infinite recursion that exhausted the stack. Reported-by: Tsuchiya Yuto <kitakar@gmail.com> 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 | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index 4e598e937dfe..afa472026ba4 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1232,29 +1232,11 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_format *fmt) { struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); - struct v4l2_subdev_format format; - int ret; - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad); - return 0; - } - - if (fmt->pad == CIO2_PAD_SINK) { - format.which = V4L2_SUBDEV_FORMAT_ACTIVE; - ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, - &format); - - if (ret) - return ret; - /* update colorspace etc */ - q->subdev_fmt.colorspace = format.format.colorspace; - q->subdev_fmt.ycbcr_enc = format.format.ycbcr_enc; - q->subdev_fmt.quantization = format.format.quantization; - q->subdev_fmt.xfer_func = format.format.xfer_func; - } - - fmt->format = q->subdev_fmt; + else + fmt->format = q->subdev_fmt; return 0; } -- 2.27.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] ipu3-cio2: Return actual subdev format 2020-10-08 20:47 ` [PATCH 1/3] ipu3-cio2: Return actual subdev format Sakari Ailus @ 2020-10-09 0:39 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2020-10-09 0:39 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu Hi Sakari, Thank you for the patch. On Thu, Oct 08, 2020 at 11:47:45PM +0300, Sakari Ailus wrote: > Return actual subdev format on ipu3-cio2 subdev pads. The earlier > implementation was based on an infinite recursion that exhausted the > stack. A bad idea indeed :-) > Reported-by: Tsuchiya Yuto <kitakar@gmail.com> > 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 | 24 +++--------------------- > 1 file changed, 3 insertions(+), 21 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index 4e598e937dfe..afa472026ba4 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1232,29 +1232,11 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd, > struct v4l2_subdev_format *fmt) > { > struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); > - struct v4l2_subdev_format format; > - int ret; > > - if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) { > + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > - return 0; > - } > - > - if (fmt->pad == CIO2_PAD_SINK) { > - format.which = V4L2_SUBDEV_FORMAT_ACTIVE; > - ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, > - &format); > - > - if (ret) > - return ret; > - /* update colorspace etc */ > - q->subdev_fmt.colorspace = format.format.colorspace; > - q->subdev_fmt.ycbcr_enc = format.format.ycbcr_enc; > - q->subdev_fmt.quantization = format.format.quantization; > - q->subdev_fmt.xfer_func = format.format.xfer_func; > - } > - > - fmt->format = q->subdev_fmt; I'm pretty speechless. All I can say is Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> and let's forget this has ever existed :-) > + else > + fmt->format = q->subdev_fmt; > > return 0; > } -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/3] ipu3-cio2: Serialise access to pad format 2020-10-08 20:47 [PATCH 0/3] ipu3-cio2 format fixes Sakari Ailus 2020-10-08 20:47 ` [PATCH 1/3] ipu3-cio2: Return actual subdev format Sakari Ailus @ 2020-10-08 20:47 ` Sakari Ailus 2020-10-09 0:44 ` Laurent Pinchart 2020-10-09 16:17 ` Andy Shevchenko 2020-10-08 20:47 ` [PATCH 3/3] ipu3-cio2: Only allow setting valid mbus codes Sakari Ailus 2 siblings, 2 replies; 11+ messages in thread From: Sakari Ailus @ 2020-10-08 20:47 UTC (permalink / raw) To: linux-media; +Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu Pad format can be accessed from user space. Serialise access to it. 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 | 11 +++++++++++ drivers/media/pci/intel/ipu3/ipu3-cio2.h | 1 + 2 files changed, 12 insertions(+) diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index afa472026ba4..9c7b527a8800 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -1233,11 +1233,15 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd, { struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); + mutex_lock(&q->subdev_lock); + if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad); else fmt->format = q->subdev_fmt; + mutex_unlock(&q->subdev_lock); + return 0; } @@ -1261,6 +1265,8 @@ 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) { *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; } else { @@ -1271,6 +1277,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, fmt->format = q->subdev_fmt; } + mutex_unlock(&q->subdev_lock); + return 0; } @@ -1529,6 +1537,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) /* Initialize miscellaneous variables */ mutex_init(&q->lock); + mutex_init(&q->subdev_lock); /* Initialize formats to default values */ fmt = &q->subdev_fmt; @@ -1646,6 +1655,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) cio2_fbpt_exit(q, &cio2->pci_dev->dev); fail_fbpt: mutex_destroy(&q->lock); + mutex_destroy(&q->subdev_lock); return r; } @@ -1658,6 +1668,7 @@ static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q) media_entity_cleanup(&q->subdev.entity); cio2_fbpt_exit(q, &cio2->pci_dev->dev); mutex_destroy(&q->lock); + mutex_destroy(&q->subdev_lock); } static int cio2_queues_init(struct cio2_device *cio2) diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h index 549b08f88f0c..df0247326a1d 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h @@ -335,6 +335,7 @@ struct cio2_queue { /* Subdev, /dev/v4l-subdevX */ struct v4l2_subdev subdev; + struct mutex subdev_lock; struct media_pad subdev_pads[CIO2_PADS]; struct v4l2_mbus_framefmt subdev_fmt; atomic_t frame_sequence; -- 2.27.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] ipu3-cio2: Serialise access to pad format 2020-10-08 20:47 ` [PATCH 2/3] ipu3-cio2: Serialise access to pad format Sakari Ailus @ 2020-10-09 0:44 ` Laurent Pinchart 2020-10-09 9:43 ` Sakari Ailus 2020-10-09 16:17 ` Andy Shevchenko 1 sibling, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2020-10-09 0:44 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu Hi Sakari, Thank you for the patch. On Thu, Oct 08, 2020 at 11:47:46PM +0300, Sakari Ailus wrote: > Pad format can be accessed from user space. Serialise access to it. > > 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 | 11 +++++++++++ > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 1 + > 2 files changed, 12 insertions(+) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index afa472026ba4..9c7b527a8800 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -1233,11 +1233,15 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd, > { > struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); > > + mutex_lock(&q->subdev_lock); > + > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > else > fmt->format = q->subdev_fmt; > > + mutex_unlock(&q->subdev_lock); > + > return 0; > } > > @@ -1261,6 +1265,8 @@ 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) { > *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > } else { > @@ -1271,6 +1277,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > fmt->format = q->subdev_fmt; > } Not a candidate for this patch, but this should restrict the pixel format and size to supported values. > > + mutex_unlock(&q->subdev_lock); > + > return 0; > } > > @@ -1529,6 +1537,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > > /* Initialize miscellaneous variables */ > mutex_init(&q->lock); > + mutex_init(&q->subdev_lock); > > /* Initialize formats to default values */ > fmt = &q->subdev_fmt; > @@ -1646,6 +1655,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > cio2_fbpt_exit(q, &cio2->pci_dev->dev); > fail_fbpt: > mutex_destroy(&q->lock); > + mutex_destroy(&q->subdev_lock); > > return r; > } > @@ -1658,6 +1668,7 @@ static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q) > media_entity_cleanup(&q->subdev.entity); > cio2_fbpt_exit(q, &cio2->pci_dev->dev); > mutex_destroy(&q->lock); > + mutex_destroy(&q->subdev_lock); > } > > static int cio2_queues_init(struct cio2_device *cio2) > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > index 549b08f88f0c..df0247326a1d 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > @@ -335,6 +335,7 @@ struct cio2_queue { > > /* Subdev, /dev/v4l-subdevX */ > struct v4l2_subdev subdev; > + struct mutex subdev_lock; Can you add a small comment to tell which field(s) this lock protects ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > struct media_pad subdev_pads[CIO2_PADS]; > struct v4l2_mbus_framefmt subdev_fmt; > atomic_t frame_sequence; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] ipu3-cio2: Serialise access to pad format 2020-10-09 0:44 ` Laurent Pinchart @ 2020-10-09 9:43 ` Sakari Ailus 2020-10-09 10:39 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2020-10-09 9:43 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu Hi Laurent, On Fri, Oct 09, 2020 at 03:44:12AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. Thanks for the review! > > On Thu, Oct 08, 2020 at 11:47:46PM +0300, Sakari Ailus wrote: > > Pad format can be accessed from user space. Serialise access to it. > > > > 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 | 11 +++++++++++ > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 1 + > > 2 files changed, 12 insertions(+) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index afa472026ba4..9c7b527a8800 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -1233,11 +1233,15 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd, > > { > > struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); > > > > + mutex_lock(&q->subdev_lock); > > + > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > > fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > > else > > fmt->format = q->subdev_fmt; > > > > + mutex_unlock(&q->subdev_lock); > > + > > return 0; > > } > > > > @@ -1261,6 +1265,8 @@ 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) { > > *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > > } else { > > @@ -1271,6 +1277,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > > fmt->format = q->subdev_fmt; > > } > > Not a candidate for this patch, but this should restrict the pixel > format and size to supported values. Yes. > > > > > + mutex_unlock(&q->subdev_lock); > > + > > return 0; > > } > > > > @@ -1529,6 +1537,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > > > > /* Initialize miscellaneous variables */ > > mutex_init(&q->lock); > > + mutex_init(&q->subdev_lock); > > > > /* Initialize formats to default values */ > > fmt = &q->subdev_fmt; > > @@ -1646,6 +1655,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > > cio2_fbpt_exit(q, &cio2->pci_dev->dev); > > fail_fbpt: > > mutex_destroy(&q->lock); > > + mutex_destroy(&q->subdev_lock); > > > > return r; > > } > > @@ -1658,6 +1668,7 @@ static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q) > > media_entity_cleanup(&q->subdev.entity); > > cio2_fbpt_exit(q, &cio2->pci_dev->dev); > > mutex_destroy(&q->lock); > > + mutex_destroy(&q->subdev_lock); > > } > > > > static int cio2_queues_init(struct cio2_device *cio2) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > index 549b08f88f0c..df0247326a1d 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > @@ -335,6 +335,7 @@ struct cio2_queue { > > > > /* Subdev, /dev/v4l-subdevX */ > > struct v4l2_subdev subdev; > > + struct mutex subdev_lock; > > Can you add a small comment to tell which field(s) this lock protects ? How about: /* Serialise access to subdev_fmt field */ Currently it's just that, but I feel locking in this driver would generally benefit from refactoring. That can wait a little though as it's not an acute problem. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > struct media_pad subdev_pads[CIO2_PADS]; > > struct v4l2_mbus_framefmt subdev_fmt; > > atomic_t frame_sequence; > > -- > Regards, > > Laurent Pinchart -- Sakari Ailus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] ipu3-cio2: Serialise access to pad format 2020-10-09 9:43 ` Sakari Ailus @ 2020-10-09 10:39 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2020-10-09 10:39 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu Hi Sakari, On Fri, Oct 09, 2020 at 12:43:47PM +0300, Sakari Ailus wrote: > On Fri, Oct 09, 2020 at 03:44:12AM +0300, Laurent Pinchart wrote: > > On Thu, Oct 08, 2020 at 11:47:46PM +0300, Sakari Ailus wrote: > > > Pad format can be accessed from user space. Serialise access to it. > > > > > > 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 | 11 +++++++++++ > > > drivers/media/pci/intel/ipu3/ipu3-cio2.h | 1 + > > > 2 files changed, 12 insertions(+) > > > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > > index afa472026ba4..9c7b527a8800 100644 > > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > > @@ -1233,11 +1233,15 @@ static int cio2_subdev_get_fmt(struct v4l2_subdev *sd, > > > { > > > struct cio2_queue *q = container_of(sd, struct cio2_queue, subdev); > > > > > > + mutex_lock(&q->subdev_lock); > > > + > > > if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) > > > fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad); > > > else > > > fmt->format = q->subdev_fmt; > > > > > > + mutex_unlock(&q->subdev_lock); > > > + > > > return 0; > > > } > > > > > > @@ -1261,6 +1265,8 @@ 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) { > > > *v4l2_subdev_get_try_format(sd, cfg, fmt->pad) = fmt->format; > > > } else { > > > @@ -1271,6 +1277,8 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd, > > > fmt->format = q->subdev_fmt; > > > } > > > > Not a candidate for this patch, but this should restrict the pixel > > format and size to supported values. > > Yes. > > > > > > > > > + mutex_unlock(&q->subdev_lock); > > > + > > > return 0; > > > } > > > > > > @@ -1529,6 +1537,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > > > > > > /* Initialize miscellaneous variables */ > > > mutex_init(&q->lock); > > > + mutex_init(&q->subdev_lock); > > > > > > /* Initialize formats to default values */ > > > fmt = &q->subdev_fmt; > > > @@ -1646,6 +1655,7 @@ static int cio2_queue_init(struct cio2_device *cio2, struct cio2_queue *q) > > > cio2_fbpt_exit(q, &cio2->pci_dev->dev); > > > fail_fbpt: > > > mutex_destroy(&q->lock); > > > + mutex_destroy(&q->subdev_lock); > > > > > > return r; > > > } > > > @@ -1658,6 +1668,7 @@ static void cio2_queue_exit(struct cio2_device *cio2, struct cio2_queue *q) > > > media_entity_cleanup(&q->subdev.entity); > > > cio2_fbpt_exit(q, &cio2->pci_dev->dev); > > > mutex_destroy(&q->lock); > > > + mutex_destroy(&q->subdev_lock); > > > } > > > > > > static int cio2_queues_init(struct cio2_device *cio2) > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.h b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > index 549b08f88f0c..df0247326a1d 100644 > > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.h > > > @@ -335,6 +335,7 @@ struct cio2_queue { > > > > > > /* Subdev, /dev/v4l-subdevX */ > > > struct v4l2_subdev subdev; > > > + struct mutex subdev_lock; > > > > Can you add a small comment to tell which field(s) this lock protects ? > > How about: > > /* Serialise access to subdev_fmt field */ That's perfect :-) > Currently it's just that, but I feel locking in this driver would generally > benefit from refactoring. That can wait a little though as it's not an > acute problem. Sure. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > struct media_pad subdev_pads[CIO2_PADS]; > > > struct v4l2_mbus_framefmt subdev_fmt; > > > atomic_t frame_sequence; -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] ipu3-cio2: Serialise access to pad format 2020-10-08 20:47 ` [PATCH 2/3] ipu3-cio2: Serialise access to pad format Sakari Ailus 2020-10-09 0:44 ` Laurent Pinchart @ 2020-10-09 16:17 ` Andy Shevchenko 1 sibling, 0 replies; 11+ messages in thread From: Andy Shevchenko @ 2020-10-09 16:17 UTC (permalink / raw) To: Sakari Ailus Cc: Linux Media Mailing List, Tsuchiya Yuto, Bingbu Cao, Yong Zhi, Tianshu Qiu On Fri, Oct 9, 2020 at 12:44 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Pad format can be accessed from user space. Serialise access to it. ... > /* Initialize miscellaneous variables */ > mutex_init(&q->lock); > + mutex_init(&q->subdev_lock); ... > mutex_destroy(&q->lock); > + mutex_destroy(&q->subdev_lock); A nit: perhaps reversed order? > mutex_destroy(&q->lock); > + mutex_destroy(&q->subdev_lock); Ditto. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/3] ipu3-cio2: Only allow setting valid mbus codes 2020-10-08 20:47 [PATCH 0/3] ipu3-cio2 format fixes Sakari Ailus 2020-10-08 20:47 ` [PATCH 1/3] ipu3-cio2: Return actual subdev format Sakari Ailus 2020-10-08 20:47 ` [PATCH 2/3] ipu3-cio2: Serialise access to pad format Sakari Ailus @ 2020-10-08 20:47 ` Sakari Ailus 2020-10-09 0:49 ` Laurent Pinchart 2 siblings, 1 reply; 11+ messages in thread From: Sakari Ailus @ 2020-10-08 20:47 UTC (permalink / raw) To: linux-media; +Cc: Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu 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(-) 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; } -- 2.27.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] ipu3-cio2: Only allow setting valid mbus codes 2020-10-08 20:47 ` [PATCH 3/3] ipu3-cio2: Only allow setting valid mbus codes Sakari Ailus @ 2020-10-09 0:49 ` Laurent Pinchart 2020-10-09 9:44 ` Sakari Ailus 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2020-10-09 0:49 UTC (permalink / raw) To: Sakari Ailus Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 3/3] ipu3-cio2: Only allow setting valid mbus codes 2020-10-09 0:49 ` Laurent Pinchart @ 2020-10-09 9:44 ` Sakari Ailus 0 siblings, 0 replies; 11+ messages in thread From: Sakari Ailus @ 2020-10-09 9:44 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-media, Tsuchiya Yuto, bingbu.cao, Yong Zhi, Tianshu Qiu 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. -- Sakari Ailus ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-10-09 16:16 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-08 20:47 [PATCH 0/3] ipu3-cio2 format fixes Sakari Ailus 2020-10-08 20:47 ` [PATCH 1/3] ipu3-cio2: Return actual subdev format Sakari Ailus 2020-10-09 0:39 ` Laurent Pinchart 2020-10-08 20:47 ` [PATCH 2/3] ipu3-cio2: Serialise access to pad format Sakari Ailus 2020-10-09 0:44 ` Laurent Pinchart 2020-10-09 9:43 ` Sakari Ailus 2020-10-09 10:39 ` Laurent Pinchart 2020-10-09 16:17 ` Andy Shevchenko 2020-10-08 20:47 ` [PATCH 3/3] ipu3-cio2: Only allow setting valid mbus codes Sakari Ailus 2020-10-09 0:49 ` Laurent Pinchart 2020-10-09 9:44 ` Sakari Ailus
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).