| Message ID | 1396289637-1013-2-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jani, can you review this one? It's still needed for us to conform to the eDP timing spec. Thanks, Jesse On Mon, 31 Mar 2014 11:13:56 -0700 Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > With the new checks in place, we can see we're doing things backwards, > so fix them up per the spec. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b6f7087..d540fbe 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > - edp_wait_backlight_off(intel_dp); > - > WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); > > /* By this time the PWM and BLC bits should be off already */ > @@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > return; > > DRM_DEBUG_KMS("\n"); > + > + intel_panel_enable_backlight(intel_dp->attached_connector); > + > /* > * If we enable the backlight right away following a panel power > * on, we may see slight flicker as the panel syncs with the eDP > @@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > - > - intel_panel_enable_backlight(intel_dp->attached_connector); > } > > void intel_edp_backlight_off(struct intel_dp *intel_dp) > @@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > /* PWM must still be enabled here */ > assert_pwm_enabled(intel_dp->attached_connector); > > - intel_panel_disable_backlight(intel_dp->attached_connector); > - > DRM_DEBUG_KMS("\n"); > pp = ironlake_get_pp_control(intel_dp); > pp &= ~EDP_BLC_ENABLE; > @@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > intel_dp->last_backlight_off = jiffies; > + > + edp_wait_backlight_off(intel_dp); > + > + intel_panel_disable_backlight(intel_dp->attached_connector); > } > > static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > With the new checks in place, we can see we're doing things backwards, > so fix them up per the spec. > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b6f7087..d540fbe 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > - edp_wait_backlight_off(intel_dp); > - Please justify moving this wait to intel_edp_backlight_off. I thought the point of these wait calls is such that we'll only end up waiting when we really have to. If this is left as-is, we can do useful stuff *while* waiting (case in point, intel_dp_sink_dpms in intel_disable_dp). Otherwise this looks good to me, although I didn't find proper explanations for everything. Do I understand correctly that the EDP_BLC_ENABLE bit in PP_CONTROL is basically the final switch for enabling/disabling the PWM signal to the eDP? So before this patch, we let the disabled PWM signal through to the panel? BR, Jani. > WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); > > /* By this time the PWM and BLC bits should be off already */ > @@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > return; > > DRM_DEBUG_KMS("\n"); > + > + intel_panel_enable_backlight(intel_dp->attached_connector); > + > /* > * If we enable the backlight right away following a panel power > * on, we may see slight flicker as the panel syncs with the eDP > @@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > - > - intel_panel_enable_backlight(intel_dp->attached_connector); > } > > void intel_edp_backlight_off(struct intel_dp *intel_dp) > @@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > /* PWM must still be enabled here */ > assert_pwm_enabled(intel_dp->attached_connector); > > - intel_panel_disable_backlight(intel_dp->attached_connector); > - > DRM_DEBUG_KMS("\n"); > pp = ironlake_get_pp_control(intel_dp); > pp &= ~EDP_BLC_ENABLE; > @@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > intel_dp->last_backlight_off = jiffies; > + > + edp_wait_backlight_off(intel_dp); > + > + intel_panel_disable_backlight(intel_dp->attached_connector); > } > > static void ironlake_edp_pll_on(struct intel_dp *intel_dp) > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 25 Jun 2014 15:21:12 +0300 Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > With the new checks in place, we can see we're doing things backwards, > > so fix them up per the spec. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index b6f7087..d540fbe 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > > > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > > > - edp_wait_backlight_off(intel_dp); > > - > > Please justify moving this wait to intel_edp_backlight_off. I thought > the point of these wait calls is such that we'll only end up waiting > when we really have to. If this is left as-is, we can do useful stuff > *while* waiting (case in point, intel_dp_sink_dpms in intel_disable_dp). The wait needs to happen between the BLC disable and the backlight disable per the eDP timing spec. We could put the disable into a delayed work queue if you want to reclaim the time, but it should be pretty small compared to a full panel power sequence. The wait here looks like it was to prevent us from re-enabling the backlight too quickly, but I don't have timing info for that; not sure if there are specific requirements there or not. Jesse > Otherwise this looks good to me, although I didn't find proper > explanations for everything. Do I understand correctly that the > EDP_BLC_ENABLE bit in PP_CONTROL is basically the final switch for > enabling/disabling the PWM signal to the eDP? So before this patch, we > let the disabled PWM signal through to the panel? Right, something like that. Enabling PWM starts driving power to some components, but the PP_CONTROL bit controls whether it actually gets out to the panel meaningfully, and at least according to the scope readouts we have, doing it in this order corrects the backward ordering we saw.
On Wed, 25 Jun 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Wed, 25 Jun 2014 15:21:12 +0300 > Jani Nikula <jani.nikula@linux.intel.com> wrote: > >> On Mon, 31 Mar 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > With the new checks in place, we can see we're doing things backwards, >> > so fix them up per the spec. >> > >> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> >> > --- >> > drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------ >> > 1 file changed, 7 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> > index b6f7087..d540fbe 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp.c >> > +++ b/drivers/gpu/drm/i915/intel_dp.c >> > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) >> > >> > DRM_DEBUG_KMS("Turn eDP power off\n"); >> > >> > - edp_wait_backlight_off(intel_dp); >> > - >> >> Please justify moving this wait to intel_edp_backlight_off. I thought >> the point of these wait calls is such that we'll only end up waiting >> when we really have to. If this is left as-is, we can do useful stuff >> *while* waiting (case in point, intel_dp_sink_dpms in intel_disable_dp). > > The wait needs to happen between the BLC disable and the backlight > disable per the eDP timing spec. We could put the disable into a > delayed work queue if you want to reclaim the time, but it should be > pretty small compared to a full panel power sequence. Okay, I'll take your word for it. ;) Do you still want to pursue patch 1? If not, please pick up the comments from there into this one, and add the above as a comment in there too. Thanks, Jani. > > The wait here looks like it was to prevent us from re-enabling the > backlight too quickly, but I don't have timing info for that; not sure > if there are specific requirements there or not. > > Jesse > >> Otherwise this looks good to me, although I didn't find proper >> explanations for everything. Do I understand correctly that the >> EDP_BLC_ENABLE bit in PP_CONTROL is basically the final switch for >> enabling/disabling the PWM signal to the eDP? So before this patch, we >> let the disabled PWM signal through to the panel? > > Right, something like that. Enabling PWM starts driving power to some > components, but the PP_CONTROL bit controls whether it actually gets > out to the panel meaningfully, and at least according to the scope > readouts we have, doing it in this order corrects the backward ordering > we saw. > > -- > Jesse Barnes, Intel Open Source Technology Center
On Thu, Jun 19, 2014 at 11:00:20AM -0700, Jesse Barnes wrote: > Jani, can you review this one? It's still needed for us to conform to > the eDP timing spec. Jani's already goofing off on vacation and I couldn't spot his r-b. Merged anyway, I guess people will scream fast enough if this breaks stuff ;-) -Daniel > > Thanks, > Jesse > > On Mon, 31 Mar 2014 11:13:56 -0700 > Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > > With the new checks in place, we can see we're doing things backwards, > > so fix them up per the spec. > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------ > > 1 file changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index b6f7087..d540fbe 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) > > > > DRM_DEBUG_KMS("Turn eDP power off\n"); > > > > - edp_wait_backlight_off(intel_dp); > > - > > WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); > > > > /* By this time the PWM and BLC bits should be off already */ > > @@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > > return; > > > > DRM_DEBUG_KMS("\n"); > > + > > + intel_panel_enable_backlight(intel_dp->attached_connector); > > + > > /* > > * If we enable the backlight right away following a panel power > > * on, we may see slight flicker as the panel syncs with the eDP > > @@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) > > > > I915_WRITE(pp_ctrl_reg, pp); > > POSTING_READ(pp_ctrl_reg); > > - > > - intel_panel_enable_backlight(intel_dp->attached_connector); > > } > > > > void intel_edp_backlight_off(struct intel_dp *intel_dp) > > @@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > > /* PWM must still be enabled here */ > > assert_pwm_enabled(intel_dp->attached_connector); > > > > - intel_panel_disable_backlight(intel_dp->attached_connector); > > - > > DRM_DEBUG_KMS("\n"); > > pp = ironlake_get_pp_control(intel_dp); > > pp &= ~EDP_BLC_ENABLE; > > @@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) > > I915_WRITE(pp_ctrl_reg, pp); > > POSTING_READ(pp_ctrl_reg); > > intel_dp->last_backlight_off = jiffies; > > + > > + edp_wait_backlight_off(intel_dp); > > + > > + intel_panel_disable_backlight(intel_dp->attached_connector); > > } > > > > static void ironlake_edp_pll_on(struct intel_dp *intel_dp) > > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b6f7087..d540fbe 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1273,8 +1273,6 @@ void intel_edp_panel_off(struct intel_dp *intel_dp) DRM_DEBUG_KMS("Turn eDP power off\n"); - edp_wait_backlight_off(intel_dp); - WARN(!intel_dp->want_panel_vdd, "Need VDD to turn off panel\n"); /* By this time the PWM and BLC bits should be off already */ @@ -1316,6 +1314,9 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) return; DRM_DEBUG_KMS("\n"); + + intel_panel_enable_backlight(intel_dp->attached_connector); + /* * If we enable the backlight right away following a panel power * on, we may see slight flicker as the panel syncs with the eDP @@ -1333,8 +1334,6 @@ void intel_edp_backlight_on(struct intel_dp *intel_dp) I915_WRITE(pp_ctrl_reg, pp); POSTING_READ(pp_ctrl_reg); - - intel_panel_enable_backlight(intel_dp->attached_connector); } void intel_edp_backlight_off(struct intel_dp *intel_dp) @@ -1350,8 +1349,6 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) /* PWM must still be enabled here */ assert_pwm_enabled(intel_dp->attached_connector); - intel_panel_disable_backlight(intel_dp->attached_connector); - DRM_DEBUG_KMS("\n"); pp = ironlake_get_pp_control(intel_dp); pp &= ~EDP_BLC_ENABLE; @@ -1361,6 +1358,10 @@ void intel_edp_backlight_off(struct intel_dp *intel_dp) I915_WRITE(pp_ctrl_reg, pp); POSTING_READ(pp_ctrl_reg); intel_dp->last_backlight_off = jiffies; + + edp_wait_backlight_off(intel_dp); + + intel_panel_disable_backlight(intel_dp->attached_connector); } static void ironlake_edp_pll_on(struct intel_dp *intel_dp)
With the new checks in place, we can see we're doing things backwards, so fix them up per the spec. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)