[Intel-gfx] [PATCH resend 12/15] drm/i915/dsi: Document always using v3 SHUTDOWN / MIPI_SEQ_DISPLAY_OFF order

[Jani Nikula]

On Mon, 27 Feb 2017, Bob Paauwe <bob.j.paauwe at intel.com> wrote:
> On Sat, 25 Feb 2017 11:42:09 +0100
> Hans de Goede <hdegoede at redhat.com> wrote:
>
>> Hi,
>> 
>> On 24-02-17 18:02, Bob Paauwe wrote:
>> > On Mon, 20 Feb 2017 15:08:42 +0100
>> > Hans de Goede <hdegoede at redhat.com> wrote:
>> >  
>> >> According to the spec for v2 VBTs we should call MIPI_SEQ_DISPLAY_OFF
>> >> before sending SHUTDOWN, where as for v3 VBTs we should send SHUTDOWN
>> >> first.
>> >>
>> >> Since the v2 order has known issues, we use the v3 order everywhere,
>> >> add a comment documenting this.
>> >>
>> >> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++
>> >>  1 file changed, 7 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
>> >> index a8d0948..1914311 100644
>> >> --- a/drivers/gpu/drm/i915/intel_dsi.c
>> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c
>> >> @@ -732,6 +732,11 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder,
>> >>  			I915_WRITE(MIPI_DEVICE_READY(port), 0);
>> >>  	}
>> >>
>> >> +	/*
>> >> +	 * XXX: According to the spec we should send SHUTDOWN before
>> >> +	 * MIPI_SEQ_DISPLAY_OFF only for v3+ VBTs, but testing in the field
>> >> +	 * has shown that we should do this for v2 VBTs too?  
>> > drop the '?'  
>> 
>> I added that because I'm not 100% sure this is true, looking through git
>> history (and android x86 kernel patch-sets) I managed to piece together that
>> at one point in time the v2 sequence was used, but that yielded problems
>> during some testing, what the commits do not tell if is that testing was
>> using boards with v3 VBTs, but assuming v2 tables are out there in the
>> wild then it seems that the v3 order works fine for v2 too.
>> 
>> TLDR I'm not 100% sure about this hence the '?', my main goal with this
>> patch is to document that we're deviating from the spec for v2 tables here.
>
> If anyone else, Jani?, has more information about this, that would be
> good to know.  

I wish. The documentation on this is disgraceful.

> I'd be OK with just stating that "field testing has shown that the v3
> sequence works with v2 VBT's so just use that."

Ack.

>
>> 
>> >> +	 */
>> >>  	if (is_vid_mode(intel_dsi)) {
>> >>  		/* Send Shutdown command to the panel in LP mode */
>> >>  		for_each_dsi_port(port, intel_dsi->ports)
>> >> @@ -764,6 +769,8 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder,
>> >>  	/*
>> >>  	 * if disable packets are sent before sending shutdown packet then in
>> >>  	 * some next enable sequence send turn on packet error is observed
>> >> +	 * XXX spec specifies SHUTDOWN before MIPI_SEQ_DISPLAY_OFF for
>> >> +	 * v3 VBTs, but not for v2 VBTs?
>> >>  	 */
>> >>  	intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF);
>> >>  
>> >
>> > Should XXX be replaced with something?  
>> 
>> XXX is used in many places in intel_dsi.c to indicate code which may need
>> work / which may needs to be investigated further. I followed that and
>> added XXX here since this code is deviating from the spec.
>> 
>> Regards,
>> 
>> Hans

-- 
Jani Nikula, Intel Open Source Technology Center