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@anholt.net wrote:
Ville Syrjälä ville.syrjala@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@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@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