| Message ID | 1439890416-16826-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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
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
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
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 > > > > >
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 >> > >> >> >> > >
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; }
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(-)