| Message ID | 20210914202202.1702601-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | eDP: Support probing eDP panels dynamically instead of hardcoding | expand |
On Tue, Sep 14, 2021 at 10:22 PM Douglas Anderson <dianders@chromium.org> wrote: > Version 5 of this series just fixes the panel ID encode macro to be > cleaner and adds Jani's review tags. > > It could possibly be ready to land? Definitely IMO, the kernel look so much better after this change, so for the series: Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi, On Tue, Sep 14, 2021 at 3:12 PM Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Sep 14, 2021 at 10:22 PM Douglas Anderson <dianders@chromium.org> wrote: > > > Version 5 of this series just fixes the panel ID encode macro to be > > cleaner and adds Jani's review tags. > > > > It could possibly be ready to land? > > Definitely IMO, the kernel look so much better after this change, > so for the series: > Acked-by: Linus Walleij <linus.walleij@linaro.org> Pushed all 15 to drm-misc-next. 5540cf8f3e8d drm/panel-edp: Implement generic "edp-panel"s probed by EDID 24e27de11560 drm/panel-edp: Don't re-read the EDID every time we power off the panel a64ad9c3e4a5 drm/panel-edp: Fix "prepare_to_enable" if panel doesn't handle HPD c46a4cc1403e drm/panel-edp: hpd_reliable shouldn't be subtraced from hpd_absent 52824ca4502d drm/panel-edp: Better describe eDP panel delays 9ea10a500045 drm/panel-edp: Split the delay structure out b6d5ffce11dd drm/panel-simple: Non-eDP panels don't need "HPD" handling 3fd68b7b13c2 drm/panel-edp: Move some wayward panels to the eDP driver 5f04e7ce392d drm/panel-edp: Split eDP panels out of panel-simple c0c11c70a6d0 arm64: defconfig: Everyone who had PANEL_SIMPLE now gets PANEL_EDP 310720875efa ARM: configs: Everyone who had PANEL_SIMPLE now gets PANEL_EDP e8de4d55c259 drm/edid: Use new encoded panel id style for quirks matching d9f91a10c3e8 drm/edid: Allow querying/working with the panel ID from the EDID bac9c2948224 drm/edid: Break out reading block 0 of the EDID 29145a566873 dt-bindings: drm/panel-simple-edp: Introduce generic eDP panels -Doug
On Mon, 20 Sep 2021, Doug Anderson <dianders@chromium.org> wrote: > Pushed all 15 to drm-misc-next. ... > e8de4d55c259 drm/edid: Use new encoded panel id style for quirks matching > d9f91a10c3e8 drm/edid: Allow querying/working with the panel ID from the EDID Hi Doug, Stan's reporting "initializer element is not constant" issues here that were discussed before [1]. I wonder what gives, you said you'd hit them on a draft version, but not with what was merged, and I can't reproduce this either. Curious. BR, Jani. In file included from drivers/gpu/drm/drm_edid.c:42:0: ./include/drm/drm_edid.h:525:2: error: initializer element is not constant ((((u32)((vend)[0]) - '@') & 0x1f) << 26 | \ ^ drivers/gpu/drm/drm_edid.c:111:14: note: in expansion of macro ‘drm_edid_encode_panel_id’ .panel_id = drm_edid_encode_panel_id(vend, product_id), \ ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/drm_edid.c:120:2: note: in expansion of macro ‘EDID_QUIRK’ EDID_QUIRK("ACR", 44358, EDID_QUIRK_PREFER_LARGE_60), ^~~~~~~~~~ ./include/drm/drm_edid.h:525:2: note: (near initialization for ‘edid_quirk_list[0].panel_id’) ((((u32)((vend)[0]) - '@') & 0x1f) << 26 | \ ^ drivers/gpu/drm/drm_edid.c:111:14: note: in expansion of macro ‘drm_edid_encode_panel_id’ .panel_id = drm_edid_encode_panel_id(vend, product_id), \ ^~~~~~~~~~~~~~~~~~~~~~~~ drivers/gpu/drm/drm_edid.c:120:2: note: in expansion of macro ‘EDID_QUIRK’ EDID_QUIRK("ACR", 44358, EDID_QUIRK_PREFER_LARGE_60), ^~~~~~~~~~ [1] https://lore.kernel.org/all/CAD=FV=XHvFq5+Rtax7WNq2-BieQr-BM4UnmOcma_eTzkX2ZtNA@mail.gmail.com/
Hi removed most cc, due to server limitation W dniu 24.09.2021 o 10:03, Jani Nikula pisze: > On Mon, 20 Sep 2021, Doug Anderson <dianders@chromium.org> wrote: >> Pushed all 15 to drm-misc-next. > ... >> e8de4d55c259 drm/edid: Use new encoded panel id style for quirks matching >> d9f91a10c3e8 drm/edid: Allow querying/working with the panel ID from >> the EDID > Hi Doug, Stan's reporting "initializer element is not constant" issues > here that were discussed before [1]. I wonder what gives, you said you'd > hit them on a draft version, but not with what was merged, and I can't > reproduce this either. Curious. Apparently this is grey area of unclear specification. gcc version below 8 reports error, above 8.1+ should work [1]. I am not sure if there is nice workaround for older gcc. [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69960#c18 Regards Andrzej > BR, > Jani. > > > In file included from drivers/gpu/drm/drm_edid.c:42:0: > ./include/drm/drm_edid.h:525:2: error: initializer element is not constant > ((((u32)((vend)[0]) - '@') & 0x1f) << 26 | \ > ^ > drivers/gpu/drm/drm_edid.c:111:14: note: in expansion of macro > ‘drm_edid_encode_panel_id’ > .panel_id = drm_edid_encode_panel_id(vend, product_id), \ > ^~~~~~~~~~~~~~~~~~~~~~~~ > drivers/gpu/drm/drm_edid.c:120:2: note: in expansion of macro ‘EDID_QUIRK’ > EDID_QUIRK("ACR", 44358, EDID_QUIRK_PREFER_LARGE_60), > ^~~~~~~~~~ > ./include/drm/drm_edid.h:525:2: note: (near initialization for > ‘edid_quirk_list[0].panel_id’) > ((((u32)((vend)[0]) - '@') & 0x1f) << 26 | \ > ^ > drivers/gpu/drm/drm_edid.c:111:14: note: in expansion of macro > ‘drm_edid_encode_panel_id’ > .panel_id = drm_edid_encode_panel_id(vend, product_id), \ > ^~~~~~~~~~~~~~~~~~~~~~~~ > drivers/gpu/drm/drm_edid.c:120:2: note: in expansion of macro ‘EDID_QUIRK’ > EDID_QUIRK("ACR", 44358, EDID_QUIRK_PREFER_LARGE_60), > ^~~~~~~~~~ > > > [1] > https://lore.kernel.org/all/CAD=FV=XHvFq5+Rtax7WNq2-BieQr-BM4UnmOcma_eTzkX2ZtNA@mail.gmail.com/ > >
Hi, On Fri, Sep 24, 2021 at 2:10 AM Andrzej Hajda <a.hajda@samsung.com> wrote: > > Hi > > removed most cc, due to server limitation > > > W dniu 24.09.2021 o 10:03, Jani Nikula pisze: > > On Mon, 20 Sep 2021, Doug Anderson <dianders@chromium.org> wrote: > >> Pushed all 15 to drm-misc-next. > > ... > >> e8de4d55c259 drm/edid: Use new encoded panel id style for quirks matching > >> d9f91a10c3e8 drm/edid: Allow querying/working with the panel ID from > >> the EDID > > Hi Doug, Stan's reporting "initializer element is not constant" issues > > here that were discussed before [1]. I wonder what gives, you said you'd > > hit them on a draft version, but not with what was merged, and I can't > > reproduce this either. Curious. > > > Apparently this is grey area of unclear specification. > > gcc version below 8 reports error, above 8.1+ should work [1]. I am not > sure if there is nice workaround for older gcc. > > > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69960#c18 So I think the only solution is to go back to the character-at-a-time version. It's slightly uglier but functionality trumps form. I'll post something today though it might need to wait a few hours before I can manage it. -Doug
Hi, On Fri, Sep 24, 2021 at 6:59 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Fri, Sep 24, 2021 at 2:10 AM Andrzej Hajda <a.hajda@samsung.com> wrote: > > > > Hi > > > > removed most cc, due to server limitation > > > > > > W dniu 24.09.2021 o 10:03, Jani Nikula pisze: > > > On Mon, 20 Sep 2021, Doug Anderson <dianders@chromium.org> wrote: > > >> Pushed all 15 to drm-misc-next. > > > ... > > >> e8de4d55c259 drm/edid: Use new encoded panel id style for quirks matching > > >> d9f91a10c3e8 drm/edid: Allow querying/working with the panel ID from > > >> the EDID > > > Hi Doug, Stan's reporting "initializer element is not constant" issues > > > here that were discussed before [1]. I wonder what gives, you said you'd > > > hit them on a draft version, but not with what was merged, and I can't > > > reproduce this either. Curious. > > > > > > Apparently this is grey area of unclear specification. > > > > gcc version below 8 reports error, above 8.1+ should work [1]. I am not > > sure if there is nice workaround for older gcc. > > > > > > [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69960#c18 > > So I think the only solution is to go back to the character-at-a-time > version. It's slightly uglier but functionality trumps form. > > I'll post something today though it might need to wait a few hours > before I can manage it. I managed to squeeze it in before my other obligations: https://lore.kernel.org/all/20210924075317.1.I1e58d74d501613f1fe7585958f451160d11b8a98@changeid I didn't CC everyone here but dri-devel and LKML are copied so hopefully if I missed someone it should still be accessible. -Doug
Hi Douglas, On Tue, Sep 14, 2021 at 10:23 PM Douglas Anderson <dianders@chromium.org> wrote: > A future change wants to be able to read just block 0 of the EDID, so > break it out of drm_do_get_edid() into a sub-function. > > This is intended to be a no-op change--just code movement. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Thanks for your patch, which is now commit bac9c29482248b00 ("drm/edid: Break out reading block 0 of the EDID") in drm-next. > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -1905,6 +1905,44 @@ int drm_add_override_edid_modes(struct drm_connector *connector) > } > EXPORT_SYMBOL(drm_add_override_edid_modes); > > +static struct edid *drm_do_get_edid_base_block( > + int (*get_edid_block)(void *data, u8 *buf, unsigned int block, > + size_t len), > + void *data, bool *edid_corrupt, int *null_edid_counter) > +{ > + int i; > + void *edid; > + > + edid = kmalloc(EDID_LENGTH, GFP_KERNEL); > + if (edid == NULL) > + return NULL; > + > + /* base block fetch */ > + for (i = 0; i < 4; i++) { > + if (get_edid_block(data, edid, 0, EDID_LENGTH)) > + goto out; > + if (drm_edid_block_valid(edid, 0, false, edid_corrupt)) > + break; > + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > + if (null_edid_counter) > + (*null_edid_counter)++; > + goto carp; > + } > + } > + if (i == 4) > + goto carp; > + > + return edid; > + > +carp: > + kfree(edid); > + return ERR_PTR(-EINVAL); > + > +out: > + kfree(edid); > + return NULL; > +} > + > /** > * drm_do_get_edid - get EDID data using a custom EDID block read function > * @connector: connector we're probing > @@ -1938,25 +1976,16 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, > if (override) > return override; > > - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > + edid = (u8 *)drm_do_get_edid_base_block(get_edid_block, data, > + &connector->edid_corrupt, > + &connector->null_edid_counter); > + if (IS_ERR_OR_NULL(edid)) { > + if (IS_ERR(edid)) So edid is an error code, not a valid pointer... > + connector_bad_edid(connector, edid, 1); ... while connector_bad_edid() expects edid to be a valid pointer, causing a crash: Unable to handle kernel NULL pointer dereference at virtual address 0000000000000068 Mem abort info: ESR = 0x96000004 EC = 0x25: DABT (current EL), IL = 32 bits SET = 0, FnV = 0 EA = 0, S1PTW = 0 FSC = 0x04: level 0 translation fault Data abort info: ISV = 0, ISS = 0x00000004 CM = 0, WnR = 0 [0000000000000068] user address but active_mm is swapper Internal error: Oops: 96000004 [#1] PREEMPT SMP CPU: 0 PID: 7 Comm: kworker/u4:0 Not tainted 5.15.0-rc3-arm64-renesas-00629-geb2d42841024-dirty #1313 Hardware name: Renesas Ebisu-4D board based on r8a77990 (DT) Workqueue: events_unbound deferred_probe_work_func pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : connector_bad_edid+0x28/0x1a8 lr : drm_do_get_edid+0x260/0x268 sp : ffff8000121336a0 x29: ffff8000121336a0 x28: ffff00000a373200 x27: 0000000000001ffe PM: ==== always-on/ee160000.mmc: stop x26: 0000000000000005 x25: 0000000000000041 x24: 0000000000000000 x23: ffff000008a25080 x22: ffff8000106bd990 x21: ffff0000081496c0 x20: 0000000000000001 x19: ffffffffffffffea x18: 0000000000000010 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000 x11: 0000000000000000 x10: 0000000000000000 x9 : 0000000000000000 x8 : 0000000000000000 x7 : 0000000000000080 x6 : ffff000009c71000 x5 : 0000000000000000 x4 : 0000000000000069 x3 : ffff00000a3c2900 x2 : 0000000000000001 x1 : ffffffffffffffea x0 : ffff000009c71000 Call trace: connector_bad_edid+0x28/0x1a8 drm_do_get_edid+0x260/0x268 adv7511_get_edid+0xb4/0xd0 adv7511_bridge_get_edid+0x10/0x18 > return NULL; > - > - /* base block fetch */ > - for (i = 0; i < 4; i++) { > - if (get_edid_block(data, edid, 0, EDID_LENGTH)) > - goto out; > - if (drm_edid_block_valid(edid, 0, false, > - &connector->edid_corrupt)) > - break; > - if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { > - connector->null_edid_counter++; > - goto carp; > - } > } > - if (i == 4) > - goto carp; > > - /* if there's no extensions, we're done */ > + /* if there's no extensions or no connector, we're done */ > valid_extensions = edid[0x7e]; > if (valid_extensions == 0) > return (struct edid *)edid; > @@ -2010,8 +2039,6 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, > > return (struct edid *)edid; > > -carp: > - connector_bad_edid(connector, edid, 1); > out: > kfree(edid); > return NULL; Gr{oetje,eeting}s, Geert
Hi, On Mon, Oct 4, 2021 at 8:42 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > > + edid = (u8 *)drm_do_get_edid_base_block(get_edid_block, data, > > + &connector->edid_corrupt, > > + &connector->null_edid_counter); > > + if (IS_ERR_OR_NULL(edid)) { > > + if (IS_ERR(edid)) > > So edid is an error code, not a valid pointer... > > > + connector_bad_edid(connector, edid, 1); > > ... while connector_bad_edid() expects edid to be a valid pointer, > causing a crash: > > Unable to handle kernel NULL pointer dereference at virtual address Sigh. Thanks for the report and analysis. I guess I don't have any displays reporting invalid EDIDs to test with. Hopefully this will help: https://lore.kernel.org/r/20211004092100.1.Ic90a5ebd44c75db963112be167a03cc96f9fb249@changeid/ -Doug
Hi Doug, On Mon, Oct 4, 2021 at 6:26 PM Doug Anderson <dianders@chromium.org> wrote: > On Mon, Oct 4, 2021 at 8:42 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > - if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) > > > + edid = (u8 *)drm_do_get_edid_base_block(get_edid_block, data, > > > + &connector->edid_corrupt, > > > + &connector->null_edid_counter); > > > + if (IS_ERR_OR_NULL(edid)) { > > > + if (IS_ERR(edid)) > > > > So edid is an error code, not a valid pointer... > > > > > + connector_bad_edid(connector, edid, 1); > > > > ... while connector_bad_edid() expects edid to be a valid pointer, > > causing a crash: > > > > Unable to handle kernel NULL pointer dereference at virtual address > > Sigh. Thanks for the report and analysis. I guess I don't have any > displays reporting invalid EDIDs to test with. Hopefully this will > help: It doesn't happen all the time. Looks like my EDID is only invalid after a reset needed to resolve an s2ram crash in the adv7511 driver... > https://lore.kernel.org/r/20211004092100.1.Ic90a5ebd44c75db963112be167a03cc96f9fb249@changeid/ Thanks for the quick fix! Gr{oetje,eeting}s, Geert
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]. Version 4 of this series is mostly small fixes / renames from review feedback. It's largely the same as v3. Other than naming / description / comment changes, the differences are: - Dropped the MIPS config patch as per request. - Reorder config patches first. - Added a new patch to use the panel ID scheme for quirks. - Landed the reorder of logicpd_type_28 / mitsubishi_aa070mc01 Version 5 of this series just fixes the panel ID encode macro to be cleaner and adds Jani's review tags. It could possibly be ready to land? [1] https://lore.kernel.org/r/YFKQaXOmOwYyeqvM@google.com/ [2] https://lore.kernel.org/r/YRTsFNTn%2FT8fLxyB@ravnborg.org/ Changes in v5: - Prettier encode panel ID function (thanks Andrzej!) - Probable => Probeable Changes in v4: - "u8 *edid" => "void *edid" to avoid cast. - ("Use new encoded panel id style for quirks matching") new for v4. - Don't put kmalloc() in the "if" test even if the old code did. - Don't refer to "panel-simple" in commit message. - PANEL_SIMPLE_EDP => PANEL_EDP - Remove "non-eDP" in panel-simple description. - Reordered config patches to be before code patch - decode_edid_id() => drm_edid_decode_panel_id() - drm_do_get_edid_blk0() => drm_do_get_edid_base_block() - drm_get_panel_id() => drm_edid_get_panel_id() - encode_edid_id() => drm_edid_encode_panel_id() - panel-simple-edp => panel-edp - split panel id extraction out to its own function. Changes in v3: - ("Better describe eDP panel delays") new for v3. - ("Don't re-read the EDID every time") moved to eDP only patch. - ("Non-eDP panels don't need "HPD" handling") new for v3. - Add AUO B116XAN06.1 to table. - Add Sharp LQ116M1JW10 to table. - Adjust endianness of product ID. - Change init order to we power at the end. - Decode hex product ID w/ same endianness as everyone else. - Fallback to conservative delays if panel not recognized. - Fix "prepare_to_enable" patch new for v3. - Generic "edp-panel" handled by the eDP panel driver now. - Move wayward panels patch new for v3. - Rename delays more generically so they can be reused. - Split eDP panels patch new for v3. - Split the delay structure out patch just on eDP now. Changes in v2: - Add "-ms" suffix to delays. - Don't support a "fallback" panel. Probed panels must be probed. - No longer allow fallback to panel-simple. - Not based on patch to copy "desc"--just allocate for probed panels. Douglas Anderson (15): dt-bindings: drm/panel-simple-edp: Introduce generic eDP panels drm/edid: Break out reading block 0 of the EDID drm/edid: Allow querying/working with the panel ID from the EDID drm/edid: Use new encoded panel id style for quirks matching ARM: configs: Everyone who had PANEL_SIMPLE now gets PANEL_EDP arm64: defconfig: Everyone who had PANEL_SIMPLE now gets PANEL_EDP drm/panel-edp: Split eDP panels out of panel-simple drm/panel-edp: Move some wayward panels to the eDP driver drm/panel-simple: Non-eDP panels don't need "HPD" handling drm/panel-edp: Split the delay structure out drm/panel-edp: Better describe eDP panel delays drm/panel-edp: hpd_reliable shouldn't be subtraced from hpd_absent drm/panel-edp: Fix "prepare_to_enable" if panel doesn't handle HPD drm/panel-edp: Don't re-read the EDID every time we power off the panel drm/panel-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 + drivers/gpu/drm/drm_edid.c | 280 ++- drivers/gpu/drm/panel/Kconfig | 16 +- drivers/gpu/drm/panel/Makefile | 1 + drivers/gpu/drm/panel/panel-edp.c | 1895 +++++++++++++++++ drivers/gpu/drm/panel/panel-simple.c | 1072 +--------- include/drm/drm_edid.h | 45 + 23 files changed, 2351 insertions(+), 1162 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.yaml create mode 100644 drivers/gpu/drm/panel/panel-edp.c