Re: [PATCH 1/3] drm/edid: parse multiple CEA extension block

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 22, 2022 at 11:19:15AM +0200, Jani Nikula wrote:
> On Tue, 22 Feb 2022, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Feb 22, 2022 at 02:38:17PM +0800, Lee Shawn C wrote:
> >> Try to find and parse more CEA ext blocks if edid->extensions
> >> is greater than one.
> >> 
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> Signed-off-by: Lee Shawn C <shawn.c.lee@intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_edid.c | 75 +++++++++++++++++++++++---------------
> >>  1 file changed, 45 insertions(+), 30 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> index 12893e7be89b..3d5dbbeca7f9 100644
> >> --- a/drivers/gpu/drm/drm_edid.c
> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> @@ -4313,43 +4313,58 @@ add_cea_modes(struct drm_connector *connector, struct edid *edid)
> >>  	const u8 *cea = drm_find_cea_extension(edid);
> >>  	const u8 *db, *hdmi = NULL, *video = NULL;
> >>  	u8 dbl, hdmi_len, video_len = 0;
> >> -	int modes = 0;
> >> +	int modes = 0, j;
> >>  
> >> -	if (cea && cea_revision(cea) >= 3) {
> >> -		int i, start, end;
> >> +	if (!cea)
> >> +		return 0;
> >>  
> >> -		if (cea_db_offsets(cea, &start, &end))
> >> -			return 0;
> >> +	for (j = (cea - (u8 *)edid) / EDID_LENGTH; j <= edid->extensions;) {
> >
> > That looks rather illegible. I think we want a
> > drm_find_cea_extension(const struct edid *edid, int *ext_index)
> > and then just loop until it stops giving us stuff.
> 
> Neither approach takes multiple CEA blocks within DisplayID extension
> into account. Or some blocks outside and some inside DisplayID
> extension.
> 
> I think we're going to need abstracted EDID iteration similar to what
> I've done for DisplayID iteration. We can't have all places
> reimplementing the iteration like we have now.

Aye. We need so many layers of iteration in various places
that the whole thing is starting to resemble a Russian doll.
Following a common form should probably make that a lot more
manageable.

I've been already thinking about introducing an iterator for
the cea db stuff. But the EDID ext blocks is definitely another
target we need to look at.

And someone is going to have to figure out what are all the
ways these need to nest. I suppose the high level code
should only have to care about the deepest layer of stuff
and the iterators should take care to iterate through all
the potential containers? Eg. if the high level code wants
to look at cea dbs then it just iterates those and 
shouldn't have to care at all where they are stored.

-- 
Ville Syrjälä
Intel