| Message ID | 93d6c446ed4831dadfb4a77635a67cf5f27e19ff.1715691257.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | None | expand |
On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote: > Prefer the struct drm_edid based functions for reading the EDID and > updating the connector. > > Simplify the flow by updating the EDID property when the EDID is read > instead of at .get_modes. > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- The patch looks good to me, I'd like to hear an opinion from Doug (added to CC). Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> What is the merge strategy for the series? Do you plan to pick up all the patches to drm-misc or should we pick up individual patches into driver trees? > > Cc: Rob Clark <robdclark@gmail.com> > Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Cc: Sean Paul <sean@poorly.run> > Cc: Marijn Suijten <marijn.suijten@somainline.org> > Cc: linux-arm-msm@vger.kernel.org > Cc: freedreno@lists.freedesktop.org > --- > drivers/gpu/drm/msm/dp/dp_display.c | 11 +++---- > drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++-------------------- > drivers/gpu/drm/msm/dp/dp_panel.h | 2 +- > 3 files changed, 20 insertions(+), 40 deletions(-) [skipped] > @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel) > panel = container_of(dp_panel, struct dp_panel_private, dp_panel); > > if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) { > + /* FIXME: get rid of drm_edid_raw() */ The code here can get use of something like drm_edid_smth_checksum(). 'Something', because I could not come up with the word that would make it clear that it is the declared checksum instead of the actual / computed one. > + const struct edid *edid = drm_edid_raw(dp_panel->drm_edid); > u8 checksum; > > - if (dp_panel->edid) > - checksum = dp_panel_get_edid_checksum(dp_panel->edid); > + if (edid) > + checksum = dp_panel_get_edid_checksum(edid); > else > checksum = dp_panel->connector->real_edid_checksum; >
On Sun, 19 May 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote: >> Prefer the struct drm_edid based functions for reading the EDID and >> updating the connector. >> >> Simplify the flow by updating the EDID property when the EDID is read >> instead of at .get_modes. >> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> >> >> --- > > The patch looks good to me, I'd like to hear an opinion from Doug (added > to CC). > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Thanks! > What is the merge strategy for the series? Do you plan to pick up all > the patches to drm-misc or should we pick up individual patches into > driver trees? I think all of the patches here are connected in theme, but independent. Either way is fine by me. > > >> >> Cc: Rob Clark <robdclark@gmail.com> >> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> Cc: Sean Paul <sean@poorly.run> >> Cc: Marijn Suijten <marijn.suijten@somainline.org> >> Cc: linux-arm-msm@vger.kernel.org >> Cc: freedreno@lists.freedesktop.org >> --- >> drivers/gpu/drm/msm/dp/dp_display.c | 11 +++---- >> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++-------------------- >> drivers/gpu/drm/msm/dp/dp_panel.h | 2 +- >> 3 files changed, 20 insertions(+), 40 deletions(-) > > [skipped] > >> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel) >> panel = container_of(dp_panel, struct dp_panel_private, dp_panel); >> >> if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) { >> + /* FIXME: get rid of drm_edid_raw() */ > > The code here can get use of something like drm_edid_smth_checksum(). > 'Something', because I could not come up with the word that would make > it clear that it is the declared checksum instead of the actual / > computed one. This is an annoying one, to be honest, and linked to the historical fact that we filter some EDID blocks that have an incorrect checksum. (Some blocks, yes. We don't filter all blocks, because there are some nasty docks out there that modify the CTA block but fail to update the checksum, and filtering the CTA blocks would render the display useless. So we accept CTA blocks with incorrect checksums. But reject others. Yay.) IMO the real fix would be to stop mucking with the EDID, and just expose it to userspace, warts and all. We could still ignore the EDID blocks with incorrect checksum while using it ourselves if we want to. And with that, we could just have a function that checks the last EDID block's checksum, and stop using this ->real_edid_checksum thing. Anyway, yes, we could add the function already. BR, Jani. > >> + const struct edid *edid = drm_edid_raw(dp_panel->drm_edid); >> u8 checksum; >> >> - if (dp_panel->edid) >> - checksum = dp_panel_get_edid_checksum(dp_panel->edid); >> + if (edid) >> + checksum = dp_panel_get_edid_checksum(edid); >> else >> checksum = dp_panel->connector->real_edid_checksum; >>
On Mon, 20 May 2024 at 15:25, Jani Nikula <jani.nikula@intel.com> wrote: > > On Sun, 19 May 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote: > >> Prefer the struct drm_edid based functions for reading the EDID and > >> updating the connector. > >> > >> Simplify the flow by updating the EDID property when the EDID is read > >> instead of at .get_modes. > >> > >> Signed-off-by: Jani Nikula <jani.nikula@intel.com> > >> > >> --- > > > > The patch looks good to me, I'd like to hear an opinion from Doug (added > > to CC). > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Thanks! > > > What is the merge strategy for the series? Do you plan to pick up all > > the patches to drm-misc or should we pick up individual patches into > > driver trees? > > I think all of the patches here are connected in theme, but > independent. Either way is fine by me. > > > > > > >> > >> Cc: Rob Clark <robdclark@gmail.com> > >> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> Cc: Sean Paul <sean@poorly.run> > >> Cc: Marijn Suijten <marijn.suijten@somainline.org> > >> Cc: linux-arm-msm@vger.kernel.org > >> Cc: freedreno@lists.freedesktop.org > >> --- > >> drivers/gpu/drm/msm/dp/dp_display.c | 11 +++---- > >> drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++-------------------- > >> drivers/gpu/drm/msm/dp/dp_panel.h | 2 +- > >> 3 files changed, 20 insertions(+), 40 deletions(-) > > > > [skipped] > > > >> @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel) > >> panel = container_of(dp_panel, struct dp_panel_private, dp_panel); > >> > >> if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) { > >> + /* FIXME: get rid of drm_edid_raw() */ > > > > The code here can get use of something like drm_edid_smth_checksum(). > > 'Something', because I could not come up with the word that would make > > it clear that it is the declared checksum instead of the actual / > > computed one. > > This is an annoying one, to be honest, and linked to the historical fact > that we filter some EDID blocks that have an incorrect checksum. It is a part of the DP test suite if I remember correctly. > > (Some blocks, yes. We don't filter all blocks, because there are some > nasty docks out there that modify the CTA block but fail to update the > checksum, and filtering the CTA blocks would render the display > useless. So we accept CTA blocks with incorrect checksums. But reject > others. Yay.) > > IMO the real fix would be to stop mucking with the EDID, and just expose > it to userspace, warts and all. We could still ignore the EDID blocks > with incorrect checksum while using it ourselves if we want to. And with > that, we could just have a function that checks the last EDID block's > checksum, and stop using this ->real_edid_checksum thing. > > Anyway, yes, we could add the function already. > > BR, > Jani. > > > > >> + const struct edid *edid = drm_edid_raw(dp_panel->drm_edid); > >> u8 checksum; > >> > >> - if (dp_panel->edid) > >> - checksum = dp_panel_get_edid_checksum(dp_panel->edid); > >> + if (edid) > >> + checksum = dp_panel_get_edid_checksum(edid); > >> else > >> checksum = dp_panel->connector->real_edid_checksum; > >> > > -- > Jani Nikula, Intel
Hi, On Sun, May 19, 2024 at 2:01 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, May 14, 2024 at 03:55:14PM +0300, Jani Nikula wrote: > > Prefer the struct drm_edid based functions for reading the EDID and > > updating the connector. > > > > Simplify the flow by updating the EDID property when the EDID is read > > instead of at .get_modes. > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > > > --- > > The patch looks good to me, I'd like to hear an opinion from Doug (added > to CC). > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > What is the merge strategy for the series? Do you plan to pick up all > the patches to drm-misc or should we pick up individual patches into > driver trees? I'm not sure I have too much to add here aside from what you guys have already talked about. I'm OK with: Reviewed-by: Douglas Anderson <dianders@chromium.org>
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 672a7ba52eda..9622e58dce3e 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -360,26 +360,25 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp, static int dp_display_process_hpd_high(struct dp_display_private *dp) { + struct drm_connector *connector = dp->dp_display.connector; + const struct drm_display_info *info = &connector->display_info; int rc = 0; - struct edid *edid; - rc = dp_panel_read_sink_caps(dp->panel, dp->dp_display.connector); + rc = dp_panel_read_sink_caps(dp->panel, connector); if (rc) goto end; dp_link_process_request(dp->link); if (!dp->dp_display.is_edp) - drm_dp_set_subconnector_property(dp->dp_display.connector, + drm_dp_set_subconnector_property(connector, connector_status_connected, dp->panel->dpcd, dp->panel->downstream_ports); - edid = dp->panel->edid; - dp->dp_display.psr_supported = dp->panel->psr_cap.version && psr_enabled; - dp->audio_supported = drm_detect_monitor_audio(edid); + dp->audio_supported = info->has_audio; dp_panel_handle_sink_request(dp->panel); /* diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c index 07db8f37cd06..a916b5f3b317 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.c +++ b/drivers/gpu/drm/msm/dp/dp_panel.c @@ -108,28 +108,6 @@ static u32 dp_panel_get_supported_bpp(struct dp_panel *dp_panel, return bpp; } -static int dp_panel_update_modes(struct drm_connector *connector, - struct edid *edid) -{ - int rc = 0; - - if (edid) { - rc = drm_connector_update_edid_property(connector, edid); - if (rc) { - DRM_ERROR("failed to update edid property %d\n", rc); - return rc; - } - rc = drm_add_edid_modes(connector, edid); - return rc; - } - - rc = drm_connector_update_edid_property(connector, NULL); - if (rc) - DRM_ERROR("failed to update edid property %d\n", rc); - - return rc; -} - int dp_panel_read_sink_caps(struct dp_panel *dp_panel, struct drm_connector *connector) { @@ -175,12 +153,13 @@ int dp_panel_read_sink_caps(struct dp_panel *dp_panel, if (rc) return rc; - kfree(dp_panel->edid); - dp_panel->edid = NULL; + drm_edid_free(dp_panel->drm_edid); + + dp_panel->drm_edid = drm_edid_read_ddc(connector, &panel->aux->ddc); + + drm_edid_connector_update(connector, dp_panel->drm_edid); - dp_panel->edid = drm_get_edid(connector, - &panel->aux->ddc); - if (!dp_panel->edid) { + if (!dp_panel->drm_edid) { DRM_ERROR("panel edid read failed\n"); /* check edid read fail is due to unplug */ if (!dp_catalog_link_is_connected(panel->catalog)) { @@ -224,13 +203,13 @@ int dp_panel_get_modes(struct dp_panel *dp_panel, return -EINVAL; } - if (dp_panel->edid) - return dp_panel_update_modes(connector, dp_panel->edid); + if (dp_panel->drm_edid) + return drm_edid_connector_add_modes(connector); return 0; } -static u8 dp_panel_get_edid_checksum(struct edid *edid) +static u8 dp_panel_get_edid_checksum(const struct edid *edid) { edid += edid->extensions; @@ -249,10 +228,12 @@ void dp_panel_handle_sink_request(struct dp_panel *dp_panel) panel = container_of(dp_panel, struct dp_panel_private, dp_panel); if (panel->link->sink_request & DP_TEST_LINK_EDID_READ) { + /* FIXME: get rid of drm_edid_raw() */ + const struct edid *edid = drm_edid_raw(dp_panel->drm_edid); u8 checksum; - if (dp_panel->edid) - checksum = dp_panel_get_edid_checksum(dp_panel->edid); + if (edid) + checksum = dp_panel_get_edid_checksum(edid); else checksum = dp_panel->connector->real_edid_checksum; @@ -539,5 +520,5 @@ void dp_panel_put(struct dp_panel *dp_panel) if (!dp_panel) return; - kfree(dp_panel->edid); + drm_edid_free(dp_panel->drm_edid); } diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h index 4ea42fa936ae..6722e3923fa5 100644 --- a/drivers/gpu/drm/msm/dp/dp_panel.h +++ b/drivers/gpu/drm/msm/dp/dp_panel.h @@ -39,7 +39,7 @@ struct dp_panel { u8 downstream_ports[DP_MAX_DOWNSTREAM_PORTS]; struct dp_link_info link_info; - struct edid *edid; + const struct drm_edid *drm_edid; struct drm_connector *connector; struct dp_display_mode dp_mode; struct dp_panel_psr psr_cap;
Prefer the struct drm_edid based functions for reading the EDID and updating the connector. Simplify the flow by updating the EDID property when the EDID is read instead of at .get_modes. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- Cc: Rob Clark <robdclark@gmail.com> Cc: Abhinav Kumar <quic_abhinavk@quicinc.com> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: Sean Paul <sean@poorly.run> Cc: Marijn Suijten <marijn.suijten@somainline.org> Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org --- drivers/gpu/drm/msm/dp/dp_display.c | 11 +++---- drivers/gpu/drm/msm/dp/dp_panel.c | 47 +++++++++-------------------- drivers/gpu/drm/msm/dp/dp_panel.h | 2 +- 3 files changed, 20 insertions(+), 40 deletions(-)