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@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
