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

[Sharma, Shashank]

Thanks Thierry and Daniel for concluding this discussion, while I was on vacation. 

Here are the next steps, which I am planning to go for (Please correct me if my understanding is not right)
-  Shashank to review first of the SCDC patch, and give comments or R-B. 
-  Shashank to work on Thierry's review comments on HF-VSDB parsing patch (including creation of a sub-class nested drm_hdmi_info within drm_display_info) and publish V2
- Thierry/Daniel to review V2 and give comments or R-B

Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, December 6, 2016 1:49 PM
To: Thierry Reding <thierry.reding at gmail.com>
Cc: Daniel Vetter <daniel at ffwll.ch>; Sharma, Shashank <shashank.sharma at intel.com>; Jose Abreu <joabreu at synopsys.com>; dri-devel at lists.freedesktop.org
Subject: Re: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

On Mon, Dec 05, 2016 at 06:11:44PM +0100, Thierry Reding wrote:
> On Mon, Dec 05, 2016 at 05:21:24PM +0100, Daniel Vetter wrote:
> > 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.
> 
> My recollection is that I had proposed that I do the work of 
> transitioning users of the parsers to the cached information but you 
> had said that it wasn't worth the churn and that we should just go 
> with the existing scheme of passing around the DPCD buffer and 
> extending the parsers as necessary.

Hm, I guess it wasn't clear to me that you've offered to do all the conversions. Doing that would be awesome I think (but quite a bit of work), and if we bother with it, parsing into a struct is imo the better idea long-term.

> From that I inferred that the same would be true for EDID and since we 
> already have a couple of helpers that operate on struct edid * and 
> which return features, continuing that scheme was preferred.
> 
> Anyway, I don't really care either way. Maybe you and Ville can duke 
> it out whether or not we want all of the fields parsed, irrespective 
> of whether or not they will be used. Then I'll go with whatever you decide.
> 
> > 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" ;-)
> 
> I think by now it would be useful to have a nested structure within 
> struct drm_display_info that contains HDMI specific bits. We already 
> have a number of candidates that could be extracted into such a 
> structure (drm_detect_hdmi_monitor(), drm_detect_monitor_audio(), 
> drm_rgb_quant_range_selectable(), ...).
> 
> Another possibility would be to subclass struct drm_display_info, as
> in:
> 
> 	struct drm_hdmi_info {
> 		struct drm_display_info display;
> 
> 		/* HDMI specific information */
> 		...
> 	};
> 
> Or yet another would be to create struct drm_hdmi_info as a separate 
> structure and provide a helper which will extract the necessary info 
> from the EDID. Drivers could then store that in driver-private data 
> whereas struct drm_display_info could be reduced to the generic bits 
> that it used to have.

I think nested drm_hdmi_info within drm_display_info sounds like a fine idea.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch