On Mon, 14 Sep 2015, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Mon, Sep 14, 2015 at 02:03:48PM +0300, Jani Nikula wrote:
>> Previously we've relied on having basically one backlight and one
>> backlight type per platform. This is already a bit quirky with PMIC PWM
>> support on VLV/CHV platforms with MIPI DSI. In the foreseeable future
>> we'll have at least DPCD based backlight control on eDP and DCS command
>> based backlight control on MIPI DSI. Backlight is becoming more and more
>> connector specific, so reflect this fact by making the backlight control
>> hooks connector specific.
>>
>> This enables further work to reuse generic backlight code in
>> intel_panel.c while adding more specific backlight code accessed via the
>> hooks.
>>
>> Cc: Deepak M <m.deepak@xxxxxxxxx>
>> Cc: Yetunde Adebisi <yetundex.adebisi@xxxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/i915/i915_drv.h | 9 ---
>> drivers/gpu/drm/i915/intel_display.c | 2 -
>> drivers/gpu/drm/i915/intel_dp.c | 2 +-
>> drivers/gpu/drm/i915/intel_drv.h | 13 +++-
>> drivers/gpu/drm/i915/intel_panel.c | 116 +++++++++++++++++++----------------
>> 5 files changed, 74 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 174f39d0bf46..813023b438b2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -664,15 +664,6 @@ struct drm_i915_display_funcs {
>> /* render clock increase/decrease */
>> /* display clock increase/decrease */
>> /* pll clock increase/decrease */
>> -
>> - int (*setup_backlight)(struct intel_connector *connector, enum pipe pipe);
>> - uint32_t (*get_backlight)(struct intel_connector *connector);
>> - void (*set_backlight)(struct intel_connector *connector,
>> - uint32_t level);
>> - void (*disable_backlight)(struct intel_connector *connector);
>> - void (*enable_backlight)(struct intel_connector *connector);
>> - uint32_t (*backlight_hz_to_pwm)(struct intel_connector *connector,
>> - uint32_t hz);
>> };
>>
>> enum forcewake_domain_id {
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index fc0086748b71..15de4ccc3903 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14520,8 +14520,6 @@ static void intel_init_display(struct drm_device *dev)
>> dev_priv->display.queue_flip = intel_default_queue_flip;
>> }
>>
>> - intel_panel_init_backlight_funcs(dev);
>> -
>> mutex_init(&dev_priv->pps_mutex);
>> }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index a6872508adec..fa1a524844d5 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -5990,7 +5990,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>> }
>>
>> intel_panel_init(&intel_connector->panel, fixed_mode, downclock_mode);
>> - intel_connector->panel.backlight_power = intel_edp_backlight_power;
>> + intel_connector->panel.backlight.power = intel_edp_backlight_power;
>> intel_panel_setup_backlight(connector, pipe);
>>
>> return true;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 02a755a50a96..35a65ca105b3 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -182,9 +182,17 @@ struct intel_panel {
>> struct pwm_device *pwm;
>>
>> struct backlight_device *device;
>> - } backlight;
>>
>> - void (*backlight_power)(struct intel_connector *, bool enable);
>> + /* Connector and platform specific backlight functions */
>> + int (*setup)(struct intel_connector *connector, enum pipe pipe);
>> + uint32_t (*get)(struct intel_connector *connector);
>> + void (*set)(struct intel_connector *connector, uint32_t level);
>> + void (*disable)(struct intel_connector *connector);
>> + void (*enable)(struct intel_connector *connector);
>> + uint32_t (*hz_to_pwm)(struct intel_connector *connector,
>> + uint32_t hz);
>> + void (*power)(struct intel_connector *, bool enable);
>> + } backlight;
>
> For the interface, could we just add a struct backlight_device instead?
> Yes there's no useful kernel-internal interface for that, but we probably
> need to fix this sooner or later anyway. And it would future-proof things
> considerable I think.
No. This is our internal interface that's been proven to abstract seven
slightly different ways of adjusting backlight, with common boilerplate
to deal with the problems of struct backlight_device and scaling and so
on.
BR,
Jani.
> -Daniel
>
>> };
>>
>> struct intel_connector {
>> @@ -1322,7 +1330,6 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>> void intel_panel_enable_backlight(struct intel_connector *connector);
>> void intel_panel_disable_backlight(struct intel_connector *connector);
>> void intel_panel_destroy_backlight(struct drm_connector *connector);
>> -void intel_panel_init_backlight_funcs(struct drm_device *dev);
>> enum drm_connector_status intel_panel_detect(struct drm_device *dev);
>> extern struct drm_display_mode *intel_find_panel_downclock(
>> struct drm_device *dev,
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> index 2034438a0664..9adb62b1b99f 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -566,7 +566,7 @@ static u32 intel_panel_get_backlight(struct intel_connector *connector)
>> mutex_lock(&dev_priv->backlight_lock);
>>
>> if (panel->backlight.enabled) {
>> - val = dev_priv->display.get_backlight(connector);
>> + val = panel->backlight.get(connector);
>> val = intel_panel_compute_brightness(connector, val);
>> }
>>
>> @@ -655,13 +655,12 @@ static void pwm_set_backlight(struct intel_connector *connector, u32 level)
>> static void
>> intel_panel_actually_set_backlight(struct intel_connector *connector, u32 level)
>> {
>> - struct drm_device *dev = connector->base.dev;
>> - struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct intel_panel *panel = &connector->panel;
>>
>> DRM_DEBUG_DRIVER("set backlight PWM = %d\n", level);
>>
>> level = intel_panel_compute_brightness(connector, level);
>> - dev_priv->display.set_backlight(connector, level);
>> + panel->backlight.set(connector, level);
>> }
>>
>> /* set backlight brightness to level in range [0..max], scaling wrt hw min */
>> @@ -836,7 +835,7 @@ void intel_panel_disable_backlight(struct intel_connector *connector)
>> if (panel->backlight.device)
>> panel->backlight.device->props.power = FB_BLANK_POWERDOWN;
>> panel->backlight.enabled = false;
>> - dev_priv->display.disable_backlight(connector);
>> + panel->backlight.disable(connector);
>>
>> mutex_unlock(&dev_priv->backlight_lock);
>> }
>> @@ -1085,7 +1084,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>> panel->backlight.device->props.max_brightness);
>> }
>>
>> - dev_priv->display.enable_backlight(connector);
>> + panel->backlight.enable(connector);
>> panel->backlight.enabled = true;
>> if (panel->backlight.device)
>> panel->backlight.device->props.power = FB_BLANK_UNBLANK;
>> @@ -1113,10 +1112,10 @@ static int intel_backlight_device_update_status(struct backlight_device *bd)
>> * callback needs to take this into account.
>> */
>> if (panel->backlight.enabled) {
>> - if (panel->backlight_power) {
>> + if (panel->backlight.power) {
>> bool enable = bd->props.power == FB_BLANK_UNBLANK &&
>> bd->props.brightness != 0;
>> - panel->backlight_power(connector, enable);
>> + panel->backlight.power(connector, enable);
>> }
>> } else {
>> bd->props.power = FB_BLANK_POWERDOWN;
>> @@ -1341,6 +1340,7 @@ static u32 get_backlight_max_vbt(struct intel_connector *connector)
>> {
>> struct drm_device *dev = connector->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> + struct intel_panel *panel = &connector->panel;
>> u16 pwm_freq_hz = dev_priv->vbt.backlight.pwm_freq_hz;
>> u32 pwm;
>>
>> @@ -1349,12 +1349,12 @@ static u32 get_backlight_max_vbt(struct intel_connector *connector)
>> return 0;
>> }
>>
>> - if (!dev_priv->display.backlight_hz_to_pwm) {
>> + if (!panel->backlight.hz_to_pwm) {
>> DRM_DEBUG_KMS("backlight frequency setting from VBT currently not supported on this platform\n");
>> return 0;
>> }
>>
>> - pwm = dev_priv->display.backlight_hz_to_pwm(connector, pwm_freq_hz);
>> + pwm = panel->backlight.hz_to_pwm(connector, pwm_freq_hz);
>> if (!pwm) {
>> DRM_DEBUG_KMS("backlight frequency conversion failed\n");
>> return 0;
>> @@ -1639,9 +1639,13 @@ int intel_panel_setup_backlight(struct drm_connector *connector, enum pipe pipe)
>> }
>> }
>>
>> + /* ensure intel_panel has been initialized first */
>> + if (WARN_ON(!panel->backlight.setup))
>> + return -ENODEV;
>> +
>> /* set level and max in panel struct */
>> mutex_lock(&dev_priv->backlight_lock);
>> - ret = dev_priv->display.setup_backlight(intel_connector, pipe);
>> + ret = panel->backlight.setup(intel_connector, pipe);
>> mutex_unlock(&dev_priv->backlight_lock);
>>
>> if (ret) {
>> @@ -1673,62 +1677,66 @@ void intel_panel_destroy_backlight(struct drm_connector *connector)
>> }
>>
>> /* Set up chip specific backlight functions */
>> -void intel_panel_init_backlight_funcs(struct drm_device *dev)
>> +static void
>> +intel_panel_init_backlight_funcs(struct intel_panel *panel)
>> {
>> + struct intel_connector *intel_connector =
>> + container_of(panel, struct intel_connector, panel);
>> + struct drm_device *dev = intel_connector->base.dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>>
>> if (IS_BROXTON(dev)) {
>> - dev_priv->display.setup_backlight = bxt_setup_backlight;
>> - dev_priv->display.enable_backlight = bxt_enable_backlight;
>> - dev_priv->display.disable_backlight = bxt_disable_backlight;
>> - dev_priv->display.set_backlight = bxt_set_backlight;
>> - dev_priv->display.get_backlight = bxt_get_backlight;
>> + panel->backlight.setup = bxt_setup_backlight;
>> + panel->backlight.enable = bxt_enable_backlight;
>> + panel->backlight.disable = bxt_disable_backlight;
>> + panel->backlight.set = bxt_set_backlight;
>> + panel->backlight.get = bxt_get_backlight;
>> } else if (HAS_PCH_LPT(dev) || HAS_PCH_SPT(dev)) {
>> - dev_priv->display.setup_backlight = lpt_setup_backlight;
>> - dev_priv->display.enable_backlight = lpt_enable_backlight;
>> - dev_priv->display.disable_backlight = lpt_disable_backlight;
>> - dev_priv->display.set_backlight = lpt_set_backlight;
>> - dev_priv->display.get_backlight = lpt_get_backlight;
>> + panel->backlight.setup = lpt_setup_backlight;
>> + panel->backlight.enable = lpt_enable_backlight;
>> + panel->backlight.disable = lpt_disable_backlight;
>> + panel->backlight.set = lpt_set_backlight;
>> + panel->backlight.get = lpt_get_backlight;
>> if (HAS_PCH_LPT(dev))
>> - dev_priv->display.backlight_hz_to_pwm = lpt_hz_to_pwm;
>> + panel->backlight.hz_to_pwm = lpt_hz_to_pwm;
>> else
>> - dev_priv->display.backlight_hz_to_pwm = spt_hz_to_pwm;
>> + panel->backlight.hz_to_pwm = spt_hz_to_pwm;
>> } else if (HAS_PCH_SPLIT(dev)) {
>> - dev_priv->display.setup_backlight = pch_setup_backlight;
>> - dev_priv->display.enable_backlight = pch_enable_backlight;
>> - dev_priv->display.disable_backlight = pch_disable_backlight;
>> - dev_priv->display.set_backlight = pch_set_backlight;
>> - dev_priv->display.get_backlight = pch_get_backlight;
>> - dev_priv->display.backlight_hz_to_pwm = pch_hz_to_pwm;
>> + panel->backlight.setup = pch_setup_backlight;
>> + panel->backlight.enable = pch_enable_backlight;
>> + panel->backlight.disable = pch_disable_backlight;
>> + panel->backlight.set = pch_set_backlight;
>> + panel->backlight.get = pch_get_backlight;
>> + panel->backlight.hz_to_pwm = pch_hz_to_pwm;
>> } else if (IS_VALLEYVIEW(dev)) {
>> if (dev_priv->vbt.has_mipi) {
>> - dev_priv->display.setup_backlight = pwm_setup_backlight;
>> - dev_priv->display.enable_backlight = pwm_enable_backlight;
>> - dev_priv->display.disable_backlight = pwm_disable_backlight;
>> - dev_priv->display.set_backlight = pwm_set_backlight;
>> - dev_priv->display.get_backlight = pwm_get_backlight;
>> + panel->backlight.setup = pwm_setup_backlight;
>> + panel->backlight.enable = pwm_enable_backlight;
>> + panel->backlight.disable = pwm_disable_backlight;
>> + panel->backlight.set = pwm_set_backlight;
>> + panel->backlight.get = pwm_get_backlight;
>> } else {
>> - dev_priv->display.setup_backlight = vlv_setup_backlight;
>> - dev_priv->display.enable_backlight = vlv_enable_backlight;
>> - dev_priv->display.disable_backlight = vlv_disable_backlight;
>> - dev_priv->display.set_backlight = vlv_set_backlight;
>> - dev_priv->display.get_backlight = vlv_get_backlight;
>> - dev_priv->display.backlight_hz_to_pwm = vlv_hz_to_pwm;
>> + panel->backlight.setup = vlv_setup_backlight;
>> + panel->backlight.enable = vlv_enable_backlight;
>> + panel->backlight.disable = vlv_disable_backlight;
>> + panel->backlight.set = vlv_set_backlight;
>> + panel->backlight.get = vlv_get_backlight;
>> + panel->backlight.hz_to_pwm = vlv_hz_to_pwm;
>> }
>> } else if (IS_GEN4(dev)) {
>> - dev_priv->display.setup_backlight = i965_setup_backlight;
>> - dev_priv->display.enable_backlight = i965_enable_backlight;
>> - dev_priv->display.disable_backlight = i965_disable_backlight;
>> - dev_priv->display.set_backlight = i9xx_set_backlight;
>> - dev_priv->display.get_backlight = i9xx_get_backlight;
>> - dev_priv->display.backlight_hz_to_pwm = i965_hz_to_pwm;
>> + panel->backlight.setup = i965_setup_backlight;
>> + panel->backlight.enable = i965_enable_backlight;
>> + panel->backlight.disable = i965_disable_backlight;
>> + panel->backlight.set = i9xx_set_backlight;
>> + panel->backlight.get = i9xx_get_backlight;
>> + panel->backlight.hz_to_pwm = i965_hz_to_pwm;
>> } else {
>> - dev_priv->display.setup_backlight = i9xx_setup_backlight;
>> - dev_priv->display.enable_backlight = i9xx_enable_backlight;
>> - dev_priv->display.disable_backlight = i9xx_disable_backlight;
>> - dev_priv->display.set_backlight = i9xx_set_backlight;
>> - dev_priv->display.get_backlight = i9xx_get_backlight;
>> - dev_priv->display.backlight_hz_to_pwm = i9xx_hz_to_pwm;
>> + panel->backlight.setup = i9xx_setup_backlight;
>> + panel->backlight.enable = i9xx_enable_backlight;
>> + panel->backlight.disable = i9xx_disable_backlight;
>> + panel->backlight.set = i9xx_set_backlight;
>> + panel->backlight.get = i9xx_get_backlight;
>> + panel->backlight.hz_to_pwm = i9xx_hz_to_pwm;
>> }
>> }
>>
>> @@ -1736,6 +1744,8 @@ int intel_panel_init(struct intel_panel *panel,
>> struct drm_display_mode *fixed_mode,
>> struct drm_display_mode *downclock_mode)
>> {
>> + intel_panel_init_backlight_funcs(panel);
>> +
>> panel->fixed_mode = fixed_mode;
>> panel->downclock_mode = downclock_mode;
>>
>> --
>> 2.1.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
