[PATCH] drm: add check for plane functions

[Daniel Vetter]

On Mon, Mar 20, 2017 at 09:58:01AM +0530, Shirish S wrote:
> First of all, thanks for your comments/insights.
> 
> On Sat, Mar 18, 2017 at 12:59 AM, Eric Anholt <eric at anholt.net> wrote:
> > Ville Syrjälä <ville.syrjala at linux.intel.com> writes:
> >
> >> 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 at 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.
> >>> >
> All the plane->funcs are guarded before being called , be it:
>              late_register()
>              early_unregister()
>             atomic_destroy_state() etc.,
> only update/disable_plane() are called without checking their
> existence, am just extending  the protocol.
> >>> > 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;
> >> }
> >>
> I think since the motive here is to
> * convey user space that it does not have permissions to
> update/disable available plane due to implementation issues.
> * Keeping system alive/usable after non-permitted call.
> Adding  a WARN_ON() trace showing something is missing at boot/insmod
> time, wont solve the purpose.
> 
> This  development phase here could be setting-up infra for adding a
> plane available on hardware,populate its capabilities
> and to know how user space reads it and tweak it before moving to
> configuring registers.
> 
> To add to what @Eric Anholt mentioned, without this patch developer
> comes to know about
> the mandatory functions required in a real tough way of panic and
> system freezes,
> just because the core framework invokes a NULL function pointer
> without checking.
> (Am re-stressing here, that only update/disable planes are exceptions
> rest all have required checks.)

Eric acked Ville's idea, not your patch.
> 
> >> rather than putting the WARN_ON()s around each call of
> >> funcs->mandatory_thing().
> >>
> There are similar checks around every
> "[crtc/encoder]->funcs->[hooked_up_function specific to vendor]",
> including  plane functions called in drm_plane.c & other places like:
>      drivers/gpu/drm/drm_crtc_helper.c:1074: if
> (plane->funcs->atomic_duplicate_state)
>      drivers/gpu/drm/drm_mode_config.c:176:          if (plane->funcs->reset)
>      drivers/gpu/drm/drm_plane.c:162:                if
> (plane->funcs->late_register)
>      drivers/gpu/drm/drm_plane.c:242:        if (plane->state &&
> plane->funcs->atomic_destroy_state)
> and so on...
> For consistency sake lets have this check.

Those are different functions. They are in transitional helpers, where
we explicitly assume not all the atomic bits are ready yet.

Different use-case, different semantics.

> >> That will fail gracefully (which I guess is what people are after here),
> >> and gives the developer a clear message what's missing.
> >
> > Having this in our init functions for funcs and helpers would have saved
> > me tons of time in vc4 and clcd.
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> 
> Thanks again for your comments, all am trying here is to only fix a
> bug that shall enable developers in a positive way.

See Ville's proposal, I think that's a good idea. Volunteered to review
the various docs and make sure we have these checks in the various _init()
functions?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch