[PATCH 10/17] drm/amdgpu: add patch for fixing a known bug

[Christian König]

Am 25.05.2018 um 17:33 schrieb Boyuan Zhang:
>
>
> On 2018-05-25 05:07 AM, Christian König wrote:
>> Am 24.05.2018 um 22:15 schrieb boyuan.zhang at amd.com:
>>> From: Boyuan Zhang <boyuan.zhang at amd.com>
>>>
>>> Allocate extra space in vcn jpeg ring buffer and store the jpeg ring 
>>> patch
>>>
>>> Signed-off-by: Boyuan Zhang <boyuan.zhang at amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c | 27 
>>> ++++++++++++++++++++++++++-
>>>   1 file changed, 26 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c 
>>> b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>> index dcd1a9a..2e4bd26 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/vcn_v1_0.c
>>> @@ -119,7 +119,8 @@ static int vcn_v1_0_sw_init(void *handle)
>>>         ring = &adev->vcn.ring_jpeg;
>>>       sprintf(ring->name, "vcn_jpeg");
>>> -    r = amdgpu_ring_init(adev, ring, 512, &adev->vcn.irq, 0);
>>> +    /* allocate extra dw in ring buffer for storing patch commands */
>>> +    r = amdgpu_ring_init(adev, ring, 512 + 64, &adev->vcn.irq, 0);

Taking a closer look that hack might not work after all. E.g. you need 
to overwrite max_dw, buf_mask and ptr_mask after initialization.

Maybe add an "extra_dw" field into amdgpu_ring_funcs and use that in 
amdgpu_ring_init().

>>>       if (r)
>>>           return r;
>>>   @@ -679,6 +680,30 @@ static int vcn_v1_0_start(struct 
>>> amdgpu_device *adev)
>>>       WREG32_SOC15(UVD, 0, mmUVD_JRBC_RB_WPTR, 0);
>>>       WREG32_SOC15(UVD, 0, mmUVD_JRBC_RB_CNTL, 0x00000002L);
>>>   +    /* set wptr to the extra allocated space in ring buffer */
>>> +    ring->wptr = RREG32_SOC15(UVD, 0, mmUVD_JRBC_RB_WPTR);
>>> +    ring->wptr += ring->max_dw * amdgpu_sched_hw_submission;
>>> +
>>> +    /* increase mask to allow to write to the extra space */
>>> +    ring->buf_mask += 64 * 4;
>>> +    ring->ptr_mask += 64 * 4;
>>
>> Well that is rather ugly. buf_mask and ptr_mask are bit masks, you 
>> could set them to 0xffffffff but what you do here might not work as 
>> expected.
>
> OK, so maybe setting both mask to 0xffffffff temporarily to allow 
> writing to extra space as mu as needed, then later on set them back?

Yeah, that should at least work as expected.  I would also add a comment 
here why we actually do all this.

>
>>
>>> +
>>> +    /* allocate extra space */
>>> +    r = amdgpu_ring_alloc(ring, 64);
>>> +    if (r) {
>>> +        DRM_ERROR("amdgpu: cp failed to lock ring %d (%d).\n",
>>> +                  ring->idx, r);
>>> +        return r;
>>> +    }

Would be nice if we could avoid that call as well, cause this is really 
not ring buffer operation but rather setting up the environment.

>>> +
>>> +    /* copy patch commands to the extra space */
>>> +    vcn_v1_0_jpeg_ring_set_patch_ring(ring);
>>
>> Can't vcn_v1_0_jpeg_ring_set_patch_ring() just patch the command 
>> directly to he end of the ring buffer?
>>
>> In other words why do we need to use amdgpu_ring_write() in 
>> vcn_v1_0_jpeg_ring_set_patch_ring()?
>>
>> Christian.
>
> Could you give me some more detailed info on how to do that? I thought 
> amdgpu_ring_write() is the best way to write commands to the ring, 
> what is the other/better way to do it in this case? Could you point me 
> to a simple example how to write command directly to the end of ring 
> (without ring_write)?

Well something like this should work:

/* pointer to the end of the ring buffer */
i = ring->max_dw * amdgpu_sched_hw_submission;
ring->ring[i++] = ....
ring->ring[i++] = ....
ring->ring[i++] = ....
ring->ring[i++] = ....

Regards,
Christian.

>
> Thanks,
> Boyuan
>
>>
>>> +
>>> +    /* reset wptr and mask */
>>> +    ring->wptr = RREG32_SOC15(UVD, 0, mmUVD_JRBC_RB_WPTR);
>>> +    ring->buf_mask -= 0x100;
>>> +    ring->ptr_mask -= 0x100;
>>> +
>>>       return 0;
>>>   }
>>
>