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

[Dmitry Baryshkov]

On Thu, 7 Apr 2022 at 17:05, Sankeerth Billakanti (QUIC)
<quic_sbillaka@xxxxxxxxxxx> wrote:
>
> 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?

>
> 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.

>
> 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?

>
> > > 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.


-- 
With best wishes
Dmitry