* [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; 10+ 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 related [flat|nested] 10+ 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; 10+ 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] 10+ 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; 10+ 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] 10+ 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 2020-08-13 14:24 ` Lad, Prabhakar 2020-08-19 14:08 ` Hans Verkuil 0 siblings, 2 replies; 10+ 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] 10+ messages in thread
* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call 2020-08-03 19:21 ` Niklas @ 2020-08-13 14:24 ` Lad, Prabhakar 2020-08-19 14:08 ` Hans Verkuil 1 sibling, 0 replies; 10+ messages in thread From: Lad, Prabhakar @ 2020-08-13 14:24 UTC (permalink / raw) To: Hans Verkuil, Laurent Pinchart Cc: Lad Prabhakar, Mauro Carvalho Chehab, Kieran Bingham, linux-media, Linux-Renesas, Niklas, LKML, Biju Das Hi Hans and Laurent, On Mon, Aug 3, 2020 at 8:21 PM Niklas <niklas.soderlund@ragnatech.se> wrote: > > 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! > Can I have your feedback on this please. Cheers, Prabhakar > > > > 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] 10+ messages in thread
* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call 2020-08-03 19:21 ` Niklas 2020-08-13 14:24 ` Lad, Prabhakar @ 2020-08-19 14:08 ` Hans Verkuil 2020-09-03 14:53 ` Lad, Prabhakar 1 sibling, 1 reply; 10+ messages in thread From: Hans Verkuil @ 2020-08-19 14:08 UTC (permalink / raw) To: Niklas, Lad, Prabhakar Cc: Laurent Pinchart, Lad Prabhakar, Mauro Carvalho Chehab, Kieran Bingham, linux-media, Linux-Renesas, LKML, Biju Das On 03/08/2020 21:21, Niklas wrote: > 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. For non-mc mode s_fmt must update any crop/compose rectangles to ensure that they are not out-of-bounds. But for mc mode the validation is done when you start streaming, so I think s_fmt won't make any changes in that mode. Double-check that with Laurent, though... Regards, Hans > > 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call 2020-08-19 14:08 ` Hans Verkuil @ 2020-09-03 14:53 ` Lad, Prabhakar 2020-09-04 2:25 ` Laurent Pinchart 0 siblings, 1 reply; 10+ messages in thread From: Lad, Prabhakar @ 2020-09-03 14:53 UTC (permalink / raw) To: Niklas, Laurent Pinchart, Hans Verkuil Cc: Lad Prabhakar, Mauro Carvalho Chehab, Kieran Bingham, linux-media, Linux-Renesas, LKML, Biju Das On Wed, Aug 19, 2020 at 3:08 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > On 03/08/2020 21:21, Niklas wrote: > > 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. > > For non-mc mode s_fmt must update any crop/compose rectangles to ensure that > they are not out-of-bounds. But for mc mode the validation is done when you > start streaming, so I think s_fmt won't make any changes in that mode. > Thank you Hans. > Double-check that with Laurent, though... > Niklas/Laurent - How do we proceed on this ? Cheers, Prabhakar ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call 2020-09-03 14:53 ` Lad, Prabhakar @ 2020-09-04 2:25 ` Laurent Pinchart 2020-09-06 19:12 ` Lad, Prabhakar 0 siblings, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2020-09-04 2:25 UTC (permalink / raw) To: Lad, Prabhakar Cc: Niklas, Laurent Pinchart, Hans Verkuil, Lad Prabhakar, Mauro Carvalho Chehab, Kieran Bingham, linux-media, Linux-Renesas, LKML, Biju Das Hi Prabhakar, On Thu, Sep 03, 2020 at 03:53:18PM +0100, Lad, Prabhakar wrote: > On Wed, Aug 19, 2020 at 3:08 PM Hans Verkuil wrote: > > On 03/08/2020 21:21, Niklas wrote: > > > On 2020-08-03 19:11:32 +0100, Lad, Prabhakar wrote: > > >> On Sat, Aug 1, 2020 at 10:04 AM Niklas wrote: > > >>> 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. > > > > For non-mc mode s_fmt must update any crop/compose rectangles to ensure that > > they are not out-of-bounds. But for mc mode the validation is done when you > > start streaming, so I think s_fmt won't make any changes in that mode. > > Thank you Hans. > > > Double-check that with Laurent, though... > > Niklas/Laurent - How do we proceed on this ? MC devices rely on userspace to propagate formats between entities, and on kernelspace to propagate formats within entities. This is documented in https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/dev-subdev.html. The configuration of an entity (formats and selection rectangles) must be valid at all times. Subdev drivers should thus either adjust or reset the crop and selection rectangles. The specification isn't clear on which behaviour should be implemented, the only related text is "Sub-devices that scale frames using variable scaling factors should reset the scale factors to default values when sink pads formats are modified. If the 1:1 scaling ratio is supported, this means that source pads formats should be reset to the sink pads formats." I would recommend resetting as the default behaviour. In any case, adjustements are needed to ensure that the configuration remains valid. -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call 2020-09-04 2:25 ` Laurent Pinchart @ 2020-09-06 19:12 ` Lad, Prabhakar 2020-09-08 15:35 ` Niklas 0 siblings, 1 reply; 10+ messages in thread From: Lad, Prabhakar @ 2020-09-06 19:12 UTC (permalink / raw) To: Laurent Pinchart Cc: Niklas, Laurent Pinchart, Hans Verkuil, Lad Prabhakar, Mauro Carvalho Chehab, Kieran Bingham, linux-media, Linux-Renesas, LKML, Biju Das Hi Laurent, On Fri, Sep 4, 2020 at 3:25 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > On Thu, Sep 03, 2020 at 03:53:18PM +0100, Lad, Prabhakar wrote: > > On Wed, Aug 19, 2020 at 3:08 PM Hans Verkuil wrote: > > > On 03/08/2020 21:21, Niklas wrote: > > > > On 2020-08-03 19:11:32 +0100, Lad, Prabhakar wrote: > > > >> On Sat, Aug 1, 2020 at 10:04 AM Niklas wrote: > > > >>> 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. > > > > > > For non-mc mode s_fmt must update any crop/compose rectangles to ensure that > > > they are not out-of-bounds. But for mc mode the validation is done when you > > > start streaming, so I think s_fmt won't make any changes in that mode. > > > > Thank you Hans. > > > > > Double-check that with Laurent, though... > > > > Niklas/Laurent - How do we proceed on this ? > > MC devices rely on userspace to propagate formats between entities, and > on kernelspace to propagate formats within entities. This is documented > in https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/dev-subdev.html. > The configuration of an entity (formats and selection rectangles) must > be valid at all times. Subdev drivers should thus either adjust or reset > the crop and selection rectangles. The specification isn't clear on > which behaviour should be implemented, the only related text is > > "Sub-devices that scale frames using variable scaling factors should > reset the scale factors to default values when sink pads formats are > modified. If the 1:1 scaling ratio is supported, this means that source > pads formats should be reset to the sink pads formats." > > I would recommend resetting as the default behaviour. In any case, > adjustements are needed to ensure that the configuration remains valid. > In that case can I have your Ack to the patch please. Cheers, Prabhakar ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: rcar-vin: Update crop and compose settings for every s_fmt call 2020-09-06 19:12 ` Lad, Prabhakar @ 2020-09-08 15:35 ` Niklas 0 siblings, 0 replies; 10+ messages in thread From: Niklas @ 2020-09-08 15:35 UTC (permalink / raw) To: Lad, Prabhakar Cc: Laurent Pinchart, Laurent Pinchart, Hans Verkuil, Lad Prabhakar, Mauro Carvalho Chehab, Kieran Bingham, linux-media, Linux-Renesas, LKML, Biju Das Hi Lad, On 2020-09-06 20:12:35 +0100, Lad, Prabhakar wrote: > Hi Laurent, > > On Fri, Sep 4, 2020 at 3:25 AM Laurent Pinchart > <laurent.pinchart@ideasonboard.com> wrote: > > > > Hi Prabhakar, > > > > On Thu, Sep 03, 2020 at 03:53:18PM +0100, Lad, Prabhakar wrote: > > > On Wed, Aug 19, 2020 at 3:08 PM Hans Verkuil wrote: > > > > On 03/08/2020 21:21, Niklas wrote: > > > > > On 2020-08-03 19:11:32 +0100, Lad, Prabhakar wrote: > > > > >> On Sat, Aug 1, 2020 at 10:04 AM Niklas wrote: > > > > >>> 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. > > > > > > > > For non-mc mode s_fmt must update any crop/compose rectangles to ensure that > > > > they are not out-of-bounds. But for mc mode the validation is done when you > > > > start streaming, so I think s_fmt won't make any changes in that mode. > > > > > > Thank you Hans. > > > > > > > Double-check that with Laurent, though... > > > > > > Niklas/Laurent - How do we proceed on this ? > > > > MC devices rely on userspace to propagate formats between entities, and > > on kernelspace to propagate formats within entities. This is documented > > in https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/dev-subdev.html. > > The configuration of an entity (formats and selection rectangles) must > > be valid at all times. Subdev drivers should thus either adjust or reset > > the crop and selection rectangles. The specification isn't clear on > > which behaviour should be implemented, the only related text is > > > > "Sub-devices that scale frames using variable scaling factors should > > reset the scale factors to default values when sink pads formats are > > modified. If the 1:1 scaling ratio is supported, this means that source > > pads formats should be reset to the sink pads formats." > > > > I would recommend resetting as the default behaviour. In any case, > > adjustements are needed to ensure that the configuration remains valid. > > > In that case can I have your Ack to the patch please. If this is the approach we wish to take here you should remove the code above and bellow the added block as it becomes redundant whit this change. > > Cheers, > Prabhakar -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-09-08 20:00 UTC | newest] Thread overview: 10+ 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 2020-08-13 14:24 ` Lad, Prabhakar 2020-08-19 14:08 ` Hans Verkuil 2020-09-03 14:53 ` Lad, Prabhakar 2020-09-04 2:25 ` Laurent Pinchart 2020-09-06 19:12 ` Lad, Prabhakar 2020-09-08 15:35 ` Niklas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).