| Message ID | 20170220140845.1714-12-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 20 Feb 2017 15:08:41 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as > we call intel_panel_enable_backlight() / intel_panel_disable_backlight(). > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I'm not sure that the added comments are necessary. Maybe if there was some explanation for why we're using two different mechanisms to enable/disable backlight instead. Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > --- > drivers/gpu/drm/i915/intel_dsi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > index 8408f59..a8d0948 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder, > msleep(100); > > intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON); > - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); > > intel_dsi_port_enable(encoder); > } > > + /* Enable backlight, both pwm and VBT */ > intel_panel_enable_backlight(intel_dsi->attached_connector); > + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); > } > > static void intel_dsi_enable_nop(struct intel_encoder *encoder, > @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder, > > DRM_DEBUG_KMS("\n"); > > + /* Disable backlight, both VBT and pwm */ > + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); > intel_panel_disable_backlight(intel_dsi->attached_connector); > > /* > @@ -762,7 +765,6 @@ 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 > */ > - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); > intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF); > > /* Transition to LP-00 */
Hi, On 24-02-17 18:00, Bob Paauwe wrote: > On Mon, 20 Feb 2017 15:08:41 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as >> we call intel_panel_enable_backlight() / intel_panel_disable_backlight(). >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > I'm not sure that the added comments are necessary. Maybe if there was > some explanation for why we're using two different mechanisms to > enable/disable backlight instead. That assumes I know why there are 2 mechanisms of I have to guess it is because on some boards only one of the mechanisms works, but that is just a guess. Just calling the VBT sequence should be enough on (most?) boards. Regards, Hans > > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > >> --- >> drivers/gpu/drm/i915/intel_dsi.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index 8408f59..a8d0948 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder, >> msleep(100); >> >> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON); >> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); >> >> intel_dsi_port_enable(encoder); >> } >> >> + /* Enable backlight, both pwm and VBT */ >> intel_panel_enable_backlight(intel_dsi->attached_connector); >> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); >> } >> >> static void intel_dsi_enable_nop(struct intel_encoder *encoder, >> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder, >> >> DRM_DEBUG_KMS("\n"); >> >> + /* Disable backlight, both VBT and pwm */ >> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); >> intel_panel_disable_backlight(intel_dsi->attached_connector); >> >> /* >> @@ -762,7 +765,6 @@ 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 >> */ >> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); >> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF); >> >> /* Transition to LP-00 */ > > >
On Sat, 25 Feb 2017 11:37:50 +0100 Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 24-02-17 18:00, Bob Paauwe wrote: > > On Mon, 20 Feb 2017 15:08:41 +0100 > > Hans de Goede <hdegoede@redhat.com> wrote: > > > >> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as > >> we call intel_panel_enable_backlight() / intel_panel_disable_backlight(). > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > I'm not sure that the added comments are necessary. Maybe if there was > > some explanation for why we're using two different mechanisms to > > enable/disable backlight instead. > > That assumes I know why there are 2 mechanisms of I have to guess it > is because on some boards only one of the mechanisms works, but that > is just a guess. Just calling the VBT sequence should be enough on > (most?) boards. > > Regards, > > Hans I don't think I can describe why we have two mechanisms so just dropping the comment would be my preference at this point. It may be that calling the intel_panel_enable_backlight() will make sure the sysfs entries are correct while the VBT sequence simply modifies the hardware. Possibly a good solution would be to enhance the code that executes the VBT sequence to also call the intel_panel_enable_backlight(), but that's not something to change here. > > > > > > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> > > > >> --- > >> drivers/gpu/drm/i915/intel_dsi.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c > >> index 8408f59..a8d0948 100644 > >> --- a/drivers/gpu/drm/i915/intel_dsi.c > >> +++ b/drivers/gpu/drm/i915/intel_dsi.c > >> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder, > >> msleep(100); > >> > >> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON); > >> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); > >> > >> intel_dsi_port_enable(encoder); > >> } > >> > >> + /* Enable backlight, both pwm and VBT */ > >> intel_panel_enable_backlight() (intel_dsi->attached_connector); > >> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); > >> } > >> > >> static void intel_dsi_enable_nop(struct intel_encoder *encoder, > >> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder, > >> > >> DRM_DEBUG_KMS("\n"); > >> > >> + /* Disable backlight, both VBT and pwm */ > >> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); > >> intel_panel_disable_backlight(intel_dsi->attached_connector); > >> > >> /* > >> @@ -762,7 +765,6 @@ 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 > >> */ > >> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); > >> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF); > >> > >> /* Transition to LP-00 */ > > > > > >
On Mon, 27 Feb 2017, Bob Paauwe <bob.j.paauwe@intel.com> wrote: > On Sat, 25 Feb 2017 11:37:50 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi, >> >> On 24-02-17 18:00, Bob Paauwe wrote: >> > On Mon, 20 Feb 2017 15:08:41 +0100 >> > Hans de Goede <hdegoede@redhat.com> wrote: >> > >> >> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as >> >> we call intel_panel_enable_backlight() / intel_panel_disable_backlight(). >> >> >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> > >> > I'm not sure that the added comments are necessary. Maybe if there was >> > some explanation for why we're using two different mechanisms to >> > enable/disable backlight instead. >> >> That assumes I know why there are 2 mechanisms of I have to guess it >> is because on some boards only one of the mechanisms works, but that >> is just a guess. Just calling the VBT sequence should be enough on >> (most?) boards. >> >> Regards, >> >> Hans > > I don't think I can describe why we have two mechanisms so just > dropping the comment would be my preference at this point. I think they're mostly alternatives, and only one or the other gets used, but since the VBT sequence is rather generic, I think it's possible one controls the SoC side and the other the panel side. > It may be that calling the intel_panel_enable_backlight() will make > sure the sysfs entries are correct while the VBT sequence simply > modifies the hardware. > > Possibly a good solution would be to enhance the code that executes the > VBT sequence to also call the intel_panel_enable_backlight(), but > that's not something to change here. Please leave it as it is. BR, Jani. > >> >> >> > >> > Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> >> > >> >> --- >> >> drivers/gpu/drm/i915/intel_dsi.c | 6 ++++-- >> >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> >> index 8408f59..a8d0948 100644 >> >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> >> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder, >> >> msleep(100); >> >> >> >> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON); >> >> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); >> >> >> >> intel_dsi_port_enable(encoder); >> >> } >> >> >> >> + /* Enable backlight, both pwm and VBT */ >> >> intel_panel_enable_backlight() (intel_dsi->attached_connector); >> >> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); >> >> } >> >> >> >> static void intel_dsi_enable_nop(struct intel_encoder *encoder, >> >> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder, >> >> >> >> DRM_DEBUG_KMS("\n"); >> >> >> >> + /* Disable backlight, both VBT and pwm */ >> >> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); >> >> intel_panel_disable_backlight(intel_dsi->attached_connector); >> >> >> >> /* >> >> @@ -762,7 +765,6 @@ 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 >> >> */ >> >> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); >> >> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF); >> >> >> >> /* Transition to LP-00 */ >> > >> > >> >
Hi, On 27-02-17 18:06, Bob Paauwe wrote: > On Sat, 25 Feb 2017 11:37:50 +0100 > Hans de Goede <hdegoede@redhat.com> wrote: > >> Hi, >> >> On 24-02-17 18:00, Bob Paauwe wrote: >>> On Mon, 20 Feb 2017 15:08:41 +0100 >>> Hans de Goede <hdegoede@redhat.com> wrote: >>> >>>> Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as >>>> we call intel_panel_enable_backlight() / intel_panel_disable_backlight(). >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> >>> I'm not sure that the added comments are necessary. Maybe if there was >>> some explanation for why we're using two different mechanisms to >>> enable/disable backlight instead. >> >> That assumes I know why there are 2 mechanisms of I have to guess it >> is because on some boards only one of the mechanisms works, but that >> is just a guess. Just calling the VBT sequence should be enough on >> (most?) boards. >> >> Regards, >> >> Hans > > I don't think I can describe why we have two mechanisms so just > dropping the comment would be my preference at this point. Ok, done for v2 of the patch-set I will go through your other comments tomorrow and then send a v2. Regards, Hans > It may be that calling the intel_panel_enable_backlight() will make > sure the sysfs entries are correct while the VBT sequence simply > modifies the hardware. > > Possibly a good solution would be to enhance the code that executes the > VBT sequence to also call the intel_panel_enable_backlight(), but > that's not something to change here. > >> >> >>> >>> Reviewed-by: Bob Paauwe <bob.j.paauwe@intel.com> >>> >>>> --- >>>> drivers/gpu/drm/i915/intel_dsi.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >>>> index 8408f59..a8d0948 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dsi.c >>>> +++ b/drivers/gpu/drm/i915/intel_dsi.c >>>> @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder, >>>> msleep(100); >>>> >>>> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON); >>>> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); >>>> >>>> intel_dsi_port_enable(encoder); >>>> } >>>> >>>> + /* Enable backlight, both pwm and VBT */ >>>> intel_panel_enable_backlight() (intel_dsi->attached_connector); >>>> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); >>>> } >>>> >>>> static void intel_dsi_enable_nop(struct intel_encoder *encoder, >>>> @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder, >>>> >>>> DRM_DEBUG_KMS("\n"); >>>> >>>> + /* Disable backlight, both VBT and pwm */ >>>> + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); >>>> intel_panel_disable_backlight(intel_dsi->attached_connector); >>>> >>>> /* >>>> @@ -762,7 +765,6 @@ 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 >>>> */ >>>> - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); >>>> intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF); >>>> >>>> /* Transition to LP-00 */ >>> >>> >>> > > >
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 8408f59..a8d0948 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -687,12 +687,13 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder, msleep(100); intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_ON); - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); intel_dsi_port_enable(encoder); } + /* Enable backlight, both pwm and VBT */ intel_panel_enable_backlight(intel_dsi->attached_connector); + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_ON); } static void intel_dsi_enable_nop(struct intel_encoder *encoder, @@ -718,6 +719,8 @@ static void intel_dsi_pre_disable(struct intel_encoder *encoder, DRM_DEBUG_KMS("\n"); + /* Disable backlight, both VBT and pwm */ + intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); intel_panel_disable_backlight(intel_dsi->attached_connector); /* @@ -762,7 +765,6 @@ 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 */ - intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_BACKLIGHT_OFF); intel_dsi_exec_vbt_sequence(intel_dsi, MIPI_SEQ_DISPLAY_OFF); /* Transition to LP-00 */
Execute the MIPI_SEQ_BACKLIGHT_ON/OFF VBT sequences at the same time as we call intel_panel_enable_backlight() / intel_panel_disable_backlight(). Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/gpu/drm/i915/intel_dsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)