>-----Original Message-----
>From: Jani Nikula [mailto:jani.nik...@linux.intel.com]
>Sent: Thursday, September 17, 2015 8:09 PM
>To: Deepak, M; intel-gfx@lists.freedesktop.org
>Cc: Deepak, M
>Subject: Re: [Intel-gfx] [MIPI SEQ PARSING v2 PATCH 05/11] drm/i915: Added
>support the v3 mipi sequence block
>
>On Thu, 10 Sep 2015, Deepak M <m.dee...@intel.com> wrote:
>> From: vkorjani <vikas.korj...@intel.com>
>>
>> The Block 53 of the VBT, which is the MIPI sequence block has
>> undergone a design change because of which the parsing logic has to be
>> changed.
>>
>> The current code will handle the parsing of v3 and other lower
>> versions of the MIPI sequence block.
>>
>> v2: rebase
>>
>> Signed-off-by: vkorjani <vikas.korj...@intel.com>
>> Signed-off-by: Deepak M <m.dee...@intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_bios.c | 119
>+++++++++++++++++++++++-----
>> drivers/gpu/drm/i915/intel_bios.h | 8 ++
>> drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 7 ++
>> 3 files changed, 114 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.c
>> b/drivers/gpu/drm/i915/intel_bios.c
>> index 34a1042..cea641f 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.c
>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>> @@ -45,6 +45,7 @@ find_section(struct drm_i915_private *dev_priv,
>> int index = 0;
>> u32 total, current_size;
>> u8 current_id;
>> + u8 version;
>>
>> /* skip to first section */
>> index += bdb->header_size;
>> @@ -56,7 +57,17 @@ find_section(struct drm_i915_private *dev_priv,
>> current_id = *(base + index);
>> index++;
>>
>> - current_size = *((const u16 *)(base + index));
>> + if (current_id == BDB_MIPI_SEQUENCE) {
>> + version = *(base + index + 2);
>> + if (version >= 3)
>> + current_size = *((const u32 *)(base +
>> + index + 3));
>> + else
>> + current_size = *((const u16 *)(base + index));
>> + } else {
>> + current_size = *((const u16 *)(base + index));
>> + }
>> +
>
>While reviewing I've realized the old kernels will hit this hard. I've
>submitted a
>patch [1] to be applied to v4.3-rc and older stable kernels so that they fail
>gracefully instead of starting to parse garbage. The real parsing is too big to
>backport to upstream stable. Please review.
>
>[1] http://mid.gmane.org/1442497327-27033-1-git-send-email-
>jani.nik...@intel.com
>
>> index += 2;
>>
>> if (index + current_size > total)
>> @@ -745,6 +756,55 @@ static u8 *goto_next_sequence(u8 *data, int *size)
>> return data;
>> }
>>
>> +static u8 *goto_next_sequence_v3(u8 *data, int *size) {
>> + int tmp = *size;
>> + int op_size;
>> +
>> + if (--tmp < 0)
>> + return NULL;
>> +
>> + /* Skip the panel id and the sequence size */
>
>It's not panel id, it's the sequence byte, right?
>
>You could also store data + 1 + size of sequence, and check whether data ends
>up pointing at the same place in the end. They should.
>
>Shouldn't you also take 4 bytes of sequence size field into account in tmp?
>
>> + data = data + 5;
>> + while (*data != 0) {
>> + u8 element_type = *data++;
>> +
>> + switch (element_type) {
>
>Would be helpful to refer to operation_byte like in the spec.
>
>> + default:
>> + DRM_ERROR("Unknown element type %d\n",
>element_type);
>> + case MIPI_SEQ_ELEM_SEND_PKT:
>> + case MIPI_SEQ_ELEM_DELAY:
>> + case MIPI_SEQ_ELEM_GPIO:
>> + case MIPI_SEQ_ELEM_I2C:
>> + case MIPI_SEQ_ELEM_SPI:
>> + case MIPI_SEQ_ELEM_PMIC:
>> + /*
>> + * skip by this element payload size
>> + * skip elem id, command flag and data type
>> + */
>> + op_size = *data++;
>> + tmp = tmp - (op_size + 1);
>> + if (tmp < 0)
>> + return NULL;
>
>Isn't each operation operation byte | size of operation | payload size, i.e.
>your
>tmp change is one byte short?
>
>The fact that the goto_next_sequence* functions increase data and decrease
>size is getting increasingly confusing to follow. One simple alternative would
>be to calculate some endp = start + size up front, and then pass the endp
>around, and check if we're about to go past the end marker.
>
>This is not a problem with your series, it was there already. And fixing it
>doesn't have to be part of your series. It just really takes ages to review
>this
>approach of range checking. Unless I close my eyes and trust there are no off-
>by-ones anywhere. But that kind of defeats the purpose of review...
>
[Deepak M] Okay will try to simplify the logic.
>> +
>> + /* skip by len */
>> + data += op_size;
>> + break;
>> + }
>> + }
>> +
>> + /* goto next sequence or end of block byte */
>> + if (--tmp < 0)
>> + return NULL;
>> +
>> + /* Skip the end element marker */
>> + data++;
>> +
>> + /* update amount of data left for the sequence block to be parsed */
>> + *size = tmp;
>> + return data;
>> +}
>> +
>> static void
>> parse_mipi(struct drm_i915_private *dev_priv, const struct bdb_header
>> *bdb) { @@ -754,7 +814,7 @@ parse_mipi(struct drm_i915_private
>> *dev_priv, const struct bdb_header *bdb)
>> const struct mipi_pps_data *pps;
>> u8 *data;
>> const u8 *seq_data;
>> - int i, panel_id, seq_size;
>> + int i, panel_id, panel_seq_size;
>> u16 block_size;
>>
>> /* parse MIPI blocks only if LFP type is MIPI */ @@ -811,29 +871,40
>> @@ parse_mipi(struct drm_i915_private *dev_priv, const struct
>> bdb_header *bdb)
>>
>> DRM_DEBUG_DRIVER("Found MIPI sequence block\n");
>>
>> - block_size = get_blocksize(sequence);
>> -
>> /*
>> * parse the sequence block for individual sequences
>> */
>> dev_priv->vbt.dsi.seq_version = sequence->version;
>>
>> seq_data = &sequence->data[0];
>> + if (dev_priv->vbt.dsi.seq_version >= 3) {
>> + block_size = *((unsigned int *)seq_data);
>
>(const u32 *)
>
>> + seq_data = seq_data + 4;
>> + } else
>> + block_size = get_blocksize(sequence);
>
>block_size should be changed to u32.
>
>>
>> /*
>> * sequence block is variable length and hence we need to parse and
>> * get the sequence data for specific panel id
>> */
>> for (i = 0; i < MAX_MIPI_CONFIGURATIONS; i++) {
>> - panel_id = *seq_data;
>> - seq_size = *((u16 *) (seq_data + 1));
>> + panel_id = *seq_data++;
>> + if (dev_priv->vbt.dsi.seq_version >= 3) {
>> + panel_seq_size = *((u32 *)seq_data);
>> + seq_data += sizeof(u32);
>> + } else {
>> + panel_seq_size = *((u16 *)seq_data);
>> + seq_data += sizeof(u16);
>> + }
>> +
>> if (panel_id == panel_type)
>> break;
>>
>> - /* skip the sequence including seq header of 3 bytes */
>> - seq_data = seq_data + 3 + seq_size;
>> + seq_data += panel_seq_size;
>> +
>> if ((seq_data - &sequence->data[0]) > block_size) {
>> - DRM_ERROR("Sequence start is beyond sequence
>block size, corrupted sequence block\n");
>> + DRM_ERROR("Sequence start is beyond seq block
>size\n");
>> + DRM_ERROR("Corrupted sequence block\n");
>
>Please don't add two consecutive DRM_ERRORs for the same error.
>
>> return;
>> }
>> }
>> @@ -845,13 +916,14 @@ parse_mipi(struct drm_i915_private *dev_priv,
>> const struct bdb_header *bdb)
>>
>> /* check if found sequence is completely within the sequence block
>> * just being paranoid */
>> - if (seq_size > block_size) {
>> + if (panel_seq_size > block_size) {
>> DRM_ERROR("Corrupted sequence/size, bailing out\n");
>> return;
>> }
>>
>> - /* skip the panel id(1 byte) and seq size(2 bytes) */
>> - dev_priv->vbt.dsi.data = kmemdup(seq_data + 3, seq_size,
>GFP_KERNEL);
>> +
>> + dev_priv->vbt.dsi.data = kmemdup(seq_data, panel_seq_size,
>> +GFP_KERNEL);
>> +
>> if (!dev_priv->vbt.dsi.data)
>> return;
>>
>> @@ -860,29 +932,36 @@ parse_mipi(struct drm_i915_private *dev_priv,
>const struct bdb_header *bdb)
>> * There are only 5 types of sequences as of now
>> */
>> data = dev_priv->vbt.dsi.data;
>> - dev_priv->vbt.dsi.size = seq_size;
>> + dev_priv->vbt.dsi.size = panel_seq_size;
>>
>> /* two consecutive 0x00 indicate end of all sequences */
>> - while (1) {
>> + while (*data != 0) {
>> int seq_id = *data;
>> + int seq_size;
>
>u32
>
>> +
>> if (MIPI_SEQ_MAX > seq_id && seq_id >
>MIPI_SEQ_UNDEFINED) {
>> dev_priv->vbt.dsi.sequence[seq_id] = data;
>> DRM_DEBUG_DRIVER("Found mipi sequence - %d\n",
>seq_id);
>> } else {
>> - DRM_ERROR("undefined sequence\n");
>> - goto err;
>> + DRM_ERROR("undefined sequence - %d\n", seq_id);
>> + seq_size = *(data + 1);
>
>Needs to be *((const u32 *)(data + 1)) or you'll ignore 3 highest order bytes.
>
>> + if (dev_priv->vbt.dsi.seq_version >= 3) {
>> + data = data + seq_size + 1;
>> + continue;
>> + } else
>> + goto err;
>> }
>>
>> /* partial parsing to skip elements */
>> - data = goto_next_sequence(data, &seq_size);
>> + if (dev_priv->vbt.dsi.seq_version >= 3)
>> + data = goto_next_sequence_v3(data,
>&panel_seq_size);
>> + else
>> + data = goto_next_sequence(data, &panel_seq_size);
>>
>> if (data == NULL) {
>> DRM_ERROR("Sequence elements going beyond
>block itself. Sequence block parsing failed\n");
>> goto err;
>> }
>> -
>> - if (*data == 0)
>> - break; /* end of sequence reached */
>> }
>>
>> DRM_DEBUG_DRIVER("MIPI related vbt parsing complete\n"); diff --
>git
>> a/drivers/gpu/drm/i915/intel_bios.h
>> b/drivers/gpu/drm/i915/intel_bios.h
>> index 21a7f3f..7a4ba41 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -948,6 +948,12 @@ enum mipi_seq {
>> MIPI_SEQ_DISPLAY_ON,
>> MIPI_SEQ_DISPLAY_OFF,
>> MIPI_SEQ_DEASSERT_RESET,
>> + MIPI_SEQ_BACKLIGHT_ON,
>> + MIPI_SEQ_BACKLIGHT_OFF,
>> + MIPI_SEQ_TEAR_ON,
>> + MIPI_SEQ_TEAR_OFF,
>> + MIPI_SEQ_POWER_ON,
>> + MIPI_SEQ_POWER_OFF,
>> MIPI_SEQ_MAX
>> };
>>
>> @@ -957,6 +963,8 @@ enum mipi_seq_element {
>> MIPI_SEQ_ELEM_DELAY,
>> MIPI_SEQ_ELEM_GPIO,
>> MIPI_SEQ_ELEM_I2C,
>> + MIPI_SEQ_ELEM_SPI,
>> + MIPI_SEQ_ELEM_PMIC,
>> MIPI_SEQ_ELEM_STATUS,
>
>Again, MIPI_SEQ_ELEM_STATUS is not spec.
>
>> MIPI_SEQ_ELEM_MAX
>> };
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 9989f61..c6a6fa1 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -317,6 +317,8 @@ static const char * const seq_name[] = {
>>
>> static void generic_exec_sequence(struct intel_dsi *intel_dsi, const
>> u8 *data) {
>> + struct drm_device *dev = intel_dsi->base.base.dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> fn_mipi_elem_exec mipi_elem_exec;
>> int index;
>>
>> @@ -327,6 +329,8 @@ static void generic_exec_sequence(struct intel_dsi
>> *intel_dsi, const u8 *data)
>>
>> /* go to the first element of the sequence */
>> data++;
>> + if (dev_priv->vbt.dsi.seq_version >= 3)
>> + data = data + 4;
>>
>> /* parse each byte till we reach end of sequence byte - 0x00 */
>> while (1) {
>> @@ -340,6 +344,9 @@ static void generic_exec_sequence(struct intel_dsi
>*intel_dsi, const u8 *data)
>> /* goto element payload */
>> data++;
>>
>> + if (dev_priv->vbt.dsi.seq_version >= 3)
>> + data++;
>> +
>> /* execute the element specific rotines */
>> data = mipi_elem_exec(intel_dsi, data);
>>
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> 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
