Re: [Intel-gfx] [PATCH v5 4/5] drm/i915/display: consider DG2_RC_CCS_CC when migrating buffers

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

On 11/10/2022 16:03, Tvrtko Ursulin wrote:
> 
> On 11/10/2022 15:39, Matthew Auld wrote:
>> Hi,
>>
>> On 11/10/2022 14:54, Tvrtko Ursulin wrote:
>>>
>>> Hi Matt,
>>>
>>> On 04/10/2022 14:19, Matthew Auld wrote:
>>>> For these types of display buffers, we need to able to CPU access some
>>>> part of the backing memory in prepare_plane_clear_colors(). As a result
>>>> we need to ensure we always place in the mappable part of lmem, which
>>>> becomes necessary on small-bar systems.
>>>>
>>>> v2(Nirmoy & Ville):
>>>>   - Add some commentary for why we need to CPU access the buffer.
>>>>   - Split out the other changes, so we just consider the display change
>>>>     here.
>>>> v3:
>>>>   - Handle this in the dpt path.
>>>> v4(Ville):
>>>>   - Drop the intel_fb_rc_ccs_cc_plane() sanity check in
>>>>     pin_and_fence_fb_obj(), since we can also trigger this on DG1 it
>>>>     seems.
>>>>
>>>> Fixes: eb1c535f0d69 ("drm/i915: turn on small BAR support")
>>>
>>> That one landed in 6.0 - do you want to send this (with 
>>> pre-requisite(s)) to stable? Or if not do you want me to send for 6.1 
>>> as part of fixes flow? In which case what are the per-requisites?
>>
>> This one is only for DG2, which is still hidden behind force_probe, so 
>> not too sure if it needs stable? I think the only pre-requisite is 
>> 999f45620772 ("drm/i915: allow control over the flags when 
>> migrating"), but again I'm not too sure how much we care about fixes 
>> for platforms hidden behind force_probe? What do you think?
> 
> It is certainly not mandatory, but now that cards are about to ship and 
> reach end users it may be nice to have if not too hard - at least for 
> 6.1 release candidates. I am not clear on the importance of the fix to 
> say for sure. Like what goes bang and under what circumstances. So I do 
> basically defer to someone who knows those answers.

It's important for small-bar DG2 systems, where it will potentially oops 
somewhere in the driver without this, so if we want to support DG2 as a 
platform in 6.1, then we also need this fix, along with 999f45620772.

> 
> Regards,
> 
> Tvrtko
> 
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>> Reported-by: Jianshui Yu <jianshui.yu@intel.com>
>>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> Cc: Nirmoy Das <nirmoy.das@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/i915/display/intel_fb_pin.c | 13 ++++++++++++-
>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/display/intel_fb_pin.c 
>>>> b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>>>> index 5031ee5695dd..e12339f46640 100644
>>>> --- a/drivers/gpu/drm/i915/display/intel_fb_pin.c
>>>> +++ b/drivers/gpu/drm/i915/display/intel_fb_pin.c
>>>> @@ -50,7 +50,18 @@ intel_pin_fb_obj_dpt(struct drm_framebuffer *fb,
>>>>               continue;
>>>>           if (HAS_LMEM(dev_priv)) {
>>>> -            ret = i915_gem_object_migrate(obj, &ww, 
>>>> INTEL_REGION_LMEM_0);
>>>> +            unsigned int flags = obj->flags;
>>>> +
>>>> +            /*
>>>> +             * For this type of buffer we need to able to read from 
>>>> the CPU
>>>> +             * the clear color value found in the buffer, hence we 
>>>> need to
>>>> +             * ensure it is always in the mappable part of lmem, if 
>>>> this is
>>>> +             * a small-bar device.
>>>> +             */
>>>> +            if (intel_fb_rc_ccs_cc_plane(fb) >= 0)
>>>> +                flags &= ~I915_BO_ALLOC_GPU_ONLY;
>>>> +            ret = __i915_gem_object_migrate(obj, &ww, 
>>>> INTEL_REGION_LMEM_0,
>>>> +                            flags);
>>>>               if (ret)
>>>>                   continue;
>>>>           }