[PATCH] drm: add check for plane functions
shirish.s12 at gmail.com
Mon Mar 20 04:28:01 UTC 2017
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:
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
>> 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
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
just because the core framework invokes a NULL function pointer
(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
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_mode_config.c:176: if (plane->funcs->reset)
drivers/gpu/drm/drm_plane.c:242: if (plane->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
Thanks again for your comments, all am trying here is to only fix a
bug that shall enable developers in a positive way.
More information about the amd-gfx