Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Thu, Dec 10, 2020 at 05:50:00PM +0100, Daniel Vetter wrote: > On Thu, Dec 10, 2020 at 5:44 PM Thierry Reding > wrote: > > > > On Thu, Dec 10, 2020 at 11:15:38AM +0100, Daniel Vetter wrote: > > > On Wed, Dec 09, 2020 at 02:28:18PM -0600, Bjorn Andersson wrote: > > > > On Tue 08 Dec 17:52 CST 2020, Daniel Vetter wrote: > > > > > > > > > On Tue, Dec 08, 2020 at 04:02:16PM -0600, Bjorn Andersson wrote: > > > > > > On Tue 08 Dec 06:47 CST 2020, Thierry Reding wrote: > > > > > > > > > > > > > On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote: > > > > > > > > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, > > > > > > > > provides > > > > > > > > means of generating a PWM signal for backlight control of the > > > > > > > > attached > > > > > > > > panel. The provided PWM chip is typically controlled by the > > > > > > > > pwm-backlight driver, which if tied to the panel will provide > > > > > > > > DPMS. > > > > > > > > > > > > > > > > But with the current implementation the panel will refuse to > > > > > > > > probe > > > > > > > > because the bridge driver has yet to probe and register the PWM > > > > > > > > chip, > > > > > > > > and the bridge driver will refuse to probe because it's unable > > > > > > > > to find > > > > > > > > the panel. > > > > > > > > > > > > > > What you're describing is basically a circular dependency. Can't > > > > > > > we get > > > > > > > rid of that in some other way? Why exactly does the bridge driver > > > > > > > refuse > > > > > > > to probe if the panel can't be found? > > > > > > > > > > > > > > In other words, I see how the bridge would /use/ the panel in > > > > > > > that it > > > > > > > forward a video stream to it. But how does the panel /use/ the > > > > > > > bridge? > > > > > > > > > > > > > > > > > > > Yes, this is indeed a circular dependency between the components. > > > > > > > > > > > > The involved parts are: > > > > > > * the bridge driver that implements the PWM chip probe defers on > > > > > > drm_of_find_panel_or_bridge() failing to find the panel. > > > > > > * the pwm-backlight driver that consumes the PWM channel probe defer > > > > > > because the pwm_chip was not registered by the bridge. > > > > > > * the panel that uses the backlight for DPMS purposes probe defer > > > > > > because drm_panel_of_backlight() fails to find the pwm-backlight. > > > > > > > > > > > > I looked at means of postponing drm_of_find_panel_or_bridge() to > > > > > > drm_bridge_funcs->attach(), but at that time "deferral" would be > > > > > > fatal. > > > > > > I looked at registering the pwm_chip earlier, but that would depend > > > > > > on a > > > > > > guarantee of the pwm-backlight and panel driver to probe > > > > > > concurrently. > > > > > > And the current solution of not tying the backlight to the panel > > > > > > means > > > > > > that when userspace decides to DPMS the display the backlight stays > > > > > > on. > > > > > > > > > > > > > > > > > > The proposed solution (hack?) means that DPMS operations happening > > > > > > before the pwm-backlight has probed will be missed, so it's not > > > > > > perfect. > > > > > > It does however allow the backlight on my laptop to turn off, which > > > > > > is a > > > > > > big improvement. > > > > > > > > > > > > But I'm certainly welcome to suggestions. > > > > > > > > > > Entirely hand-waving, why doesn't the following work: > > > > > > > > > > 1. driver for the platform device which is the bridge loads > > > > > 2. that platform driver registers the pwm > > > > > 3. it registers some magic for later on (more below) > > > > > 4. panel driver has deferred loading until step 2 happened > > > > > 5. panel driver registers drm_panel > > > > > 6. the magic from step 3 picks up (after having been deferred for a > > > > > few > > > > > times probably) grabs the panel, and sets up the actual drm_bridge > > > > > driver > > > > > > > > > > Everyone happy, or not? From the description it looks like the problem > > > > > that the pwm that we need for the backlight is tied to the same > > > > > driver as > > > > > the drm_bridge, and always torn down too if the drm_bridge setup fails > > > > > somehow for a reason. And that reason is the circular dependency this > > > > > creates. > > > > > > > > > > Now for the magic in step 3, there's options: > > > > > - change DT to split out that pwm as a separate platform_device, that > > > > > way > > > > > bridge and panel can load indepedently (hopefully) > > > > > > > > > > > > > This is an i2c device, so describing it multiple times would mean we > > > > have multiple devices with the same address... > > > > > > > > > - convert bridge to a multi-function device (mfd), essentially a way > > > > > to > > > > > instantiate more devices with their drivers at runtime. Then the > > > > > actual > > > > > pwm and drm_bridge parts of your bridge driver bind against those > > > > > sub-functions, and can defer indepedently > > >
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Thu, Dec 10, 2020 at 5:44 PM Thierry Reding wrote: > > On Thu, Dec 10, 2020 at 11:15:38AM +0100, Daniel Vetter wrote: > > On Wed, Dec 09, 2020 at 02:28:18PM -0600, Bjorn Andersson wrote: > > > On Tue 08 Dec 17:52 CST 2020, Daniel Vetter wrote: > > > > > > > On Tue, Dec 08, 2020 at 04:02:16PM -0600, Bjorn Andersson wrote: > > > > > On Tue 08 Dec 06:47 CST 2020, Thierry Reding wrote: > > > > > > > > > > > On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote: > > > > > > > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, > > > > > > > provides > > > > > > > means of generating a PWM signal for backlight control of the > > > > > > > attached > > > > > > > panel. The provided PWM chip is typically controlled by the > > > > > > > pwm-backlight driver, which if tied to the panel will provide > > > > > > > DPMS. > > > > > > > > > > > > > > But with the current implementation the panel will refuse to probe > > > > > > > because the bridge driver has yet to probe and register the PWM > > > > > > > chip, > > > > > > > and the bridge driver will refuse to probe because it's unable to > > > > > > > find > > > > > > > the panel. > > > > > > > > > > > > What you're describing is basically a circular dependency. Can't we > > > > > > get > > > > > > rid of that in some other way? Why exactly does the bridge driver > > > > > > refuse > > > > > > to probe if the panel can't be found? > > > > > > > > > > > > In other words, I see how the bridge would /use/ the panel in that > > > > > > it > > > > > > forward a video stream to it. But how does the panel /use/ the > > > > > > bridge? > > > > > > > > > > > > > > > > Yes, this is indeed a circular dependency between the components. > > > > > > > > > > The involved parts are: > > > > > * the bridge driver that implements the PWM chip probe defers on > > > > > drm_of_find_panel_or_bridge() failing to find the panel. > > > > > * the pwm-backlight driver that consumes the PWM channel probe defer > > > > > because the pwm_chip was not registered by the bridge. > > > > > * the panel that uses the backlight for DPMS purposes probe defer > > > > > because drm_panel_of_backlight() fails to find the pwm-backlight. > > > > > > > > > > I looked at means of postponing drm_of_find_panel_or_bridge() to > > > > > drm_bridge_funcs->attach(), but at that time "deferral" would be > > > > > fatal. > > > > > I looked at registering the pwm_chip earlier, but that would depend > > > > > on a > > > > > guarantee of the pwm-backlight and panel driver to probe concurrently. > > > > > And the current solution of not tying the backlight to the panel means > > > > > that when userspace decides to DPMS the display the backlight stays > > > > > on. > > > > > > > > > > > > > > > The proposed solution (hack?) means that DPMS operations happening > > > > > before the pwm-backlight has probed will be missed, so it's not > > > > > perfect. > > > > > It does however allow the backlight on my laptop to turn off, which > > > > > is a > > > > > big improvement. > > > > > > > > > > But I'm certainly welcome to suggestions. > > > > > > > > Entirely hand-waving, why doesn't the following work: > > > > > > > > 1. driver for the platform device which is the bridge loads > > > > 2. that platform driver registers the pwm > > > > 3. it registers some magic for later on (more below) > > > > 4. panel driver has deferred loading until step 2 happened > > > > 5. panel driver registers drm_panel > > > > 6. the magic from step 3 picks up (after having been deferred for a few > > > > times probably) grabs the panel, and sets up the actual drm_bridge > > > > driver > > > > > > > > Everyone happy, or not? From the description it looks like the problem > > > > that the pwm that we need for the backlight is tied to the same driver > > > > as > > > > the drm_bridge, and always torn down too if the drm_bridge setup fails > > > > somehow for a reason. And that reason is the circular dependency this > > > > creates. > > > > > > > > Now for the magic in step 3, there's options: > > > > - change DT to split out that pwm as a separate platform_device, that > > > > way > > > > bridge and panel can load indepedently (hopefully) > > > > > > > > > > This is an i2c device, so describing it multiple times would mean we > > > have multiple devices with the same address... > > > > > > > - convert bridge to a multi-function device (mfd), essentially a way to > > > > instantiate more devices with their drivers at runtime. Then the > > > > actual > > > > pwm and drm_bridge parts of your bridge driver bind against those > > > > sub-functions, and can defer indepedently > > > > > > > > > > But, this sounds reasonable and would rely on the existing probe > > > deferral logic and if there's ever any improvements in this area we > > > would directly benefit from it. > > > > > > > - we could create a callback/wait function for "pls wait for any panel > > > > to > > > > show up". Then your bridge
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Thu, Dec 10, 2020 at 11:15:38AM +0100, Daniel Vetter wrote: > On Wed, Dec 09, 2020 at 02:28:18PM -0600, Bjorn Andersson wrote: > > On Tue 08 Dec 17:52 CST 2020, Daniel Vetter wrote: > > > > > On Tue, Dec 08, 2020 at 04:02:16PM -0600, Bjorn Andersson wrote: > > > > On Tue 08 Dec 06:47 CST 2020, Thierry Reding wrote: > > > > > > > > > On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote: > > > > > > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides > > > > > > means of generating a PWM signal for backlight control of the > > > > > > attached > > > > > > panel. The provided PWM chip is typically controlled by the > > > > > > pwm-backlight driver, which if tied to the panel will provide DPMS. > > > > > > > > > > > > But with the current implementation the panel will refuse to probe > > > > > > because the bridge driver has yet to probe and register the PWM > > > > > > chip, > > > > > > and the bridge driver will refuse to probe because it's unable to > > > > > > find > > > > > > the panel. > > > > > > > > > > What you're describing is basically a circular dependency. Can't we > > > > > get > > > > > rid of that in some other way? Why exactly does the bridge driver > > > > > refuse > > > > > to probe if the panel can't be found? > > > > > > > > > > In other words, I see how the bridge would /use/ the panel in that it > > > > > forward a video stream to it. But how does the panel /use/ the bridge? > > > > > > > > > > > > > Yes, this is indeed a circular dependency between the components. > > > > > > > > The involved parts are: > > > > * the bridge driver that implements the PWM chip probe defers on > > > > drm_of_find_panel_or_bridge() failing to find the panel. > > > > * the pwm-backlight driver that consumes the PWM channel probe defer > > > > because the pwm_chip was not registered by the bridge. > > > > * the panel that uses the backlight for DPMS purposes probe defer > > > > because drm_panel_of_backlight() fails to find the pwm-backlight. > > > > > > > > I looked at means of postponing drm_of_find_panel_or_bridge() to > > > > drm_bridge_funcs->attach(), but at that time "deferral" would be fatal. > > > > I looked at registering the pwm_chip earlier, but that would depend on a > > > > guarantee of the pwm-backlight and panel driver to probe concurrently. > > > > And the current solution of not tying the backlight to the panel means > > > > that when userspace decides to DPMS the display the backlight stays on. > > > > > > > > > > > > The proposed solution (hack?) means that DPMS operations happening > > > > before the pwm-backlight has probed will be missed, so it's not perfect. > > > > It does however allow the backlight on my laptop to turn off, which is a > > > > big improvement. > > > > > > > > But I'm certainly welcome to suggestions. > > > > > > Entirely hand-waving, why doesn't the following work: > > > > > > 1. driver for the platform device which is the bridge loads > > > 2. that platform driver registers the pwm > > > 3. it registers some magic for later on (more below) > > > 4. panel driver has deferred loading until step 2 happened > > > 5. panel driver registers drm_panel > > > 6. the magic from step 3 picks up (after having been deferred for a few > > > times probably) grabs the panel, and sets up the actual drm_bridge driver > > > > > > Everyone happy, or not? From the description it looks like the problem > > > that the pwm that we need for the backlight is tied to the same driver as > > > the drm_bridge, and always torn down too if the drm_bridge setup fails > > > somehow for a reason. And that reason is the circular dependency this > > > creates. > > > > > > Now for the magic in step 3, there's options: > > > - change DT to split out that pwm as a separate platform_device, that way > > > bridge and panel can load indepedently (hopefully) > > > > > > > This is an i2c device, so describing it multiple times would mean we > > have multiple devices with the same address... > > > > > - convert bridge to a multi-function device (mfd), essentially a way to > > > instantiate more devices with their drivers at runtime. Then the actual > > > pwm and drm_bridge parts of your bridge driver bind against those > > > sub-functions, and can defer indepedently > > > > > > > But, this sounds reasonable and would rely on the existing probe > > deferral logic and if there's ever any improvements in this area we > > would directly benefit from it. > > > > > - we could create a callback/wait function for "pls wait for any panel to > > > show up". Then your bridge driver could launch a work_struct with that > > > wait function, which will do the bridge setup once the panel has shown > > > up. The pwm will be registered right away. It's essentially hand-rolling > > > EPROBE_DEFERRED for work_struct in drm/panel. Maybe we might even have > > > that exported from the driver core, e.g. > > > > > > register_bridge_fn(struct work *)
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Wed, Dec 09, 2020 at 02:28:18PM -0600, Bjorn Andersson wrote:
> On Tue 08 Dec 17:52 CST 2020, Daniel Vetter wrote:
>
> > On Tue, Dec 08, 2020 at 04:02:16PM -0600, Bjorn Andersson wrote:
> > > On Tue 08 Dec 06:47 CST 2020, Thierry Reding wrote:
> > >
> > > > On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote:
> > > > > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides
> > > > > means of generating a PWM signal for backlight control of the attached
> > > > > panel. The provided PWM chip is typically controlled by the
> > > > > pwm-backlight driver, which if tied to the panel will provide DPMS.
> > > > >
> > > > > But with the current implementation the panel will refuse to probe
> > > > > because the bridge driver has yet to probe and register the PWM chip,
> > > > > and the bridge driver will refuse to probe because it's unable to find
> > > > > the panel.
> > > >
> > > > What you're describing is basically a circular dependency. Can't we get
> > > > rid of that in some other way? Why exactly does the bridge driver refuse
> > > > to probe if the panel can't be found?
> > > >
> > > > In other words, I see how the bridge would /use/ the panel in that it
> > > > forward a video stream to it. But how does the panel /use/ the bridge?
> > > >
> > >
> > > Yes, this is indeed a circular dependency between the components.
> > >
> > > The involved parts are:
> > > * the bridge driver that implements the PWM chip probe defers on
> > > drm_of_find_panel_or_bridge() failing to find the panel.
> > > * the pwm-backlight driver that consumes the PWM channel probe defer
> > > because the pwm_chip was not registered by the bridge.
> > > * the panel that uses the backlight for DPMS purposes probe defer
> > > because drm_panel_of_backlight() fails to find the pwm-backlight.
> > >
> > > I looked at means of postponing drm_of_find_panel_or_bridge() to
> > > drm_bridge_funcs->attach(), but at that time "deferral" would be fatal.
> > > I looked at registering the pwm_chip earlier, but that would depend on a
> > > guarantee of the pwm-backlight and panel driver to probe concurrently.
> > > And the current solution of not tying the backlight to the panel means
> > > that when userspace decides to DPMS the display the backlight stays on.
> > >
> > >
> > > The proposed solution (hack?) means that DPMS operations happening
> > > before the pwm-backlight has probed will be missed, so it's not perfect.
> > > It does however allow the backlight on my laptop to turn off, which is a
> > > big improvement.
> > >
> > > But I'm certainly welcome to suggestions.
> >
> > Entirely hand-waving, why doesn't the following work:
> >
> > 1. driver for the platform device which is the bridge loads
> > 2. that platform driver registers the pwm
> > 3. it registers some magic for later on (more below)
> > 4. panel driver has deferred loading until step 2 happened
> > 5. panel driver registers drm_panel
> > 6. the magic from step 3 picks up (after having been deferred for a few
> > times probably) grabs the panel, and sets up the actual drm_bridge driver
> >
> > Everyone happy, or not? From the description it looks like the problem
> > that the pwm that we need for the backlight is tied to the same driver as
> > the drm_bridge, and always torn down too if the drm_bridge setup fails
> > somehow for a reason. And that reason is the circular dependency this
> > creates.
> >
> > Now for the magic in step 3, there's options:
> > - change DT to split out that pwm as a separate platform_device, that way
> > bridge and panel can load indepedently (hopefully)
> >
>
> This is an i2c device, so describing it multiple times would mean we
> have multiple devices with the same address...
>
> > - convert bridge to a multi-function device (mfd), essentially a way to
> > instantiate more devices with their drivers at runtime. Then the actual
> > pwm and drm_bridge parts of your bridge driver bind against those
> > sub-functions, and can defer indepedently
> >
>
> But, this sounds reasonable and would rely on the existing probe
> deferral logic and if there's ever any improvements in this area we
> would directly benefit from it.
>
> > - we could create a callback/wait function for "pls wait for any panel to
> > show up". Then your bridge driver could launch a work_struct with that
> > wait function, which will do the bridge setup once the panel has shown
> > up. The pwm will be registered right away. It's essentially hand-rolling
> > EPROBE_DEFERRED for work_struct in drm/panel. Maybe we might even have
> > that exported from the driver core, e.g.
> >
> > register_bridge_fn(struct work *)
> > {
> > do_wait_probe_defer();
> > panel = drm_of_find_panel_or_bridge();
> > if (!panel) {
> > schedule_work(); /* want to restart the work so it can be
> > stopped on driver unload */
> > return;
> > }
> >
> > /* we have the panel now, register drm_bridge */
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Tue 08 Dec 17:52 CST 2020, Daniel Vetter wrote:
> On Tue, Dec 08, 2020 at 04:02:16PM -0600, Bjorn Andersson wrote:
> > On Tue 08 Dec 06:47 CST 2020, Thierry Reding wrote:
> >
> > > On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote:
> > > > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides
> > > > means of generating a PWM signal for backlight control of the attached
> > > > panel. The provided PWM chip is typically controlled by the
> > > > pwm-backlight driver, which if tied to the panel will provide DPMS.
> > > >
> > > > But with the current implementation the panel will refuse to probe
> > > > because the bridge driver has yet to probe and register the PWM chip,
> > > > and the bridge driver will refuse to probe because it's unable to find
> > > > the panel.
> > >
> > > What you're describing is basically a circular dependency. Can't we get
> > > rid of that in some other way? Why exactly does the bridge driver refuse
> > > to probe if the panel can't be found?
> > >
> > > In other words, I see how the bridge would /use/ the panel in that it
> > > forward a video stream to it. But how does the panel /use/ the bridge?
> > >
> >
> > Yes, this is indeed a circular dependency between the components.
> >
> > The involved parts are:
> > * the bridge driver that implements the PWM chip probe defers on
> > drm_of_find_panel_or_bridge() failing to find the panel.
> > * the pwm-backlight driver that consumes the PWM channel probe defer
> > because the pwm_chip was not registered by the bridge.
> > * the panel that uses the backlight for DPMS purposes probe defer
> > because drm_panel_of_backlight() fails to find the pwm-backlight.
> >
> > I looked at means of postponing drm_of_find_panel_or_bridge() to
> > drm_bridge_funcs->attach(), but at that time "deferral" would be fatal.
> > I looked at registering the pwm_chip earlier, but that would depend on a
> > guarantee of the pwm-backlight and panel driver to probe concurrently.
> > And the current solution of not tying the backlight to the panel means
> > that when userspace decides to DPMS the display the backlight stays on.
> >
> >
> > The proposed solution (hack?) means that DPMS operations happening
> > before the pwm-backlight has probed will be missed, so it's not perfect.
> > It does however allow the backlight on my laptop to turn off, which is a
> > big improvement.
> >
> > But I'm certainly welcome to suggestions.
>
> Entirely hand-waving, why doesn't the following work:
>
> 1. driver for the platform device which is the bridge loads
> 2. that platform driver registers the pwm
> 3. it registers some magic for later on (more below)
> 4. panel driver has deferred loading until step 2 happened
> 5. panel driver registers drm_panel
> 6. the magic from step 3 picks up (after having been deferred for a few
> times probably) grabs the panel, and sets up the actual drm_bridge driver
>
> Everyone happy, or not? From the description it looks like the problem
> that the pwm that we need for the backlight is tied to the same driver as
> the drm_bridge, and always torn down too if the drm_bridge setup fails
> somehow for a reason. And that reason is the circular dependency this
> creates.
>
> Now for the magic in step 3, there's options:
> - change DT to split out that pwm as a separate platform_device, that way
> bridge and panel can load indepedently (hopefully)
>
This is an i2c device, so describing it multiple times would mean we
have multiple devices with the same address...
> - convert bridge to a multi-function device (mfd), essentially a way to
> instantiate more devices with their drivers at runtime. Then the actual
> pwm and drm_bridge parts of your bridge driver bind against those
> sub-functions, and can defer indepedently
>
But, this sounds reasonable and would rely on the existing probe
deferral logic and if there's ever any improvements in this area we
would directly benefit from it.
> - we could create a callback/wait function for "pls wait for any panel to
> show up". Then your bridge driver could launch a work_struct with that
> wait function, which will do the bridge setup once the panel has shown
> up. The pwm will be registered right away. It's essentially hand-rolling
> EPROBE_DEFERRED for work_struct in drm/panel. Maybe we might even have
> that exported from the driver core, e.g.
>
> register_bridge_fn(struct work *)
> {
> do_wait_probe_defer();
> panel = drm_of_find_panel_or_bridge();
> if (!panel) {
> schedule_work(); /* want to restart the work so it can be
> stopped on driver unload */
> return;
> }
>
> /* we have the panel now, register drm_bridge */
> }
>
> - cobble something together with component.c, but that's more for
> collecting unrelated struct device into a logical one than splitting it
> up more.
>
> tldr; I think you can split this loop here at the bridge by untangling the
> pwm
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Tue 08 Dec 06:47 CST 2020, Thierry Reding wrote: > On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote: > > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides > > means of generating a PWM signal for backlight control of the attached > > panel. The provided PWM chip is typically controlled by the > > pwm-backlight driver, which if tied to the panel will provide DPMS. > > > > But with the current implementation the panel will refuse to probe > > because the bridge driver has yet to probe and register the PWM chip, > > and the bridge driver will refuse to probe because it's unable to find > > the panel. > > What you're describing is basically a circular dependency. Can't we get > rid of that in some other way? Why exactly does the bridge driver refuse > to probe if the panel can't be found? > > In other words, I see how the bridge would /use/ the panel in that it > forward a video stream to it. But how does the panel /use/ the bridge? > Yes, this is indeed a circular dependency between the components. The involved parts are: * the bridge driver that implements the PWM chip probe defers on drm_of_find_panel_or_bridge() failing to find the panel. * the pwm-backlight driver that consumes the PWM channel probe defer because the pwm_chip was not registered by the bridge. * the panel that uses the backlight for DPMS purposes probe defer because drm_panel_of_backlight() fails to find the pwm-backlight. I looked at means of postponing drm_of_find_panel_or_bridge() to drm_bridge_funcs->attach(), but at that time "deferral" would be fatal. I looked at registering the pwm_chip earlier, but that would depend on a guarantee of the pwm-backlight and panel driver to probe concurrently. And the current solution of not tying the backlight to the panel means that when userspace decides to DPMS the display the backlight stays on. The proposed solution (hack?) means that DPMS operations happening before the pwm-backlight has probed will be missed, so it's not perfect. It does however allow the backlight on my laptop to turn off, which is a big improvement. But I'm certainly welcome to suggestions. Regards, Bjorn ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Mon 07 Dec 23:48 CST 2020, Sam Ravnborg wrote:
> Hi Bjorn,
> On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote:
> > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides
> > means of generating a PWM signal for backlight control of the attached
> > panel. The provided PWM chip is typically controlled by the
> > pwm-backlight driver, which if tied to the panel will provide DPMS.
> >
> > But with the current implementation the panel will refuse to probe
> > because the bridge driver has yet to probe and register the PWM chip,
> > and the bridge driver will refuse to probe because it's unable to find
> > the panel.
> >
> > Mitigate this catch-22 situation by allowing the panel driver to probe
> > and retry the attachment of the backlight as the panel is turned on or
> > off.
> >
> > Signed-off-by: Bjorn Andersson
> > ---
> > drivers/gpu/drm/drm_panel.c | 47 +++--
> > include/drm/drm_panel.h | 8 +++
> > 2 files changed, 43 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index f634371c717a..7487329bd22d 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -43,6 +43,34 @@ static LIST_HEAD(panel_list);
> > * take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
> > */
> >
> > +#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
> > +static int drm_panel_of_backlight_lazy(struct drm_panel *panel)
> > +{
> > + struct backlight_device *backlight;
> > +
> > + if (!panel || !panel->dev)
> > + return -EINVAL;
> > +
> > + backlight = devm_of_find_backlight(panel->dev);
> > +
> > + if (IS_ERR(backlight)) {
> > + if (PTR_ERR(backlight) == -EPROBE_DEFER) {
> > + panel->backlight_init_pending = true;
> > + return 0;
> > + }
> > +
> > + return PTR_ERR(backlight);
> Use dev_err_probe()
>
I need special handling of EPROBE_DEFER, both in terms of remembering
that we should retry and to not pass the error back to the panel driver.
I also don't want to introduce an error print here.
> > + }
> > +
> > + panel->backlight = backlight;
> > + panel->backlight_init_pending = false;
> > +
> > + return 0;
> > +}
> > +#else
> > +static int drm_panel_of_backlight_lazy(struct drm_panel *panel) { return
> > 0; }
> > +#endif
> > +
> > /**
> > * drm_panel_init - initialize a panel
> > * @panel: DRM panel
> > @@ -161,6 +189,9 @@ int drm_panel_enable(struct drm_panel *panel)
> > return ret;
> > }
> >
> > + if (panel->backlight_init_pending)
> > + drm_panel_of_backlight_lazy(panel);
> > +
> > ret = backlight_enable(panel->backlight);
> > if (ret < 0)
> > DRM_DEV_INFO(panel->dev, "failed to enable backlight: %d\n",
> > @@ -187,6 +218,9 @@ int drm_panel_disable(struct drm_panel *panel)
> > if (!panel)
> > return -EINVAL;
> >
> > + if (panel->backlight_init_pending)
> > + drm_panel_of_backlight_lazy(panel);
> > +
> > ret = backlight_disable(panel->backlight);
> > if (ret < 0)
> > DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n",
> > @@ -328,18 +362,7 @@ EXPORT_SYMBOL(of_drm_get_panel_orientation);
> > */
> > int drm_panel_of_backlight(struct drm_panel *panel)
> > {
> > - struct backlight_device *backlight;
> > -
> > - if (!panel || !panel->dev)
> > - return -EINVAL;
> > -
> > - backlight = devm_of_find_backlight(panel->dev);
> > -
> > - if (IS_ERR(backlight))
> > - return PTR_ERR(backlight);
> > -
> > - panel->backlight = backlight;
> > - return 0;
> > + return drm_panel_of_backlight_lazy(panel);
> Could you update the drm_panel_of_backlight() implementation (and
> do not forget the documentation) and avoid drm_panel_of_backlight_lazy()?
>
That sounds reasonable, there's not really a reason for introducing a
new function for what I'm doing.
>
> > }
> > EXPORT_SYMBOL(drm_panel_of_backlight);
> > #endif
> > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> > index 33605c3f0eba..b126abebb2f3 100644
> > --- a/include/drm/drm_panel.h
> > +++ b/include/drm/drm_panel.h
> > @@ -149,6 +149,14 @@ struct drm_panel {
> > */
> > struct backlight_device *backlight;
> >
> > + /**
> > +* @backlight_init_pending
> > +*
> > +* Backlight driver is not yet available so further attempts to
> > +* initialize @backlight is necessary.
> > +*/
> > + bool backlight_init_pending;
> > +
>
> We have not done so today for other fields, but it would be good
> to document this is for drm_panel use only and drivers shall not touch.
>
Of course.
Thanks,
Bjorn
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Tue, Dec 08, 2020 at 04:02:16PM -0600, Bjorn Andersson wrote:
> On Tue 08 Dec 06:47 CST 2020, Thierry Reding wrote:
>
> > On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote:
> > > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides
> > > means of generating a PWM signal for backlight control of the attached
> > > panel. The provided PWM chip is typically controlled by the
> > > pwm-backlight driver, which if tied to the panel will provide DPMS.
> > >
> > > But with the current implementation the panel will refuse to probe
> > > because the bridge driver has yet to probe and register the PWM chip,
> > > and the bridge driver will refuse to probe because it's unable to find
> > > the panel.
> >
> > What you're describing is basically a circular dependency. Can't we get
> > rid of that in some other way? Why exactly does the bridge driver refuse
> > to probe if the panel can't be found?
> >
> > In other words, I see how the bridge would /use/ the panel in that it
> > forward a video stream to it. But how does the panel /use/ the bridge?
> >
>
> Yes, this is indeed a circular dependency between the components.
>
> The involved parts are:
> * the bridge driver that implements the PWM chip probe defers on
> drm_of_find_panel_or_bridge() failing to find the panel.
> * the pwm-backlight driver that consumes the PWM channel probe defer
> because the pwm_chip was not registered by the bridge.
> * the panel that uses the backlight for DPMS purposes probe defer
> because drm_panel_of_backlight() fails to find the pwm-backlight.
>
> I looked at means of postponing drm_of_find_panel_or_bridge() to
> drm_bridge_funcs->attach(), but at that time "deferral" would be fatal.
> I looked at registering the pwm_chip earlier, but that would depend on a
> guarantee of the pwm-backlight and panel driver to probe concurrently.
> And the current solution of not tying the backlight to the panel means
> that when userspace decides to DPMS the display the backlight stays on.
>
>
> The proposed solution (hack?) means that DPMS operations happening
> before the pwm-backlight has probed will be missed, so it's not perfect.
> It does however allow the backlight on my laptop to turn off, which is a
> big improvement.
>
> But I'm certainly welcome to suggestions.
Entirely hand-waving, why doesn't the following work:
1. driver for the platform device which is the bridge loads
2. that platform driver registers the pwm
3. it registers some magic for later on (more below)
4. panel driver has deferred loading until step 2 happened
5. panel driver registers drm_panel
6. the magic from step 3 picks up (after having been deferred for a few
times probably) grabs the panel, and sets up the actual drm_bridge driver
Everyone happy, or not? From the description it looks like the problem
that the pwm that we need for the backlight is tied to the same driver as
the drm_bridge, and always torn down too if the drm_bridge setup fails
somehow for a reason. And that reason is the circular dependency this
creates.
Now for the magic in step 3, there's options:
- change DT to split out that pwm as a separate platform_device, that way
bridge and panel can load indepedently (hopefully)
- convert bridge to a multi-function device (mfd), essentially a way to
instantiate more devices with their drivers at runtime. Then the actual
pwm and drm_bridge parts of your bridge driver bind against those
sub-functions, and can defer indepedently
- we could create a callback/wait function for "pls wait for any panel to
show up". Then your bridge driver could launch a work_struct with that
wait function, which will do the bridge setup once the panel has shown
up. The pwm will be registered right away. It's essentially hand-rolling
EPROBE_DEFERRED for work_struct in drm/panel. Maybe we might even have
that exported from the driver core, e.g.
register_bridge_fn(struct work *)
{
do_wait_probe_defer();
panel = drm_of_find_panel_or_bridge();
if (!panel) {
schedule_work(); /* want to restart the work so it can be
stopped on driver unload */
return;
}
/* we have the panel now, register drm_bridge */
}
- cobble something together with component.c, but that's more for
collecting unrelated struct device into a logical one than splitting it
up more.
tldr; I think you can split this loop here at the bridge by untangling the
pwm from the drm_bridge part sufficiently.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote: > Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides > means of generating a PWM signal for backlight control of the attached > panel. The provided PWM chip is typically controlled by the > pwm-backlight driver, which if tied to the panel will provide DPMS. > > But with the current implementation the panel will refuse to probe > because the bridge driver has yet to probe and register the PWM chip, > and the bridge driver will refuse to probe because it's unable to find > the panel. What you're describing is basically a circular dependency. Can't we get rid of that in some other way? Why exactly does the bridge driver refuse to probe if the panel can't be found? In other words, I see how the bridge would /use/ the panel in that it forward a video stream to it. But how does the panel /use/ the bridge? Thierry signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC PATCH] drm/panel: Make backlight attachment lazy
Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides
means of generating a PWM signal for backlight control of the attached
panel. The provided PWM chip is typically controlled by the
pwm-backlight driver, which if tied to the panel will provide DPMS.
But with the current implementation the panel will refuse to probe
because the bridge driver has yet to probe and register the PWM chip,
and the bridge driver will refuse to probe because it's unable to find
the panel.
Mitigate this catch-22 situation by allowing the panel driver to probe
and retry the attachment of the backlight as the panel is turned on or
off.
Signed-off-by: Bjorn Andersson
---
drivers/gpu/drm/drm_panel.c | 47 +++--
include/drm/drm_panel.h | 8 +++
2 files changed, 43 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index f634371c717a..7487329bd22d 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -43,6 +43,34 @@ static LIST_HEAD(panel_list);
* take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
*/
+#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
+static int drm_panel_of_backlight_lazy(struct drm_panel *panel)
+{
+ struct backlight_device *backlight;
+
+ if (!panel || !panel->dev)
+ return -EINVAL;
+
+ backlight = devm_of_find_backlight(panel->dev);
+
+ if (IS_ERR(backlight)) {
+ if (PTR_ERR(backlight) == -EPROBE_DEFER) {
+ panel->backlight_init_pending = true;
+ return 0;
+ }
+
+ return PTR_ERR(backlight);
+ }
+
+ panel->backlight = backlight;
+ panel->backlight_init_pending = false;
+
+ return 0;
+}
+#else
+static int drm_panel_of_backlight_lazy(struct drm_panel *panel) { return 0; }
+#endif
+
/**
* drm_panel_init - initialize a panel
* @panel: DRM panel
@@ -161,6 +189,9 @@ int drm_panel_enable(struct drm_panel *panel)
return ret;
}
+ if (panel->backlight_init_pending)
+ drm_panel_of_backlight_lazy(panel);
+
ret = backlight_enable(panel->backlight);
if (ret < 0)
DRM_DEV_INFO(panel->dev, "failed to enable backlight: %d\n",
@@ -187,6 +218,9 @@ int drm_panel_disable(struct drm_panel *panel)
if (!panel)
return -EINVAL;
+ if (panel->backlight_init_pending)
+ drm_panel_of_backlight_lazy(panel);
+
ret = backlight_disable(panel->backlight);
if (ret < 0)
DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n",
@@ -328,18 +362,7 @@ EXPORT_SYMBOL(of_drm_get_panel_orientation);
*/
int drm_panel_of_backlight(struct drm_panel *panel)
{
- struct backlight_device *backlight;
-
- if (!panel || !panel->dev)
- return -EINVAL;
-
- backlight = devm_of_find_backlight(panel->dev);
-
- if (IS_ERR(backlight))
- return PTR_ERR(backlight);
-
- panel->backlight = backlight;
- return 0;
+ return drm_panel_of_backlight_lazy(panel);
}
EXPORT_SYMBOL(drm_panel_of_backlight);
#endif
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 33605c3f0eba..b126abebb2f3 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -149,6 +149,14 @@ struct drm_panel {
*/
struct backlight_device *backlight;
+ /**
+* @backlight_init_pending
+*
+* Backlight driver is not yet available so further attempts to
+* initialize @backlight is necessary.
+*/
+ bool backlight_init_pending;
+
/**
* @funcs:
*
--
2.29.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH] drm/panel: Make backlight attachment lazy
Hi Bjorn,
On Mon, Dec 07, 2020 at 10:44:46PM -0600, Bjorn Andersson wrote:
> Some bridge chips, such as the TI SN65DSI86 DSI/eDP bridge, provides
> means of generating a PWM signal for backlight control of the attached
> panel. The provided PWM chip is typically controlled by the
> pwm-backlight driver, which if tied to the panel will provide DPMS.
>
> But with the current implementation the panel will refuse to probe
> because the bridge driver has yet to probe and register the PWM chip,
> and the bridge driver will refuse to probe because it's unable to find
> the panel.
>
> Mitigate this catch-22 situation by allowing the panel driver to probe
> and retry the attachment of the backlight as the panel is turned on or
> off.
>
> Signed-off-by: Bjorn Andersson
> ---
> drivers/gpu/drm/drm_panel.c | 47 +++--
> include/drm/drm_panel.h | 8 +++
> 2 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index f634371c717a..7487329bd22d 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -43,6 +43,34 @@ static LIST_HEAD(panel_list);
> * take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
> */
>
> +#if IS_REACHABLE(CONFIG_BACKLIGHT_CLASS_DEVICE)
> +static int drm_panel_of_backlight_lazy(struct drm_panel *panel)
> +{
> + struct backlight_device *backlight;
> +
> + if (!panel || !panel->dev)
> + return -EINVAL;
> +
> + backlight = devm_of_find_backlight(panel->dev);
> +
> + if (IS_ERR(backlight)) {
> + if (PTR_ERR(backlight) == -EPROBE_DEFER) {
> + panel->backlight_init_pending = true;
> + return 0;
> + }
> +
> + return PTR_ERR(backlight);
Use dev_err_probe()
> + }
> +
> + panel->backlight = backlight;
> + panel->backlight_init_pending = false;
> +
> + return 0;
> +}
> +#else
> +static int drm_panel_of_backlight_lazy(struct drm_panel *panel) { return 0; }
> +#endif
> +
> /**
> * drm_panel_init - initialize a panel
> * @panel: DRM panel
> @@ -161,6 +189,9 @@ int drm_panel_enable(struct drm_panel *panel)
> return ret;
> }
>
> + if (panel->backlight_init_pending)
> + drm_panel_of_backlight_lazy(panel);
> +
> ret = backlight_enable(panel->backlight);
> if (ret < 0)
> DRM_DEV_INFO(panel->dev, "failed to enable backlight: %d\n",
> @@ -187,6 +218,9 @@ int drm_panel_disable(struct drm_panel *panel)
> if (!panel)
> return -EINVAL;
>
> + if (panel->backlight_init_pending)
> + drm_panel_of_backlight_lazy(panel);
> +
> ret = backlight_disable(panel->backlight);
> if (ret < 0)
> DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n",
> @@ -328,18 +362,7 @@ EXPORT_SYMBOL(of_drm_get_panel_orientation);
> */
> int drm_panel_of_backlight(struct drm_panel *panel)
> {
> - struct backlight_device *backlight;
> -
> - if (!panel || !panel->dev)
> - return -EINVAL;
> -
> - backlight = devm_of_find_backlight(panel->dev);
> -
> - if (IS_ERR(backlight))
> - return PTR_ERR(backlight);
> -
> - panel->backlight = backlight;
> - return 0;
> + return drm_panel_of_backlight_lazy(panel);
Could you update the drm_panel_of_backlight() implementation (and
do not forget the documentation) and avoid drm_panel_of_backlight_lazy()?
Sam
> }
> EXPORT_SYMBOL(drm_panel_of_backlight);
> #endif
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 33605c3f0eba..b126abebb2f3 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -149,6 +149,14 @@ struct drm_panel {
>*/
> struct backlight_device *backlight;
>
> + /**
> + * @backlight_init_pending
> + *
> + * Backlight driver is not yet available so further attempts to
> + * initialize @backlight is necessary.
> + */
> + bool backlight_init_pending;
> +
We have not done so today for other fields, but it would be good
to document this is for drm_panel use only and drivers shall not touch.
Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
