On Fri, 8 Apr 2022 at 20:38, Sankeerth Billakanti <sbillaka@xxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti > > > > > > > > > > <quic_sbillaka@xxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > > > > > The panel-edp driver modes needs to be validated > > > > > > > > > > > differently from DP because the link capabilities are > > > > > > > > > > > not available for EDP by > > > > > > that time. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Sankeerth Billakanti > > > > > > > > > > > <quic_sbillaka@xxxxxxxxxxx> > > > > > > > > > > > > > > > > > > > > This should not be necessary after > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://patchwork.freedesktop.org/patch/479261/?series=101682&rev=1. > > > > > > > > > > Could you please check? > > > > > > > > > > > > > > > > > > > > > > > > > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary > > > > > > > > > anymore but > > > > > > we > > > > > > > > > need to return early for eDP because unlike DP, eDP > > > > > > > > > context will not have the information about the number of > > > > > > > > > lanes and link > > > > clock. > > > > > > > > > > > > > > > > > > So, I will modify the patch to return after the > > > > > > > > > DP_MAX_PIXEL_CLK_KHZ > > > > > > > > check if is_eDP is set. > > > > > > > > > > > > > > > > I haven't walked through all the relevant code but something > > > > > > > > you said above sounds strange. You say that for eDP we don't > > > > > > > > have info about the number of lanes? We _should_. > > > > > > > > > > > > > > > > It's certainly possible to have a panel that supports > > > > > > > > _either_ 1 or > > > > > > > > 2 lanes but then only physically connect 1 lane to it. ...or > > > > > > > > you could have a panel that supports 2 or 4 lanes and you > > > > > > > > only connect 1 > > > > lane. > > > > > > > > See, for instance, ti_sn_bridge_parse_lanes. There we assume > > > > > > > > 4 lanes but if a "data-lanes" property is present then we > > > > > > > > can use that to know that fewer lanes are physically connected. > > > > > > > > > > > > > > > > It's also possible to connect more lanes to a panel than it supports. > > > > > > > > You could connect 2 lanes to it but then it only supports 1. > > > > > > > > This case needs to be handled as well... > > > > > > > > > > > > > > > > > > > > > > I was referring to the checks we do for DP in > > > > > > > dp_bridge_mode_valid. We check if the Link bandwidth can > > > > > > > support the pixel bandwidth. For an external DP connection, > > > > > > > the Initial DPCD/EDID read after cable connection will return > > > > > > > the sink capabilities like link rate, lane count and bpp > > > > > > > information that are used to we filter out the unsupported > > > > > > modes from the list of modes from EDID. > > > > > > > > > > > > > > For eDP case, the dp driver performs the first dpcd read > > > > > > > during bridge_enable. The dp_bridge_mode_valid function is > > > > > > > executed before bridge_enable and hence does not have the full > > > > > > > link or the sink capabilities information like external DP connection, > > by then. > > > > > > > > > > > > It sounds to me like we should emulate the HPD event for eDP to > > > > > > be handled earlier than the get_modes()/prepare() calls are > > attempted. > > > > > > However this might open another can of worms. > > > > > > > > > > > > > > > > For DP, the HPD handler mainly initiates link training and gets the EDID. > > > > > > > > > > Before adding support for a separate eDP panel, we had discussed > > > > > about this internally and decided to emulate eDP HPD during > > > > > enable(). Main reason being, eDP power is guaranteed to be on only > > > > > after > > > > bridge_enable(). > > > > > So, eDP link training can happen and sustain only after bridge_enable(). > > > > > > > > > > Emulating HPD before/during get_modes will not have any effect > > because: > > > > > > > > As we have seen, the term HPD is significantly overloaded. What do > > > > you want to emulate? > > > > > > > > > > On DP, we use HPD event for link training and EDID read. > > > > > > I understood that you wanted me to emulate HPD event before > > > get_modes() but because the panel power is controlled by panel-edp, > > > whatever programming we do on the sink side will be reset when panel > > > power will be turned off by the pm_runtime_put_autosuspend() of the > > panel-edp in panel_edp_get_modes(). > > > > The pm_runtime_put_autosuspend() wouldn't suspend the device > > immediately. It will be suspended after the grace period finished, if nobody > > resumes the devices again. This is how it works in the > > sn65dsi86 driver. It sets the timeout delay long enough, so that get_modes > > and pre_enable would typically work together without suspending the host. > > > > Okay. We are not implementing a bridge pre_enable currently > > > > > > > > > > > > > > 1. get_modes() will go to panel's get_modes() function to power on > > > > > read EDID > > > > > > > > > > 2. panel power will be turned off after get_modes(). Panel power > > > > > off will reset every write transaction in DPCD. anyway > > > > > invalidating link training > > > > > > > > I tend to agree with Doug here. eDP link power status should be > > > > handled by the pm_runtime_autosuspend with grace period being high > > > > enough to cover the timeslot between get_mode() and enable(). > > > > > > > > panel-edp already does most of required work. > > > > > > > > > > The eDP controller resources are enabled through the host_init() and > > > the link resources need to be powered on for doing link training, > > > which needs to happen in the enable() with generic panel-edp. > > > > nothing wrong with that up to now > > > > > > > > > > > > > > > 3. mode_valid will land in dp driver but panel will not be powered > > > > > on at that time and we cannot do aux transfers or DPCD read writes. > > > > > > > > Why do we need to perform AUX writes in mode_valid? > > > > > > > > > > I am trying to justify why we cannot have mode_valid() implementation > > similar to DP for eDP. > > > The detect() and get_modes() are called from panel bridge and panel- > > edp.c respectively. > > > The first eDP specific call which will land in the dp_driver is > > > mode_valid(), in which the controller cannot perform aux access because > > the panel will not be powered-on. > > > > I fail to understand why you'd like to perform aux access from mode_valid at > > all. > > I don't want to perform it in mode_valid. I am just saying that, apart from mode_valid(), > there is no other eDP call (other than aux_transfer) which will land in the DP driver before the mode_set(). > So, currently there is no function in which we can get the eDP sink capabilities before enable(). > So, we assume the mode will be supported if the pixel clock is less than the max clock of 675MHz. > > > > > > As the panel-power and backlight are panel resources, we are not > > > enabling/voting for them from the DP/eDP controller driver. > > > > correct > > > > > > > > > > > > > > > > > So, we need to proceed with the reported mode for eDP. > > > > > > > > > > > > Well... Even if during the first call to get_modes() the DPCD is > > > > > > not read, during subsequent calls the driver has necessary > > > > > > information, so it can proceed with all the checks, can't it? > > > > > > > > > > > > > > > > get_modes() currently does not land in DP driver. It gets executed > > > > > in panel bridge. But the mode_valid() comes to DP driver to check > > > > > the controller compatibility. > > > > > > > > Yes, this is correct. the DP's mode_valid() knows the hardware > > > > limitations (max clock speed, amount of lanes, etc) and thus it can > > > > decide whether the mode is supported by the whole chain or not. > > > > We should not skip such checks for the eDP. > > > > > > > > > > > > > > For eDP, we have no information about the number of lanes or the link > > > rate supported We only know the max lanes from the data-lanes DT > > property. > > > > If the device connects just a single line to the eDP panel, the DT will be > > changed to list that single lane. > > It looks like we have to call dp_panel_read_sink_caps() somewhere for the > > eDP case. For the DP case the HPD callbacks do this work. > > > > No, mode_valid doesn't look like a proper place. We already have read > > modes, so the AUX bus has been powered for some time. We could do it > > earlier. > > Correct, we have to do it earlier. But is there some function in which we can get > the dp_panel_read_sink_caps() before get_modes? > > A way could be to implement the mode_valid also in panel-eDP along with the > get_modes. We can read the sink capabilities in get_modes in panel-edp.c and > check in the mode_valid() in panel-edp.c. > > I also feel the mode_valid() needs to check if a controller can support it rather > than the panel. So, I cannot find a way where to get the sink caps now before > the mode_set() or enable() Anywhere after you have the reference to the next_bridge, you can be sure that the panel is present. So you can power up the AUX bus, read the caps, and (auto-)suspend it again. -- With best wishes Dmitry
