[Intel-gfx] [PATCH 09/15] drm/i915/bios: interpret the i2c element

[Ville Syrjälä]

On Thu, Jan 07, 2016 at 11:31:25AM +0200, Jani Nikula wrote:
> On Tue, 05 Jan 2016, Ville Syrjälä <ville.syrjala at 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 at 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.

I don't really see where the 240 comes from. The spec lists 240 byte
payload size limit for i2c, spi, and send packet operations, but the
size of the header is different for those so I can't see how all
would end up with the same payload length limitation. So to me it seems
like the max payload limit should be 255-7 for i2c, 255-6 for spi, and
255-4 for send packet (since the "size of operation" byte doesn't seem
to include itself or the "operation byte"). So to me the 240 seems like
a totally arbitrary limit.

> 
> 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 at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC