| Message ID | 1483953372-26510-1-git-send-email-vidya.srinivas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
This is definitely needed to pass igt test on bxt 'gem_exec_suspend --run-subtest basic-S3' Tested-by: Mika Kahola <mika.kahola@intel.com> On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > From: Uma Shankar <uma.shankar@intel.com> > > Enable MIPI IO WA for BXT DSI as per bspec. > > Signed-off-by: Uma Shankar <uma.shankar@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) > +#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()? > /* 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 */
On Thu, 12 Jan 2017, Mika Kahola <mika.kahola@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@intel.com> > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: >> From: Uma Shankar <uma.shankar@intel.com> >> >> Enable MIPI IO WA for BXT DSI as per bspec. >> >> Signed-off-by: Uma Shankar <uma.shankar@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? >> +#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 */
On Fri, 2017-01-13 at 09:55 +0200, Jani Nikula wrote: > On Thu, 12 Jan 2017, Mika Kahola <mika.kahola@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@intel.com> > > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > > > > > > From: Uma Shankar <uma.shankar@intel.com> > > > > > > Enable MIPI IO WA for BXT DSI as per bspec. > > > > > > Signed-off-by: Uma Shankar <uma.shankar@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. AFAIK this is an uncore register and not exactly part of DPIO. The DPIO phy bits are power requests that go to the P-Unit, so somewhat similar to power well enabling. The DSI usage seems orthogonal to the DPIO phys, so I don't think it makes a lot of sense to do it in intel_dpio_phy.c. Ander > > 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 */
On pe, 2017-01-13 at 09:55 +0200, Jani Nikula wrote: > On Thu, 12 Jan 2017, Mika Kahola <mika.kahola@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@intel.com> > > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > > > From: Uma Shankar <uma.shankar@intel.com> > > > > > > Enable MIPI IO WA for BXT DSI as per bspec. > > > > > > Signed-off-by: Uma Shankar <uma.shankar@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? 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). Btw, what about the 0x160020, 0x160054 regs? According to BSpec we need to program these too in the same sequence. --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 */ >
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@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@intel.com> > > > > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > > > > From: Uma Shankar <uma.shankar@intel.com> > > > > > > > > Enable MIPI IO WA for BXT DSI as per bspec. > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar@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. > > 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). > > Btw, what about the 0x160020, 0x160054 regs? According to BSpec we need > to program these too in the same sequence. > > --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 */ > >
> -----Original Message----- > From: Kahola, Mika > Sent: Thursday, January 12, 2017 5:13 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 10/14] drm/i915: Add MIPI_IO WA > > This is definitely needed to pass igt test on bxt > > 'gem_exec_suspend --run-subtest basic-S3' > > Tested-by: Mika Kahola <mika.kahola@intel.com> > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > > From: Uma Shankar <uma.shankar@intel.com> > > > > Enable MIPI IO WA for BXT DSI as per bspec. > > > > Signed-off-by: Uma Shankar <uma.shankar@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) > > +#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()? Yes, this can be done in intel_dsi_pre_enable itself. Will make the change and re-send. Thanks for the input. > > > /* 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 */ > -- > Mika Kahola - Intel OTC
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Friday, January 13, 2017 9:02 PM > To: Deak, Imre <imre.deak@intel.com> > Cc: Jani Nikula <jani.nikula@linux.intel.com>; Kahola, Mika > <mika.kahola@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander > <ander.conselvan.de.oliveira@intel.com>; imre.deak@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@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@intel.com> > > > > > > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > > > > > From: Uma Shankar <uma.shankar@intel.com> > > > > > > > > > > Enable MIPI IO WA for BXT DSI as per bspec. > > > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar@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
On Mon, 16 Jan 2017, "Srinivas, Vidya" <vidya.srinivas@intel.com> wrote: >> -----Original Message----- >> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> Sent: Friday, January 13, 2017 9:02 PM >> To: Deak, Imre <imre.deak@intel.com> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>; Kahola, Mika >> <mika.kahola@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>; intel- >> gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander >> <ander.conselvan.de.oliveira@intel.com>; imre.deak@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@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@intel.com> >> > > > >> > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: >> > > > > From: Uma Shankar <uma.shankar@intel.com> >> > > > > >> > > > > Enable MIPI IO WA for BXT DSI as per bspec. >> > > > > >> > > > > Signed-off-by: Uma Shankar <uma.shankar@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. When you do, please reorder your patch series to have fixes in front. BR, Jani. >> > >> > --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
On Mon, Jan 16, 2017 at 12:06:56PM +0200, Srinivas, Vidya wrote: > > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Friday, January 13, 2017 9:02 PM > > To: Deak, Imre <imre.deak@intel.com> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; Kahola, Mika > > <mika.kahola@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>; intel- > > gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander > > <ander.conselvan.de.oliveira@intel.com>; imre.deak@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@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@intel.com> > > > > > > > > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > > > > > > From: Uma Shankar <uma.shankar@intel.com> > > > > > > > > > > > > Enable MIPI IO WA for BXT DSI as per bspec. > > > > > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar@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. Ok. I assume the async case will need a generic solution then. > 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 ? Yes, based on the above the connection_mutex provides for the serialization. Just use the existing define as Jani said. > > > > > > 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. Yes ideally, but checking at least if this is really the case would make sense. But I'm ok with the current approach for now. > > > > > > 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. Any reason for having separate patches? --Imre
> -----Original Message----- > From: Deak, Imre > Sent: Wednesday, January 18, 2017 3:46 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Jani Nikula > <jani.nikula@linux.intel.com>; Kahola, Mika <mika.kahola@intel.com>; intel- > gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander > <ander.conselvan.de.oliveira@intel.com>; imre.deak@linux.intel.com > Subject: Re: [Intel-gfx] [PATCH 10/14] drm/i915: Add MIPI_IO WA > > On Mon, Jan 16, 2017 at 12:06:56PM +0200, Srinivas, Vidya wrote: > > > > > > > -----Original Message----- > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > Sent: Friday, January 13, 2017 9:02 PM > > > To: Deak, Imre <imre.deak@intel.com> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>; Kahola, Mika > > > <mika.kahola@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>; > > > intel- gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander > > > <ander.conselvan.de.oliveira@intel.com>; imre.deak@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@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@intel.com> > > > > > > > > > > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > > > > > > > From: Uma Shankar <uma.shankar@intel.com> > > > > > > > > > > > > > > Enable MIPI IO WA for BXT DSI as per bspec. > > > > > > > > > > > > > > Signed-off-by: Uma Shankar <uma.shankar@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. > > Ok. I assume the async case will need a generic solution then. > > > 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 ? > > Yes, based on the above the connection_mutex provides for the > serialization. Just use the existing define as Jani said. > > > > > > > > > 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. > > Yes ideally, but checking at least if this is really the case would make sense. > But I'm ok with the current approach for now. > > > > > > > > > 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. > > Any reason for having separate patches? We can merge them together. Will re-send merging them together. > > --Imre
> -----Original Message----- > From: Jani Nikula [mailto:jani.nikula@linux.intel.com] > Sent: Wednesday, January 18, 2017 3:08 PM > To: Srinivas, Vidya <vidya.srinivas@intel.com>; Ville Syrjälä > <ville.syrjala@linux.intel.com>; Deak, Imre <imre.deak@intel.com> > Cc: Kahola, Mika <mika.kahola@intel.com>; intel-gfx@lists.freedesktop.org; > Conselvan De Oliveira, Ander <ander.conselvan.de.oliveira@intel.com>; > imre.deak@linux.intel.com > Subject: RE: [Intel-gfx] [PATCH 10/14] drm/i915: Add MIPI_IO WA > > On Mon, 16 Jan 2017, "Srinivas, Vidya" <vidya.srinivas@intel.com> wrote: > >> -----Original Message----- > >> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > >> Sent: Friday, January 13, 2017 9:02 PM > >> To: Deak, Imre <imre.deak@intel.com> > >> Cc: Jani Nikula <jani.nikula@linux.intel.com>; Kahola, Mika > >> <mika.kahola@intel.com>; Srinivas, Vidya <vidya.srinivas@intel.com>; > >> intel- gfx@lists.freedesktop.org; Conselvan De Oliveira, Ander > >> <ander.conselvan.de.oliveira@intel.com>; imre.deak@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@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@intel.com> > >> > > > > >> > > > On Mon, 2017-01-09 at 14:46 +0530, Vidya Srinivas wrote: > >> > > > > From: Uma Shankar <uma.shankar@intel.com> > >> > > > > > >> > > > > Enable MIPI IO WA for BXT DSI as per bspec. > >> > > > > > >> > > > > Signed-off-by: Uma Shankar <uma.shankar@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. > > When you do, please reorder your patch series to have fixes in front. Yeah, we will re-send it separately and rebase other patches on top of it. > > BR, > Jani. > > > >> > > >> > --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 > > -- > Jani Nikula, Intel Open Source Technology Center
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(0x138090) +#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); + /* 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 */