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, 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