2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nikula@xxxxxxxxx>:
> From: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx>
>
> The panel power sequencer on vlv doesn't appear to accept changes to its
> T12 power down duration during warm reboots. This change forces a delay
> for warm reboots to the T12 panel timing as defined in the VBT table for
> the connected panel.
>
> Ver2: removed redundant pr_crit(), commented magic value for pp_div_reg
>
> Ver3: moved SYS_RESTART check earlier, new name for pp_div.
>
> Ver4: Minor issue changes
>
> Signed-off-by: Clint Taylor <clinton.a.taylor@xxxxxxxxx>
> [Jani: rebased on current -nightly.]
> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>
> ---
>
> I ended up doing the rebase myself, but I'd like to have a quick review
> before pushing.
>
> Thanks,
> Jani.
> ---
> drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b5ec48913b47..f0d23c435cf6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -28,6 +28,8 @@
> #include <linux/i2c.h>
> #include <linux/slab.h>
> #include <linux/export.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>
> #include <drm/drmP.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_crtc_helper.h>
> @@ -336,6 +338,36 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
> return VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
> }
>
> +/* Reboot notifier handler to shutdown panel power to guarantee T12 timing */
> +static int edp_notify_handler(struct notifier_block *this, unsigned long code,
> + void *unused)
> +{
> + struct intel_dp *intel_dp = container_of(this, typeof(* intel_dp),
> + edp_notifier);
> + struct drm_device *dev = intel_dp_to_dev(intel_dp);
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 pp_div;
> + u32 pp_ctrl_reg, pp_div_reg;
> + enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> +
> + if (!is_edp(intel_dp) || code != SYS_RESTART)
What if someone does a power off and _very quickly_ starts the system again? =P
<insert same statement for the other "code" possibilities>
Also, depending based on the suggestions below, you may not need the
is_edp() check (or you may want to convert it to a WARN).
> + return 0;
> +
> + if (IS_VALLEYVIEW(dev)) {
This check is not really needed. It could also be an early return or a WARN.
> + pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> + pp_div_reg = VLV_PIPE_PP_DIVISOR(pipe);
> + pp_div = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
Or "pp_div = I915_READ(pp_div_reg);", since we just defined it :)
> + pp_div &= PP_REFERENCE_DIVIDER_MASK;
> +
> + /* 0x1F write to PP_DIV_REG sets max cycle delay */
> + I915_WRITE(pp_div_reg, pp_div | 0x1F);
> + I915_WRITE(pp_ctrl_reg, PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
So this is basically turning the panel off and turning the "force VDD"
bit off too, and we're not putting any power domain references back.
Even though this might not be a big problem since we're shutting down
the machine anyway, did we attach a serial cable and check if this
won't print any WARNs while shutting down? Shouldn't we try to make
this function call the other functions that shut down stuff instead of
forcing the panel down here?
> + msleep(intel_dp->panel_power_cycle_delay);
> + }
> +
> + return 0;
> +}
> +
> static bool edp_have_panel_power(struct intel_dp *intel_dp)
> {
> struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -3785,6 +3817,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> edp_panel_vdd_off_sync(intel_dp);
> drm_modeset_unlock(&dev->mode_config.connection_mutex);
> + if (intel_dp->edp_notifier.notifier_call) {
> + unregister_reboot_notifier(&intel_dp->edp_notifier);
> + intel_dp->edp_notifier.notifier_call = NULL;
> + }
> }
> kfree(intel_dig_port);
> }
> @@ -4353,6 +4389,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
> if (is_edp(intel_dp)) {
> intel_dp_init_panel_power_timestamps(intel_dp);
> intel_dp_init_panel_power_sequencer(dev, intel_dp, &power_seq);
> + if (IS_VALLEYVIEW(dev)) {
> + intel_dp->edp_notifier.notifier_call = edp_notify_handler;
> + register_reboot_notifier(&intel_dp->edp_notifier);
Why not put this inside intel_edp_init_connector? If you keep it here,
you also have to undo the notifier at the point
intel_dp_init_connector returns false (a few lines below). If you move
to the _edp version, then it depends on where inside
_edp_init_connector you put this..
> + }
> }
>
> intel_dp_aux_init(intel_dp, intel_connector);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5f7c7bd94d90..87d1715db21d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -541,6 +541,8 @@ struct intel_dp {
> unsigned long last_power_cycle;
> unsigned long last_power_on;
> unsigned long last_backlight_off;
> + struct notifier_block edp_notifier;
> +
> bool use_tps3;
> struct intel_connector *attached_connector;
>
> --
> 2.0.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
