From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm: call connector->dpms(OFF) when disabling Date: Fri, 28 Sep 2012 11:56:05 +0200 Message-ID: <20120928095605.GJ2098@bremse> References: <1348817238-30228-1-git-send-email-rob.clark@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-bk0-f49.google.com (mail-bk0-f49.google.com [209.85.214.49]) by gabe.freedesktop.org (Postfix) with ESMTP id BC5D1A0AAC for ; Fri, 28 Sep 2012 02:56:11 -0700 (PDT) Received: by bkwj4 with SMTP id j4so2932145bkw.36 for ; Fri, 28 Sep 2012 02:56:10 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1348817238-30228-1-git-send-email-rob.clark@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Rob Clark Cc: Rob Clark , patches@linaro.org, dri-devel@lists.freedesktop.org, rogerq@ti.com List-Id: dri-devel@lists.freedesktop.org On Fri, Sep 28, 2012 at 09:27:18AM +0200, Rob Clark wrote: > From: Rob Clark > > When disabling unused connectors, be sure to call connector->dpms(OFF), > so if there is actually some IP to turn off (such as external bridge > chips, etc), these actually do get turned off. > > Signed-off-by: Rob Clark > Tested-by: Roger Quadros I think this runs counter to the crtc helper design: connectors are pretty much just dummy entities, and any dpms handling is done in the encoders/crtcs. If you call connector->funcs->dpms you don't call a crtc helper function, but actually the official dpms interface (which then again calls down into the encoder/crtc dpms callbacks (which are again crtc helper private). This functions are called again a few lines down in disable_unused_functions anyway, so the only way this changes behaviour is if you already overwrite the dpms interface and don't directly use drm_helper_connector_dpms. Which is imo a clear sign that the crtc helper is a misfit for your hw and you should stop using it ;-) And adding special-case handling like this into common code, especially if it weakens the semantic concepts in the helper layer is a recipe for a maintainance disaster a few years down the road. Hence NACKed-by: Daniel Vetter Cheers, Daniel > > --- > drivers/gpu/drm/drm_crtc_helper.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index 3252e70..000cda4 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -244,16 +244,16 @@ void drm_helper_disable_unused_functions(struct drm_device *dev) > struct drm_crtc *crtc; > > list_for_each_entry(connector, &dev->mode_config.connector_list, head) { > - if (!connector->encoder) > - continue; > if (connector->status == connector_status_disconnected) > connector->encoder = NULL; > + if (!connector->encoder) > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF); > } > > list_for_each_entry(encoder, &dev->mode_config.encoder_list, head) { > if (!drm_helper_encoder_in_use(encoder)) { > drm_encoder_disable(encoder); > - /* disconnector encoder from any connector */ > + /* disconnect encoder from any connector */ > encoder->crtc = NULL; > } > } > -- > 1.7.9.5 > > _______________________________________________ > dri-devel mailing list > > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch