[PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[clinton.a.taylor]

From: Clint Taylor <clinton.a.taylor@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

Ver5: Move registration of reboot notifier to edp_connector_init,
      Added warning comment to handler about lack of PM notification.

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    2 ++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b5ec489..7204dee 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,37 @@ 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
+   This function only applicable when panel PM state is not to be tracked */
+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)
+		return 0;
+
+	if (IS_VALLEYVIEW(dev)) {
+		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
+		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
+		pp_div = I915_READ(pp_div_reg);
+		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);
+		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 +3818,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);
 }
@@ -4262,6 +4299,11 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
 	}
 	mutex_unlock(&dev->mode_config.mutex);
 
+	if (IS_VALLEYVIEW(dev)) {
+		intel_dp->edp_notifier.notifier_call = edp_notify_handler;
+		register_reboot_notifier(&intel_dp->edp_notifier);
+	}
+
 	intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
 	intel_panel_setup_backlight(connector);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5f7c7bd..87d1715 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;
 
-- 
1.7.9.5

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Paulo Zanoni]

2014-07-07 17:01 GMT-03:00  <clinton.a.taylor@intel.com>:
> From: Clint Taylor <clinton.a.taylor@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
>
> Ver5: Move registration of reboot notifier to edp_connector_init,
>       Added warning comment to handler about lack of PM notification.

Not everything I pointed was implemented, but at least the ghost eDP
bug was fixed, so the current patch is already an improvement:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    2 ++
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b5ec489..7204dee 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,37 @@ 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
> +   This function only applicable when panel PM state is not to be tracked */
> +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)
> +               return 0;
> +
> +       if (IS_VALLEYVIEW(dev)) {
> +               pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> +               pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> +               pp_div = I915_READ(pp_div_reg);
> +               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);
> +               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 +3818,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);
>  }
> @@ -4262,6 +4299,11 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>         }
>         mutex_unlock(&dev->mode_config.mutex);
>
> +       if (IS_VALLEYVIEW(dev)) {
> +               intel_dp->edp_notifier.notifier_call = edp_notify_handler;
> +               register_reboot_notifier(&intel_dp->edp_notifier);
> +       }
> +
>         intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>         intel_panel_setup_backlight(connector);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5f7c7bd..87d1715 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;
>
> --
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Daniel Vetter]

On Tue, Jul 08, 2014 at 11:06:00AM -0300, Paulo Zanoni wrote:
> 2014-07-07 17:01 GMT-03:00  <clinton.a.taylor@intel.com>:
> > From: Clint Taylor <clinton.a.taylor@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
> >
> > Ver5: Move registration of reboot notifier to edp_connector_init,
> >       Added warning comment to handler about lack of PM notification.
> 
> Not everything I pointed was implemented, but at least the ghost eDP
> bug was fixed, so the current patch is already an improvement:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Picked up for -fixes, thanks for the patch.
-Daniel

> 
> >
> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |    2 ++
> >  2 files changed, 44 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b5ec489..7204dee 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,37 @@ 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
> > +   This function only applicable when panel PM state is not to be tracked */
> > +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)
> > +               return 0;
> > +
> > +       if (IS_VALLEYVIEW(dev)) {
> > +               pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> > +               pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> > +               pp_div = I915_READ(pp_div_reg);
> > +               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);
> > +               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 +3818,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);
> >  }
> > @@ -4262,6 +4299,11 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
> >         }
> >         mutex_unlock(&dev->mode_config.mutex);
> >
> > +       if (IS_VALLEYVIEW(dev)) {
> > +               intel_dp->edp_notifier.notifier_call = edp_notify_handler;
> > +               register_reboot_notifier(&intel_dp->edp_notifier);
> > +       }
> > +
> >         intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
> >         intel_panel_setup_backlight(connector);
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 5f7c7bd..87d1715 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;
> >
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Clint Taylor]

On 07/04/2014 05:26 AM, Paulo Zanoni wrote:
> 2014-07-03 19:07 GMT-03:00 Clint Taylor <clinton.a.taylor@intel.com>:
>> On 07/02/2014 07:40 AM, Paulo Zanoni wrote:
>>>
>>> 2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
>>>>
>>>> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
>>>> [Jani: rebased on current -nightly.]
>>>> Signed-off-by: Jani Nikula <jani.nikula@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
>>>
>>>
>>>
>>>
>>
>
>
>

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Daniel Vetter]

On Thu, Jul 03, 2014 at 11:44:34PM +0300, Jani Nikula wrote:
> On Thu, 03 Jul 2014, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> > On 07/02/2014 01:35 AM, Jani Nikula wrote:
> >> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
> >> [Jani: rebased on current -nightly.]
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >>
> >> ---
> >>
> >> I ended up doing the rebase myself, but I'd like to have a quick review
> >> before pushing.
> >
> > Quick review complete. Everything appears OK.
> 
> See Paulo's review; want to take over?

Imo this is -fixes material, but it looks like it's not yet complete. I'll
wait for a resend safe when someone yells at me that the current patch is
ok-ish (maybe just needs a pimped commit message or a comment somewhere to
address Paulo's concerns).
-Daniel
> 
> Jani.
> 
> 
> >
> >
> >>
> >> 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)
> >> +		return 0;
> >> +
> >> +	if (IS_VALLEYVIEW(dev)) {
> >> +		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));
> >> +		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);
> >> +		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);
> >> +		}
> >>   	}
> >>
> >>   	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;
> >>
> >>
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Paulo Zanoni]

2014-07-03 19:07 GMT-03:00 Clint Taylor <clinton.a.taylor@intel.com>:
> On 07/02/2014 07:40 AM, Paulo Zanoni wrote:
>>
>> 2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
>>>
>>> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
>>> [Jani: rebased on current -nightly.]
>>> Signed-off-by: Jani Nikula <jani.nikula@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.


>
>> 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
>>
>>
>>
>>
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Clint Taylor]

On 07/02/2014 07:40 AM, Paulo Zanoni wrote:
> 2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
>> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
>> [Jani: rebased on current -nightly.]
>> Signed-off-by: Jani Nikula <jani.nikula@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.

> 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.

>
>
>> +               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
>
>
>

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Jani Nikula]

On Thu, 03 Jul 2014, Clint Taylor <clinton.a.taylor@intel.com> wrote:
> On 07/02/2014 01:35 AM, Jani Nikula wrote:
>> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
>> [Jani: rebased on current -nightly.]
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>
>> ---
>>
>> I ended up doing the rebase myself, but I'd like to have a quick review
>> before pushing.
>
> Quick review complete. Everything appears OK.

See Paulo's review; want to take over?

Jani.


>
>
>>
>> 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)
>> +		return 0;
>> +
>> +	if (IS_VALLEYVIEW(dev)) {
>> +		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));
>> +		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);
>> +		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);
>> +		}
>>   	}
>>
>>   	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;
>>
>>
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Clint Taylor]

On 07/02/2014 01:35 AM, Jani Nikula wrote:
> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
> [Jani: rebased on current -nightly.]
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> ---
>
> I ended up doing the rebase myself, but I'd like to have a quick review
> before pushing.

Quick review complete. Everything appears OK.


>
> 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)
> +		return 0;
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		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));
> +		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);
> +		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);
> +		}
>   	}
>
>   	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;
>
>

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Paulo Zanoni]

2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nikula@intel.com>:
> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
> [Jani: rebased on current -nightly.]
> Signed-off-by: Jani Nikula <jani.nikula@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>

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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot

[Jani Nikula]

From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
[Jani: rebased on current -nightly.]
Signed-off-by: Jani Nikula <jani.nikula@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)
+		return 0;
+
+	if (IS_VALLEYVIEW(dev)) {
+		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));
+		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);
+		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);
+		}
 	}
 
 	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

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Jani Nikula]

On Tue, 03 Jun 2014, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>

Clint, the patch no longer applies cleanly. Please submit a rebased
version on top of current -nightly. Sorry for the extra trouble.

BR,
Jani.



> ---
>  drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    2 ++
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5ca68aa9..cede9bc 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>
> @@ -302,6 +304,38 @@ 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	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 )
> +		return 0;
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		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));
> +		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);
> +		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);
> @@ -3344,6 +3378,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  		mutex_lock(&dev->mode_config.mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.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);
>  }
> @@ -3782,6 +3820,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);
> +		}
>  	}
>  
>  	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 328b1a7..6d0d96e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -510,6 +510,8 @@ struct intel_dp {
>  	unsigned long last_power_on;
>  	unsigned long last_backlight_off;
>  	bool psr_setup_done;
> +	struct notifier_block edp_notifier;
> +
>  	bool use_tps3;
>  	struct intel_connector *attached_connector;
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Jani Nikula]

On Tue, 03 Jun 2014, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <Clinton.A.Taylor@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
>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    2 ++
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5ca68aa9..cede9bc 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>
> @@ -302,6 +304,38 @@ 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	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 )
> +		return 0;
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		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));
> +		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);
> +		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);
> @@ -3344,6 +3378,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  		mutex_lock(&dev->mode_config.mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.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);
>  }
> @@ -3782,6 +3820,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);
> +		}
>  	}
>  
>  	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 328b1a7..6d0d96e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -510,6 +510,8 @@ struct intel_dp {
>  	unsigned long last_power_on;
>  	unsigned long last_backlight_off;
>  	bool psr_setup_done;
> +	struct notifier_block edp_notifier;
> +
>  	bool use_tps3;
>  	struct intel_connector *attached_connector;
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[clinton.a.taylor]

From: Clint Taylor <Clinton.A.Taylor@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.taylor@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    2 ++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5ca68aa9..cede9bc 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>
@@ -302,6 +304,38 @@ 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	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 )
+		return 0;
+
+	if (IS_VALLEYVIEW(dev)) {
+		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));
+		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);
+		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);
@@ -3344,6 +3378,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 		mutex_lock(&dev->mode_config.mutex);
 		edp_panel_vdd_off_sync(intel_dp);
 		mutex_unlock(&dev->mode_config.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);
 }
@@ -3782,6 +3820,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);
+		}
 	}
 
 	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 328b1a7..6d0d96e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -510,6 +510,8 @@ struct intel_dp {
 	unsigned long last_power_on;
 	unsigned long last_backlight_off;
 	bool psr_setup_done;
+	struct notifier_block edp_notifier;
+
 	bool use_tps3;
 	struct intel_connector *attached_connector;
 
-- 
1.7.9.5

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Jani Nikula]

On Mon, 02 Jun 2014, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <Clinton.A.Taylor@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.
>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    2 ++
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5ca68aa9..fb7725a 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>
> @@ -302,6 +304,38 @@ 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	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 ))

Should be:

	if (!is_edp(intel_dp) || code != SYS_RESTART)

> +		return 0;
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		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));
> +		pp_div &= PP_REFERENCE_DIVIDER_MASK;
> +
> +		/* 0x1F write to PP_DIV_REG sets max cycle delay */
> +		I915_WRITE(pp_div_reg , pp_div | 0x1F);

Superfluous space before comma.

> +		I915_WRITE(pp_ctrl_reg,
> +			   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> +		msleep(intel_dp->panel_power_cycle_delay);
> +	}

A blank line before final return statement is customary.

> +	return 0;
> +}
> +
>  static bool edp_have_panel_power(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -3344,6 +3378,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  		mutex_lock(&dev->mode_config.mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.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);
>  }
> @@ -3782,6 +3820,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);
> +		}
>  	}
>  
>  	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 328b1a7..ea2cc07 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -510,6 +510,8 @@ struct intel_dp {
>  	unsigned long last_power_on;
>  	unsigned long last_backlight_off;
>  	bool psr_setup_done;
> +	struct notifier_block  edp_notifier;

Use only one space between type and name.

With all of the above fixed it's

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +
>  	bool use_tps3;
>  	struct intel_connector *attached_connector;
>  
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[clinton.a.taylor]

From: Clint Taylor <Clinton.A.Taylor@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.

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |   42 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    2 ++
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5ca68aa9..fb7725a 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>
@@ -302,6 +304,38 @@ 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	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 ))
+		return 0;
+
+	if (IS_VALLEYVIEW(dev)) {
+		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));
+		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);
+		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);
@@ -3344,6 +3378,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 		mutex_lock(&dev->mode_config.mutex);
 		edp_panel_vdd_off_sync(intel_dp);
 		mutex_unlock(&dev->mode_config.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);
 }
@@ -3782,6 +3820,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);
+		}
 	}
 
 	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 328b1a7..ea2cc07 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -510,6 +510,8 @@ struct intel_dp {
 	unsigned long last_power_on;
 	unsigned long last_backlight_off;
 	bool psr_setup_done;
+	struct notifier_block  edp_notifier;
+
 	bool use_tps3;
 	struct intel_connector *attached_connector;
 
-- 
1.7.9.5

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Jani Nikula]

On Sat, 17 May 2014, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <Clinton.A.Taylor@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.

Wrap to about 72 cols.

>
> Ver2: removed redundant pr_crit(), commented magic value for pp_div_reg
>
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>  2 files changed, 44 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5ca68aa9..023efda 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>
> @@ -302,6 +304,39 @@ 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 gaurantee T12 timing */

guarantee

> +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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pp;
> +	u32 pp_ctrl_reg, pp_div_reg;
> +	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> +
> +	if (!is_edp(intel_dp))
> +		return 0;
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> +		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> +		if (code == SYS_RESTART) {

Check for code != SYS_RESTART along with !is_edp above and bail out
early.

> +			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));

Elsewhere pp is used for the control reg. Please use pp_div instead.

You use pp_div_reg inconsistently; the above doesn't have it. (I think
you could drop the temp vars for regs here completely, but up to you.)

> +			pp &= PP_REFERENCE_DIVIDER_MASK;
> +			/* 0x1F write to PP_DIV_REG sets max cycle delay */
> +			I915_WRITE(pp_div_reg , pp | 0x1F);
> +			I915_WRITE(pp_ctrl_reg,
> +					   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> +			/* VBT entries are in tenths of uS convert to mS */

Use s for seconds; S is Siemens. But really, drop the comment
altogether because...

> +			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);

I think you should use msleep(intel_dp->panel_power_cycle_delay)
instead. The units are already taken care of, and it has fallbacks for
when VBT is bogus or zero.

> +		}
> +	}
> +	return 0;
> +}
> +
>  static bool edp_have_panel_power(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -3344,6 +3379,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  		mutex_lock(&dev->mode_config.mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.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);
>  }
> @@ -3782,6 +3821,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);
> +		}
>  	}
>  
>  	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 328b1a7..1ea193a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -512,6 +512,7 @@ struct intel_dp {
>  	bool psr_setup_done;
>  	bool use_tps3;
>  	struct intel_connector *attached_connector;
> +	struct notifier_block  edp_notifier;

This belongs after psr_setup_done field, preferrably add a space after
them to separate edp stuff from the rest. (I think we should have a sub
struct for edp in the long run.)

BR,
Jani.

>  
>  	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>  	/*
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[clinton.a.taylor]

From: Clint Taylor <Clinton.A.Taylor@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

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5ca68aa9..023efda 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>
@@ -302,6 +304,39 @@ 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 gaurantee 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 pp;
+	u32 pp_ctrl_reg, pp_div_reg;
+	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
+
+	if (!is_edp(intel_dp))
+		return 0;
+
+	if (IS_VALLEYVIEW(dev)) {
+		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
+		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
+		if (code == SYS_RESTART) {
+			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
+			pp &= PP_REFERENCE_DIVIDER_MASK;
+			/* 0x1F write to PP_DIV_REG sets max cycle delay */
+			I915_WRITE(pp_div_reg , pp | 0x1F);
+			I915_WRITE(pp_ctrl_reg,
+					   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
+			/* VBT entries are in tenths of uS convert to mS */
+			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);
+		}
+	}
+	return 0;
+}
+
 static bool edp_have_panel_power(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -3344,6 +3379,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 		mutex_lock(&dev->mode_config.mutex);
 		edp_panel_vdd_off_sync(intel_dp);
 		mutex_unlock(&dev->mode_config.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);
 }
@@ -3782,6 +3821,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);
+		}
 	}
 
 	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 328b1a7..1ea193a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -512,6 +512,7 @@ struct intel_dp {
 	bool psr_setup_done;
 	bool use_tps3;
 	struct intel_connector *attached_connector;
+	struct notifier_block  edp_notifier;
 
 	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 	/*
-- 
1.7.9.5

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Jesse Barnes]

On Wed, 14 May 2014 00:32:30 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, May 13, 2014 at 02:53:22PM -0700, Jesse Barnes wrote:
> > On Tue, 13 May 2014 22:26:08 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> > 
> > > On Tue, May 13, 2014 at 11:51:11AM -0700, clinton.a.taylor@intel.com wrote:
> > > > From: Clint Taylor <Clinton.A.Taylor@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.
> > > > 
> > > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |    1 +
> > > >  2 files changed, 44 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 5ca68aa9..03ac64f 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>
> > > > @@ -302,6 +304,39 @@ 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 gaurantee 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	u32 pp;
> > > > +	u32 pp_ctrl_reg, pp_div_reg;
> > > > +	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> > > > +
> > > > +	if (!is_edp(intel_dp))
> > > > +		return 0;
> > > > +
> > > > +	if (IS_VALLEYVIEW(dev)) {
> > > > +		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> > > > +		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> > > > +		if (code == SYS_RESTART) {
> > > > +			pr_crit("eDP Notifier Handler\n");
> > > > +			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
> > > > +			pp &= PP_REFERENCE_DIVIDER_MASK;
> > > > +			I915_WRITE(pp_div_reg , pp | 0x1F);
> > > > +			I915_WRITE(pp_ctrl_reg,
> > > > +				   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> > > > +			/* VBT entries are in tenths of uS convert to mS */
> > > > +			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);
> > > 
> > > Imo just compute the desired delay before setting up this reboot handler
> > > and pass it as the argument.
> > > 
> > > But that leaves a bit the question why we don't need that everywhere else
> > > and what exactly this does? I'm a bit confused with the commit message.
> > > -Daniel
> > 
> > We could potentially do it on other platforms, but it's really only
> > needed on those that don't do any display stuff in the BIOS (e.g.
> > Chromebooks).  In that case, a warm reboot might immediately jump back
> > to driver init, and if we haven't full done a PPS, we'll get into
> > trouble and potentially confuse the panel in the worst case.
> > 
> > Previous BIOS-free systems may have had this problem too, but this was
> > the first one we got an actual eDP timing spec violation about, so it's
> > a good first step.  It can easily be extended as needed.
> > 
> > Computing the delay ahead of time in eDP init would make it more
> > platform agnostic, and we could key off a separate flag as to whether
> > the delay was needed, but the above is pretty simple too, albeit VLV
> > specific.
> > 
> > The pr_crit() could probably be dropped though, and the magic 0x1f
> > needs a comment.
> 
> In that case I think rolling this out everywhere can't hurt. Gives us a
> notch more testing and rebooting tends to not be a performance critical
> thing really.
> 
> Otoh I've thought the hardware ensure that the t12 timing is obeyed under
> all cases? Does that get lost in the reset?

Yeah on reset the PPS will get reset too, so the off->on delay may not
be honored.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Daniel Vetter]

On Tue, May 13, 2014 at 02:53:22PM -0700, Jesse Barnes wrote:
> On Tue, 13 May 2014 22:26:08 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Tue, May 13, 2014 at 11:51:11AM -0700, clinton.a.taylor@intel.com wrote:
> > > From: Clint Taylor <Clinton.A.Taylor@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.
> > > 
> > > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |    1 +
> > >  2 files changed, 44 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 5ca68aa9..03ac64f 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>
> > > @@ -302,6 +304,39 @@ 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 gaurantee 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	u32 pp;
> > > +	u32 pp_ctrl_reg, pp_div_reg;
> > > +	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> > > +
> > > +	if (!is_edp(intel_dp))
> > > +		return 0;
> > > +
> > > +	if (IS_VALLEYVIEW(dev)) {
> > > +		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> > > +		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> > > +		if (code == SYS_RESTART) {
> > > +			pr_crit("eDP Notifier Handler\n");
> > > +			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
> > > +			pp &= PP_REFERENCE_DIVIDER_MASK;
> > > +			I915_WRITE(pp_div_reg , pp | 0x1F);
> > > +			I915_WRITE(pp_ctrl_reg,
> > > +				   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> > > +			/* VBT entries are in tenths of uS convert to mS */
> > > +			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);
> > 
> > Imo just compute the desired delay before setting up this reboot handler
> > and pass it as the argument.
> > 
> > But that leaves a bit the question why we don't need that everywhere else
> > and what exactly this does? I'm a bit confused with the commit message.
> > -Daniel
> 
> We could potentially do it on other platforms, but it's really only
> needed on those that don't do any display stuff in the BIOS (e.g.
> Chromebooks).  In that case, a warm reboot might immediately jump back
> to driver init, and if we haven't full done a PPS, we'll get into
> trouble and potentially confuse the panel in the worst case.
> 
> Previous BIOS-free systems may have had this problem too, but this was
> the first one we got an actual eDP timing spec violation about, so it's
> a good first step.  It can easily be extended as needed.
> 
> Computing the delay ahead of time in eDP init would make it more
> platform agnostic, and we could key off a separate flag as to whether
> the delay was needed, but the above is pretty simple too, albeit VLV
> specific.
> 
> The pr_crit() could probably be dropped though, and the magic 0x1f
> needs a comment.

In that case I think rolling this out everywhere can't hurt. Gives us a
notch more testing and rebooting tends to not be a performance critical
thing really.

Otoh I've thought the hardware ensure that the t12 timing is obeyed under
all cases? Does that get lost in the reset?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Jesse Barnes]

On Tue, 13 May 2014 22:26:08 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, May 13, 2014 at 11:51:11AM -0700, clinton.a.taylor@intel.com wrote:
> > From: Clint Taylor <Clinton.A.Taylor@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.
> > 
> > Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |    1 +
> >  2 files changed, 44 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 5ca68aa9..03ac64f 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>
> > @@ -302,6 +304,39 @@ 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 gaurantee 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_device *dev = intel_dig_port->base.base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	u32 pp;
> > +	u32 pp_ctrl_reg, pp_div_reg;
> > +	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> > +
> > +	if (!is_edp(intel_dp))
> > +		return 0;
> > +
> > +	if (IS_VALLEYVIEW(dev)) {
> > +		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> > +		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> > +		if (code == SYS_RESTART) {
> > +			pr_crit("eDP Notifier Handler\n");
> > +			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
> > +			pp &= PP_REFERENCE_DIVIDER_MASK;
> > +			I915_WRITE(pp_div_reg , pp | 0x1F);
> > +			I915_WRITE(pp_ctrl_reg,
> > +				   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> > +			/* VBT entries are in tenths of uS convert to mS */
> > +			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);
> 
> Imo just compute the desired delay before setting up this reboot handler
> and pass it as the argument.
> 
> But that leaves a bit the question why we don't need that everywhere else
> and what exactly this does? I'm a bit confused with the commit message.
> -Daniel

We could potentially do it on other platforms, but it's really only
needed on those that don't do any display stuff in the BIOS (e.g.
Chromebooks).  In that case, a warm reboot might immediately jump back
to driver init, and if we haven't full done a PPS, we'll get into
trouble and potentially confuse the panel in the worst case.

Previous BIOS-free systems may have had this problem too, but this was
the first one we got an actual eDP timing spec violation about, so it's
a good first step.  It can easily be extended as needed.

Computing the delay ahead of time in eDP init would make it more
platform agnostic, and we could key off a separate flag as to whether
the delay was needed, but the above is pretty simple too, albeit VLV
specific.

The pr_crit() could probably be dropped though, and the magic 0x1f
needs a comment.

Thanks,
--
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[Daniel Vetter]

On Tue, May 13, 2014 at 11:51:11AM -0700, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <Clinton.A.Taylor@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.
> 
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5ca68aa9..03ac64f 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>
> @@ -302,6 +304,39 @@ 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 gaurantee 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 pp;
> +	u32 pp_ctrl_reg, pp_div_reg;
> +	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
> +
> +	if (!is_edp(intel_dp))
> +		return 0;
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
> +		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
> +		if (code == SYS_RESTART) {
> +			pr_crit("eDP Notifier Handler\n");
> +			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
> +			pp &= PP_REFERENCE_DIVIDER_MASK;
> +			I915_WRITE(pp_div_reg , pp | 0x1F);
> +			I915_WRITE(pp_ctrl_reg,
> +				   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
> +			/* VBT entries are in tenths of uS convert to mS */
> +			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);

Imo just compute the desired delay before setting up this reboot handler
and pass it as the argument.

But that leaves a bit the question why we don't need that everywhere else
and what exactly this does? I'm a bit confused with the commit message.
-Daniel

> +		}
> +	}
> +	return 0;
> +}
> +
>  static bool edp_have_panel_power(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> @@ -3344,6 +3379,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  		mutex_lock(&dev->mode_config.mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.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);
>  }
> @@ -3782,6 +3821,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);
> +		}
>  	}
>  
>  	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 328b1a7..1ea193a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -512,6 +512,7 @@ struct intel_dp {
>  	bool psr_setup_done;
>  	bool use_tps3;
>  	struct intel_connector *attached_connector;
> +	struct notifier_block  edp_notifier;
>  
>  	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
>  	/*
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot.

[clinton.a.taylor]

From: Clint Taylor <Clinton.A.Taylor@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.

Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5ca68aa9..03ac64f 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>
@@ -302,6 +304,39 @@ 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 gaurantee 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 intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 pp;
+	u32 pp_ctrl_reg, pp_div_reg;
+	enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
+
+	if (!is_edp(intel_dp))
+		return 0;
+
+	if (IS_VALLEYVIEW(dev)) {
+		pp_ctrl_reg = VLV_PIPE_PP_CONTROL(pipe);
+		pp_div_reg  = VLV_PIPE_PP_DIVISOR(pipe);
+		if (code == SYS_RESTART) {
+			pr_crit("eDP Notifier Handler\n");
+			pp = I915_READ(VLV_PIPE_PP_DIVISOR(pipe));
+			pp &= PP_REFERENCE_DIVIDER_MASK;
+			I915_WRITE(pp_div_reg , pp | 0x1F);
+			I915_WRITE(pp_ctrl_reg,
+				   PANEL_UNLOCK_REGS | PANEL_POWER_OFF);
+			/* VBT entries are in tenths of uS convert to mS */
+			msleep(dev_priv->vbt.edp_pps.t11_t12 / 10);
+		}
+	}
+	return 0;
+}
+
 static bool edp_have_panel_power(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -3344,6 +3379,10 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
 		mutex_lock(&dev->mode_config.mutex);
 		edp_panel_vdd_off_sync(intel_dp);
 		mutex_unlock(&dev->mode_config.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);
 }
@@ -3782,6 +3821,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);
+		}
 	}
 
 	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 328b1a7..1ea193a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -512,6 +512,7 @@ struct intel_dp {
 	bool psr_setup_done;
 	bool use_tps3;
 	struct intel_connector *attached_connector;
+	struct notifier_block  edp_notifier;
 
 	uint32_t (*get_aux_clock_divider)(struct intel_dp *dp, int index);
 	/*
-- 
1.7.9.5