[Intel-gfx] [PATCH v2 1/3] drm/i915: Move stolen memory handling into i915_gem_stolen
Nirmoy Das
nirmoy.das at linux.intel.com
Thu Aug 3 08:51:54 UTC 2023
Hi Jouni,
On 8/2/2023 9:52 AM, Hogander, Jouni wrote:
> On Wed, 2023-08-02 at 09:51 +0200, Nirmoy Das wrote:
>> 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 at intel.com>
>>>>> Signed-off-by: Maarten Lankhorst
>>>>> <maarten.lankhorst at 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 ?
> Yes, this is our target currently.
Got it, in that case
Reviewed-by: Nirmoy Das <nirmoy.das at intel.com>
Regards,
Nirmoy
>
>>
>> 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__ */
More information about the Intel-gfx
mailing list