[12/15] drm/edid: Add API to help find connection type

Commit Message

Subhransu S. Prusty Dec. 1, 2015, 5:47 p.m. UTC

To fill the audio infoframe it is required to identify the connection type
as DP or HDMI. So parse the required bits in ELD to find the connection
type.

Signed-off-by: Subhransu S. Prusty <[e-mail]>
Signed-off-by: Vinod Koul <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: [e-mail]
Cc: Daniel Vetter <[e-mail]>
---
 include/drm/drm_edid.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Jani Nikula Dec. 2, 2015, 9:53 a.m. UTC | #1

On Tue, 01 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> To fill the audio infoframe it is required to identify the connection type
> as DP or HDMI. So parse the required bits in ELD to find the connection
> type.
>
> Signed-off-by: Subhransu S. Prusty <[e-mail]>
> Signed-off-by: Vinod Koul <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: [e-mail]
> Cc: Daniel Vetter <[e-mail]>
> ---
>  include/drm/drm_edid.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 2af9769..c7595a5 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
>  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
>  }
>  
> +/**
> + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> + * @eld: pointer to an eld memory structure
> + */
> +static inline int drm_eld_get_conn_type(const uint8_t *eld)
> +{
> +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
> +		DRM_ELD_CONN_TYPE_SHIFT;
> +}

I'm not sure how much this helps when the caller still needs to
magically know what the return value means...  Indeed the next patch
with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.

How about just not shifting the return value, and using
DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
points for referencing those in the kernel-doc above.

BR,
Jani.


> +
>  struct edid *drm_do_get_edid(struct drm_connector *connector,
>  	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
>  			      size_t len),

Thierry Reding Dec. 2, 2015, 5:07 p.m. UTC | #2

On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> On Tue, 01 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> > To fill the audio infoframe it is required to identify the connection type
> > as DP or HDMI. So parse the required bits in ELD to find the connection
> > type.
> >
> > Signed-off-by: Subhransu S. Prusty <[e-mail]>
> > Signed-off-by: Vinod Koul <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: [e-mail]
> > Cc: Daniel Vetter <[e-mail]>
> > ---
> >  include/drm/drm_edid.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 2af9769..c7595a5 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
> >  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> >  }
> >  
> > +/**
> > + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> > + * @eld: pointer to an eld memory structure
> > + */
> > +static inline int drm_eld_get_conn_type(const uint8_t *eld)
> > +{
> > +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
> > +		DRM_ELD_CONN_TYPE_SHIFT;
> > +}
> 
> I'm not sure how much this helps when the caller still needs to
> magically know what the return value means...  Indeed the next patch
> with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.
> 
> How about just not shifting the return value, and using
> DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
> points for referencing those in the kernel-doc above.

We already have a similar function for detecting HDMI vs. DVI (see the
drm_detect_hdmi_monitor()), so perhaps adhering to that convention might
be preferable. This could be:

	static inline bool drm_eld_detect_dp(const u8 *eld)
	{
		u8 type = eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;

		return type == DRM_ELD_CONN_TYPE_DP;
	}

Thierry

Subhransu S. Prusty Dec. 2, 2015, 5:16 p.m. UTC | #3

On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> On Tue, 01 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> > To fill the audio infoframe it is required to identify the connection type
> > as DP or HDMI. So parse the required bits in ELD to find the connection
> > type.
> >
> > Signed-off-by: Subhransu S. Prusty <[e-mail]>
> > Signed-off-by: Vinod Koul <[e-mail]>
> > Cc: David Airlie <[e-mail]>
> > Cc: [e-mail]
> > Cc: Daniel Vetter <[e-mail]>
> > ---
> >  include/drm/drm_edid.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 2af9769..c7595a5 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
> >  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> >  }
> >  
> > +/**
> > + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> > + * @eld: pointer to an eld memory structure
> > + */
> > +static inline int drm_eld_get_conn_type(const uint8_t *eld)
> > +{
> > +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
> > +		DRM_ELD_CONN_TYPE_SHIFT;
> > +}
> 
> I'm not sure how much this helps when the caller still needs to
> magically know what the return value means...  Indeed the next patch
> with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.
> 
> How about just not shifting the return value, and using
> DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
> points for referencing those in the kernel-doc above.

Sure, will update and submit again.

Regards,
Subhransu
> 
> BR,
> Jani.
> 
> 
> > +
> >  struct edid *drm_do_get_edid(struct drm_connector *connector,
> >  	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
> >  			      size_t len),
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

Jani Nikula Dec. 3, 2015, 11:09 a.m. UTC | #4

On Thu, 03 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> On Wed, Dec 02, 2015 at 06:07:01PM +0100, Thierry Reding wrote:
>> On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
>> > On Tue, 01 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
>> > > To fill the audio infoframe it is required to identify the connection type
>> > > as DP or HDMI. So parse the required bits in ELD to find the connection
>> > > type.
>> > >
>> > > Signed-off-by: Subhransu S. Prusty <[e-mail]>
>> > > Signed-off-by: Vinod Koul <[e-mail]>
>> > > Cc: David Airlie <[e-mail]>
>> > > Cc: [e-mail]
>> > > Cc: Daniel Vetter <[e-mail]>
>> > > ---
>> > >  include/drm/drm_edid.h | 10 ++++++++++
>> > >  1 file changed, 10 insertions(+)
>> > >
>> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> > > index 2af9769..c7595a5 100644
>> > > --- a/include/drm/drm_edid.h
>> > > +++ b/include/drm/drm_edid.h
>> > > @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
>> > >  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
>> > >  }
>> > >  
>> > > +/**
>> > > + * drm_eld_get_conn_type - Get device type hdmi/dp connected
>> > > + * @eld: pointer to an eld memory structure
>> > > + */
>> > > +static inline int drm_eld_get_conn_type(const uint8_t *eld)
>> > > +{
>> > > +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
>> > > +		DRM_ELD_CONN_TYPE_SHIFT;
>> > > +}
>> > 
>> > I'm not sure how much this helps when the caller still needs to
>> > magically know what the return value means...  Indeed the next patch
>> > with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.
>> > 
>> > How about just not shifting the return value, and using
>> > DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
>> > points for referencing those in the kernel-doc above.
>> 
>> We already have a similar function for detecting HDMI vs. DVI (see the
>> drm_detect_hdmi_monitor()), so perhaps adhering to that convention might
>> be preferable. This could be:
>> 
>> 	static inline bool drm_eld_detect_dp(const u8 *eld)
>> 	{
>> 		u8 type = eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
>> 
>> 		return type == DRM_ELD_CONN_TYPE_DP;
>> 	}
>
> With this approach it needs two APIs to be added for HDMI or DP
> detection. So I prefer what Jani suggested and caller compares
> whether it is HDMI/DP connection type. Will updae the kernel doc
> for the same as well.

I presume Thierry means you'd assume HDMI if drm_eld_detect_dp() returns
false.

I'm fine with either approach.

BR,
Jani.

>
>> 
>> Thierry
>
>
>
>> _______________________________________________
>> Alsa-devel mailing list
>> [e-mail]
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Thierry Reding Dec. 3, 2015, 11:21 a.m. UTC | #5

On Thu, Dec 03, 2015 at 01:09:16PM +0200, Jani Nikula wrote:
> On Thu, 03 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> > On Wed, Dec 02, 2015 at 06:07:01PM +0100, Thierry Reding wrote:
> >> On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> >> > On Tue, 01 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> >> > > To fill the audio infoframe it is required to identify the connection type
> >> > > as DP or HDMI. So parse the required bits in ELD to find the connection
> >> > > type.
> >> > >
> >> > > Signed-off-by: Subhransu S. Prusty <[e-mail]>
> >> > > Signed-off-by: Vinod Koul <[e-mail]>
> >> > > Cc: David Airlie <[e-mail]>
> >> > > Cc: [e-mail]
> >> > > Cc: Daniel Vetter <[e-mail]>
> >> > > ---
> >> > >  include/drm/drm_edid.h | 10 ++++++++++
> >> > >  1 file changed, 10 insertions(+)
> >> > >
> >> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> >> > > index 2af9769..c7595a5 100644
> >> > > --- a/include/drm/drm_edid.h
> >> > > +++ b/include/drm/drm_edid.h
> >> > > @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
> >> > >  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> >> > >  }
> >> > >  
> >> > > +/**
> >> > > + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> >> > > + * @eld: pointer to an eld memory structure
> >> > > + */
> >> > > +static inline int drm_eld_get_conn_type(const uint8_t *eld)
> >> > > +{
> >> > > +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
> >> > > +		DRM_ELD_CONN_TYPE_SHIFT;
> >> > > +}
> >> > 
> >> > I'm not sure how much this helps when the caller still needs to
> >> > magically know what the return value means...  Indeed the next patch
> >> > with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.
> >> > 
> >> > How about just not shifting the return value, and using
> >> > DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
> >> > points for referencing those in the kernel-doc above.
> >> 
> >> We already have a similar function for detecting HDMI vs. DVI (see the
> >> drm_detect_hdmi_monitor()), so perhaps adhering to that convention might
> >> be preferable. This could be:
> >> 
> >> 	static inline bool drm_eld_detect_dp(const u8 *eld)
> >> 	{
> >> 		u8 type = eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
> >> 
> >> 		return type == DRM_ELD_CONN_TYPE_DP;
> >> 	}
> >
> > With this approach it needs two APIs to be added for HDMI or DP
> > detection. So I prefer what Jani suggested and caller compares
> > whether it is HDMI/DP connection type. Will updae the kernel doc
> > for the same as well.
> 
> I presume Thierry means you'd assume HDMI if drm_eld_detect_dp() returns
> false.

Yes, that's what I was implying. This is probably highly subjective, but
I personally find boolean return values much easier to deal with because
of the limited set of values. With drm_eld_get_conn_type() you'd need to
explicitly check == DRM_ELD_CONN_TYPE_HDMI and == DRM_ELD_CONN_TYPE_DP
and then still have special code to reject all other values. Unless of
course if you consider != DRM_ELD_CONN_TYPE_DP as being HDMI, in which
case a boolean is much more concise.

But like I said, this is just my opinion, and I don't feel strongly
enough to object to the current patch.

Thierry

Subhransu S. Prusty Dec. 3, 2015, 4:08 p.m. UTC | #6

On Wed, Dec 02, 2015 at 06:07:01PM +0100, Thierry Reding wrote:
> On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> > On Tue, 01 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> > > To fill the audio infoframe it is required to identify the connection type
> > > as DP or HDMI. So parse the required bits in ELD to find the connection
> > > type.
> > >
> > > Signed-off-by: Subhransu S. Prusty <[e-mail]>
> > > Signed-off-by: Vinod Koul <[e-mail]>
> > > Cc: David Airlie <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: Daniel Vetter <[e-mail]>
> > > ---
> > >  include/drm/drm_edid.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > > index 2af9769..c7595a5 100644
> > > --- a/include/drm/drm_edid.h
> > > +++ b/include/drm/drm_edid.h
> > > @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
> > >  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> > >  }
> > >  
> > > +/**
> > > + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> > > + * @eld: pointer to an eld memory structure
> > > + */
> > > +static inline int drm_eld_get_conn_type(const uint8_t *eld)
> > > +{
> > > +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
> > > +		DRM_ELD_CONN_TYPE_SHIFT;
> > > +}
> > 
> > I'm not sure how much this helps when the caller still needs to
> > magically know what the return value means...  Indeed the next patch
> > with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.
> > 
> > How about just not shifting the return value, and using
> > DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
> > points for referencing those in the kernel-doc above.
> 
> We already have a similar function for detecting HDMI vs. DVI (see the
> drm_detect_hdmi_monitor()), so perhaps adhering to that convention might
> be preferable. This could be:
> 
> 	static inline bool drm_eld_detect_dp(const u8 *eld)
> 	{
> 		u8 type = eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
> 
> 		return type == DRM_ELD_CONN_TYPE_DP;
> 	}

With this approach it needs two APIs to be added for HDMI or DP
detection. So I prefer what Jani suggested and caller compares
whether it is HDMI/DP connection type. Will updae the kernel doc
for the same as well.

> 
> Thierry



> _______________________________________________
> Alsa-devel mailing list
> [e-mail]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Subhransu S. Prusty Dec. 3, 2015, 5:14 p.m. UTC | #7

On Thu, Dec 03, 2015 at 12:21:42PM +0100, Thierry Reding wrote:
> On Thu, Dec 03, 2015 at 01:09:16PM +0200, Jani Nikula wrote:
> > On Thu, 03 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> > > On Wed, Dec 02, 2015 at 06:07:01PM +0100, Thierry Reding wrote:
> > >> On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> > >> > On Tue, 01 Dec 2015, "Subhransu S. Prusty" <[e-mail]> wrote:
> > >> > > To fill the audio infoframe it is required to identify the connection type
> > >> > > as DP or HDMI. So parse the required bits in ELD to find the connection
> > >> > > type.
> > >> > >
> > >> > > Signed-off-by: Subhransu S. Prusty <[e-mail]>
> > >> > > Signed-off-by: Vinod Koul <[e-mail]>
> > >> > > Cc: David Airlie <[e-mail]>
> > >> > > Cc: [e-mail]
> > >> > > Cc: Daniel Vetter <[e-mail]>
> > >> > > ---
> > >> > >  include/drm/drm_edid.h | 10 ++++++++++
> > >> > >  1 file changed, 10 insertions(+)
> > >> > >
> > >> > > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > >> > > index 2af9769..c7595a5 100644
> > >> > > --- a/include/drm/drm_edid.h
> > >> > > +++ b/include/drm/drm_edid.h
> > >> > > @@ -403,6 +403,16 @@ static inline int drm_eld_size(const uint8_t *eld)
> > >> > >  	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
> > >> > >  }
> > >> > >  
> > >> > > +/**
> > >> > > + * drm_eld_get_conn_type - Get device type hdmi/dp connected
> > >> > > + * @eld: pointer to an eld memory structure
> > >> > > + */
> > >> > > +static inline int drm_eld_get_conn_type(const uint8_t *eld)
> > >> > > +{
> > >> > > +	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
> > >> > > +		DRM_ELD_CONN_TYPE_SHIFT;
> > >> > > +}
> > >> > 
> > >> > I'm not sure how much this helps when the caller still needs to
> > >> > magically know what the return value means...  Indeed the next patch
> > >> > with /* 0 is hdmi and 1 is DP */ and "conn_type == 0" is a bit ugly.
> > >> > 
> > >> > How about just not shifting the return value, and using
> > >> > DRM_ELD_CONN_TYPE_HDMI and DRM_ELD_CONN_TYPE_DP in the caller? Bonus
> > >> > points for referencing those in the kernel-doc above.
> > >> 
> > >> We already have a similar function for detecting HDMI vs. DVI (see the
> > >> drm_detect_hdmi_monitor()), so perhaps adhering to that convention might
> > >> be preferable. This could be:
> > >> 
> > >> 	static inline bool drm_eld_detect_dp(const u8 *eld)
> > >> 	{
> > >> 		u8 type = eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK;
> > >> 
> > >> 		return type == DRM_ELD_CONN_TYPE_DP;
> > >> 	}
> > >
> > > With this approach it needs two APIs to be added for HDMI or DP
> > > detection. So I prefer what Jani suggested and caller compares
> > > whether it is HDMI/DP connection type. Will updae the kernel doc
> > > for the same as well.
> > 
> > I presume Thierry means you'd assume HDMI if drm_eld_detect_dp() returns
> > false.
> 
> Yes, that's what I was implying. This is probably highly subjective, but
> I personally find boolean return values much easier to deal with because
> of the limited set of values. With drm_eld_get_conn_type() you'd need to
> explicitly check == DRM_ELD_CONN_TYPE_HDMI and == DRM_ELD_CONN_TYPE_DP
> and then still have special code to reject all other values. Unless of

I don't know what does the second bit mean in the connection type. So was
just planning to reject anything other that DP/HDMI. If that bit doesn't
carry any information, then yes I would also prefer returning a boolean.

> course if you consider != DRM_ELD_CONN_TYPE_DP as being HDMI, in which
> case a boolean is much more concise.
> 
> But like I said, this is just my opinion, and I don't feel strongly
> enough to object to the current patch.
> 
> Thierry

Patch

diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 2af9769..c7595a5 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -403,6 +403,16 @@  static inline int drm_eld_size(const uint8_t *eld)
 	return DRM_ELD_HEADER_BLOCK_SIZE + eld[DRM_ELD_BASELINE_ELD_LEN] * 4;
 }
 
+/**
+ * drm_eld_get_conn_type - Get device type hdmi/dp connected
+ * @eld: pointer to an eld memory structure
+ */
+static inline int drm_eld_get_conn_type(const uint8_t *eld)
+{
+	return (eld[DRM_ELD_SAD_COUNT_CONN_TYPE] & DRM_ELD_CONN_TYPE_MASK) >>
+		DRM_ELD_CONN_TYPE_SHIFT;
+}
+
 struct edid *drm_do_get_edid(struct drm_connector *connector,
 	int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
 			      size_t len),