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

2016-12-19 Thread 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.vet...@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, December 6, 2016 1:49 PM
To: Thierry Reding 
Cc: Daniel Vetter ; Sharma, Shashank ; Jose Abreu ; 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 +, 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

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

2016-12-07 Thread Jani Nikula 
On Tue, 06 Dec 2016, Daniel Vetter  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 +, 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

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

2016-12-06 Thread Daniel Vetter 
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 +, 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

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

2016-12-05 Thread Thierry Reding 
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 +, 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.

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

2016-12-05 Thread 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 +, 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

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

2016-12-05 Thread Ville Syrjälä 
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 +, 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/

Yeah, the same old "adding stuff all over without actual users" story.
It's near impossible to judge whether the added things are actually
useful (or if the implementation is the best) without seeing how it's
going to be used.

-- 
Ville Syrjälä
Intel OTC

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

2016-12-05 Thread Thierry Reding 
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 +, 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/

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

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

Thierry

> > > -Original Message-
> > > From: Thierry Reding [mailto:thierry.reding at gmail.com] 
> > > Sent: Saturday, December 3, 2016 12:54 AM
> > > To: dri-devel at lists.freedesktop.org
> > > Cc: Jani Nikula ; Sharma, Shashank 
> > > ; Ville Syrjälä  > > linux.intel.com>; Jose Abreu 
> > > Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection
> > > 
> > > From: Thierry Reding 
> > > 
> > > Sinks compliant with the HDMI 2.0 specification may support SCDC, a 
> > > mechanism for the source and the sink to communicate. Sinks advertise 
> > > support for this feature in the HDMI Forum Vendor Specific Data Block as 
> > > defined in the HDMI 2.0 specification, section 10.4 "Status and Control 
> > > Data Channel". Implement a small helper that find the HF-VSDB and parses 
> > > it to check if the sink supports SCDC.
> > > 
> > > Signed-off-by: Thierry Reding 
> > > ---
> > > Changes in v2:
> > > - rename internal function to cea_db_is_hdmi_forum_vsdb() for more
> > >   clarity (Ville Syrjälä)
> > > 
> > >  drivers/gpu/drm/drm_edid.c | 49 
> > > ++
> > >  include/drm/drm_edid.h |  1 +
> > >  include/linux/hdmi.h   |  1 +
> > >  3 files changed, 51 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c 
> > > index 336be31ff3de..369961597ee5 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> > >   return hdmi_id == HDMI_IEEE_OUI;
> > >  }
> > >  
> > > +static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
> > > + unsigned int oui;
> > > +
> > > + if (cea_db_tag(db) != VENDOR_BLOCK)
> > > + return false;
> > > +
> > > + if (cea_db_payload_len(db) < 7)
> > > + return false;
> > > +
> > > + oui = db[3] << 16 | db[2] << 8 | db[1];
> > > +
> > > + return oui == HDMI_FORUM_IEEE_OUI;
> > > +}
> > > +
> > >  #define for_each_cea_db(cea, i, s

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

2016-12-05 Thread Daniel Vetter 
On Mon, Dec 05, 2016 at 08:57:43AM +0100, Thierry Reding wrote:
> On Sat, Dec 03, 2016 at 04:35:24AM +, 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.
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 ;-)

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

Thanks, Daniel

> 
> Thierry
> 
> > -Original Message-
> > From: Thierry Reding [mailto:thierry.reding at gmail.com] 
> > Sent: Saturday, December 3, 2016 12:54 AM
> > To: dri-devel at lists.freedesktop.org
> > Cc: Jani Nikula ; Sharma, Shashank 
> > ; Ville Syrjälä  > linux.intel.com>; Jose Abreu 
> > Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection
> > 
> > From: Thierry Reding 
> > 
> > Sinks compliant with the HDMI 2.0 specification may support SCDC, a 
> > mechanism for the source and the sink to communicate. Sinks advertise 
> > support for this feature in the HDMI Forum Vendor Specific Data Block as 
> > defined in the HDMI 2.0 specification, section 10.4 "Status and Control 
> > Data Channel". Implement a small helper that find the HF-VSDB and parses it 
> > to check if the sink supports SCDC.
> > 
> > Signed-off-by: Thierry Reding 
> > ---
> > Changes in v2:
> > - rename internal function to cea_db_is_hdmi_forum_vsdb() for more
> >   clarity (Ville Syrjälä)
> > 
> >  drivers/gpu/drm/drm_edid.c | 49 
> > ++
> >  include/drm/drm_edid.h |  1 +
> >  include/linux/hdmi.h   |  1 +
> >  3 files changed, 51 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 
> > 336be31ff3de..369961597ee5 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
> > return hdmi_id == HDMI_IEEE_OUI;
> >  }
> >  
> > +static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
> > +   unsigned int oui;
> > +
> > +   if (cea_db_tag(db) != VENDOR_BLOCK)
> > +   return false;
> > +
> > +   if (cea_db_payload_len(db) < 7)
> > +   return false;
> > +
> > +   oui = db[3] << 16 | db[2] << 8 | db[1];
> > +
> > +   return oui == HDMI_FORUM_IEEE_OUI;
> > +}
> > +
> >  #define for_each_cea_db(cea, i, start, end) \
> > for ((i) = (start); (i) < (end) && (i) + 
> > cea_db_payload_len(&(cea)[(i)]) < (end); (i) += 
> > cea_db_payload_len(&(cea)[(i)]) + 1)
> >  
> > @@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  
> > EXPORT_SYMBOL(drm_detect_hdmi_monitor);
> >  
> >  /**
> > + * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
> > + * @edid: sink EDID information
> > + *
> > + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> > + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> > + * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
> > + *
> > + * Returns:
> > + * True if the sink supports SCDC, false otherwise.
> > + */
> > +bool drm_detect_hdmi_scdc(struct edid *edid) {
> > +   unsigned int start, end, i;
> > +   const u8 *cea;
> > +
> > +   cea = drm_find_cea_extension(edid);
> > +   if (!cea)
> > +   return false;
> > +
> > +   if (cea_db_offsets(cea, , ))
> > +   return false;
> > +
> > +   for_each_cea_db(cea, i, start, end) {
> > +   if (cea_db_is_hdmi_forum_vsdb([i])) {
> > +   if (cea[i + 6] & 0x80)
> > +   return true;
> > +

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

2016-12-05 Thread Thierry Reding 
On Sat, Dec 03, 2016 at 04:35:24AM +, 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.

Other than that the patches are mostly equivalent, except yours parses
more information than just the SCDC bits.

Thierry

> -Original Message-
> From: Thierry Reding [mailto:thierry.reding at gmail.com] 
> Sent: Saturday, December 3, 2016 12:54 AM
> To: dri-devel at lists.freedesktop.org
> Cc: Jani Nikula ; Sharma, Shashank 
> ; Ville Syrjälä  linux.intel.com>; Jose Abreu 
> Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection
> 
> From: Thierry Reding 
> 
> Sinks compliant with the HDMI 2.0 specification may support SCDC, a mechanism 
> for the source and the sink to communicate. Sinks advertise support for this 
> feature in the HDMI Forum Vendor Specific Data Block as defined in the HDMI 
> 2.0 specification, section 10.4 "Status and Control Data Channel". Implement 
> a small helper that find the HF-VSDB and parses it to check if the sink 
> supports SCDC.
> 
> Signed-off-by: Thierry Reding 
> ---
> Changes in v2:
> - rename internal function to cea_db_is_hdmi_forum_vsdb() for more
>   clarity (Ville Syrjälä)
> 
>  drivers/gpu/drm/drm_edid.c | 49 
> ++
>  include/drm/drm_edid.h |  1 +
>  include/linux/hdmi.h   |  1 +
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 
> 336be31ff3de..369961597ee5 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
>   return hdmi_id == HDMI_IEEE_OUI;
>  }
>  
> +static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
> + unsigned int oui;
> +
> + if (cea_db_tag(db) != VENDOR_BLOCK)
> + return false;
> +
> + if (cea_db_payload_len(db) < 7)
> + return false;
> +
> + oui = db[3] << 16 | db[2] << 8 | db[1];
> +
> + return oui == HDMI_FORUM_IEEE_OUI;
> +}
> +
>  #define for_each_cea_db(cea, i, start, end) \
>   for ((i) = (start); (i) < (end) && (i) + 
> cea_db_payload_len(&(cea)[(i)]) < (end); (i) += 
> cea_db_payload_len(&(cea)[(i)]) + 1)
>  
> @@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  
> EXPORT_SYMBOL(drm_detect_hdmi_monitor);
>  
>  /**
> + * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
> + * @edid: sink EDID information
> + *
> + * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
> + * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
> + * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
> + *
> + * Returns:
> + * True if the sink supports SCDC, false otherwise.
> + */
> +bool drm_detect_hdmi_scdc(struct edid *edid) {
> + unsigned int start, end, i;
> + const u8 *cea;
> +
> + cea = drm_find_cea_extension(edid);
> + if (!cea)
> + return false;
> +
> + if (cea_db_offsets(cea, , ))
> + return false;
> +
> + for_each_cea_db(cea, i, start, end) {
> + if (cea_db_is_hdmi_forum_vsdb([i])) {
> + if (cea[i + 6] & 0x80)
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL(drm_detect_hdmi_scdc);
> +
> +/**
>   * drm_detect_monitor_audio - check monitor audio capability
>   * @edid: EDID block to scan
>   *
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 
> 38eabf65f19d..7ea7e90846d8 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, 
> struct edid *edid);
>  u8 drm_match_cea_mode(const struct drm_display_mode *to_match);  enum 
> hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);  bool 
> drm_detect_hdmi_monitor(struct edid *edid);
> +bool drm_detect_hdmi_scdc(struct edid *edid);
>  bool drm_detect_monitor_audio(struct edid *edid);  bool 
> drm_rgb_quant_range_selectable(struct edid *edid);  int 
> drm_add_modes_noedid(struct drm_connector *connector, diff --git 
> a/include/linux/hdmi.h b/include/linux/hdmi.h index 
> edbb4fc674ed..d271ff23984f 100644
> --- a/include/linux/hdmi.h
> +++ b/i

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

2016-12-03 Thread Sharma, Shashank 
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/ 

Regards
Shashank
-Original Message-
From: Thierry Reding [mailto:thierry.red...@gmail.com] 
Sent: Saturday, December 3, 2016 12:54 AM
To: dri-devel at lists.freedesktop.org
Cc: Jani Nikula ; Sharma, Shashank 
; Ville Syrjälä ; Jose Abreu 
Subject: [PATCH v2 2/3] drm/edid: Implement SCDC support detection

From: Thierry Reding <tred...@nvidia.com>

Sinks compliant with the HDMI 2.0 specification may support SCDC, a mechanism 
for the source and the sink to communicate. Sinks advertise support for this 
feature in the HDMI Forum Vendor Specific Data Block as defined in the HDMI 2.0 
specification, section 10.4 "Status and Control Data Channel". Implement a 
small helper that find the HF-VSDB and parses it to check if the sink supports 
SCDC.

Signed-off-by: Thierry Reding 
---
Changes in v2:
- rename internal function to cea_db_is_hdmi_forum_vsdb() for more
  clarity (Ville Syrjälä)

 drivers/gpu/drm/drm_edid.c | 49 ++
 include/drm/drm_edid.h |  1 +
 include/linux/hdmi.h   |  1 +
 3 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 
336be31ff3de..369961597ee5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
return hdmi_id == HDMI_IEEE_OUI;
 }

+static bool cea_db_is_hdmi_forum_vsdb(const u8 *db) {
+   unsigned int oui;
+
+   if (cea_db_tag(db) != VENDOR_BLOCK)
+   return false;
+
+   if (cea_db_payload_len(db) < 7)
+   return false;
+
+   oui = db[3] << 16 | db[2] << 8 | db[1];
+
+   return oui == HDMI_FORUM_IEEE_OUI;
+}
+
 #define for_each_cea_db(cea, i, start, end) \
for ((i) = (start); (i) < (end) && (i) + 
cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) 
+ 1)

@@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)  
EXPORT_SYMBOL(drm_detect_hdmi_monitor);

 /**
+ * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
+ * @edid: sink EDID information
+ *
+ * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
+ * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
+ * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
+ *
+ * Returns:
+ * True if the sink supports SCDC, false otherwise.
+ */
+bool drm_detect_hdmi_scdc(struct edid *edid) {
+   unsigned int start, end, i;
+   const u8 *cea;
+
+   cea = drm_find_cea_extension(edid);
+   if (!cea)
+   return false;
+
+   if (cea_db_offsets(cea, , ))
+   return false;
+
+   for_each_cea_db(cea, i, start, end) {
+   if (cea_db_is_hdmi_forum_vsdb([i])) {
+   if (cea[i + 6] & 0x80)
+   return true;
+   }
+   }
+
+   return false;
+}
+EXPORT_SYMBOL(drm_detect_hdmi_scdc);
+
+/**
  * drm_detect_monitor_audio - check monitor audio capability
  * @edid: EDID block to scan
  *
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h index 
38eabf65f19d..7ea7e90846d8 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, 
struct edid *edid);
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);  enum 
hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);  bool 
drm_detect_hdmi_monitor(struct edid *edid);
+bool drm_detect_hdmi_scdc(struct edid *edid);
 bool drm_detect_monitor_audio(struct edid *edid);  bool 
drm_rgb_quant_range_selectable(struct edid *edid);  int 
drm_add_modes_noedid(struct drm_connector *connector, diff --git 
a/include/linux/hdmi.h b/include/linux/hdmi.h index edbb4fc674ed..d271ff23984f 
100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -35,6 +35,7 @@ enum hdmi_infoframe_type {  };

 #define HDMI_IEEE_OUI 0x000c03
+#define HDMI_FORUM_IEEE_OUI 0xc45dd8
 #define HDMI_INFOFRAME_HEADER_SIZE  4
 #define HDMI_AVI_INFOFRAME_SIZE13
 #define HDMI_SPD_INFOFRAME_SIZE25
--
2.10.2

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

2016-12-02 Thread Thierry Reding 
From: Thierry Reding 

Sinks compliant with the HDMI 2.0 specification may support SCDC, a
mechanism for the source and the sink to communicate. Sinks advertise
support for this feature in the HDMI Forum Vendor Specific Data Block
as defined in the HDMI 2.0 specification, section 10.4 "Status and
Control Data Channel". Implement a small helper that find the HF-VSDB
and parses it to check if the sink supports SCDC.

Signed-off-by: Thierry Reding 
---
Changes in v2:
- rename internal function to cea_db_is_hdmi_forum_vsdb() for more
  clarity (Ville Syrjälä)

 drivers/gpu/drm/drm_edid.c | 49 ++
 include/drm/drm_edid.h |  1 +
 include/linux/hdmi.h   |  1 +
 3 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 336be31ff3de..369961597ee5 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3238,6 +3238,21 @@ static bool cea_db_is_hdmi_vsdb(const u8 *db)
return hdmi_id == HDMI_IEEE_OUI;
 }

+static bool cea_db_is_hdmi_forum_vsdb(const u8 *db)
+{
+   unsigned int oui;
+
+   if (cea_db_tag(db) != VENDOR_BLOCK)
+   return false;
+
+   if (cea_db_payload_len(db) < 7)
+   return false;
+
+   oui = db[3] << 16 | db[2] << 8 | db[1];
+
+   return oui == HDMI_FORUM_IEEE_OUI;
+}
+
 #define for_each_cea_db(cea, i, start, end) \
for ((i) = (start); (i) < (end) && (i) + 
cea_db_payload_len(&(cea)[(i)]) < (end); (i) += cea_db_payload_len(&(cea)[(i)]) 
+ 1)

@@ -3687,6 +3702,40 @@ bool drm_detect_hdmi_monitor(struct edid *edid)
 EXPORT_SYMBOL(drm_detect_hdmi_monitor);

 /**
+ * drm_detect_hdmi_scdc - detect whether an HDMI sink supports SCDC
+ * @edid: sink EDID information
+ *
+ * Parse the CEA extension according to CEA-861-B to find an HF-VSDB as
+ * defined in HDMI 2.0, section 10.3.2 "HDMI Forum Vendor Specific Data
+ * Block" and checks if the SCDC_Present bit (bit 7 of byte 6) is set.
+ *
+ * Returns:
+ * True if the sink supports SCDC, false otherwise.
+ */
+bool drm_detect_hdmi_scdc(struct edid *edid)
+{
+   unsigned int start, end, i;
+   const u8 *cea;
+
+   cea = drm_find_cea_extension(edid);
+   if (!cea)
+   return false;
+
+   if (cea_db_offsets(cea, , ))
+   return false;
+
+   for_each_cea_db(cea, i, start, end) {
+   if (cea_db_is_hdmi_forum_vsdb([i])) {
+   if (cea[i + 6] & 0x80)
+   return true;
+   }
+   }
+
+   return false;
+}
+EXPORT_SYMBOL(drm_detect_hdmi_scdc);
+
+/**
  * drm_detect_monitor_audio - check monitor audio capability
  * @edid: EDID block to scan
  *
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 38eabf65f19d..7ea7e90846d8 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -440,6 +440,7 @@ int drm_add_edid_modes(struct drm_connector *connector, 
struct edid *edid);
 u8 drm_match_cea_mode(const struct drm_display_mode *to_match);
 enum hdmi_picture_aspect drm_get_cea_aspect_ratio(const u8 video_code);
 bool drm_detect_hdmi_monitor(struct edid *edid);
+bool drm_detect_hdmi_scdc(struct edid *edid);
 bool drm_detect_monitor_audio(struct edid *edid);
 bool drm_rgb_quant_range_selectable(struct edid *edid);
 int drm_add_modes_noedid(struct drm_connector *connector,
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index edbb4fc674ed..d271ff23984f 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -35,6 +35,7 @@ enum hdmi_infoframe_type {
 };

 #define HDMI_IEEE_OUI 0x000c03
+#define HDMI_FORUM_IEEE_OUI 0xc45dd8
 #define HDMI_INFOFRAME_HEADER_SIZE  4
 #define HDMI_AVI_INFOFRAME_SIZE13
 #define HDMI_SPD_INFOFRAME_SIZE25
-- 
2.10.2