Re: [PATCH v5 00/10] MediaTek DisplayPort: support eDP and aux-bus

[Chen-Yu Tsai]

On Thu, Jul 13, 2023 at 5:57 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
>
> Il 13/07/23 11:54, Chen-Yu Tsai ha scritto:
> > On Thu, Jul 13, 2023 at 5:01 PM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> >>
> >> Changes in v5:
> >>   - Added .wait_hpd_asserted() callback for aux-bus
> >>   - Avoid enabling and registering HPD interrupt + handlers for
> >>     eDP case only (keeps HPD interrupts enabled for full DP case)
> >>   - Support not always-on eDP panels (boot with regulator off,
> >>     suspend with regulator off) for power saving in PM suspend.
> >
> > This still doesn't work when the DRM driver is builtin, and the panel
> > driver is a module. This is still with regulator-always-on.
> >
> >  From what I can tell from the kernel logs, the DRM driver is not waiting
> > for eDP panel to probe (which sort of makes sense?), and simply uses
> > the default EDID. When the panel does probe, nothing triggers the DRM
> > driver to update its connector.
> >
> > [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:32:eDP-1]
> > [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:32:eDP-1]
> > status updated from unknown to connected
> > [drm:drm_mode_debug_printmodeline] Modeline "640x480": 60 25175 640
> > 656 752 800 480 490 492 525 0x40 0xa
> > [drm:drm_mode_prune_invalid] Not using 640x480 mode: CLOCK_HIGH
> > [drm:drm_mode_debug_printmodeline] Modeline "800x600": 56 36000 800
> > 824 896 1024 600 601 603 625 0x40 0x5
> > [drm:drm_mode_prune_invalid] Not using 800x600 mode: CLOCK_HIGH
> > [drm:drm_mode_debug_printmodeline] Modeline "800x600": 60 40000 800
> > 840 968 1056 600 601 605 628 0x40 0x5
> > [drm:drm_mode_prune_invalid] Not using 800x600 mode: CLOCK_HIGH
> > [drm:drm_mode_debug_printmodeline] Modeline "848x480": 60 33750 848
> > 864 976 1088 480 486 494 517 0x40 0x5
> > [drm:drm_mode_prune_invalid] Not using 848x480 mode: CLOCK_HIGH
> > [drm:drm_mode_debug_printmodeline] Modeline "1024x768": 60 65000 1024
> > 1048 1184 1344 768 771 777 806 0x40 0xa
> > [drm:drm_mode_prune_invalid] Not using 1024x768 mode: CLOCK_HIGH
> > [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:34:DP-1]
> > [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:34:DP-1]
> > status updated from unknown to disconnected
> > [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:34:DP-1] disconnected
> > [drm:drm_client_modeset_probe] No connectors reported connected with modes
> > [drm:drm_client_modeset_probe] connector 32 enabled? yes
> > [drm:drm_client_modeset_probe] connector 34 enabled? no
> > [drm:drm_client_firmware_config.constprop.0.isra.0] Not using firmware
> > configuration
> > [drm:drm_client_modeset_probe] looking for cmdline mode on connector 32
> > [drm:drm_client_modeset_probe] looking for preferred mode on connector 32 0
> > [drm:drm_client_modeset_probe] found mode none
> > [drm:drm_client_modeset_probe] picking CRTCs for 4096x4096 config
> > mediatek-drm mediatek-drm.12.auto:
> > [drm:__drm_fb_helper_initial_config_and_unlock] test CRTC 0 primary
> > plane
> > mediatek-drm mediatek-drm.12.auto: [drm] Cannot find any crtc or sizes
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_dpcd_probe] aux_mtk_dp:
> > 0x00000 AUX -> (ret=  1) 14
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_dpcd_read] aux_mtk_dp:
> > 0x00000 AUX -> (ret= 15) 14 0a 84 41 00 00 01 80 02 00 00 00 0f 09 80
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_dpcd_probe] aux_mtk_dp:
> > 0x00000 AUX -> (ret=  1) 14
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_dpcd_read] aux_mtk_dp:
> > 0x02200 AUX -> (ret= 15) 14 0a 84 41 00 00 01 80 02 00 00 00 0f 01 80
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_read_dpcd_caps]
> > aux_mtk_dp: Base DPCD: 14 0a 84 41 00 00 01 80 02 00 00 00 0f 09 80
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_read_dpcd_caps]
> > aux_mtk_dp: DPCD: 14 0a 84 41 00 00 01 80 02 00 00 00 0f 01 80
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_dpcd_probe] aux_mtk_dp:
> > 0x00000 AUX -> (ret=  1) 14
> > mediatek-drm mediatek-drm.12.auto: [drm:drm_dp_dpcd_read] aux_mtk_dp:
> > 0x00021 AUX -> (ret=  1) 00
> > panel-simple-dp-aux aux-1c500000.edp-tx: Detected BOE NE135FBM-N41 v8.1 (0x095f)
> >
> > If the panel is also built-in, then the eDP panel probe happens between
> > the drm driver adding components and binding to them, and everything seems
> > to work OK.
> >
>
> Argh. I actually forgot to test that case. Sorry about that.
>
> Anyway, you don't need regulator-always-on anymore, nor regulator-boot-on...

Confirmed this case works now.

> ...I'll recheck with panel-edp as module and fix.

With the module case, I also get an devlink error:

mediatek-drm-dp 1c500000.edp-tx: Failed to create device link (0x180)
with regulator-pp3300-disp-x

which doesn't happen with the builtin case. Not sure if it gets a retry
or anything later.

ChenYu

> Cheers,
> Angelo
>
> >
> > ChenYu
> >
> >> Changes in v4:
> >>   - Set data lanes to idle to prevent stalls if bootloader didn't
> >>     properly close the eDP port
> >>   - Now using the .done_probing() callback for AUX bus to prevent
> >>     probe deferral loops in case the panel-edp driver is a module
> >>     as previously seen with another bridge driver (ANX7625) on
> >>     some other SoCs (MT8192 and others)
> >>   - Rebased over next-20230706
> >>   - Dropped Chen-Yu's T-b tag on last patch as some logic changed
> >>     (before, I wasn't using the .done_probing() callback).
> >>
> >> Changes in v3:
> >>   - Added DPTX AUX block initialization before trying to communicate
> >>     to stop relying on the bootloader keeping it initialized before
> >>     booting Linux.
> >>   - Fixed commit description for patch [09/09] and removed commented
> >>     out code (that slipped from dev phase.. sorry!).
> >>
> >> This series adds "real" support for eDP in the mtk-dp DisplayPort driver.
> >>
> >> Explaining the "real":
> >> Before this change, the DisplayPort driver did support eDP to some
> >> extent, but it was treating it entirely like a regular DP interface
> >> which is partially fine, after all, embedded DisplayPort *is* actually
> >> DisplayPort, but there might be some differences to account for... and
> >> this is for both small performance improvements and, more importantly,
> >> for correct functionality in some systems.
> >>
> >> Functionality first:
> >>
> >> One of the common differences found in various boards implementing eDP
> >> and machines using an eDP panel is that many times the HPD line is not
> >> connected. This *must* be accounted for: at startup, this specific IP
> >> will raise a HPD interrupt (which should maybe be ignored... as it does
> >> not appear to be a "real" event...) that will make the eDP panel to be
> >> detected and to actually work but, after a suspend-resume cycle, there
> >> will be no HPD interrupt (as there's no HPD line in my case!) producing
> >> a functionality issue - specifically, the DP Link Training fails because
> >> the panel doesn't get powered up, then it stays black and won't work
> >> until rebooting the machine (or removing and reinserting the module I
> >> think, but I haven't tried that).
> >>
> >> Now for.. both:
> >> eDP panels are *e*DP because they are *not* removable (in the sense that
> >> you can't unplug the cable without disassembling the machine, in which
> >> case, the machine shall be powered down..!): this (correct) assumption
> >> makes us able to solve some issues and to also gain a little performance
> >> during PM operations.
> >>
> >> What was done here is:
> >>   - Caching the EDID if the panel is eDP: we're always going to read the
> >>     same data everytime, so we can just cache that (as it's small enough)
> >>     shortening PM resume times for the eDP driver instance;
> >>   - Always return connector_status_connected if it's eDP: non-removable
> >>     means connector_status_disconnected can't happen during runtime...
> >>     this also saves us some time and even power, as we won't have to
> >>     perform yet another power cycle of the HW;
> >>   - Added aux-bus support!
> >>     This makes us able to rely on panel autodetection from the EDID,
> >>     avoiding to add more and more panel timings to panel-edp and, even
> >>     better, allowing to use one panel node in devicetrees for multiple
> >>     variants of the same machine since, at that point, it's not important
> >>     to "preventively know" what panel we have (eh, it's autodetected...!).
> >>
> >> This was tested on a MT8195 Cherry Tomato Chromebook (panel-edp on aux-bus)
> >>
> >>
> >> P.S.: For your own testing commodity, here's a reference devicetree:
> >>
> >> pp3300_disp_x: regulator-pp3300-disp-x {
> >>          compatible = "regulator-fixed";
> >>          regulator-name = "pp3300_disp_x";
> >>          regulator-min-microvolt = <3300000>;
> >>          regulator-max-microvolt = <3300000>;
> >>          enable-active-high;
> >>          gpio = <&pio 55 GPIO_ACTIVE_HIGH>;
> >>          pinctrl-names = "default";
> >>          pinctrl-0 = <&panel_fixed_pins>;
> >> };
> >>
> >> &edp_tx {
> >>          status = "okay";
> >>
> >>          pinctrl-names = "default";
> >>          pinctrl-0 = <&edptx_pins_default>;
> >>
> >>          ports {
> >>                  #address-cells = <1>;
> >>                  #size-cells = <0>;
> >>
> >>                  port@0 {
> >>                          reg = <0>;
> >>                          edp_in: endpoint {
> >>                                  remote-endpoint = <&dp_intf0_out>;
> >>                          };
> >>                  };
> >>
> >>                  port@1 {
> >>                          reg = <1>;
> >>                          edp_out: endpoint {
> >>                                  data-lanes = <0 1 2 3>;
> >>                                  remote-endpoint = <&panel_in>;
> >>                          };
> >>                  };
> >>          };
> >>
> >>          aux-bus {
> >>                  panel: panel {
> >>                          compatible = "edp-panel";
> >>                          power-supply = <&pp3300_disp_x>;
> >>                          backlight = <&backlight_lcd0>;
> >>                          port {
> >>                                  panel_in: endpoint {
> >>                                          remote-endpoint = <&edp_out>;
> >>                                  };
> >>                          };
> >>                  };
> >>          };
> >> };
> >>
> >> AngeloGioacchino Del Regno (10):
> >>    drm/mediatek: dp: Move AUX and panel poweron/off sequence to function
> >>    drm/mediatek: dp: Change logging to dev for mtk_dp_aux_transfer()
> >>    drm/mediatek: dp: Use devm variant of drm_bridge_add()
> >>    drm/mediatek: dp: Move AUX_P0 setting to
> >>      mtk_dp_initialize_aux_settings()
> >>    drm/mediatek: dp: Enable event interrupt only when bridge attached
> >>    drm/mediatek: dp: Avoid mutex locks if audio is not supported/enabled
> >>    drm/mediatek: dp: Move PHY registration to new function
> >>    drm/mediatek: dp: Add support for embedded DisplayPort aux-bus
> >>    drm/mediatek: dp: Add .wait_hpd_asserted() for AUX bus
> >>    drm/mediatek: dp: Don't register HPD interrupt handler for eDP case
> >>
> >>   drivers/gpu/drm/mediatek/Kconfig  |   1 +
> >>   drivers/gpu/drm/mediatek/mtk_dp.c | 335 ++++++++++++++++++++----------
> >>   2 files changed, 224 insertions(+), 112 deletions(-)
> >>
> >> --
> >> 2.40.1
> >>
> >
>