| Message ID | 1432895827-5185-7-git-send-email-gaurav.k.singh@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, May 29, 2015 at 04:06:58PM +0530, Gaurav K Singh wrote: > vblank interrupt should be disabled before starting the disable > sequence for MIPI command mode. Otherwise when pipe is disabled > TE interurpt will be still handled and one memory write command > will be sent with pipe disabled. This makes the pipe hw to get > stuck and it doesn't recover in the next enable sequence causing > display blank out. > > Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> > --- > drivers/gpu/drm/i915/intel_dsi.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 04d8ce0..aeea289 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -513,11 +513,25 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder) > > static void intel_dsi_pre_disable(struct intel_encoder *encoder) > { > + struct drm_device *dev = encoder->base.dev; > struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > + int pipe = intel_crtc->pipe; > enum port port; > > DRM_DEBUG_KMS("\n"); > > + if (is_cmd_mode(intel_dsi)) { > + dev->driver->disable_vblank(dev, pipe); > + > + /* > + * Make sure that the last frame is sent otherwise pipe can get > + * stuck. Currently providing delay time for ~2 vblanks > + * assuming 60fps. > + */ > + mdelay(40); > + } Nope. You need to move around the drm_vblank_off suitably, only that function correctly handles all the book-keeping around vblank interrupts. If this doesn't work out because of ordering we need to dig into this and figure out something. Worst case we need to push the drm_vblank_off call into encoder callbacks for everyone. That's something we already discussed but then decided against. -Daniel > + > if (is_vid_mode(intel_dsi)) { > /* Send Shutdown command to the panel in LP mode */ > for_each_dsi_port(port, intel_dsi->ports) > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 29, 2015 at 07:14:43PM +0200, Daniel Vetter wrote: > On Fri, May 29, 2015 at 04:06:58PM +0530, Gaurav K Singh wrote: > > vblank interrupt should be disabled before starting the disable > > sequence for MIPI command mode. Otherwise when pipe is disabled > > TE interurpt will be still handled and one memory write command > > will be sent with pipe disabled. This makes the pipe hw to get > > stuck and it doesn't recover in the next enable sequence causing > > display blank out. > > > > Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> > > Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dsi.c | 14 ++++++++++++++ > > 1 file changed, 14 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > > index 04d8ce0..aeea289 100644 > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > @@ -513,11 +513,25 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder) > > > > static void intel_dsi_pre_disable(struct intel_encoder *encoder) > > { > > + struct drm_device *dev = encoder->base.dev; > > struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > > + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > > + int pipe = intel_crtc->pipe; > > enum port port; > > > > DRM_DEBUG_KMS("\n"); > > > > + if (is_cmd_mode(intel_dsi)) { > > + dev->driver->disable_vblank(dev, pipe); > > + > > + /* > > + * Make sure that the last frame is sent otherwise pipe can get > > + * stuck. Currently providing delay time for ~2 vblanks > > + * assuming 60fps. > > + */ > > + mdelay(40); > > + } > > Nope. You need to move around the drm_vblank_off suitably, only that > function correctly handles all the book-keeping around vblank interrupts. > If this doesn't work out because of ordering we need to dig into this and > figure out something. Worst case we need to push the drm_vblank_off call > into encoder callbacks for everyone. That's something we already discussed > but then decided against. I seem to be blind, but where exactly is that vblank-driven upload code? -Daniel > -Daniel > > > + > > if (is_vid_mode(intel_dsi)) { > > /* Send Shutdown command to the panel in LP mode */ > > for_each_dsi_port(port, intel_dsi->ports) > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
On 5/29/2015 10:53 PM, Daniel Vetter wrote: > On Fri, May 29, 2015 at 07:14:43PM +0200, Daniel Vetter wrote: >> On Fri, May 29, 2015 at 04:06:58PM +0530, Gaurav K Singh wrote: >>> vblank interrupt should be disabled before starting the disable >>> sequence for MIPI command mode. Otherwise when pipe is disabled >>> TE interurpt will be still handled and one memory write command >>> will be sent with pipe disabled. This makes the pipe hw to get >>> stuck and it doesn't recover in the next enable sequence causing >>> display blank out. >>> >>> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> >>> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_dsi.c | 14 ++++++++++++++ >>> 1 file changed, 14 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >>> index 04d8ce0..aeea289 100644 >>> --- a/drivers/gpu/drm/i915/intel_dsi.c >>> +++ b/drivers/gpu/drm/i915/intel_dsi.c >>> @@ -513,11 +513,25 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder) >>> >>> static void intel_dsi_pre_disable(struct intel_encoder *encoder) >>> { >>> + struct drm_device *dev = encoder->base.dev; >>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); >>> + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); >>> + int pipe = intel_crtc->pipe; >>> enum port port; >>> >>> DRM_DEBUG_KMS("\n"); >>> >>> + if (is_cmd_mode(intel_dsi)) { >>> + dev->driver->disable_vblank(dev, pipe); >>> + >>> + /* >>> + * Make sure that the last frame is sent otherwise pipe can get >>> + * stuck. Currently providing delay time for ~2 vblanks >>> + * assuming 60fps. >>> + */ >>> + mdelay(40); >>> + } >> Nope. You need to move around the drm_vblank_off suitably, only that >> function correctly handles all the book-keeping around vblank interrupts. >> If this doesn't work out because of ordering we need to dig into this and >> figure out something. Worst case we need to push the drm_vblank_off call >> into encoder callbacks for everyone. That's something we already discussed >> but then decided against. > I seem to be blind, but where exactly is that vblank-driven upload code? > -Daniel Hi Daniel, dev->driver->disable_vblank calls valleyview_disable_vblank which already exists. But I did check with drm_vblank_off, it seems to work fine. I will float the updated patch shortly. With regards, Gaurav >> -Daniel >> >>> + >>> if (is_vid_mode(intel_dsi)) { >>> /* Send Shutdown command to the panel in LP mode */ >>> for_each_dsi_port(port, intel_dsi->ports) >>> -- >>> 1.7.9.5 >>> >>> _______________________________________________ >>> Intel-gfx mailing list >>> Intel-gfx@lists.freedesktop.org >>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch
On Tue, Jun 16, 2015 at 10:24:57PM +0530, Singh, Gaurav K wrote: > > > On 5/29/2015 10:53 PM, Daniel Vetter wrote: > >On Fri, May 29, 2015 at 07:14:43PM +0200, Daniel Vetter wrote: > >>On Fri, May 29, 2015 at 04:06:58PM +0530, Gaurav K Singh wrote: > >>>vblank interrupt should be disabled before starting the disable > >>>sequence for MIPI command mode. Otherwise when pipe is disabled > >>>TE interurpt will be still handled and one memory write command > >>>will be sent with pipe disabled. This makes the pipe hw to get > >>>stuck and it doesn't recover in the next enable sequence causing > >>>display blank out. > >>> > >>>Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com> > >>>Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com> > >>>--- > >>> drivers/gpu/drm/i915/intel_dsi.c | 14 ++++++++++++++ > >>> 1 file changed, 14 insertions(+) > >>> > >>>diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > >>>index 04d8ce0..aeea289 100644 > >>>--- a/drivers/gpu/drm/i915/intel_dsi.c > >>>+++ b/drivers/gpu/drm/i915/intel_dsi.c > >>>@@ -513,11 +513,25 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder) > >>> static void intel_dsi_pre_disable(struct intel_encoder *encoder) > >>> { > >>>+ struct drm_device *dev = encoder->base.dev; > >>> struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); > >>>+ struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); > >>>+ int pipe = intel_crtc->pipe; > >>> enum port port; > >>> DRM_DEBUG_KMS("\n"); > >>>+ if (is_cmd_mode(intel_dsi)) { > >>>+ dev->driver->disable_vblank(dev, pipe); > >>>+ > >>>+ /* > >>>+ * Make sure that the last frame is sent otherwise pipe can get > >>>+ * stuck. Currently providing delay time for ~2 vblanks > >>>+ * assuming 60fps. > >>>+ */ > >>>+ mdelay(40); > >>>+ } > >>Nope. You need to move around the drm_vblank_off suitably, only that > >>function correctly handles all the book-keeping around vblank interrupts. > >>If this doesn't work out because of ordering we need to dig into this and > >>figure out something. Worst case we need to push the drm_vblank_off call > >>into encoder callbacks for everyone. That's something we already discussed > >>but then decided against. > >I seem to be blind, but where exactly is that vblank-driven upload code? This question is still open for me. Why exactly do we have to disable vblanks here already? I feel like this is hiding some deeper synchronization issue - just disabling the vblank source doesn't mean they have all be processed correctly by the irq handler. If this is really required then you still have a big race here, even when using drm_vblank_off. -Daneil
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 04d8ce0..aeea289 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -513,11 +513,25 @@ static void intel_dsi_enable_nop(struct intel_encoder *encoder) static void intel_dsi_pre_disable(struct intel_encoder *encoder) { + struct drm_device *dev = encoder->base.dev; struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base); + struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc); + int pipe = intel_crtc->pipe; enum port port; DRM_DEBUG_KMS("\n"); + if (is_cmd_mode(intel_dsi)) { + dev->driver->disable_vblank(dev, pipe); + + /* + * Make sure that the last frame is sent otherwise pipe can get + * stuck. Currently providing delay time for ~2 vblanks + * assuming 60fps. + */ + mdelay(40); + } + if (is_vid_mode(intel_dsi)) { /* Send Shutdown command to the panel in LP mode */ for_each_dsi_port(port, intel_dsi->ports)