[Intel-gfx] [PATCH 1/8] drm: Add crtc->name and use it in debug messages
On Tue, Nov 17, 2015 at 11:15:43AM +0100, Daniel Vetter wrote: > On Fri, Nov 13, 2015 at 01:03:48PM +0200, Ville Syrjälä wrote: > > On Fri, Nov 13, 2015 at 12:18:24PM +0200, Jani Nikula wrote: > > > On Thu, 12 Nov 2015, ville.syrjala at linux.intel.com wrote: > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > > index 24c5434..ea00a69 100644 > > > > --- a/drivers/gpu/drm/drm_crtc.c > > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > > @@ -677,6 +677,9 @@ int drm_crtc_init_with_planes(struct drm_device > > > > *dev, struct drm_crtc *crtc, > > > > crtc->dev = dev; > > > > crtc->funcs = funcs; > > > > > > > > + if (!crtc->name) > > > > + crtc->name = ""; > > > > + > > > > > > This, and the cleanup dance you have to do in patch 5/8, are my only > > > gripes with the series. I would prefer it if crtc->name and plane->name > > > were managed dynamically by the init/cleanup functions like > > > connector->name and encoder->name are. However those are easier because > > > the caller doesn't decide the name. > > > > > > Do you think it would be too ugly to have this here: > > > > > > crtc->name = kstrdup(crtc->name ? crtc->name : "", GFP_KERNEL); > > > > Hmm. Yeah that might be a decentish alternative. I initialliy tried to > > do dynamic allocation in both the driver and core, and that made the > > error handling during init rather nasty. But if it's only the core that > > does it, it might be OK. > > > > > > > > It would of course be neater if the name was passed in as parameter, but > > > that would generate quite a bit of unnecessary churn. > > > > Yeah, I've been hitting a lot of drivers recently with a bunch of stuff, > > and was sort of hoping to avoid it this time. But if people prefer a > > passed in parameter, I could do it as well. Maybe this time I could even > > get coccinelle to cooperate with me (whether that happens seems to depend > > on the phase of the moon or something). > > See my suggestion to go with idx-%i, drm_plane_index as the default. > That's fairly reasonable (especially for any hw with true universal planes > shared across all crtc), in which case there's imo not much point in > passing it in directly. I think most hw has some kind of naming scheme for the planes in the spec, so IMO passing it in usually makes sense. So forcing everyone to pass something might encourage people to actually put something sensible there. > And for most drivers you want some printf like > anyway, so the passed-in argument won't result in cleaner code. It will since the driver doesn't have to remember to kfree() it all over the place, or deal with the malloc error. As for the printf thing, we could pass a printf style format string + va args to the function and just pass those through to kvasprintf(). > -Daniel > > > > > > > > > If you are all "meh" I guess I can live with your approach too. > > > > > > BR, > > > Jani. > > > > > > > > > > > > -- > > > Jani Nikula, Intel Open Source Technology Center > > > > -- > > Ville Syrjälä > > Intel OTC > > ___ > > dri-devel mailing list > > dri-devel at lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Ville Syrjälä Intel OTC
[Intel-gfx] [PATCH 1/8] drm: Add crtc->name and use it in debug messages
On Fri, Nov 13, 2015 at 01:03:48PM +0200, Ville Syrjälä wrote: > On Fri, Nov 13, 2015 at 12:18:24PM +0200, Jani Nikula wrote: > > On Thu, 12 Nov 2015, ville.syrjala at linux.intel.com wrote: > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > index 24c5434..ea00a69 100644 > > > --- a/drivers/gpu/drm/drm_crtc.c > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > @@ -677,6 +677,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, > > > struct drm_crtc *crtc, > > > crtc->dev = dev; > > > crtc->funcs = funcs; > > > > > > + if (!crtc->name) > > > + crtc->name = ""; > > > + > > > > This, and the cleanup dance you have to do in patch 5/8, are my only > > gripes with the series. I would prefer it if crtc->name and plane->name > > were managed dynamically by the init/cleanup functions like > > connector->name and encoder->name are. However those are easier because > > the caller doesn't decide the name. > > > > Do you think it would be too ugly to have this here: > > > > crtc->name = kstrdup(crtc->name ? crtc->name : "", GFP_KERNEL); > > Hmm. Yeah that might be a decentish alternative. I initialliy tried to > do dynamic allocation in both the driver and core, and that made the > error handling during init rather nasty. But if it's only the core that > does it, it might be OK. > > > > > It would of course be neater if the name was passed in as parameter, but > > that would generate quite a bit of unnecessary churn. > > Yeah, I've been hitting a lot of drivers recently with a bunch of stuff, > and was sort of hoping to avoid it this time. But if people prefer a > passed in parameter, I could do it as well. Maybe this time I could even > get coccinelle to cooperate with me (whether that happens seems to depend > on the phase of the moon or something). See my suggestion to go with idx-%i, drm_plane_index as the default. That's fairly reasonable (especially for any hw with true universal planes shared across all crtc), in which case there's imo not much point in passing it in directly. And for most drivers you want some printf like anyway, so the passed-in argument won't result in cleaner code. -Daniel > > > > > If you are all "meh" I guess I can live with your approach too. > > > > BR, > > Jani. > > > > > > > > -- > > Jani Nikula, Intel Open Source Technology Center > > -- > Ville Syrjälä > Intel OTC > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[Intel-gfx] [PATCH 1/8] drm: Add crtc->name and use it in debug messages
On Fri, Nov 13, 2015 at 12:18:24PM +0200, Jani Nikula wrote: > On Thu, 12 Nov 2015, ville.syrjala at linux.intel.com wrote: > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 24c5434..ea00a69 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -677,6 +677,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, > > struct drm_crtc *crtc, > > crtc->dev = dev; > > crtc->funcs = funcs; > > > > + if (!crtc->name) > > + crtc->name = ""; > > + > > This, and the cleanup dance you have to do in patch 5/8, are my only > gripes with the series. I would prefer it if crtc->name and plane->name > were managed dynamically by the init/cleanup functions like > connector->name and encoder->name are. However those are easier because > the caller doesn't decide the name. > > Do you think it would be too ugly to have this here: > > crtc->name = kstrdup(crtc->name ? crtc->name : "", GFP_KERNEL); Hmm. Yeah that might be a decentish alternative. I initialliy tried to do dynamic allocation in both the driver and core, and that made the error handling during init rather nasty. But if it's only the core that does it, it might be OK. > > It would of course be neater if the name was passed in as parameter, but > that would generate quite a bit of unnecessary churn. Yeah, I've been hitting a lot of drivers recently with a bunch of stuff, and was sort of hoping to avoid it this time. But if people prefer a passed in parameter, I could do it as well. Maybe this time I could even get coccinelle to cooperate with me (whether that happens seems to depend on the phase of the moon or something). > > If you are all "meh" I guess I can live with your approach too. > > BR, > Jani. > > > > -- > Jani Nikula, Intel Open Source Technology Center -- Ville Syrjälä Intel OTC
[Intel-gfx] [PATCH 1/8] drm: Add crtc->name and use it in debug messages
On Thu, 12 Nov 2015, ville.syrjala at linux.intel.com wrote: > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 24c5434..ea00a69 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -677,6 +677,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, > struct drm_crtc *crtc, > crtc->dev = dev; > crtc->funcs = funcs; > > + if (!crtc->name) > + crtc->name = ""; > + This, and the cleanup dance you have to do in patch 5/8, are my only gripes with the series. I would prefer it if crtc->name and plane->name were managed dynamically by the init/cleanup functions like connector->name and encoder->name are. However those are easier because the caller doesn't decide the name. Do you think it would be too ugly to have this here: crtc->name = kstrdup(crtc->name ? crtc->name : "", GFP_KERNEL); It would of course be neater if the name was passed in as parameter, but that would generate quite a bit of unnecessary churn. If you are all "meh" I guess I can live with your approach too. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center
[PATCH 1/8] drm: Add crtc->name and use it in debug messages
From: Ville Syrjälä
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_atomic.c| 41 ++-
drivers/gpu/drm/drm_atomic_helper.c | 56 +++--
drivers/gpu/drm/drm_crtc.c | 8 --
drivers/gpu/drm/drm_crtc_helper.c | 24
include/drm/drm_crtc.h | 2 ++
5 files changed, 71 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7bb3845..2944655 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -288,8 +288,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
state->crtcs[index] = crtc;
crtc_state->state = state;
- DRM_DEBUG_ATOMIC("Added [CRTC:%d] %p state to %p\n",
-crtc->base.id, crtc_state, state);
+ DRM_DEBUG_ATOMIC("Added [CRTC:%d:%s] %p state to %p\n",
+crtc->base.id, crtc->name, crtc_state, state);
return crtc_state;
}
@@ -480,8 +480,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
*/
if (state->active && !state->enable) {
- DRM_DEBUG_ATOMIC("[CRTC:%d] active without enabled\n",
-crtc->base.id);
+ DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active without enabled\n",
+crtc->base.id, crtc->name);
return -EINVAL;
}
@@ -490,15 +490,15 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
* be able to trigger. */
if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
WARN_ON(state->enable && !state->mode_blob)) {
- DRM_DEBUG_ATOMIC("[CRTC:%d] enabled without mode blob\n",
-crtc->base.id);
+ DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled without mode blob\n",
+crtc->base.id, crtc->name);
return -EINVAL;
}
if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
WARN_ON(!state->enable && state->mode_blob)) {
- DRM_DEBUG_ATOMIC("[CRTC:%d] disabled with mode blob\n",
-crtc->base.id);
+ DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled with mode blob\n",
+crtc->base.id, crtc->name);
return -EINVAL;
}
@@ -980,8 +980,8 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state
*plane_state,
}
if (crtc)
- DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d]\n",
-plane_state, crtc->base.id);
+ DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d:%s]\n",
+plane_state, crtc->base.id, crtc->name);
else
DRM_DEBUG_ATOMIC("Link plane state %p to [NOCRTC]\n",
plane_state);
@@ -1048,8 +1048,8 @@ drm_atomic_set_crtc_for_connector(struct
drm_connector_state *conn_state,
conn_state->crtc = crtc;
if (crtc)
- DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d]\n",
-conn_state, crtc->base.id);
+ DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n",
+conn_state, crtc->base.id, crtc->name);
else
DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n",
conn_state);
@@ -1088,8 +1088,8 @@ drm_atomic_add_affected_connectors(struct
drm_atomic_state *state,
if (ret)
return ret;
- DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d] to %p\n",
-crtc->base.id, state);
+ DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d:%s] to
%p\n",
+crtc->base.id, crtc->name, state);
/*
* Changed connectors are already in @state, so only need to look at the
@@ -1169,8 +1169,9 @@ drm_atomic_connectors_for_crtc(struct drm_atomic_state
*state,
num_connected_connectors++;
}
- DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d]\n",
-state, num_connected_connectors, crtc->base.id);
+ DRM_DEBUG_ATOMIC("State %p has %i connectors for [CRTC:%d:%s]\n",
+state, num_connected_connectors,
+crtc->base.id, crtc->name);
return num_connected_connectors;
}
@@ -1237,8 +1238,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
for_each_crtc_in_state(state, crtc, crtc_state, i) {
ret = drm_atomic_crtc_check(crtc, crtc_state);
if (ret) {
- DRM_DEBUG_ATOMIC("[CRTC:%d] atomic core check failed\n",
-crtc->base.id);
+ DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check
failed\n",
