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

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

On Tue, 19 Jan 2016, Daniel Vetter <daniel@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@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@linux.intel.com>
>> >> Reported-by: Mika Kahola <mika.kahola@intel.com>
>> >> Fixes: 2a33d93486f2 ("drm/i915/bios: add support for MIPI sequence block v3")
>> >> Signed-off-by: Jani Nikula <jani.nikula@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@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.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx