| Message ID | 20161118150934.29851-5-mahesh1.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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? > > /* 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 > */
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@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 >> */
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@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 > 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 > > > */ >
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@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 >>>> */
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@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 >>>> */
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); /* 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 */
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@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(+)