| Message ID | 1526032781-14319-5-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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@jmondi.org> > --- > 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 >
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@jmondi.org> > > --- > > 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
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; }
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. Fix this by using the actual format width and height when no crop rectangle has been programmed. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/platform/rcar-vin/rcar-dma.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)