Message ID | 1448992031-8271-12-git-send-email-subhransu.s.prusty@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, 01 Dec 2015, "Subhransu S. Prusty" <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 <subhransu.s.prusty@intel.com> > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > Cc: David Airlie <airlied@linux.ie> > Cc: dri-devel@lists.freedesktop.org > Cc: Daniel Vetter <daniel.vetter@intel.com> > --- > 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),
On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote: > On Tue, 01 Dec 2015, "Subhransu S. Prusty" <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 <subhransu.s.prusty@intel.com> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: dri-devel@lists.freedesktop.org > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > --- > > 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
On Wed, Dec 02, 2015 at 11:53:02AM +0200, Jani Nikula wrote: > On Tue, 01 Dec 2015, "Subhransu S. Prusty" <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 <subhransu.s.prusty@intel.com> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > Cc: David Airlie <airlied@linux.ie> > > Cc: dri-devel@lists.freedesktop.org > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > --- > > 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
On Thu, 03 Dec 2015, "Subhransu S. Prusty" <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" <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 <subhransu.s.prusty@intel.com> >> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> >> > > Cc: David Airlie <airlied@linux.ie> >> > > Cc: dri-devel@lists.freedesktop.org >> > > Cc: Daniel Vetter <daniel.vetter@intel.com> >> > > --- >> > > 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@alsa-project.org >> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Thu, Dec 03, 2015 at 01:09:16PM +0200, Jani Nikula wrote: > On Thu, 03 Dec 2015, "Subhransu S. Prusty" <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" <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 <subhransu.s.prusty@intel.com> > >> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > >> > > Cc: David Airlie <airlied@linux.ie> > >> > > Cc: dri-devel@lists.freedesktop.org > >> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > >> > > --- > >> > > 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
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" <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 <subhransu.s.prusty@intel.com> > > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > > Cc: David Airlie <airlied@linux.ie> > > > Cc: dri-devel@lists.freedesktop.org > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > --- > > > 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@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
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" <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" <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 <subhransu.s.prusty@intel.com> > > >> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com> > > >> > > Cc: David Airlie <airlied@linux.ie> > > >> > > Cc: dri-devel@lists.freedesktop.org > > >> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > >> > > --- > > >> > > 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
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),