>-----Original Message----- >From: Jani Nikula [mailto:jani.nikula@xxxxxxxxxxxxxxx] >Sent: Thursday, September 17, 2015 8:09 PM >To: Deepak, M; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: Deepak, M >Subject: Re: [MIPI SEQ PARSING v2 PATCH 05/11] drm/i915: Added >support the v3 mipi sequence block > >On Thu, 10 Sep 2015, Deepak M <m.deepak@xxxxxxxxx> wrote: >> From: vkorjani <vikas.korjani@xxxxxxxxx> >> >> 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.korjani@xxxxxxxxx> >> Signed-off-by: Deepak M <m.deepak@xxxxxxxxx> >> --- >> 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.nikula@xxxxxxxxx > >> 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >-- >Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
