[Intel-gfx] [PATCH v3 7/9] drm/i915/bxt: Enable IPC support
Mahesh Kumar
mahesh1.kumar at intel.com
Thu Sep 22 10:14:30 UTC 2016
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 at 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 at 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);
More information about the Intel-gfx
mailing list