Re: [RFC 7/7] drm/i915: Modify refs to intel dp timestamps

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 20, 2014 at 06:20:09PM +0530, Vandana Kannan wrote:
> Moving timestamp values to intel_panel as part of moving all refs of PPS to
> intel_panel.
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>

On second though, how does the code still work before applying patch 6&7?
Presuming I've not read this the wrong way round it looks like we only
init the new values, but the code still uses the old values. Which means
it's broken, which isn't ok.

I guess the patch 2 should be redone as two steps:
1. Move code to intel_panel.c, no actual code changes (code movement can't
   be reviewed really without completely redoing the patch, so code
   movement should never change a single line).
2. Do all the variable changes.
3. Do the refactoring like you have it already.

Of course you could also exchange steps 2/3.

But the code must work after every patch completely, that's a hard rule in
linux kernel development. Or maybe I completely missed something here.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c    | 29 ++++++++++++++---------------
>  drivers/gpu/drm/i915/intel_drv.h   |  7 ++++---
>  drivers/gpu/drm/i915/intel_panel.c |  9 +++++++++
>  3 files changed, 27 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index bdb8317..96296a4 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1327,7 +1327,8 @@ static void wait_panel_power_cycle(struct intel_dp *intel_dp)
>  
>  	/* When we disable the VDD override bit last we have to do the manual
>  	 * wait. */
> -	wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle,
> +	wait_remaining_ms_from_jiffies(
> +			intel_connector->panel.pps.last_power_cycle,
>  			intel_connector->panel.pps.panel_power_cycle_delay);
>  
>  	wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE);
> @@ -1336,14 +1337,16 @@ static void wait_panel_power_cycle(struct intel_dp *intel_dp)
>  static void wait_backlight_on(struct intel_dp *intel_dp)
>  {
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> -	wait_remaining_ms_from_jiffies(intel_dp->last_power_on,
> +	wait_remaining_ms_from_jiffies(
> +			intel_connector->panel.pps.last_power_on,
>  			intel_connector->panel.pps.backlight_on_delay);
>  }
>  
>  static void edp_wait_backlight_off(struct intel_dp *intel_dp)
>  {
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> -	wait_remaining_ms_from_jiffies(intel_dp->last_backlight_off,
> +	wait_remaining_ms_from_jiffies(
> +			intel_connector->panel.pps.last_backlight_off,
>  			intel_connector->panel.pps.backlight_off_delay);
>  }
>  
> @@ -1449,6 +1452,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  	struct intel_digital_port *intel_dig_port =
>  		dp_to_dig_port(intel_dp);
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	enum intel_display_power_domain power_domain;
>  	u32 pp;
>  	u32 pp_stat_reg, pp_ctrl_reg;
> @@ -1476,7 +1480,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
>  	I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg));
>  
>  	if ((pp & POWER_TARGET_ON) == 0)
> -		intel_dp->last_power_cycle = jiffies;
> +		intel_connector->panel.pps.last_power_cycle = jiffies;
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_put(dev_priv, power_domain);
> @@ -1553,6 +1557,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	u32 pp;
>  	u32 pp_ctrl_reg;
>  
> @@ -1587,7 +1592,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
>  	POSTING_READ(pp_ctrl_reg);
>  
>  	wait_panel_on(intel_dp);
> -	intel_dp->last_power_on = jiffies;
> +	intel_connector->panel.pps.last_power_on = jiffies;
>  
>  	if (IS_GEN5(dev)) {
>  		pp |= PANEL_POWER_RESET; /* restore panel reset bit */
> @@ -1605,6 +1610,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	enum intel_display_power_domain power_domain;
>  	u32 pp;
>  	u32 pp_ctrl_reg;
> @@ -1631,7 +1637,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
>  	I915_WRITE(pp_ctrl_reg, pp);
>  	POSTING_READ(pp_ctrl_reg);
>  
> -	intel_dp->last_power_cycle = jiffies;
> +	intel_connector->panel.pps.last_power_cycle = jiffies;
>  	wait_panel_off(intel_dp);
>  
>  	/* We got a reference when we enabled the VDD. */
> @@ -1688,6 +1694,7 @@ static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	u32 pp;
>  	u32 pp_ctrl_reg;
>  
> @@ -1706,7 +1713,7 @@ static void _intel_edp_backlight_off(struct intel_dp *intel_dp)
>  
>  	pps_unlock(intel_dp);
>  
> -	intel_dp->last_backlight_off = jiffies;
> +	intel_connector->panel.pps.last_backlight_off = jiffies;
>  	edp_wait_backlight_off(intel_dp);
>  }
>  
> @@ -4712,13 +4719,6 @@ intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connect
>  	}
>  }
>  
> -static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp)
> -{
> -	intel_dp->last_power_cycle = jiffies;
> -	intel_dp->last_power_on = jiffies;
> -	intel_dp->last_backlight_off = jiffies;
> -}
> -
>  void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4920,7 +4920,6 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  
>  	/* We now know it's not a ghost, init power sequence regs. */
>  	pps_lock(intel_dp);
> -	intel_dp_init_panel_power_timestamps(intel_dp);
>  	intel_panel_setup_panel_power_sequencer(intel_connector);
>  	intel_panel_set_pps_registers(intel_connector, port);
>  	pps_unlock(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f99cbc5..e6e2925 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -194,6 +194,10 @@ struct intel_panel {
>  		int panel_power_cycle_delay;
>  		int backlight_on_delay;
>  		int backlight_off_delay;
> +		/* timestamps */
> +		unsigned long last_power_cycle;
> +		unsigned long last_power_on;
> +		unsigned long last_backlight_off;
>  	} pps;
>  };
>  
> @@ -583,9 +587,6 @@ struct intel_dp {
>  	uint8_t train_set[4];
>  	struct delayed_work panel_vdd_work;
>  	bool want_panel_vdd;
> -	unsigned long last_power_cycle;
> -	unsigned long last_power_on;
> -	unsigned long last_backlight_off;
>  
>  	struct notifier_block edp_notifier;
>  
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index c5e6c10..e6a72dd 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1522,6 +1522,13 @@ static struct edp_power_seq vlv_setup_pps(struct intel_connector *connector)
>  			pp_off_reg, pp_div_reg);
>  }
>  
> +static void intel_panel_init_pps_timestamps(struct intel_panel *panel)
> +{
> +	panel->pps.last_power_cycle = jiffies;
> +	panel->pps.last_power_on = jiffies;
> +	panel->pps.last_backlight_off = jiffies;
> +}
> +
>  void
>  intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
>  {
> @@ -1530,6 +1537,8 @@ intel_panel_setup_panel_power_sequencer(struct intel_connector *connector)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct edp_power_seq cur, vbt, spec, final;
>  
> +	intel_panel_init_pps_timestamps(panel);
> +
>  	/* Get chip specific register values */
>  	cur = dev_priv->display.setup_panel_power_seq(connector);
>  
> -- 
> 2.0.1
> 

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