[Intel-gfx] [PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Mahesh Kumar]

Hi,


On Tuesday 22 November 2016 12:16 AM, Paulo Zanoni wrote:
> Em Sex, 2016-11-18 às 20:39 +0530, Mahesh Kumar escreveu:
>> This patch adds IPC support for platforms. This patch enables IPC
>> only for BXT/KBL platform as for SKL recommendation is to keep is
>> disabled.
>> IPC (Isochronous Priority Control) is the hardware feature, which
>> dynamically controles the memory read priority of Display.
>>
>> When IPC is enabled, plane read requests are sent at high priority
>> until
>> filling above the transition watermark, then the requests are sent at
>> lower priority until dropping below the level 0 watermark.
>> The lower priority requests allow other memory clients to have better
>> memory access. When IPC is disabled, all plane read requests are sent
>> at
>> high priority.
>>
>> Changes since V1:
>>   - Remove commandline parameter to disable ipc
>>   - Address Paulo's comments
>> Changes since V2:
>>   - Address review comments
>>   - Set ipc_enabled flag
>>
>> Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c  |  1 +
>>   drivers/gpu/drm/i915/i915_reg.h  |  1 +
>>   drivers/gpu/drm/i915/intel_drv.h |  1 +
>>   drivers/gpu/drm/i915/intel_pm.c  | 15 +++++++++++++++
>>   4 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c
>> b/drivers/gpu/drm/i915/i915_drv.c
>> index 1b0a589..4074601 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1244,6 +1244,7 @@ int i915_driver_load(struct pci_dev *pdev,
>> const struct pci_device_id *ent)
>>   	intel_runtime_pm_enable(dev_priv);
>>   
>>   	dev_priv->ipc_enabled = false;
>> +	intel_enable_ipc(dev_priv);
> So now we have to places that touch dev_priv->ipc_enabled. This one and
> intel_enable_ipc(). Please move that "dev_priv->ipc_enabled = false"
> line to inside intel_enable_ipc(). It's much easier to read the code
> when there's a single function responsible for setting the appropriate
> value to a variable.
>
>
> Besides, my understanding of your discussion with Maarten in the last
> revision of this patch was that we needed to change where
> intel_enable_ipc() is called in order to make sure the bit stays
> enabled after suspend/resume. If that's not needed, why is it not
> needed?
We don't overwrite DISP_ARB_CTL2 register during suspend/resume
So there will not be any impact of it & handling during suspend/resume 
is not needed.
thanks for review

Regards,
-Mahesh
>>   
>>   	/* Everything is in place, we can now relax! */
>>   	DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index c70c07a..300418a 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6076,6 +6076,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 cd132c2..ad542a2 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1745,6 +1745,7 @@ bool skl_ddb_allocation_overlaps(const struct
>> skl_ddb_entry **entries,
>>   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 df39b50..d8090aa 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -4682,6 +4682,21 @@ void intel_update_watermarks(struct intel_crtc
>> *crtc)
>>   		dev_priv->display.update_wm(crtc);
>>   }
>>   
>> +void intel_enable_ipc(struct drm_i915_private *dev_priv)
>> +{
>> +	u32 val;
>> +
>> +	if (!(IS_BROXTON(dev_priv) || IS_KABYLAKE(dev_priv)))
>> +		return;
>> +
>> +	val = I915_READ(DISP_ARB_CTL2);
>> +
>> +	val |= DISP_IPC_ENABLE;
>> +
>> +	I915_WRITE(DISP_ARB_CTL2, val);
>> +	dev_priv->ipc_enabled = true;
>> +}
>> +
>>   /*
>>    * Lock protecting IPS related data structures
>>    */