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

[Jani Nikula <jani.nikula@intel.com>]

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