On 07/04/2014 05:26 AM, Paulo Zanoni wrote:
2014-07-03 19:07 GMT-03:00 Clint Taylor <clinton.a.tay...@intel.com>:
On 07/02/2014 07:40 AM, Paulo Zanoni wrote:
2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nik...@intel.com>:
From: Clint Taylor <clinton.a.tay...@intel.com>
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.tay...@intel.com>
[Jani: rebased on current -nightly.]
Signed-off-by: Jani Nikula <jani.nik...@intel.com>
---
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>
If someone removes and applies power within ~300ms this W/A will break down
and the power sequence will not meet the eDP T12 timing. Since the PPS
sequencer does not allow modifications to the default time intervals during
the initial sequence the only way to guarantee we meet T12 time would be to
delay display block power ungate for 300ms. Further delay of the boot time
was not an acceptable solution for the customers.
My suggestion here was just to not-return in more cases, instead of
only SYS_RESTART.
#define SYS_DOWN 0x0001 /* Notify of system down */
#define SYS_RESTART SYS_DOWN
#define SYS_HALT 0x0002 /* Notify of system halt */
#define SYS_POWER_OFF 0x0003 /* Notify of system power off */
The only real option would be just to ignore the code parameter as the
only other values are full power offs.
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.
Since we currently don't handle PCH offsets this was a safe way to allowing
adding of the PCH functionality later if necessary.
+ 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 :)
Agreed that's another way to do the same thing.
+ 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?
Development of this W/A was done with serial port attached. This function is
the last method called in the I915 driver before power is removed. At reboot
notifier time there is no error handling possible calling the normal
shutdown functions does more housekeeping then we need for a system that
will not have power in < 2 ms.
For this code, even if we don't change it, I think we should at least
put a comment here describing this is an "acceptable" solution for a
machine shutdown, but that this code should not be reused in other
cases since we're forcing a panel shutdown without respecting the PM
references or using the standard ways of waiting for the timers.
Programmers from the future love code refactors, and I fear they may
start using this function for more cases than the current one, so the
comment may prevent future bugs.
+ 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..
Agreed that if the device does not have DPCD and a ghost is created this
notifier would need to be unregistered.
+ }
}
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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
