Re: [PATCH 1/2] drm/i915/dsi: log send packet sequence errors

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

On Tue, Oct 28, 2025 at 05:57:11PM +0200, Jani Nikula wrote:
> We might be getting send packet sequence errors and never know. Log
> them.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 24 ++++++++++++--------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index 23402408e172..748e5462bd95 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -106,6 +106,7 @@ static const u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi,
>  	u8 type, flags, seq_port;
>  	u16 len;
>  	enum port port;
> +	ssize_t ret;
>  
>  	drm_dbg_kms(display->drm, "\n");
>  
> @@ -138,36 +139,41 @@ static const u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi,
>  
>  	switch (type) {
>  	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> -		mipi_dsi_generic_write(dsi_device, NULL, 0);
> +		ret = mipi_dsi_generic_write(dsi_device, NULL, 0);
>  		break;
>  	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> -		mipi_dsi_generic_write(dsi_device, data, 1);
> +		ret = mipi_dsi_generic_write(dsi_device, data, 1);
>  		break;
>  	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> -		mipi_dsi_generic_write(dsi_device, data, 2);
> +		ret = mipi_dsi_generic_write(dsi_device, data, 2);
>  		break;
>  	case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
>  	case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
>  	case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
> -		drm_dbg_kms(display->drm, "Generic Read not yet implemented or used\n");
> +		ret = -EOPNOTSUPP;
>  		break;
>  	case MIPI_DSI_GENERIC_LONG_WRITE:
> -		mipi_dsi_generic_write(dsi_device, data, len);
> +		ret = mipi_dsi_generic_write(dsi_device, data, len);
>  		break;
>  	case MIPI_DSI_DCS_SHORT_WRITE:
> -		mipi_dsi_dcs_write_buffer(dsi_device, data, 1);
> +		ret = mipi_dsi_dcs_write_buffer(dsi_device, data, 1);
>  		break;
>  	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> -		mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
> +		ret = mipi_dsi_dcs_write_buffer(dsi_device, data, 2);
>  		break;
>  	case MIPI_DSI_DCS_READ:
> -		drm_dbg_kms(display->drm, "DCS Read not yet implemented or used\n");
> +		ret = -EOPNOTSUPP;
>  		break;
>  	case MIPI_DSI_DCS_LONG_WRITE:
> -		mipi_dsi_dcs_write_buffer(dsi_device, data, len);
> +		ret = mipi_dsi_dcs_write_buffer(dsi_device, data, len);
>  		break;
>  	}
>  
> +	if (ret == -EOPNOTSUPP)

Do we know that the write functions never return this value?

> +		drm_dbg_kms(display->drm, "DSI read not supported\n");

Is there a reason you didn't make this a drm_err() as well?

We also have other debug messages related to unimplemented/unknown
sequences/etc that I think should all be drm_err(). Otherwise we'll
never find out if they're needed or not.

> +	else if (ret < 0)
> +		drm_err(display->drm, "DSI write failed with %pe\n", ERR_PTR(ret));
> +
>  	if (DISPLAY_VER(display) < 11)
>  		vlv_dsi_wait_for_fifo_empty(intel_dsi, port);
>  
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel