Re: [PATCH] drm/amdgpu:Added support for MV packet

["Christian König" <christian.koenig-5C7GfCeVMHo@public.gmane.org>]

Am 02.04.2018 um 22:27 schrieb James Zhu:
>
>
> On 2018-03-31 01:51 PM, Christian König wrote:
>> Am 29.03.2018 um 23:02 schrieb James Zhu:
>>> Motion vector packet needs support in physic mode.
>>>
>>> Signed-off-by: James Zhu <James.Zhu@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 24 ++++++++++++++++++++++++
>>>   1 file changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>>> index 4dfa868..ef6b780 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
>>> @@ -767,6 +767,18 @@ int amdgpu_vce_ring_parse_cs(struct 
>>> amdgpu_cs_parser *p, uint32_t ib_idx)
>>>               if (r)
>>>                   goto out;
>>>               break;
>>> +
>>> +        case 0x0500000d: /* MV buffer */
>>> +            r = amdgpu_vce_validate_bo(p, ib_idx, idx + 3,
>>> +                            idx + 2, 0, 0);
>>> +            if (r)
>>> +                goto out;
>>> +
>>> +            r = amdgpu_vce_validate_bo(p, ib_idx, idx + 8,
>>> +                            idx + 7, 0, 0);
>>> +            if (r)
>>> +                goto out;
>>> +            break;
>>
>> You need to specify a size here or otherwise userspace could allocate 
>> only a 4kb buffer and hope that VCE write over the end of the buffer.
>>
>> Since the MVs are easily controllable userspace can hope to hit and 
>> fill a page table with this. That would be a security hole you can 
>> push an elephant through, taking over the whole system with that is 
>> just a typing exercise.
>>
>> Regards,
>> Christian.
>>
> Hi Christian,
>
> The first buffer is for input frame, I saw encode doesn't specify the 
> size.

Good point, that makes this far less problematic, but I still think 
correct size handling would be nice to have.

Take a look at the second pass for the encode command, size handling is 
there:
>                 case 0x03000001: /* encode */
>                         r = amdgpu_vce_cs_reloc(p, ib_idx, idx + 10, 
> idx + 9,
>                                                 *size, 0);
>                         if (r)
>                                 goto out;

If I'm not completely mistake you just need to specify "*size" and 
"*size / 3" as parameters for your call to amdgpu_vce_cs_reloc as well.

> The 2nd buffer is for mv dump buffer, the size should be fixed with 
> ALIGN(enc.width, 16) * ALIGN(enc.height, 16) / 8 which is less than 
> 4k. Since IB doesn't pass down this value. how to specify the size for 
> this case?

Why do you think that ALIGN(enc.width, 16) * ALIGN(enc.height, 16) / 8 
is less than 4k? If I do the math correctly for a 1920x1088 input 
picture that should be around 255k.

*size should be size of the input picture in bytes (luma+chroma), so 
something like "*ALIGN(size * 3 / 2 / 8, 256) / 8" should do it.

Regards,
Christian.

>
> Best Regards!
> James Zhu
>>>           }
>>>             idx += len / 4;
>>> @@ -884,6 +896,18 @@ int amdgpu_vce_ring_parse_cs(struct 
>>> amdgpu_cs_parser *p, uint32_t ib_idx)
>>>                   goto out;
>>>               break;
>>>   +        case 0x0500000d: /* MV buffer */
>>> +            r = amdgpu_vce_cs_reloc(p, ib_idx, idx + 3,
>>> +                            idx + 2, 0, 0);
>>> +            if (r)
>>> +                goto out;
>>> +
>>> +            r = amdgpu_vce_cs_reloc(p, ib_idx, idx + 8,
>>> +                            idx + 7, 0, 0);
>>> +            if (r)
>>> +                goto out;
>>> +            break;
>>> +
>>>           default:
>>>               DRM_ERROR("invalid VCE command (0x%x)!\n", cmd);
>>>               r = -EINVAL;
>>
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx