diff mbox series

[v2,2/2] drm/msm/dpu: don't play tricks with debug macros

Message ID 20240802-dpu-fix-wb-v2-2-7eac9eb8e895@linaro.org (mailing list archive)
State New, archived
Headers show
Series drm/msm/dpu: two fixes targeting 6.11 | expand

Commit Message

Dmitry Baryshkov Aug. 2, 2024, 7:47 p.m. UTC
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.

I don't think that it is a good idea for a generic DPU_DEBUG macro to be
tied to DRM_UT_KMS. It is used to report a debug message from driver, so by
default it should go to the DRM_UT_DRIVER channel. While refactoring
debug macros later on we might end up with particular messages going to
ATOMIC or KMS, but DRIVER should be the default.

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(-)

Comments

Abhinav Kumar Aug. 5, 2024, 7:25 p.m. UTC | #1
On 8/2/2024 12:47 PM, 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.
> 
> I don't think that it is a good idea for a generic DPU_DEBUG macro to be
> tied to DRM_UT_KMS. It is used to report a debug message from driver, so by
> default it should go to the DRM_UT_DRIVER channel. While refactoring
> debug macros later on we might end up with particular messages going to
> ATOMIC or KMS, but DRIVER should be the default.
> 
> 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(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Konrad Dybcio Aug. 27, 2024, 9:39 a.m. UTC | #2
On 2.08.2024 9:47 PM, 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.
> 
> I don't think that it is a good idea for a generic DPU_DEBUG macro to be
> tied to DRM_UT_KMS. It is used to report a debug message from driver, so by
> default it should go to the DRM_UT_DRIVER channel. While refactoring
> debug macros later on we might end up with particular messages going to
> ATOMIC or KMS, but DRIVER should be the default.
> 
> 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__)

Should we just get rid of these macros at this point and use
DRM_DEBUG_DRIVER directly?

Konrad
Dmitry Baryshkov Aug. 28, 2024, 7:38 p.m. UTC | #3
On Tue, Aug 27, 2024 at 11:39:45AM GMT, Konrad Dybcio wrote:
> On 2.08.2024 9:47 PM, 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.
> > 
> > I don't think that it is a good idea for a generic DPU_DEBUG macro to be
> > tied to DRM_UT_KMS. It is used to report a debug message from driver, so by
> > default it should go to the DRM_UT_DRIVER channel. While refactoring
> > debug macros later on we might end up with particular messages going to
> > ATOMIC or KMS, but DRIVER should be the default.
> > 
> > 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__)
> 
> Should we just get rid of these macros at this point and use
> DRM_DEBUG_DRIVER directly?

I was hoping to get this into 6.11 as shown by the series subject.
Reworking the debug macros is on my plate, but it going to be more
intrusive. As such, it will probably be a 6.13+ material.
diff mbox series

Patch

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__)