| Message ID | 20160909080106.17506-8-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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);
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 <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? 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 <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". 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 (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);
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); + /* 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 + */ +void intel_enable_ipc(struct drm_i915_private *dev_priv) +{ + u32 val; + + /* enable IPC only for Broxton for now*/ + 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);