Re: [Intel-xe] [PATCH v2 12/14] drm/xe/display: annotate CC buffers with NEEDS_CPU_ACCESS

[Matthew Auld <matthew.auld@intel.com>]

On 02/03/2023 11:51, Maarten Lankhorst wrote:
> Hey,
> 
> On 2023-02-28 16:20, Matthew Auld wrote:
>> On 28/02/2023 14:48, Maarten Lankhorst wrote:
>>> Hey,
>>>
>>> On 2023-02-28 11:41, Matthew Auld wrote:
>>>> The display code wants to read the clear color value from the buffer.
>>>> However if the buffer is the non-mappable part of lmem then we fail the
>>>> kmap. The simplest solution is to just mark the buffer with
>>>> XE_BO_NEEDS_CPU_ACCESS, which will either allocate the buffer in the
>>>> mappable part of lmem, or migrate it there.
>>>>
>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/xe/display/xe_fb_pin.c | 8 ++++++++
>>>>   1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c 
>>>> b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>>>> index 65c0bc28a3d1..66e1309e21d8 100644
>>>> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
>>>> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
>>>> @@ -203,6 +203,14 @@ static struct i915_vma *__xe_pin_fb_vma(struct 
>>>> intel_framebuffer *fb,
>>>>       if (ret)
>>>>           goto err;
>>>> +    /*
>>>> +     * For this type of buffer we need to able to read from the CPU 
>>>> the
>>>> +     * clear color value found in the buffer. This doesn't do 
>>>> anything on
>>>> +     * non small-bar devices.
>>>> +     */
>>>> +    if (intel_fb_rc_ccs_cc_plane(&fb->base) >= 0)
>>>> +        bo->flags |= XE_BO_NEEDS_CPU_ACCESS;
>>>
>>> While we do need a change, do we need to do this inside pinning? We 
>>> could also require userspace to create VRAM BO's with CPU_ACCESS, and 
>>> reject CCS here.
>>
>> Do you mean reject CCS if CPU_ACCESS is not set? The trouble is that 
>> CPU_ACCESS also requires system memory as a spill option when 
>> specifying the placements, which then prevents using CCS. Or at least 
>> that's how it was in i915.
> We should be able to handle it without moving to sysmem, if that's what 
> we need.

Ok, so maybe drop NEEDS_CPU_ACCESS, and just have NEEDS_VISIBLE_VRAM 
instead, and to get the old behaviour the user can just attach system 
memory as an extra placement? And then for clear color stuff they can 
use VISIBLE_VRAM by itself, and we can enforce that here?

>>>
>>> Of course we should probably also add a UAPI to allow setting the 
>>> SCANOUT flag on externally imported DMA-BUF bo's, i don't think we 
>>> require CPU_ACCESS flag for that as it's not our BO.

For discrete specifically, can you have externally imported dma-buf for 
display (not originating from the same device)? The scanout needs vram 
local to the device, right?

>>
>> Hmm, maybe if SCANOUT then internally use the mappable part of lmem by 
>> default? For dumb buffers I think it does something like that. And 
>> keep the above as a fallback?
> For dumb bo's, it is expected they are mapped, so we can set the 
> CPU_ACCESS flag. I don't know if we need it all the time, so for VRAM 
> bo's it could be specified as a separate flag, which is ignored for sysmem.
>>
>>>
>>> Might require some more thinking on how we want to handle this.
>>>
>>> Other patches look good, except for the comments I had:
>>>
>>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>