Re: [PATCH 09/15] drm/i915/bios: interpret the i2c element

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

On Tue, 05 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 21, 2015 at 03:11:00PM +0200, Jani Nikula wrote:
>> Add parsing of the i2c element, defined in MIPI sequence block v2. Drop
>> the status operation byte while at it, that does not exist.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.c | 5 +++++
>>  drivers/gpu/drm/i915/intel_bios.h | 2 +-
>>  2 files changed, 6 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>> index d6eaf32f33e5..45a7a2bc96c6 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -812,6 +812,11 @@ static int goto_next_sequence(const u8 *data, int index, int total)
>>  		case MIPI_SEQ_ELEM_GPIO:
>>  			len = 2;
>
> Somewhat off topic, but I wonder if this is correct. The "structure"
> diagram shows it as 2 bytes for v1 and v2, but I'm not sure that section
> isn't there just as an example. Later the describing the GPIO block it
> seems to say it's 2 bytes for v1, and three bytes for v2.

I've held on to some old spec versions (bdb version 177 has sequence v1,
bdb version 185 has sequence v2). Both v1 and v2 have 2 bytes payload
for the GPIO element.

The *meaning* of the first of those bytes has changed from v1->v2
though. Can't do much about that here, it's up to the generic vbt dsi
"driver"...

>
>>  			break;
>> +		case MIPI_SEQ_ELEM_I2C:
>> +			if (index + 7 > total)
>> +				return 0;
>> +			len = *(data + index + 6) + 7;
>> +			break;
>
> This one isn't show in the structure diagrams at all, so I guess we get
> to trust the other section. It says this was introduced in v2. Should be
> add a paranoia check for that?

The spec with bdb version 185 has this.

I contemplated adding a check for v2, but then I thought it probably
doesn't really matter all that much. If we get an i2c elem with v1 and
reject it, we'll probably end up with a black screen. If we just assume
it's an i2c element but it isn't, it'll trip over something else later.

> Should we also check that the payload length is below the specified max
> of 240 bytes?

You'll love this. In v2 the max is actually the whole byte i.e. 255. In
v3 they added a length field for these operations for forward
compatibility (can now skip unknown new operations). And that's 8
bits. So the header + payload for the i2c data can't exceed 255, so
there's now an arbitrary 240 byte limit for i2c payload in v3.

BR,
Jani.


>
>>  		default:
>>  			DRM_ERROR("Unknown operation byte\n");
>>  			return 0;
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 4e87df16e7b3..411b33794536 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -968,7 +968,7 @@ enum mipi_seq_element {
>>  	MIPI_SEQ_ELEM_SEND_PKT,
>>  	MIPI_SEQ_ELEM_DELAY,
>>  	MIPI_SEQ_ELEM_GPIO,
>> -	MIPI_SEQ_ELEM_STATUS,
>> +	MIPI_SEQ_ELEM_I2C,		/* sequence block v2+ */
>>  	MIPI_SEQ_ELEM_MAX
>>  };
>>  
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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