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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jan 11, 2016 at 02:46:52PM +0200, Jani Nikula wrote:
> On Thu, 07 Jan 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Jan 07, 2016 at 11:31:25AM +0200, Jani Nikula wrote:
> >> 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.
> >
> > 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.
> 
> We're in agreement that the spec seems just whimsical about this.
> 
> However this patch is for mipi vbt sequence block v2, which doesn't have
> the limit. Can you r-b so we can move forward please?

Sure. I'll take your word on the v2 vs. v3 thing since the spec is
absolutely useless here.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> BR,
> Jani.
> 
> 
> 
> >
> >> 
> >> 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
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

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