On Tue, Jul 01, 2025 at 11:56:23AM +0300, Jani Nikula wrote: > On Tue, 01 Jul 2025, Loic Poulain <[email protected]> wrote: > > On Mon, Jun 30, 2025 at 10:40 AM Maxime Ripard <[email protected]> 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 <[email protected]> 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 <[email protected]> > >> > > > --- > >> > > > 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.
Given the extent of Jani's suggested rework, I'm fine with doing it as a follow-up. Maxime
signature.asc
Description: PGP signature
