Re: [PATCH v3 7/9] drm/i915/bxt: Enable IPC support

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Hi

Lots of nitpicking in my review. Feel free to disagree with them.


Em Sex, 2016-09-09 às 13:31 +0530, Kumar, Mahesh escreveu:
> From: Mahesh Kumar <mahesh1.kumar@intel.com>
> 
> This patch adds IPC support for platforms. This patch enables IPC
> only for BXT platform as for SKL recommendation is to keep is
> disabled

But don't we want it for KBL too?

Also, can you please elaborate the commit message a little bit more?
What exactly does this feature do? What do we gain with it? Are there
any downsides?

> This patch also adds kernel command-line option to enable/disable
> the IPC if required.

Any reason on why someone would want to do this besides for debugging?
I'm not sure if every single little feature like this one-bit one-
platform feature requires an i915 option, so unless there's a strong
reason, we can just omit the option.

> 

> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c    |  5 +++++
>  drivers/gpu/drm/i915/i915_params.c |  5 +++++
>  drivers/gpu/drm/i915/i915_params.h |  1 +
>  drivers/gpu/drm/i915/i915_reg.h    |  1 +
>  drivers/gpu/drm/i915/intel_drv.h   |  1 +
>  drivers/gpu/drm/i915/intel_pm.c    | 21 +++++++++++++++++++++
>  6 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 0a4f18d..22d84e6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1335,6 +1335,11 @@ int i915_driver_load(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  
>  	intel_runtime_pm_enable(dev_priv);
>  
> +	/*
> +	 * Now enable the IPC for supported platforms
> +	 */
> +	intel_enable_ipc(dev_priv);

The comment is unnecessary: it basically only says what the function
name already says.

Also, I think it makes more sense to move this to intel_init_pm(). As a
bonus, you'll be able to make the function static. Or even just make
the IPC code be part of init_clock_gating() since the whole feature is
just "enable this bit".


> +
>  	/* Everything is in place, we can now relax! */
>  	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>  		 driver.name, driver.major, driver.minor,
> driver.patchlevel,
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 768ad89..cc41b8d 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -62,6 +62,7 @@ struct i915_params i915 __read_mostly = {
>  	.inject_load_failure = 0,
>  	.enable_dpcd_backlight = false,
>  	.enable_gvt = false,
> +	.enable_ipc = true,
>  };
>  
>  module_param_named(modeset, i915.modeset, int, 0400);
> @@ -233,3 +234,7 @@ MODULE_PARM_DESC(enable_dpcd_backlight,
>  module_param_named(enable_gvt, i915.enable_gvt, bool, 0400);
>  MODULE_PARM_DESC(enable_gvt,
>  	"Enable support for Intel GVT-g graphics virtualization host
> support(default:false)");
> +
> +module_param_named(enable_ipc, i915.enable_ipc, bool, 0400);
> +MODULE_PARM_DESC(enable_ipc,
> +		"Enable Isochronous Priority Control
> (default:true)");
> diff --git a/drivers/gpu/drm/i915/i915_params.h
> b/drivers/gpu/drm/i915/i915_params.h
> index 3a0dd78..f99b9b9 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -65,6 +65,7 @@ struct i915_params {
>  	bool enable_dp_mst;
>  	bool enable_dpcd_backlight;
>  	bool enable_gvt;
> +	bool enable_ipc;
>  };
>  
>  extern struct i915_params i915 __read_mostly;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index b38445c..75b5b52 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6139,6 +6139,7 @@ enum {
>  #define  DISP_FBC_WM_DIS		(1<<15)
>  #define DISP_ARB_CTL2	_MMIO(0x45004)
>  #define  DISP_DATA_PARTITION_5_6	(1<<6)
> +#define  DISP_IPC_ENABLE		(1<<3)
>  #define DBUF_CTL	_MMIO(0x45008)
>  #define  DBUF_POWER_REQUEST		(1<<31)
>  #define  DBUF_POWER_STATE		(1<<30)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 66cb46c..56c8ac8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1753,6 +1753,7 @@ void skl_write_plane_wm(struct intel_crtc
> *intel_crtc,
>  uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> *pipe_config);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
> +void intel_enable_ipc(struct drm_i915_private *dev_priv);
>  static inline int intel_enable_rc6(void)
>  {
>  	return i915.enable_rc6;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index d4390e4..8d0037c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4793,6 +4793,27 @@ void intel_update_watermarks(struct drm_crtc
> *crtc)
>  }
>  
>  /*
> + * enable IPC for Supported Platforms
> + */

This comment also doesn't say very much. Also, s/enable/Enable/ if you
keep it.

> +void intel_enable_ipc(struct drm_i915_private *dev_priv)
> +{
> +	u32 val;
> +
> +	/* enable IPC only for Broxton for now*/

Also not a useful comment, unless you explain why.

> +	if (!IS_BROXTON(dev_priv))
> +		return;
> +
> +	val = I915_READ(DISP_ARB_CTL2);
> +
> +	if (i915.enable_ipc)
> +		val |= DISP_IPC_ENABLE;
> +	else
> +		val &= ~DISP_IPC_ENABLE;
> +
> +	I915_WRITE(DISP_ARB_CTL2, val);

> +}
> +
> +/*
>   * Lock protecting IPS related data structures
>   */
>  DEFINE_SPINLOCK(mchdev_lock);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx