[alsa-devel] [PATCH 12/15] drm/edid: Add API to help find connection type
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" > intel.com> 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" > >> > intel.com> 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
> > >> > > Signed-off-by: Vinod Koul
> > >> > > Cc: David Airlie
> > >> > > Cc: dri-devel at lists.freedesktop.org
> > >> > > Cc: Daniel Vetter
> > >> > > ---
> > >> > > 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
--
[alsa-devel] [PATCH 12/15] drm/edid: Add API to help find connection type
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" > intel.com> 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
> > > Signed-off-by: Vinod Koul
> > > Cc: David Airlie
> > > Cc: dri-devel at lists.freedesktop.org
> > > Cc: Daniel Vetter
> > > ---
> > > 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
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
[alsa-devel] [PATCH 12/15] drm/edid: Add API to help find connection type
On Thu, 03 Dec 2015, "Subhransu S. Prusty"
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" > > intel.com> 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
>> > > Signed-off-by: Vinod Koul
>> > > Cc: David Airlie
>> > > Cc: dri-devel at lists.freedesktop.org
>> > > Cc: Daniel Vetter
>> > > ---
>> > > 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
>> Alsa-devel at alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
--
Jani Nikula, Intel Open Source Technology Center
[alsa-devel] [PATCH 12/15] drm/edid: Add API to help find connection type
On Thu, Dec 03, 2015 at 01:09:16PM +0200, Jani Nikula wrote:
> On Thu, 03 Dec 2015, "Subhransu S. Prusty"
> 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" >> > intel.com> 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
> >> > > Signed-off-by: Vinod Koul
> >> > > Cc: David Airlie
> >> > > Cc: dri-devel at lists.freedesktop.org
> >> > > Cc: Daniel Vetter
> >> > > ---
> >> > > 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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
[PATCH 12/15] drm/edid: Add API to help find connection type
On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> On Tue, 01 Dec 2015, "Subhransu S. Prusty"
> 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
> > Signed-off-by: Vinod Koul
> > Cc: David Airlie
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: Daniel Vetter
> > ---
> > 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
--
[PATCH 12/15] drm/edid: Add API to help find connection type
On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote:
> On Tue, 01 Dec 2015, "Subhransu S. Prusty"
> 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
> > Signed-off-by: Vinod Koul
> > Cc: David Airlie
> > Cc: dri-devel at lists.freedesktop.org
> > Cc: Daniel Vetter
> > ---
> > 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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
[PATCH 12/15] drm/edid: Add API to help find connection type
On Tue, 01 Dec 2015, "Subhransu S. Prusty"
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
> Signed-off-by: Vinod Koul
> Cc: David Airlie
> Cc: dri-devel at lists.freedesktop.org
> Cc: Daniel Vetter
> ---
> 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),
--
Jani Nikula, Intel Open Source Technology Center
[PATCH 12/15] drm/edid: Add API to help find connection type
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
Signed-off-by: Vinod Koul
Cc: David Airlie
Cc: dri-devel at lists.freedesktop.org
Cc: Daniel Vetter
---
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;
+}
+
struct edid *drm_do_get_edid(struct drm_connector *connector,
int (*get_edid_block)(void *data, u8 *buf, unsigned int block,
size_t len),
--
1.9.1
