| Message ID | d8a2998977feee2f5b5ad609aaca787adfb41479.1450702954.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > 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? Should we also check that the payload length is below the specified max of 240 bytes? > 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
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
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. > > 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
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? 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
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
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; break; + case MIPI_SEQ_ELEM_I2C: + if (index + 7 > total) + return 0; + len = *(data + index + 6) + 7; + break; 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 };
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(-)