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

[Daniel Vetter]

On Thu, Oct 13, 2016 at 06:31:37PM +0530, Mahesh Kumar wrote:
> Hi,
> 
> 
> On Thursday 13 October 2016 04:49 PM, Maarten Lankhorst wrote:
> > Op 13-10-16 om 12:58 schreef Kumar, Mahesh:
> > > From: Mahesh Kumar <mahesh1.kumar at intel.com>
> > > 
> > > 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
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar at intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.c  |  2 ++
> > >   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, 19 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index b5f601c..58abbaa 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1415,6 +1415,8 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >   	intel_runtime_pm_enable(dev_priv);
> > > +	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",
> > >   		 driver.name, driver.major, driver.minor, driver.patchlevel,
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index a9c467c..c9ebf23 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -6144,6 +6144,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 2c1897b..45b0fa4 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1766,6 +1766,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 4263212..543aa5d 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4833,6 +4833,21 @@ void intel_update_watermarks(struct drm_crtc *crtc)
> > >   		dev_priv->display.update_wm(crtc);
> > >   }
> > > +void intel_enable_ipc(struct drm_i915_private *dev_priv)
> > > +{
> > > +	u32 val;
> > > +
> > > +	/* enable IPC only for Broxton for now*/
> > > +	if (!IS_BROXTON(dev_priv) || !IS_KABYLAKE(dev_priv))
> > > +		return;
> > > +
> > This comment doesn't match the code.
> > Is it ok to enable IPC right away? Not when the driver is writing the first watermarks? (distrust_bios_wm)
> > And what about suspend/resume, should this flag be set again after resume?
> > 
> > ~Maarten
> hmm.. comment should have been Broxton/Kabylake.
> Yes we can enable IPC at any time. In future BIOS itself may enable IPC.
> (though I'm not sure about the behavior if WM programmed by BIOS are not
> correct)
> We don't reset (save/restore) this during suspend/resume, it's onetime
> programming.

The comment is also not really useful, since it's obvious from the code
what's going on. If you add a comment, explain _why_ you're doing
something. The _what_ should be clear from the code flow, if not you need
to refactor.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch