Quoting Bjorn Andersson (2023-05-11 08:44:16) > On Wed, May 10, 2023 at 05:39:07PM -0700, Abhinav Kumar wrote: > > > > > > On 5/10/2023 4:55 PM, Stephen Boyd wrote: > > > Quoting Kuogee Hsieh (2023-05-10 13:31:04) > > > > The internal_hpd flag was introduced to handle external DP HPD derived from GPIO > > > > pinmuxed into DP controller. > > > > > > Was it? It looks more like it was done to differentiate between eDP and > > > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the > > > bridge and we only set the bridge op if the connector type is DP. The > > > assumption looks like if you have DP connector_type, you have the gpio > > > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat > > > that gpio as an irq either, because it isn't. Instead the gpio is muxed > > > to the mdss inside the SoC and then that generates an mdss interrupt > > > that's combined with non-HPD things like "video ready". > > > > > > If that all follows, then I don't quite understand why we're setting > > > internal_hpd to false at all at runtime. It should be set to true at > > > some point, but ideally that point is during probe. > > > > > > > Kuogee had the same thought originally but were not entirely sure of this > > part of the commit message in Bjorn's original commit which introduced these > > changes. > > > > "This difference is not appropriately represented by the "is_edp" > > boolean, but is properly represented by the frameworks invocation of the > > hpd_enable() and hpd_disable() callbacks. Switch the current condition > > to rely on these callbacks instead" > > > > Does this along with below documentation mean we should generate the hpd > > interrupts only after hpd_enable callback happens? > > > > " * Call &drm_bridge_funcs.hpd_enable if implemented and register the given > > @cb > > * and @data as hot plug notification callback. From now on the @cb will be > > * called with @data when an output status change is detected by the bridge, > > * until hot plug notification gets disabled with drm_bridge_hpd_disable(). > > " > > > > Bjorn, can you please clarify this? > > > > We currently have 3 cases: > > 1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false > and internal HPD-logic is in used (internal_hpd = true). Power needs to > be on at all times etc. > > 2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and > internal HPD-logic should not be used/enabled (internal_hpd = false). > Power doesn't need to be on unless hpd_notify is invoked to tell us that > there's something connected... > > 3) eDP with or without HPD signal and/or HPD gpio. Downstream > drm_bridge/panel is connected, is_edp = true and internal HPD logic is > short-circuited regardless of the panel providing HPD signal or not. Oh weird. I thought that the "is_edp" controller on sc7280 didn't have HPD hardware in the PHY (phy@aec2a00), hence all the logic to avoid using the HPD interrupts and bits there. What is "is_edp" about then? > > > In #1 dp_bridge_hpd_enable() will be invoked to indicate that the DP > controller is expected to perform HPD handling. In #2 > dp_bridge_hpd_enable() will _not_ be invoked, instead some downstream > drm_bridge/panel will get the hpd_enable() callback and will be > responsible to updating the HPD state of the chain, which will cause > hpd_notify to be invoked. > > > Note that #3 is based entirely on the controller, it has currently no > relation to what is attached. It seems reasonable that this is just > another case of #2 (perhaps just always reporting > connector_status_connected?). > Looking at drm_bridge_connector_detect() the default is to consider DRM_MODE_CONNECTOR_eDP as connector_status_connected. I wonder if panel_bridge_bridge_funcs can gain a 'detect' function and also set DRM_BRIDGE_OP_DETECT if the connector_type is DRM_MODE_CONNECTOR_eDP.
