Hi again,
On 2018-05-11 13:10:37 +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2018-05-11 11:59:40 +0200, Jacopo Mondi wrote:
> > The crop_scale routine uses the crop rectangle memebers to configure the
> > VIN clipping rectangle. When crop is not configured all its fields are
> > 0s, and setting the clipping rectangle vertical and horizontal extensions
> > to (0 - 1) causes the registers to be written with an incorrect
> > 0xffffffff value.
>
> This is an interesting find and a clear bug in my code. But I can't see
> how crop ever could be 0. When s_fmt is called it always resets the crop
> and compose width's to the requested format size.
>
> I'm curious how you found this bug, I tried to reproduce it but could
> not.
My bad I was looking at the wrong thing, yes I can reproduce this on
CSI-2 capture as well. Really nice find!
> This is indeed something we should fix! But I think the proper fix is
>not allowing crop to be 0 and not treating the symptom in
>rvin_crop_scale_comp().
>
> >
> > Fix this by using the actual format width and height when no crop
> > rectangle has been programmed.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>
> > ---
> > drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------
> > 1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index b41ba9a..ea7a120 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -579,22 +579,25 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
> >
> > void rvin_crop_scale_comp(struct rvin_dev *vin)
> > {
> > - /* Set Start/End Pixel/Line Pre-Clip */
> > + u32 width = vin->crop.width ? vin->crop.left + vin->crop.width :
> > + vin->format.width;
> > + u32 height = vin->crop.height ? vin->crop.top + vin->crop.height :
> > + vin->format.height;
> > +
> > + /* Set Start/End Pixel/Line Pre-Clip if crop is configured. */
> > rvin_write(vin, vin->crop.left, VNSPPRC_REG);
> > - rvin_write(vin, vin->crop.left + vin->crop.width - 1, VNEPPRC_REG);
> > + rvin_write(vin, width - 1, VNEPPRC_REG);
> >
> > switch (vin->format.field) {
> > case V4L2_FIELD_INTERLACED:
> > case V4L2_FIELD_INTERLACED_TB:
> > case V4L2_FIELD_INTERLACED_BT:
> > rvin_write(vin, vin->crop.top / 2, VNSLPRC_REG);
> > - rvin_write(vin, (vin->crop.top + vin->crop.height) / 2 - 1,
> > - VNELPRC_REG);
> > + rvin_write(vin, height / 2 - 1, VNELPRC_REG);
> > break;
> > default:
> > rvin_write(vin, vin->crop.top, VNSLPRC_REG);
> > - rvin_write(vin, vin->crop.top + vin->crop.height - 1,
> > - VNELPRC_REG);
> > + rvin_write(vin, height - 1, VNELPRC_REG);
> > break;
> > }
> >
> > --
> > 2.7.4
> >
>
> --
> Regards,
> Niklas Söderlund
--
Regards,
Niklas Söderlund
