[PATCH v2 2/3] drm/edid: Implement SCDC support detection

[Daniel Vetter]

On Mon, Dec 05, 2016 at 12:11:46PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 09:16:27AM +0100, Daniel Vetter wrote:
> > On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> > > On Sat, Dec 03, 2016 at 04:35:24AM +0000, Sharma, Shashank wrote:
> > > > Hi Thierry, 
> > > > 
> > > > If you can please have a look on this patch, I had written one to parse HF-VSDB, which was covering SCDC detection too. 
> > > > https://patchwork.kernel.org/patch/9452259/ 
> > > 
> > > I think there had been pushback before about caching capabilities from
> > > EDID, so from that point of view my patch is more inline with existing
> > > EDID parsing API.
> > 
> > Hm, where was that pushback? We do have a bit a mess between explicitly
> > parsing stuff (e.g. eld) and stuffing parsed data into drm_display_info.
> 
> You did object to a very similar patch some time ago that did a similar
> thing for DPCD stuff. And also Villa had commented on an earlier patch
> from Jose about concerns of bloating core structures:
> 
> 	https://patchwork.freedesktop.org/patch/104806/

DPCD I complained about because somehow we ended up with 2 sets of
helpers, one filling a struct and the others returning directly. I
objected to the fact that there's 2 (and imo your patch duplicated even
more), not that I think one approach is clearly inferior to the other.

Demanding that there's some real users is also a valid point.

> > I think long-term stuffing it into drm_display_info is probably better,
> > since then we only have 1 interaction point between the probe code and the
> > atomic_check code. That should be useful for eventually fixing the lack of
> > locking between the two, if I ever get around to that ;-)
> 
> I don't really have objections to caching the results of parsing, it's
> what I had proposed and what seemed most natural back when I was working
> on the DPCD helpers. But if we now agree that this is the preferred way
> to do things, then we should at least agree that it applies to all areas
> for the sake of consistency.
> 
> Also, it might be worth looking into improving the structures, and maybe
> adding new ones to order things more conveniently or at least group them
> in some logical way. In my opinion some of our data structures are
> becoming somewhat... unwieldy.

We're pretty good at consuming fairly invasive refactorings in drm-misc.
So it just boils down to get some agreement on what things should look
like (+1 from my side to parsing stuff into structs as a general idea),
and then massaging all the existing users of the "wrong" interface using
cocci and sed.

Unfortunately "just" ;-)

> > > Other than that the patches are mostly equivalent, except yours parses
> > > more information than just the SCDC bits.
> > 
> > So merge patch 1 from your series + Shashank's parsing patch? Everyone
> > agrees and can you pls cross-r-b stamp so I can apply it all?
> 
> I think I spotted a mistake in Shashank's parsing patch. Let me take
> another look.
> 
> If we can agree on a common way forward on how to deal with this kind of
> static data, I have no objections to caching data for the duration of a
> hotplug period.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch