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 <[email protected]>
---
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);
--
2.38.0.135.g90850a2211-goog
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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>>
>> 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 <[email protected]>
Hi,
On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
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 <[email protected]> wrote:
> Hi,
>
> On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> >>
>> >> 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 <[email protected]>
>
> 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
--
Jani Nikula, Intel Open Source Graphics Center
Hi,
On Mon, Nov 14, 2022 at 2:02 AM Jani Nikula <[email protected]> wrote:
>
> On Fri, 11 Nov 2022, Doug Anderson <[email protected]> wrote:
> > Hi,
> >
> > On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >> >>
> >> >> 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 <[email protected]>
> >
> > 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