Quoting Jani Nikula (2024-12-30 09:58:31-03:00)
>On Tue, 24 Dec 2024, Gustavo Sousa <gustavo.sousa@xxxxxxxxx> wrote:
>> Quoting Ville Syrjala (2024-12-19 10:08:25-03:00)
>>>From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>
>>>Include the standard "[CRTC:...]" information in the scaler debugs
>>>to make life easier.
>>
>> Drive-by comment (and going a bit off-topic):
>>
>> $ git grep '\[[A-Z]\+:%d:%s]' -- drivers/gpu/drm | wc -l
>> 600
>>
>> Has someone already considered creating DRM_*_FMT and DRM_*_ARG() for
>> those? E.g. DRM_CRTC_FMT and CRM_CRTC_ARG(crtc->base), which, IMO, would
>> be easier to use and arguably more concise.
>
>Personally, I'm not in favour. I dislike having to concatenate the
>string constants all over the place.
>
>> I tried doing a quick search on lore.kernel.org/dri-devel, but I'm not
>> sure what would be good search terms to find previous attempts to see
>> possible arguments against it.
>
>We've gone through this a number of times, and some of the other options
>are:
Thanks for the recap!
>
>- Add allocated debug string to the objects, e.g. crtc->debug or
> crtc->id and print it with %s. It also works when you want to print
> the info of e.g. both the connector and the encoder.
This one seems interesting, although there would be some increased
memory usage. Maybe that's not too bad?
Was the increase in memory usage what caused it not to be adopted?
--
Gustavo Sousa
>
>- Add function pointers for debug logging in the drm objects, and have
> the drm_dbg* family of functions use them based on the type passed to
> it with generics. You'd do drm_dbg_kms(connector, "foo\n"); and that
> would add the [CONNECTOR:...] prefix. Falls short when you want to
> print the info of multiple objects.
>
>- Object specific debug logging macros. connector_dbg() etc. I'm
> strongly against the proliferation of logging macros. All the variants
> (like once, ratelimited, etc.) get multiplied by the number of object
> types. (Yes, I also dislike the gt/guc macros, but I digress.) And
> this also doesn't solve the logging of multiple objects at once.
>
>- Add printk format specifiers. There's apparently no way to do this in
> a human readable way, as anything nice throws off compiler printf
> format checks. So you end up with stuff like %pXYZ where the XYZ are
> meaningless magic letters. And they'd need to be implemented in kernel
> core.
>
>
>BR,
>Jani.
>
>
>>
>> --
>> Gustavo Sousa
>>
>>>
>>>Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>>---
>>> drivers/gpu/drm/i915/display/skl_scaler.c | 25 +++++++++++++++--------
>>> 1 file changed, 16 insertions(+), 9 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/i915/display/skl_scaler.c b/drivers/gpu/drm/i915/display/skl_scaler.c
>>>index cbc71e44fcbb..f6d76ef1a854 100644
>>>--- a/drivers/gpu/drm/i915/display/skl_scaler.c
>>>+++ b/drivers/gpu/drm/i915/display/skl_scaler.c
>>>@@ -166,7 +166,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>> if (DISPLAY_VER(display) >= 9 && crtc_state->hw.enable &&
>>> need_scaler && adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
>>> drm_dbg_kms(display->drm,
>>>- "Pipe/Plane scaling not supported with IF-ID mode\n");
>>>+ "[CRTC:%d:%s] scaling not supported with IF-ID mode\n",
>>>+ crtc->base.base.id, crtc->base.name);
>>> return -EINVAL;
>>> }
>>>
>>>@@ -186,8 +187,9 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>> scaler_state->scalers[*scaler_id].in_use = false;
>>>
>>> drm_dbg_kms(display->drm,
>>>- "scaler_user index %u.%u: "
>>>+ "[CRTC:%d:%s] scaler_user index %u.%u: "
>>> "Staged freeing scaler id %d scaler_users = 0x%x\n",
>>>+ crtc->base.base.id, crtc->base.name,
>>> crtc->pipe, scaler_user, *scaler_id,
>>> scaler_state->scaler_users);
>>> *scaler_id = -1;
>>>@@ -207,8 +209,9 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>> src_w > max_src_w || src_h > max_src_h ||
>>> dst_w > max_dst_w || dst_h > max_dst_h) {
>>> drm_dbg_kms(display->drm,
>>>- "scaler_user index %u.%u: src %ux%u dst %ux%u "
>>>+ "[CRTC:%d:%s] scaler_user index %u.%u: src %ux%u dst %ux%u "
>>> "size is out of scaler range\n",
>>>+ crtc->base.base.id, crtc->base.name,
>>> crtc->pipe, scaler_user, src_w, src_h,
>>> dst_w, dst_h);
>>> return -EINVAL;
>>>@@ -224,16 +227,18 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>>> */
>>> if (pipe_src_w > max_dst_w || pipe_src_h > max_dst_h) {
>>> drm_dbg_kms(display->drm,
>>>- "scaler_user index %u.%u: pipe src size %ux%u "
>>>+ "[CRTC:%d:%s] scaler_user index %u.%u: pipe src size %ux%u "
>>> "is out of scaler range\n",
>>>+ crtc->base.base.id, crtc->base.name,
>>> crtc->pipe, scaler_user, pipe_src_w, pipe_src_h);
>>> return -EINVAL;
>>> }
>>>
>>> /* mark this plane as a scaler user in crtc_state */
>>> scaler_state->scaler_users |= (1 << scaler_user);
>>>- drm_dbg_kms(display->drm, "scaler_user index %u.%u: "
>>>+ drm_dbg_kms(display->drm, "[CRTC:%d:%s] scaler_user index %u.%u: "
>>> "staged scaling request for %ux%u->%ux%u scaler_users = 0x%x\n",
>>>+ crtc->base.base.id, crtc->base.name,
>>> crtc->pipe, scaler_user, src_w, src_h, dst_w, dst_h,
>>> scaler_state->scaler_users);
>>>
>>>@@ -421,8 +426,8 @@ static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_stat
>>>
>>> if (hscale < 0 || vscale < 0) {
>>> drm_dbg_kms(display->drm,
>>>- "Scaler %d doesn't support required plane scaling\n",
>>>- *scaler_id);
>>>+ "[CRTC:%d:%s] scaler %d doesn't support required plane scaling\n",
>>>+ crtc->base.base.id, crtc->base.name, *scaler_id);
>>> drm_rect_debug_print("src: ", src, true);
>>> drm_rect_debug_print("dst: ", dst, false);
>>>
>>>@@ -430,7 +435,8 @@ static int intel_atomic_setup_scaler(struct intel_crtc_scaler_state *scaler_stat
>>> }
>>> }
>>>
>>>- drm_dbg_kms(display->drm, "Attached scaler id %u.%u to %s:%d\n",
>>>+ drm_dbg_kms(display->drm, "[CRTC:%d:%s] attached scaler id %u.%u to %s:%d\n",
>>>+ crtc->base.base.id, crtc->base.name,
>>> crtc->pipe, *scaler_id, name, idx);
>>> scaler_state->scalers[*scaler_id].mode = mode;
>>>
>>>@@ -530,7 +536,8 @@ int intel_atomic_setup_scalers(struct intel_atomic_state *state,
>>> /* fail if required scalers > available scalers */
>>> if (num_scalers_need > crtc->num_scalers) {
>>> drm_dbg_kms(display->drm,
>>>- "Too many scaling requests %d > %d\n",
>>>+ "[CRTC:%d:%s] too many scaling requests %d > %d\n",
>>>+ crtc->base.base.id, crtc->base.name,
>>> num_scalers_need, crtc->num_scalers);
>>> return -EINVAL;
>>> }
>>>--
>>>2.45.2
>>>
>
>--
>Jani Nikula, Intel
