[PATCH] drm: call connector->dpms(OFF) when disabling

[Rob Clark]

On Fri, Sep 28, 2012 at 11:56 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Fri, Sep 28, 2012 at 09:27:18AM +0200, Rob Clark wrote:
>> From: Rob Clark <rob at ti.com>
>>
>> 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 <rob at ti.com>
>> Tested-by: Roger Quadros <rogerq at ti.com>
>
> 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.

yeah, I do this.. I have my own fxn in the connector which does some
stuff, and then calls drm_helper_connector_dpms().

Maybe it just makes sense to always do connector->dpms(OFF) before
unhooking the chain, rather than directly calling dpms on the
encoder/crtc?

> 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

well, I think by the time we start adding atomic-modeset and
common/dsi panel framework, there might be need for a new set of
helpers.. but I'm not sure that the hw is quite strange enough to need
an omap specific set of helpers.  Maybe I start w/ something in
omapdrm but then refactor into common functions once a few drivers are
converted to atomic modeset and panel framework.

BR,
-R

>
> NACKed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>
> 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
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel