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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2014-07-03 19:07 GMT-03:00 Clint Taylor <clinton.a.taylor@xxxxxxxxx>:
> On 07/02/2014 07:40 AM, Paulo Zanoni wrote:
>>
>> 2014-07-02 5:35 GMT-03:00 Jani Nikula <jani.nikula@xxxxxxxxx>:
>>>
>>> From: Clint Taylor <Clinton.A.Taylor@xxxxxxxxx>
>>>
>>> The panel power sequencer on vlv doesn't appear to accept changes to its
>>> T12 power down duration during warm reboots. This change forces a delay
>>> for warm reboots to the T12 panel timing as defined in the VBT table for
>>> the connected panel.
>>>
>>> Ver2: removed redundant pr_crit(), commented magic value for pp_div_reg
>>>
>>> Ver3: moved SYS_RESTART check earlier, new name for pp_div.
>>>
>>> Ver4: Minor issue changes
>>>
>>> Signed-off-by: Clint Taylor <clinton.a.taylor@xxxxxxxxx>
>>> [Jani: rebased on current -nightly.]
>>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>>>
>>> ---
>>>
>>> I ended up doing the rebase myself, but I'd like to have a quick review
>>> before pushing.
>>>
>>> Thanks,
>>> Jani.
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c  | 40
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/i915/intel_drv.h |  2 ++
>>>   2 files changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index b5ec48913b47..f0d23c435cf6 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -28,6 +28,8 @@
>>>   #include <linux/i2c.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/export.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/reboot.h>
>>>   #include <drm/drmP.h>
>>>   #include <drm/drm_crtc.h>
>>>   #include <drm/drm_crtc_helper.h>
>>> @@ -336,6 +338,36 @@ static u32 _pp_stat_reg(struct intel_dp *intel_dp)
>>>                  return
>>> VLV_PIPE_PP_STATUS(vlv_power_sequencer_pipe(intel_dp));
>>>   }
>>>
>>> +/* Reboot notifier handler to shutdown panel power to guarantee T12
>>> timing */
>>> +static int edp_notify_handler(struct notifier_block *this, unsigned long
>>> code,
>>> +                             void *unused)
>>> +{
>>> +       struct intel_dp *intel_dp = container_of(this, typeof(*
>>> intel_dp),
>>> +                                                edp_notifier);
>>> +       struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>> +       struct drm_i915_private *dev_priv = dev->dev_private;
>>> +       u32 pp_div;
>>> +       u32 pp_ctrl_reg, pp_div_reg;
>>> +       enum pipe pipe = vlv_power_sequencer_pipe(intel_dp);
>>> +
>>> +       if (!is_edp(intel_dp) || code != SYS_RESTART)
>>
>>
>> What if someone does a power off and _very quickly_ starts the system
>> again? =P
>> <insert same statement for the other "code" possibilities>
>>
> 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@xxxxxxxxxxxxxxxxxxxxx
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>>
>



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux