Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 5 Apr 2020 16:27:15 +0200
From:   Sam Ravnborg <sam@ravnborg.org>
To:     Joonas =?iso-8859-1?Q?Kylm=E4l=E4?= <joonas.kylmala@iki.fi>
Cc:     Andrzej Hajda <a.hajda@samsung.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org,
        paul.kocialkowski@bootlin.com, GNUtoo@cyberdimension.org
Subject: Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control
 support
Message-ID: <20200405142715.GA28291@ravnborg.org>
References: <CGME20190921125017epcas3p2f5661cca04f0959f9707f6111102435d@epcas3p2.samsung.com>
 <20190921124843.6967-1-joonas.kylmala@iki.fi>
 <d8a8bf25-0c5e-8d94-9406-b1f74e3edfac@samsung.com>
 <53385e44-1847-ace0-cd87-5571f6acd3f2@iki.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <53385e44-1847-ace0-cd87-5571f6acd3f2@iki.fi>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Joonas.

On Sat, Apr 04, 2020 at 04:27:02PM +0300, Joonas Kylm?l? wrote:
> Hi,
> 
> addressing this email to you all since there might be widespread race
> condition issue in the DRM panel drivers that are using MIPI DSI. See
> below for my message.
> 
> Andrzej Hajda:
> >> +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.
> 
> I have done now research and if I understand right one issue here might
> be with setting the backlight brightness if the DSI device is not
> attached before calling update_status() since calling it would call
> subsequently s6e8aa0_set_brightness() -> s6e8aa0_dcs_write() ->
> mipi_dsi_dcs_write_buffer(), which then requires DSI to be attached.

Not directly related to your comments above.
But I have looked at the backlight support for the various
samsung panels.

None of them are good examples to follow.
Please have a look at for example panel-novatek-nt35510.c
which is a good example how to have a local backligth
and tie it into the general way it is used by drm_panel.

I have typed patches to fix all three samsung panels, will
post patches later when I get more time.

If we are concerned with set_brightness() being called
while not ready, this can be checked in the
set_brightness() function and return error if not OK.

As the the race concerns see Daniel's reply.

	Sam