* Re: [PATCH v2] media: rcar-vin: Update crop and compose settings for every s_fmt call
2020-09-13 18:21 [PATCH v2] media: rcar-vin: Update crop and compose settings for every s_fmt call Lad Prabhakar
@ 2020-09-14 23:48 ` Laurent Pinchart
2020-09-15 0:04 ` Niklas Söderlund
2020-09-15 9:07 ` Hans Verkuil
2 siblings, 0 replies; 4+ messages in thread
From: Laurent Pinchart @ 2020-09-14 23:48 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Niklas Söderlund, Hans Verkuil, Mauro Carvalho Chehab,
Kieran Bingham, linux-media, linux-renesas-soc, linux-kernel,
Biju Das, Prabhakar
Hi Prabhakar,
Thank you for the patch.
On Sun, Sep 13, 2020 at 07:21:40PM +0100, Lad Prabhakar wrote:
> The crop and compose settings for VIN in non mc mode werent updated
s/mc/MC/
s/werent/weren't/
> in s_fmt call this resulted in captured images being clipped.
>
> With the below sequence on the third capture where size is set to
> 640x480 resulted in clipped image of size 320x240.
>
> high(640x480) -> low (320x240) -> high (640x480)
>
> This patch makes sure the VIN crop and compose settings are updated.
>
> Fixes: 104464f573d ("media: rcar-vin: Do not reset the crop and compose rectangles in s_fmt")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> Changes for v2:
> * Dropped redundant code mapping crop and compose rects
>
> v1 - https://lkml.org/lkml/2020/7/31/364
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 0e066bba747e..1bd59a8453b4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -305,7 +305,7 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> struct v4l2_format *f)
> {
> struct rvin_dev *vin = video_drvdata(file);
> - struct v4l2_rect fmt_rect, src_rect;
> + struct v4l2_rect src_rect;
> int ret;
>
> if (vb2_is_busy(&vin->queue))
> @@ -317,14 +317,11 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> return ret;
>
> vin->format = f->fmt.pix;
> -
> - fmt_rect.top = 0;
> - fmt_rect.left = 0;
> - fmt_rect.width = vin->format.width;
> - fmt_rect.height = vin->format.height;
> -
> - v4l2_rect_map_inside(&vin->crop, &src_rect);
> - v4l2_rect_map_inside(&vin->compose, &fmt_rect);
I would add a comment here:
/* Reset the crop and compose rectangles to default values. */
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> + vin->crop.top = 0;
> + vin->crop.left = 0;
> + vin->crop.width = vin->format.width;
> + vin->crop.height = vin->format.height;
> + vin->compose = vin->crop;
> vin->src_rect = src_rect;
>
> return 0;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] media: rcar-vin: Update crop and compose settings for every s_fmt call
2020-09-13 18:21 [PATCH v2] media: rcar-vin: Update crop and compose settings for every s_fmt call Lad Prabhakar
2020-09-14 23:48 ` Laurent Pinchart
@ 2020-09-15 0:04 ` Niklas Söderlund
2020-09-15 9:07 ` Hans Verkuil
2 siblings, 0 replies; 4+ messages in thread
From: Niklas Söderlund @ 2020-09-15 0:04 UTC (permalink / raw)
To: Lad Prabhakar
Cc: Laurent Pinchart, Hans Verkuil, Mauro Carvalho Chehab,
Kieran Bingham, linux-media, linux-renesas-soc, linux-kernel,
Biju Das, Prabhakar
Hi Lad,
Thanks for your work.
On 2020-09-13 19:21:40 +0100, Lad Prabhakar wrote:
> The crop and compose settings for VIN in non mc mode werent updated
> in s_fmt call this resulted in captured images being clipped.
>
> With the below sequence on the third capture where size is set to
> 640x480 resulted in clipped image of size 320x240.
>
> high(640x480) -> low (320x240) -> high (640x480)
>
> This patch makes sure the VIN crop and compose settings are updated.
With this change I get failures with v4l2-compliance I don't have
before. I think we need to align this the other way around, copy the
non-MC behavior to the MC S_FTM implementation.
# v4l2-compliance -d /dev/video28
v4l2-compliance SHA: c7f03287bbd64c168975e7ff3192e6fd3b507686, 32 bits, 32-bit time_t
Compliance test for rcar_vin device /dev/video28:
Driver Info:
Driver name : rcar_vin
Card type : R_Car_VIN
Bus info : platform:e6ef1000.video
Driver version : 5.9.0
Capabilities : 0x85200001
Video Capture
Read/Write
Streaming
Extended Pix Format
Device Capabilities
Device Caps : 0x05200001
Video Capture
Read/Write
Streaming
Extended Pix Format
Required ioctls:
test VIDIOC_QUERYCAP: OK
Allow for multiple opens:
test second /dev/video28 open: OK
test VIDIOC_QUERYCAP: OK
test VIDIOC_G/S_PRIORITY: OK
test for unlimited opens: OK
test invalid ioctls: OK
Debug ioctls:
test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
test VIDIOC_LOG_STATUS: OK
Input ioctls:
test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
test VIDIOC_ENUMAUDIO: OK (Not Supported)
test VIDIOC_G/S/ENUMINPUT: OK
test VIDIOC_G/S_AUDIO: OK (Not Supported)
Inputs: 1 Audio Inputs: 0 Tuners: 0
Output ioctls:
test VIDIOC_G/S_MODULATOR: OK (Not Supported)
test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
test VIDIOC_ENUMAUDOUT: OK (Not Supported)
test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
test VIDIOC_G/S_AUDOUT: OK (Not Supported)
Outputs: 0 Audio Outputs: 0 Modulators: 0
Input/Output configuration ioctls:
test VIDIOC_ENUM/G/S/QUERY_STD: OK
test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
test VIDIOC_G/S_EDID: OK (Not Supported)
Control ioctls (Input 0):
test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
test VIDIOC_QUERYCTRL: OK
test VIDIOC_G/S_CTRL: OK
test VIDIOC_G/S/TRY_EXT_CTRLS: OK
test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
Standard Controls: 6 Private Controls: 1
Format ioctls (Input 0):
test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
test VIDIOC_G/S_PARM: OK
test VIDIOC_G_FBUF: OK (Not Supported)
test VIDIOC_G_FMT: OK
test VIDIOC_TRY_FMT: OK
test VIDIOC_S_FMT: OK
test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
fail: v4l2-test-formats.cpp(1544): !rect_is_inside(&sel_crop.r, &sel_bounds.r)
fail: v4l2-test-formats.cpp(1649): testBasicCrop(node, V4L2_BUF_TYPE_VIDEO_CAPTURE)
test Cropping: FAIL
test Composing: OK
test Scaling: OK
Codec ioctls (Input 0):
test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
test VIDIOC_G_ENC_INDEX: OK (Not Supported)
test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
Buffer ioctls (Input 0):
test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
test VIDIOC_EXPBUF: OK
test Requests: OK (Not Supported)
Total for rcar_vin device /dev/video28: 45, Succeeded: 44, Failed: 1, Warnings: 0
Without this patch all test pass.
>
> Fixes: 104464f573d ("media: rcar-vin: Do not reset the crop and compose rectangles in s_fmt")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> Changes for v2:
> * Dropped redundant code mapping crop and compose rects
>
> v1 - https://lkml.org/lkml/2020/7/31/364
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 0e066bba747e..1bd59a8453b4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -305,7 +305,7 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> struct v4l2_format *f)
> {
> struct rvin_dev *vin = video_drvdata(file);
> - struct v4l2_rect fmt_rect, src_rect;
> + struct v4l2_rect src_rect;
> int ret;
>
> if (vb2_is_busy(&vin->queue))
> @@ -317,14 +317,11 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> return ret;
>
> vin->format = f->fmt.pix;
> -
> - fmt_rect.top = 0;
> - fmt_rect.left = 0;
> - fmt_rect.width = vin->format.width;
> - fmt_rect.height = vin->format.height;
> -
> - v4l2_rect_map_inside(&vin->crop, &src_rect);
> - v4l2_rect_map_inside(&vin->compose, &fmt_rect);
> + vin->crop.top = 0;
> + vin->crop.left = 0;
> + vin->crop.width = vin->format.width;
> + vin->crop.height = vin->format.height;
> + vin->compose = vin->crop;
> vin->src_rect = src_rect;
>
> return 0;
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] media: rcar-vin: Update crop and compose settings for every s_fmt call
2020-09-13 18:21 [PATCH v2] media: rcar-vin: Update crop and compose settings for every s_fmt call Lad Prabhakar
2020-09-14 23:48 ` Laurent Pinchart
2020-09-15 0:04 ` Niklas Söderlund
@ 2020-09-15 9:07 ` Hans Verkuil
2 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2020-09-15 9:07 UTC (permalink / raw)
To: Lad Prabhakar, Niklas Söderlund, Laurent Pinchart,
Mauro Carvalho Chehab, Kieran Bingham, linux-media,
linux-renesas-soc
Cc: linux-kernel, Biju Das, Prabhakar
On 13/09/2020 20:21, Lad Prabhakar wrote:
> The crop and compose settings for VIN in non mc mode werent updated
> in s_fmt call this resulted in captured images being clipped.
>
> With the below sequence on the third capture where size is set to
> 640x480 resulted in clipped image of size 320x240.
>
> high(640x480) -> low (320x240) -> high (640x480)
>
> This patch makes sure the VIN crop and compose settings are updated.
I'm not sure the original behavior was wrong at all.
When calling S_FMT(320x240) it should force the crop and compose rectangles
into 320x240, but when calling S_FMT(640x480) the crop and compose rectangles
do not need to be modified and are kept. It is up to userspace to update those
crop/compose rectangles.
Calling S_FMT must, however, update the crop/compose bounds/default rectangles
where applicable.
Note that the crop coordinates are against the video source resolution, *not*
the format width/height. So this patch is definitely wrong in that respect.
Regards,
Hans
>
> Fixes: 104464f573d ("media: rcar-vin: Do not reset the crop and compose rectangles in s_fmt")
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
> Changes for v2:
> * Dropped redundant code mapping crop and compose rects
>
> v1 - https://lkml.org/lkml/2020/7/31/364
> ---
> drivers/media/platform/rcar-vin/rcar-v4l2.c | 15 ++++++---------
> 1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 0e066bba747e..1bd59a8453b4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -305,7 +305,7 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> struct v4l2_format *f)
> {
> struct rvin_dev *vin = video_drvdata(file);
> - struct v4l2_rect fmt_rect, src_rect;
> + struct v4l2_rect src_rect;
> int ret;
>
> if (vb2_is_busy(&vin->queue))
> @@ -317,14 +317,11 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> return ret;
>
> vin->format = f->fmt.pix;
> -
> - fmt_rect.top = 0;
> - fmt_rect.left = 0;
> - fmt_rect.width = vin->format.width;
> - fmt_rect.height = vin->format.height;
> -
> - v4l2_rect_map_inside(&vin->crop, &src_rect);
> - v4l2_rect_map_inside(&vin->compose, &fmt_rect);
> + vin->crop.top = 0;
> + vin->crop.left = 0;
> + vin->crop.width = vin->format.width;
> + vin->crop.height = vin->format.height;
> + vin->compose = vin->crop;
> vin->src_rect = src_rect;
>
> return 0;
>
^ permalink raw reply [flat|nested] 4+ messages in thread