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

2016-09-22 Thread Mahesh Kumar 

Hi,


On Thursday 22 September 2016 01:36 AM, Paulo Zanoni wrote:

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 

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?

I didn't check for KBL, it may be require for KBL also.


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?

Will add more description about the feature in the commit.

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.\
Main intention of giving one command line option was to keep IPC disable 
for debugging only.
I can remove the command-line option, But for debugging we need some 
option to control the enable/disable.
What would you suggest, which I can use instead of command-line option? 
OR we can totally ignore that?



Signed-off-by: Mahesh Kumar 
---
  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".

agree, keeping this as part of intel_init_pm seems more relevant,
But we have faced some issue because of enabling it early in intel_init_pm,
If BIOS is not enabling the IPC & we only are going to enable first, 
then BIOS was not taking care of programming transition WM
& since we were enabling it early before sanitizing WMs (during 
pm_init), this was causing flickers/underrun.
Didn't tried it recently, but as transition WM are disabled now we 
should not face that issue for now*.


Thanks,
-Mahesh




+
/* 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

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

2016-09-21 Thread Paulo Zanoni 
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 
> 
> 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 
> ---
>  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