[Intel-gfx] [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot
Clint Taylor
clinton.a.taylor at intel.com
Mon Jul 7 19:19:27 CEST 2014
On 07/04/2014 05:26 AM, Paulo Zanoni wrote:
> 2014-07-03 19:07 GMT-03:00 Clint Taylor <clinton.a.taylor at intel.com>:
>> On 07/02/2014 07:40 AM, Paulo Zanoni wrote:
>>>
>>> 2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nikula at intel.com>:
>>>>
>>>> From: Clint Taylor <Clinton.A.Taylor at 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 at intel.com>
>>>> [Jani: rebased on current -nightly.]
>>>> Signed-off-by: Jani Nikula <jani.nikula at 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 at lists.freedesktop.org
>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>>>
>>>
>>>
>>
>
>
>
More information about the Intel-gfx
mailing list