Am 11.07.2018 um 17:37 schrieb Andrey Grodzovsky: > > > On 07/11/2018 11:05 AM, Christian König wrote: >> Am 11.07.2018 um 16:51 schrieb Andrey Grodzovsky: >>> [SNIP] >>> + >>> +   bool destroy_bo_list; >> >> I think you don't need this. IIRC the bo_list structure was reference >> counted. >> >> So all you need to do is to make sure that the temporary bo_list you >> create has a reference count of 1 and so is destroyed when the CS >> IOCTL calls amdgpu_bo_list_put() on it. >> >> That would simplify the patch quite a bit. > > amdgpu_bo_list_destroy is essential because it removes the list from > idr struct bo_list_handles, amdgpu_bo_list_put doesn't do it. > Regarding the refcount, it's 2 because it's 1 on list create in > amdgpu_cs_bo_handles_chunk->amdgpu_bo_list_create and then it's > incremented > to 2 in amdgpu_cs_parser_bos->amdgpu_bo_list_get. So by calling > amdgpu_bo_list_put and then amdgpu_bo_list_destroy the count properly > drops down to 0. Why is the bo_list on the idr in the first place? That sounds like it is unnecessary and maybe even quite dangerous when somebody guesses the temporary allocated id and messes it from another CPU at the same time while the CS IOCTL is processing things. Christian. > > Andrey > >> >> Apart from that looks really good to me, especially that you only >> need a new chunk ID for the UAPI is quite nice. >> >> Thanks, >> Christian.
