[PATCH] drm: add check for plane functions

[Shirish S]

On Mon, Mar 20, 2017 at 1:51 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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
As i mentioned earlier also, WARN_ON() wont solve the purpose,
the panic cant be avoided making the system unusable.
May be i will use a WIP patch internally till everything is in place.
Thanks for your inputs.
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Regards,
Shirish S