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

[Mahesh Kumar]

Hi,


On Tuesday 22 November 2016 08:15 PM, Paulo Zanoni wrote:
> Em Ter, 2016-11-22 às 19:05 +0530, Mahesh Kumar escreveu:
>> 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.
> Just to make sure I understood: any value that our driver sets to this
> register will persist after a suspend/resume cycle? That's not what I
> was expecting. Usually after a suspend/resume cycle a lot of registers
> are reset to their default value, so our driver needs to write them all
> again.
>
> Can you also confirm whether this is true during runtime PM?
My understanding is HW takes care of register save/restore during deeper 
power states(DC5/DC6 or DC9)
Will confirm this & comeback.

Regards,
-Mahesh
> Thanks,
> Paulo
>
>> 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
>>>>     */