Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control support

[Andrzej Hajda <a.hajda@samsung.com>]

Hi Joonas,


On 21.09.2019 14:48, Joonas Kylmälä wrote:
> This makes the backlight brightness controllable from the
> userspace.
>
> Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
> ---
>  drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 82 ++++++++++++++++++++-------
>  1 file changed, 60 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> index dbced6501204..aa75934f5bed 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c
> @@ -10,8 +10,12 @@
>   * Eunchul Kim <chulspro.kim@samsung.com>
>   * Tomasz Figa <t.figa@samsung.com>
>   * Andrzej Hajda <a.hajda@samsung.com>
> + *
> + * Derived from panel-samsung-s6e63m0.c:
> + *  Copyright (C) 2019 Paweł Chmiel <pawel.mikolaj.chmiel@gmail.com>
>  */
>  
> +#include <linux/backlight.h>
>  #include <linux/delay.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
> @@ -85,6 +89,8 @@
>  #define AID_2				(0x6)
>  #define AID_3				(0x7)
>  
> +#define MAX_BRIGHTNESS (GAMMA_LEVEL_NUM - 1)
> +
>  typedef u8 s6e8aa0_gamma_table[GAMMA_TABLE_LEN];
>  
>  struct s6e8aa0_variant {
> @@ -95,6 +101,7 @@ struct s6e8aa0_variant {
>  struct s6e8aa0 {
>  	struct device *dev;
>  	struct drm_panel panel;
> +	struct backlight_device *bl_dev;
>  
>  	struct regulator_bulk_data supplies[2];
>  	struct gpio_desc *reset_gpio;
> @@ -110,7 +117,6 @@ struct s6e8aa0 {
>  	u8 version;
>  	u8 id;
>  	const struct s6e8aa0_variant *variant;
> -	int brightness;
>  
>  	/* This field is tested by functions directly accessing DSI bus before
>  	 * transfer, transfer is skipped if it is set. In case of transfer
> @@ -321,9 +327,10 @@ static void s6e8aa0_etc_elvss_control(struct s6e8aa0 *ctx)
>  
>  static void s6e8aa0_elvss_nvm_set_v142(struct s6e8aa0 *ctx)
>  {
> +	struct backlight_device *bd = ctx->bl_dev;
>  	u8 br;
>  
> -	switch (ctx->brightness) {
> +	switch (bd->props.brightness) {
>  	case 0 ... 6: /* 30cd ~ 100cd */
>  		br = 0xdf;
>  		break;
> @@ -762,24 +769,6 @@ static const struct s6e8aa0_variant s6e8aa0_variants[] = {
>  	}
>  };
>  
> -static void s6e8aa0_brightness_set(struct s6e8aa0 *ctx)
> -{
> -	const u8 *gamma;
> -
> -	if (ctx->error)
> -		return;
> -
> -	gamma = ctx->variant->gamma_tables[ctx->brightness];
> -
> -	if (ctx->version >= 142)
> -		s6e8aa0_elvss_nvm_set(ctx);
> -
> -	s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
> -
> -	/* update gamma table. */
> -	s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
> -}
> -
>  static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)
>  {
>  	s6e8aa0_apply_level_1_key(ctx);
> @@ -791,7 +780,7 @@ static void s6e8aa0_panel_init(struct s6e8aa0 *ctx)
>  
>  	s6e8aa0_panel_cond_set(ctx);
>  	s6e8aa0_display_condition_set(ctx);
> -	s6e8aa0_brightness_set(ctx);
> +	backlight_enable(ctx->bl_dev);
>  	s6e8aa0_etc_source_control(ctx);
>  	s6e8aa0_etc_pentile_control(ctx);
>  	s6e8aa0_elvss_nvm_set(ctx);
> @@ -974,6 +963,53 @@ static int s6e8aa0_parse_dt(struct s6e8aa0 *ctx)
>  	return 0;
>  }
>  
> +static int s6e8aa0_set_brightness(struct backlight_device *bd)
> +{
> +	struct s6e8aa0 *ctx = bl_get_data(bd);
> +	const u8 *gamma;
> +
> +	if (ctx->error)
> +		return;
> +
> +	gamma = ctx->variant->gamma_tables[bd->props.brightness];
> +
> +	if (ctx->version >= 142)
> +		s6e8aa0_elvss_nvm_set(ctx);
> +
> +	s6e8aa0_dcs_write(ctx, gamma, GAMMA_TABLE_LEN);
> +
> +	/* update gamma table. */
> +	s6e8aa0_dcs_write_seq_static(ctx, 0xf7, 0x03);
> +
> +	return s6e8aa0_clear_error(ctx);
> +}
> +
> +static const struct backlight_ops s6e8aa0_backlight_ops = {
> +	.update_status	= s6e8aa0_set_brightness,


This is racy, update_status can be called in any time between probe and
remove, particularly:

a) before panel enable,

b) during panel enable,

c) when panel is enabled,

d) during panel disable,

e) after panel disable,


b and d are racy for sure - backlight and drm callbacks are async.

IMO the best solution would be to register backlight after attaching
panel to drm, but for this drm_panel_funcs should have attach/detach
callbacks (like drm_bridge_funcs),

then update_status callback should take some drm_connector lock to
synchronize with drm, and write to hw only when pipe is enabled.


Regards

Andrzej



> +};
> +
> +static int s6e8aa0_backlight_register(struct s6e8aa0 *ctx)
> +{
> +	struct backlight_properties props = {
> +		.type		= BACKLIGHT_RAW,
> +		.brightness	= MAX_BRIGHTNESS,
> +		.max_brightness = MAX_BRIGHTNESS
> +	};
> +	struct device *dev = ctx->dev;
> +	int ret = 0;
> +
> +	ctx->bl_dev = devm_backlight_device_register(dev, "panel", dev, ctx,
> +						     &s6e8aa0_backlight_ops,
> +						     &props);
> +	if (IS_ERR(ctx->bl_dev)) {
> +		ret = PTR_ERR(ctx->bl_dev);
> +		DRM_DEV_ERROR(dev, "error registering backlight device (%d)\n",
> +			      ret);
> +	}
> +
> +	return ret;
> +}
> +
>  static int s6e8aa0_probe(struct mipi_dsi_device *dsi)
>  {
>  	struct device *dev = &dsi->dev;
> @@ -1015,7 +1051,9 @@ static int s6e8aa0_probe(struct mipi_dsi_device *dsi)
>  		return PTR_ERR(ctx->reset_gpio);
>  	}
>  
> -	ctx->brightness = GAMMA_LEVEL_NUM - 1;
> +	ret = s6e8aa0_backlight_register(ctx);
> +	if (ret < 0)
> +		return ret;
>  
>  	drm_panel_init(&ctx->panel, dev, &s6e8aa0_drm_funcs,
>  		       DRM_MODE_CONNECTOR_DSI);