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

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

[Jani Nikula]

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));
+	index += 4;
+
+	seq_end = index + size_of_sequence;
 	if (seq_end > total) {
 		DRM_ERROR("Invalid sequence size\n");
 		return 0;
 	}
 
-	/* Skip Sequence Byte and Size of Sequence. */
-	for (index = index + 5; index < total; index += len) {
+	for (; index < total; index += len) {
 		u8 operation_byte = *(data + index);
 		index++;
 
-- 
2.1.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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>

BTW I was thinking that we could maybe add some kind of
"read the thing at index, and and increment the index past it" helpers.

Eg.
int get_u8(const void *data, int index, int size, u8 *ret);
int get_u32(const void *data, int index, int size, u32 *ret);

they could also do the index vs. size check and just return an error if
we try to go too far.

>  	if (seq_end > total) {
>  		DRM_ERROR("Invalid sequence size\n");
>  		return 0;
>  	}
>  
> -	/* Skip Sequence Byte and Size of Sequence. */
> -	for (index = index + 5; index < total; index += len) {
> +	for (; index < total; index += len) {
>  		u8 operation_byte = *(data + index);
>  		index++;
>  
> -- 
> 2.1.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ failure: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 8fb2feecca499d11e104264071ac55e273e23af5 drm-intel-nightly: 2016y-01m-14d-13h-06m-44s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (skl-i5k-2) UNSTABLE
                dmesg-warn -> PASS       (bdw-nuci7)
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (bdw-ultra)

bdw-nuci7        total:138  pass:129  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:138  pass:131  dwarn:1   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:141  pass:115  dwarn:2   dfail:0   fail:0   skip:24 
hsw-brixbox      total:141  pass:134  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:141  pass:137  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:141  pass:100  dwarn:4   dfail:0   fail:0   skip:37 
ivb-t430s        total:135  pass:122  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:141  pass:132  dwarn:1   dfail:0   fail:0   skip:8  
snb-dellxps      total:141  pass:122  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:141  pass:122  dwarn:5   dfail:0   fail:1   skip:13 

HANGED skl-i7k-2 in Patchwork_1190/skl-i7k-2/tests/115.json:igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b Patchwork_1190/skl-i7k-2/tests/116.json:igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c Patchwork_1190/skl-i7k-2/tests/117.json:igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a Patchwork_1190/skl-i7k-2/tests/118.json:igt@gem_ctx_create@basic Patchwork_1190/skl-i7k-2/tests/119.json:igt@gem_ctx_param_basic@invalid-ctx-get Patchwork_1190/skl-i7k-2/tests/120.json:igt@kms_addfb_basic@size-max Patchwork_1190/skl-i7k-2/tests/121.json:igt@kms_addfb_basic@no-handle Patchwork_1190/skl-i7k-2/tests/122.json:igt@kms_pipe_crc_basic@read-crc-pipe-a Patchwork_1190/skl-i7k-2/tests/123.json:igt@kms_pipe_crc_basic@read-crc-pipe-c Patchwork_1190/skl-i7k-2/tests/124.json:igt@kms_pipe_crc_basic@read-crc-pipe-b Patchwork_1190/skl-i7k-2/tests/125.json:igt@kms_addfb_basic@bad-pitch-256 Patchwork_1190/skl-i7k-2/tests/126.json:igt@gem_mmap_gtt@basic-write-gtt Patchwork_1190/skl-i7k-2/tests/127.json:igt@kms_s
 etmode@basic-clone-single-crtc Patchwork_1190/skl-i7k-2/tests/128.json:igt@kms_addfb_basic@addfb25-x-tiled Patchwork_1190/skl-i7k-2/tests/129.json:igt@gem_basic@bad-close Patchwork_1190/skl-i7k-2/tests/130.json:igt@gem_render_linear_blits@basic Patchwork_1190/skl-i7k-2/tests/131.json:igt@gem_mmap_gtt@basic-copy Patchwork_1190/skl-i7k-2/tests/132.json:igt@kms_addfb_basic@small-bo Patchwork_1190/skl-i7k-2/tests/133.json:igt@kms_addfb_basic@basic Patchwork_1190/skl-i7k-2/tests/134.json:igt@kms_flip@basic-flip-vs-modeset Patchwork_1190/skl-i7k-2/tests/135.json:igt@kms_addfb_basic@unused-pitches Patchwork_1190/skl-i7k-2/tests/136.json:igt@kms_addfb_basic@bo-too-small-due-to-tiling Patchwork_1190/skl-i7k-2/tests/137.json:igt@gem_storedw_loop@basic-blt Patchwork_1190/skl-i7k-2/tests/138.json:igt@drv_getparams_basic@basic-eu-total Patchwork_1190/skl-i7k-2/tests/139.json:igt@gem_ctx_param_basic@invalid-ctx-set Patchwork_1190/skl-i7k-2/tests/140.json:igt@kms_pipe_crc_basic@read-crc-pipe-a-fra
 me-sequence

Results at /archive/results/CI_IGT_test/Patchwork_1190/

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mika Kahola]

On Thu, 2016-01-14 at 17:12 +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.
> 
Tested-by: Mika Kahola <mika.kahola@intel.com>
> 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));
> +	index += 4;
> +
> +	seq_end = index + size_of_sequence;
>  	if (seq_end > total) {
>  		DRM_ERROR("Invalid sequence size\n");
>  		return 0;
>  	}
>  
> -	/* Skip Sequence Byte and Size of Sequence. */
> -	for (index = index + 5; index < total; index += len) {
> +	for (; index < total; index += len) {
>  		u8 operation_byte = *(data + index);
>  		index++;
>  


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

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.

BR,
Jani.

>
> BTW I was thinking that we could maybe add some kind of
> "read the thing at index, and and increment the index past it" helpers.
>
> Eg.
> int get_u8(const void *data, int index, int size, u8 *ret);
> int get_u32(const void *data, int index, int size, u32 *ret);
>
> they could also do the index vs. size check and just return an error if
> we try to go too far.
>
>>  	if (seq_end > total) {
>>  		DRM_ERROR("Invalid sequence size\n");
>>  		return 0;
>>  	}
>>  
>> -	/* Skip Sequence Byte and Size of Sequence. */
>> -	for (index = index + 5; index < total; index += len) {
>> +	for (; index < total; index += len) {
>>  		u8 operation_byte = *(data + index);
>>  		index++;
>>  
>> -- 
>> 2.1.4

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

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

[Daniel Vetter]

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.
I'll paste you the link to the internal wiki in private.
-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

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

[Jani Nikula]

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

Re: [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@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.

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx