Message ID | 20240503213441.177109-1-dianders@chromium.org (mailing list archive) |
---|---|
Headers | show |
Series | drm/panel: Remove most store/double-check of prepared/enabled state | expand |
On Fri, May 3, 2024 at 11:36 PM Douglas Anderson <dianders@chromium.org> wrote: > As talked about in commit d2aacaf07395 ("drm/panel: Check for already > prepared/enabled in drm_panel"), we want to remove needless code from > panel drivers that was storing and double-checking the > prepared/enabled state. Even if someone was relying on the > double-check before, that double-check is now in the core and not > needed in individual drivers. > > This series attempts to do just that. While the original grep, AKA: > git grep 'if.*>prepared' -- drivers/gpu/drm/panel > git grep 'if.*>enabled' -- drivers/gpu/drm/panel > ...still produces a few hits after my series, they are _mostly_ all > gone. The ones that are left are less trivial to fix. > > One of the main reasons that many panels probably needed to store and > double-check their prepared/enabled appears to have been to handle > shutdown and/or remove. Panels drivers often wanted to force the power > off for panels in these cases and this was a good reason for the > double-check. > > In response to my V1 series [1] we had much discussion of what to > do. The conclusion was that as long as DRM modeset drivers properly > called drm_atomic_helper_shutdown() that we should be able to remove > the explicit shutdown/remove handling in the panel drivers. Most of > the patches to improve DRM modeset drivers [2] [3] [4] have now > landed. > > In contrast to my V1 series, I broke the V2 series up a lot > more. Since a few of the panel drivers in V1 already landed, we had > fewer total drivers and so we could devote a patch to each panel. > Also, since we were now relying on DRM modeset drivers I felt like we > should split the patches for each panel into two: one that's > definitely safe and one that could be reverted if we found a > problematic DRM modeset driver that we couldn't fix. > > Sorry for the large number of patches. I've set things to mostly just > CC people on the cover letter and the patches that are relevant to > them. I've tried to CC people on the whole series that have shown > interest in this TODO item. > > As patches in this series are reviewed and/or tested they could be > landed. There's really no ordering requirement for the series unless > patches touch the same driver. > > NOTE: this touches _a lot_ of drivers, is repetitive, and is not > really possible to generate automatically. That means it's entirely > possible that my eyes glazed over and I did something wrong. Please > double-check me and don't assume that I got everything perfect, though > I did my best. I have at least confirmed that "allmodconfig" for arm64 > doesn't fall on its face with this series. I haven't done a ton of > other testing. > > [1] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid > [2] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org > [3] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org > [4] https://lore.kernel.org/r/20230921192749.1542462-1-dianders@chromium.org This is the right thing to do, thanks for looking into this! As for the behaviour of .remove() I doubt whether in many cases the original driver authors have even tested this themselves. I would say we should just apply the series as soon as it's non-RFC after the next merge window and see what happens. I doubt it will cause much trouble. The series: Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Mon, May 06, 2024 at 08:52:39AM GMT, Linus Walleij wrote: > On Fri, May 3, 2024 at 11:36 PM Douglas Anderson <dianders@chromium.org> wrote: > > > As talked about in commit d2aacaf07395 ("drm/panel: Check for already > > prepared/enabled in drm_panel"), we want to remove needless code from > > panel drivers that was storing and double-checking the > > prepared/enabled state. Even if someone was relying on the > > double-check before, that double-check is now in the core and not > > needed in individual drivers. > > > > This series attempts to do just that. While the original grep, AKA: > > git grep 'if.*>prepared' -- drivers/gpu/drm/panel > > git grep 'if.*>enabled' -- drivers/gpu/drm/panel > > ...still produces a few hits after my series, they are _mostly_ all > > gone. The ones that are left are less trivial to fix. > > > > One of the main reasons that many panels probably needed to store and > > double-check their prepared/enabled appears to have been to handle > > shutdown and/or remove. Panels drivers often wanted to force the power > > off for panels in these cases and this was a good reason for the > > double-check. > > > > In response to my V1 series [1] we had much discussion of what to > > do. The conclusion was that as long as DRM modeset drivers properly > > called drm_atomic_helper_shutdown() that we should be able to remove > > the explicit shutdown/remove handling in the panel drivers. Most of > > the patches to improve DRM modeset drivers [2] [3] [4] have now > > landed. > > > > In contrast to my V1 series, I broke the V2 series up a lot > > more. Since a few of the panel drivers in V1 already landed, we had > > fewer total drivers and so we could devote a patch to each panel. > > Also, since we were now relying on DRM modeset drivers I felt like we > > should split the patches for each panel into two: one that's > > definitely safe and one that could be reverted if we found a > > problematic DRM modeset driver that we couldn't fix. > > > > Sorry for the large number of patches. I've set things to mostly just > > CC people on the cover letter and the patches that are relevant to > > them. I've tried to CC people on the whole series that have shown > > interest in this TODO item. > > > > As patches in this series are reviewed and/or tested they could be > > landed. There's really no ordering requirement for the series unless > > patches touch the same driver. > > > > NOTE: this touches _a lot_ of drivers, is repetitive, and is not > > really possible to generate automatically. That means it's entirely > > possible that my eyes glazed over and I did something wrong. Please > > double-check me and don't assume that I got everything perfect, though > > I did my best. I have at least confirmed that "allmodconfig" for arm64 > > doesn't fall on its face with this series. I haven't done a ton of > > other testing. > > > > [1] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid > > [2] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org > > [3] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org > > [4] https://lore.kernel.org/r/20230921192749.1542462-1-dianders@chromium.org > > This is the right thing to do, thanks for looking into this! > > As for the behaviour of .remove() I doubt whether in many cases > the original driver authors have even tested this themselves. > I would say we should just apply the series as soon as it's non-RFC > after the next merge window and see what happens. I doubt it > will cause much trouble. In the case of st7703 driver, yes tested, and proper shutdown of the panel is necessary, because lack of it can lead to otherwise inexplainable blinking of the entire screen, when the panel is quickly powered up and re-initialized again (eg. as happens when bootloader has display support). Blinking then only ever stops if the panel is left completely powered off for several minutes. There's a note about this in the controller datasheet, that proper power off is needed to enable dicharge of the panel. Kind regards, o. > The series: > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > Yours, > Linus Walleij
On Fri, 3 May 2024 14:32:41 -0700, Douglas Anderson wrote: > > As talked about in commit d2aacaf07395 ("drm/panel: Check for already > prepared/enabled in drm_panel"), we want to remove needless code from > panel drivers that was storing and double-checking the > prepared/enabled state. Even if someone was relying on the > > [ ... ] Acked-by: Maxime Ripard <mripard@kernel.org> Thanks! Maxime
Hi, On Sun, May 5, 2024 at 11:52 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Fri, May 3, 2024 at 11:36 PM Douglas Anderson <dianders@chromium.org> wrote: > > > As talked about in commit d2aacaf07395 ("drm/panel: Check for already > > prepared/enabled in drm_panel"), we want to remove needless code from > > panel drivers that was storing and double-checking the > > prepared/enabled state. Even if someone was relying on the > > double-check before, that double-check is now in the core and not > > needed in individual drivers. > > > > This series attempts to do just that. While the original grep, AKA: > > git grep 'if.*>prepared' -- drivers/gpu/drm/panel > > git grep 'if.*>enabled' -- drivers/gpu/drm/panel > > ...still produces a few hits after my series, they are _mostly_ all > > gone. The ones that are left are less trivial to fix. > > > > One of the main reasons that many panels probably needed to store and > > double-check their prepared/enabled appears to have been to handle > > shutdown and/or remove. Panels drivers often wanted to force the power > > off for panels in these cases and this was a good reason for the > > double-check. > > > > In response to my V1 series [1] we had much discussion of what to > > do. The conclusion was that as long as DRM modeset drivers properly > > called drm_atomic_helper_shutdown() that we should be able to remove > > the explicit shutdown/remove handling in the panel drivers. Most of > > the patches to improve DRM modeset drivers [2] [3] [4] have now > > landed. > > > > In contrast to my V1 series, I broke the V2 series up a lot > > more. Since a few of the panel drivers in V1 already landed, we had > > fewer total drivers and so we could devote a patch to each panel. > > Also, since we were now relying on DRM modeset drivers I felt like we > > should split the patches for each panel into two: one that's > > definitely safe and one that could be reverted if we found a > > problematic DRM modeset driver that we couldn't fix. > > > > Sorry for the large number of patches. I've set things to mostly just > > CC people on the cover letter and the patches that are relevant to > > them. I've tried to CC people on the whole series that have shown > > interest in this TODO item. > > > > As patches in this series are reviewed and/or tested they could be > > landed. There's really no ordering requirement for the series unless > > patches touch the same driver. > > > > NOTE: this touches _a lot_ of drivers, is repetitive, and is not > > really possible to generate automatically. That means it's entirely > > possible that my eyes glazed over and I did something wrong. Please > > double-check me and don't assume that I got everything perfect, though > > I did my best. I have at least confirmed that "allmodconfig" for arm64 > > doesn't fall on its face with this series. I haven't done a ton of > > other testing. > > > > [1] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid > > [2] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org > > [3] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org > > [4] https://lore.kernel.org/r/20230921192749.1542462-1-dianders@chromium.org > > This is the right thing to do, thanks for looking into this! > > As for the behaviour of .remove() I doubt whether in many cases > the original driver authors have even tested this themselves. Yeah, I'd tend to agree. > I would say we should just apply the series as soon as it's non-RFC It's not actually RFC now, but "RFT" (request for testing). I don't _think_ there's any need to send a version without the RFT tag before landing unless someone really feels strongly about it. > after the next merge window With drm-misc there's not really any specific reason to wait for the merge window to open/close as we can land in drm-misc-next at any time regardless of the merge window. drm-misc-next will simply stop feeding linuxnext for a while. That all being said, I'm happy to delay landing this until after the next -rc1 comes out if people would prefer that. If I don't hear anything, I guess I'll just wait until -rc1 before landing any of these. > and see what happens. I doubt it > will cause much trouble. I can land the whole series if that's what everyone agrees on. As I mentioned above, I'm at least slightly worried that I did something stupid _somewhere_ in this series since no automation was possible and with repetitive tasks like this it's super easy to flub something up. It's _probably_ fine, but I guess I still have the worry in the back of my mind. If folks think I should just apply the whole series then I'm happy to do that. If folks think I should just land parts of the series as they are reviewed/tested I can do that as well. Let me know. If I don't hear anything I'd tend to just land patches that are reviewed/tested. Then after a month or so (hopefully) I'd send out a v2 with anything left. > The series: > Acked-by: Linus Walleij <linus.walleij@linaro.org> Thanks! -Doug
Hi, On Wed, May 8, 2024 at 2:14 PM Doug Anderson <dianders@chromium.org> wrote: > > > This is the right thing to do, thanks for looking into this! > > > > As for the behaviour of .remove() I doubt whether in many cases > > the original driver authors have even tested this themselves. > > Yeah, I'd tend to agree. > > > > I would say we should just apply the series as soon as it's non-RFC > > It's not actually RFC now, but "RFT" (request for testing). I don't > _think_ there's any need to send a version without the RFT tag before > landing unless someone really feels strongly about it. > > > > after the next merge window > > With drm-misc there's not really any specific reason to wait for the > merge window to open/close as we can land in drm-misc-next at any time > regardless of the merge window. drm-misc-next will simply stop feeding > linuxnext for a while. > > That all being said, I'm happy to delay landing this until after the > next -rc1 comes out if people would prefer that. If I don't hear > anything, I guess I'll just wait until -rc1 before landing any of > these. > > > > and see what happens. I doubt it > > will cause much trouble. > > I can land the whole series if that's what everyone agrees on. As I > mentioned above, I'm at least slightly worried that I did something > stupid _somewhere_ in this series since no automation was possible and > with repetitive tasks like this it's super easy to flub something up. > It's _probably_ fine, but I guess I still have the worry in the back > of my mind. > > If folks think I should just apply the whole series then I'm happy to > do that. If folks think I should just land parts of the series as they > are reviewed/tested I can do that as well. Let me know. If I don't > hear anything I'd tend to just land patches that are reviewed/tested. > Then after a month or so (hopefully) I'd send out a v2 with anything > left. Nobody said anything, so I did what I indicated above: 1. I've applied all patches that someone responded to with Linus + Maxime's Acks + any given tags. This includes the st7703 panels which Ondřej replied to the cover letter about but didn't officially get any tags. 2. I also applied patches for panels that I was personally involved with. This includes panel-edp, panel-simple, samsung-atna33xc20, boe-tv101wum-nl6. Anything totally unresponded to I've left unapplied. I'll wait a little while (at least a week) and then plan to send a v2 with anything still outstanding. If someone sends Tested-by/Reviewed-by for some panels in the meantime I'll apply them. Here are the 25 patches applied to drm-misc-next: [01/48] drm/panel: raydium-rm692e5: Stop tracking prepared commit: 598dc42f25cc3060fd350db0f52af1075af3f500 [04/48] drm/panel: boe-tv101wum-nl6: Stop tracking prepared commit: 3c24e31c908eb12e99420ff33b74c01f045253fe [05/48] drm/panel: boe-tv101wum-nl6: Don't call unprepare+disable at shutdown/remove commit: 1985e3512b5a3777f6a18c36e40f3926037120bb [06/48] drm/panel: edp: Stop tracking prepared/enabled commit: 3904f317fd977533f6d7d3c4bfd75e0ac6169bb7 [07/48] drm/panel: edp: Add a comment about unprepare+disable at shutdown/remove commit: ec7629859331fb67dbfb6bcd47f887a402e390ff [08/48] drm/panel: innolux-p079zca: Stop tracking prepared/enabled commit: f9055051292442d52092f17e191cf0a58d23d4ed [09/48] drm/panel: innolux-p079zca: Don't call unprepare+disable at shutdown/remove commit: eeb133ff78476eb1e6e88154dfb75a741e8a034a [12/48] drm/panel: kingdisplay-kd097d04: Stop tracking prepared/enabled commit: 157c1381780a453e06430f8b35bb8c5d439eb8c6 [13/48] drm/panel: kingdisplay-kd097d04: Don't call unprepare+disable at shutdown/remove commit: 68c205ef3c39edce4a3346b8a53fd2b700394a0c [14/48] drm/panel: ltk050h3146w: Stop tracking prepared commit: f124478dd18c519544489caddce78e7c5796a758 [15/48] drm/panel: ltk050h3146w: Don't call unprepare+disable at shutdown/remove commit: b7ca446ecb53205944968617b158f073bcacaedc [16/48] drm/panel: ltk500hd1829: Stop tracking prepared commit: 2b8c19b9d7bc9d03e8c44bd391d21e95c07a2c83 [17/48] drm/panel: ltk500hd1829: Don't call unprepare+disable at shutdown/remove commit: 3357f6f465e62c0bc5e906365063734740c9f6d4 [18/48] drm/panel: novatek-nt36672a: Stop tracking prepared commit: b605f257f386b7f4b6fc9c0f82b86b75d0579287 [19/48] drm/panel: novatek-nt36672a: Don't call unprepare+disable at shutdown/remove commit: 2a9487b5aa55753993fde80e4841128c8da4df71 [24/48] drm/panel: samsung-atna33xc20: Stop tracking prepared/enabled commit: 5a847750aac8454a1604070ab99d689c0a6e4290 [25/48] drm/panel: samsung-atna33xc20: Don't call unprepare+disable at shutdown/remove commit: 49869668ff0e3f380858b4c20b8d0cb02b933f48 [26/48] drm/panel: simple: Stop tracking prepared/enabled commit: 2a1c99d7159b798288bfb20a76c1e665e2344126 [27/48] drm/panel: simple: Add a comment about unprepare+disable at shutdown/remove commit: bc62654df3c888dec735343f5db9907ac93aea60 [30/48] drm/panel: xinpeng-xpp055c272: Stop tracking prepared commit: 4e5e6fa77a9d40cdf85ade7f86d07dc8929941c9 [31/48] drm/panel: xinpeng-xpp055c272: Don't call unprepare+disable at shutdown/remove commit: ac9e1786271f771ff1f774742602330be2d57a12 [42/48] drm/panel: sitronix-st7703: Stop tracking prepared commit: 3004d2e9cca5d59d25dff670a03a005d40601ded [43/48] drm/panel: sitronix-st7703: Don't call disable at shutdown/remove commit: 718bd8a1a5ee873778a72523c06da054a89108b4 [46/48] drm/panel: sony-acx565akm: Don't double-check enabled state in disable commit: e28df86aeeff0b84c13e676f641ea879abbdb809 [47/48] drm/panel: sony-acx565akm: Don't call disable at remove commit: 6afebd850d1ab5518c273b32532f0b2086cc633a -Doug