Re: [PATCH v2] drm/i915: Per-DDI I_boost override

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH v2] drm/i915: Per-DDI I_boost override
Date: Mon, 6 Jul 2015 15:00:28 +0200	[thread overview]
Message-ID: <20150706130028.GA2156@phenom.ffwll.local> (raw)
In-Reply-To: <20150703153555.GG5176@intel.com>

On Fri, Jul 03, 2015 at 06:35:55PM +0300, Ville Syrjälä wrote:
> On Fri, Jul 03, 2015 at 04:25:15PM +0100, Damien Lespiau wrote:
> > On Fri, Jul 03, 2015 at 06:21:58PM +0300, Ville Syrjälä wrote:
> > > > In the old VBT spec I have, each child_dev_config is supposed to have
> > > > only 33 bytes. But in this patch you're increasing it to 38. I believe
> > > > this is what's causing the errors I see when I boot my BDW.
> > > > 
> > > > Are you sure they increased the VBT's ChildDevInfo to more than 33
> > > > bytes? I don't have access to your VBT spec right now, so I can't do a
> > > > proper review or a suggestion on how to fix the problem.
> > > 
> > > The size depends on the version. On BSW last I looked it was maybe 36 or
> > > 37 bytes.
> > > 
> > > I'm not sure we want to change common_child_dev_config since it's meant
> > > to be some kind of common stuff. So maybe add another member into the
> > > union for this stuff. Of course that still incrases the size of the union
> > > so other changes are required to make it work.
> > > 
> > > Fortunately we copy these things out from the VBT, so I think just
> > > making sure the memcpy() doesn't try to copy too much and fixing the size
> > > check should be all that's needed. That would leave any extra we didn't
> > > fill with the copy zeroed, and if having it zeroed isn't good enough
> > > we can of course sprinkle more version checks around.
> > > 
> > > And I guess we should still check that the size is at least 33 bytes
> > > since that was the official size for the longest time.
> > 
> > So child_dev_size lies to us?
> 
> No, it's correct. But we require it to be at least as big as our
> union child_device_config, which the patch changes from 33 to 37
> bytes.

Tbh I'd like us to be more paranoid about vbt, i.e. if they change struct
sizes I think we should have an if ladder (checking vbt
versions/platforms) to make sure that our expectation of the size matches
reality in all cases. Ofc that should be a separate patch to start this
series.

Given the state of vbt revisions handling and documentation I don't think
we can opt for too much safety checks here, sadly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx