| Message ID | 1526032781-14319-6-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/11/18 11:59, Jacopo Mondi wrote: > Since commit (015060cb "media: rcar-vin: enable field toggle after a set > number of lines for Gen3) the VIN generates an internal field signal > toggle after a fixed number of received lines, and uses the internal > field signal to drive capture operations. When capturing from digital > input, using FTEH driven field signal toggling messes up the received > image sizes. Fall back to use FTEV driven signal toggling when capturing > from digital input. > > As explained in the comment, this disables buffer overflow protection > for digital input capture, for which the FOE overflow might be used in > future. I don't know the details of the hardware, but this sounds dangerous. You should know that with HDMI input it is perfectly possible that you get more data than you should. I.e. instead of 1080 lines (assuming full HD) you might get more lines. This happens if the vertical sync is missed due to pin bounce when connecting a source. Other reasons for this are flaky signals, bad clocks, etc. It's rare, but it really happens. A good DMA engine will refuse to write more than fits in the buffer. If you disable that, then you will get overflows eventually. The reality with HDMI input is that you should never assume clean valid data. You do not control the source and it can send anything it likes. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/platform/rcar-vin/rcar-dma.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > index ea7a120..8dc3455 100644 > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > @@ -685,11 +685,27 @@ static int rvin_setup(struct rvin_dev *vin) > break; > } > > - if (vin->info->model == RCAR_GEN3) { > + if (vin->info->model == RCAR_GEN3 && > + vin->mbus_cfg.type == V4L2_MBUS_CSI2) { > /* Enable HSYNC Field Toggle mode after height HSYNC inputs. */ > lines = vin->format.height / (halfsize ? 2 : 1); > dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines); > vin_dbg(vin, "Field Toogle after %u lines\n", lines); Typo: Toogle -> Toggle > + } else if (vin->info->model == RCAR_GEN3 && > + vin->mbus_cfg.type == V4L2_MBUS_PARALLEL) { > + /* > + * FIXME > + * Section 26.3.17 specifies that for digital input there's no > + * need to program FTEH or FTEV to generate internal > + * field toggle signal to driver capture. Although when > + * running on GEN3 with digital input no EFE interrupt is ever > + * generated, and we need to rely on FTEV driven field signal > + * toggling, as using FTEH as in the CSI-2 case, messes up > + * the output image size. This implies no protection > + * against buffer overflow is in place for Gen3 digital input > + * capture. > + */ > + dmr2 = VNDMR2_FTEV; > } else { > /* Enable VSYNC Field Toogle mode after one VSYNC input. */ Ditto. Search and replace? > dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1); > Regards, Hans
Hi Hans, On Fri, May 11, 2018 at 12:28:55PM +0200, Hans Verkuil wrote: > On 05/11/18 11:59, Jacopo Mondi wrote: > > Since commit (015060cb "media: rcar-vin: enable field toggle after a set > > number of lines for Gen3) the VIN generates an internal field signal > > toggle after a fixed number of received lines, and uses the internal > > field signal to drive capture operations. When capturing from digital > > input, using FTEH driven field signal toggling messes up the received > > image sizes. Fall back to use FTEV driven signal toggling when capturing > > from digital input. > > > > As explained in the comment, this disables buffer overflow protection > > for digital input capture, for which the FOE overflow might be used in > > future. > > I don't know the details of the hardware, but this sounds dangerous. > > You should know that with HDMI input it is perfectly possible that you get > more data than you should. I.e. instead of 1080 lines (assuming full HD) > you might get more lines. This happens if the vertical sync is missed due > to pin bounce when connecting a source. > > Other reasons for this are flaky signals, bad clocks, etc. > > It's rare, but it really happens. > > A good DMA engine will refuse to write more than fits in the buffer. > > If you disable that, then you will get overflows eventually. > > The reality with HDMI input is that you should never assume clean valid > data. You do not control the source and it can send anything it likes. Thanks for the informations. I agree HDMI input is lot of fun (-.-) and we've seen weird things happening too. With the patch Niklas has just sent that fixes the crop/compose rectangle, also the previously in-place protection agains overflow has been reverted, so this patch is not required anymore. I re-send re-basing this on top of Niklas' latest fix. Thanks j > > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/platform/rcar-vin/rcar-dma.c | 18 +++++++++++++++++- > > 1 file changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c > > index ea7a120..8dc3455 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c > > @@ -685,11 +685,27 @@ static int rvin_setup(struct rvin_dev *vin) > > break; > > } > > > > - if (vin->info->model == RCAR_GEN3) { > > + if (vin->info->model == RCAR_GEN3 && > > + vin->mbus_cfg.type == V4L2_MBUS_CSI2) { > > /* Enable HSYNC Field Toggle mode after height HSYNC inputs. */ > > lines = vin->format.height / (halfsize ? 2 : 1); > > dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines); > > vin_dbg(vin, "Field Toogle after %u lines\n", lines); > > Typo: Toogle -> Toggle > > > + } else if (vin->info->model == RCAR_GEN3 && > > + vin->mbus_cfg.type == V4L2_MBUS_PARALLEL) { > > + /* > > + * FIXME > > + * Section 26.3.17 specifies that for digital input there's no > > + * need to program FTEH or FTEV to generate internal > > + * field toggle signal to driver capture. Although when > > + * running on GEN3 with digital input no EFE interrupt is ever > > + * generated, and we need to rely on FTEV driven field signal > > + * toggling, as using FTEH as in the CSI-2 case, messes up > > + * the output image size. This implies no protection > > + * against buffer overflow is in place for Gen3 digital input > > + * capture. > > + */ > > + dmr2 = VNDMR2_FTEV; > > } else { > > /* Enable VSYNC Field Toogle mode after one VSYNC input. */ > > Ditto. Search and replace? > > > dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1); > > > > Regards, > > Hans
Hello! On 5/11/2018 12:59 PM, Jacopo Mondi wrote: > Since commit (015060cb Need 12 digits here, and SHA1 ID should be cited outside the parens. > "media: rcar-vin: enable field toggle after a set > number of lines for Gen3) The commit summary must be enclosed in (""). And I think Niklas has posted the patches reverting that commit and fixing the driver properly. > the VIN generates an internal field signal > toggle after a fixed number of received lines, and uses the internal > field signal to drive capture operations. When capturing from digital > input, using FTEH driven field signal toggling messes up the received > image sizes. Fall back to use FTEV driven signal toggling when capturing > from digital input. > > As explained in the comment, this disables buffer overflow protection > for digital input capture, for which the FOE overflow might be used in > future. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> [...] MBR, Sergei
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c index ea7a120..8dc3455 100644 --- a/drivers/media/platform/rcar-vin/rcar-dma.c +++ b/drivers/media/platform/rcar-vin/rcar-dma.c @@ -685,11 +685,27 @@ static int rvin_setup(struct rvin_dev *vin) break; } - if (vin->info->model == RCAR_GEN3) { + if (vin->info->model == RCAR_GEN3 && + vin->mbus_cfg.type == V4L2_MBUS_CSI2) { /* Enable HSYNC Field Toggle mode after height HSYNC inputs. */ lines = vin->format.height / (halfsize ? 2 : 1); dmr2 = VNDMR2_FTEH | VNDMR2_HLV(lines); vin_dbg(vin, "Field Toogle after %u lines\n", lines); + } else if (vin->info->model == RCAR_GEN3 && + vin->mbus_cfg.type == V4L2_MBUS_PARALLEL) { + /* + * FIXME + * Section 26.3.17 specifies that for digital input there's no + * need to program FTEH or FTEV to generate internal + * field toggle signal to driver capture. Although when + * running on GEN3 with digital input no EFE interrupt is ever + * generated, and we need to rely on FTEV driven field signal + * toggling, as using FTEH as in the CSI-2 case, messes up + * the output image size. This implies no protection + * against buffer overflow is in place for Gen3 digital input + * capture. + */ + dmr2 = VNDMR2_FTEV; } else { /* Enable VSYNC Field Toogle mode after one VSYNC input. */ dmr2 = VNDMR2_FTEV | VNDMR2_VLV(1);
Since commit (015060cb "media: rcar-vin: enable field toggle after a set number of lines for Gen3) the VIN generates an internal field signal toggle after a fixed number of received lines, and uses the internal field signal to drive capture operations. When capturing from digital input, using FTEH driven field signal toggling messes up the received image sizes. Fall back to use FTEV driven signal toggling when capturing from digital input. As explained in the comment, this disables buffer overflow protection for digital input capture, for which the FOE overflow might be used in future. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/platform/rcar-vin/rcar-dma.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)