Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call
@ 2020-07-31  9:29 Lad Prabhakar
  2020-08-01  9:04 ` Niklas
  0 siblings, 1 reply; 4+ messages in thread
From: Lad Prabhakar @ 2020-07-31  9:29 UTC (permalink / raw)
  To: Niklas, Mauro Carvalho Chehab, Hans Verkuil, Kieran Bingham,
	linux-media, linux-renesas-soc
  Cc: linux-kernel, Biju Das, Prabhakar, Lad Prabhakar

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.

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>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index f421e25..a9b13d9 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -319,6 +319,12 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
 	fmt_rect.width = vin->format.width;
 	fmt_rect.height = vin->format.height;
 
+	vin->crop.top = 0;
+	vin->crop.left = 0;
+	vin->crop.width = vin->format.width;
+	vin->crop.height = vin->format.height;
+	vin->compose = vin->crop;
+
 	v4l2_rect_map_inside(&vin->crop, &src_rect);
 	v4l2_rect_map_inside(&vin->compose, &fmt_rect);
 	vin->src_rect = src_rect;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call
  2020-07-31  9:29 [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call Lad Prabhakar
@ 2020-08-01  9:04 ` Niklas
  2020-08-03 18:11   ` Lad, Prabhakar
  0 siblings, 1 reply; 4+ messages in thread
From: Niklas @ 2020-08-01  9:04 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Mauro Carvalho Chehab, Hans Verkuil, Kieran Bingham, linux-media,
	linux-renesas-soc, linux-kernel, Biju Das, Prabhakar

Hi Lad,

Thanks for your work.

On 2020-07-31 10:29:05 +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.

This is clearly an inconsistency in the VIN driver that should be fixed.  
But I think the none-mc mode implements the correct behavior. That is 
that S_FMT should not modify the crop/compose rectangles other then make 
sure they don't go out of bounds. This is an area we tried to clarify in 
the past but I'm still not sure what the correct answer to.

> 
> 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>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index f421e25..a9b13d9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -319,6 +319,12 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
>  	fmt_rect.width = vin->format.width;
>  	fmt_rect.height = vin->format.height;
>  
> +	vin->crop.top = 0;
> +	vin->crop.left = 0;
> +	vin->crop.width = vin->format.width;
> +	vin->crop.height = vin->format.height;
> +	vin->compose = vin->crop;
> +
>  	v4l2_rect_map_inside(&vin->crop, &src_rect);
>  	v4l2_rect_map_inside(&vin->compose, &fmt_rect);
>  	vin->src_rect = src_rect;
> -- 
> 2.7.4
> 

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call
  2020-08-01  9:04 ` Niklas
@ 2020-08-03 18:11   ` Lad, Prabhakar
  2020-08-03 19:21     ` Niklas
  0 siblings, 1 reply; 4+ messages in thread
From: Lad, Prabhakar @ 2020-08-03 18:11 UTC (permalink / raw)
  To: Hans Verkuil, Laurent Pinchart
  Cc: Lad Prabhakar, Mauro Carvalho Chehab, Kieran Bingham,
	linux-media, Linux-Renesas, LKML, Biju Das, Niklas

Hi Hans,

On Sat, Aug 1, 2020 at 10:04 AM Niklas <niklas.soderlund@ragnatech.se> wrote:
>
> Hi Lad,
>
> Thanks for your work.
>
> On 2020-07-31 10:29:05 +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.
>
> This is clearly an inconsistency in the VIN driver that should be fixed.
> But I think the none-mc mode implements the correct behavior. That is
> that S_FMT should not modify the crop/compose rectangles other then make
> sure they don't go out of bounds. This is an area we tried to clarify in
> the past but I'm still not sure what the correct answer to.
>
What should be the exact behaviour of the bridge driver  for s_fmt
call. Should the crop/compose settings be updated for every s_fmt
callback or should they be only updated on s_selection callback.
Currently the non-mc rcar-vin doesnt update the crop/compose setting
in s_fmt callback due to which I see the above issue as mentioned.

Cheers,
Prabhakar

> >
> > 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>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index f421e25..a9b13d9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -319,6 +319,12 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> >       fmt_rect.width = vin->format.width;
> >       fmt_rect.height = vin->format.height;
> >
> > +     vin->crop.top = 0;
> > +     vin->crop.left = 0;
> > +     vin->crop.width = vin->format.width;
> > +     vin->crop.height = vin->format.height;
> > +     vin->compose = vin->crop;
> > +
> >       v4l2_rect_map_inside(&vin->crop, &src_rect);
> >       v4l2_rect_map_inside(&vin->compose, &fmt_rect);
> >       vin->src_rect = src_rect;
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call
  2020-08-03 18:11   ` Lad, Prabhakar
@ 2020-08-03 19:21     ` Niklas
  0 siblings, 0 replies; 4+ messages in thread
From: Niklas @ 2020-08-03 19:21 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Hans Verkuil, Laurent Pinchart, Lad Prabhakar,
	Mauro Carvalho Chehab, Kieran Bingham, linux-media,
	Linux-Renesas, LKML, Biju Das

Hi Lad, Hans,

On 2020-08-03 19:11:32 +0100, Lad, Prabhakar wrote:
> Hi Hans,
> 
> On Sat, Aug 1, 2020 at 10:04 AM Niklas <niklas.soderlund@ragnatech.se> wrote:
> >
> > Hi Lad,
> >
> > Thanks for your work.
> >
> > On 2020-07-31 10:29:05 +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.
> >
> > This is clearly an inconsistency in the VIN driver that should be fixed.
> > But I think the none-mc mode implements the correct behavior. That is
> > that S_FMT should not modify the crop/compose rectangles other then make
> > sure they don't go out of bounds. This is an area we tried to clarify in
> > the past but I'm still not sure what the correct answer to.
> >
> What should be the exact behaviour of the bridge driver  for s_fmt
> call. Should the crop/compose settings be updated for every s_fmt
> callback or should they be only updated on s_selection callback.
> Currently the non-mc rcar-vin doesnt update the crop/compose setting
> in s_fmt callback due to which I see the above issue as mentioned.

This is not entirely correct. It does update the crop and compose 
rectangles on s_fmt, it makes sure they are not out-of-bounds for the 
new format if it's accepted by s_fmt. See v4l2_rect_map_inside() calls 
in the snippet bellow.

That being said there is a difference how this is handled in the VIN 
driver between it's MC and non-MC modes and I would love to learn the 
correct mode of operation and seeing VIN being updated to doing it 
correct in both cases. Thanks Lad for dealing with this!

> 
> Cheers,
> Prabhakar
> 
> > >
> > > 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>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > index f421e25..a9b13d9 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > @@ -319,6 +319,12 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> > >       fmt_rect.width = vin->format.width;
> > >       fmt_rect.height = vin->format.height;
> > >
> > > +     vin->crop.top = 0;
> > > +     vin->crop.left = 0;
> > > +     vin->crop.width = vin->format.width;
> > > +     vin->crop.height = vin->format.height;
> > > +     vin->compose = vin->crop;
> > > +
> > >       v4l2_rect_map_inside(&vin->crop, &src_rect);
> > >       v4l2_rect_map_inside(&vin->compose, &fmt_rect);
> > >       vin->src_rect = src_rect;
> > > --
> > > 2.7.4
> > >
> >
> > --
> > Regards,
> > Niklas Söderlund

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-31  9:29 [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call Lad Prabhakar
2020-08-01  9:04 ` Niklas
2020-08-03 18:11   ` Lad, Prabhakar
2020-08-03 19:21     ` Niklas

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git