Re: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp

[Dmitry Baryshkov]

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