[Intel-gfx] [PATCH 1/2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

Tue Nov 29 14:06:31 UTC 2016

On Tue, Nov 29, 2016 at 02:06:20PM +0100, Hans de Goede wrote:
> Hi,
> 
> Thanks for the quick reply.
> 
> On 29-11-16 13:48, Ville Syrjälä wrote:
> > On Tue, Nov 29, 2016 at 01:38:57PM +0100, Hans de Goede wrote:
> >> Looking at the ADF code from the Android kernel sources for a
> >> cherrytrail tablet I noticed that it is calling the
> >> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.
> >>
> >> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
> >> in panel prepare/unprepare hooks") the mainline i915 code was doing the
> >> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
> >> MIPI_SEQ_DEASSERT_RESET.
> >>
> >> Looking at the naming of the sequences that is the right thing to do,
> >> but the problem is, that the old mainline code and the ADF code was
> >> actually calling the right sequence (tested on a cube iwork8 air tablet),
> >> and the swapping of the calling breaks things.
> >>
> >> This breakage was likely not noticed in testing because on cherrytrail,
> >> currently chv_exec_gpio ends up disabling the gpio pins rather then
> >> setting them (this is fixed in the next patch in this patch-set).
> >>
> >> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
> >> places in the enum defining them, so that their (new) names match their
> >> actual use.
> >>
> >> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
> >> Cc: Jani Nikula <jani.nikula at intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
> >>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> >> index 8405b5a..642a5eb 100644
> >> --- a/drivers/gpu/drm/i915/intel_bios.h
> >> +++ b/drivers/gpu/drm/i915/intel_bios.h
> >> @@ -49,11 +49,11 @@ struct edp_power_seq {
> >>  /* MIPI Sequence Block definitions */
> >>  enum mipi_seq {
> >>  	MIPI_SEQ_END = 0,
> >> -	MIPI_SEQ_ASSERT_RESET,
> >> +	MIPI_SEQ_DEASSERT_RESET,
> >>  	MIPI_SEQ_INIT_OTP,
> >>  	MIPI_SEQ_DISPLAY_ON,
> >>  	MIPI_SEQ_DISPLAY_OFF,
> >> -	MIPI_SEQ_DEASSERT_RESET,
> >> +	MIPI_SEQ_ASSERT_RESET,
> >
> > That naming would be against the spec, so I don't think we want to do it
> > like this. Unless the spec is totally wrong, that is.
> 
> Given that both the ADF code and the i915 driver until now have been using
> assert on prepare and deassert on unprepare I'm inclined to believe that
> the spec is wrong. Is the spec available somewhere public ?

I don't think so. And sadly even if it would it wouldn't really help
since about the only thing it says is:

00 – Reserved
01 - MIPIAssertResetPin
02 – MIPISendInitialDcsCmds (Use this sequence type for sending initialization commands in LP mode)
03 - MIPIDisplayOn (Use this sequence type for sending initialization commands in HS mode)
04 – MIPIDisplayOff (Use this sequence type for sending DisplayOff commands in LP mode)
05 – MIPIDeassertResetPin
06 – MIPIBacklightOn
07 - MIPIBacklightOff
08 – MIPITearOn
09 - MIPITearOff
10 - MIPIPanelPowerOn
11 - MIPIPanelPowerOff
Others – Reserved

So pretty much useless if you actually want to write a working driver.

> 
> Also if you look at the v1 sequences with the new names:
> 
> 	MIPI_SEQ_DEASSERT_RESET,
>   	MIPI_SEQ_INIT_OTP,
>   	MIPI_SEQ_DISPLAY_ON,
>   	MIPI_SEQ_DISPLAY_OFF,
> 	MIPI_SEQ_ASSERT_RESET,
> 
> Then they are exactly in the order as one would call them on
> enable, followed by disable, which I believe is not a coincidence.
> 
> > Can you provide the VBT for the affected machine so other people can
> > have a look at it?
> 
> Sure I can do that, what would be the easiest way (both for me to
> produce and for you to consume) to do this ?

/sys/kernel/debug/dri/0/i915_opregion

For the best chance of preserving the dump for posterity I would
suggest filing a new bug and attaching it there.

https://bugs.freedesktop.org/enter_bug.cgi?product=DRI&component=DRM/Intel

> 
> While developing this set, I've added printk calls to the code executing the
> sequences, there are 2 gpios involved nr 60 (backlight enable AFAICT, also used
> by the BACKLIGHT sequences) and 72 (reset / panel_enable ?).
> When efifb is up both are 1 / high.
> 
> With the OLD naming, MIPI_SEQ_ASSERT_RESET does:
> 
> gpio 72 high
> delay
> gpio 72 low
> delay
> gpio 72 high

Hmm. OK so it just toggles the reset pin it seems.

> 
> And DEASSERT does:
> 
> gpio 72 low
> gpio 60 low

And this leaves the reset pin asserted, assuming it's active low,
which your patch would seem to confirm.

> 
> So with the old naming DEASSERT leaves the panel disabled / in reset and
> the backlight disabled, which is exactly what we do not want...

Right. Hmm. If we do flip them over like you suggest I think we'll at
least need a big comment to inform people why we seem to go against the
spec.

I just filed a bug against the spec, but given past history I'm not
expecting that to result in much of anything TBH.

> 
> I guess it would be good if someone could check if my patch helps
> or not on other tablets which use these sequences.

Unfortunaltey my VLV doesn't have them.

Jani has a CHT surface tablet I think, so that might have something?

We would probably want to look at a BXT VBT too. Mika, can you grab one?

I think we might want to attach them all to the same bug so that
everyone can get them easily.

> 
> Regards,
> 
> Hans
> 
> 
> p.s.
> 
> I'm also trying to come up with some patches which properly
> integrate pwm-lpss with the i915 driver instead of it
> throwing a "Failed to own the pwm chip" error. But as soon
> as I hook up things so that pwm_get() returns the pwm-lpss
> pwm0 I hit:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=97330
> 
> I get the same pipe-a stuck (or not seeing vblank irqs?)
> problems sometimes without the pwm-lpss driver in the loop
> but then only sometimes and with pwm-lpss I get this
> problem when loading the i915 driver most of the time
> (but not all the time).
> 
> I've been debugging this a couple of evenings in a row
> now (*) but no luck so far before I fixed the gpio and
> assert/deassert swapping I had commented out the
> chv_exec_gpio call otherwise the screen would go off
> and never come back, with that call commented I was
> seeing the same issue.
> 
> I was hoping that properly resetting the screen when
> fbcon does its disable / re-enable dance would fix this,
> but it does not :|  Any hints would be greatly appreciated.

I don't have any good ideas right now. Some ordering/timing
problem perhaps in the enable/disable sequences.

> 
> *) This series is one result of this, I also have some
> designware i2c pmic mux fixes I need to post
> 
> 
> >>  	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
> >>  	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
> >>  	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
> >> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> index 0d8ff00..579d2f5 100644
> >> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
> >> @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = {
> >>   */
> >>
> >>  static const char * const seq_name[] = {
> >> -	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
> >> +	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
> >>  	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
> >>  	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
> >>  	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
> >> -	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
> >> +	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
> >>  	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
> >>  	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
> >>  	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
> >> --
> >> 2.9.3
> >

-- 
Ville Syrjälä
Intel OTC