Re: [PATCH] drm: add check for plane functions

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

On Fri, Mar 17, 2017 at 05:57:52PM +0100, Daniel Vetter wrote:
> On Fri, Mar 17, 2017 at 01:08:43PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 17, 2017 at 03:46:34PM +0530, Shirish S wrote:
> > > On Fri, Mar 17, 2017 at 3:26 PM, Ville Syrjälä
> > > <ville.syrjala@linux.intel.com> wrote:
> > > > On Fri, Mar 17, 2017 at 01:25:08PM +0530, Shirish S wrote:
> > > >> update_plane() and disable_plane() functions
> > > >> assoiciated with setting plane are called
> > > >> without any check, causing kernel panic.
> > > >
> > > > Why are you registering a plane without the funcs?
> > > >
> > > Basically, enabling planes and making them fully functional is
> > > generally a 2 -step process,
> > > so i suggest for new drivers wanting to implement/re-design  planes,
> > > would like to tap
> > > the flow at enabling(listing caps) and later at ensuring it works.
> > 
> > I don't think there's much point in exposing something that
> > doesn't work. And even if you do, you could always just use
> > stub functions.
> 
> Yes, just wire up stub functions if you want to enable planes with
> multi-step patch series.
> 
> > > I noticed that there is a underlying assumption only for
> > > plane->(funcs) are implemented, whereas for
> > > other function for crtc/connector/encoder function calls there is a
> > > sanity check(or WARN_ON) through out the framework.
> > > 
> > > I believe this check wont cause any performance/functional impact.
> > > Please let me know if am missing anything.
> > > And further more help developers to focus on enabling planes via
> > > various tests without causing reboots/system hangs.
> > 
> > I don't particularly like adding more unconditional runtime checks
> > that just to protect developers from themselves. If you really
> > think there's value in these, then at least add the checks into
> > the plane init codepath so that it's a one time cost.
> > 
> > The same approach could be used for all the other non-optional
> > hooks. Otherwise the same WARN_ON()s would have to be sprinkled
> > all over the place, and there's always the risk of missing a few
> > codepaths that call a specific hook.
> 
> I think for these here there's negative value - it allows developers to
> create completely broken planes. Stub functions really seem like a much
> better idea.

I was thinking 

drm_whatever_init()
{
	if (WARN_ON(!funcs->mandatory_thing))
		return -EINVAL;
}

rather than putting the WARN_ON()s around each call of
funcs->mandatory_thing().

That will fail gracefully (which I guess is what people are after here),
and gives the developer a clear message what's missing.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx