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

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

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.

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

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