Re: [PATCH 1/2] drm/i915/dsi: Send SHUTDOWN only for v3+ VBT's

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: "Shankar, Uma" <uma.shankar@intel.com>
Cc: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Srinivas, Vidya" <vidya.srinivas@intel.com>
Subject: Re: [PATCH 1/2] drm/i915/dsi: Send SHUTDOWN only for v3+ VBT's
Date: Tue, 5 Sep 2017 16:16:02 +0300	[thread overview]
Message-ID: <20170905131602.GK4914@intel.com> (raw)
In-Reply-To: <E7C9878FBA1C6D42A1CA3F62AEB6945F7EFF80AB@BGSMSX104.gar.corp.intel.com>

On Tue, Sep 05, 2017 at 09:27:47AM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Kahola, Mika
> >Sent: Tuesday, September 5, 2017 2:03 PM
> >To: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>
> >Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915/dsi: Send SHUTDOWN only for v3+
> >VBT's
> >
> >On Mon, 2017-09-04 at 18:04 +0300, Ville Syrjälä wrote:
> >> On Mon, Sep 04, 2017 at 10:59:32AM +0300, Mika Kahola wrote:
> >> >
> >> > On Fri, 2017-09-01 at 16:43 +0300, Ville Syrjälä wrote:
> >> > >
> >> > > On Fri, Sep 01, 2017 at 10:51:00AM +0300, Mika Kahola wrote:
> >> > > >
> >> > > >
> >> > > > According to spec we should send SHUTDOWN before
> >> > > > MIPI_SEQ_DISPLAY_OFF for
> >> > > > v3+ VBT's. Testing with VBT v3 the current implementation
> >> > > > yields
> >> > > > the
> >> > > > following error message
> >> > > >
> >> > > > *ERROR* Video mode command 0x00000041 send failed.
> >> > > >
> >> > > > To get rid of this error message, let's limit SHUTDOWN only for
> >> > > > VBT versions 3 or higher.
> >> > > In the patch you limit it to version 4+, which doesn't make sense
> >> > > since AFAIK there is no version 4 of the sequence block.
> >> > It seems that sending SHUTDOWN signal doesn't make any sense either.
> >> > Whenever we send that signal it just causes this error message. Do
> >> > we really need to signal this? From functionality point of view
> >> > there's no difference.
> >> Well, the spec doesn't even explain what this "shut down" command
> >> does.
> >> Is it actually the DCS "display off" command, or something else?
> >It seems to do something. At least the intel_wait_for_register() times out when
> >sending SHUTDOWN signal. Maybe it just shuts down display so we can't even
> >read the registers after that.
> >
> >This brings me to another idea. Maybe we should skip
> >intel_wait_for_register() if we have sent out SHUTDOWN signal?
> >
> >>
> >> Did you try reverting bbdf0b2ff32a ("drm/i915/bxt: Disable device
> >> ready before shutdown command")? That looks suspicious to me. But so
> >> does about half of the DSI code since it never seems to follow the
> >> spec, and we end up doing totally different things on different
> >> platforms without any explanation why that is).
> >I haven't tried that. I will give it a go and see what happens.
> >
> 
> The issue is happening due to Device Ready setting. It was added to resolve a DSI split screen
> issue in  dual link DSI panels. Since DSI dual link is not enabled and will require a bit of work in
> upstream, we should revert this change. Vidya will  send the change and also work on enabling
> dual link support in upstream.
> 
> FYI - SHUTDOWN is a short packet peripheral command that turns off the display in a Video Mode
> display module for power saving. 

There is no SHUTDOWN comamnd in the DCS spec that I can find.

> 
> For DSI, there have been issues with different panels requiring some unconventional changes, and also bspec
> sometimes doesn't cover everything generically.  So, it gets a bit challenging :(
> 
> Regards,
> Uma Shankar
> 
> >> >
> >> >
> >> > >
> >> > >
> >> > > >
> >> > > >
> >> > > >
> >> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102404
> >> > > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> >> > > > ---
> >> > > >  drivers/gpu/drm/i915/intel_dsi.c | 2 +-
> >> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > > >
> >> > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c
> >> > > > b/drivers/gpu/drm/i915/intel_dsi.c
> >> > > > index 2a0f5d3..b48b9b7 100644
> >> > > > --- a/drivers/gpu/drm/i915/intel_dsi.c
> >> > > > +++ b/drivers/gpu/drm/i915/intel_dsi.c
> >> > > > @@ -916,7 +916,7 @@ static void intel_dsi_disable(struct
> >> > > > intel_encoder *encoder,
> >> > > >  	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but field testing
> >> > > >  	 * has shown that the v3 sequence works for v2 VBTs too
> >> > > >  	 */
> >> > > > -	if (is_vid_mode(intel_dsi)) {
> >> > > > +	if (is_vid_mode(intel_dsi) && dev_priv-
> >> > > > >
> >> > > > > vbt.dsi.seq_version > 3) {
> >> > > >  		/* Send Shutdown command to the panel in LP mode */
> >> > > >  		for_each_dsi_port(port, intel_dsi->ports)
> >> > > >  			dpi_send_cmd(intel_dsi, SHUTDOWN, false, port);
> >> > > > --
> >> > > > 2.7.4
> >> > > >
> >> > > > _______________________________________________
> >> > > > Intel-gfx mailing list
> >> > > > Intel-gfx@lists.freedesktop.org
> >> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> > --
> >> > Mika Kahola - Intel OTC
> >--
> >Mika Kahola - Intel OTC
> 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx