Re: [Intel-gfx] [PATCH 1/8] drm: Add crtc->name and use it in debug messages

[Daniel Vetter <daniel@ffwll.ch>]

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. 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel