Hi Dmitry, > > > > > > > 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(). > > > > 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. > > > > 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. As the panel-power and backlight are panel resources, we are not enabling/voting for them from the DP/eDP controller driver. > > > > > > 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. > -- > With best wishes > Dmitry
