| Message ID | 20170220140845.1714-13-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 20 Feb 2017 15:08:42 +0100 Hans de Goede <hdegoede@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@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 '?' > + */ > 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?
Hi, On 24-02-17 18:02, Bob Paauwe wrote: > On Mon, 20 Feb 2017 15:08:42 +0100 > Hans de Goede <hdegoede@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@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 (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
On Sat, 25 Feb 2017 11:42:09 +0100 Hans de Goede <hdegoede@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@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@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'd be OK with just stating that "field testing has shown that the v3 sequence works with v2 VBT's so just use that." > > >> + */ > >> 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
On Mon, 27 Feb 2017, Bob Paauwe <bob.j.paauwe@intel.com> wrote: > On Sat, 25 Feb 2017 11:42:09 +0100 > Hans de Goede <hdegoede@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@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@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
Hi, On 27-02-17 18:16, Bob Paauwe wrote: > On Sat, 25 Feb 2017 11:42:09 +0100 > Hans de Goede <hdegoede@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@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@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'd be OK with just stating that "field testing has shown that the v3 > sequence works with v2 VBT's so just use that." Ok, done for v2. Regards, Hans > >> >>>> + */ >>>> 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 > > >
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? + */ 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);
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@redhat.com> --- drivers/gpu/drm/i915/intel_dsi.c | 7 +++++++ 1 file changed, 7 insertions(+)