On Sun, 19 May 2024, Dmitry Baryshkov <[email protected]> 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 <[email protected]> >> >> --- > > The patch looks good to me, I'd like to hear an opinion from Doug (added > to CC). > > Reviewed-by: Dmitry Baryshkov <[email protected]>
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 <[email protected]> >> Cc: Abhinav Kumar <[email protected]> >> Cc: Dmitry Baryshkov <[email protected]> >> Cc: Sean Paul <[email protected]> >> Cc: Marijn Suijten <[email protected]> >> Cc: [email protected] >> Cc: [email protected] >> --- >> 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; >> -- Jani Nikula, Intel
