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

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

[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;
 		}
 
 		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;
-- 
2.7.4

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

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

[Christian König]

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.

>   		}
>   
>   		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

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

[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.
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?

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

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

[Christian König]

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

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

[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 a33804b..d7261e0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
@@ -755,6 +755,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;
 		}
 
 		idx += len / 4;
@@ -860,6 +872,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, *size, 0);
+			if (r)
+				goto out;
+
+			r = amdgpu_vce_cs_reloc(p, ib_idx, idx + 8,
+							idx + 7, *size / 12, 0);
+			if (r)
+				goto out;
+			break;
+
 		default:
 			DRM_ERROR("invalid VCE command (0x%x)!\n", cmd);
 			r = -EINVAL;
-- 
2.7.4

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

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

[James Zhu]

Hi Christian,

Thanks for correction.

Since MV dump is not normal encoding, its size is HxW/8. So the 2nd 
buffer size if  1/12 of the 1st one.

Thanks and Best Regards!

James Zhu


On 2018-04-03 05:11 AM, Christian König wrote:
> 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

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

[Christian König]

Am 03.04.2018 um 16:49 schrieb James Zhu:
> Motion vector packet needs support in physic mode.
>
> Signed-off-by: James Zhu <James.Zhu@amd.com>

Acked-by: Christian König <christian.koenig@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 a33804b..d7261e0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> @@ -755,6 +755,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;
>   		}
>   
>   		idx += len / 4;
> @@ -860,6 +872,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, *size, 0);
> +			if (r)
> +				goto out;
> +
> +			r = amdgpu_vce_cs_reloc(p, ib_idx, idx + 8,
> +							idx + 7, *size / 12, 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