[Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi enc disable and blank at bootup

[Daniel Vetter]

On Wed, Oct 14, 2015 at 04:37:27PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> >Sent: Wednesday, October 14, 2015 6:20 PM
> >To: Shankar, Uma
> >Cc: Daniel Vetter; intel-gfx at lists.freedesktop.org; Kumar, Shobhit
> >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt: Fixed dsi enc
> >disable and blank at bootup
> >
> >On Wed, Oct 14, 2015 at 10:20:32AM +0000, Shankar, Uma wrote:
> >>
> >>
> >> >-----Original Message-----
> >> >From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> >> >Daniel Vetter
> >> >Sent: Tuesday, October 13, 2015 9:17 PM
> >> >To: Shankar, Uma
> >> >Cc: intel-gfx at lists.freedesktop.org; Kumar, Shobhit
> >> >Subject: Re: [Intel-gfx] [BXT DSI timing fixes v1 3/3] drm/i915/bxt:
> >> >Fixed dsi enc disable and blank at bootup
> >> >
> >> >On Mon, Oct 12, 2015 at 10:55:03PM +0530, Uma Shankar wrote:
> >> >> During bootup, mipi display was blanking in BXT. This is because
> >> >> during driver load, though encoder, connector were active but crtc
> >> >> returned inactive. This caused sanitize function to disable the DSI
> >> >> panel. In AOS, this is fine since HWC will do a modeset and crtc,
> >> >> connector, encoder mapping will be restored. But in Charging OS, no
> >> >> modeset is called, it just calls DPMS ON/OFF. Hence display doesn't
> >> >> come in COS. This is needed even for seamless booting to Android
> >> >> without a
> >> >blank.
> >> >>
> >> >> This is fine on BYT/CHT since transcoder is common b/w all encoders.
> >> >> But for BXT, there is a separate mipi transcoder. Hence this needs
> >> >> special handling for BXT DSI.
> >> >>
> >> >> Signed-off-by: Uma Shankar <uma.shankar at intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/i915_drv.h      |    3 +++
> >> >>  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++----
> >> >>  2 files changed, 26 insertions(+), 4 deletions(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >> >> b/drivers/gpu/drm/i915/i915_drv.h index bf14096..ae790ec 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> >> @@ -1948,6 +1948,9 @@ struct drm_i915_private {
> >> >>  	/* perform PHY state sanity checks? */
> >> >>  	bool chv_phy_assert[2];
> >> >>
> >> >> +	/* To check if DSI is initializing at bootup */
> >> >> +	bool dsi_enumerating;
> >> >
> >> >See my other comment, you're working around the broken design of
> >> >patch 2 here. Special casing fastboot is a big no-no which needs some
> >> >really good reasons. An example is the special edp clock readout code
> >> >in the encoder callbacks we have for ilk-ivb cpu edp.
> >> >-Daniel
> >> >
> >>
> >> I agree this is not a clean solution but was not getting any better
> >> ideas. As part of driver load and initialization, readout_state
> >> returns encoder, connector as active but all crtc return inactive.
> >> This make sanitize encoder to consider this as inconsistent hw state
> >> and it goes ahead and disables encoder, thereby disabling the BIOS/GOP
> >> Initialization.
> >>
> >> After this only way to recover is a modeset which doesn't come in
> >> cases like Charging OS in Android. Also this will create issues in
> >> having a seamless boot without a blank (a must have for Android devices).
> >>
> >> For EDP case, we can read DDI_FUNC_CTL and get a pipe number and match
> >> it with crtc->pipe to detect EDP is on that pipe and map it to crtc.
> >>
> >> In DSI there is no such mechanism to detect this. Can you suggest some
> >> pointers as to how to approach this issue.
> >
> >MIPI_CTRL(port) has BXT_PIPE_SELECT_A/C bits. Which btw we don't ever seem
> >to check anywhere in intel_dsi_compute_config, which is a pretty stellar bug
> >that running kms_flip even once should have caught.
> >-Daniel
> >--
> 
> These fields were removed from the port control register in the bspec updates. These fields remained in the code
> and should be cleaned up and removed. As per latest bspec status, there is no pipe info in the port control for BXT DSI.
> 
> I am not sure, we could add something like below in compute_config to update transcoder info:
> 
> if (IS_BROXTON(dev)) {
> +               if (intel_dsi->ports & (1 << PORT_A))
> +                       config->cpu_transcoder = TRANSCODER_MIPI_A;
> +               else
> +                       config->cpu_transcoder = TRANSCODER_MIPI_C;
> +       }
> 
> Thoughts ?

That doesn't fix the hw state readout really - we need to fix that to
reflect reality. If dsi is enabled at boot then the corresponding crtc
should state that it's active. So on bxt we also need to check the dsi
port registers besides all the other transcoders. Maybe it makes sense to
extend the cpu_transcoder enum to include MIPI_A/C, but I'm not sure about
that.

But the bug here is in haswell_get_pipe_config. And probably we should
split that out into a broxton_get_pipe_config if the dsi stuff gets too
complicated.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch