Re: [PATCH v5 4/8] drm/i915/bxt: Enable IPC support

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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
> > >    */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx