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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 06 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> 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.

I'm concerned about invalidating the data in the structs at the right
times. We keep having issues with that whenever we cache stuff.

BR,
Jani.



>
>> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel