| Message ID | 1648656179-10347-9-git-send-email-quic_sbillaka@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for the eDP panel over aux_bus | expand |
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? > --- > drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 8bafdd0..f9c7d9a 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1003,6 +1003,12 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, > return -EINVAL; > } > > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) { > + if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) > + return MODE_CLOCK_HIGH; > + return MODE_OK; > + } > + > if ((dp->max_pclk_khz <= 0) || > (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || > (mode->clock > dp->max_pclk_khz)) > -- > 2.7.4 >
Hi Dmitry, > 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. > > --- > > drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > > b/drivers/gpu/drm/msm/dp/dp_display.c > > index 8bafdd0..f9c7d9a 100644 > > --- a/drivers/gpu/drm/msm/dp/dp_display.c > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > > @@ -1003,6 +1003,12 @@ enum drm_mode_status > dp_bridge_mode_valid(struct drm_bridge *bridge, > > return -EINVAL; > > } > > > > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) { > > + if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) > > + return MODE_CLOCK_HIGH; > > + return MODE_OK; > > + } > > + > > if ((dp->max_pclk_khz <= 0) || > > (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || > > (mode->clock > dp->max_pclk_khz)) > > -- > > 2.7.4 > > > > > -- > With best wishes > Dmitry
Hi, On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com> wrote: > > Hi Dmitry, > > > 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... -Doug
Hi Doug, > On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC) > <quic_sbillaka@quicinc.com> wrote: > > > > Hi Dmitry, > > > > > 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. So, we need to proceed with the reported mode for eDP. > > -Doug
On Mon, 4 Apr 2022 at 21:21, Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com> wrote: > > Hi Doug, > > > On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC) > > <quic_sbillaka@quicinc.com> wrote: > > > > > > Hi Dmitry, > > > > > > > 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. > 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?
Hi Dmitry, > > > > > 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: 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 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. > > 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. > -- > With best wishes > Dmitry Thank you, Sankeerth
On Thu, 7 Apr 2022 at 17:05, Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com> wrote: > > Hi Dmitry, > > > > > > > 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? > > 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.
Hi Dmitry, > > > > > > > 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(). > > > > 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
On Fri, 8 Apr 2022 at 18:50, Sankeerth Billakanti <sbillaka@qti.qualcomm.com> wrote: > > Hi Dmitry, > > > > > > > > > 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. > > > > > > > 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. > 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.
> > > > > > > > > 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() > > -- > With best wishes > Dmitry
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.
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 8bafdd0..f9c7d9a 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1003,6 +1003,12 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, return -EINVAL; } + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) { + if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) + return MODE_CLOCK_HIGH; + return MODE_OK; + } + if ((dp->max_pclk_khz <= 0) || (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) || (mode->clock > dp->max_pclk_khz))
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> --- drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++++ 1 file changed, 6 insertions(+)