From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 4/5] drm/i915: Idleness detection for DRRS Date: Fri, 14 Feb 2014 10:30:15 +0000 Message-ID: <20140214103015.GC32602@nuc-i3427.alporthouse.com> References: <1392372142-16905-1-git-send-email-vandana.kannan@intel.com> <1392372142-16905-5-git-send-email-vandana.kannan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id 15A83FB4C6 for ; Fri, 14 Feb 2014 02:30:19 -0800 (PST) Content-Disposition: inline In-Reply-To: <1392372142-16905-5-git-send-email-vandana.kannan@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Vandana Kannan Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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