Re: Unmappable VRAM patchset V3

[Thomas H. <the...@vm...>]

Jerome Glisse wrote:
> On Wed, Feb 24, 2010 at 01:37:42PM +0100, Thomas Hellstrom wrote:
>   
>> Jerome Glisse wrote:
>>     
>>> On Tue, Feb 23, 2010 at 02:05:50PM +0100, Thomas Hellstrom wrote:
>>>       
>>>> Jerome Glisse wrote:
>>>>         
>>>>> On Mon, Feb 22, 2010 at 08:58:28PM +0100, Thomas Hellstrom wrote:
>>>>>           
>>>>>> Jerome Glisse wrote:
>>>>>>             
>>>>>>> On Mon, Feb 22, 2010 at 06:30:24PM +0100, Thomas Hellstrom wrote:
>>>>>>>               
>>>>>>>> Jerome Glisse wrote:
>>>>>>>>                 
>>>>>>>>> Thomas i think i addressed your concern here, the ttm_bo_validate
>>>>>>>>> didn't needed a new argument or i did not understand what was
>>>>>>>>> necessary beside no_wait. In this patchset we check the value
>>>>>>>>> of callback in case of EBUSY (call set_need_resched) or ERESTARTSYS
>>>>>>>>> we return VM_FAULT_NOPAGE.
>>>>>>>>>                   
>>>>>>>> Well, if we from the fault callback call any function that might
>>>>>>>> call ttm_bo_reserve or ttm_bo_reserve_locked, we must make sure
>>>>>>>> that we never wait, but return -EBUSY all the way back to the
>>>>>>>> fault function. Such a case may be ttm_bo_validate that calls
>>>>>>>> ttm_bo_evict_first, or something causing a swapout...
>>>>>>>> ttm_bo_validate currently doesn't have that functionality,
>>>>>>>> because @no_wait just means don't wait for GPU.
>>>>>>>>                 
>>>>>>> What do you mean by never wait ? Is this GPU wait ? or CPU wait ie don't
>>>>>>> use mutex or kernel code path that might sleep ?
>>>>>>>               
>>>>>> I mean waiting while reserving a bo. (If another thread has the bo
>>>>>> reserved).
>>>>>>
>>>>>>             
>>>>>>> After a new review i don't think we ever wait for the GPU with the current
>>>>>>> patch and as far as i can tell we will return EBUSY or ERESTART all the
>>>>>>> way up.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Jerome
>>>>>>>               
>>>>>> If there is *no* code path trying to reserve a bo or create a
>>>>>> user-space visible object from within the fault handler, it should
>>>>>> be ok.
>>>>>>
>>>>>> /Thomas
>>>>>>
>>>>>>
>>>>>>             
>>>>> Did a new review again here is the call chain :
>>>>> ttm_bo_move_buffer
>>>>> ttm_bo_mem_space
>>>>>   ttm_bo_mem_force_space
>>>>>     ttm_mem_evict_first
>>>>>       ttm_bo_reserve_locked (no_wait = true)
>>>>>           
>>>> Here ttm_mem_evict_fist may wait for unreserve IIRC (the -EBUSY
>>>> return from ttm_bo_reserve_locked) is not propagated back.
>>>>         
>>> The code is not straightforward but if no_wait is true the
>>> -EBUSY of ttm_bo_reserve_locked will be propagated back.
>>>       
>> The point is that we don't want to set no_wait to true, because it's
>> OK to wait for GPU. We want to add an extra argument
>> no_wait_reserve.
>>
>> /Thomas
>>
>>     
>
> My patchset doesn't change the code there, no_wait will be true
> only if it's true as argument of ttm_bo_validate from the ttm
> fault callback in the driver.
>
>   

But you did call ttm_bo_move_buffer from the fault callback?
 Given the above, it's illegal to call it with no_wait set to false, and 
undesirable to call it with no_wait set to true.

/Thomas


> Cheers,
> Jerome
>