* [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
* [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
* [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 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
* 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 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 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 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
* 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
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).