[PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Jani Nikula]

This reverts

commit 047fe6e6db9161e69271f56daaafdaf2add023b1
Author: David Weinehall <david.weinehall@linux.intel.com>
Date:   Tue Aug 4 16:55:52 2015 +0300

    drm/i915: Allow parsing of variable size child device entries from VBT

That commit is not valid for v4.2, however it will be valid for v4.3. It
was simply queued too early.

The referenced regressing commit is just fine until the size of struct
common_child_dev_config changes, and that won't happen until
v4.3. Indeed, the expected size checks here rely on the increased size
of the struct, breaking new platforms.

Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Weinehall <david.weinehall@linux.intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>

---

There was another patch from David increasing the size of the struct
[1], but that then regresses sdvo mapping parsing. It's the simplest to
just revert and fix this up properly for v4.3.

[1] http://mid.gmane.org/20150813131415.GO6150@boom
---
 drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
 1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 3dcd59e694db..198fc3c3291b 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 	const union child_device_config *p_child;
 	union child_device_config *child_dev_ptr;
 	int i, child_device_num, count;
-	u8 expected_size;
-	u16 block_size;
+	u16	block_size;
 
 	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
 	if (!p_defs) {
 		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
 		return;
 	}
-	if (bdb->version < 195) {
-		expected_size = 33;
-	} else if (bdb->version == 195) {
-		expected_size = 37;
-	} else if (bdb->version <= 197) {
-		expected_size = 38;
-	} else {
-		expected_size = 38;
-		DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
-				 expected_size, bdb->version);
-	}
-
-	if (expected_size > sizeof(*p_child)) {
-		DRM_ERROR("child_device_config cannot fit in p_child\n");
-		return;
-	}
-
-	if (p_defs->child_dev_size != expected_size) {
-		DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
-			  p_defs->child_dev_size, expected_size, bdb->version);
+	if (p_defs->child_dev_size < sizeof(*p_child)) {
+		DRM_ERROR("General definiton block child device size is too small.\n");
 		return;
 	}
 	/* get the block size of general definitions */
@@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
 
 		child_dev_ptr = dev_priv->vbt.child_dev + count;
 		count++;
-		memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
+		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
 	}
 	return;
 }
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Ville Syrjälä]

On Tue, Aug 18, 2015 at 12:33:36PM +0300, Jani Nikula wrote:
> This reverts
> 
> commit 047fe6e6db9161e69271f56daaafdaf2add023b1
> Author: David Weinehall <david.weinehall@linux.intel.com>
> Date:   Tue Aug 4 16:55:52 2015 +0300
> 
>     drm/i915: Allow parsing of variable size child device entries from VBT
> 
> That commit is not valid for v4.2, however it will be valid for v4.3. It
> was simply queued too early.
> 
> The referenced regressing commit is just fine until the size of struct
> common_child_dev_config changes, and that won't happen until
> v4.3. Indeed, the expected size checks here rely on the increased size
> of the struct, breaking new platforms.
> 
> Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> ---
> 
> There was another patch from David increasing the size of the struct
> [1], but that then regresses sdvo mapping parsing. It's the simplest to
> just revert and fix this up properly for v4.3.
> 
> [1] http://mid.gmane.org/20150813131415.GO6150@boom
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 3dcd59e694db..198fc3c3291b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  	const union child_device_config *p_child;
>  	union child_device_config *child_dev_ptr;
>  	int i, child_device_num, count;
> -	u8 expected_size;
> -	u16 block_size;
> +	u16	block_size;
>  
>  	p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>  	if (!p_defs) {
>  		DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>  		return;
>  	}
> -	if (bdb->version < 195) {
> -		expected_size = 33;
> -	} else if (bdb->version == 195) {
> -		expected_size = 37;
> -	} else if (bdb->version <= 197) {
> -		expected_size = 38;
> -	} else {
> -		expected_size = 38;
> -		DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
> -				 expected_size, bdb->version);
> -	}
> -
> -	if (expected_size > sizeof(*p_child)) {
> -		DRM_ERROR("child_device_config cannot fit in p_child\n");
> -		return;
> -	}
> -
> -	if (p_defs->child_dev_size != expected_size) {
> -		DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
> -			  p_defs->child_dev_size, expected_size, bdb->version);
> +	if (p_defs->child_dev_size < sizeof(*p_child)) {
> +		DRM_ERROR("General definiton block child device size is too small.\n");
>  		return;
>  	}
>  	/* get the block size of general definitions */
> @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>  
>  		child_dev_ptr = dev_priv->vbt.child_dev + count;
>  		count++;
> -		memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
> +		memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>  	}
>  	return;
>  }
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Daniel Vetter]

On Tue, Aug 18, 2015 at 2:33 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> This reverts
>
> commit 047fe6e6db9161e69271f56daaafdaf2add023b1
> Author: David Weinehall <david.weinehall@linux.intel.com>
> Date:   Tue Aug 4 16:55:52 2015 +0300
>
>     drm/i915: Allow parsing of variable size child device entries from VBT
>
> That commit is not valid for v4.2, however it will be valid for v4.3. It
> was simply queued too early.

Nope, this patch from David also incidentally fixes a regression from
90e4f1592bb6e82f6690f0e05a8aadc which is why I merged it to. If you
want to revert this, you need to revert more (and with that also break
BSW).

Isn't there a more minimal fix we can do for 4.2?
-Daniel

>
> The referenced regressing commit is just fine until the size of struct
> common_child_dev_config changes, and that won't happen until
> v4.3. Indeed, the expected size checks here rely on the increased size
> of the struct, breaking new platforms.
>
> Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> ---
>
> There was another patch from David increasing the size of the struct
> [1], but that then regresses sdvo mapping parsing. It's the simplest to
> just revert and fix this up properly for v4.3.
>
> [1] http://mid.gmane.org/20150813131415.GO6150@boom
> ---
>  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
>  1 file changed, 4 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 3dcd59e694db..198fc3c3291b 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>         const union child_device_config *p_child;
>         union child_device_config *child_dev_ptr;
>         int i, child_device_num, count;
> -       u8 expected_size;
> -       u16 block_size;
> +       u16     block_size;
>
>         p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>         if (!p_defs) {
>                 DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>                 return;
>         }
> -       if (bdb->version < 195) {
> -               expected_size = 33;
> -       } else if (bdb->version == 195) {
> -               expected_size = 37;
> -       } else if (bdb->version <= 197) {
> -               expected_size = 38;
> -       } else {
> -               expected_size = 38;
> -               DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
> -                                expected_size, bdb->version);
> -       }
> -
> -       if (expected_size > sizeof(*p_child)) {
> -               DRM_ERROR("child_device_config cannot fit in p_child\n");
> -               return;
> -       }
> -
> -       if (p_defs->child_dev_size != expected_size) {
> -               DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
> -                         p_defs->child_dev_size, expected_size, bdb->version);
> +       if (p_defs->child_dev_size < sizeof(*p_child)) {
> +               DRM_ERROR("General definiton block child device size is too small.\n");
>                 return;
>         }
>         /* get the block size of general definitions */
> @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>
>                 child_dev_ptr = dev_priv->vbt.child_dev + count;
>                 count++;
> -               memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
> +               memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>         }
>         return;
>  }
> --
> 2.1.4
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Ville Syrjälä]

On Tue, Aug 18, 2015 at 09:58:57AM -0700, Daniel Vetter wrote:
> On Tue, Aug 18, 2015 at 2:33 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> > This reverts
> >
> > commit 047fe6e6db9161e69271f56daaafdaf2add023b1
> > Author: David Weinehall <david.weinehall@linux.intel.com>
> > Date:   Tue Aug 4 16:55:52 2015 +0300
> >
> >     drm/i915: Allow parsing of variable size child device entries from VBT
> >
> > That commit is not valid for v4.2, however it will be valid for v4.3. It
> > was simply queued too early.
> 
> Nope, this patch from David also incidentally fixes a regression from
> 90e4f1592bb6e82f6690f0e05a8aadc which is why I merged it to.

What regression?

> If you
> want to revert this, you need to revert more (and with that also break
> BSW).
> 
> Isn't there a more minimal fix we can do for 4.2?
> -Daniel
> 
> >
> > The referenced regressing commit is just fine until the size of struct
> > common_child_dev_config changes, and that won't happen until
> > v4.3. Indeed, the expected size checks here rely on the increased size
> > of the struct, breaking new platforms.
> >
> > Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >
> > ---
> >
> > There was another patch from David increasing the size of the struct
> > [1], but that then regresses sdvo mapping parsing. It's the simplest to
> > just revert and fix this up properly for v4.3.
> >
> > [1] http://mid.gmane.org/20150813131415.GO6150@boom
> > ---
> >  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
> >  1 file changed, 4 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 3dcd59e694db..198fc3c3291b 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >         const union child_device_config *p_child;
> >         union child_device_config *child_dev_ptr;
> >         int i, child_device_num, count;
> > -       u8 expected_size;
> > -       u16 block_size;
> > +       u16     block_size;
> >
> >         p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
> >         if (!p_defs) {
> >                 DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
> >                 return;
> >         }
> > -       if (bdb->version < 195) {
> > -               expected_size = 33;
> > -       } else if (bdb->version == 195) {
> > -               expected_size = 37;
> > -       } else if (bdb->version <= 197) {
> > -               expected_size = 38;
> > -       } else {
> > -               expected_size = 38;
> > -               DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
> > -                                expected_size, bdb->version);
> > -       }
> > -
> > -       if (expected_size > sizeof(*p_child)) {
> > -               DRM_ERROR("child_device_config cannot fit in p_child\n");
> > -               return;
> > -       }
> > -
> > -       if (p_defs->child_dev_size != expected_size) {
> > -               DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
> > -                         p_defs->child_dev_size, expected_size, bdb->version);
> > +       if (p_defs->child_dev_size < sizeof(*p_child)) {
> > +               DRM_ERROR("General definiton block child device size is too small.\n");
> >                 return;
> >         }
> >         /* get the block size of general definitions */
> > @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >
> >                 child_dev_ptr = dev_priv->vbt.child_dev + count;
> >                 count++;
> > -               memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
> > +               memcpy(child_dev_ptr, p_child, sizeof(*p_child));
> >         }
> >         return;
> >  }
> > --
> > 2.1.4
> >
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Daniel Vetter]

On Tue, Aug 18, 2015 at 10:00 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Aug 18, 2015 at 09:58:57AM -0700, Daniel Vetter wrote:
>> On Tue, Aug 18, 2015 at 2:33 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> > This reverts
>> >
>> > commit 047fe6e6db9161e69271f56daaafdaf2add023b1
>> > Author: David Weinehall <david.weinehall@linux.intel.com>
>> > Date:   Tue Aug 4 16:55:52 2015 +0300
>> >
>> >     drm/i915: Allow parsing of variable size child device entries from VBT
>> >
>> > That commit is not valid for v4.2, however it will be valid for v4.3. It
>> > was simply queued too early.
>>
>> Nope, this patch from David also incidentally fixes a regression from
>> 90e4f1592bb6e82f6690f0e05a8aadc which is why I merged it to.
>
> What regression?

Quoting the commit message: "...  since we're hitting a DRM_ERROR on
older platforms with this." Not every platform has the BSW vbt layout
;-) Oh and why do I even bother to write this stuff when no one reads
it?
-Daniel

>> If you
>> want to revert this, you need to revert more (and with that also break
>> BSW).
>>
>> Isn't there a more minimal fix we can do for 4.2?
>> -Daniel
>>
>> >
>> > The referenced regressing commit is just fine until the size of struct
>> > common_child_dev_config changes, and that won't happen until
>> > v4.3. Indeed, the expected size checks here rely on the increased size
>> > of the struct, breaking new platforms.
>> >
>> > Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>> > Cc: David Weinehall <david.weinehall@linux.intel.com>
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >
>> > ---
>> >
>> > There was another patch from David increasing the size of the struct
>> > [1], but that then regresses sdvo mapping parsing. It's the simplest to
>> > just revert and fix this up properly for v4.3.
>> >
>> > [1] http://mid.gmane.org/20150813131415.GO6150@boom
>> > ---
>> >  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
>> >  1 file changed, 4 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> > index 3dcd59e694db..198fc3c3291b 100644
>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>> > @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>> >         const union child_device_config *p_child;
>> >         union child_device_config *child_dev_ptr;
>> >         int i, child_device_num, count;
>> > -       u8 expected_size;
>> > -       u16 block_size;
>> > +       u16     block_size;
>> >
>> >         p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>> >         if (!p_defs) {
>> >                 DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>> >                 return;
>> >         }
>> > -       if (bdb->version < 195) {
>> > -               expected_size = 33;
>> > -       } else if (bdb->version == 195) {
>> > -               expected_size = 37;
>> > -       } else if (bdb->version <= 197) {
>> > -               expected_size = 38;
>> > -       } else {
>> > -               expected_size = 38;
>> > -               DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
>> > -                                expected_size, bdb->version);
>> > -       }
>> > -
>> > -       if (expected_size > sizeof(*p_child)) {
>> > -               DRM_ERROR("child_device_config cannot fit in p_child\n");
>> > -               return;
>> > -       }
>> > -
>> > -       if (p_defs->child_dev_size != expected_size) {
>> > -               DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
>> > -                         p_defs->child_dev_size, expected_size, bdb->version);
>> > +       if (p_defs->child_dev_size < sizeof(*p_child)) {
>> > +               DRM_ERROR("General definiton block child device size is too small.\n");
>> >                 return;
>> >         }
>> >         /* get the block size of general definitions */
>> > @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>> >
>> >                 child_dev_ptr = dev_priv->vbt.child_dev + count;
>> >                 count++;
>> > -               memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>> > +               memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>> >         }
>> >         return;
>> >  }
>> > --
>> > 2.1.4
>> >
>>
>>
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> --
> Ville Syrjälä
> Intel OTC



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Jani Nikula]

On Wed, 19 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Aug 18, 2015 at 10:00 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> On Tue, Aug 18, 2015 at 09:58:57AM -0700, Daniel Vetter wrote:
>>> On Tue, Aug 18, 2015 at 2:33 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>>> > This reverts
>>> >
>>> > commit 047fe6e6db9161e69271f56daaafdaf2add023b1
>>> > Author: David Weinehall <david.weinehall@linux.intel.com>
>>> > Date:   Tue Aug 4 16:55:52 2015 +0300
>>> >
>>> >     drm/i915: Allow parsing of variable size child device entries from VBT
>>> >
>>> > That commit is not valid for v4.2, however it will be valid for v4.3. It
>>> > was simply queued too early.
>>>
>>> Nope, this patch from David also incidentally fixes a regression from
>>> 90e4f1592bb6e82f6690f0e05a8aadc which is why I merged it to.
>>
>> What regression?
>
> Quoting the commit message: "...  since we're hitting a DRM_ERROR on
> older platforms with this." Not every platform has the BSW vbt layout
> ;-) Oh and why do I even bother to write this stuff when no one reads
> it?

I read it, and I think it's wrong. I even explained why in my commit
message (yes, please read it). I don't think there was a DRM_ERROR on
older platform with Ville's patch, on v4.2 where the revert is targeted,
and even if there were, David's patch would *not* fix it. Indeed there
was no discussion of a regression on the mailing list.

If there was any regression, it was introduced by

commit 75067ddecf21271631bc018d2fb23ddd09b66aae
Author: Antti Koskipaa <antti.koskipaa@linux.intel.com>
Date:   Fri Jul 10 14:10:55 2015 +0300

    drm/i915: Per-DDI I_boost override

when the size of union child_device_config changed. But that's headed
for v4.3. And we're talking about v4.2, which is getting pretty
urgent. We'll have a bit more time to sort out the mess that will land
in v4.3 (and I've already sent one patch sorting out SDVO breakage).

BR,
Jani.



> -Daniel
>
>>> If you
>>> want to revert this, you need to revert more (and with that also break
>>> BSW).
>>>
>>> Isn't there a more minimal fix we can do for 4.2?
>>> -Daniel
>>>
>>> >
>>> > The referenced regressing commit is just fine until the size of struct
>>> > common_child_dev_config changes, and that won't happen until
>>> > v4.3. Indeed, the expected size checks here rely on the increased size
>>> > of the struct, breaking new platforms.
>>> >
>>> > Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
>>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>>> > Cc: David Weinehall <david.weinehall@linux.intel.com>
>>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> >
>>> > ---
>>> >
>>> > There was another patch from David increasing the size of the struct
>>> > [1], but that then regresses sdvo mapping parsing. It's the simplest to
>>> > just revert and fix this up properly for v4.3.
>>> >
>>> > [1] http://mid.gmane.org/20150813131415.GO6150@boom
>>> > ---
>>> >  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
>>> >  1 file changed, 4 insertions(+), 23 deletions(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>> > index 3dcd59e694db..198fc3c3291b 100644
>>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> > @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>> >         const union child_device_config *p_child;
>>> >         union child_device_config *child_dev_ptr;
>>> >         int i, child_device_num, count;
>>> > -       u8 expected_size;
>>> > -       u16 block_size;
>>> > +       u16     block_size;
>>> >
>>> >         p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>>> >         if (!p_defs) {
>>> >                 DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>>> >                 return;
>>> >         }
>>> > -       if (bdb->version < 195) {
>>> > -               expected_size = 33;
>>> > -       } else if (bdb->version == 195) {
>>> > -               expected_size = 37;
>>> > -       } else if (bdb->version <= 197) {
>>> > -               expected_size = 38;
>>> > -       } else {
>>> > -               expected_size = 38;
>>> > -               DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
>>> > -                                expected_size, bdb->version);
>>> > -       }
>>> > -
>>> > -       if (expected_size > sizeof(*p_child)) {
>>> > -               DRM_ERROR("child_device_config cannot fit in p_child\n");
>>> > -               return;
>>> > -       }
>>> > -
>>> > -       if (p_defs->child_dev_size != expected_size) {
>>> > -               DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
>>> > -                         p_defs->child_dev_size, expected_size, bdb->version);
>>> > +       if (p_defs->child_dev_size < sizeof(*p_child)) {
>>> > +               DRM_ERROR("General definiton block child device size is too small.\n");
>>> >                 return;
>>> >         }
>>> >         /* get the block size of general definitions */
>>> > @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>> >
>>> >                 child_dev_ptr = dev_priv->vbt.child_dev + count;
>>> >                 count++;
>>> > -               memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>>> > +               memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>>> >         }
>>> >         return;
>>> >  }
>>> > --
>>> > 2.1.4
>>> >
>>>
>>>
>>>
>>> --
>>> Daniel Vetter
>>> Software Engineer, Intel Corporation
>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>
>> --
>> Ville Syrjälä
>> Intel OTC
>
>
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Jani Nikula]

On Wed, 19 Aug 2015, Jani Nikula <jani.nikula@intel.com> wrote:
> On Wed, 19 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, Aug 18, 2015 at 10:00 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>> On Tue, Aug 18, 2015 at 09:58:57AM -0700, Daniel Vetter wrote:
>>>> On Tue, Aug 18, 2015 at 2:33 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>>>> > This reverts
>>>> >
>>>> > commit 047fe6e6db9161e69271f56daaafdaf2add023b1
>>>> > Author: David Weinehall <david.weinehall@linux.intel.com>
>>>> > Date:   Tue Aug 4 16:55:52 2015 +0300
>>>> >
>>>> >     drm/i915: Allow parsing of variable size child device entries from VBT
>>>> >
>>>> > That commit is not valid for v4.2, however it will be valid for v4.3. It
>>>> > was simply queued too early.
>>>>
>>>> Nope, this patch from David also incidentally fixes a regression from
>>>> 90e4f1592bb6e82f6690f0e05a8aadc which is why I merged it to.
>>>
>>> What regression?
>>
>> Quoting the commit message: "...  since we're hitting a DRM_ERROR on
>> older platforms with this." Not every platform has the BSW vbt layout
>> ;-) Oh and why do I even bother to write this stuff when no one reads
>> it?
>
> I read it, and I think it's wrong. I even explained why in my commit
> message (yes, please read it). I don't think there was a DRM_ERROR on
> older platform with Ville's patch, on v4.2 where the revert is targeted,
> and even if there were, David's patch would *not* fix it. Indeed there
> was no discussion of a regression on the mailing list.
>
> If there was any regression, it was introduced by
>
> commit 75067ddecf21271631bc018d2fb23ddd09b66aae
> Author: Antti Koskipaa <antti.koskipaa@linux.intel.com>
> Date:   Fri Jul 10 14:10:55 2015 +0300
>
>     drm/i915: Per-DDI I_boost override
>
> when the size of union child_device_config changed. But that's headed
> for v4.3. And we're talking about v4.2, which is getting pretty
> urgent. We'll have a bit more time to sort out the mess that will land
> in v4.3 (and I've already sent one patch sorting out SDVO breakage).

I'm not waiting with this. I'm taking my chances, pushed to
drm-intel-fixes. Thanks for the review.

Note that the commit remains in drm-intel-next-fixes and
drm-intel-next-queued, but not in drm-intel-nightly due to this
revert. We'll need to clear that up, but for the time being getting v4.2
fixed up is of a higher priority to me.

BR,
Jani.




>
> BR,
> Jani.
>
>
>
>> -Daniel
>>
>>>> If you
>>>> want to revert this, you need to revert more (and with that also break
>>>> BSW).
>>>>
>>>> Isn't there a more minimal fix we can do for 4.2?
>>>> -Daniel
>>>>
>>>> >
>>>> > The referenced regressing commit is just fine until the size of struct
>>>> > common_child_dev_config changes, and that won't happen until
>>>> > v4.3. Indeed, the expected size checks here rely on the increased size
>>>> > of the struct, breaking new platforms.
>>>> >
>>>> > Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
>>>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> > Cc: David Weinehall <david.weinehall@linux.intel.com>
>>>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>> >
>>>> > ---
>>>> >
>>>> > There was another patch from David increasing the size of the struct
>>>> > [1], but that then regresses sdvo mapping parsing. It's the simplest to
>>>> > just revert and fix this up properly for v4.3.
>>>> >
>>>> > [1] http://mid.gmane.org/20150813131415.GO6150@boom
>>>> > ---
>>>> >  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
>>>> >  1 file changed, 4 insertions(+), 23 deletions(-)
>>>> >
>>>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>>> > index 3dcd59e694db..198fc3c3291b 100644
>>>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>>>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>>>> > @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>>> >         const union child_device_config *p_child;
>>>> >         union child_device_config *child_dev_ptr;
>>>> >         int i, child_device_num, count;
>>>> > -       u8 expected_size;
>>>> > -       u16 block_size;
>>>> > +       u16     block_size;
>>>> >
>>>> >         p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>>>> >         if (!p_defs) {
>>>> >                 DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>>>> >                 return;
>>>> >         }
>>>> > -       if (bdb->version < 195) {
>>>> > -               expected_size = 33;
>>>> > -       } else if (bdb->version == 195) {
>>>> > -               expected_size = 37;
>>>> > -       } else if (bdb->version <= 197) {
>>>> > -               expected_size = 38;
>>>> > -       } else {
>>>> > -               expected_size = 38;
>>>> > -               DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
>>>> > -                                expected_size, bdb->version);
>>>> > -       }
>>>> > -
>>>> > -       if (expected_size > sizeof(*p_child)) {
>>>> > -               DRM_ERROR("child_device_config cannot fit in p_child\n");
>>>> > -               return;
>>>> > -       }
>>>> > -
>>>> > -       if (p_defs->child_dev_size != expected_size) {
>>>> > -               DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
>>>> > -                         p_defs->child_dev_size, expected_size, bdb->version);
>>>> > +       if (p_defs->child_dev_size < sizeof(*p_child)) {
>>>> > +               DRM_ERROR("General definiton block child device size is too small.\n");
>>>> >                 return;
>>>> >         }
>>>> >         /* get the block size of general definitions */
>>>> > @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>>>> >
>>>> >                 child_dev_ptr = dev_priv->vbt.child_dev + count;
>>>> >                 count++;
>>>> > -               memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>>>> > +               memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>>>> >         }
>>>> >         return;
>>>> >  }
>>>> > --
>>>> > 2.1.4
>>>> >
>>>>
>>>>
>>>>
>>>> --
>>>> Daniel Vetter
>>>> Software Engineer, Intel Corporation
>>>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>>>
>>> --
>>> Ville Syrjälä
>>> Intel OTC
>>
>>
>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Mika Kahola]

On Tue, 2015-08-18 at 09:58 -0700, Daniel Vetter wrote:
> On Tue, Aug 18, 2015 at 2:33 AM, Jani Nikula <jani.nikula@intel.com> wrote:
> > This reverts
> >
> > commit 047fe6e6db9161e69271f56daaafdaf2add023b1
> > Author: David Weinehall <david.weinehall@linux.intel.com>
> > Date:   Tue Aug 4 16:55:52 2015 +0300
> >
> >     drm/i915: Allow parsing of variable size child device entries from VBT
> >
> > That commit is not valid for v4.2, however it will be valid for v4.3. It
> > was simply queued too early.
> 
> Nope, this patch from David also incidentally fixes a regression from
> 90e4f1592bb6e82f6690f0e05a8aadc which is why I merged it to. If you
> want to revert this, you need to revert more (and with that also break
> BSW).
> 
> Isn't there a more minimal fix we can do for 4.2?
I proposed a very minimal solution just to increase the size of the raw
buffer to the maximum of 38 bytes so we could later on parse the correct
info.

May not be relevant anymore

https://bugs.freedesktop.org/show_bug.cgi?id=91269

-Mika-

> -Daniel
> 
> >
> > The referenced regressing commit is just fine until the size of struct
> > common_child_dev_config changes, and that won't happen until
> > v4.3. Indeed, the expected size checks here rely on the increased size
> > of the struct, breaking new platforms.
> >
> > Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: David Weinehall <david.weinehall@linux.intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >
> > ---
> >
> > There was another patch from David increasing the size of the struct
> > [1], but that then regresses sdvo mapping parsing. It's the simplest to
> > just revert and fix this up properly for v4.3.
> >
> > [1] http://mid.gmane.org/20150813131415.GO6150@boom
> > ---
> >  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
> >  1 file changed, 4 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> > index 3dcd59e694db..198fc3c3291b 100644
> > --- a/drivers/gpu/drm/i915/intel_bios.c
> > +++ b/drivers/gpu/drm/i915/intel_bios.c
> > @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >         const union child_device_config *p_child;
> >         union child_device_config *child_dev_ptr;
> >         int i, child_device_num, count;
> > -       u8 expected_size;
> > -       u16 block_size;
> > +       u16     block_size;
> >
> >         p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
> >         if (!p_defs) {
> >                 DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
> >                 return;
> >         }
> > -       if (bdb->version < 195) {
> > -               expected_size = 33;
> > -       } else if (bdb->version == 195) {
> > -               expected_size = 37;
> > -       } else if (bdb->version <= 197) {
> > -               expected_size = 38;
> > -       } else {
> > -               expected_size = 38;
> > -               DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
> > -                                expected_size, bdb->version);
> > -       }
> > -
> > -       if (expected_size > sizeof(*p_child)) {
> > -               DRM_ERROR("child_device_config cannot fit in p_child\n");
> > -               return;
> > -       }
> > -
> > -       if (p_defs->child_dev_size != expected_size) {
> > -               DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
> > -                         p_defs->child_dev_size, expected_size, bdb->version);
> > +       if (p_defs->child_dev_size < sizeof(*p_child)) {
> > +               DRM_ERROR("General definiton block child device size is too small.\n");
> >                 return;
> >         }
> >         /* get the block size of general definitions */
> > @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
> >
> >                 child_dev_ptr = dev_priv->vbt.child_dev + count;
> >                 count++;
> > -               memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
> > +               memcpy(child_dev_ptr, p_child, sizeof(*p_child));
> >         }
> >         return;
> >  }
> > --
> > 2.1.4
> >
> 
> 
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH for v4.2] Revert "drm/i915: Allow parsing of variable size child device entries from VBT"

[Jani Nikula]

On Wed, 19 Aug 2015, Mika Kahola <mika.kahola@intel.com> wrote:
> On Tue, 2015-08-18 at 09:58 -0700, Daniel Vetter wrote:
>> On Tue, Aug 18, 2015 at 2:33 AM, Jani Nikula <jani.nikula@intel.com> wrote:
>> > This reverts
>> >
>> > commit 047fe6e6db9161e69271f56daaafdaf2add023b1
>> > Author: David Weinehall <david.weinehall@linux.intel.com>
>> > Date:   Tue Aug 4 16:55:52 2015 +0300
>> >
>> >     drm/i915: Allow parsing of variable size child device entries from VBT
>> >
>> > That commit is not valid for v4.2, however it will be valid for v4.3. It
>> > was simply queued too early.
>> 
>> Nope, this patch from David also incidentally fixes a regression from
>> 90e4f1592bb6e82f6690f0e05a8aadc which is why I merged it to. If you
>> want to revert this, you need to revert more (and with that also break
>> BSW).
>> 
>> Isn't there a more minimal fix we can do for 4.2?
> I proposed a very minimal solution just to increase the size of the raw
> buffer to the maximum of 38 bytes so we could later on parse the correct
> info.

That doesn't work because it breaks parse_sdvo_device_mapping(). I've
fixed this now in drm-intel-next-fixes to be queued for v4.3, but I do
not want to start patching up broken commits at v4.2-rc7 stage.

As I've said, if Ville's commit 90e4f1592bb6 really does break stuff in
v4.2 (*), David's commit 047fe6e6db91 does not fix it in v4.2 anyway.

Thus I've applied the revert. It is the minimal fix. v4.2 will be
fine. For v4.3 I'll need to make a backmerge from drm-intel-fixes to
drm-intel-next-fixes, and we can move on to fixing v4.3.

BR,
Jani.


(*) which I highly doubt in the first place; likely this was seen in
-nightly and erroneously attributed as a regression on 90e4f1592bb6.



>
> May not be relevant anymore
>
> https://bugs.freedesktop.org/show_bug.cgi?id=91269
>
> -Mika-
>
>> -Daniel
>> 
>> >
>> > The referenced regressing commit is just fine until the size of struct
>> > common_child_dev_config changes, and that won't happen until
>> > v4.3. Indeed, the expected size checks here rely on the increased size
>> > of the struct, breaking new platforms.
>> >
>> > Fixes: 047fe6e6db91 ("drm/i915: Allow parsing of variable size child device entries from VBT")
>> > Cc: Daniel Vetter <daniel@ffwll.ch>
>> > Cc: David Weinehall <david.weinehall@linux.intel.com>
>> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >
>> > ---
>> >
>> > There was another patch from David increasing the size of the struct
>> > [1], but that then regresses sdvo mapping parsing. It's the simplest to
>> > just revert and fix this up properly for v4.3.
>> >
>> > [1] http://mid.gmane.org/20150813131415.GO6150@boom
>> > ---
>> >  drivers/gpu/drm/i915/intel_bios.c | 27 ++++-----------------------
>> >  1 file changed, 4 insertions(+), 23 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> > index 3dcd59e694db..198fc3c3291b 100644
>> > --- a/drivers/gpu/drm/i915/intel_bios.c
>> > +++ b/drivers/gpu/drm/i915/intel_bios.c
>> > @@ -1075,34 +1075,15 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>> >         const union child_device_config *p_child;
>> >         union child_device_config *child_dev_ptr;
>> >         int i, child_device_num, count;
>> > -       u8 expected_size;
>> > -       u16 block_size;
>> > +       u16     block_size;
>> >
>> >         p_defs = find_section(bdb, BDB_GENERAL_DEFINITIONS);
>> >         if (!p_defs) {
>> >                 DRM_DEBUG_KMS("No general definition block is found, no devices defined.\n");
>> >                 return;
>> >         }
>> > -       if (bdb->version < 195) {
>> > -               expected_size = 33;
>> > -       } else if (bdb->version == 195) {
>> > -               expected_size = 37;
>> > -       } else if (bdb->version <= 197) {
>> > -               expected_size = 38;
>> > -       } else {
>> > -               expected_size = 38;
>> > -               DRM_DEBUG_DRIVER("Expected child_device_config size for BDB version %u not known; assuming %u\n",
>> > -                                expected_size, bdb->version);
>> > -       }
>> > -
>> > -       if (expected_size > sizeof(*p_child)) {
>> > -               DRM_ERROR("child_device_config cannot fit in p_child\n");
>> > -               return;
>> > -       }
>> > -
>> > -       if (p_defs->child_dev_size != expected_size) {
>> > -               DRM_ERROR("Size mismatch; child_device_config size=%u (expected %u); bdb->version: %u\n",
>> > -                         p_defs->child_dev_size, expected_size, bdb->version);
>> > +       if (p_defs->child_dev_size < sizeof(*p_child)) {
>> > +               DRM_ERROR("General definiton block child device size is too small.\n");
>> >                 return;
>> >         }
>> >         /* get the block size of general definitions */
>> > @@ -1149,7 +1130,7 @@ parse_device_mapping(struct drm_i915_private *dev_priv,
>> >
>> >                 child_dev_ptr = dev_priv->vbt.child_dev + count;
>> >                 count++;
>> > -               memcpy(child_dev_ptr, p_child, p_defs->child_dev_size);
>> > +               memcpy(child_dev_ptr, p_child, sizeof(*p_child));
>> >         }
>> >         return;
>> >  }
>> > --
>> > 2.1.4
>> >
>> 
>> 
>> 
>
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx