[v2,4/6] rcar-vin: Do not reset the crop and compose rectangles in s_fmt
diff mbox series

Message ID 20190808011850.21219-5-niklas.soderlund+renesas@ragnatech.se
State Superseded
Delegated to: Kieran Bingham
Headers show
Series
  • rcar-vin: Add support for V4L2_FIELD_ALTERNATE
Related show

Commit Message

Niklas Söderlund Aug. 8, 2019, 1:18 a.m. UTC
The crop and compose rectangles are reset when s_fmt is called
resulting in potentially valid rectangles being lost when updating the
format. Fix this by mapping the rectangles inside the new format.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Laurent Pinchart Aug. 8, 2019, 8:37 a.m. UTC | #1
Hi Niklas,

Thank you for the patch.

On Thu, Aug 08, 2019 at 03:18:48AM +0200, Niklas Söderlund wrote:
> The crop and compose rectangles are reset when s_fmt is called
> resulting in potentially valid rectangles being lost when updating the
> format.

Isn't this the expected behaviour ?

> Fix this by mapping the rectangles inside the new format.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -279,8 +279,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
>  		return ret;
>  
>  	vin->format = f->fmt.pix;
> -	vin->crop = crop;
> -	vin->compose = compose;
> +	v4l2_rect_map_inside(&vin->crop, &crop);
> +	v4l2_rect_map_inside(&vin->compose, &compose);
>  	vin->src_rect = crop;
>  
>  	return 0;
Niklas Söderlund Aug. 8, 2019, 1:35 p.m. UTC | #2
Hi Laurent,

Thanks for your feedback.

On 2019-08-08 11:37:51 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, Aug 08, 2019 at 03:18:48AM +0200, Niklas Söderlund wrote:
> > The crop and compose rectangles are reset when s_fmt is called
> > resulting in potentially valid rectangles being lost when updating the
> > format.
> 
> Isn't this the expected behaviour ?

I though so to at first but I had a nagging feeling as I was not sure.  
So I went and looked at vivid and it does do map the old crop/compose 
rectangles inside the new limits vivid_s_fmt_vid_cap().

Please note that the variable names in this patch are horrible and are 
fixed later in this series. In essence the current crop rectangle is 
mapped inside the size of the video coming from the sensor and the 
compose rectangle is mapped inside the new format set on the VIN video 
device.

I'm open to dropping this patch, I just want this driver to get this 
part right so I mimic vivid.

> 
> > Fix this by mapping the rectangles inside the new format.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -279,8 +279,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
> >  		return ret;
> >  
> >  	vin->format = f->fmt.pix;
> > -	vin->crop = crop;
> > -	vin->compose = compose;
> > +	v4l2_rect_map_inside(&vin->crop, &crop);
> > +	v4l2_rect_map_inside(&vin->compose, &compose);
> >  	vin->src_rect = crop;
> >  
> >  	return 0;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Hans Verkuil Aug. 28, 2019, 10:25 a.m. UTC | #3
On 8/8/19 3:35 PM, Niklas Söderlund wrote:
> Hi Laurent,
> 
> Thanks for your feedback.
> 
> On 2019-08-08 11:37:51 +0300, Laurent Pinchart wrote:
>> Hi Niklas,
>>
>> Thank you for the patch.
>>
>> On Thu, Aug 08, 2019 at 03:18:48AM +0200, Niklas Söderlund wrote:
>>> The crop and compose rectangles are reset when s_fmt is called
>>> resulting in potentially valid rectangles being lost when updating the
>>> format.
>>
>> Isn't this the expected behaviour ?
> 
> I though so to at first but I had a nagging feeling as I was not sure.  
> So I went and looked at vivid and it does do map the old crop/compose 
> rectangles inside the new limits vivid_s_fmt_vid_cap().
> 
> Please note that the variable names in this patch are horrible and are 
> fixed later in this series. In essence the current crop rectangle is 
> mapped inside the size of the video coming from the sensor and the 
> compose rectangle is mapped inside the new format set on the VIN video 
> device.

The only ioctls that completely reset everything are S_STD and S_DV_TIMINGS.

Ioctls such as S_FMT and S_SELECTION will try to keep as much of the
pre-existing configuration as possible.

So this patch is correct.

Regards,

	Hans

> 
> I'm open to dropping this patch, I just want this driver to get this 
> part right so I mimic vivid.
> 
>>
>>> Fix this by mapping the rectangles inside the new format.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> @@ -279,8 +279,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
>>>  		return ret;
>>>  
>>>  	vin->format = f->fmt.pix;
>>> -	vin->crop = crop;
>>> -	vin->compose = compose;
>>> +	v4l2_rect_map_inside(&vin->crop, &crop);
>>> +	v4l2_rect_map_inside(&vin->compose, &compose);
>>>  	vin->src_rect = crop;
>>>  
>>>  	return 0;
>>
>> -- 
>> Regards,
>>
>> Laurent Pinchart
>

Patch
diff mbox series

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 8b30267f1636aaf1..5dcd787a9cf96ac9 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -279,8 +279,8 @@  static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
 		return ret;
 
 	vin->format = f->fmt.pix;
-	vin->crop = crop;
-	vin->compose = compose;
+	v4l2_rect_map_inside(&vin->crop, &crop);
+	v4l2_rect_map_inside(&vin->compose, &compose);
 	vin->src_rect = crop;
 
 	return 0;