Re: [PATCH 14/15] drm/i915/dsi: skip unknown elements for sequence block v3+

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 05 Jan 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 21, 2015 at 03:11:05PM +0200, Jani Nikula wrote:
>> The sequence block has sizes of elements after the operation byte since
>> sequence block v3. Use it to skip elements we don't support yet.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 43 +++++++++++++++++-------------
>>  1 file changed, 24 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index eabfd9eb9cc0..1f9c80d21904 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -335,31 +335,36 @@ static void generic_exec_sequence(struct intel_dsi *intel_dsi, const u8 *data)
>>  	if (dev_priv->vbt.dsi.seq_version >= 3)
>>  		data += 4;
>>  
>> -	/* parse each byte till we reach end of sequence byte - 0x00 */
>>  	while (1) {
>>  		u8 operation_byte = *data++;
>> -		if (operation_byte >= ARRAY_SIZE(exec_elem) ||
>> -		    !exec_elem[operation_byte]) {
>> +		u8 operation_size = 0;
>> +
>> +		if (operation_byte == MIPI_SEQ_ELEM_END)
>> +			break;
>> +
>> +		if (operation_byte < ARRAY_SIZE(exec_elem) &&
>> +		    exec_elem[operation_byte])
>
> && exec_elem[operation_byte] is redundant since you assing it to NULL
> anyway in the else clause. While I bikeshed: Might look prettier with ?:

Silly me.

I might prefer an if-else if the ?: doesn't fit on one line.

>
>
>> +			mipi_elem_exec = exec_elem[operation_byte];
>> +		else
>> +			mipi_elem_exec = NULL;
>> +
>> +		/* Size of Operation. */
>> +		if (dev_priv->vbt.dsi.seq_version >= 3)
>> +			operation_size = *data++;
>> +
>> +		if (mipi_elem_exec) {
>> +			data = mipi_elem_exec(intel_dsi, data);
>> +		} else if (operation_size) {
>> +			/* We have size, skip. */
>> +			DRM_DEBUG_KMS("Unsupported MIPI operation byte %u\n",
>> +				      operation_byte);
>
> DRM_ERROR, like in the other cases we fail to parse the sequence fully?

I suppose we could add that in intel_bios.c to do it a limited number of
times at driver load, but here it would keep spewing out the errors
every modeset and possibly more. This is not necessarily a showstopper
with sequence v3, as we have the size to move on to the next one.

BR,
Jani.

>
> Either way on both bikesheds: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>
>> +			data += operation_size;
>> +		} else {
>> +			/* No size, can't skip without parsing. */
>>  			DRM_ERROR("Unsupported MIPI operation byte %u\n",
>>  				  operation_byte);
>>  			return;
>>  		}
>> -		mipi_elem_exec = exec_elem[operation_byte];
>> -
>> -		/* Skip Size of Operation. */
>> -		if (dev_priv->vbt.dsi.seq_version >= 3)
>> -			data++;
>> -
>> -		/* execute the element specific rotines */
>> -		data = mipi_elem_exec(intel_dsi, data);
>> -
>> -		/*
>> -		 * After processing the element, data should point to
>> -		 * next element or end of sequence
>> -		 * check if have we reached end of sequence
>> -		 */
>> -		if (*data == 0x00)
>> -			break;
>>  	}
>>  }
>>  
>> -- 
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx