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@xxxxxxxxx>
> > > > Signed-off-by: Maarten Lankhorst
> > > > <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > > > ---
> > > > 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.
>
>
> 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__ */
