[Intel-gfx] [PATCH 10/14] drm/i915: Add MIPI_IO WA

[Srinivas, Vidya]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Friday, January 13, 2017 9:02 PM
> To: Deak, Imre <imre.deak at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>; Kahola, Mika
> <mika.kahola at intel.com>; Srinivas, Vidya <vidya.srinivas at intel.com>; intel-
> gfx at lists.freedesktop.org; Conselvan De Oliveira, Ander
> <ander.conselvan.de.oliveira at intel.com>; imre.deak at linux.intel.com
> Subject: Re: [Intel-gfx] [PATCH 10/14] drm/i915: Add MIPI_IO WA
> 
> On Fri, Jan 13, 2017 at 05:03:33PM +0200, Imre Deak wrote:
> > On pe, 2017-01-13 at 09:55 +0200, Jani Nikula wrote:
> > > On Thu, 12 Jan 2017, Mika Kahola <mika.kahola at intel.com> wrote:
> > > > This is definitely needed to pass igt test on bxt
> > > >
> > > > 'gem_exec_suspend --run-subtest basic-S3'
> > > >
> > > > Tested-by: Mika Kahola <mika.kahola at intel.com>
> > > >
> > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote:
> > > > > From: Uma Shankar <uma.shankar at intel.com>
> > > > >
> > > > > Enable MIPI IO WA for BXT DSI as per bspec.
> > > > >
> > > > > Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_reg.h  | 3 +++
> > > > >  drivers/gpu/drm/i915/intel_dsi.c | 9 +++++++++
> > > > >  2 files changed, 12 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > > b/drivers/gpu/drm/i915/i915_reg.h index 71b978a..b9d7e98 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -8301,6 +8301,9 @@ enum {
> > > > >  #define _BXT_MIPIC_PORT_CTRL
> 	0x6B8C0
> > > > >  #define BXT_MIPI_PORT_CTRL(tc)	_MMIO_MIPI(tc,
> > > > > _BXT_MIPIA_PORT_CTRL, _BXT_MIPIC_PORT_CTRL)
> > > > >
> > > > > +#define BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR
> 	_MMIO(0
> > > > > x138090)
> > >
> > > Observe that this register is already defined as
> > > BXT_P_CR_GT_DISP_PWRON, and already used in intel_dpio_phy.c. It
> > > seems to me changing the bits in this register should be hooked at the
> dpio level.
> > >
> > > Imre?
> >
> > At least the RMW access for this register would need locking against a
> > concurrent access via the DDI encoder enable/disable code?
> 
> Full modesets should be serialized by connection_mutex, or perhaps by
> some other thing with async modesets. Although I have a feeling that if
> we're doing async modeset commits without locks half the driveer is going to
> be on fire. Not sure what people are doing/have planned there.
I hope, with the current design, writing to this register from DSI path should
be okay and we don't need to take any explicit locks. Is this understanding
correct ?
> 
> >
> > We should also make sure the IO is turned off during booting/resuming
> > if DSI won't be used (and so the DSI disable hook won't be called),
> > see the BSpec "Broxton Sequences to Initialize Display". We could do
> > this by enabling/disabling the IO via the power well code which would
> > solve the locking issue too. This would mean using
> > POWER_DOMAIN_PORT_DSI, or adding a new power domain if diverging
> from
> > the BSpec sequence is a problem (would be worth checking with HW
> > people, since AFAICS we've been doing this so far).
AFAIK, this will be taken care in BIOS itself if there is no DSI connected.
If DSI is connected, this can be controlled in DSI disable/enable sequence.
> >
> > Btw, what about the 0x160020, 0x160054 regs? According to BSpec we
> > need to program these too in the same sequence.
Yes, this is needed. We have a patch for this. Will float the same for review.
Thanks for the input.
> >
> > --Imre
> >
> > > > > +#define  MIPIO_RST_CTRL					(1 <<
> > > > > 2)
> > > > > +
> > > > >  #define  DPI_ENABLE					(1 << 31)
> > > > > /* A + C */
> > > > >  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_SHIFT		27
> > > > >  #define  MIPIA_MIPI4DPHY_DELAY_COUNT_MASK		(0xf
> << 27)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> > > > > b/drivers/gpu/drm/i915/intel_dsi.c
> > > > > index a4bda92..9252490 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> > > > > @@ -366,6 +366,11 @@ static void bxt_dsi_device_ready(struct
> > > > > intel_encoder *encoder)
> > > > >
> > > > >  	DRM_DEBUG_KMS("\n");
> > > > >
> > > > > +	/* Add MIPI IO reset programming for modeset */
> > > > > +	val =
> I915_READ(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR);
> > > > > +
> 	I915_WRITE(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR,
> > > > > +					val | MIPIO_RST_CTRL);
> > > > > +
> > > > Should we move this WA to intel_dsi_pre_enable() as the
> > > > counterpart of this WA is defined intel_dsi_post_disable()?
> > >
> > > As I said, this should probably be managed in intel_dpio_phy.c.
> > >
> > > And if not, this is BXT specific, and this hunk runs it on
> > > everything else too.
> > >
> > > BR,
> > > Jani.
> > >
> > >
> > > >
> > > > >  	/* Enable MIPI PHY transparent latch */
> > > > >  	for_each_dsi_port(port, intel_dsi->ports) {
> > > > >  		val = I915_READ(BXT_MIPI_PORT_CTRL(port));
> > > > > @@ -757,6 +762,10 @@ static void intel_dsi_post_disable(struct
> > > > > intel_encoder *encoder,
> > > > >  	drm_panel_power_off(intel_dsi->panel);
> > > > >  	msleep(intel_dsi->panel_off_delay);
> > > > >
> > > > > +	val =
> I915_READ(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR);
> > > > > +
> 	I915_WRITE(BXT_P_CR_GT_DISP_PWRON_0_2_0_GTTMMADR,
> > > > > +					val & ~MIPIO_RST_CTRL);
> > > > > +
> > > > >  	intel_disable_dsi_pll(encoder);
> > > > >
> > > > >  	/* Panel Disable over CRC PMIC */
> > >
> 
> --
> Ville Syrjälä
> Intel OTC