| Message ID | 20240802-dpu-fix-wb-v2-2-7eac9eb8e895@linaro.org |
|---|---|
| State | Accepted |
| Commit | df24373435f5899a2a98b7d377479c8d4376613b |
| Headers | show |
| Series | drm/msm/dpu: two fixes targeting 6.11 | expand |
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>
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
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 --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. 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(-)