[Intel-gfx] [PATCH 2/9] drm/print: add drm_debug_enabled()

Eric Engestrom eric.engestrom at intel.com
Fri Sep 20 11:51:29 UTC 2019


On Monday, 2019-09-16 16:23:13 +0300, Jani Nikula wrote:
> On Mon, 16 Sep 2019, Eric Engestrom <eric.engestrom at intel.com> wrote:
> > On Monday, 2019-09-16 11:53:24 +0300, Jani Nikula wrote:
> >> On Fri, 13 Sep 2019, Eric Engestrom <eric.engestrom at intel.com> wrote:
> >> > On Friday, 2019-09-13 14:51:39 +0300, Jani Nikula wrote:
> >> >> Add helper to check if a drm debug category is enabled. Convert drm core
> >> >> to use it. No functional changes.
> >> >> 
> >> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> >> ---
> >> >>  drivers/gpu/drm/drm_atomic_uapi.c     | 2 +-
> >> >>  drivers/gpu/drm/drm_dp_mst_topology.c | 6 +++---
> >> >>  drivers/gpu/drm/drm_edid.c            | 2 +-
> >> >>  drivers/gpu/drm/drm_edid_load.c       | 2 +-
> >> >>  drivers/gpu/drm/drm_mipi_dbi.c        | 4 ++--
> >> >>  drivers/gpu/drm/drm_print.c           | 4 ++--
> >> >>  drivers/gpu/drm/drm_vblank.c          | 6 +++---
> >> >>  include/drm/drm_print.h               | 5 +++++
> >> >>  8 files changed, 18 insertions(+), 13 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> >> >> index 5a5b42db6f2a..6576cd997cbd 100644
> >> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> >> @@ -1406,7 +1406,7 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> >> >>  	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> >> >>  		ret = drm_atomic_nonblocking_commit(state);
> >> >>  	} else {
> >> >> -		if (unlikely(drm_debug & DRM_UT_STATE))
> >> >> +		if (unlikely(drm_debug_enabled(DRM_UT_STATE)))
> >> >>  			drm_atomic_print_state(state);
> >> >>  
> >> >>  		ret = drm_atomic_commit(state);
> >> >> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c b/drivers/gpu/drm/drm_dp_mst_topology.c
> >> >> index 97216099a718..f47c5b6b51f7 100644
> >> >> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
> >> >> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
> >> >> @@ -1180,7 +1180,7 @@ static int drm_dp_mst_wait_tx_reply(struct drm_dp_mst_branch *mstb,
> >> >>  		}
> >> >>  	}
> >> >>  out:
> >> >> -	if (unlikely(ret == -EIO && drm_debug & DRM_UT_DP)) {
> >> >> +	if (unlikely(ret == -EIO && drm_debug_enabled(DRM_UT_DP))) {
> >> >>  		struct drm_printer p = drm_debug_printer(DBG_PREFIX);
> >> >>  
> >> >>  		drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
> >> >> @@ -2321,7 +2321,7 @@ static int process_single_tx_qlock(struct drm_dp_mst_topology_mgr *mgr,
> >> >>  	idx += tosend + 1;
> >> >>  
> >> >>  	ret = drm_dp_send_sideband_msg(mgr, up, chunk, idx);
> >> >> -	if (unlikely(ret && drm_debug & DRM_UT_DP)) {
> >> >> +	if (unlikely(ret && drm_debug_enabled(DRM_UT_DP))) {
> >> >>  		struct drm_printer p = drm_debug_printer(DBG_PREFIX);
> >> >>  
> >> >>  		drm_printf(&p, "sideband msg failed to send\n");
> >> >> @@ -2388,7 +2388,7 @@ static void drm_dp_queue_down_tx(struct drm_dp_mst_topology_mgr *mgr,
> >> >>  	mutex_lock(&mgr->qlock);
> >> >>  	list_add_tail(&txmsg->next, &mgr->tx_msg_downq);
> >> >>  
> >> >> -	if (unlikely(drm_debug & DRM_UT_DP)) {
> >> >> +	if (unlikely(drm_debug_enabled(DRM_UT_DP))) {
> >> >>  		struct drm_printer p = drm_debug_printer(DBG_PREFIX);
> >> >>  
> >> >>  		drm_dp_mst_dump_sideband_msg_tx(&p, txmsg);
> >> >> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> >> index 12c783f4d956..58dad4d24cd4 100644
> >> >> --- a/drivers/gpu/drm/drm_edid.c
> >> >> +++ b/drivers/gpu/drm/drm_edid.c
> >> >> @@ -1551,7 +1551,7 @@ static void connector_bad_edid(struct drm_connector *connector,
> >> >>  {
> >> >>  	int i;
> >> >>  
> >> >> -	if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS))
> >> >> +	if (connector->bad_edid_counter++ && !drm_debug_enabled(DRM_UT_KMS))
> >> >>  		return;
> >> >>  
> >> >>  	dev_warn(connector->dev->dev,
> >> >> diff --git a/drivers/gpu/drm/drm_edid_load.c b/drivers/gpu/drm/drm_edid_load.c
> >> >> index d38b3b255926..37d8ba3ddb46 100644
> >> >> --- a/drivers/gpu/drm/drm_edid_load.c
> >> >> +++ b/drivers/gpu/drm/drm_edid_load.c
> >> >> @@ -175,7 +175,7 @@ static void *edid_load(struct drm_connector *connector, const char *name,
> >> >>  	u8 *edid;
> >> >>  	int fwsize, builtin;
> >> >>  	int i, valid_extensions = 0;
> >> >> -	bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS);
> >> >> +	bool print_bad_edid = !connector->bad_edid_counter || drm_debug_enabled(DRM_UT_KMS);
> >> >>  
> >> >>  	builtin = match_string(generic_edid_name, GENERIC_EDIDS, name);
> >> >>  	if (builtin >= 0) {
> >> >> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> >> >> index f8154316a3b0..ccfb5b33c5e3 100644
> >> >> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> >> >> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> >> >> @@ -783,7 +783,7 @@ static int mipi_dbi_spi1e_transfer(struct mipi_dbi *dbi, int dc,
> >> >>  	int i, ret;
> >> >>  	u8 *dst;
> >> >>  
> >> >> -	if (drm_debug & DRM_UT_DRIVER)
> >> >> +	if (drm_debug_enabled(DRM_UT_DRIVER))
> >> >>  		pr_debug("[drm:%s] dc=%d, max_chunk=%zu, transfers:\n",
> >> >>  			 __func__, dc, max_chunk);
> >> >>  
> >> >> @@ -907,7 +907,7 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *dbi, int dc,
> >> >>  	max_chunk = dbi->tx_buf9_len;
> >> >>  	dst16 = dbi->tx_buf9;
> >> >>  
> >> >> -	if (drm_debug & DRM_UT_DRIVER)
> >> >> +	if (drm_debug_enabled(DRM_UT_DRIVER))
> >> >>  		pr_debug("[drm:%s] dc=%d, max_chunk=%zu, transfers:\n",
> >> >>  			 __func__, dc, max_chunk);
> >> >>  
> >> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> >> >> index c9b57012d412..a7c89ec5ff26 100644
> >> >> --- a/drivers/gpu/drm/drm_print.c
> >> >> +++ b/drivers/gpu/drm/drm_print.c
> >> >> @@ -264,7 +264,7 @@ void drm_dev_dbg(const struct device *dev, unsigned int category,
> >> >>  	struct va_format vaf;
> >> >>  	va_list args;
> >> >>  
> >> >> -	if (!(drm_debug & category))
> >> >> +	if (!drm_debug_enabled(category))
> >> >>  		return;
> >> >>  
> >> >>  	va_start(args, format);
> >> >> @@ -287,7 +287,7 @@ void drm_dbg(unsigned int category, const char *format, ...)
> >> >>  	struct va_format vaf;
> >> >>  	va_list args;
> >> >>  
> >> >> -	if (!(drm_debug & category))
> >> >> +	if (!drm_debug_enabled(category))
> >> >>  		return;
> >> >>  
> >> >>  	va_start(args, format);
> >> >> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> >> >> index 9c6899758bc9..4f7962b6427b 100644
> >> >> --- a/drivers/gpu/drm/drm_vblank.c
> >> >> +++ b/drivers/gpu/drm/drm_vblank.c
> >> >> @@ -332,7 +332,7 @@ u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc)
> >> >>  	u64 vblank;
> >> >>  	unsigned long flags;
> >> >>  
> >> >> -	WARN_ONCE(drm_debug & DRM_UT_VBL && !dev->driver->get_vblank_timestamp,
> >> >> +	WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && !dev->driver->get_vblank_timestamp,
> >> >>  		  "This function requires support for accurate vblank timestamps.");
> >> >>  
> >> >>  	spin_lock_irqsave(&dev->vblank_time_lock, flags);
> >> >> @@ -706,7 +706,7 @@ bool drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> >> >>  	 */
> >> >>  	*vblank_time = ktime_sub_ns(etime, delta_ns);
> >> >>  
> >> >> -	if ((drm_debug & DRM_UT_VBL) == 0)
> >> >> +	if (!drm_debug_enabled(DRM_UT_VBL))
> >> >>  		return true;
> >> >>  
> >> >>  	ts_etime = ktime_to_timespec64(etime);
> >> >> @@ -1352,7 +1352,7 @@ void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
> >> >>  	assert_spin_locked(&dev->vblank_time_lock);
> >> >>  
> >> >>  	vblank = &dev->vblank[pipe];
> >> >> -	WARN_ONCE((drm_debug & DRM_UT_VBL) && !vblank->framedur_ns,
> >> >> +	WARN_ONCE(drm_debug_enabled(DRM_UT_VBL) && !vblank->framedur_ns,
> >> >>  		  "Cannot compute missed vblanks without frame duration\n");
> >> >>  	framedur_ns = vblank->framedur_ns;
> >> >>  
> >> >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> >> >> index e5c421abce48..e13f901312a4 100644
> >> >> --- a/include/drm/drm_print.h
> >> >> +++ b/include/drm/drm_print.h
> >> >> @@ -294,6 +294,11 @@ static inline struct drm_printer drm_err_printer(const char *prefix)
> >> >>  #define DRM_UT_LEASE		0x80
> >> >>  #define DRM_UT_DP		0x100
> >> >>  
> >> >> +static inline bool drm_debug_enabled(unsigned int category)
> >> >> +{
> >> >> +	return drm_debug & category;
> >> >
> >> > Worth making that `return unlikely(drm_debug & category);` to make sure
> >> > the preloaded path is always to skip the debug stuff?
> >> 
> >> If that would let us drop the unlikely() wrapping from the various users
> >> of this function, agreed, it would be great.
> >> 
> >> However I'm not at all certain unlikely() here will propagate like
> >> that. I just don't know. Do you?
> >
> > I don't know either, but I expect it would?
> > Otherwise, this definitely would:
> >   #define drm_debug_enabled(cat) unlikely(drm_debug & category)
> >
> > And yeah, dropping the unlikely() sprinkled around some drm_debug_enabled()
> > calls is a definite bonus.
> 
> With either of the approaches, would it be okay to change call sites
> like this:
> 
> - 	if (unlikely(ret && drm_debug_enabled(DRM_UT_DP))) {
> + 	if (ret && drm_debug_enabled(DRM_UT_DP)) {
> 
> In theory the compiler should be able to conclude that the unlikely
> covers the whole branch even if ret is short-circuit evaluated.

Yeah, the compiler should definitely be smart enough to figure out that
`ret && probably_false` is `probably_false` :)

> 
> 
> BR,
> Jani.
> 
> 
> 
> >
> >> 
> >> BR,
> >> Jani.
> >> 
> >> 
> >> >
> >> >> +}
> >> >> +
> >> >>  __printf(3, 4)
> >> >>  void drm_dev_printk(const struct device *dev, const char *level,
> >> >>  		    const char *format, ...);
> >> >> -- 
> >> >> 2.20.1
> >> >> 
> >> >> _______________________________________________
> >> >> dri-devel mailing list
> >> >> dri-devel at lists.freedesktop.org
> >> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> > _______________________________________________
> >> > dri-devel mailing list
> >> > dri-devel at lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >> 
> >> -- 
> >> Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center


More information about the Intel-gfx mailing list