Hi Kieran,
On Monday 22 May 2017 13:16:11 Kieran Bingham wrote:
> On 17/05/17 00:20, Laurent Pinchart wrote:
> > For planes handled by a VSP instance, map the framebuffer memory through
> > the VSP to ensure proper IOMMU handling.
> >
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
>
> Looks good except for a small unsigned int comparison causing an infinite
> loop on fail case.
>
> With the loop fixed:
>
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@xxxxxxxxxxxxxxxx>
>
> > ---
> >
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 74 +++++++++++++++++++++++++++---
> > drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 2 +
> > 2 files changed, 70 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index b0ff304ce3dc..1b874cfd40f3
> > 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
[snip]
> > @@ -187,6 +186,67 @@ static void rcar_du_vsp_plane_setup(struct
> > rcar_du_vsp_plane *plane)
> > vsp1_du_atomic_update(plane->vsp->vsp, plane->index, &cfg);
> > }
> >
> > +static int rcar_du_vsp_plane_prepare_fb(struct drm_plane *plane,
> > + struct drm_plane_state *state)
> > +{
> > + struct rcar_du_vsp_plane_state *rstate =
to_rcar_vsp_plane_state(state);
> > + struct rcar_du_vsp *vsp = to_rcar_vsp_plane(plane)->vsp;
> > + struct rcar_du_device *rcdu = vsp->dev;
> > + unsigned int i;
>
> Unsigned here..
>
> > + int ret;
> > +
> > + if (!state->fb)
> > + return 0;
> > +
> > + for (i = 0; i < rstate->format->planes; ++i) {
> > + struct drm_gem_cma_object *gem =
> > + drm_fb_cma_get_gem_obj(state->fb, i);
> > + struct sg_table *sgt = &rstate->sg_tables[i];
> > +
> > + ret = dma_get_sgtable(rcdu->dev, sgt, gem->vaddr, gem->paddr,
> > + gem->base.size);
> > + if (ret)
> > + goto fail;
> > +
> > + ret = vsp1_du_map_sg(vsp->vsp, sgt);
> > + if (!ret) {
> > + sg_free_table(sgt);
> > + ret = -ENOMEM;
> > + goto fail;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +fail:
> > + for (i--; i >= 0; i--) {
>
> Means that this loop will never exit; i will always be >= 0;
>
> I'd propose just making signed for this usage.
>
> I've checked the i values for the breakouts - so they are good, and we need
> to use i == 0 to unmap the last table.
>
> > + struct sg_table *sgt = &rstate->sg_tables[i];
How about keep i unsigned and using
for (; i > 0; i--) {
struct sg_table *sgt = &rstate->sg_tables[i-1];
...
}
If you want to fix while applying, you can pick your favourite version.
> > +
> > + vsp1_du_unmap_sg(vsp->vsp, sgt);
> > + sg_free_table(sgt);
> > + }
> > +
> > + return ret;
> > +}
--
Regards,
Laurent Pinchart
