[PATCH] drm/bridge: anx7625: Fix invalid EDID size

[Jani Nikula]

On Tue, 01 Jul 2025, Loic Poulain <loic.poulain at oss.qualcomm.com> wrote:
> On Mon, Jun 30, 2025 at 10:40 AM Maxime Ripard <mripard at kernel.org> wrote:
>>
>> On Mon, Jun 30, 2025 at 09:46:40AM +0200, Loic Poulain wrote:
>> > Hi Maxime,
>> >
>> > On Mon, Jun 30, 2025 at 9:07 AM Maxime Ripard <mripard at kernel.org> wrote:
>> > > On Sun, Jun 29, 2025 at 04:38:36AM +0200, Loic Poulain wrote:
>> > > > DRM checks EDID block count against allocated size in drm_edid_valid
>> > > > function. We have to allocate the right EDID size instead of the max
>> > > > size to prevent the EDID to be reported as invalid.
>> > > >
>> > > > Fixes: 7c585f9a71aa ("drm/bridge: anx7625: use struct drm_edid more")
>> > > > Signed-off-by: Loic Poulain <loic.poulain at oss.qualcomm.com>
>> > > > ---
>> > > >  drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +-
>> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
>> > > > index 8a9079c2ed5c..5a81d1bfc815 100644
>> > > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
>> > > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
>> > > > @@ -1801,7 +1801,7 @@ static const struct drm_edid *anx7625_edid_read(struct anx7625_data *ctx)
>> > > >               return NULL;
>> > > >       }
>> > > >
>> > > > -     ctx->cached_drm_edid = drm_edid_alloc(edid_buf, FOUR_BLOCK_SIZE);
>> > > > +     ctx->cached_drm_edid = drm_edid_alloc(edid_buf, edid_num * ONE_BLOCK_SIZE);
>> > > >       kfree(edid_buf);
>> > >
>> > > Do we need to cache the whole EDIDs? AFAIU, it's only ever used to get
>> > > the manufacturer name, which fits into a u32 / 4 u8. We should probably
>> > > just cache that.
>> >
>> > While the cached EDID is indeed used internally to retrieve the
>> > product ID, its content is also returned via the DRM read_edid
>> > callback. This value is then used by the DRM core to enumerate
>> > available display modes, and likely also when reading EDID from sysfs.
>>
>> You still don't need to allocate and store a copy of the EDIDs in your
>> driver to implement what you listed so far.
>
> Right, we could change how the driver behaves on callback and just
> cache what we need for internal usage. That change was initially a
> pure fix, do you recommend changing all of this in this change, or in
> a follow-up one.

If there's a follow-up, I really *really* think it should be to rewrite
EDID reading in anx7625.c altogether. The current thing is busted in
more ways than I have time to enumerate right now. And it's not because
I'm in a huge rush. Just look at sp_tx_edid_read() and the functions it
calls.

The end result should be based on providing a straightforward read_block
callback for drm_edid_read_custom().

I've actually started this a few times myself, but it's a bit much for
someone without the hardware to test it, nor skin in the game. The
current code is too complex to trivially refactor.


BR,
Jani.


-- 
Jani Nikula, Intel