| Message ID | 20170119175830.3754-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote: > The primary consumer of the lpss pwm is the i915 kms driver, > the i915 driver does not support get_pwm returning -EPROBE_DEFER and > its init is very complex making this is almost impossible to fix. > > This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so > that when the i915 driver loads the lpss pwm will be available avoiding > the -EPROBE_DEFER issue. Note that this is identical to how the same > problem was solved for the pwm-crc driver, which is used by the i915 > driver on other platforms. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Acked-by: Jani Nikula <jani.nikula@intel.com> > --- > Changes in v2: > -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver) > --- > drivers/pwm/Kconfig | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) For the record I think this is completely wrong and i915 should be taught how to deal with -EPROBE_DEFER. We've gone through a lot of pain to clean up this kind of init-level ordering on other devices and the result is, in my opinion, a *lot* better than what we had before. It'd be shame to see i915 backpedal on that. That said, if everyone else thinks that it really can't be done and this workaround is the best way forward, I'll just shut up about it and stop caring. Mika, Andy, any objections to this? Thierry
On Fri, Jan 20, 2017 at 08:03:33AM +0100, Thierry Reding wrote: > On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote: > > The primary consumer of the lpss pwm is the i915 kms driver, > > the i915 driver does not support get_pwm returning -EPROBE_DEFER and > > its init is very complex making this is almost impossible to fix. > > > > This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so > > that when the i915 driver loads the lpss pwm will be available avoiding > > the -EPROBE_DEFER issue. Note that this is identical to how the same > > problem was solved for the pwm-crc driver, which is used by the i915 > > driver on other platforms. > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > --- > > Changes in v2: > > -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver) > > --- > > drivers/pwm/Kconfig | 12 +++--------- > > 1 file changed, 3 insertions(+), 9 deletions(-) > > For the record I think this is completely wrong and i915 should be > taught how to deal with -EPROBE_DEFER. We've gone through a lot of > pain to clean up this kind of init-level ordering on other devices > and the result is, in my opinion, a *lot* better than what we had > before. It'd be shame to see i915 backpedal on that. Looking into i915 a little, I don't see why handling -EPROBE_DEFER would be very complicated. The call stack looks somewhat like this: i915_pci_probe() i915_driver_load() i915_load_modeset_init() intel_modeset_init() intel_setup_outputs() intel_dsi_init() intel_panel_setup_backlight() pwm_setup_backlight() In the above, intel_modeset_init() is the last one that propagates errors, but its caller, i915_load_modeset_init() properly handles failure from other function calls. Also, pwm_setup_backlight() can return errors to intel_panel_setup_backlight(), which will in turn propagate them to intel_dsi_init(). So I'd think that in order to properly handle -EPROBE_DEFER you'd only need to propagate errors back up this way: intel_panel_setup_backlight() intel_dsi_init() intel_setup_outputs() intel_modeset_init() That seems to me to be far from "almost impossible". Thierry
Hi, On 20-01-17 08:18, Thierry Reding wrote: > On Fri, Jan 20, 2017 at 08:03:33AM +0100, Thierry Reding wrote: >> On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote: >>> The primary consumer of the lpss pwm is the i915 kms driver, >>> the i915 driver does not support get_pwm returning -EPROBE_DEFER and >>> its init is very complex making this is almost impossible to fix. >>> >>> This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so >>> that when the i915 driver loads the lpss pwm will be available avoiding >>> the -EPROBE_DEFER issue. Note that this is identical to how the same >>> problem was solved for the pwm-crc driver, which is used by the i915 >>> driver on other platforms. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> Acked-by: Jani Nikula <jani.nikula@intel.com> >>> --- >>> Changes in v2: >>> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver) >>> --- >>> drivers/pwm/Kconfig | 12 +++--------- >>> 1 file changed, 3 insertions(+), 9 deletions(-) >> >> For the record I think this is completely wrong and i915 should be >> taught how to deal with -EPROBE_DEFER. We've gone through a lot of >> pain to clean up this kind of init-level ordering on other devices >> and the result is, in my opinion, a *lot* better than what we had >> before. It'd be shame to see i915 backpedal on that. > > Looking into i915 a little, I don't see why handling -EPROBE_DEFER would > be very complicated. The call stack looks somewhat like this: > > i915_pci_probe() > i915_driver_load() > i915_load_modeset_init() > intel_modeset_init() > intel_setup_outputs() > intel_dsi_init() > intel_panel_setup_backlight() > pwm_setup_backlight() > > In the above, intel_modeset_init() is the last one that propagates > errors, but its caller, i915_load_modeset_init() properly handles > failure from other function calls. Also, pwm_setup_backlight() can > return errors to intel_panel_setup_backlight(), which will in turn > propagate them to intel_dsi_init(). > > So I'd think that in order to properly handle -EPROBE_DEFER you'd only > need to propagate errors back up this way: > > intel_panel_setup_backlight() > intel_dsi_init() > intel_setup_outputs() > intel_modeset_init() > > That seems to me to be far from "almost impossible". I will let the i915 devs answer this. IIRC a lot of setup is already done at this point, including some changes to the hw and we really do not want to change the init-sequence, or repeat parts of it. I myself was actually thinking about solving this by figuring out if the pwm is necessary and getting it early on, but the figuring out bit is non trivial. Anyways as said I will let the i915 devs answer this. Regards, Hans
On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote: > On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote: >> The primary consumer of the lpss pwm is the i915 kms driver, >> the i915 driver does not support get_pwm returning -EPROBE_DEFER and >> its init is very complex making this is almost impossible to fix. >> >> This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so >> that when the i915 driver loads the lpss pwm will be available avoiding >> the -EPROBE_DEFER issue. Note that this is identical to how the same >> problem was solved for the pwm-crc driver, which is used by the i915 >> driver on other platforms. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> Acked-by: Jani Nikula <jani.nikula@intel.com> >> --- >> Changes in v2: >> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver) >> --- >> drivers/pwm/Kconfig | 12 +++--------- >> 1 file changed, 3 insertions(+), 9 deletions(-) > > For the record I think this is completely wrong and i915 should be > taught how to deal with -EPROBE_DEFER. We've gone through a lot of > pain to clean up this kind of init-level ordering on other devices > and the result is, in my opinion, a *lot* better than what we had > before. It'd be shame to see i915 backpedal on that. > > That said, if everyone else thinks that it really can't be done and > this workaround is the best way forward, I'll just shut up about it > and stop caring. Superficially, it is, of course, easy to agree we should handle deferred probing. i915 is a complex driver for complex hardware. We require a ton of initialization before we even get to the point we realize we might need the PWM. Naturally, we'd need to gracefully tear all that down for -EPROBE_DEFER handling. And we've been slowly working towards this; we've even added injected probe failures in CI to test this. But we're not there yet. This patch seems like a rather simple workaround for the time being. There are two other related things I wonder about. I see module reloading mostly as a developer feature. I don't think I'm alone in that. You just don't recommend anyone doing module reloads in a production environment. However, deferred probing is in some ways more demanding than module reload, because it needs to gracefully handle partial probes. Yet that is the solution of choice for init ordering. Makes you wonder. Another thing that comes up a lot with graphics is that people really do appreciate any crappy degraded image over a black screen. If the PWM never shows up, all the external screens will be black in addition to the embedded display. We're always torn between failing fast vs. plunging on despite failures. --- That said, I suppose there could be an alternative to handling pwm_get() failures at probe. We could just go on with our init, but schedule a retry later. Perhaps a bit hacky, but it would address both of the concerns above. Again, this patch seems a simple workaround in the mean time. BR, Jani.
On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: > On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote: > >> The primary consumer of the lpss pwm is the i915 kms driver, > >> the i915 driver does not support get_pwm returning -EPROBE_DEFER and > >> its init is very complex making this is almost impossible to fix. > >> > >> This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so > >> that when the i915 driver loads the lpss pwm will be available avoiding > >> the -EPROBE_DEFER issue. Note that this is identical to how the same > >> problem was solved for the pwm-crc driver, which is used by the i915 > >> driver on other platforms. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > >> Acked-by: Jani Nikula <jani.nikula@intel.com> > >> --- > >> Changes in v2: > >> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver) > >> --- > >> drivers/pwm/Kconfig | 12 +++--------- > >> 1 file changed, 3 insertions(+), 9 deletions(-) > > > > For the record I think this is completely wrong and i915 should be > > taught how to deal with -EPROBE_DEFER. We've gone through a lot of > > pain to clean up this kind of init-level ordering on other devices > > and the result is, in my opinion, a *lot* better than what we had > > before. It'd be shame to see i915 backpedal on that. > > > > That said, if everyone else thinks that it really can't be done and > > this workaround is the best way forward, I'll just shut up about it > > and stop caring. > > Superficially, it is, of course, easy to agree we should handle deferred > probing. > > i915 is a complex driver for complex hardware. We require a ton of > initialization before we even get to the point we realize we might need > the PWM. Naturally, we'd need to gracefully tear all that down for > -EPROBE_DEFER handling. And we've been slowly working towards this; > we've even added injected probe failures in CI to test this. But we're > not there yet. This patch seems like a rather simple workaround for the > time being. > > There are two other related things I wonder about. > > I see module reloading mostly as a developer feature. I don't think I'm > alone in that. You just don't recommend anyone doing module reloads in a > production environment. However, deferred probing is in some ways more > demanding than module reload, because it needs to gracefully handle > partial probes. Yet that is the solution of choice for init > ordering. Makes you wonder. Well, there have been proposals in the past to get rid of deferred probing by replacing it with something more formal, but it's a fairly difficult issue to solve. While deferred probing is indeed a rather heavy-weight solution, it's one that's proven to work well enough for most of the world. Also gracefully handling partial probes is something you need in most cases anyway. Typically the easiest solution is to make sure to run all the code that could possibly fail as early as possible, like Hans had suggested, so that you need to unwind as little as possible. > Another thing that comes up a lot with graphics is that people really do > appreciate any crappy degraded image over a black screen. If the PWM > never shows up, all the external screens will be black in addition to > the embedded display. We're always torn between failing fast > vs. plunging on despite failures. Yes, I see how deferred probe would get in the way here. To be fair, deferred probing was never meant to solve this kind of use-case. One of the reasons why it is so heavy-weight is that drivers can usually not continue without the resources they're trying to get. In this particular case, however, only a very small subset of the driver relies on the PWM, so it's more of an optional, nice to have, resource rather than an essential one. > That said, I suppose there could be an alternative to handling pwm_get() > failures at probe. We could just go on with our init, but schedule a > retry later. Perhaps a bit hacky, but it would address both of the > concerns above. Again, this patch seems a simple workaround in the mean > time. I don't think that's hacky at all. In fact I think it's a really nice solution for this particular case and could probably be a good fit for other use-cases as well. As for adding a simple workaround in the meantime, is that really necessary? This doesn't really fix any bugs, right? Its just that new hardware may not work properly, isn't it? I'm somewhat reluctant to temporarily add this workaround because I know Paul Gortmaker will immediately send out a patch to make the driver use builtin_{pci, platform}_driver and all of a sudden we've got a bunch of things to untangle because of a "simple workaround". Hans, do you think you could find some time to at least investigate whether or not Jani's proposal above would be a viable option that wouldn't take ages to implement? If it's excessively complicated to do, I'm okay with this patch, though you may want to do the module_* to builtin_* conversion while at it to save Paul the extra work. And maybe add a comment somewhere that this is meant to be a temporary workaround until i915 can deal with this more nicely? Thanks, Thierry
Hi, On 20-01-17 09:56, Thierry Reding wrote: > On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: >> On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote: >>> On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote: >>>> The primary consumer of the lpss pwm is the i915 kms driver, >>>> the i915 driver does not support get_pwm returning -EPROBE_DEFER and >>>> its init is very complex making this is almost impossible to fix. >>>> >>>> This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so >>>> that when the i915 driver loads the lpss pwm will be available avoiding >>>> the -EPROBE_DEFER issue. Note that this is identical to how the same >>>> problem was solved for the pwm-crc driver, which is used by the i915 >>>> driver on other platforms. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> Acked-by: Jani Nikula <jani.nikula@intel.com> >>>> --- >>>> Changes in v2: >>>> -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver) >>>> --- >>>> drivers/pwm/Kconfig | 12 +++--------- >>>> 1 file changed, 3 insertions(+), 9 deletions(-) >>> >>> For the record I think this is completely wrong and i915 should be >>> taught how to deal with -EPROBE_DEFER. We've gone through a lot of >>> pain to clean up this kind of init-level ordering on other devices >>> and the result is, in my opinion, a *lot* better than what we had >>> before. It'd be shame to see i915 backpedal on that. >>> >>> That said, if everyone else thinks that it really can't be done and >>> this workaround is the best way forward, I'll just shut up about it >>> and stop caring. >> >> Superficially, it is, of course, easy to agree we should handle deferred >> probing. >> >> i915 is a complex driver for complex hardware. We require a ton of >> initialization before we even get to the point we realize we might need >> the PWM. Naturally, we'd need to gracefully tear all that down for >> -EPROBE_DEFER handling. And we've been slowly working towards this; >> we've even added injected probe failures in CI to test this. But we're >> not there yet. This patch seems like a rather simple workaround for the >> time being. >> >> There are two other related things I wonder about. >> >> I see module reloading mostly as a developer feature. I don't think I'm >> alone in that. You just don't recommend anyone doing module reloads in a >> production environment. However, deferred probing is in some ways more >> demanding than module reload, because it needs to gracefully handle >> partial probes. Yet that is the solution of choice for init >> ordering. Makes you wonder. > > Well, there have been proposals in the past to get rid of deferred > probing by replacing it with something more formal, but it's a fairly > difficult issue to solve. While deferred probing is indeed a rather > heavy-weight solution, it's one that's proven to work well enough for > most of the world. > > Also gracefully handling partial probes is something you need in most > cases anyway. Typically the easiest solution is to make sure to run all > the code that could possibly fail as early as possible, like Hans had > suggested, so that you need to unwind as little as possible. > >> Another thing that comes up a lot with graphics is that people really do >> appreciate any crappy degraded image over a black screen. If the PWM >> never shows up, all the external screens will be black in addition to >> the embedded display. We're always torn between failing fast >> vs. plunging on despite failures. > > Yes, I see how deferred probe would get in the way here. To be fair, > deferred probing was never meant to solve this kind of use-case. One > of the reasons why it is so heavy-weight is that drivers can usually > not continue without the resources they're trying to get. > > In this particular case, however, only a very small subset of the driver > relies on the PWM, so it's more of an optional, nice to have, resource > rather than an essential one. > >> That said, I suppose there could be an alternative to handling pwm_get() >> failures at probe. We could just go on with our init, but schedule a >> retry later. Perhaps a bit hacky, but it would address both of the >> concerns above. Again, this patch seems a simple workaround in the mean >> time. > > I don't think that's hacky at all. In fact I think it's a really nice > solution for this particular case and could probably be a good fit for > other use-cases as well. > > As for adding a simple workaround in the meantime, is that really > necessary? This doesn't really fix any bugs, right? It fixes the bug of not being able to control the backlight on many cherrytrail devices. > Its just that new > hardware may not work properly, isn't it? I'm somewhat reluctant to > temporarily add this workaround because I know Paul Gortmaker will > immediately send out a patch to make the driver use builtin_{pci, > platform}_driver and all of a sudden we've got a bunch of things to > untangle because of a "simple workaround". > > Hans, do you think you could find some time to at least investigate > whether or not Jani's proposal above would be a viable option that > wouldn't take ages to implement? The whole backlight situation on x86 is much more complicated then it has any right to be I'm afraid. If the i915 driver does not register a backlight interface right away, then the acpi-video driver will register a backlight interface instead (*), which may or may not work and if userspace manages to try and use that interface before the i915 driver gets around to register its own interface the firmware may mess up some i915 or pwm_lpss register settings in such a way that the backlight will later never work, flicker, be inverted, whatever. *) Which will get unregistered again when the i915 driver does register its own backlight later, yes I kid you not. The whole firmware interface to the backlight stuff on x86 is a horrendous mess (I know I've spend a significant amount of time the last couple of years making it mostly work and adding dmi based quirks where necessary). > If it's excessively complicated to do, It is probably doable, but I'm very much against trick with this because of what I've written above. Ah this also brings up another problem with the i915 driver init order. On systems without an i915 vbios opregion, the acpi-video bus driver will immediately enumerate that "bus" and register e.g. backlight device(s) based on acpi tables. But when an i915 vbios opregion is present, the acpi-video bus modules' init function will just return 0 and expects the i915 driver to call its probe function later, after the i915 opregion's init function has run, because otherwise bad things may happen. I've not yet checked if this call to acpi_video's probe function happens before or after we try to get the pwm, but if it happens before, undoing that is also a problem ... > I'm okay with this patch, though you may want to do the module_* > to builtin_* conversion while at it to save Paul the extra work. And > maybe add a comment somewhere that this is meant to be a temporary > workaround until i915 can deal with this more nicely? I'm fine with doing a v3 with a comment, how about putting that comment right at all the module* stuff and explain there that that is to stay as the builtin only status is meant to be temporary ? Regards, Hans
On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote: > I'm fine with doing a v3 with a comment, how about putting that > comment > right at all the module* stuff and explain there that that is to > stay as the builtin only status is meant to be temporary ? Can we do other way around? I mean that either i915 selects PWM_LPSS to be built-in, or uses request_module() call?
On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: > That said, I suppose there could be an alternative to handling pwm_get() > failures at probe. We could just go on with our init, but schedule a > retry later. Perhaps a bit hacky, but it would address both of the > concerns above. Again, this patch seems a simple workaround in the mean > time. Not sure if this works or how hacky it is, but can't you request_module() before you start looking up for the pwm?
On Fri, Jan 20, 2017 at 10:48:52AM +0100, Hans de Goede wrote: > Hi, > > On 20-01-17 09:56, Thierry Reding wrote: > > On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: > > > On Fri, 20 Jan 2017, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > On Thu, Jan 19, 2017 at 06:58:30PM +0100, Hans de Goede wrote: > > > > > The primary consumer of the lpss pwm is the i915 kms driver, > > > > > the i915 driver does not support get_pwm returning -EPROBE_DEFER and > > > > > its init is very complex making this is almost impossible to fix. > > > > > > > > > > This commit changes the PWM_LPSS Kconfig from a tristate to a bool, so > > > > > that when the i915 driver loads the lpss pwm will be available avoiding > > > > > the -EPROBE_DEFER issue. Note that this is identical to how the same > > > > > problem was solved for the pwm-crc driver, which is used by the i915 > > > > > driver on other platforms. > > > > > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > Acked-by: Jani Nikula <jani.nikula@intel.com> > > > > > --- > > > > > Changes in v2: > > > > > -Drop the pwm_add_table call (this has been moved to the acpi_lpss driver) > > > > > --- > > > > > drivers/pwm/Kconfig | 12 +++--------- > > > > > 1 file changed, 3 insertions(+), 9 deletions(-) > > > > > > > > For the record I think this is completely wrong and i915 should be > > > > taught how to deal with -EPROBE_DEFER. We've gone through a lot of > > > > pain to clean up this kind of init-level ordering on other devices > > > > and the result is, in my opinion, a *lot* better than what we had > > > > before. It'd be shame to see i915 backpedal on that. > > > > > > > > That said, if everyone else thinks that it really can't be done and > > > > this workaround is the best way forward, I'll just shut up about it > > > > and stop caring. > > > > > > Superficially, it is, of course, easy to agree we should handle deferred > > > probing. > > > > > > i915 is a complex driver for complex hardware. We require a ton of > > > initialization before we even get to the point we realize we might need > > > the PWM. Naturally, we'd need to gracefully tear all that down for > > > -EPROBE_DEFER handling. And we've been slowly working towards this; > > > we've even added injected probe failures in CI to test this. But we're > > > not there yet. This patch seems like a rather simple workaround for the > > > time being. > > > > > > There are two other related things I wonder about. > > > > > > I see module reloading mostly as a developer feature. I don't think I'm > > > alone in that. You just don't recommend anyone doing module reloads in a > > > production environment. However, deferred probing is in some ways more > > > demanding than module reload, because it needs to gracefully handle > > > partial probes. Yet that is the solution of choice for init > > > ordering. Makes you wonder. > > > > Well, there have been proposals in the past to get rid of deferred > > probing by replacing it with something more formal, but it's a fairly > > difficult issue to solve. While deferred probing is indeed a rather > > heavy-weight solution, it's one that's proven to work well enough for > > most of the world. > > > > Also gracefully handling partial probes is something you need in most > > cases anyway. Typically the easiest solution is to make sure to run all > > the code that could possibly fail as early as possible, like Hans had > > suggested, so that you need to unwind as little as possible. > > > > > Another thing that comes up a lot with graphics is that people really do > > > appreciate any crappy degraded image over a black screen. If the PWM > > > never shows up, all the external screens will be black in addition to > > > the embedded display. We're always torn between failing fast > > > vs. plunging on despite failures. > > > > Yes, I see how deferred probe would get in the way here. To be fair, > > deferred probing was never meant to solve this kind of use-case. One > > of the reasons why it is so heavy-weight is that drivers can usually > > not continue without the resources they're trying to get. > > > > In this particular case, however, only a very small subset of the driver > > relies on the PWM, so it's more of an optional, nice to have, resource > > rather than an essential one. > > > > > That said, I suppose there could be an alternative to handling pwm_get() > > > failures at probe. We could just go on with our init, but schedule a > > > retry later. Perhaps a bit hacky, but it would address both of the > > > concerns above. Again, this patch seems a simple workaround in the mean > > > time. > > > > I don't think that's hacky at all. In fact I think it's a really nice > > solution for this particular case and could probably be a good fit for > > other use-cases as well. > > > > As for adding a simple workaround in the meantime, is that really > > necessary? This doesn't really fix any bugs, right? > > It fixes the bug of not being able to control the backlight on many > cherrytrail devices. Okay, but it's not a regression because this clearly can't have worked before, either. What I'm trying to say is that a workaround is much more acceptable if it fixes a regression. For new features we try to do things properly, even if they are complicated. > > Its just that new > > hardware may not work properly, isn't it? I'm somewhat reluctant to > > temporarily add this workaround because I know Paul Gortmaker will > > immediately send out a patch to make the driver use builtin_{pci, > > platform}_driver and all of a sudden we've got a bunch of things to > > untangle because of a "simple workaround". > > > > Hans, do you think you could find some time to at least investigate > > whether or not Jani's proposal above would be a viable option that > > wouldn't take ages to implement? > > The whole backlight situation on x86 is much more complicated then > it has any right to be I'm afraid. If the i915 driver does not register > a backlight interface right away, then the acpi-video driver will > register a backlight interface instead (*), which may or may not work > and if userspace manages to try and use that interface before the > i915 driver gets around to register its own interface the firmware > may mess up some i915 or pwm_lpss register settings in such a way > that the backlight will later never work, flicker, be inverted, > whatever. > > *) Which will get unregistered again when the i915 driver does > register its own backlight later, yes I kid you not. > > The whole firmware interface to the backlight stuff on x86 is > a horrendous mess (I know I've spend a significant amount of time > the last couple of years making it mostly work and adding dmi based > quirks where necessary). I don't understand why people keep saying that ARM is messy... > > If it's excessively complicated to do, > > It is probably doable, but I'm very much against trick with this > because of what I've written above. Ah this also brings up another > problem with the i915 driver init order. On systems without an > i915 vbios opregion, the acpi-video bus driver will immediately > enumerate that "bus" and register e.g. backlight device(s) based > on acpi tables. But when an i915 vbios opregion is present, > the acpi-video bus modules' init function will just return 0 and > expects the i915 driver to call its probe function later, after > the i915 opregion's init function has run, because otherwise > bad things may happen. > > I've not yet checked if this call to acpi_video's probe function > happens before or after we try to get the pwm, but if it happens > before, undoing that is also a problem ... > > > I'm okay with this patch, though you may want to do the module_* > > to builtin_* conversion while at it to save Paul the extra work. And > > maybe add a comment somewhere that this is meant to be a temporary > > workaround until i915 can deal with this more nicely? > > I'm fine with doing a v3 with a comment, how about putting that comment > right at all the module* stuff and explain there that that is to > stay as the builtin only status is meant to be temporary ? Given the above and the extent of whackiness involved here, do you really think the state will be temporary? If it isn't then there's no need to add a comment that we'll never get rid of again. I'll leave it up to you whether or not you want to add the comment or convert to builtin_*, but if you do the former, right near the module_* seems fine. I'll do my best to fend off Paul until you've fixed the remaining root causes. =) Thierry
Hi, On 20-01-17 10:55, Andy Shevchenko wrote: > On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote: >> I'm fine with doing a v3 with a comment, how about putting that >> comment >> right at all the module* stuff and explain there that that is to >> stay as the builtin only status is meant to be temporary ? > > Can we do other way around? I mean that either i915 selects PWM_LPSS to > be built-in, or uses request_module() call? PWM_LPSS needs to be built-in if enabled, a stripped down kernel for non cherrytrail hardware does not need it ... Also (and esp for request_module) this means building knowledge into the i915 driver about which pwm hardware there is on which boards which is undesirable. Thierry, this does give me an idea though, what if we extend the info passed to pwm_add_table with a module-name and make pwm_get call request_module() ? That seems like a good solution and we could then also make PWM_CRC a tristate again (after someone with hw access has tested this). Regards, Hans
On Fri, Jan 20, 2017 at 11:18:29AM +0100, Hans de Goede wrote: > Hi, > > On 20-01-17 10:55, Andy Shevchenko wrote: > > On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote: > > > I'm fine with doing a v3 with a comment, how about putting that > > > comment > > > right at all the module* stuff and explain there that that is to > > > stay as the builtin only status is meant to be temporary ? > > > > Can we do other way around? I mean that either i915 selects PWM_LPSS to > > be built-in, or uses request_module() call? > > PWM_LPSS needs to be built-in if enabled, a stripped down kernel > for non cherrytrail hardware does not need it ... > > Also (and esp for request_module) this means building knowledge > into the i915 driver about which pwm hardware there is on which > boards which is undesirable. > > Thierry, this does give me an idea though, what if we extend > the info passed to pwm_add_table with a module-name and > make pwm_get call request_module() ? I'm not sure that's even necessary. request_module() forwards the string you pass to it to the userspace helper, so you can pass things like the modalias to it. I suspect that for ACPI the modalias could be trivially derived from the provider name already in the table. Or maybe it couldn't. We don't have information about the type of device and the name doesn't match, or at least it would be much more difficult to extend the same method to other types of devices. Module name doesn't sound too bad, as long as it's optional. Thierry
Hi, On 20-01-17 11:42, Thierry Reding wrote: > On Fri, Jan 20, 2017 at 11:18:29AM +0100, Hans de Goede wrote: >> Hi, >> >> On 20-01-17 10:55, Andy Shevchenko wrote: >>> On Fri, 2017-01-20 at 10:48 +0100, Hans de Goede wrote: >>>> I'm fine with doing a v3 with a comment, how about putting that >>>> comment >>>> right at all the module* stuff and explain there that that is to >>>> stay as the builtin only status is meant to be temporary ? >>> >>> Can we do other way around? I mean that either i915 selects PWM_LPSS to >>> be built-in, or uses request_module() call? >> >> PWM_LPSS needs to be built-in if enabled, a stripped down kernel >> for non cherrytrail hardware does not need it ... >> >> Also (and esp for request_module) this means building knowledge >> into the i915 driver about which pwm hardware there is on which >> boards which is undesirable. >> >> Thierry, this does give me an idea though, what if we extend >> the info passed to pwm_add_table with a module-name and >> make pwm_get call request_module() ? > > I'm not sure that's even necessary. request_module() forwards the string > you pass to it to the userspace helper, so you can pass things like the > modalias to it. I suspect that for ACPI the modalias could be trivially > derived from the provider name already in the table. As you can see in the patch-set I've just send I've chosen to go with the module_name in pwm_lookup. You're right that simply doing: module_request("acpi:%s", chosen->provider); Would work, but I don't like having the "acpi:%s" bit in the pwm-core, esp. not since someone will them come along and add support for i2c. pci, etc. So just adding a module_name field seems saner. Regards, Hans
On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: >> That said, I suppose there could be an alternative to handling pwm_get() >> failures at probe. We could just go on with our init, but schedule a >> retry later. Perhaps a bit hacky, but it would address both of the >> concerns above. Again, this patch seems a simple workaround in the mean >> time. > > Not sure if this works or how hacky it is, but can't you > request_module() before you start looking up for the pwm? I eyeballed this a little, and noticed: drivers/acpi/acpi_lpss.c: static struct pwm_lookup bsw_pwm_lookup[] = { PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL, "pwm-lpss-platform"), }; drivers/mfd/intel_soc_pmic_core.c: static struct pwm_lookup crc_pwm_lookup[] = { PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL), }; Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module exactly? pwm_get() does an automatic request_module(), if the module is given. And will this still be enough? BR, Jani.
Hi, On 08-03-17 10:40, Jani Nikula wrote: > On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: >> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: >>> That said, I suppose there could be an alternative to handling pwm_get() >>> failures at probe. We could just go on with our init, but schedule a >>> retry later. Perhaps a bit hacky, but it would address both of the >>> concerns above. Again, this patch seems a simple workaround in the mean >>> time. >> >> Not sure if this works or how hacky it is, but can't you >> request_module() before you start looking up for the pwm? > > I eyeballed this a little, and noticed: > > drivers/acpi/acpi_lpss.c: > > static struct pwm_lookup bsw_pwm_lookup[] = { > PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0", > "pwm_backlight", 0, PWM_POLARITY_NORMAL, > "pwm-lpss-platform"), > }; > > drivers/mfd/intel_soc_pmic_core.c: > > static struct pwm_lookup crc_pwm_lookup[] = { > PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL), > }; > > Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module > exactly? pwm_get() does an automatic request_module(), if the module is > given. > > And will this still be enough? I was thinking about this a couple of days ago, unfortunately the situation with pwm_crc is more complicated as that is part of an i2c mfd device, so both the i2c-adapter driver and the mfd driver (intel_soc_pmic_crc) need to be builtin currently the mfd driver cannot be modular, but the i2c-adapter driver can (and on most Linux distribution kernels is) configured to be modular. So just doing request module for pwm-crc is not going to help since the i2c-adapter driver may not yet have loaded / initialized. All in all we really need to find a way to figure out if we will need to do a pwm_get earlier on during i915 initialization (by moving the VBT parsing to earlier, or at least part of it) and do the pwm_get before we do anything i-reversible and if it fails then return -EPROBE_DEFER. Then we can make pwm_crc modular as well as all of intel_soc_pmic* as it really should be. Regards, Hans
On Wed, 08 Mar 2017, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 08-03-17 10:40, Jani Nikula wrote: >> On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: >>> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: >>>> That said, I suppose there could be an alternative to handling pwm_get() >>>> failures at probe. We could just go on with our init, but schedule a >>>> retry later. Perhaps a bit hacky, but it would address both of the >>>> concerns above. Again, this patch seems a simple workaround in the mean >>>> time. >>> >>> Not sure if this works or how hacky it is, but can't you >>> request_module() before you start looking up for the pwm? >> >> I eyeballed this a little, and noticed: >> >> drivers/acpi/acpi_lpss.c: >> >> static struct pwm_lookup bsw_pwm_lookup[] = { >> PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0", >> "pwm_backlight", 0, PWM_POLARITY_NORMAL, >> "pwm-lpss-platform"), >> }; >> >> drivers/mfd/intel_soc_pmic_core.c: >> >> static struct pwm_lookup crc_pwm_lookup[] = { >> PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL), >> }; >> >> Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module >> exactly? pwm_get() does an automatic request_module(), if the module is >> given. >> >> And will this still be enough? > > I was thinking about this a couple of days ago, unfortunately the > situation with pwm_crc is more complicated as that is part of > an i2c mfd device, so both the i2c-adapter driver and the mfd driver > (intel_soc_pmic_crc) need to be builtin currently the mfd driver > cannot be modular, but the i2c-adapter driver can (and on most > Linux distribution kernels is) configured to be modular. > > So just doing request module for pwm-crc is not going to help > since the i2c-adapter driver may not yet have loaded / initialized. > > All in all we really need to find a way to figure out if we will need > to do a pwm_get earlier on during i915 initialization (by moving the > VBT parsing to earlier, or at least part of it) and do the pwm_get > before we do anything i-reversible and if it fails then return > -EPROBE_DEFER. Then we can make pwm_crc modular as well as all of > intel_soc_pmic* as it really should be. The other alternatives are: 1) Handle defers using a workqueue within i915. It's a bit tedious, but I didn't spot any show stoppers with the approach. We'd register a non-functional backlight interface until the pwm_get() succeeds. 2) Rip out pwm backlight from i915 altogether, and turn it into a separate platform backlight that uses pwm. I think there's ready infrastructure for that. It's not without problems, though, as then we lose control over the sequence in which backlight gets enabled/disabled. BR, Jani. > > Regards, > > Hans
Hi, On 08-03-17 11:15, Jani Nikula wrote: > On Wed, 08 Mar 2017, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 08-03-17 10:40, Jani Nikula wrote: >>> On Fri, 20 Jan 2017, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: >>>> On Fri, Jan 20, 2017 at 10:02:50AM +0200, Jani Nikula wrote: >>>>> That said, I suppose there could be an alternative to handling pwm_get() >>>>> failures at probe. We could just go on with our init, but schedule a >>>>> retry later. Perhaps a bit hacky, but it would address both of the >>>>> concerns above. Again, this patch seems a simple workaround in the mean >>>>> time. >>>> >>>> Not sure if this works or how hacky it is, but can't you >>>> request_module() before you start looking up for the pwm? >>> >>> I eyeballed this a little, and noticed: >>> >>> drivers/acpi/acpi_lpss.c: >>> >>> static struct pwm_lookup bsw_pwm_lookup[] = { >>> PWM_LOOKUP_WITH_MODULE("80862288:00", 0, "0000:00:02.0", >>> "pwm_backlight", 0, PWM_POLARITY_NORMAL, >>> "pwm-lpss-platform"), >>> }; >>> >>> drivers/mfd/intel_soc_pmic_core.c: >>> >>> static struct pwm_lookup crc_pwm_lookup[] = { >>> PWM_LOOKUP("crystal_cove_pwm", 0, "0000:00:02.0", "pwm_backlight", 0, PWM_POLARITY_NORMAL), >>> }; >>> >>> Should crc_pwm_lookup also use PWM_LOOKUP_WITH_MODULE? And which module >>> exactly? pwm_get() does an automatic request_module(), if the module is >>> given. >>> >>> And will this still be enough? >> >> I was thinking about this a couple of days ago, unfortunately the >> situation with pwm_crc is more complicated as that is part of >> an i2c mfd device, so both the i2c-adapter driver and the mfd driver >> (intel_soc_pmic_crc) need to be builtin currently the mfd driver >> cannot be modular, but the i2c-adapter driver can (and on most >> Linux distribution kernels is) configured to be modular. >> >> So just doing request module for pwm-crc is not going to help >> since the i2c-adapter driver may not yet have loaded / initialized. >> >> All in all we really need to find a way to figure out if we will need >> to do a pwm_get earlier on during i915 initialization (by moving the >> VBT parsing to earlier, or at least part of it) and do the pwm_get >> before we do anything i-reversible and if it fails then return >> -EPROBE_DEFER. Then we can make pwm_crc modular as well as all of >> intel_soc_pmic* as it really should be. > > The other alternatives are: > > 1) Handle defers using a workqueue within i915. It's a bit tedious, but > I didn't spot any show stoppers with the approach. We'd register a > non-functional backlight interface until the pwm_get() succeeds. Ack, although I would prefer for i915 just to properly at deferred probing handling instead of this hack. But if doing the VBT parsing earlier so we know in advance if we will need the pwm or not turns out to be really tricky we could go with this instead. > 2) Rip out pwm backlight from i915 altogether, and turn it into a > separate platform backlight that uses pwm. I think there's ready > infrastructure for that. It's not without problems, though, as then we > lose control over the sequence in which backlight gets enabled/disabled. Yeah, not a fan of this. Regards, Hans
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index f92dd41..12a6cf8 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -249,28 +249,22 @@ config PWM_LPC32XX will be called pwm-lpc32xx. config PWM_LPSS - tristate + bool config PWM_LPSS_PCI - tristate "Intel LPSS PWM PCI driver" + bool "Intel LPSS PWM PCI driver" depends on X86 && PCI select PWM_LPSS help The PCI driver for Intel Low Power Subsystem PWM controller. - To compile this driver as a module, choose M here: the module - will be called pwm-lpss-pci. - config PWM_LPSS_PLATFORM - tristate "Intel LPSS PWM platform driver" + bool "Intel LPSS PWM platform driver" depends on X86 && ACPI select PWM_LPSS help The platform driver for Intel Low Power Subsystem PWM controller. - To compile this driver as a module, choose M here: the module - will be called pwm-lpss-platform. - config PWM_MESON tristate "Amlogic Meson PWM driver" depends on ARCH_MESON