| Message ID | 20240709-dpu-fix-wb-v1-2-448348bfd4cb@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm/dpu: two fixes targeting 6.11 | expand |
On 7/9/2024 6:48 AM, Dmitry Baryshkov wrote: > DPU debugging macros need to be converted to a proper drm_debug_* > macros, however this is a going an intrusive patch, not suitable for a > fix. Wire DPU_DEBUG and DPU_DEBUG_DRIVER to always use DRM_DEBUG_DRIVER > to make sure that DPU debugging messages always end up in the drm debug > messages and are controlled via the usual drm.debug mask. > These macros have been deprecated, is this waht you meant by the conversion to proper drm_debug_*? /* NOTE: this is deprecated in favor of drm_dbg(NULL, ...). */ #define DRM_DEBUG_DRIVER(fmt, ...) \ __drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__) I think all that this macro was doing was to have appropriate DRM_UT_* macros enabled before calling the corresponding DRM_DEBUG_* macros. But I think what was incorrect here is for DPU_DEBUG, we could have used DRM_UT_CORE instead of DRM_UT_KMS. And DRM_DEBUG_DRIVER should have been used instead of DRM_ERROR. Was this causing the issue of the prints not getting enabled? > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 14 ++------------ > 1 file changed, 2 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > index e2adc937ea63..935ff6fd172c 100644 > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > @@ -31,24 +31,14 @@ > * @fmt: Pointer to format string > */ > #define DPU_DEBUG(fmt, ...) \ > - do { \ > - if (drm_debug_enabled(DRM_UT_KMS)) \ > - DRM_DEBUG(fmt, ##__VA_ARGS__); \ > - else \ > - pr_debug(fmt, ##__VA_ARGS__); \ > - } while (0) > + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) > > /** > * DPU_DEBUG_DRIVER - macro for hardware driver logging > * @fmt: Pointer to format string > */ > #define DPU_DEBUG_DRIVER(fmt, ...) \ > - do { \ > - if (drm_debug_enabled(DRM_UT_DRIVER)) \ > - DRM_ERROR(fmt, ##__VA_ARGS__); \ > - else \ > - pr_debug(fmt, ##__VA_ARGS__); \ > - } while (0) > + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) > > #define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__) > #define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" fmt, ##__VA_ARGS__) >
On Tue, 9 Jul 2024 at 22:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > On 7/9/2024 6:48 AM, Dmitry Baryshkov wrote: > > DPU debugging macros need to be converted to a proper drm_debug_* > > macros, however this is a going an intrusive patch, not suitable for a > > fix. Wire DPU_DEBUG and DPU_DEBUG_DRIVER to always use DRM_DEBUG_DRIVER > > to make sure that DPU debugging messages always end up in the drm debug > > messages and are controlled via the usual drm.debug mask. > > > > These macros have been deprecated, is this waht you meant by the > conversion to proper drm_debug_*? Yes. Drop the driver-specific wrappers where they don't make sense. Use sensible format strings in the cases where it actually does (like VIDENC or _PLANE) > > /* NOTE: this is deprecated in favor of drm_dbg(NULL, ...). */ > #define DRM_DEBUG_DRIVER(fmt, ...) \ > __drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__) > > I think all that this macro was doing was to have appropriate DRM_UT_* > macros enabled before calling the corresponding DRM_DEBUG_* macros. But > I think what was incorrect here is for DPU_DEBUG, we could have used > DRM_UT_CORE instead of DRM_UT_KMS. It pretty much tries to overplay the existing drm debugging mechanism by either sending the messages to the DRM channel or just using pr_debug. With DYNAMIC_DEBUG being disabled pr_debug is just an empty macro, so all the messages can end up in /dev/null. We should not be trying to be too smart, using standard DRM_DEBUG_DRIVER should be enough. This way all driver-related messages are controlled by drm.debug including or excluding the 0x02 bit. > > And DRM_DEBUG_DRIVER should have been used instead of DRM_ERROR. > > Was this causing the issue of the prints not getting enabled? I pretty much think so. > > > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > --- > > drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 14 ++------------ > > 1 file changed, 2 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > index e2adc937ea63..935ff6fd172c 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h > > @@ -31,24 +31,14 @@ > > * @fmt: Pointer to format string > > */ > > #define DPU_DEBUG(fmt, ...) \ > > - do { \ > > - if (drm_debug_enabled(DRM_UT_KMS)) \ > > - DRM_DEBUG(fmt, ##__VA_ARGS__); \ > > - else \ > > - pr_debug(fmt, ##__VA_ARGS__); \ > > - } while (0) > > + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) > > > > /** > > * DPU_DEBUG_DRIVER - macro for hardware driver logging > > * @fmt: Pointer to format string > > */ > > #define DPU_DEBUG_DRIVER(fmt, ...) \ > > - do { \ > > - if (drm_debug_enabled(DRM_UT_DRIVER)) \ > > - DRM_ERROR(fmt, ##__VA_ARGS__); \ > > - else \ > > - pr_debug(fmt, ##__VA_ARGS__); \ > > - } while (0) > > + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) > > > > #define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__) > > #define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" fmt, ##__VA_ARGS__) > >
On 7/10/2024 12:40 AM, Dmitry Baryshkov wrote: > On Tue, 9 Jul 2024 at 22:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> On 7/9/2024 6:48 AM, Dmitry Baryshkov wrote: >>> DPU debugging macros need to be converted to a proper drm_debug_* >>> macros, however this is a going an intrusive patch, not suitable for a >>> fix. Wire DPU_DEBUG and DPU_DEBUG_DRIVER to always use DRM_DEBUG_DRIVER >>> to make sure that DPU debugging messages always end up in the drm debug >>> messages and are controlled via the usual drm.debug mask. >>> >> >> These macros have been deprecated, is this waht you meant by the >> conversion to proper drm_debug_*? > > Yes. Drop the driver-specific wrappers where they don't make sense. > Use sensible format strings in the cases where it actually does (like > VIDENC or _PLANE) > Ack but we need to not just drop the wrappers but drop the usage of these macros as well because it is documented that they are deprecated. So I assume you want to get this in and do that as a follow up change? >> >> /* NOTE: this is deprecated in favor of drm_dbg(NULL, ...). */ >> #define DRM_DEBUG_DRIVER(fmt, ...) \ >> __drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__) >> >> I think all that this macro was doing was to have appropriate DRM_UT_* >> macros enabled before calling the corresponding DRM_DEBUG_* macros. But >> I think what was incorrect here is for DPU_DEBUG, we could have used >> DRM_UT_CORE instead of DRM_UT_KMS. > > It pretty much tries to overplay the existing drm debugging mechanism > by either sending the messages to the DRM channel or just using > pr_debug. With DYNAMIC_DEBUG being disabled pr_debug is just an empty > macro, so all the messages can end up in /dev/null. We should not be > trying to be too smart, using standard DRM_DEBUG_DRIVER should be > enough. This way all driver-related messages are controlled by > drm.debug including or excluding the 0x02 bit. > > >> >> And DRM_DEBUG_DRIVER should have been used instead of DRM_ERROR. >> >> Was this causing the issue of the prints not getting enabled? > > I pretty much think so. > Alright, I am okay with the approach, just one minor suggestion, to keep the behavior intact, previously the code wanted DPU_DEBUG to be controlled by DRM_UT_KMS and DPU_DEBUG_DRIVER controlled by DRM_UT_DRIVER. Keeping that intact, we need to use DRM_DEBUG_KMS for DPU_DEBUG? >> >>> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >>> --- >>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 14 ++------------ >>> 1 file changed, 2 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> index e2adc937ea63..935ff6fd172c 100644 >>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h >>> @@ -31,24 +31,14 @@ >>> * @fmt: Pointer to format string >>> */ >>> #define DPU_DEBUG(fmt, ...) \ >>> - do { \ >>> - if (drm_debug_enabled(DRM_UT_KMS)) \ >>> - DRM_DEBUG(fmt, ##__VA_ARGS__); \ >>> - else \ >>> - pr_debug(fmt, ##__VA_ARGS__); \ >>> - } while (0) >>> + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) >>> >>> /** >>> * DPU_DEBUG_DRIVER - macro for hardware driver logging >>> * @fmt: Pointer to format string >>> */ >>> #define DPU_DEBUG_DRIVER(fmt, ...) \ >>> - do { \ >>> - if (drm_debug_enabled(DRM_UT_DRIVER)) \ >>> - DRM_ERROR(fmt, ##__VA_ARGS__); \ >>> - else \ >>> - pr_debug(fmt, ##__VA_ARGS__); \ >>> - } while (0) >>> + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) >>> >>> #define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__) >>> #define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" fmt, ##__VA_ARGS__) >>> > > >
On Thu, Jul 11, 2024 at 11:03:15AM GMT, Abhinav Kumar wrote: > > > On 7/10/2024 12:40 AM, Dmitry Baryshkov wrote: > > On Tue, 9 Jul 2024 at 22:39, Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > > > > > > > > > > > On 7/9/2024 6:48 AM, Dmitry Baryshkov wrote: > > > > DPU debugging macros need to be converted to a proper drm_debug_* > > > > macros, however this is a going an intrusive patch, not suitable for a > > > > fix. Wire DPU_DEBUG and DPU_DEBUG_DRIVER to always use DRM_DEBUG_DRIVER > > > > to make sure that DPU debugging messages always end up in the drm debug > > > > messages and are controlled via the usual drm.debug mask. > > > > > > > > > > These macros have been deprecated, is this waht you meant by the > > > conversion to proper drm_debug_*? > > > > Yes. Drop the driver-specific wrappers where they don't make sense. > > Use sensible format strings in the cases where it actually does (like > > VIDENC or _PLANE) > > > > Ack but we need to not just drop the wrappers but drop the usage of these > macros as well because it is documented that they are deprecated. > > So I assume you want to get this in and do that as a follow up change? Yes, somewhere in the long list of cleanups. I have a similar item against DP driver, which uses correct macros, > > > /* NOTE: this is deprecated in favor of drm_dbg(NULL, ...). */ > > > #define DRM_DEBUG_DRIVER(fmt, ...) \ > > > __drm_dbg(DRM_UT_DRIVER, fmt, ##__VA_ARGS__) > > > > > > I think all that this macro was doing was to have appropriate DRM_UT_* > > > macros enabled before calling the corresponding DRM_DEBUG_* macros. But > > > I think what was incorrect here is for DPU_DEBUG, we could have used > > > DRM_UT_CORE instead of DRM_UT_KMS. > > > > It pretty much tries to overplay the existing drm debugging mechanism > > by either sending the messages to the DRM channel or just using > > pr_debug. With DYNAMIC_DEBUG being disabled pr_debug is just an empty > > macro, so all the messages can end up in /dev/null. We should not be > > trying to be too smart, using standard DRM_DEBUG_DRIVER should be > > enough. This way all driver-related messages are controlled by > > drm.debug including or excluding the 0x02 bit. > > > > > > > > > > And DRM_DEBUG_DRIVER should have been used instead of DRM_ERROR. > > > > > > Was this causing the issue of the prints not getting enabled? > > > > I pretty much think so. > > > > Alright, I am okay with the approach, just one minor suggestion, to keep the > behavior intact, previously the code wanted DPU_DEBUG to be controlled by > DRM_UT_KMS and DPU_DEBUG_DRIVER controlled by DRM_UT_DRIVER. > > Keeping that intact, we need to use DRM_DEBUG_KMS for DPU_DEBUG? I might make that more explicit: I don't think that it is a good idea for a generic DPU_DEBUG macro to be tied to DRM_UT_KMS. We are reporting a debug message from driver, so by default it should go to the DRM_UT_DRIVER channel. While refactoring things we might end up with messages going to ATOMIC or KMS, but DRIVER should be the default.
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h index e2adc937ea63..935ff6fd172c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h @@ -31,24 +31,14 @@ * @fmt: Pointer to format string */ #define DPU_DEBUG(fmt, ...) \ - do { \ - if (drm_debug_enabled(DRM_UT_KMS)) \ - DRM_DEBUG(fmt, ##__VA_ARGS__); \ - else \ - pr_debug(fmt, ##__VA_ARGS__); \ - } while (0) + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) /** * DPU_DEBUG_DRIVER - macro for hardware driver logging * @fmt: Pointer to format string */ #define DPU_DEBUG_DRIVER(fmt, ...) \ - do { \ - if (drm_debug_enabled(DRM_UT_DRIVER)) \ - DRM_ERROR(fmt, ##__VA_ARGS__); \ - else \ - pr_debug(fmt, ##__VA_ARGS__); \ - } while (0) + DRM_DEBUG_DRIVER(fmt, ##__VA_ARGS__) #define DPU_ERROR(fmt, ...) pr_err("[dpu error]" fmt, ##__VA_ARGS__) #define DPU_ERROR_RATELIMITED(fmt, ...) pr_err_ratelimited("[dpu error]" fmt, ##__VA_ARGS__)
DPU debugging macros need to be converted to a proper drm_debug_* macros, however this is a going an intrusive patch, not suitable for a fix. Wire DPU_DEBUG and DPU_DEBUG_DRIVER to always use DRM_DEBUG_DRIVER to make sure that DPU debugging messages always end up in the drm debug messages and are controlled via the usual drm.debug mask. Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support") Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.h | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)