Hi Laurent,
On 22/05/17 13:24, Laurent Pinchart wrote:
> 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];
> ...
> }
My only distaste there is having to then add the [i-1] index to the sg_tables.
I have just experimented with:
fail:
for (; i-- != 0;) {
struct sg_table *sgt = &rstate->sg_tables[i];
...
}
This performs the correct loops, with the correct indexes, but does the
decrement in the condition offend coding styles ?
If that's disliked even more I'll just apply your suggestion.
--
Kieran
>
> 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;
>>> +}
>
