Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/dmc: allocate dmc structure dynamically

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 17 Feb 2023, Imre Deak <imre.deak@intel.com> wrote:
> On Fri, Feb 17, 2023 at 12:04:14PM +0200, Jani Nikula wrote:
>> On Thu, 16 Feb 2023, Imre Deak <imre.deak@intel.com> wrote:
>> > On Thu, Feb 16, 2023 at 06:17:39PM +0200, Jani Nikula wrote:
>> >> sizeof(struct intel_dmc) > 1024 bytes, allocated on all platforms as
>> >> part of struct drm_i915_private, whether they have DMC or not.
>> >> 
>> >> Allocate struct intel_dmc dynamically, and hide all the dmc details
>> >> behind an opaque pointer in intel_dmc.c.
>> >> 
>> >> Care must be taken to take into account all cases: DMC not supported on
>> >> the platform, DMC supported but not initialized, and DMC initialized but
>> >> not loaded. For the second case, we need to move the wakeref out of
>> >> struct intel_dmc.
>> >> 
>> >> Cc: Imre Deak <imre.deak@intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >> ---
>> >>  .../gpu/drm/i915/display/intel_display_core.h |  8 ++-
>> >>  drivers/gpu/drm/i915/display/intel_dmc.c      | 50 +++++++++++++++++--
>> >>  drivers/gpu/drm/i915/display/intel_dmc.h      | 34 +------------
>> >>  .../drm/i915/display/intel_modeset_setup.c    |  1 +
>> >>  4 files changed, 53 insertions(+), 40 deletions(-)
>> >> 
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> index b870f7f47f2b..ff51149c5fcb 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
>> >> @@ -19,7 +19,6 @@
>> >>  #include "intel_cdclk.h"
>> >>  #include "intel_display_limits.h"
>> >>  #include "intel_display_power.h"
>> >> -#include "intel_dmc.h"
>> >>  #include "intel_dpll_mgr.h"
>> >>  #include "intel_fbc.h"
>> >>  #include "intel_global_state.h"
>> >> @@ -40,6 +39,7 @@ struct intel_cdclk_vals;
>> >>  struct intel_color_funcs;
>> >>  struct intel_crtc;
>> >>  struct intel_crtc_state;
>> >> +struct intel_dmc;
>> >>  struct intel_dpll_funcs;
>> >>  struct intel_dpll_mgr;
>> >>  struct intel_fbdev;
>> >> @@ -340,6 +340,11 @@ struct intel_display {
>> >>  		spinlock_t phy_lock;
>> >>  	} dkl;
>> >>  
>> >> +	struct {
>> >> +		struct intel_dmc *dmc;
>> >> +		intel_wakeref_t wakeref;
>> >> +	} dmc;
>> >> +
>> >>  	struct {
>> >>  		/* VLV/CHV/BXT/GLK DSI MMIO register base address */
>> >>  		u32 mmio_base;
>> >> @@ -467,7 +472,6 @@ struct intel_display {
>> >>  
>> >>  	/* Grouping using named structs. Keep sorted. */
>> >>  	struct intel_audio audio;
>> >> -	struct intel_dmc dmc;
>> >>  	struct intel_dpll dpll;
>> >>  	struct intel_fbc *fbc[I915_MAX_FBCS];
>> >>  	struct intel_frontbuffer_tracking fb_tracking;
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> index 1d156ac2eb4c..8428d08e0c3d 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
>> >> @@ -38,9 +38,37 @@
>> >>   * low-power state and comes back to normal.
>> >>   */
>> >>  
>> >> +enum intel_dmc_id {
>> >> +	DMC_FW_MAIN = 0,
>> >> +	DMC_FW_PIPEA,
>> >> +	DMC_FW_PIPEB,
>> >> +	DMC_FW_PIPEC,
>> >> +	DMC_FW_PIPED,
>> >> +	DMC_FW_MAX
>> >> +};
>> >> +
>> >> +struct intel_dmc {
>> >> +	struct drm_i915_private *i915;
>> >> +	struct work_struct work;
>> >> +	const char *fw_path;
>> >> +	u32 max_fw_size; /* bytes */
>> >> +	u32 version;
>> >> +	struct dmc_fw_info {
>> >> +		u32 mmio_count;
>> >> +		i915_reg_t mmioaddr[20];
>> >> +		u32 mmiodata[20];
>> >> +		u32 dmc_offset;
>> >> +		u32 start_mmioaddr;
>> >> +		u32 dmc_fw_size; /*dwords */
>> >> +		u32 *payload;
>> >> +		bool present;
>> >> +	} dmc_info[DMC_FW_MAX];
>> >> +};
>> >> +
>> >> +/* Note: This may be NULL. */
>> >>  static struct intel_dmc *i915_to_dmc(struct drm_i915_private *i915)
>> >>  {
>> >> -	return &i915->display.dmc;
>> >> +	return i915->display.dmc.dmc;
>> >>  }
>> >>  
>> >>  #define DMC_VERSION(major, minor)	((major) << 16 | (minor))
>> >> @@ -937,7 +965,10 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
>> >>  	if (!HAS_DMC(dev_priv))
>> >>  		return;
>> >>  
>> >> -	dmc = i915_to_dmc(dev_priv);
>> >> +	dmc = kzalloc(sizeof(*dmc), GFP_KERNEL);
>> >> +	if (!dmc)
>> >> +		return;
>> >
>> > Couldn't driver loading fail in this case instead (simplifying the
>> > dmc==NULL checks elsewhere)?
>> 
>> We could fail driver load, yes, but it still wouldn't simplify the NULL
>> checks because you could use i915.dmc_firmware_path="" to disable the
>> firmware, and what's the point in keeping the 1k memory allocated in
>> that case?
>
> Right, only noticed the same later. But that's only a debug feature so
> could be acceptable trade-off for simplicity imo; also the supported but
> non-initialized state wouldn't be needed then either.
>
> It's not a big deal and the check is local to intel_dmc.c, so either way
> on the patchset:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
> (Querying the DMC FW presence in IGT needs to be fixed before merging.)

The IGT part seemed harder to fix than just preserving the debugfs
output for dmc == NULL. v3 at:

https://patchwork.freedesktop.org/series/114431/

>
>> BR,
>> Jani.
>> 
>> 
>> >
>> >> +
>> >>  	dmc->i915 = dev_priv;
>> >>  
>> >>  	INIT_WORK(&dmc->work, dmc_load_work_fn);
>> >> @@ -991,10 +1022,9 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
>> >>  
>> >>  	if (dev_priv->params.dmc_firmware_path) {
>> >>  		if (strlen(dev_priv->params.dmc_firmware_path) == 0) {
>> >> -			dmc->fw_path = NULL;
>> >>  			drm_info(&dev_priv->drm,
>> >>  				 "Disabling DMC firmware and runtime PM\n");
>> >> -			return;
>> >> +			goto out;
>> >>  		}
>> >>  
>> >>  		dmc->fw_path = dev_priv->params.dmc_firmware_path;
>> >> @@ -1003,11 +1033,18 @@ void intel_dmc_init(struct drm_i915_private *dev_priv)
>> >>  	if (!dmc->fw_path) {
>> >>  		drm_dbg_kms(&dev_priv->drm,
>> >>  			    "No known DMC firmware for platform, disabling runtime PM\n");
>> >> -		return;
>> >> +		goto out;
>> >>  	}
>> >>  
>> >> +	dev_priv->display.dmc.dmc = dmc;
>> >> +
>> >>  	drm_dbg_kms(&dev_priv->drm, "Loading %s\n", dmc->fw_path);
>> >>  	schedule_work(&dmc->work);
>> >> +
>> >> +	return;
>> >> +
>> >> +out:
>> >> +	kfree(dmc);
>> >>  }
>> >>  
>> >>  /**
>> >> @@ -1074,6 +1111,9 @@ void intel_dmc_fini(struct drm_i915_private *dev_priv)
>> >>  	if (dmc) {
>> >>  		for_each_dmc_id(dmc_id)
>> >>  			kfree(dmc->dmc_info[dmc_id].payload);
>> >> +
>> >> +		kfree(dmc);
>> >> +		dev_priv->display.dmc.dmc = NULL;
>> >>  	}
>> >>  }
>> >>  
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> index b74635497aa7..fd607afff2ef 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
>> >> @@ -6,44 +6,12 @@
>> >>  #ifndef __INTEL_DMC_H__
>> >>  #define __INTEL_DMC_H__
>> >>  
>> >> -#include "i915_reg_defs.h"
>> >> -#include "intel_wakeref.h"
>> >> -#include <linux/workqueue.h>
>> >> +#include <linux/types.h>
>> >>  
>> >>  struct drm_i915_error_state_buf;
>> >>  struct drm_i915_private;
>> >> -
>> >>  enum pipe;
>> >>  
>> >> -enum intel_dmc_id {
>> >> -	DMC_FW_MAIN = 0,
>> >> -	DMC_FW_PIPEA,
>> >> -	DMC_FW_PIPEB,
>> >> -	DMC_FW_PIPEC,
>> >> -	DMC_FW_PIPED,
>> >> -	DMC_FW_MAX
>> >> -};
>> >> -
>> >> -struct intel_dmc {
>> >> -	struct drm_i915_private *i915;
>> >> -	struct work_struct work;
>> >> -	const char *fw_path;
>> >> -	u32 max_fw_size; /* bytes */
>> >> -	u32 version;
>> >> -	struct dmc_fw_info {
>> >> -		u32 mmio_count;
>> >> -		i915_reg_t mmioaddr[20];
>> >> -		u32 mmiodata[20];
>> >> -		u32 dmc_offset;
>> >> -		u32 start_mmioaddr;
>> >> -		u32 dmc_fw_size; /*dwords */
>> >> -		u32 *payload;
>> >> -		bool present;
>> >> -	} dmc_info[DMC_FW_MAX];
>> >> -
>> >> -	intel_wakeref_t wakeref;
>> >> -};
>> >> -
>> >>  void intel_dmc_init(struct drm_i915_private *i915);
>> >>  void intel_dmc_load_program(struct drm_i915_private *i915);
>> >>  void intel_dmc_disable_program(struct drm_i915_private *i915);
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_modeset_setup.c b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> >> index 5359b9663a07..42bfd56fcbe4 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_modeset_setup.c
>> >> @@ -22,6 +22,7 @@
>> >>  #include "intel_display.h"
>> >>  #include "intel_display_power.h"
>> >>  #include "intel_display_types.h"
>> >> +#include "intel_dmc.h"
>> >>  #include "intel_modeset_setup.h"
>> >>  #include "intel_pch_display.h"
>> >>  #include "intel_pm.h"
>> >> -- 
>> >> 2.34.1
>> >> 
>> 
>> -- 
>> Jani Nikula, Intel Open Source Graphics Center

-- 
Jani Nikula, Intel Open Source Graphics Center