From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH v3 7/7] drm/i915: Only reprobe display on encoder which has received an HPD event Date: Thu, 11 Apr 2013 16:35:04 +0300 Message-ID: <87wqs9nqbb.fsf@intel.com> References: <1365499470-28646-1-git-send-email-eich@freedesktop.org> <1365499470-28646-8-git-send-email-eich@freedesktop.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 39F4BE5DEB for ; Thu, 11 Apr 2013 06:34:50 -0700 (PDT) In-Reply-To: <1365499470-28646-8-git-send-email-eich@freedesktop.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Egbert Eich , intel-gfx@lists.freedesktop.org Cc: Egbert Eich , Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org On Tue, 09 Apr 2013, Egbert Eich wrote: > From: Egbert Eich > > Instead of calling into the DRM helper layer to poll all connectors for > changes in connected displays probe only those connectors which have > received a hotplug event. Thanks, we've all been waiting for someone(tm) to do this. Apart from the bikesheds below, Reviewed-by: Jani Nikula > > Signed-off-by: Egbert Eich > --- > drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++++------ > 1 file changed, 31 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 92041b9..7788536 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -334,6 +334,24 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, > crtc); > } > > +/** > + * drm_helper_hpd_irq_single_connector_event() - call this function with mode_config.mutex lock held > + */ That kernel-doc line should include a one line description what the function does, not what the preconditions are. You can achieve the same with: WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); I'd also like to bikeshed the function name, because we'll be seeing it a lot in the dmesgs through DRM_DEBUG_KMS. Just intel_hpd_irq_event()? > + > +static int intel_hpd_irq_single_connector_event(struct drm_device *dev, struct drm_connector *connector) > +{ > + enum drm_connector_status old_status; > + > + old_status = connector->status; > + > + connector->status = connector->funcs->detect(connector, false); > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] status updated from %d to %d\n", > + connector->base.id, > + drm_get_connector_name(connector), > + old_status, connector->status); And while at it, another bikeshed about having different messages for when old_status == connector->status vs. not. I've always found the "status updated from 1 to 1" messages a bit dumb... ;) > + return (old_status != connector->status); > +} > + > /* > * Handle hotplug events outside the interrupt handler proper. > */ > @@ -348,6 +366,7 @@ static void i915_hotplug_work_func(struct work_struct *work) > struct drm_connector *connector; > unsigned long irqflags; > bool connector_disabled = false; > + bool changed = false; > u32 hpd_event_bits; > > /* HPD irq before everything is fully set up. */ > @@ -387,14 +406,20 @@ static void i915_hotplug_work_func(struct work_struct *work) > > spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > - list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head) > - if (intel_encoder->hot_plug) > - intel_encoder->hot_plug(intel_encoder); > - > + list_for_each_entry(connector, &mode_config->connector_list, head) { > + intel_connector = to_intel_connector(connector); > + intel_encoder = intel_connector->encoder; > + if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { > + if (intel_encoder->hot_plug) > + intel_encoder->hot_plug(intel_encoder); > + if (intel_hpd_irq_single_connector_event(dev, connector)) > + changed = true; > + } > + } > mutex_unlock(&mode_config->mutex); > > - /* Just fire off a uevent and let userspace tell us what to do */ > - drm_helper_hpd_irq_event(dev); > + if (changed) > + drm_kms_helper_hotplug_event(dev); > } > > static void ironlake_handle_rps_change(struct drm_device *dev) > -- > 1.8.1.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx