On Mon, 20 May 2024 at 15:25, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
>
> On Sun, 19 May 2024, Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> 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@xxxxxxxxx>
> >>
> >> ---
> >
> > The patch looks good to me, I'd like to hear an opinion from Doug (added
> > to CC).
> >
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> >> Cc: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> >> Cc: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >> Cc: Sean Paul <sean@xxxxxxxxxx>
> >> Cc: Marijn Suijten <marijn.suijten@xxxxxxxxxxxxxx>
> >> Cc: linux-arm-msm@xxxxxxxxxxxxxxx
> >> Cc: freedreno@xxxxxxxxxxxxxxxxxxxxx
> >> ---
> >> 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
--
With best wishes
Dmitry
