[Intel-gfx] [PATCH] drm/i915/vlv: T12 eDP panel timing enforcement during reboot
Clint Taylor
clinton.a.taylor at intel.com
Fri Jul 4 00:07:30 CEST 2014
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.
> 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
More information about the Intel-gfx
mailing list