| Message ID | 20181004200402.15113-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | rcar-vin: add support for UDS (Up Down Scaler) | expand |
Hi Niklas, Thank you for the patch. On Thursday, 4 October 2018 23:04:00 EEST Niklas Söderlund wrote: > Instead of aligning the image width to match the image stride at stream > start time do so when configuring the format. This allows the format > width to strictly match the image stride which is useful when enabling > scaling on Gen3. But is this required ? Aren't there use cases where an image with a width not aligned with the stride requirements should be captured ? As long as the stride itself matches the hardware requirements, I don't see a reason to disallow that. > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 5 +---- > drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++ > 2 files changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c > b/drivers/media/platform/rcar-vin/rcar-dma.c index > 92323310f7352147..e752bc86e40153b1 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin) > if (vin->info->model != RCAR_GEN3) > rvin_crop_scale_comp_gen2(vin); > > - if (vin->format.pixelformat == V4L2_PIX_FMT_NV16) > - rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG); > - else > - rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG); > + rvin_write(vin, vin->format.width, VNIS_REG); > } > > /* > --------------------------------------------------------------------------- > -- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > dc77682b47857c97..94bc559a0cb1e47a 100644 > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin, > struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32)) > pix->pixelformat = RVIN_DEFAULT_FORMAT; > > + switch (pix->pixelformat) { > + case V4L2_PIX_FMT_NV16: > + pix->width = ALIGN(pix->width, 0x20); > + break; > + default: > + pix->width = ALIGN(pix->width, 0x10); > + break; > + } > + > switch (pix->field) { > case V4L2_FIELD_TOP: > case V4L2_FIELD_BOTTOM:
Hi Laurent, Thanks for your feedback. On 2018-10-04 23:11:50 +0300, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thursday, 4 October 2018 23:04:00 EEST Niklas Söderlund wrote: > > Instead of aligning the image width to match the image stride at stream > > start time do so when configuring the format. This allows the format > > width to strictly match the image stride which is useful when enabling > > scaling on Gen3. > > But is this required ? Aren't there use cases where an image with a width not > aligned with the stride requirements should be captured ? As long as the > stride itself matches the hardware requirements, I don't see a reason to > disallow that. Yes there is a use-case for that. And the rcar-vin driver is starting to reaching a point where the whole format handling for buffers, source format, croping and scaling needs to be rewritten to enable more valid use-cases. This fix is however in my view required at this time with the current driver design. If we keep aligning the width at stream on and enable the UDS it becomes apparent that when the alignment is needed the values for the stride register conflicts which how the scaling coefficients are calculated and the captured frame is distorted. My hope is to add upstream to support for the UDS, support for sequential field captures and some more output pixel formats. And once the driver feature complete on Gen3 come back and simplify and if possible align the Gen2 and Gen3 format handling which in part adds to the somewhat messy current state. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-dma.c | 5 +---- > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++ > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c > > b/drivers/media/platform/rcar-vin/rcar-dma.c index > > 92323310f7352147..e752bc86e40153b1 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > > @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin) > > if (vin->info->model != RCAR_GEN3) > > rvin_crop_scale_comp_gen2(vin); > > > > - if (vin->format.pixelformat == V4L2_PIX_FMT_NV16) > > - rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG); > > - else > > - rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG); > > + rvin_write(vin, vin->format.width, VNIS_REG); > > } > > > > /* > > --------------------------------------------------------------------------- > > -- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > > dc77682b47857c97..94bc559a0cb1e47a 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin, > > struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32)) > > pix->pixelformat = RVIN_DEFAULT_FORMAT; > > > > + switch (pix->pixelformat) { > > + case V4L2_PIX_FMT_NV16: > > + pix->width = ALIGN(pix->width, 0x20); > > + break; > > + default: > > + pix->width = ALIGN(pix->width, 0x10); > > + break; > > + } > > + > > switch (pix->field) { > > case V4L2_FIELD_TOP: > > case V4L2_FIELD_BOTTOM: > > -- > Regards, > > Laurent Pinchart > > >
Hi Niklas, On Thursday, 4 October 2018 23:29:51 EEST Niklas Söderlund wrote: > On 2018-10-04 23:11:50 +0300, Laurent Pinchart wrote: > > On Thursday, 4 October 2018 23:04:00 EEST Niklas Söderlund wrote: > >> Instead of aligning the image width to match the image stride at stream > >> start time do so when configuring the format. This allows the format > >> width to strictly match the image stride which is useful when enabling > >> scaling on Gen3. > > > > But is this required ? Aren't there use cases where an image with a width > > not aligned with the stride requirements should be captured ? As long as > > the stride itself matches the hardware requirements, I don't see a reason > > to disallow that. > > Yes there is a use-case for that. And the rcar-vin driver is starting to > reaching a point where the whole format handling for buffers, source > format, croping and scaling needs to be rewritten to enable more valid > use-cases. > > This fix is however in my view required at this time with the current > driver design. If we keep aligning the width at stream on and enable the > UDS it becomes apparent that when the alignment is needed the values for > the stride register conflicts which how the scaling coefficients are > calculated and the captured frame is distorted. > > My hope is to add upstream to support for the UDS, support for > sequential field captures and some more output pixel formats. And once > the driver feature complete on Gen3 come back and simplify and if > possible align the Gen2 and Gen3 format handling which in part adds to > the somewhat messy current state. I'd argue that it would be better to do it the other way around, but I won't block this patch series. You might however get angry e-mails from VIN users who all of a sudden realize that their use case stopped functioning. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > > > > drivers/media/platform/rcar-vin/rcar-dma.c | 5 +---- > > > drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++ > > > 2 files changed, 10 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c > > > b/drivers/media/platform/rcar-vin/rcar-dma.c index > > > 92323310f7352147..e752bc86e40153b1 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > > > @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin) > > > > > > if (vin->info->model != RCAR_GEN3) > > > > > > rvin_crop_scale_comp_gen2(vin); > > > > > > - if (vin->format.pixelformat == V4L2_PIX_FMT_NV16) > > > - rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG); > > > - else > > > - rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG); > > > + rvin_write(vin, vin->format.width, VNIS_REG); > > > > > > } > > > > > > /* > > > > > > ------------------------------------------------------------------------ > > > --- > > > -- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index > > > dc77682b47857c97..94bc559a0cb1e47a 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c > > > @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin, > > > struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32)) > > > > > > pix->pixelformat = RVIN_DEFAULT_FORMAT; > > > > > > + switch (pix->pixelformat) { > > > + case V4L2_PIX_FMT_NV16: > > > + pix->width = ALIGN(pix->width, 0x20); > > > + break; > > > + default: > > > + pix->width = ALIGN(pix->width, 0x10); > > > + break; > > > + } > > > + > > > > > > switch (pix->field) { > > > case V4L2_FIELD_TOP: > > > case V4L2_FIELD_BOTTOM:
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index 92323310f7352147..e752bc86e40153b1 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin) if (vin->info->model != RCAR_GEN3) rvin_crop_scale_comp_gen2(vin); - if (vin->format.pixelformat == V4L2_PIX_FMT_NV16) - rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG); - else - rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG); + rvin_write(vin, vin->format.width, VNIS_REG); } /* ----------------------------------------------------------------------------- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c index dc77682b47857c97..94bc559a0cb1e47a 100644 --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32)) pix->pixelformat = RVIN_DEFAULT_FORMAT; + switch (pix->pixelformat) { + case V4L2_PIX_FMT_NV16: + pix->width = ALIGN(pix->width, 0x20); + break; + default: + pix->width = ALIGN(pix->width, 0x10); + break; + } + switch (pix->field) { case V4L2_FIELD_TOP: case V4L2_FIELD_BOTTOM:
Instead of aligning the image width to match the image stride at stream start time do so when configuring the format. This allows the format width to strictly match the image stride which is useful when enabling scaling on Gen3. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/media/platform/rcar-vin/rcar-dma.c | 5 +---- drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++ 2 files changed, 10 insertions(+), 4 deletions(-)