Date:   Sun, 5 Apr 2020 15:57:26 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Joonas Kylmälä <joonas.kylmala@....fi>
Cc:     Andrzej Hajda <a.hajda@...sung.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Sam Ravnborg <sam@...nborg.org>,
        David Airlie <airlied@...ux.ie>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        GNUtoo@...erdimension.org
Subject: Re: [PATCH] drm/panel: samsung: s6e8aa0: Add backlight control support

On Sat, Apr 4, 2020 at 3:27 PM Joonas Kylmälä <joonas.kylmala@....fi> 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.
>
> But now to the part that affects many of the panel drivers using MIPI
> DSI, like panel-samsung-s6e63j0x03.c, panel-simple.c, etc.:
> mipi_dsi_attach(dsi) seems to be always called only after the DRM panel
> helper drm_panel_add(). Now I think this is problematic since
> drm_panel_add() makes the panel available for use in userspace but if
> the user tries to actually do something with the panel before
> mipi_dsi_attach(dsi) is called it would not work.
>
> So for some reason the mipi_dsi_attach() is called in all those drivers
> after drm_panel_add() and at least to the problem I pointed out above
> moving the call there before drm_panel_add() would fix the issue but
> then I don't know if it causes some other issue.

Nope, drm_panel_add only makes the panel visible to other drm drivers,
not yet to userspace. That only happens once the overall drm driver
calls drm_dev_register. At that point the mipi_dsi_attach should have
happened I think.

That in turn calls a drm_connector_funcs->late_register hook, which is
meant to be use to register stuff like backlight interfaces. If you
set up your backlight before that, yes I expect some good fireworks.
Now as you noticed, we're not wiring that through to panels, so maybe
the best solution would be if drm_panel gets a backlight pointer with
an initialized, but not yet registered backlight. And then we can
drive this in the bridge or connector wrapper for panels.

Or I'm totally not understanding the issue even, which is also possible :-)

Cheers, Daniel

> Also I don't know if Andrzej had some other issues in mind that could be
> caused by this race condition, so if there are multiple instead of just
> that one issue with DSI not being attached then we might want to have
> all these issues fixed by for example the solution Andrzej proposed
> where we have attach/detach callbacks in drm_bridge_funcs.
>
> Please let me know if anything I write above doesn't make sense, I'm
> still trying to understand the DRM subsystem better.
>
> Joonas



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Powered by blists - more mailing lists