| Message ID | 20210901201934.1084250-1-dianders@chromium.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | eDP: Support probing eDP panels dynamically instead of hardcoding | expand |
Removed most CC: SMTP server protested. On 01.09.2021 22:19, Douglas Anderson wrote: > The goal of this patch series is to move away from hardcoding exact > eDP panels in device tree files. As discussed in the various patches > in this series (I'm not repeating everything here), most eDP panels > are 99% probable and we can get that last 1% by allowing two "power > up" delays to be specified in the device tree file and then using the > panel ID (found in the EDID) to look up additional power sequencing > delays for the panel. > > This patch series is the logical contiunation of a previous patch > series where I proposed solving this problem by adding a > board-specific compatible string [1]. In the discussion that followed > it sounded like people were open to something like the solution > proposed in this new series. > > In version 2 I got rid of the idea that we could have a "fallback" > compatible string that we'd use if we didn't recognize the ID in the > EDID. This simplifies the bindings a lot and the implementation > somewhat. As a result of not having a "fallback", though, I'm not > confident in transitioning any existing boards over to this since > we'll have to fallback to very conservative timings if we don't > recognize the ID from the EDID and I can't guarantee that I've seen > every panel that might have shipped on an existing product. The plan > is to use "edp-panel" only on new boards or new revisions of old > boards where we can guarantee that every EDID that ships out of the > factory has an ID in the table. > > Version 3 of this series now splits out all eDP panels to their own > driver and adds the generic eDP panel support to this new driver. I > believe this is what Sam was looking for [2]. > > [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ > [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ > I like the idea - if something can be configured dynamically lets do it. But I have few questions: 1. Have you read different real panels id's? In many cases (MIPI DSI, LVDS with EDID) manufacturers often forgot to set proper id fields. I do not know how EDID's ids are reliable in case of edp panels. 2. You are working with edp panels - beside EDID they have DPCD which contains things like IEEE_OUI and "Device Identification String", I guess they could be also used for detecting panels, have you considered it? I think DPCD Id should be assigned to EDP-Sink interface, and EDID Id to the actual panel behind it. With this assumption one could consider which timings should be property of which entity. Regards Andrzej
Hi, On Thu, Sep 2, 2021 at 3:10 PM Andrzej Hajda <a.hajda@samsung.com> wrote: > > Removed most CC: SMTP server protested. Yeah, it was because of the dang defconfig patches. My general policy is to send the cover letter to everyone even if not everyone gets CCed on all patches, but it ended up as a lot... Speaking of which, I'd definitely be interested in your take on the defconfig topic: https://lore.kernel.org/r/CAD=FV=WPXAUyuAHb1jKx9F_aw+JGX4MWB3or=Eq5rXoKY=OQMw@mail.gmail.com Do you agree with Olof that I should set the "default" for the new config to be the same as the old config? It doesn't make sense going forward but it helps for anyone with old configs... > On 01.09.2021 22:19, Douglas Anderson wrote: > > The goal of this patch series is to move away from hardcoding exact > > eDP panels in device tree files. As discussed in the various patches > > in this series (I'm not repeating everything here), most eDP panels > > are 99% probable and we can get that last 1% by allowing two "power > > up" delays to be specified in the device tree file and then using the > > panel ID (found in the EDID) to look up additional power sequencing > > delays for the panel. > > > > This patch series is the logical contiunation of a previous patch > > series where I proposed solving this problem by adding a > > board-specific compatible string [1]. In the discussion that followed > > it sounded like people were open to something like the solution > > proposed in this new series. > > > > In version 2 I got rid of the idea that we could have a "fallback" > > compatible string that we'd use if we didn't recognize the ID in the > > EDID. This simplifies the bindings a lot and the implementation > > somewhat. As a result of not having a "fallback", though, I'm not > > confident in transitioning any existing boards over to this since > > we'll have to fallback to very conservative timings if we don't > > recognize the ID from the EDID and I can't guarantee that I've seen > > every panel that might have shipped on an existing product. The plan > > is to use "edp-panel" only on new boards or new revisions of old > > boards where we can guarantee that every EDID that ships out of the > > factory has an ID in the table. > > > > Version 3 of this series now splits out all eDP panels to their own > > driver and adds the generic eDP panel support to this new driver. I > > believe this is what Sam was looking for [2]. > > > > [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ > > [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ > > > I like the idea - if something can be configured dynamically lets do it. > But I have few questions: > 1. Have you read different real panels id's? In many cases (MIPI DSI, > LVDS with EDID) manufacturers often forgot to set proper id fields. I do > not know how EDID's ids are reliable in case of edp panels. I have read several and (so far) they have been quite reliable but I will admit that I haven't done an exhaustive sample. I guess my answer to whether we can trust it is: a) Presumably you'd only use this new "edp-panel" compatible on systems whose panel IDs are known to be reliable. Old systems can keep determining the panel by compatible string. The two schemes can co-exist. b) As we start using this new scheme then there will be more validation of panel ID strings and, presumably, they will become more reliable. It is still true that ID conflicts could exist. A vendor could ship two different panels with the same ID and maybe nobody would notice because they weren't ever hooked up to the same board. In that case we'd have a question of what to do upstream. If this really happens then I suppose (worst case) we could use the device tree to help differentiate and each board could say that "panel ID <x> hooked up to this board is actually panel <y>". I hope we don't have to do that because it feels dirty, but at least it gives us _something_ if we get backed into a corner. > 2. You are working with edp panels - beside EDID they have DPCD which > contains things like IEEE_OUI and "Device Identification String", I > guess they could be also used for detecting panels, have you considered > it? I think DPCD Id should be assigned to EDP-Sink interface, and EDID > Id to the actual panel behind it. With this assumption one could > consider which timings should be property of which entity. This could be another way to help us if we're backed into a corner in the future. Right now the driver requires that you give access to a full eDP AUX bus to use the "generic eDP" panel support even though it currently only relies on the EDID, so it would be pretty easy to utilize this info in the future. So far the ID has been reliable for me though. -Doug
Hi Douglas, On Wed, Sep 01, 2021 at 01:19:18PM -0700, Douglas Anderson wrote: > The goal of this patch series is to move away from hardcoding exact > eDP panels in device tree files. As discussed in the various patches > in this series (I'm not repeating everything here), most eDP panels > are 99% probable and we can get that last 1% by allowing two "power > up" delays to be specified in the device tree file and then using the > panel ID (found in the EDID) to look up additional power sequencing > delays for the panel. > > This patch series is the logical contiunation of a previous patch > series where I proposed solving this problem by adding a > board-specific compatible string [1]. In the discussion that followed > it sounded like people were open to something like the solution > proposed in this new series. > > In version 2 I got rid of the idea that we could have a "fallback" > compatible string that we'd use if we didn't recognize the ID in the > EDID. This simplifies the bindings a lot and the implementation > somewhat. As a result of not having a "fallback", though, I'm not > confident in transitioning any existing boards over to this since > we'll have to fallback to very conservative timings if we don't > recognize the ID from the EDID and I can't guarantee that I've seen > every panel that might have shipped on an existing product. The plan > is to use "edp-panel" only on new boards or new revisions of old > boards where we can guarantee that every EDID that ships out of the > factory has an ID in the table. > > Version 3 of this series now splits out all eDP panels to their own > driver and adds the generic eDP panel support to this new driver. I > believe this is what Sam was looking for [2]. > > [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ > [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ > > Changes in v3: > - Decode hex product ID w/ same endianness as everyone else. > - ("Reorder logicpd_type_28...") patch new for v3. > - Split eDP panels patch new for v3. > - Move wayward panels patch new for v3. > - ("Non-eDP panels don't need "HPD" handling") new for v3. > - Split the delay structure out patch just on eDP now. > - ("Better describe eDP panel delays") new for v3. > - Fix "prepare_to_enable" patch new for v3. > - ("Don't re-read the EDID every time") moved to eDP only patch. > - Generic "edp-panel" handled by the eDP panel driver now. > - Change init order to we power at the end. > - Adjust endianness of product ID. > - Fallback to conservative delays if panel not recognized. > - Add Sharp LQ116M1JW10 to table. > - Add AUO B116XAN06.1 to table. > - Rename delays more generically so they can be reused. > > Changes in v2: > - No longer allow fallback to panel-simple. > - Add "-ms" suffix to delays. > - Don't support a "fallback" panel. Probed panels must be probed. > - Not based on patch to copy "desc"--just allocate for probed panels. > - Add "-ms" suffix to delays. > > Douglas Anderson (16): > dt-bindings: drm/panel-simple-edp: Introduce generic eDP panels > drm/edid: Break out reading block 0 of the EDID > drm/edid: Allow the querying/working with the panel ID from the EDID > drm/panel-simple: Reorder logicpd_type_28 / mitsubishi_aa070mc01 > drm/panel-simple-edp: Split eDP panels out of panel-simple > ARM: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP > arm64: defconfig: Everyone who had PANEL_SIMPLE now gets > PANEL_SIMPLE_EDP > MIPS: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP > drm/panel-simple-edp: Move some wayward panels to the eDP driver > drm/panel-simple: Non-eDP panels don't need "HPD" handling > drm/panel-simple-edp: Split the delay structure out > drm/panel-simple-edp: Better describe eDP panel delays > drm/panel-simple-edp: hpd_reliable shouldn't be subtraced from > hpd_absent > drm/panel-simple-edp: Fix "prepare_to_enable" if panel doesn't handle > HPD > drm/panel-simple-edp: Don't re-read the EDID every time we power off > the panel > drm/panel-simple-edp: Implement generic "edp-panel"s probed by EDID Thanks for looking into this. I really like the outcome. We have panel-simple that now (mostly) handle simple panels, and thus all the eDP functionality is in a separate driver. I have provided a few nits. My only take on this is the naming - as we do not want to confuse panel-simple and panel-edp I strongly suggest renaming the driver to panel-edp. And then rename the corresponding Kconfig entry. With these few changes all patches are: Acked-by: Sam Ravnborg <sam@ravnborg.org> For bisectability I suggest to move the defconfig patches up before you introduce the new Kconfig symbol. Or maybe they will be added via another tree and then this is not possible to control I assume you will apply the patches yourself. Thanks again! Sam
Hi, On Sun, Sep 5, 2021 at 11:55 AM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Douglas, > > On Wed, Sep 01, 2021 at 01:19:18PM -0700, Douglas Anderson wrote: > > The goal of this patch series is to move away from hardcoding exact > > eDP panels in device tree files. As discussed in the various patches > > in this series (I'm not repeating everything here), most eDP panels > > are 99% probable and we can get that last 1% by allowing two "power > > up" delays to be specified in the device tree file and then using the > > panel ID (found in the EDID) to look up additional power sequencing > > delays for the panel. > > > > This patch series is the logical contiunation of a previous patch > > series where I proposed solving this problem by adding a > > board-specific compatible string [1]. In the discussion that followed > > it sounded like people were open to something like the solution > > proposed in this new series. > > > > In version 2 I got rid of the idea that we could have a "fallback" > > compatible string that we'd use if we didn't recognize the ID in the > > EDID. This simplifies the bindings a lot and the implementation > > somewhat. As a result of not having a "fallback", though, I'm not > > confident in transitioning any existing boards over to this since > > we'll have to fallback to very conservative timings if we don't > > recognize the ID from the EDID and I can't guarantee that I've seen > > every panel that might have shipped on an existing product. The plan > > is to use "edp-panel" only on new boards or new revisions of old > > boards where we can guarantee that every EDID that ships out of the > > factory has an ID in the table. > > > > Version 3 of this series now splits out all eDP panels to their own > > driver and adds the generic eDP panel support to this new driver. I > > believe this is what Sam was looking for [2]. > > > > [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ > > [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ > > > > Changes in v3: > > - Decode hex product ID w/ same endianness as everyone else. > > - ("Reorder logicpd_type_28...") patch new for v3. > > - Split eDP panels patch new for v3. > > - Move wayward panels patch new for v3. > > - ("Non-eDP panels don't need "HPD" handling") new for v3. > > - Split the delay structure out patch just on eDP now. > > - ("Better describe eDP panel delays") new for v3. > > - Fix "prepare_to_enable" patch new for v3. > > - ("Don't re-read the EDID every time") moved to eDP only patch. > > - Generic "edp-panel" handled by the eDP panel driver now. > > - Change init order to we power at the end. > > - Adjust endianness of product ID. > > - Fallback to conservative delays if panel not recognized. > > - Add Sharp LQ116M1JW10 to table. > > - Add AUO B116XAN06.1 to table. > > - Rename delays more generically so they can be reused. > > > > Changes in v2: > > - No longer allow fallback to panel-simple. > > - Add "-ms" suffix to delays. > > - Don't support a "fallback" panel. Probed panels must be probed. > > - Not based on patch to copy "desc"--just allocate for probed panels. > > - Add "-ms" suffix to delays. > > > > Douglas Anderson (16): > > dt-bindings: drm/panel-simple-edp: Introduce generic eDP panels > > drm/edid: Break out reading block 0 of the EDID > > drm/edid: Allow the querying/working with the panel ID from the EDID > > drm/panel-simple: Reorder logicpd_type_28 / mitsubishi_aa070mc01 > > drm/panel-simple-edp: Split eDP panels out of panel-simple > > ARM: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP > > arm64: defconfig: Everyone who had PANEL_SIMPLE now gets > > PANEL_SIMPLE_EDP > > MIPS: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP > > drm/panel-simple-edp: Move some wayward panels to the eDP driver > > drm/panel-simple: Non-eDP panels don't need "HPD" handling > > drm/panel-simple-edp: Split the delay structure out > > drm/panel-simple-edp: Better describe eDP panel delays > > drm/panel-simple-edp: hpd_reliable shouldn't be subtraced from > > hpd_absent > > drm/panel-simple-edp: Fix "prepare_to_enable" if panel doesn't handle > > HPD > > drm/panel-simple-edp: Don't re-read the EDID every time we power off > > the panel > > drm/panel-simple-edp: Implement generic "edp-panel"s probed by EDID > > Thanks for looking into this. I really like the outcome. > We have panel-simple that now (mostly) handle simple panels, > and thus all the eDP functionality is in a separate driver. > > I have provided a few nits. > My only take on this is the naming - as we do not want to confuse > panel-simple and panel-edp I strongly suggest renaming the driver to > panel-edp. Sure, I'll do that. I was trying to express the fact that the new "panel-edp" driver won't actually handle _all_ eDP panels, only the eDP panels that are (comparatively) simpler. For instance, I'm not planning to handle panel-samsung-atna33xc20.c in "panel-edp". I guess people will figure it out, though. > And then rename the corresponding Kconfig entry. > > With these few changes all patches are: > Acked-by: Sam Ravnborg <sam@ravnborg.org> Thanks, I'll add it to the patches. If there's anything major I need to change I'll give you a yell to make sure you see it. > For bisectability I suggest to move the defconfig patches up before you > introduce the new Kconfig symbol. Or maybe they will be added via > another tree and then this is not possible to control Yup, I'll do that. There was some question about the defconfig patch but they are hopefully cleared up now. > I assume you will apply the patches yourself. Sure, I can do that with your Ack. I'll also make sure that patches that Jani commented on get resolved. -Doug
The goal of this patch series is to move away from hardcoding exact eDP panels in device tree files. As discussed in the various patches in this series (I'm not repeating everything here), most eDP panels are 99% probable and we can get that last 1% by allowing two "power up" delays to be specified in the device tree file and then using the panel ID (found in the EDID) to look up additional power sequencing delays for the panel. This patch series is the logical contiunation of a previous patch series where I proposed solving this problem by adding a board-specific compatible string [1]. In the discussion that followed it sounded like people were open to something like the solution proposed in this new series. In version 2 I got rid of the idea that we could have a "fallback" compatible string that we'd use if we didn't recognize the ID in the EDID. This simplifies the bindings a lot and the implementation somewhat. As a result of not having a "fallback", though, I'm not confident in transitioning any existing boards over to this since we'll have to fallback to very conservative timings if we don't recognize the ID from the EDID and I can't guarantee that I've seen every panel that might have shipped on an existing product. The plan is to use "edp-panel" only on new boards or new revisions of old boards where we can guarantee that every EDID that ships out of the factory has an ID in the table. Version 3 of this series now splits out all eDP panels to their own driver and adds the generic eDP panel support to this new driver. I believe this is what Sam was looking for [2]. [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ Changes in v3: - Decode hex product ID w/ same endianness as everyone else. - ("Reorder logicpd_type_28...") patch new for v3. - Split eDP panels patch new for v3. - Move wayward panels patch new for v3. - ("Non-eDP panels don't need "HPD" handling") new for v3. - Split the delay structure out patch just on eDP now. - ("Better describe eDP panel delays") new for v3. - Fix "prepare_to_enable" patch new for v3. - ("Don't re-read the EDID every time") moved to eDP only patch. - Generic "edp-panel" handled by the eDP panel driver now. - Change init order to we power at the end. - Adjust endianness of product ID. - Fallback to conservative delays if panel not recognized. - Add Sharp LQ116M1JW10 to table. - Add AUO B116XAN06.1 to table. - Rename delays more generically so they can be reused. Changes in v2: - No longer allow fallback to panel-simple. - Add "-ms" suffix to delays. - Don't support a "fallback" panel. Probed panels must be probed. - Not based on patch to copy "desc"--just allocate for probed panels. - Add "-ms" suffix to delays. Douglas Anderson (16): dt-bindings: drm/panel-simple-edp: Introduce generic eDP panels drm/edid: Break out reading block 0 of the EDID drm/edid: Allow the querying/working with the panel ID from the EDID drm/panel-simple: Reorder logicpd_type_28 / mitsubishi_aa070mc01 drm/panel-simple-edp: Split eDP panels out of panel-simple ARM: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP arm64: defconfig: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP MIPS: configs: Everyone who had PANEL_SIMPLE now gets PANEL_SIMPLE_EDP drm/panel-simple-edp: Move some wayward panels to the eDP driver drm/panel-simple: Non-eDP panels don't need "HPD" handling drm/panel-simple-edp: Split the delay structure out drm/panel-simple-edp: Better describe eDP panel delays drm/panel-simple-edp: hpd_reliable shouldn't be subtraced from hpd_absent drm/panel-simple-edp: Fix "prepare_to_enable" if panel doesn't handle HPD drm/panel-simple-edp: Don't re-read the EDID every time we power off the panel drm/panel-simple-edp: Implement generic "edp-panel"s probed by EDID .../bindings/display/panel/panel-edp.yaml | 188 ++ arch/arm/configs/at91_dt_defconfig | 1 + arch/arm/configs/exynos_defconfig | 1 + arch/arm/configs/imx_v6_v7_defconfig | 1 + arch/arm/configs/lpc32xx_defconfig | 1 + arch/arm/configs/multi_v5_defconfig | 1 + arch/arm/configs/multi_v7_defconfig | 1 + arch/arm/configs/omap2plus_defconfig | 1 + arch/arm/configs/qcom_defconfig | 1 + arch/arm/configs/realview_defconfig | 1 + arch/arm/configs/sama5_defconfig | 1 + arch/arm/configs/shmobile_defconfig | 1 + arch/arm/configs/sunxi_defconfig | 1 + arch/arm/configs/tegra_defconfig | 1 + arch/arm/configs/versatile_defconfig | 1 + arch/arm/configs/vexpress_defconfig | 1 + arch/arm64/configs/defconfig | 1 + arch/mips/configs/qi_lb60_defconfig | 1 + arch/mips/configs/rs90_defconfig | 1 + drivers/gpu/drm/drm_edid.c | 121 +- drivers/gpu/drm/panel/Kconfig | 16 +- drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-simple-edp.c | 1895 +++++++++++++++++ drivers/gpu/drm/panel/panel-simple.c | 1100 +--------- include/drm/drm_edid.h | 47 + 25 files changed, 2293 insertions(+), 1093 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.yaml create mode 100644 drivers/gpu/drm/panel/panel-simple-edp.c