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 [email protected]:
From: Boyuan Zhang <[email protected]>
Allocate extra space in vcn jpeg ring buffer and store the jpeg
ring patch
Signed-off-by: Boyuan Zhang <[email protected]>
---
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
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
