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@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch