On Fri, Sep 28, 2012 at 11:56 AM, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Fri, Sep 28, 2012 at 09:27:18AM +0200, Rob Clark wrote:
>> From: Rob Clark <rob@xxxxxx>
>>
>> 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@xxxxxx>
>> Tested-by: Roger Quadros <rogerq@xxxxxx>
>
> 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@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
