Re: [PATCH v10 1/4] drm: Introduce device wedged event

[Raag Jadav <raag.jadav@intel.com>]

On Thu, Dec 12, 2024 at 03:31:01PM -0300, André Almeida wrote:
> Hi Raag,
> 
> Thank you for your patch.
> 
> Em 28/11/2024 12:37, Raag Jadav escreveu:
> 
> [...]
> 
> > +int drm_dev_wedged_event(struct drm_device *dev, unsigned long method)
> > +{
> > +	const char *recovery = NULL;
> > +	unsigned int len, opt;
> > +	/* Event string length up to 28+ characters with available methods */
> > +	char event_string[32];
> > +	char *envp[] = { event_string, NULL };
> > +
> > +	len = scnprintf(event_string, sizeof(event_string), "%s", "WEDGED=");
> > +
> > +	for_each_set_bit(opt, &method, BITS_PER_TYPE(method)) {
> > +		recovery = drm_get_wedge_recovery(opt);
> > +		if (drm_WARN(dev, !recovery, "device wedged, invalid recovery method %u\n", opt))
> > +			break;
> > +
> > +		len += scnprintf(event_string + len, sizeof(event_string), "%s,", recovery);
> > +	}
> > +
> > +	if (recovery)
> > +		/* Get rid of trailing comma */
> > +		event_string[len - 1] = '\0';
> > +	else
> > +		/* Caller is unsure about recovery, do the best we can at this point. */
> > +		snprintf(event_string, sizeof(event_string), "%s", "WEDGED=unknown");
> > +
> > +	drm_info(dev, "device wedged, needs recovery\n");
> 
> As documented in the commit message "No explicit device recovery is expected
> from the consumer in this case", I think this should be like this:
> 
> if (method != DRM_WEDGE_RECOVERY_NONE)
>     drm_info(dev, "device wedged, needs recovery\n");
> 
> and maybe a note like this:
> 
> else
>     drm_info(dev, "device reseted, but managed to recover\n");

Or perhaps

	drm_info(dev, "device wedged, but recovered through reset\n");

> Either way, this patch is:
> 
> Reviewed-by: André Almeida <andrealmeid@igalia.com>

Thanks for the review.

Raag