[PATCH] drm: add check for plane functions

[Eric Anholt]

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.
>> > 
>> > 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;
> }
>
> rather than putting the WARN_ON()s around each call of
> funcs->mandatory_thing().
>
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20170317/1496b9f5/attachment.sig>