Re: [PATCH 1/1] drm/i915: make backlight hooks connector specific

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: Nikula, Jani
>Sent: Tuesday, September 15, 2015 12:31 PM
>To: Daniel Vetter
>Cc: Deepak, M; Adebisi, YetundeX; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>Subject: Re:  [PATCH 1/1] drm/i915: make backlight hooks
>connector specific
>
>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>
>>> ---
Reviewed-by: Deepak M <m.deepak@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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux