As I told you in irc I'm not sure if the best option is to use common,
raw and old, but since I don't have a better idea and while nobody
else has, just to go ahead ;)
And I liked very much the raw part, although I had to count the bytes
amount on VBT doc when reviewing following patches ;)
For this patch:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Still have to continue reviewing the following ones
On Tue, Aug 27, 2013 at 5:22 PM, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> We currently treat the child_device_config as a simple struct, but
> this is not correct: new BDB versions change the meaning of some
> offsets, so the struct needs to be adjusted for each version.
>
> Since there are too many changes (today we're in version 170!), making
> a big versioned union would be too complicated, so child_device_config
> is now a union of 3 things: (i) a "raw" byte array that's safe to use
> anywhere; (ii) an "old" structure that's the one we've been using and
> should be safe to keep in the SDVO and TV code; and (iii) a "common"
> structure that should contain only fields that are common for all the
> known VBT versions.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/intel_bios.c | 36 ++++++++++++++++++------------------
> drivers/gpu/drm/i915/intel_bios.h | 33 +++++++++++++++++++++++++++++++--
> drivers/gpu/drm/i915/intel_dp.c | 6 +++---
> drivers/gpu/drm/i915/intel_lvds.c | 3 ++-
> drivers/gpu/drm/i915/intel_tv.c | 8 ++++----
> 6 files changed, 59 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 84b95b1..08fe1b6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1067,7 +1067,7 @@ struct intel_vbt_data {
> int crt_ddc_pin;
>
> int child_dev_num;
> - struct child_device_config *child_dev;
> + union child_device_config *child_dev;
> };
>
> enum intel_ddb_partitioning {
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 53f2bed..8a27925 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -389,7 +389,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
> {
> struct sdvo_device_mapping *p_mapping;
> struct bdb_general_definitions *p_defs;
> - struct child_device_config *p_child;
> + union child_device_config *p_child;
> int i, child_device_num, count;
> u16 block_size;
>
> @@ -416,36 +416,36 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
> count = 0;
> for (i = 0; i < child_device_num; i++) {
> p_child = &(p_defs->devices[i]);
> - if (!p_child->device_type) {
> + if (!p_child->old.device_type) {
> /* skip the device block if device type is invalid */
> continue;
> }
> - if (p_child->slave_addr != SLAVE_ADDR1 &&
> - p_child->slave_addr != SLAVE_ADDR2) {
> + if (p_child->old.slave_addr != SLAVE_ADDR1 &&
> + p_child->old.slave_addr != SLAVE_ADDR2) {
> /*
> * If the slave address is neither 0x70 nor 0x72,
> * it is not a SDVO device. Skip it.
> */
> continue;
> }
> - if (p_child->dvo_port != DEVICE_PORT_DVOB &&
> - p_child->dvo_port != DEVICE_PORT_DVOC) {
> + if (p_child->old.dvo_port != DEVICE_PORT_DVOB &&
> + p_child->old.dvo_port != DEVICE_PORT_DVOC) {
> /* skip the incorrect SDVO port */
> DRM_DEBUG_KMS("Incorrect SDVO port. Skip it\n");
> continue;
> }
> DRM_DEBUG_KMS("the SDVO device with slave addr %2x is found on"
> " %s port\n",
> - p_child->slave_addr,
> - (p_child->dvo_port == DEVICE_PORT_DVOB) ?
> + p_child->old.slave_addr,
> + (p_child->old.dvo_port == DEVICE_PORT_DVOB) ?
> "SDVOB" : "SDVOC");
> - p_mapping = &(dev_priv->sdvo_mappings[p_child->dvo_port - 1]);
> + p_mapping = &(dev_priv->sdvo_mappings[p_child->old.dvo_port - 1]);
> if (!p_mapping->initialized) {
> - p_mapping->dvo_port = p_child->dvo_port;
> - p_mapping->slave_addr = p_child->slave_addr;
> - p_mapping->dvo_wiring = p_child->dvo_wiring;
> - p_mapping->ddc_pin = p_child->ddc_pin;
> - p_mapping->i2c_pin = p_child->i2c_pin;
> + p_mapping->dvo_port = p_child->old.dvo_port;
> + p_mapping->slave_addr = p_child->old.slave_addr;
> + p_mapping->dvo_wiring = p_child->old.dvo_wiring;
> + p_mapping->ddc_pin = p_child->old.ddc_pin;
> + p_mapping->i2c_pin = p_child->old.i2c_pin;
> p_mapping->initialized = 1;
> DRM_DEBUG_KMS("SDVO device: dvo=%x, addr=%x, wiring=%d, ddc_pin=%d, i2c_pin=%d\n",
> p_mapping->dvo_port,
> @@ -457,7 +457,7 @@ parse_sdvo_device_mapping(struct drm_i915_private *dev_priv,
> DRM_DEBUG_KMS("Maybe one SDVO port is shared by "
> "two SDVO device.\n");
> }
> - if (p_child->slave2_addr) {
> + if (p_child->old.slave2_addr) {
> /* Maybe this is a SDVO device with multiple inputs */
> /* And the mapping info is not added */
> DRM_DEBUG_KMS("there exists the slave2_addr. Maybe this"
> @@ -573,7 +573,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> struct bdb_header *bdb)
> {
> struct bdb_general_definitions *p_defs;
> - struct child_device_config *p_child, *child_dev_ptr;
> + union child_device_config *p_child, *child_dev_ptr;
> int i, child_device_num, count;
> u16 block_size;
>
> @@ -601,7 +601,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> /* get the number of child device that is present */
> for (i = 0; i < child_device_num; i++) {
> p_child = &(p_defs->devices[i]);
> - if (!p_child->device_type) {
> + if (!p_child->common.device_type) {
> /* skip the device block if device type is invalid */
> continue;
> }
> @@ -621,7 +621,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> count = 0;
> for (i = 0; i < child_device_num; i++) {
> p_child = &(p_defs->devices[i]);
> - if (!p_child->device_type) {
> + if (!p_child->common.device_type) {
> /* skip the device block if device type is invalid */
> continue;
> }
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index e088d6f..ae1b423 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -201,7 +201,10 @@ struct bdb_general_features {
> #define DEVICE_PORT_DVOB 0x01
> #define DEVICE_PORT_DVOC 0x02
>
> -struct child_device_config {
> +/* We used to keep this struct but without any version control. We should avoid
> + * using it in the future, but it should be safe to keep using it in the old
> + * code. */
> +struct old_child_dev_config {
> u16 handle;
> u16 device_type;
> u8 device_id[10]; /* ascii string */
> @@ -223,6 +226,32 @@ struct child_device_config {
> u8 dvo_function;
> } __attribute__((packed));
>
> +/* This one contains field offsets that are known to be common for all BDB
> + * versions. Notice that the meaning of the contents contents may still change,
> + * but at least the offsets are consistent. */
> +struct common_child_dev_config {
> + u16 handle;
> + u16 device_type;
> + u8 not_common1[12];
> + u8 dvo_port;
> + u8 not_common2[2];
> + u8 ddc_pin;
> + u16 edid_ptr;
> +} __attribute__((packed));
> +
> +/* This field changes depending on the BDB version, so the most reliable way to
> + * read it is by checking the BDB version and reading the raw pointer. */
> +union child_device_config {
> + /* This one is safe to be used anywhere, but the code should still check
> + * the BDB version. */
> + u8 raw[33];
> + /* This one should only be kept for legacy code. */
> + struct old_child_dev_config old;
> + /* This one should also be safe to use anywhere, even without version
> + * checks. */
> + struct common_child_dev_config common;
> +};
> +
> struct bdb_general_definitions {
> /* DDC GPIO */
> u8 crt_ddc_gmbus_pin;
> @@ -248,7 +277,7 @@ struct bdb_general_definitions {
> * number = (block_size - sizeof(bdb_general_definitions))/
> * sizeof(child_device_config);
> */
> - struct child_device_config devices[0];
> + union child_device_config devices[0];
> } __attribute__((packed));
>
> struct bdb_lvds_options {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 2151d13..6224ea2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3090,7 +3090,7 @@ intel_trans_dp_port_sel(struct drm_crtc *crtc)
> bool intel_dpd_is_edp(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct child_device_config *p_child;
> + union child_device_config *p_child;
> int i;
>
> if (!dev_priv->vbt.child_dev_num)
> @@ -3099,8 +3099,8 @@ bool intel_dpd_is_edp(struct drm_device *dev)
> for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> p_child = dev_priv->vbt.child_dev + i;
>
> - if (p_child->dvo_port == PORT_IDPD &&
> - p_child->device_type == DEVICE_TYPE_eDP)
> + if (p_child->common.dvo_port == PORT_IDPD &&
> + p_child->common.device_type == DEVICE_TYPE_eDP)
> return true;
> }
> return false;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 4d33278..dc99e38 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -786,7 +786,8 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
> return true;
>
> for (i = 0; i < dev_priv->vbt.child_dev_num; i++) {
> - struct child_device_config *child = dev_priv->vbt.child_dev + i;
> + union child_device_config *uchild = dev_priv->vbt.child_dev + i;
> + struct old_child_dev_config *child = &uchild->old;
>
> /* If the device type is not LFP, continue.
> * We have to check both the new identifiers as well as the
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index f2c6d79..78631a2 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1510,7 +1510,7 @@ static const struct drm_encoder_funcs intel_tv_enc_funcs = {
> static int tv_is_present_in_vbt(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> - struct child_device_config *p_child;
> + union child_device_config *p_child;
> int i, ret;
>
> if (!dev_priv->vbt.child_dev_num)
> @@ -1522,13 +1522,13 @@ static int tv_is_present_in_vbt(struct drm_device *dev)
> /*
> * If the device type is not TV, continue.
> */
> - if (p_child->device_type != DEVICE_TYPE_INT_TV &&
> - p_child->device_type != DEVICE_TYPE_TV)
> + if (p_child->old.device_type != DEVICE_TYPE_INT_TV &&
> + p_child->old.device_type != DEVICE_TYPE_TV)
> continue;
> /* Only when the addin_offset is non-zero, it is regarded
> * as present.
> */
> - if (p_child->addin_offset) {
> + if (p_child->old.addin_offset) {
> ret = 1;
> break;
> }
> --
> 1.8.1.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
