[Intel-gfx] [PATCH] drm/i915/bios: Fix the sequence size calculations for MIPI seq v3

[Daniel Vetter]

On Tue, Jan 19, 2016 at 07:55:58PM +0200, Jani Nikula wrote:
> On Tue, 19 Jan 2016, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Fri, Jan 15, 2016 at 11:51:31AM +0200, Jani Nikula wrote:
> >> On Thu, 14 Jan 2016, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> >> > On Thu, Jan 14, 2016 at 05:12:07PM +0200, Jani Nikula wrote:
> >> >> Two errors in a single line. The size was read from the wrong offset,
> >> >> and the end index didn't take the five bytes for sequence byte and size
> >> >> of sequence into account. Fix it all, and break up the calculations a
> >> >> bit to make it clearer.
> >> >> 
> >> >> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> >> Reported-by: Mika Kahola <mika.kahola at intel.com>
> >> >> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3")
> >> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_bios.c | 17 ++++++++++++++---
> >> >>  1 file changed, 14 insertions(+), 3 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >> >> index 12e2f8b8bf9c..bf62a19c8f69 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_bios.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_bios.c
> >> >> @@ -842,6 +842,7 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total)
> >> >>  {
> >> >>  	int seq_end;
> >> >>  	u16 len;
> >> >> +	u32 size_of_sequence;
> >> >>  
> >> >>  	/*
> >> >>  	 * Could skip sequence based on Size of Sequence alone, but also do some
> >> >> @@ -852,14 +853,24 @@ static int goto_next_sequence_v3(const u8 *data, int index, int total)
> >> >>  		return 0;
> >> >>  	}
> >> >>  
> >> >> -	seq_end = index + *((const u32 *)(data + 1));
> >> >> +	/* Skip Sequence Byte. */
> >> >> +	index++;
> >> >> +
> >> >> +	/*
> >> >> +	 * Size of Sequence. Excludes the Sequence Byte and the size itself,
> >> >> +	 * includes MIPI_SEQ_ELEM_END byte, excludes the final MIPI_SEQ_END
> >> >> +	 * byte.
> >> >> +	 */
> >> >> +	size_of_sequence = *((const uint32_t *)(data + index));
> >> >
> >> > Hmm. So it was reading from 'data+1' and now it's basically 'data+index+1'.
> >> > So it was correct for the first sequence, and busted for later ones I
> >> > suppose.
> >> >
> >> >> +	index += 4;
> >> >> +
> >> >> +	seq_end = index + size_of_sequence;
> >> >
> >> > And now we count the size of the sequence starting from the operation
> >> > byte, before we counted it from the sequence byte. "Fortunately" the spec
> >> > doesn't even tell us which is correct. If it works, it works.
> >> >
> >> > Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >> 
> >> Pushed to drm-intel-next-queued, thanks for the review and testing.
> >
> > You failed bat CI. Please make that the failure really is pre-existing and
> > if so dig out the bugzilla for it. If that's not the case please revert.
> 
> Hey, can't apply new rules after the fact. Seriously.
> 
> I looked at the results before pushing and observed they were bogus wrt
> this patch. The changed code shouldn't be run on any of the CI machines,
> and even if it were run (e.g. due to bogus BIOS), none of the CI
> machines have DSI displays where the change would matter (a CI fail of a
> bigger scale).
> 
> I was about as worried for the test results change as I would have been
> for a pure comment update.

It's not just about your patch, but to make sure we do have all the random
noise tracked somewhere. Because there's going to be another person who'll
run into this, and for him/her this will again be random noise no one
seems to care about.

This specific bug is tracked already in

https://bugs.freedesktop.org/show_bug.cgi?id=93699

And yes I did check the backtraces to make sure it's indeed a match. If
you want to object to this please raise it in Jesse's meeting this week.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch