Re: [Intel-gfx] [PATCH v2 1/3] drm/i915: Move stolen memory handling into i915_gem_stolen

[Nirmoy Das <nirmoy.das@linux.intel.com>]

On 8/1/2023 10:33 AM, Hogander, Jouni wrote:
> On Tue, 2023-08-01 at 10:02 +0200, Nirmoy Das wrote:
>> Hi Jouni,
>>
>> On 6/14/2023 7:17 AM, Jouni Högander wrote:
>>> We are preparing for Xe. Xe stolen memory handling differs from
>>> i915 so we
>>> want to move stolen memory handling details into i915_gem_stolen.
>>>
>>> Also add a common type for fbc compressed fb and use it from fbc
>>> code
>>> instead of underlying type directly. This way we can have common
>>> type
>>> i915_stolen_fb for both i915 and Xe.
>>>
>>> v2: Fix couple of checkpatch warnings
>>>
>>> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
>>> Signed-off-by: Maarten Lankhorst
>>> <maarten.lankhorst@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_fbc.c   | 46 +++++++++++------
>>> -----
>>>    drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 36 +++++++++++++++++
>>>    drivers/gpu/drm/i915/gem/i915_gem_stolen.h | 13 ++++++
>>>    3 files changed, 73 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
>>> b/drivers/gpu/drm/i915/display/intel_fbc.c
>>> index 7f8b2d7713c7..a18e84efe911 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
>>> @@ -94,8 +94,7 @@ struct intel_fbc {
>>>          struct mutex lock;
>>>          unsigned int busy_bits;
>>>    
>>> -       struct drm_mm_node compressed_fb;
>>> -       struct drm_mm_node compressed_llb;
>>> +       struct i915_stolen_fb compressed_fb, compressed_llb;
>>>    
>>>          enum intel_fbc_id id;
>>>    
>>> @@ -332,15 +331,16 @@ static void i8xx_fbc_program_cfb(struct
>>> intel_fbc *fbc)
>>>    {
>>>          struct drm_i915_private *i915 = fbc->i915;
>>>    
>>> -       GEM_BUG_ON(range_overflows_end_t(u64, i915-
>>>> dsm.stolen.start,
>>> -                                        fbc->compressed_fb.start,
>>> U32_MAX));
>>> -       GEM_BUG_ON(range_overflows_end_t(u64, i915-
>>>> dsm.stolen.start,
>>> -                                        fbc->compressed_llb.start,
>>> U32_MAX));
>>> -
>>> +       GEM_BUG_ON(range_overflows_end_t(u64,
>>> i915_gem_stolen_area_address(i915),
>>> +
>>> i915_gem_stolen_node_offset(&fbc->compressed_fb),
>>> +                                        U32_MAX));
>>> +       GEM_BUG_ON(range_overflows_end_t(u64,
>>> i915_gem_stolen_area_address(i915),
>>> +
>>> i915_gem_stolen_node_offset(&fbc->compressed_llb),
>>> +                                        U32_MAX));
>>>          intel_de_write(i915, FBC_CFB_BASE,
>>> -                      i915->dsm.stolen.start + fbc-
>>>> compressed_fb.start);
>>> +                      i915_gem_stolen_node_address(i915, &fbc-
>>>> compressed_fb));
>>>          intel_de_write(i915, FBC_LL_BASE,
>>> -                      i915->dsm.stolen.start + fbc-
>>>> compressed_llb.start);
>>> +                      i915_gem_stolen_node_address(i915, &fbc-
>>>> compressed_llb));
>>>    }
>>>    
>>>    static const struct intel_fbc_funcs i8xx_fbc_funcs = {
>>> @@ -447,7 +447,8 @@ static void g4x_fbc_program_cfb(struct
>>> intel_fbc *fbc)
>>>    {
>>>          struct drm_i915_private *i915 = fbc->i915;
>>>    
>>> -       intel_de_write(i915, DPFC_CB_BASE, fbc-
>>>> compressed_fb.start);
>>> +       intel_de_write(i915, DPFC_CB_BASE,
>>> +                      i915_gem_stolen_node_offset(&fbc-
>>>> compressed_fb));
>>>    }
>>>    
>>>    static const struct intel_fbc_funcs g4x_fbc_funcs = {
>>> @@ -498,7 +499,8 @@ static void ilk_fbc_program_cfb(struct
>>> intel_fbc *fbc)
>>>    {
>>>          struct drm_i915_private *i915 = fbc->i915;
>>>    
>>> -       intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id), fbc-
>>>> compressed_fb.start);
>>> +       intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id),
>>> +                      i915_gem_stolen_node_offset(&fbc-
>>>> compressed_fb));
>>>    }
>>>    
>>>    static const struct intel_fbc_funcs ilk_fbc_funcs = {
>>> @@ -713,7 +715,7 @@ static u64 intel_fbc_stolen_end(struct
>>> drm_i915_private *i915)
>>>           * underruns, even if that range is not reserved by the
>>> BIOS. */
>>>          if (IS_BROADWELL(i915) ||
>>>              (DISPLAY_VER(i915) == 9 && !IS_BROXTON(i915)))
>>> -               end = resource_size(&i915->dsm.stolen) - 8 * 1024 *
>>> 1024;
>>> +               end = i915_gem_stolen_area_size(i915) - 8 * 1024 *
>>> 1024;
>>>          else
>>>                  end = U64_MAX;
>>>    
>>> @@ -770,9 +772,9 @@ static int intel_fbc_alloc_cfb(struct intel_fbc
>>> *fbc,
>>>          int ret;
>>>    
>>>          drm_WARN_ON(&i915->drm,
>>> -                   drm_mm_node_allocated(&fbc->compressed_fb));
>>> +                   i915_gem_stolen_node_allocated(&fbc-
>>>> compressed_fb));
>>>          drm_WARN_ON(&i915->drm,
>>> -                   drm_mm_node_allocated(&fbc->compressed_llb));
>>> +                   i915_gem_stolen_node_allocated(&fbc-
>>>> compressed_llb));
>>>    
>>>          if (DISPLAY_VER(i915) < 5 && !IS_G4X(i915)) {
>>>                  ret = i915_gem_stolen_insert_node(i915, &fbc-
>>>> compressed_llb,
>>> @@ -792,15 +794,14 @@ static int intel_fbc_alloc_cfb(struct
>>> intel_fbc *fbc,
>>>    
>>>          drm_dbg_kms(&i915->drm,
>>>                      "reserved %llu bytes of contiguous stolen space
>>> for FBC, limit: %d\n",
>>> -                   fbc->compressed_fb.size, fbc->limit);
>>> -
>>> +                   i915_gem_stolen_node_size(&fbc->compressed_fb),
>>> fbc->limit);
>>>          return 0;
>>>    
>>>    err_llb:
>>> -       if (drm_mm_node_allocated(&fbc->compressed_llb))
>>> +       if (i915_gem_stolen_node_allocated(&fbc->compressed_llb))
>>>                  i915_gem_stolen_remove_node(i915, &fbc-
>>>> compressed_llb);
>>>    err:
>>> -       if (drm_mm_initialized(&i915->mm.stolen))
>>> +       if (i915_gem_stolen_initialized(i915))
>>>                  drm_info_once(&i915->drm, "not enough stolen space
>>> for compressed buffer (need %d more bytes), disabling. Hint: you
>>> may be able to increase stolen memory size in the BIOS to avoid
>>> this.\n", size);
>>>          return -ENOSPC;
>>>    }
>>> @@ -825,9 +826,9 @@ static void __intel_fbc_cleanup_cfb(struct
>>> intel_fbc *fbc)
>>>          if (WARN_ON(intel_fbc_hw_is_active(fbc)))
>>>                  return;
>>>    
>>> -       if (drm_mm_node_allocated(&fbc->compressed_llb))
>>> +       if (i915_gem_stolen_node_allocated(&fbc->compressed_llb))
>>>                  i915_gem_stolen_remove_node(i915, &fbc-
>>>> compressed_llb);
>>> -       if (drm_mm_node_allocated(&fbc->compressed_fb))
>>> +       if (i915_gem_stolen_node_allocated(&fbc->compressed_fb))
>>>                  i915_gem_stolen_remove_node(i915, &fbc-
>>>> compressed_fb);
>>>    }
>>>    
>>> @@ -1030,7 +1031,8 @@ static bool intel_fbc_is_cfb_ok(const struct
>>> intel_plane_state *plane_state)
>>>          struct intel_fbc *fbc = plane->fbc;
>>>    
>>>          return intel_fbc_min_limit(plane_state) <= fbc->limit &&
>>> -               intel_fbc_cfb_size(plane_state) <= fbc-
>>>> compressed_fb.size * fbc->limit;
>>> +               intel_fbc_cfb_size(plane_state) <= fbc->limit *
>>> +                       i915_gem_stolen_node_size(&fbc-
>>>> compressed_fb);
>>>    }
>>>    
>>>    static bool intel_fbc_is_ok(const struct intel_plane_state
>>> *plane_state)
>>> @@ -1707,7 +1709,7 @@ void intel_fbc_init(struct drm_i915_private
>>> *i915)
>>>    {
>>>          enum intel_fbc_id fbc_id;
>>>    
>>> -       if (!drm_mm_initialized(&i915->mm.stolen))
>>> +       if (!i915_gem_stolen_initialized(i915))
>>>                  DISPLAY_RUNTIME_INFO(i915)->fbc_mask = 0;
>>>    
>>>          if (need_fbc_vtd_wa(i915))
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>>> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>>> index 3b094d36a0b0..78bac1e611dd 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
>>> @@ -974,3 +974,39 @@ bool i915_gem_object_is_stolen(const struct
>>> drm_i915_gem_object *obj)
>>>    {
>>>          return obj->ops == &i915_gem_object_stolen_ops;
>>>    }
>>> +
>>> +bool i915_gem_stolen_initialized(const struct drm_i915_private
>>> *i915)
>>> +{
>>> +       return drm_mm_initialized(&i915->mm.stolen);
>>> +}
>>> +
>>> +u64 i915_gem_stolen_area_address(const struct drm_i915_private
>>> *i915)
>>> +{
>>> +       return i915->dsm.stolen.start;
>>> +}
>>> +
>>> +u64 i915_gem_stolen_area_size(const struct drm_i915_private *i915)
>>> +{
>>> +       return resource_size(&i915->dsm.stolen);
>>> +}
>>> +
>>> +u64 i915_gem_stolen_node_address(const struct drm_i915_private
>>> *i915,
>>> +                                const struct drm_mm_node *node)
>>> +{
>>> +       return i915->dsm.stolen.start +
>>> i915_gem_stolen_node_offset(node);
>>> +}
>>> +
>>> +bool i915_gem_stolen_node_allocated(const struct drm_mm_node
>>> *node)
>>> +{
>>> +       return drm_mm_node_allocated(node);
>>> +}
>>> +
>>> +u64 i915_gem_stolen_node_offset(const struct drm_mm_node *node)
>>> +{
>>> +       return node->start;
>>> +}
>>> +
>>> +u64 i915_gem_stolen_node_size(const struct drm_mm_node *node)
>>> +{
>>> +       return node->size;
>>
>> Above 3 functions are core drm functions/struct and not related to
>> stolen so I don't think
>>
>> they deserve special functions in stolen code.
> Xe and i915 have differing implementations for stolen memory. We want
> to remove these details from FBC code. The thing here is that in i915
> case stolen memory node == drm mm node. In Xe case it is not and
> interfaces for these queries will have own implementation for Xe. See:
>
> https://patchwork.freedesktop.org/patch/540793/?series=118560&rev=3


Is the motivation to keep display code same for both XE and i915 without 
adding lots #if ?


Regards,

Nirmoy

>
> BR,
>
> Jouni Högander
>>
>> Regards,
>>
>> Nirmoy
>>
>>> +}
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
>>> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
>>> index d5005a39d130..258381d1c054 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.h
>>> @@ -12,6 +12,8 @@ struct drm_i915_private;
>>>    struct drm_mm_node;
>>>    struct drm_i915_gem_object;
>>>    
>>> +#define i915_stolen_fb drm_mm_node
>>> +
>>>    int i915_gem_stolen_insert_node(struct drm_i915_private
>>> *dev_priv,
>>>                                  struct drm_mm_node *node, u64 size,
>>>                                  unsigned alignment);
>>> @@ -36,4 +38,15 @@ bool i915_gem_object_is_stolen(const struct
>>> drm_i915_gem_object *obj);
>>>    
>>>    #define I915_GEM_STOLEN_BIAS SZ_128K
>>>    
>>> +bool i915_gem_stolen_initialized(const struct drm_i915_private
>>> *i915);
>>> +u64 i915_gem_stolen_area_address(const struct drm_i915_private
>>> *i915);
>>> +u64 i915_gem_stolen_area_size(const struct drm_i915_private
>>> *i915);
>>> +
>>> +u64 i915_gem_stolen_node_address(const struct drm_i915_private
>>> *i915,
>>> +                                const struct drm_mm_node *node);
>>> +
>>> +bool i915_gem_stolen_node_allocated(const struct drm_mm_node
>>> *node);
>>> +u64 i915_gem_stolen_node_offset(const struct drm_mm_node *node);
>>> +u64 i915_gem_stolen_node_size(const struct drm_mm_node *node);
>>> +
>>>    #endif /* __I915_GEM_STOLEN_H__ */