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

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

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.
>>
>> 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.
>
> 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>
>>