[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:
