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; +} + 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 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 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, 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 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
--
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
--
dri-devel@lists.freedesktop.org
-
Jani Nikula -
Subhransu S. Prusty -
Thierry Reding