Re: [PATCH 3/3] media: atomisp: Improve binary finding debug logging

[Hans de Goede <hdegoede@redhat.com>]

Hi Andy,

Thank you for the reviews.

On 9/2/24 2:03 PM, Andy Shevchenko wrote:
> On Mon, Sep 02, 2024 at 11:52:29AM +0200, Hans de Goede wrote:
>> The atomisp firmware contains a number of different pipeline binaries
>> inside its firmware file and the driver selects the right one depending
>> on the selected pipeline configuration.
>>
>> Sometimes (e.g. when the selected output resolution is too big) it fails
>> to find a binary. This happens especially when adding support for new
>> sensors.
>>
>> Improve the logging when this happens to make debugging easier:
>>
>> 1. Replace ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, ...) with standard
>>    dev_dbg() calls so that the logs can be enabled with dyndbg
>>
>> 2. Do not dump_stack() when this fails, doing so adds no useful extra
>>    info
>>
>> 3. With the dump_stack() call gone, remove the wrapper and rename
>>    __ia_css_binary_find() to ia_css_binary_find()
>>
>> 4. On error use dev_err() instead of dev_dbg() so that when things
>>    fail it is clear why they fail without needing to enable dyndbg
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note the log messages could also do with a bit of rewording and
>> reflowing them to put more arguments on a single line to use less
>> lines. I consider that out of scope for this patch which already
>> enough (see the 1-4 enough) something like this should be done
>> in a follow-up patch IMHO.
> 
> Yes, but...
> 
> ...
> 
>> -static int __ia_css_binary_find(struct ia_css_binary_descr *descr,
>> -				struct ia_css_binary *binary) {
>> +int ia_css_binary_find(struct ia_css_binary_descr *descr,
>> +		       struct ia_css_binary *binary)
> 
> ...for example, in this case you touched both lines, so making them a single
> line just reduces the future churn.

Ack, fixed in my media-atomisp branch.

> 
> ...
> 
>> -	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
>> -			    "ia_css_binary_find() enter: descr=%p, (mode=%d), binary=%p\n",
>> -			    descr, descr->mode,
>> -			    binary);
>> +	dev_dbg(atomisp_dev,
>> +		"ia_css_binary_find() enter: descr=%p, (mode=%d), binary=%p\n",
> 
>> +		descr, descr->mode,
>> +		binary);
> 
> So does this.

Ack, fixed in my media-atomisp branch.

> 
> ...
> 
>>  	/* print a map of the binary file */
>> -	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,	"BINARY INFO:\n");
>> +	dev_dbg(atomisp_dev,	"BINARY INFO:\n");
> 
> TAB instead of space.

Ack, fixed in my media-atomisp branch.

> ...
> 
>> -			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
>> -					    "ia_css_binary_find() [%d] continue: !%d && %d && (%d != %d)\n",
>> -					    __LINE__, candidate->enable.continuous,
>> -					    continuous, mode,
>> -					    IA_CSS_BINARY_MODE_COPY);
>> +			dev_dbg(atomisp_dev,
>> +				"ia_css_binary_find() [%d] continue: !%d && %d && (%d != %d)\n",
>> +				__LINE__, candidate->enable.continuous,
> 
>> +				continuous, mode,
>> +				IA_CSS_BINARY_MODE_COPY);
> 
> Now one line?

Ack, fixed in my media-atomisp branch.

> ...
> 
>> -			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
>> -					    "ia_css_binary_find() [%d] continue: binary is not striped\n",
>> -					    __LINE__);
>> +			dev_dbg(atomisp_dev,
>> +				"ia_css_binary_find() [%d] continue: binary is not striped\n",
>> +				__LINE__);
> 
> Now the __LINE__ argument is redundant as one may run-time turn it on and off.
> So, trimming it while converting to dev_dbg() makes sense to me as a one
> logical change. Ditto for other __LINE__ cases.

Dropping __LINE__ is the bit which I want to postpone to a follow-up patch,
the messages rejecting certain binaries as not suitable are not very
unique and the __LINE__ print is necessary (atm) to help find which check
exactly is failing. Not ideal I know, something for the TODO list.

> Otherwise it's a good clean up!

Thanks.

Regards,

Hans