Re: [PATCH v2 3/6] drm/edid: detect SCDC support in HF-VSDB

[Jose Abreu]

Hi,



On 07-02-2017 16:36, Thierry Reding wrote:
> On Tue, Feb 07, 2017 at 09:43:15PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/7/2017 4:31 PM, Jose Abreu wrote:
>>> Hi Shashank,
>>>
>>>
>>>
>>> On 06-02-2017 13:59, Shashank Sharma wrote:
 This patch does following:
 - Adds a new structure (drm_hdmi_info) in drm_display_info.
This structure will be used to save and indicate if sink
supports advanced HDMI 2.0 features
 - Adds another structure drm_scdc within drm_hdmi_info, to
reflect scdc support and capabilities in connected HDMI 2.0 sink.
 - Checks the HF-VSDB block for presence of SCDC, and marks it
in scdc structure
 - If SCDC is present, checks if sink is capable of generating
SCDC read request, and marks it in scdc structure.

 V2: Addressed review comments
 Thierry:
 - Fix typos in commit message and make abbreviation consistent
across the commit message.
 - Change structure object name from hdmi_info -> hdmi
 - Fix typos and abbreviations in description of structure drm_hdmi_info
end the description with a full stop.
 - Create a structure drm_scdc, and keep all information related to SCDC
register set (supported, read request supported) etc in it.

 Ville:
 - Change rr -> read_request
 - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all
of HF-VSDB parsing can be kept in same function, in incremental
patches.

 Reviewed-by: Thierry Reding 
 Signed-off-by: Shashank Sharma 
 ---
   drivers/gpu/drm/drm_edid.c  | 14 ++
   include/drm/drm_connector.h | 33 +
   2 files changed, 47 insertions(+)

 diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
 index 96d3e47..a487b80 100644
 --- a/drivers/gpu/drm/drm_edid.c
 +++ b/drivers/gpu/drm/drm_edid.c
 @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
   }
   EXPORT_SYMBOL(drm_default_rgb_quant_range);
 +static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
 +   const u8 *hf_vsdb)
 +{
 +  struct drm_hdmi_info *hdmi = >display_info.hdmi;
 +
 +  if (hf_vsdb[6] & 0x80) {
>>> BIT(7) ?
>> Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ?
 +  hdmi->scdc.supported = true;
 +  if (hf_vsdb[6] & 0x40)
>>> BIT(6) ?
>> Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB.
> I think what Jose was trying to say is that you should be using BIT(7)
> instead of 0x80 and BIT(6) instead of 0x40. That said, I think either is
> fine, but perhaps another idea would be to define macros for these. I
> know that most (all?) of the EDID parsing code uses literals, so this is
> consistent with existing code. Also usually code will be like:
>
>   if (hf_vsdb[X] & 0xYZ)
>   foo_supported = true;
>
> So the meaning of the bit is easy to read from the context. I think
> literals are fine in this case.
>
> Thierry

Thats exactly what I meant :) I think with BIT(x) the code is
easier to read (my hex skills are not very good :)). Anyway, if
the remaining code uses literals then maybe its better to keep
consistency.

Best regards,
Jose Miguel Abreu

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

Re: [PATCH v2 3/6] drm/edid: detect SCDC support in HF-VSDB

[Sharma, Shashank]

Regards

Shashank


On 2/8/2017 5:06 PM, Jose Abreu wrote:

Hi,



On 07-02-2017 16:36, Thierry Reding wrote:

On Tue, Feb 07, 2017 at 09:43:15PM +0530, Sharma, Shashank wrote:

Regards

Shashank


On 2/7/2017 4:31 PM, Jose Abreu wrote:

Hi Shashank,



On 06-02-2017 13:59, Shashank Sharma wrote:

This patch does following:
- Adds a new structure (drm_hdmi_info) in drm_display_info.
This structure will be used to save and indicate if sink
supports advanced HDMI 2.0 features
- Adds another structure drm_scdc within drm_hdmi_info, to
reflect scdc support and capabilities in connected HDMI 2.0 sink.
- Checks the HF-VSDB block for presence of SCDC, and marks it
in scdc structure
- If SCDC is present, checks if sink is capable of generating
SCDC read request, and marks it in scdc structure.

V2: Addressed review comments
Thierry:
- Fix typos in commit message and make abbreviation consistent
across the commit message.
- Change structure object name from hdmi_info -> hdmi
- Fix typos and abbreviations in description of structure drm_hdmi_info
end the description with a full stop.
- Create a structure drm_scdc, and keep all information related to SCDC
register set (supported, read request supported) etc in it.

Ville:
- Change rr -> read_request
- Call drm_detect_scrambling function drm_parse_hf_vsdb so that all
of HF-VSDB parsing can be kept in same function, in incremental
patches.

Reviewed-by: Thierry Reding 
Signed-off-by: Shashank Sharma 
---
   drivers/gpu/drm/drm_edid.c  | 14 ++
   include/drm/drm_connector.h | 33 +
   2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 96d3e47..a487b80 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
   }
   EXPORT_SYMBOL(drm_default_rgb_quant_range);
+static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
+const u8 *hf_vsdb)
+{
+   struct drm_hdmi_info *hdmi = >display_info.hdmi;
+
+   if (hf_vsdb[6] & 0x80) {

BIT(7) ?

Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ?

+   hdmi->scdc.supported = true;
+   if (hf_vsdb[6] & 0x40)

BIT(6) ?

Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB.

I think what Jose was trying to say is that you should be using BIT(7)
instead of 0x80 and BIT(6) instead of 0x40. That said, I think either is
fine, but perhaps another idea would be to define macros for these. I
know that most (all?) of the EDID parsing code uses literals, so this is
consistent with existing code. Also usually code will be like:

if (hf_vsdb[X] & 0xYZ)
foo_supported = true;

So the meaning of the bit is easy to read from the context. I think
literals are fine in this case.

Thierry

Thats exactly what I meant :) I think with BIT(x) the code is
easier to read (my hex skills are not very good :)). Anyway, if
the remaining code uses literals then maybe its better to keep
consistency.

Thanks, then we will keep this code, as it is.
- Shashank

Best regards,
Jose Miguel Abreu



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

Re: [PATCH v2 3/6] drm/edid: detect SCDC support in HF-VSDB

[Jose Abreu]

Hi Shashank,



On 06-02-2017 13:59, Shashank Sharma wrote:
> This patch does following:
> - Adds a new structure (drm_hdmi_info) in drm_display_info.
>   This structure will be used to save and indicate if sink
>   supports advanced HDMI 2.0 features
> - Adds another structure drm_scdc within drm_hdmi_info, to
>   reflect scdc support and capabilities in connected HDMI 2.0 sink.
> - Checks the HF-VSDB block for presence of SCDC, and marks it
>   in scdc structure
> - If SCDC is present, checks if sink is capable of generating
>   SCDC read request, and marks it in scdc structure.
>
> V2: Addressed review comments
> Thierry:
> - Fix typos in commit message and make abbreviation consistent
>   across the commit message.
> - Change structure object name from hdmi_info -> hdmi
> - Fix typos and abbreviations in description of structure drm_hdmi_info
>   end the description with a full stop.
> - Create a structure drm_scdc, and keep all information related to SCDC
>   register set (supported, read request supported) etc in it.
>
> Ville:
> - Change rr -> read_request
> - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all
>   of HF-VSDB parsing can be kept in same function, in incremental
>   patches.
>
> Reviewed-by: Thierry Reding 
> Signed-off-by: Shashank Sharma 
> ---
>  drivers/gpu/drm/drm_edid.c  | 14 ++
>  include/drm/drm_connector.h | 33 +
>  2 files changed, 47 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 96d3e47..a487b80 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
>  }
>  EXPORT_SYMBOL(drm_default_rgb_quant_range);
>  
> +static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
> +  const u8 *hf_vsdb)
> +{
> + struct drm_hdmi_info *hdmi = >display_info.hdmi;
> +
> + if (hf_vsdb[6] & 0x80) {

BIT(7) ?

> + hdmi->scdc.supported = true;
> + if (hf_vsdb[6] & 0x40)

BIT(6) ?

> + hdmi->scdc.read_request = true;
> + }
> +}
> +
>  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
>  const u8 *hdmi)
>  {
> @@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector 
> *connector,
>  
>   if (cea_db_is_hdmi_vsdb(db))
>   drm_parse_hdmi_vsdb_video(connector, db);
> + if (cea_db_is_hdmi_forum_vsdb(db))
> + drm_parse_hdmi_forum_vsdb(connector, db);
>   }
>  }
>  
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index e5e1edd..6d5304e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -87,6 +87,34 @@ enum subpixel_order {
>   SubPixelVerticalRGB,
>   SubPixelVerticalBGR,
>   SubPixelNone,
> +
> +};
> +
> +/*
> + * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
> + *
> + * Provides SCDC register support and capabilities related information on a
> + * HDMI 2.0 sink. In case of a HDMI 1.4 sink, all parameter must be 0.
> + */
> +struct drm_scdc {
> + /**
> +  * @supported: status control & data channel present.
> +  */
> + bool supported;
> + /**
> +  * @read_request: sink is capable of generating scdc read request.
> +  */
> + bool read_request;
> +};
> +
> +/**
> + * struct drm_hdmi_info - runtime information about the connected HDMI sink
> + *
> + * Describes if a given display supports advanced HDMI 2.0 features.
> + * This information is available in CEA-861-F extension blocks (like 
> HF-VSDB).
> + */
> +struct drm_hdmi_info {
> + struct drm_scdc scdc;
>  };
>  
>  /**
> @@ -188,6 +216,11 @@ struct drm_display_info {
>* @cea_rev: CEA revision of the HDMI sink.
>*/
>   u8 cea_rev;
> +
> + /**
> +  * @hdmi: advance features of a HDMI sink.

Maybe change to the same general description you used above:
"Runtime information about the connected HDMI sink" ?

> +  */
> + struct drm_hdmi_info hdmi;
>  };
>  
>  int drm_display_info_set_bus_formats(struct drm_display_info *info,

Best regards,
Jose Miguel Abreu
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/edid: detect SCDC support in HF-VSDB

[Thierry Reding]

On Tue, Feb 07, 2017 at 09:43:15PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 2/7/2017 4:31 PM, Jose Abreu wrote:
> > Hi Shashank,
> > 
> > 
> > 
> > On 06-02-2017 13:59, Shashank Sharma wrote:
> > > This patch does following:
> > > - Adds a new structure (drm_hdmi_info) in drm_display_info.
> > >This structure will be used to save and indicate if sink
> > >supports advanced HDMI 2.0 features
> > > - Adds another structure drm_scdc within drm_hdmi_info, to
> > >reflect scdc support and capabilities in connected HDMI 2.0 sink.
> > > - Checks the HF-VSDB block for presence of SCDC, and marks it
> > >in scdc structure
> > > - If SCDC is present, checks if sink is capable of generating
> > >SCDC read request, and marks it in scdc structure.
> > > 
> > > V2: Addressed review comments
> > > Thierry:
> > > - Fix typos in commit message and make abbreviation consistent
> > >across the commit message.
> > > - Change structure object name from hdmi_info -> hdmi
> > > - Fix typos and abbreviations in description of structure drm_hdmi_info
> > >end the description with a full stop.
> > > - Create a structure drm_scdc, and keep all information related to SCDC
> > >register set (supported, read request supported) etc in it.
> > > 
> > > Ville:
> > > - Change rr -> read_request
> > > - Call drm_detect_scrambling function drm_parse_hf_vsdb so that all
> > >of HF-VSDB parsing can be kept in same function, in incremental
> > >patches.
> > > 
> > > Reviewed-by: Thierry Reding 
> > > Signed-off-by: Shashank Sharma 
> > > ---
> > >   drivers/gpu/drm/drm_edid.c  | 14 ++
> > >   include/drm/drm_connector.h | 33 +
> > >   2 files changed, 47 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > > index 96d3e47..a487b80 100644
> > > --- a/drivers/gpu/drm/drm_edid.c
> > > +++ b/drivers/gpu/drm/drm_edid.c
> > > @@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
> > >   }
> > >   EXPORT_SYMBOL(drm_default_rgb_quant_range);
> > > +static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,
> > > +  const u8 *hf_vsdb)
> > > +{
> > > + struct drm_hdmi_info *hdmi = >display_info.hdmi;
> > > +
> > > + if (hf_vsdb[6] & 0x80) {
> > BIT(7) ?
> Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ?
> > 
> > > + hdmi->scdc.supported = true;
> > > + if (hf_vsdb[6] & 0x40)
> > BIT(6) ?
> Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB.

I think what Jose was trying to say is that you should be using BIT(7)
instead of 0x80 and BIT(6) instead of 0x40. That said, I think either is
fine, but perhaps another idea would be to define macros for these. I
know that most (all?) of the EDID parsing code uses literals, so this is
consistent with existing code. Also usually code will be like:

if (hf_vsdb[X] & 0xYZ)
foo_supported = true;

So the meaning of the bit is easy to read from the context. I think
literals are fine in this case.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/6] drm/edid: detect SCDC support in HF-VSDB

[Sharma, Shashank]

Regards

Shashank


On 2/7/2017 4:31 PM, Jose Abreu wrote:

Hi Shashank,



On 06-02-2017 13:59, Shashank Sharma wrote:

This patch does following:
- Adds a new structure (drm_hdmi_info) in drm_display_info.
   This structure will be used to save and indicate if sink
   supports advanced HDMI 2.0 features
- Adds another structure drm_scdc within drm_hdmi_info, to
   reflect scdc support and capabilities in connected HDMI 2.0 sink.
- Checks the HF-VSDB block for presence of SCDC, and marks it
   in scdc structure
- If SCDC is present, checks if sink is capable of generating
   SCDC read request, and marks it in scdc structure.

V2: Addressed review comments
Thierry:
- Fix typos in commit message and make abbreviation consistent
   across the commit message.
- Change structure object name from hdmi_info -> hdmi
- Fix typos and abbreviations in description of structure drm_hdmi_info
   end the description with a full stop.
- Create a structure drm_scdc, and keep all information related to SCDC
   register set (supported, read request supported) etc in it.

Ville:
- Change rr -> read_request
- Call drm_detect_scrambling function drm_parse_hf_vsdb so that all
   of HF-VSDB parsing can be kept in same function, in incremental
   patches.

Reviewed-by: Thierry Reding 
Signed-off-by: Shashank Sharma 
---
  drivers/gpu/drm/drm_edid.c  | 14 ++
  include/drm/drm_connector.h | 33 +
  2 files changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 96d3e47..a487b80 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3802,6 +3802,18 @@ enum hdmi_quantization_range
  }
  EXPORT_SYMBOL(drm_default_rgb_quant_range);
  
+static void drm_parse_hdmi_forum_vsdb(struct drm_connector *connector,

+const u8 *hf_vsdb)
+{
+   struct drm_hdmi_info *hdmi = >display_info.hdmi;
+
+   if (hf_vsdb[6] & 0x80) {

BIT(7) ?

Yes, SCDC_present bit is bit 7, byte 6 in HF-VSDB. Am I missing something ?



+   hdmi->scdc.supported = true;
+   if (hf_vsdb[6] & 0x40)

BIT(6) ?

Yes, RR_Capable bit is bit 6, byte 6 in HF-VSDB.



+   hdmi->scdc.read_request = true;
+   }
+}
+
  static void drm_parse_hdmi_deep_color_info(struct drm_connector *connector,
   const u8 *hdmi)
  {
@@ -3916,6 +3928,8 @@ static void drm_parse_cea_ext(struct drm_connector 
*connector,
  
  		if (cea_db_is_hdmi_vsdb(db))

drm_parse_hdmi_vsdb_video(connector, db);
+   if (cea_db_is_hdmi_forum_vsdb(db))
+   drm_parse_hdmi_forum_vsdb(connector, db);
}
  }
  
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h

index e5e1edd..6d5304e 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -87,6 +87,34 @@ enum subpixel_order {
SubPixelVerticalRGB,
SubPixelVerticalBGR,
SubPixelNone,
+
+};
+
+/*
+ * struct drm_scdc - Information about scdc capabilities of a HDMI 2.0 sink
+ *
+ * Provides SCDC register support and capabilities related information on a
+ * HDMI 2.0 sink. In case of a HDMI 1.4 sink, all parameter must be 0.
+ */
+struct drm_scdc {
+   /**
+* @supported: status control & data channel present.
+*/
+   bool supported;
+   /**
+* @read_request: sink is capable of generating scdc read request.
+*/
+   bool read_request;
+};
+
+/**
+ * struct drm_hdmi_info - runtime information about the connected HDMI sink
+ *
+ * Describes if a given display supports advanced HDMI 2.0 features.
+ * This information is available in CEA-861-F extension blocks (like HF-VSDB).
+ */
+struct drm_hdmi_info {
+   struct drm_scdc scdc;
  };
  
  /**

@@ -188,6 +216,11 @@ struct drm_display_info {
 * @cea_rev: CEA revision of the HDMI sink.
 */
u8 cea_rev;
+
+   /**
+* @hdmi: advance features of a HDMI sink.

Maybe change to the same general description you used above:
"Runtime information about the connected HDMI sink" ?
Actually the description I used in patch set 1, was similar, but this is 
based on feedbacks from review set 1.

- Shashank

+*/
+   struct drm_hdmi_info hdmi;
  };
  
  int drm_display_info_set_bus_formats(struct drm_display_info *info,

Best regards,
Jose Miguel Abreu


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