On Wed, 19 Aug 2015, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Tue, Aug 18, 2015 at 10:00 AM, Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
>>> > This reverts
>>> >
>>> > commit 047fe6e6db9161e69271f56daaafdaf2add023b1
>>> > Author: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
>>> > 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@xxxxxxxxxxxxxxx>
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@xxxxxxxx>
>>> > Cc: David Weinehall <david.weinehall@xxxxxxxxxxxxxxx>
>>> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>> > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>>> >
>>> > ---
>>> >
>>> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
