lkml.org 
[lkml]   [2022]   [Apr]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp
    On Fri, 8 Apr 2022 at 20:38, Sankeerth Billakanti
    <sbillaka@qti.qualcomm.com> wrote:
    >
    > > > > > > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
    > > > > > > > > > > <quic_sbillaka@quicinc.com> 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@quicinc.com>
    > > > > > > > > > >
    > > > > > > > > > > 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

    \
     
     \ /
      Last update: 2022-04-08 20:08    [W:2.212 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site