Re: [PATCH 4/5] drm/i915: Idleness detection for DRRS

[Chris Wilson <chris@chris-wilson.co.uk>]

On Fri, Feb 14, 2014 at 03:32:21PM +0530, Vandana Kannan wrote:
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1933675..3407af6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3411,11 +3411,17 @@ void intel_dp_encoder_destroy(struct drm_encoder *encoder)
>  	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
>  	struct intel_dp *intel_dp = &intel_dig_port->dp;
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	i2c_del_adapter(&intel_dp->adapter);
>  	drm_encoder_cleanup(encoder);
>  	if (is_edp(intel_dp)) {
>  		cancel_delayed_work_sync(&intel_dp->panel_vdd_work);
> +		/* DRRS cleanup */
> +		if (intel_dp->drrs_state.type == SEAMLESS_DRRS_SUPPORT) {
> +			kfree(dev_priv->drrs.drrs_work);
> +			dev_priv->drrs.drrs_work = NULL;
> +		}

This is dangerous.

 if (dp == dev_priv->drrs.dp) {
   intel_drrs_disable();
   cancel_delayed_work_sync(&dev_priv->drrs.work);
   dev_priv->drrs.dp = NULL;
 }
(call this intel_dp_drrs_fini(dev_priv, dp) rather than touching
dev_priv here - lets try to keep the layering violations to a minimum)

and just embed the drrs work into dev_priv.

Also try to spot the bug in the above logic I just wrote. Caveat lector.

>  		mutex_lock(&dev->mode_config.mutex);
>  		edp_panel_vdd_off_sync(intel_dp);
>  		mutex_unlock(&dev->mode_config.mutex);
> @@ -3799,6 +3805,9 @@ intel_dp_drrs_initialize(struct intel_digital_port *intel_dig_port,
>  		dev_priv->drrs.connector = intel_connector;
>  		dev_priv->drrs.dp = intel_dp;
>  
> +		intel_init_drrs_idleness_detection(dev,
> +			intel_connector, intel_dp);
> +

And this is just plain ugly.

intel_dp is a synoym for intel_connector, so we only need one of them.
You are setting dev_priv state outside of the init() routine only to
clear it inside, and pass all the state you set into the init() routine.

initialize()! Just use init() like everywhere else.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre