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 ?
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 ?
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
And DEASSERT does:
gpio 72 low
gpio 60 low
So with the old naming DEASSERT leaves the panel disabled / in reset and
the backlight disabled, which is exactly what we do not want...
I guess it would be good if someone could check if my patch helps
or not on other tablets which use these sequences.
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.
*) 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
>
