Re: 3.13 i915 brightness settings broken when going from docked -> undocked

[Josh Boyer]

On Tue, Feb 25, 2014 at 3:36 AM, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Sun, 23 Feb 2014, Josh Boyer <jwboyer@xxxxxxxxxxxxxxxxx> wrote:
>> Backport of upstream commit c91c9f328
>>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       |  1 +
>>  drivers/gpu/drm/i915/intel_opregion.c | 31 ++++++-------------------------
>>  drivers/gpu/drm/i915/intel_panel.c    |  4 ++++
>>  3 files changed, 11 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 221ac62..d6d4349 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1371,6 +1371,7 @@ typedef struct drm_i915_private {
>>
>>       /* backlight */
>>       struct {
>> +             bool present;
>>               int level;
>>               bool enabled;
>>               spinlock_t lock; /* bl registers and the above bl fields */
>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
>> index 6d69a9b..b2a51ae 100644
>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>> @@ -414,38 +414,19 @@ static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>>               return ASLC_BACKLIGHT_FAILED;
>>
>>       mutex_lock(&dev->mode_config.mutex);
>> -     /*
>> -      * Could match the OpRegion connector here instead, but we'd also need
>> -      * to verify the connector could handle a backlight call.
>> -      */
>> -     list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
>> -             if (encoder->crtc == crtc) {
>> -                     found = true;
>> -                     break;
>> -             }
>> -
>> -     if (!found) {
>> -             ret = ASLC_BACKLIGHT_FAILED;
>> -             goto out;
>> -     }
>>
>> -     list_for_each_entry(connector, &dev->mode_config.connector_list, head)
>> -             if (connector->encoder == encoder)
>> -                     intel_connector = to_intel_connector(connector);
>> -
>> -     if (!intel_connector) {
>> -             ret = ASLC_BACKLIGHT_FAILED;
>> -             goto out;
>> +     DRM_DEBUG_KMS("updating opregion backlight %d/255\n", bclp);
>> +     list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
>> +             intel_connector = to_intel_connector(connector);
>> +             if (dev_priv->backlight.present)
>> +                     intel_panel_set_backlight(intel_connector, bclp, 255);
>>       }
>
> This is not correct because in v3.13 dev_priv->backlight is still driver
> global, not per connector. This means that if you have at least one
> connector with backlight control, the backlight is attempted to change
> on all connectors. In your case, I'm not sure if it leads to anything
> more than extra adjusting of the same backlight.

Well, empirically it leads to the backlight actually changing after
undocking my machine whereas without it, it doesn't.  So maybe by
changing it globally, it is hitting the connector that does have
backlight control.

Anyway, I'm not arguing my patch is correct.  Just that it does do
_something_ to make the backlight work :).

> If you move the 'bool present' field to intel_connector or intel_panel,
> I think this is all right.

That sounds like more of an invasive change.  I could poke at it, but
I'm not sure it would be suitable for e.g. 3.13.y stable.  Thoughts on
that?  Admittedly it is a somewhat minor problem, so if it's not
easily stable material, I don't think anyone is going to lose sleep
over it.

josh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/