* [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2
@ 2024-09-02 9:52 Hans de Goede
2024-09-02 9:52 ` [PATCH 2/3] media: atomisp: Drop dev_dbg() calls from hmm_[alloc|free]() Hans de Goede
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Hans de Goede @ 2024-09-02 9:52 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus
Cc: Hans de Goede, Tsuchiya Yuto, Andy Shevchenko, Yury Luneff, Nable,
andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media,
linux-staging
The t4ka3 sensor on the Xiaomi Mipad2 is used as a back facing sensor,
it uses 4 CSI lanes, but the _DSM has CsiLanes set to 2. Extend
the existing Xiaomi Mipad2 DMI quirk to override the wrong _DSM setting.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
index 31c9e05a1435..b2a3243ae2d4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c
@@ -109,6 +109,8 @@ static struct gmin_cfg_var lenovo_ideapad_miix_310_vars[] = {
static struct gmin_cfg_var xiaomi_mipad2_vars[] = {
/* _DSM contains the wrong CsiPort for the front facing OV5693 sensor */
{ "INT33BE:00", "CsiPort", "0" },
+ /* _DSM contains the wrong CsiLanes for the back facing T4KA3 sensor */
+ { "XMCC0003:00", "CsiLanes", "4" },
{}
};
--
2.46.0
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/3] media: atomisp: Drop dev_dbg() calls from hmm_[alloc|free]() 2024-09-02 9:52 [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede @ 2024-09-02 9:52 ` Hans de Goede 2024-09-02 11:06 ` Andy Shevchenko 2024-09-02 9:52 ` [PATCH 3/3] media: atomisp: Improve binary finding debug logging Hans de Goede ` (2 subsequent siblings) 3 siblings, 1 reply; 10+ messages in thread From: Hans de Goede @ 2024-09-02 9:52 UTC (permalink / raw) To: Mauro Carvalho Chehab, Sakari Ailus Cc: Hans de Goede, Tsuchiya Yuto, Andy Shevchenko, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging Debug logging every alloc + free just polutes the debug logs without adding much value, remove the alloc + free dev_dbg() calls. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/staging/media/atomisp/pci/hmm/hmm.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/staging/media/atomisp/pci/hmm/hmm.c b/drivers/staging/media/atomisp/pci/hmm/hmm.c index 3e2899ad8517..e8c5d728fd55 100644 --- a/drivers/staging/media/atomisp/pci/hmm/hmm.c +++ b/drivers/staging/media/atomisp/pci/hmm/hmm.c @@ -204,9 +204,6 @@ static ia_css_ptr __hmm_alloc(size_t bytes, enum hmm_bo_type type, goto bind_err; } - dev_dbg(atomisp_dev, "pages: 0x%08x (%zu bytes), type: %d, vmalloc %p\n", - bo->start, bytes, type, vmalloc_noprof); - return bo->start; bind_err: @@ -231,8 +228,6 @@ void hmm_free(ia_css_ptr virt) { struct hmm_buffer_object *bo; - dev_dbg(atomisp_dev, "%s: free 0x%08x\n", __func__, virt); - if (WARN_ON(virt == mmgr_EXCEPTION)) return; -- 2.46.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] media: atomisp: Drop dev_dbg() calls from hmm_[alloc|free]() 2024-09-02 9:52 ` [PATCH 2/3] media: atomisp: Drop dev_dbg() calls from hmm_[alloc|free]() Hans de Goede @ 2024-09-02 11:06 ` Andy Shevchenko 0 siblings, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2024-09-02 11:06 UTC (permalink / raw) To: Hans de Goede Cc: Mauro Carvalho Chehab, Sakari Ailus, Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging On Mon, Sep 02, 2024 at 11:52:28AM +0200, Hans de Goede wrote: > Debug logging every alloc + free just polutes the debug logs without > adding much value, remove the alloc + free dev_dbg() calls. Reviewed-by: Andy Shevchenko <andy@kernel.org> Agree on the point and ideally we should remove/replace most of (all?) the messages by trace points. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/3] media: atomisp: Improve binary finding debug logging 2024-09-02 9:52 [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede 2024-09-02 9:52 ` [PATCH 2/3] media: atomisp: Drop dev_dbg() calls from hmm_[alloc|free]() Hans de Goede @ 2024-09-02 9:52 ` Hans de Goede 2024-09-02 12:03 ` Andy Shevchenko 2024-09-02 10:37 ` [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede 2024-09-02 11:06 ` Andy Shevchenko 3 siblings, 1 reply; 10+ messages in thread From: Hans de Goede @ 2024-09-02 9:52 UTC (permalink / raw) To: Mauro Carvalho Chehab, Sakari Ailus Cc: Hans de Goede, Tsuchiya Yuto, Andy Shevchenko, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging 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. --- .../atomisp/pci/runtime/binary/src/binary.c | 295 +++++++++--------- 1 file changed, 139 insertions(+), 156 deletions(-) diff --git a/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c b/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c index b0f904a5e442..1c72c2b5817f 100644 --- a/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c +++ b/drivers/staging/media/atomisp/pci/runtime/binary/src/binary.c @@ -923,8 +923,9 @@ ia_css_binary_fill_info(const struct ia_css_binary_xinfo *xinfo, return 0; } -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) +{ int mode; bool online; bool two_ppc; @@ -953,10 +954,10 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, /* MW: used after an error check, may accept NULL, but doubtfull */ assert(binary); - 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); mode = descr->mode; online = descr->online; @@ -1001,15 +1002,15 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, } /* 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"); for (i = 0; i < IA_CSS_BINARY_NUM_MODES; i++) { xcandidate = binary_infos[i]; if (xcandidate) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, "%d:\n", i); + dev_dbg(atomisp_dev, "%d:\n", i); while (xcandidate) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, " Name:%s Type:%d Cont:%d\n", - xcandidate->blob->name, xcandidate->type, - xcandidate->sp.enable.continuous); + dev_dbg(atomisp_dev, " Name:%s Type:%d Cont:%d\n", + xcandidate->blob->name, xcandidate->type, + xcandidate->sp.enable.continuous); xcandidate = xcandidate->next; } } @@ -1021,9 +1022,9 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, struct ia_css_binary_info *candidate = &xcandidate->sp; /* printf("sh_css_binary_find: evaluating candidate: * %d\n",candidate->id); */ - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() candidate = %p, mode = %d ID = %d\n", - candidate, candidate->pipeline.mode, candidate->id); + dev_dbg(atomisp_dev, + "ia_css_binary_find() candidate = %p, mode = %d ID = %d\n", + candidate, candidate->pipeline.mode, candidate->id); /* * MW: Only a limited set of jointly configured binaries can @@ -1032,17 +1033,17 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, */ if (!candidate->enable.continuous && continuous && (mode != IA_CSS_BINARY_MODE_COPY)) { - 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); continue; } if (striped && candidate->iterator.num_stripes == 1) { - 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__); continue; } @@ -1050,58 +1051,58 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, (mode != IA_CSS_BINARY_MODE_COPY) && (mode != IA_CSS_BINARY_MODE_CAPTURE_PP) && (mode != IA_CSS_BINARY_MODE_VF_PP)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d != %d)\n", - __LINE__, - candidate->pipeline.isp_pipe_version, isp_pipe_version); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d != %d)\n", + __LINE__, + candidate->pipeline.isp_pipe_version, isp_pipe_version); continue; } if (!candidate->enable.reduced_pipe && enable_reduced_pipe) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d && %d\n", - __LINE__, - candidate->enable.reduced_pipe, - enable_reduced_pipe); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d && %d\n", + __LINE__, + candidate->enable.reduced_pipe, + enable_reduced_pipe); continue; } if (!candidate->enable.dvs_6axis && enable_dvs_6axis) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d && %d\n", - __LINE__, - candidate->enable.dvs_6axis, - enable_dvs_6axis); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d && %d\n", + __LINE__, + candidate->enable.dvs_6axis, + enable_dvs_6axis); continue; } if (candidate->enable.high_speed && !enable_high_speed) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: %d && !%d\n", - __LINE__, - candidate->enable.high_speed, - enable_high_speed); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: %d && !%d\n", + __LINE__, + candidate->enable.high_speed, + enable_high_speed); continue; } if (!candidate->enable.xnr && need_xnr) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: %d && !%d\n", - __LINE__, - candidate->enable.xnr, - need_xnr); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: %d && !%d\n", + __LINE__, + candidate->enable.xnr, + need_xnr); continue; } if (!(candidate->enable.ds & 2) && enable_yuv_ds) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d && %d\n", - __LINE__, - ((candidate->enable.ds & 2) != 0), - enable_yuv_ds); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d && %d\n", + __LINE__, + ((candidate->enable.ds & 2) != 0), + enable_yuv_ds); continue; } if ((candidate->enable.ds & 2) && !enable_yuv_ds) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: %d && !%d\n", - __LINE__, - ((candidate->enable.ds & 2) != 0), - enable_yuv_ds); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: %d && !%d\n", + __LINE__, + ((candidate->enable.ds & 2) != 0), + enable_yuv_ds); continue; } @@ -1115,100 +1116,100 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, candidate->vf_dec.is_variable || /* or more than one output pin. */ xcandidate->num_output_pins > 1)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%p != NULL) && !(%d || %d || (%d >%d))\n", - __LINE__, req_vf_info, - candidate->enable.vf_veceven, - candidate->vf_dec.is_variable, - xcandidate->num_output_pins, 1); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%p != NULL) && !(%d || %d || (%d >%d))\n", + __LINE__, req_vf_info, + candidate->enable.vf_veceven, + candidate->vf_dec.is_variable, + xcandidate->num_output_pins, 1); continue; } if (!candidate->enable.dvs_envelope && need_dvs) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d && %d\n", - __LINE__, - candidate->enable.dvs_envelope, (int)need_dvs); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d && %d\n", + __LINE__, + candidate->enable.dvs_envelope, (int)need_dvs); continue; } /* internal_res check considers input, output, and dvs envelope sizes */ ia_css_binary_internal_res(req_in_info, req_bds_out_info, req_bin_out_info, &dvs_env, candidate, &internal_res); if (internal_res.width > candidate->internal.max_width) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d > %d)\n", - __LINE__, internal_res.width, - candidate->internal.max_width); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d > %d)\n", + __LINE__, internal_res.width, + candidate->internal.max_width); continue; } if (internal_res.height > candidate->internal.max_height) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d > %d)\n", - __LINE__, internal_res.height, - candidate->internal.max_height); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d > %d)\n", + __LINE__, internal_res.height, + candidate->internal.max_height); continue; } if (!candidate->enable.ds && need_ds && !(xcandidate->num_output_pins > 1)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d && %d\n", - __LINE__, candidate->enable.ds, (int)need_ds); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d && %d\n", + __LINE__, candidate->enable.ds, (int)need_ds); continue; } if (!candidate->enable.uds && !candidate->enable.dvs_6axis && need_dz) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d && !%d && %d\n", - __LINE__, candidate->enable.uds, - candidate->enable.dvs_6axis, (int)need_dz); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d && !%d && %d\n", + __LINE__, candidate->enable.uds, + candidate->enable.dvs_6axis, (int)need_dz); continue; } if (online && candidate->input.source == IA_CSS_BINARY_INPUT_MEMORY) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: %d && (%d == %d)\n", - __LINE__, online, candidate->input.source, - IA_CSS_BINARY_INPUT_MEMORY); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: %d && (%d == %d)\n", + __LINE__, online, candidate->input.source, + IA_CSS_BINARY_INPUT_MEMORY); continue; } if (!online && candidate->input.source == IA_CSS_BINARY_INPUT_SENSOR) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d && (%d == %d)\n", - __LINE__, online, candidate->input.source, - IA_CSS_BINARY_INPUT_SENSOR); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d && (%d == %d)\n", + __LINE__, online, candidate->input.source, + IA_CSS_BINARY_INPUT_SENSOR); continue; } if (req_bin_out_info->res.width < candidate->output.min_width || req_bin_out_info->res.width > candidate->output.max_width) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d > %d) || (%d < %d)\n", - __LINE__, - req_bin_out_info->padded_width, - candidate->output.min_width, - req_bin_out_info->padded_width, - candidate->output.max_width); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d > %d) || (%d < %d)\n", + __LINE__, + req_bin_out_info->padded_width, + candidate->output.min_width, + req_bin_out_info->padded_width, + candidate->output.max_width); continue; } if (xcandidate->num_output_pins > 1 && /* in case we have a second output pin, */ req_vf_info) { /* and we need vf output. */ if (req_vf_info->res.width > candidate->output.max_width) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d < %d)\n", - __LINE__, - req_vf_info->res.width, - candidate->output.max_width); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d < %d)\n", + __LINE__, + req_vf_info->res.width, + candidate->output.max_width); continue; } } if (req_in_info->padded_width > candidate->input.max_width) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d > %d)\n", - __LINE__, req_in_info->padded_width, - candidate->input.max_width); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d > %d)\n", + __LINE__, req_in_info->padded_width, + candidate->input.max_width); continue; } if (!binary_supports_output_format(xcandidate, req_bin_out_info->format)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: !%d\n", - __LINE__, - binary_supports_output_format(xcandidate, req_bin_out_info->format)); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: !%d\n", + __LINE__, + binary_supports_output_format(xcandidate, req_bin_out_info->format)); continue; } if (xcandidate->num_output_pins > 1 && @@ -1217,11 +1218,11 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, /* check if the required vf format is supported. */ !binary_supports_output_format(xcandidate, req_vf_info->format)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d > %d) && (%p != NULL) && !%d\n", - __LINE__, xcandidate->num_output_pins, 1, - req_vf_info, - binary_supports_output_format(xcandidate, req_vf_info->format)); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d > %d) && (%p != NULL) && !%d\n", + __LINE__, xcandidate->num_output_pins, 1, + req_vf_info, + binary_supports_output_format(xcandidate, req_vf_info->format)); continue; } @@ -1229,11 +1230,11 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, if (xcandidate->num_output_pins == 1 && req_vf_info && candidate->enable.vf_veceven && !binary_supports_vf_format(xcandidate, req_vf_info->format)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d == %d) && (%p != NULL) && %d && !%d\n", - __LINE__, xcandidate->num_output_pins, 1, - req_vf_info, candidate->enable.vf_veceven, - binary_supports_vf_format(xcandidate, req_vf_info->format)); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d == %d) && (%p != NULL) && %d && !%d\n", + __LINE__, xcandidate->num_output_pins, 1, + req_vf_info, candidate->enable.vf_veceven, + binary_supports_vf_format(xcandidate, req_vf_info->format)); continue; } @@ -1241,37 +1242,37 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, if (xcandidate->num_output_pins == 1 && req_vf_info && candidate->enable.vf_veceven) { /* and we need vf output. */ if (req_vf_info->res.width > candidate->output.max_width) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: (%d < %d)\n", - __LINE__, - req_vf_info->res.width, - candidate->output.max_width); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: (%d < %d)\n", + __LINE__, + req_vf_info->res.width, + candidate->output.max_width); continue; } } if (!supports_bds_factor(candidate->bds.supported_bds_factors, descr->required_bds_factor)) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: 0x%x & 0x%x)\n", - __LINE__, candidate->bds.supported_bds_factors, - descr->required_bds_factor); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: 0x%x & 0x%x)\n", + __LINE__, candidate->bds.supported_bds_factors, + descr->required_bds_factor); continue; } if (!candidate->enable.dpc && need_dpc) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: 0x%x & 0x%x)\n", - __LINE__, candidate->enable.dpc, - descr->enable_dpc); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: 0x%x & 0x%x)\n", + __LINE__, candidate->enable.dpc, + descr->enable_dpc); continue; } if (candidate->uds.use_bci && enable_capture_pp_bli) { - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() [%d] continue: 0x%x & 0x%x)\n", - __LINE__, candidate->uds.use_bci, - descr->enable_capture_pp_bli); + dev_dbg(atomisp_dev, + "ia_css_binary_find() [%d] continue: 0x%x & 0x%x)\n", + __LINE__, candidate->uds.use_bci, + descr->enable_capture_pp_bli); continue; } @@ -1290,13 +1291,6 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, break; } - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() selected = %p, mode = %d ID = %d\n", - xcandidate, xcandidate ? xcandidate->sp.pipeline.mode : 0, xcandidate ? xcandidate->sp.id : 0); - - ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE, - "ia_css_binary_find() leave: return_err=%d\n", err); - if (!err && xcandidate) dev_dbg(atomisp_dev, "Using binary %s (id %d), type %d, mode %d, continuous %s\n", @@ -1306,23 +1300,12 @@ static int __ia_css_binary_find(struct ia_css_binary_descr *descr, xcandidate->sp.pipeline.mode, xcandidate->sp.enable.continuous ? "true" : "false"); + if (err) + dev_err(atomisp_dev, "Failed to find a firmware binary matching the pipeline parameters\n"); return err; } -int ia_css_binary_find(struct ia_css_binary_descr *descr, - struct ia_css_binary *binary) -{ - int ret = __ia_css_binary_find(descr, binary); - - if (unlikely(ret)) { - dev_dbg(atomisp_dev, "Seeking for binary failed at:"); - dump_stack(); - } - - return ret; -} - unsigned ia_css_binary_max_vf_width(void) { -- 2.46.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: atomisp: Improve binary finding debug logging 2024-09-02 9:52 ` [PATCH 3/3] media: atomisp: Improve binary finding debug logging Hans de Goede @ 2024-09-02 12:03 ` Andy Shevchenko 2024-09-03 9:27 ` Hans de Goede 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2024-09-02 12:03 UTC (permalink / raw) To: Hans de Goede Cc: Mauro Carvalho Chehab, Sakari Ailus, Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging 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. ... > - 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. ... > /* 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. ... > - 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? ... > - 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. ... Otherwise it's a good clean up! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: atomisp: Improve binary finding debug logging 2024-09-02 12:03 ` Andy Shevchenko @ 2024-09-03 9:27 ` Hans de Goede 2024-09-03 10:30 ` Andy Shevchenko 0 siblings, 1 reply; 10+ messages in thread From: Hans de Goede @ 2024-09-03 9:27 UTC (permalink / raw) To: Andy Shevchenko Cc: Mauro Carvalho Chehab, Sakari Ailus, Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: atomisp: Improve binary finding debug logging 2024-09-03 9:27 ` Hans de Goede @ 2024-09-03 10:30 ` Andy Shevchenko 2024-09-03 11:12 ` Hans de Goede 0 siblings, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2024-09-03 10:30 UTC (permalink / raw) To: Hans de Goede Cc: Andy Shevchenko, Mauro Carvalho Chehab, Sakari Ailus, Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging On Tue, Sep 3, 2024 at 12:27 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 9/2/24 2:03 PM, Andy Shevchenko wrote: > > On Mon, Sep 02, 2024 at 11:52:29AM +0200, Hans de Goede wrote: ... > >> -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. (Actually there are more opportunities like this that I haven't commented on in the previous reply. I hope you have already found them and fixed accordingly) ... > >> + 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. Yes, it's fine, after I sent the previous reply I saw that the previous implementation was a simple wrapper over dev_dbg() and __LINE__ "issue" was already there. I.o.w. I agree on the followup! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: atomisp: Improve binary finding debug logging 2024-09-03 10:30 ` Andy Shevchenko @ 2024-09-03 11:12 ` Hans de Goede 0 siblings, 0 replies; 10+ messages in thread From: Hans de Goede @ 2024-09-03 11:12 UTC (permalink / raw) To: Andy Shevchenko Cc: Andy Shevchenko, Mauro Carvalho Chehab, Sakari Ailus, Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging Hi, On 9/3/24 12:30 PM, Andy Shevchenko wrote: > On Tue, Sep 3, 2024 at 12:27 PM Hans de Goede <hdegoede@redhat.com> wrote: >> On 9/2/24 2:03 PM, Andy Shevchenko wrote: >>> On Mon, Sep 02, 2024 at 11:52:29AM +0200, Hans de Goede wrote: > > ... > >>>> -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. > > (Actually there are more opportunities like this that I haven't > commented on in the previous reply. I hope you have already found them > and fixed accordingly) Yes I've gone over the entire patch and made lines longer to avoid needing multiple lines where possible. Regards, Hans ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 2024-09-02 9:52 [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede 2024-09-02 9:52 ` [PATCH 2/3] media: atomisp: Drop dev_dbg() calls from hmm_[alloc|free]() Hans de Goede 2024-09-02 9:52 ` [PATCH 3/3] media: atomisp: Improve binary finding debug logging Hans de Goede @ 2024-09-02 10:37 ` Hans de Goede 2024-09-02 11:06 ` Andy Shevchenko 3 siblings, 0 replies; 10+ messages in thread From: Hans de Goede @ 2024-09-02 10:37 UTC (permalink / raw) To: Mauro Carvalho Chehab, Sakari Ailus Cc: Tsuchiya Yuto, Andy Shevchenko, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging Hi, On 9/2/24 11:52 AM, Hans de Goede wrote: > The t4ka3 sensor on the Xiaomi Mipad2 is used as a back facing sensor, > it uses 4 CSI lanes, but the _DSM has CsiLanes set to 2. Extend > the existing Xiaomi Mipad2 DMI quirk to override the wrong _DSM setting. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I have merged these 3 patches in my media-atomisp branch: https://git.kernel.org/pub/scm/linux/kernel/git/hansg/linux.git/log/?h=media-atomisp now. And this/these will be included in my next pull-request to Mauro (to media subsystem maintainer) Regards, Hans > --- > drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > index 31c9e05a1435..b2a3243ae2d4 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > @@ -109,6 +109,8 @@ static struct gmin_cfg_var lenovo_ideapad_miix_310_vars[] = { > static struct gmin_cfg_var xiaomi_mipad2_vars[] = { > /* _DSM contains the wrong CsiPort for the front facing OV5693 sensor */ > { "INT33BE:00", "CsiPort", "0" }, > + /* _DSM contains the wrong CsiLanes for the back facing T4KA3 sensor */ > + { "XMCC0003:00", "CsiLanes", "4" }, > {} > }; > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 2024-09-02 9:52 [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede ` (2 preceding siblings ...) 2024-09-02 10:37 ` [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede @ 2024-09-02 11:06 ` Andy Shevchenko 3 siblings, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2024-09-02 11:06 UTC (permalink / raw) To: Hans de Goede Cc: Mauro Carvalho Chehab, Sakari Ailus, Tsuchiya Yuto, Yury Luneff, Nable, andrey.i.trufanov, Fabio Aiuto, Kate Hsuan, linux-media, linux-staging On Mon, Sep 02, 2024 at 11:52:27AM +0200, Hans de Goede wrote: > The t4ka3 sensor on the Xiaomi Mipad2 is used as a back facing sensor, > it uses 4 CSI lanes, but the _DSM has CsiLanes set to 2. Extend > the existing Xiaomi Mipad2 DMI quirk to override the wrong _DSM setting. Reviewed-by: Andy Shevchenko <andy@kernel.org> -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-09-03 11:12 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-02 9:52 [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede 2024-09-02 9:52 ` [PATCH 2/3] media: atomisp: Drop dev_dbg() calls from hmm_[alloc|free]() Hans de Goede 2024-09-02 11:06 ` Andy Shevchenko 2024-09-02 9:52 ` [PATCH 3/3] media: atomisp: Improve binary finding debug logging Hans de Goede 2024-09-02 12:03 ` Andy Shevchenko 2024-09-03 9:27 ` Hans de Goede 2024-09-03 10:30 ` Andy Shevchenko 2024-09-03 11:12 ` Hans de Goede 2024-09-02 10:37 ` [PATCH 1/3] media: atomisp: csi2-bridge: Add DMI quirk for t4ka3 on Xiaomi Mipad2 Hans de Goede 2024-09-02 11:06 ` Andy Shevchenko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox