| Message ID | 20190808011850.21219-5-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Add support for V4L2_FIELD_ALTERNATE | expand |
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;
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
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 >
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;