mbox series

[0/5] ipu3-cio2 format fixes

Message ID 20201009150756.3397-1-sakari.ailus@linux.intel.com
Headers show
Series ipu3-cio2 format fixes | expand

Message

Sakari Ailus Oct. 9, 2020, 3:07 p.m. UTC
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.

since v1:

- Validate try format, too

- Set field in subdev format to V4L2_FIELD_NONE

- Add a comment explaining the lock

- Make values that should be unsigned, unsigned

Sakari Ailus (5):
  ipu3-cio2: Return actual subdev format
  ipu3-cio2: Serialise access to pad format
  ipu3-cio2: Use unsigned values where appropriate
  ipu3-cio2: Validate mbus format in setting subdev format
  ipu3-cio2: Make the field on subdev format V4L2_FIELD_NONE

 drivers/media/pci/intel/ipu3/ipu3-cio2.c |  60 +++++----
 drivers/media/pci/intel/ipu3/ipu3-cio2.h | 157 ++++++++++++-----------
 2 files changed, 111 insertions(+), 106 deletions(-)

Comments

Sakari Ailus Oct. 9, 2020, 3:08 p.m. UTC | #1
On Fri, Oct 09, 2020 at 06:07:51PM +0300, Sakari Ailus wrote:
> 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.

> 

> since v1:

> 

> - Validate try format, too

> 

> - Set field in subdev format to V4L2_FIELD_NONE

> 

> - Add a comment explaining the lock

> 

> - Make values that should be unsigned, unsigned


This is obviously v2. v1 is here:

<URL:https://lore.kernel.org/linux-media/20201008204747.26320-1-sakari.ailus@linux.intel.com/T/#t>

-- 
Sakari Ailus
Andy Shevchenko Oct. 9, 2020, 4:19 p.m. UTC | #2
On Fri, Oct 9, 2020 at 6:10 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Pad format can be accessed from user space. Serialise access to it.

Same nit-picks as per v1, feel free to ignore it.
Andy Shevchenko Oct. 9, 2020, 4:23 p.m. UTC | #3
On Fri, Oct 9, 2020 at 6:11 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
>
> On Fri, Oct 09, 2020 at 06:07:51PM +0300, Sakari Ailus wrote:
> > 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.
> >
> > since v1:
> >
> > - Validate try format, too
> >
> > - Set field in subdev format to V4L2_FIELD_NONE
> >
> > - Add a comment explaining the lock
> >
> > - Make values that should be unsigned, unsigned
>
> This is obviously v2. v1 is here:

v2 is good enough, but few nit-picks here and there. In any case
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

>
> <URL:https://lore.kernel.org/linux-media/20201008204747.26320-1-sakari.ailus@linux.intel.com/T/#t>
>
> --
> Sakari Ailus
Laurent Pinchart Oct. 9, 2020, 5:10 p.m. UTC | #4
Hi Sakari,

Thank you for the patch.

On Fri, Oct 09, 2020 at 06:07:56PM +0300, Sakari Ailus wrote:
> The ipu3-cio2 doesn't make use of the field and this is reflected in V4L2
> buffers as well as the try format. Do this in active format, too.
> 
> Fixes: c2a6a07afe4a ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 9726091c9985..a821c40627f9 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -1284,6 +1284,7 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,
>  
>  	fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);
>  	fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);
> +	fmt->format.field = V4L2_FIELD_NONE;
>  
>  	mutex_lock(&q->subdev_lock);
>  	*mbus = fmt->format;
Cao, Bingbu Oct. 12, 2020, 1:34 a.m. UTC | #5
Sakari, thank you for the patch.

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
Cao, Bingbu Oct. 12, 2020, 1:44 a.m. UTC | #6
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>


________________________
BRs,  
Bingbu Cao                          


> -----Original Message-----

> From: Sakari Ailus <sakari.ailus@linux.intel.com>

> Sent: Friday, October 9, 2020 11:08 PM

> To: linux-media@vger.kernel.org

> Cc: Tsuchiya Yuto <kitakar@gmail.com>; Cao, Bingbu <bingbu.cao@intel.com>;

> Zhi, Yong <yong.zhi@intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>;

> laurent.pinchart@ideasonboard.com

> Subject: [PATCH 5/5] ipu3-cio2: Make the field on subdev format

> V4L2_FIELD_NONE

> 

> The ipu3-cio2 doesn't make use of the field and this is reflected in V4L2

> buffers as well as the try format. Do this in active format, too.

> 

> 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 | 1 +

>  1 file changed, 1 insertion(+)

> 

> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c

> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

> index 9726091c9985..a821c40627f9 100644

> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c

> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c

> @@ -1284,6 +1284,7 @@ static int cio2_subdev_set_fmt(struct v4l2_subdev *sd,

> 

>  	fmt->format.width = min(fmt->format.width, CIO2_IMAGE_MAX_WIDTH);

>  	fmt->format.height = min(fmt->format.height, CIO2_IMAGE_MAX_LENGTH);

> +	fmt->format.field = V4L2_FIELD_NONE;

> 

>  	mutex_lock(&q->subdev_lock);

>  	*mbus = fmt->format;

> --

> 2.27.0
Cao, Bingbu Oct. 12, 2020, 1:50 a.m. UTC | #7
Sakari, thanks for your patch.

Reviewed-by: Bingbu Cao <Bingbu.cao@intel.com>
Sakari Ailus Oct. 12, 2020, 8:13 a.m. UTC | #8
Hi Andy,

On Fri, Oct 09, 2020 at 07:23:57PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 9, 2020 at 6:11 PM Sakari Ailus <sakari.ailus@iki.fi> wrote:
> >
> > On Fri, Oct 09, 2020 at 06:07:51PM +0300, Sakari Ailus wrote:
> > > 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.
> > >
> > > since v1:
> > >
> > > - Validate try format, too
> > >
> > > - Set field in subdev format to V4L2_FIELD_NONE
> > >
> > > - Add a comment explaining the lock
> > >
> > > - Make values that should be unsigned, unsigned
> >
> > This is obviously v2. v1 is here:
> 
> v2 is good enough, but few nit-picks here and there. In any case
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks!