[PATCH] drm: add check for plane functions

[Shirish S]

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.)

>> 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.

>> 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.

Regards,
Shirish S