drm/edid: Dump the EDID when drm_edid_get_panel_id() has an error

  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@chromium.org>
---

 drivers/gpu/drm/drm_edid.c | 2 ++
 1 file changed, 2 insertions(+)
  

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@chromium.org>

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.

> ---
> 
>   drivers/gpu/drm/drm_edid.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 47465b9765f1..d63e26ec88b1 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2721,6 +2721,8 @@ u32 drm_edid_get_panel_id(struct i2c_adapter *adapter)
>   
>   	if (edid_block_status_valid(status, edid_block_tag(base_block)))
>   		panel_id = edid_extract_panel_id(base_block);
> +	else
> +		edid_block_dump(KERN_NOTICE, base_block, 0);
>   
>   	kfree(base_block);
>
  

Hi,

On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@quicinc.com> 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@chromium.org>
>
> 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
  

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@quicinc.com> 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@chromium.org>
>>
>> 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@quicinc.com>
  

Hi,

On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com> 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@chromium.org>
> >>
> >> 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@quicinc.com>

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
  

On Fri, 11 Nov 2022, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com> 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@chromium.org>
>> >>
>> >> 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@quicinc.com>
>
> 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.

Ack, it's benign enough.

BR,
Jani.

>
> Thanks!
>
> -Doug
  

Hi,

On Mon, Nov 14, 2022 at 2:02 AM Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
> On Fri, 11 Nov 2022, Doug Anderson <dianders@chromium.org> wrote:
> > Hi,
> >
> > On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@quicinc.com> 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@quicinc.com> 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@chromium.org>
> >> >>
> >> >> 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@quicinc.com>
> >
> > 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.
>
> Ack, it's benign enough.

Thanks! Since you didn't write the above as an Acked-by tag I didn't
add it to the commit, but I went ahead and landed with Abhinav's tag.

69c7717c20cc drm/edid: Dump the EDID when drm_edid_get_panel_id() has an error

-Doug