Re: [PATCH] drm/edid: Dump the EDID when drm_edid_get_panel_id() has an error

[Doug Anderson]

Hi,

On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
>
> Hi Doug
>
> On 10/24/2022 1:28 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:
> >>
> >> Hi Doug
> >>
> >> On 10/21/2022 1:07 PM, Douglas Anderson wrote:
> >>> If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd
> >>> like to see the EDID that was read so we have a chance of
> >>> understanding what's wrong. There's already a function for that, so
> >>> let's call it in the error case.
> >>>
> >>> NOTE: edid_block_read() has a retry loop in it, so actually we'll only
> >>> print the block read back from the final attempt. This still seems
> >>> better than nothing.
> >>>
> >>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> >>
> >> Instead of checkinf for edid_block_status_valid() on the base_block, do
> >> you want to use drm_edid_block_valid() instead?
> >>
> >> That way you get the edid_block_dump() for free if it was invalid.
> >
> > I can... ...but it feels a bit awkward and maybe not quite how the
> > functions were intended to work together?
> >
> > One thing I notice is that if I call drm_edid_block_valid() I'm doing
> > a bunch of duplicate work that already happened in edid_block_read(),
> > which already calls edid_block_check() and handles fixing headers. I
> > guess also if I call drm_edid_block_valid() then I should ignore the
> > "status" return value of edid_block_read() because we don't need to
> > pass it anywhere (because the work is re-done in
> > drm_edid_block_valid()).
> >
> > So I guess I'm happy to do a v2 like that if everyone likes it better,
> > but to me it feels a little weird.
> >
> > -Doug
>
> Alright, agreed. There is some duplication of code happening if we use
> drm_edid_block_valid(). I had suggested that because it has inherent
> support for dumping the bad EDID.
>
> In that case, this change LGTM, because in principle you are doing the
> same thing as _drm_do_get_edid() (with the only difference being here we
> read only the base block as opposed to the full EDID there).
>
> Hence,
>
> Reviewed-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>

I've given this patch a bunch of time because it wasn't urgent, but
seems like it could be about time to land. I'll plan to land it next
Monday or Tuesday unless anyone has any other comments.

Thanks!

-Doug