[Intel-gfx] [PATCH v5 4/8] drm/i915/bxt: Enable IPC support
Mahesh Kumar
mahesh1.kumar at intel.com
Fri Dec 2 11:51:13 UTC 2016
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?
>
> Thanks,
> Paulo
>
After discussing with HW team, My understanding was not fully correct.
HW/(DMC fw) does the restoring of registers only in case of DC5/6,
In case of DC9 sw need to take-care of restoring. Will add restore part
in intel_display_resume & intel_runtime_resume.
above two places are correct places for restoring?
Regards,
-Mahesh
>> 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
>>>> */
More information about the Intel-gfx
mailing list