| Message ID | 1644396932-17932-1-git-send-email-quic_sbillaka@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support for the eDP panel on sc7280 CRD | expand |
Quoting Sankeerth Billakanti (2022-02-09 00:55:32) > Add support in the DP driver to utilize the custom eDP panels > from drm/panels. > > An eDP panel is always connected to the platform. So, the eDP > connector can be reported as always connected. The display mode > will be sourced from the panel. The panel mode will be set after > the link training is completed. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > > Changes in v3: > None > > drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++ > drivers/gpu/drm/msm/dp/dp_drm.c | 54 +++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/msm/dp/dp_parser.h | 3 +++ > 3 files changed, 60 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 7cc4d21..410fda4 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1513,6 +1513,10 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) > return -EINVAL; > } > > + /* handle eDP on */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_plug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > @@ -1577,6 +1581,10 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder) > > dp_display = container_of(dp, struct dp_display_private, dp_display); > > + /* handle edp off */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_unplug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > index d4d360d..12fa8c1 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -39,6 +39,10 @@ static enum drm_connector_status dp_connector_detect(struct drm_connector *conn, > > dp = to_dp_connector(conn)->dp_display; > > + /* eDP is always connected */ > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + return connector_status_connected; Why not implement different connector ops for eDP and then not implement this function at all in that case? > + > DRM_DEBUG_DP("is_connected = %s\n", > (dp->is_connected) ? "true" : "false"); > > @@ -123,6 +127,35 @@ static enum drm_mode_status dp_connector_mode_valid( > return dp_display_validate_mode(dp_disp, mode->clock); > } > > +static int edp_connector_get_modes(struct drm_connector *connector) > +{ > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + dp = to_dp_connector(connector)->dp_display; > + > + return drm_bridge_get_modes(dp->panel_bridge, connector); > +} > + > +static enum drm_mode_status edp_connector_mode_valid( > + struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + dp = to_dp_connector(connector)->dp_display; > + > + if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ) > + return MODE_BAD; Why not return MODE_CLOCK_HIGH? > + > + return MODE_OK; > +} > + > static const struct drm_connector_funcs dp_connector_funcs = { > .detect = dp_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > @@ -137,6 +170,12 @@ static const struct drm_connector_helper_funcs dp_connector_helper_funcs = { > .mode_valid = dp_connector_mode_valid, > }; > > +static const struct drm_connector_helper_funcs edp_connector_helper_funcs = { > + .get_modes = edp_connector_get_modes, > + .mode_valid = edp_connector_mode_valid, > + Why the extra newline? > +}; > + > /* connector initialization */ > struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > { > @@ -160,12 +199,17 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > if (ret) > return ERR_PTR(ret); > > - drm_connector_helper_add(connector, &dp_connector_helper_funcs); > + if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) { > + drm_connector_helper_add(connector, > + &edp_connector_helper_funcs); > + } else { > + drm_connector_helper_add(connector, &dp_connector_helper_funcs); > > - /* > - * Enable HPD to let hpd event is handled when cable is connected. > - */ > - connector->polled = DRM_CONNECTOR_POLL_HPD; > + /* > + * Enable HPD to let hpd event is handled when cable is connected. > + */ > + connector->polled = DRM_CONNECTOR_POLL_HPD; > + } > > drm_connector_attach_encoder(connector, dp_display->encoder); >
Hi Stephen, Will make the changes. -----Original Message----- From: Stephen Boyd <swboyd@chromium.org> Sent: Thursday, February 10, 2022 6:52 AM To: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; agross@kernel.org; airlied@linux.ie; bjorn.andersson@linaro.org; daniel@ffwll.ch; devicetree@vger.kernel.org; dianders@chromium.org; dri-devel@lists.freedesktop.org; freedreno@lists.freedesktop.org; krzysztof.kozlowski@canonical.com; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; robdclark@gmail.com; robh+dt@kernel.org; sam@ravnborg.org; seanpaul@chromium.org; thierry.reding@gmail.com Cc: quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) <quic_khsieh@quicinc.com>; quic_mkrishn <quic_mkrishn@quicinc.com>; quic_vproddut <quic_vproddut@quicinc.com>; dmitry.baryshkov@linaro.org Subject: Re: [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel Quoting Sankeerth Billakanti (2022-02-09 00:55:32) > Add support in the DP driver to utilize the custom eDP panels from > drm/panels. > > An eDP panel is always connected to the platform. So, the eDP > connector can be reported as always connected. The display mode will > be sourced from the panel. The panel mode will be set after the link > training is completed. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > > Changes in v3: > None > > drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++ > drivers/gpu/drm/msm/dp/dp_drm.c | 54 +++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/msm/dp/dp_parser.h | 3 +++ > 3 files changed, 60 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index 7cc4d21..410fda4 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1513,6 +1513,10 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) > return -EINVAL; > } > > + /* handle eDP on */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_plug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > @@ -1577,6 +1581,10 @@ int msm_dp_display_disable(struct msm_dp *dp, > struct drm_encoder *encoder) > > dp_display = container_of(dp, struct dp_display_private, > dp_display); > > + /* handle edp off */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_unplug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c > b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d..12fa8c1 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -39,6 +39,10 @@ static enum drm_connector_status > dp_connector_detect(struct drm_connector *conn, > > dp = to_dp_connector(conn)->dp_display; > > + /* eDP is always connected */ > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + return connector_status_connected; Why not implement different connector ops for eDP and then not implement this function at all in that case? > + > DRM_DEBUG_DP("is_connected = %s\n", > (dp->is_connected) ? "true" : "false"); > > @@ -123,6 +127,35 @@ static enum drm_mode_status dp_connector_mode_valid( > return dp_display_validate_mode(dp_disp, mode->clock); } > > +static int edp_connector_get_modes(struct drm_connector *connector) { > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + dp = to_dp_connector(connector)->dp_display; > + > + return drm_bridge_get_modes(dp->panel_bridge, connector); } > + > +static enum drm_mode_status edp_connector_mode_valid( > + struct drm_connector *connector, > + struct drm_display_mode *mode) { > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + dp = to_dp_connector(connector)->dp_display; > + > + if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ) > + return MODE_BAD; Why not return MODE_CLOCK_HIGH? > + > + return MODE_OK; > +} > + > static const struct drm_connector_funcs dp_connector_funcs = { > .detect = dp_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > @@ -137,6 +170,12 @@ static const struct drm_connector_helper_funcs dp_connector_helper_funcs = { > .mode_valid = dp_connector_mode_valid, }; > > +static const struct drm_connector_helper_funcs edp_connector_helper_funcs = { > + .get_modes = edp_connector_get_modes, > + .mode_valid = edp_connector_mode_valid, > + Why the extra newline? > +}; > + > /* connector initialization */ > struct drm_connector *dp_drm_connector_init(struct msm_dp > *dp_display) { @@ -160,12 +199,17 @@ struct drm_connector > *dp_drm_connector_init(struct msm_dp *dp_display) > if (ret) > return ERR_PTR(ret); > > - drm_connector_helper_add(connector, &dp_connector_helper_funcs); > + if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) { > + drm_connector_helper_add(connector, > + &edp_connector_helper_funcs); > + } else { > + drm_connector_helper_add(connector, > + &dp_connector_helper_funcs); > > - /* > - * Enable HPD to let hpd event is handled when cable is connected. > - */ > - connector->polled = DRM_CONNECTOR_POLL_HPD; > + /* > + * Enable HPD to let hpd event is handled when cable is connected. > + */ > + connector->polled = DRM_CONNECTOR_POLL_HPD; > + } > > drm_connector_attach_encoder(connector, dp_display->encoder); >