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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

On 2023-03-03 13:12, Matthew Auld wrote:
> 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?
You can't put a FB In sysmem anyway, so we need to pass a flag to put it 
in visible VRAM.
>>>>
>>>> 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?
I don't think it's currently possible, I have no idea whether external 
VRAM is possible, I think it would depend on the hardware, but if it was 
supported, I would imagine sysmem would also have been.
>>>
>>> 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>
>>>>
~Maarten