Re: [PATCH v2] drm/ast: DisplayPort edid supports 256 bytes

[Thomas Zimmermann]

Hi Jammy

Am 18.08.26 um 08:43 schrieb Jammy Huang:
> Hi Thomas,
>
> I'd like to get your thoughts on Jani's point that the kernel should avoid modifying EDID and instead expose the data as-is to userspace.

I agree with Jani that the EDID should best be unmodified. But I've 
found that returning unprocessed EDID makes other components fail. In 
some cases, such as ast, at least the checksum needs to be corrected.

> In practice, most monitors I have encountered only contain two EDID blocks, so simply returning block 0 and block 1 already works for the majority of cases.
>
> Given that, would you agree that it may be acceptable to stop modifying the EDID in the driver and only support the blocks the hardware can actually read now?

AFAIK there's currently no clear way to signal an EOF to the EDID 
parser.  Returning any errno code will make it fail hard.   IMHO that 
next best thing is to zero-out the EDID buffer, so that the parser stops 
silently. (Your v3, I think.) That's also easy enough to revert if/when 
the EDID parser gets a real EOF state.

Best regards
Thomas


> Best regards,
> Jammy
>
> > Hi Jani,
> >
> > Thanks for your input. So, your suggestion is go back to return -EIO for block >
> > 1, and stop trying to modify the EDID.
> > In this way, we can handle EDID whose blocks is less than 2.
> >
> > BR,
> > Jammy
> >
> > > > Hi Thomas,
> > > >
> > > > Good catch - returning 0 without writing to buf wasn't safe. But I
> > > > also found that returning -EIO isn't right either:
> > > > _drm_do_get_edid() treats EDID_BLOCK_READ_FAIL as fatal for any
> > > > block index, not just block 0, so that would have discarded the
> > > > whole EDID for any monitor reporting more than one extension.
> > > >
> > > > In v3, for block > 1 I now zero the buffer explicitly and return 0.
> > > > That makes drm_edid's block checker classify it as EDID_BLOCK_ZERO
> > > > instead, which is non-fatal - it gets trimmed via
> > > > edid_filter_invalid_blocks() (extensions count reduced, checksum
> > > > recomputed), and blocks 0 and 1 still come through fine. This also
> > > > directly addresses your point about the buffer not being guaranteed
> > > > zeroed, since we zero it ourselves now.
> > > There's a long-term goal to *not* modify the EDID in kernel, but
> > > rather return even the broken EDID extensions to userspace, if that's what
> > the display has.
> > > There are probably a bunch of hurdles in making that happen, but
> > > intentionally returning zeroed out EDID blocks is just adding another
> > unnecessary hurdle.
> > > BR,
> > > Jani.
> > >
> > >
> > > --
> > > Jani Nikula, Intel

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)