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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4401 bytes --]

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.

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.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel