> -----Original Message----- > From: Intel-gfx <[email protected]> On Behalf Of Ville > Syrjala > Sent: Wednesday, November 24, 2021 1:37 PM > To: [email protected] > Subject: [Intel-gfx] [PATCH 13/20] drm/i915/fbc: Allocate intel_fbc > dynamically > > From: Ville Syrjälä <[email protected]> > > In the future we may have more than one FBC instance on some platforms. So > let's just allocate it dynamically. This also lets us fully hide the > implementation > from prying eyes. > > Signed-off-by: Ville Syrjälä <[email protected]>
Reviewed-by: Mika Kahola <[email protected]> > --- > drivers/gpu/drm/i915/display/i9xx_plane.c | 2 +- > drivers/gpu/drm/i915/display/intel_fbc.c | 154 +++++++++++++----- > .../drm/i915/display/skl_universal_plane.c | 2 +- > drivers/gpu/drm/i915/i915_drv.h | 59 +------ > 4 files changed, 116 insertions(+), 101 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c > b/drivers/gpu/drm/i915/display/i9xx_plane.c > index 84f50c90728f..85950ff67609 100644 > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c > @@ -125,7 +125,7 @@ static struct intel_fbc *i9xx_plane_fbc(struct > drm_i915_private *dev_priv, > enum i9xx_plane_id i9xx_plane) > { > if (i9xx_plane_has_fbc(dev_priv, i9xx_plane)) > - return &dev_priv->fbc; > + return dev_priv->fbc; > else > return NULL; > } > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > b/drivers/gpu/drm/i915/display/intel_fbc.c > index 9be8e7dcaab6..1daf4f7b5d80 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -59,6 +59,63 @@ struct intel_fbc_funcs { > void (*set_false_color)(struct intel_fbc *fbc, bool enable); }; > > +struct intel_fbc_state { > + const char *no_fbc_reason; > + enum i9xx_plane_id i9xx_plane; > + unsigned int cfb_stride; > + unsigned int cfb_size; > + unsigned int fence_y_offset; > + u16 override_cfb_stride; > + u16 interval; > + s8 fence_id; > +}; > + > +struct intel_fbc { > + struct drm_i915_private *i915; > + const struct intel_fbc_funcs *funcs; > + > + /* > + * This is always the inner lock when overlapping with > + * struct_mutex and it's the outer lock when overlapping > + * with stolen_lock. > + */ > + struct mutex lock; > + unsigned int possible_framebuffer_bits; > + unsigned int busy_bits; > + struct intel_plane *plane; > + > + struct drm_mm_node compressed_fb; > + struct drm_mm_node compressed_llb; > + > + u8 limit; > + > + bool false_color; > + > + bool active; > + bool activated; > + bool flip_pending; > + > + bool underrun_detected; > + struct work_struct underrun_work; > + > + /* > + * Due to the atomic rules we can't access some structures without the > + * appropriate locking, so we cache information here in order to avoid > + * these problems. > + */ > + struct intel_fbc_state state_cache; > + > + /* > + * This structure contains everything that's relevant to program the > + * hardware registers. When we want to figure out if we need to disable > + * and re-enable FBC for a new configuration we just check if there's > + * something different in the struct. The genx_fbc_activate functions > + * are supposed to read from it in order to program the registers. > + */ > + struct intel_fbc_state params; > + const char *no_fbc_reason; > +}; > + > /* plane stride in pixels */ > static unsigned int intel_fbc_plane_stride(const struct intel_plane_state > *plane_state) { @@ -762,14 +819,16 @@ static void > __intel_fbc_cleanup_cfb(struct intel_fbc *fbc) > > void intel_fbc_cleanup(struct drm_i915_private *i915) { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = i915->fbc; > > - if (!HAS_FBC(i915)) > + if (!fbc) > return; > > mutex_lock(&fbc->lock); > __intel_fbc_cleanup_cfb(fbc); > mutex_unlock(&fbc->lock); > + > + kfree(fbc); > } > > static bool stride_is_valid(const struct intel_plane_state *plane_state) @@ - > 1319,9 +1378,9 @@ void intel_fbc_invalidate(struct drm_i915_private *i915, > unsigned int frontbuffer_bits, > enum fb_op_origin origin) > { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = i915->fbc; > > - if (!HAS_FBC(i915)) > + if (!fbc) > return; > > if (origin == ORIGIN_FLIP || origin == ORIGIN_CURSOR_UPDATE) @@ - > 1340,9 +1399,9 @@ void intel_fbc_invalidate(struct drm_i915_private *i915, > void intel_fbc_flush(struct drm_i915_private *i915, > unsigned int frontbuffer_bits, enum fb_op_origin origin) { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = i915->fbc; > > - if (!HAS_FBC(i915)) > + if (!fbc) > return; > > mutex_lock(&fbc->lock); > @@ -1484,9 +1543,9 @@ void intel_fbc_update(struct intel_atomic_state > *state, > */ > void intel_fbc_global_disable(struct drm_i915_private *i915) { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = i915->fbc; > > - if (!HAS_FBC(i915)) > + if (!fbc) > return; > > mutex_lock(&fbc->lock); > @@ -1497,9 +1556,8 @@ void intel_fbc_global_disable(struct drm_i915_private > *i915) > > static void intel_fbc_underrun_work_fn(struct work_struct *work) { > - struct drm_i915_private *i915 = > - container_of(work, struct drm_i915_private, > fbc.underrun_work); > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = container_of(work, typeof(*fbc), > underrun_work); > + struct drm_i915_private *i915 = fbc->i915; > > mutex_lock(&fbc->lock); > > @@ -1524,9 +1582,9 @@ static void intel_fbc_underrun_work_fn(struct > work_struct *work) > */ > void intel_fbc_reset_underrun(struct drm_i915_private *i915) { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = i915->fbc; > > - if (!HAS_FBC(i915)) > + if (!fbc) > return; > > cancel_work_sync(&fbc->underrun_work); > @@ -1559,9 +1617,9 @@ void intel_fbc_reset_underrun(struct > drm_i915_private *i915) > */ > void intel_fbc_handle_fifo_underrun_irq(struct drm_i915_private *i915) { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = i915->fbc; > > - if (!HAS_FBC(i915)) > + if (!fbc) > return; > > /* There's no guarantee that underrun_detected won't be set to true > @@ -1621,35 +1679,17 @@ void intel_fbc_add_plane(struct intel_fbc *fbc, > struct intel_plane *plane) > fbc->possible_framebuffer_bits |= plane->frontbuffer_bit; } > > -/** > - * intel_fbc_init - Initialize FBC > - * @i915: the i915 device > - * > - * This function might be called during PM init process. > - */ > -void intel_fbc_init(struct drm_i915_private *i915) > +static struct intel_fbc *intel_fbc_create(struct drm_i915_private > +*i915) > { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc; > + > + fbc = kzalloc(sizeof(*fbc), GFP_KERNEL); > + if (!fbc) > + return NULL; > > fbc->i915 = i915; > INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn); > mutex_init(&fbc->lock); > - fbc->active = false; > - > - if (!drm_mm_initialized(&i915->mm.stolen)) > - mkwrite_device_info(i915)->display.has_fbc = false; > - > - if (need_fbc_vtd_wa(i915)) > - mkwrite_device_info(i915)->display.has_fbc = false; > - > - i915->params.enable_fbc = intel_sanitize_fbc_option(i915); > - drm_dbg_kms(&i915->drm, "Sanitized enable_fbc value: %d\n", > - i915->params.enable_fbc); > - > - if (!HAS_FBC(i915)) { > - fbc->no_fbc_reason = "unsupported by this chipset"; > - return; > - } > > if (DISPLAY_VER(i915) >= 7) > fbc->funcs = &ivb_fbc_funcs; > @@ -1664,11 +1704,43 @@ void intel_fbc_init(struct drm_i915_private *i915) > else > fbc->funcs = &i8xx_fbc_funcs; > > + return fbc; > +} > + > +/** > + * intel_fbc_init - Initialize FBC > + * @i915: the i915 device > + * > + * This function might be called during PM init process. > + */ > +void intel_fbc_init(struct drm_i915_private *i915) { > + struct intel_fbc *fbc; > + > + if (!drm_mm_initialized(&i915->mm.stolen)) > + mkwrite_device_info(i915)->display.has_fbc = false; > + > + if (need_fbc_vtd_wa(i915)) > + mkwrite_device_info(i915)->display.has_fbc = false; > + > + i915->params.enable_fbc = intel_sanitize_fbc_option(i915); > + drm_dbg_kms(&i915->drm, "Sanitized enable_fbc value: %d\n", > + i915->params.enable_fbc); > + > + if (!HAS_FBC(i915)) > + return; > + > + fbc = intel_fbc_create(i915); > + if (!fbc) > + return; > + > /* We still don't have any sort of hardware state readout for FBC, so > * deactivate it in case the BIOS activated it to make sure software > * matches the hardware state. */ > if (intel_fbc_hw_is_active(fbc)) > intel_fbc_hw_deactivate(fbc); > + > + i915->fbc = fbc; > } > > static int intel_fbc_debugfs_status_show(struct seq_file *m, void *unused) @@ > -1743,8 +1815,8 @@ static void intel_fbc_debugfs_add(struct intel_fbc *fbc) > > void intel_fbc_debugfs_register(struct drm_i915_private *i915) { > - struct intel_fbc *fbc = &i915->fbc; > + struct intel_fbc *fbc = i915->fbc; > > - if (HAS_FBC(i915)) > + if (fbc) > intel_fbc_debugfs_add(fbc); > } > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c > b/drivers/gpu/drm/i915/display/skl_universal_plane.c > index 22ec6901ee30..980f23680842 100644 > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c > @@ -1820,7 +1820,7 @@ static struct intel_fbc *skl_plane_fbc(struct > drm_i915_private *dev_priv, > enum pipe pipe, enum plane_id plane_id) > { > if (skl_plane_has_fbc(dev_priv, pipe, plane_id)) > - return &dev_priv->fbc; > + return dev_priv->fbc; > else > return NULL; > } > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index f632b026ce34..12099f7ff98e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -399,64 +399,8 @@ struct drm_i915_display_funcs { > void (*commit_modeset_enables)(struct intel_atomic_state *state); }; > > -struct intel_fbc_funcs; > - > #define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address > space */ > > -struct intel_fbc_state { > - const char *no_fbc_reason; > - enum i9xx_plane_id i9xx_plane; > - unsigned int cfb_stride; > - unsigned int cfb_size; > - unsigned int fence_y_offset; > - u16 override_cfb_stride; > - u16 interval; > - s8 fence_id; > -}; > - > -struct intel_fbc { > - struct drm_i915_private *i915; > - const struct intel_fbc_funcs *funcs; > - > - /* This is always the inner lock when overlapping with struct_mutex and > - * it's the outer lock when overlapping with stolen_lock. */ > - struct mutex lock; > - unsigned int possible_framebuffer_bits; > - unsigned int busy_bits; > - struct intel_plane *plane; > - > - struct drm_mm_node compressed_fb; > - struct drm_mm_node compressed_llb; > - > - u8 limit; > - > - bool false_color; > - > - bool active; > - bool activated; > - bool flip_pending; > - > - bool underrun_detected; > - struct work_struct underrun_work; > - > - /* > - * Due to the atomic rules we can't access some structures without the > - * appropriate locking, so we cache information here in order to avoid > - * these problems. > - */ > - struct intel_fbc_state state_cache; > - > - /* > - * This structure contains everything that's relevant to program the > - * hardware registers. When we want to figure out if we need to disable > - * and re-enable FBC for a new configuration we just check if there's > - * something different in the struct. The genx_fbc_activate functions > - * are supposed to read from it in order to program the registers. > - */ > - struct intel_fbc_state params; > - const char *no_fbc_reason; > -}; > - > /* > * HIGH_RR is the highest eDP panel refresh rate read from EDID > * LOW_RR is the lowest eDP panel refresh rate found from EDID @@ -493,7 > +437,6 @@ struct i915_drrs { #define > QUIRK_NO_PPS_BACKLIGHT_POWER_HOOK (1<<8) > > struct intel_fbdev; > -struct intel_fbc_work; > > struct intel_gmbus { > struct i2c_adapter adapter; > @@ -892,7 +835,7 @@ struct drm_i915_private { > u32 pipestat_irq_mask[I915_MAX_PIPES]; > > struct i915_hotplug hotplug; > - struct intel_fbc fbc; > + struct intel_fbc *fbc; > struct i915_drrs drrs; > struct intel_opregion opregion; > struct intel_vbt_data vbt; > -- > 2.32.0
