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

[Boyuan Zhang]

On 2018-05-28 02:28 AM, Christian König wrote:
> 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().

Agree. I removed all of this and add an extra_dw field. Please see new 
patch set (0010 and 0011) just I sent out.

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

Same as above, this part is not needed after adding extra_dw. Please see 
new patch set (0010 and 0011)

>
>>
>>>
>>>> +
>>>> +    /* 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.

Same as above, this is removed as well.

>
>>>> +
>>>> +    /* 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.

Yes, direct assignment seems better than using amdgpu_ring_write() for 
the patch. Removed all amdgpu_ring_write() and replaced with 
assignments. Please see the new 0009 patch.

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

All changes have been marked as v2 in new patch sets that just send out.

Regards,
Boyuan