diff mbox series

[8/9] media: sun6i-isp: capture: Implement MC I/O with extended enum_fmt

Message ID 20230324151228.2778112-9-paul.kocialkowski@bootlin.com
State New
Headers
Series media: sun6i-csi/isp: Implement MC I/O support |

Commit Message

Paul Kocialkowski March 24, 2023, 3:12 p.m. UTC 
  This driver needs the media-controller API to operate and should not be
considered as a video-device-centric implementation.

Properly report the IO_MC device cap and extend the enum_fmt
implementation to support enumeration with an explicit mbus_code.

Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
---
 .../staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c    | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Jernej Škrabec March 25, 2023, 7:14 a.m. UTC | #1 
Dne petek, 24. marec 2023 ob 16:12:27 CET je Paul Kocialkowski napisal(a):
> This driver needs the media-controller API to operate and should not be
> considered as a video-device-centric implementation.
> 
> Properly report the IO_MC device cap and extend the enum_fmt
> implementation to support enumeration with an explicit mbus_code.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  .../staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c    | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> index 1595a9607775..5160b93b69ff 100644
> --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> @@ -439,6 +439,12 @@ static int sun6i_isp_capture_enum_fmt(struct file *file, void *private,
>  				      struct v4l2_fmtdesc *fmtdesc)
>  {
>  	u32 index = fmtdesc->index;
> +	u32 mbus_code = fmtdesc->mbus_code;

Nit: reverse christmas tree

Other than that:
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

> +
> +	if (mbus_code && !sun6i_isp_proc_format_find(mbus_code))
> +		return -EINVAL;
> +
> +	/* Capture format is independent from proc format. */
>  
>  	if (index >= ARRAY_SIZE(sun6i_isp_capture_formats))
>  		return -EINVAL;
> @@ -685,7 +691,8 @@ int sun6i_isp_capture_setup(struct sun6i_isp_device *isp_dev)
>  
>  	strscpy(video_dev->name, SUN6I_ISP_CAPTURE_NAME,
>  		sizeof(video_dev->name));
> -	video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
> +				 V4L2_CAP_IO_MC;
>  	video_dev->vfl_dir = VFL_DIR_RX;
>  	video_dev->release = video_device_release_empty;
>  	video_dev->fops = &sun6i_isp_capture_fops;
> -- 
> 2.39.2
> 
>
Laurent Pinchart March 25, 2023, 9:42 p.m. UTC | #2 
Hi Paul,

Thank you for the patch.

On Fri, Mar 24, 2023 at 04:12:27PM +0100, Paul Kocialkowski wrote:
> This driver needs the media-controller API to operate and should not be
> considered as a video-device-centric implementation.
> 
> Properly report the IO_MC device cap and extend the enum_fmt
> implementation to support enumeration with an explicit mbus_code.
> 
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> ---
>  .../staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c    | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> index 1595a9607775..5160b93b69ff 100644
> --- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> +++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
> @@ -439,6 +439,12 @@ static int sun6i_isp_capture_enum_fmt(struct file *file, void *private,
>  				      struct v4l2_fmtdesc *fmtdesc)
>  {
>  	u32 index = fmtdesc->index;
> +	u32 mbus_code = fmtdesc->mbus_code;
> +
> +	if (mbus_code && !sun6i_isp_proc_format_find(mbus_code))
> +		return -EINVAL;
> +

This doesn't look right. As far as I understand,
sun6i_isp_proc_format_find() looks up media bus codes for the ISP sink
pad. Here you enuemrate pixel formats of the ISP output, so the media
bus code given by userspace corresponds to the ISP source pad.

I've had a look at sun6i_isp_proc_set_fmt() to see what media bus codes
are used on the ISP output, and couldn't figure it out as it seems
incorrectly implemented :-) The function doesn't check format->pad.

> +	/* Capture format is independent from proc format. */
>  
>  	if (index >= ARRAY_SIZE(sun6i_isp_capture_formats))
>  		return -EINVAL;
> @@ -685,7 +691,8 @@ int sun6i_isp_capture_setup(struct sun6i_isp_device *isp_dev)
>  
>  	strscpy(video_dev->name, SUN6I_ISP_CAPTURE_NAME,
>  		sizeof(video_dev->name));
> -	video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
> +	video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
> +				 V4L2_CAP_IO_MC;
>  	video_dev->vfl_dir = VFL_DIR_RX;
>  	video_dev->release = video_device_release_empty;
>  	video_dev->fops = &sun6i_isp_capture_fops;
diff mbox series

Patch

diff --git a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
index 1595a9607775..5160b93b69ff 100644
--- a/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
+++ b/drivers/staging/media/sunxi/sun6i-isp/sun6i_isp_capture.c
@@ -439,6 +439,12 @@  static int sun6i_isp_capture_enum_fmt(struct file *file, void *private,
 				      struct v4l2_fmtdesc *fmtdesc)
 {
 	u32 index = fmtdesc->index;
+	u32 mbus_code = fmtdesc->mbus_code;
+
+	if (mbus_code && !sun6i_isp_proc_format_find(mbus_code))
+		return -EINVAL;
+
+	/* Capture format is independent from proc format. */
 
 	if (index >= ARRAY_SIZE(sun6i_isp_capture_formats))
 		return -EINVAL;
@@ -685,7 +691,8 @@  int sun6i_isp_capture_setup(struct sun6i_isp_device *isp_dev)
 
 	strscpy(video_dev->name, SUN6I_ISP_CAPTURE_NAME,
 		sizeof(video_dev->name));
-	video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
+	video_dev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
+				 V4L2_CAP_IO_MC;
 	video_dev->vfl_dir = VFL_DIR_RX;
 	video_dev->release = video_device_release_empty;
 	video_dev->fops = &sun6i_isp_capture_fops;