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