[PATCH] drm: add check for plane functions

[PATCH] drm: add check for plane functions

[Shirish S]

update_plane() and disable_plane() functions
assoiciated with setting plane are called
without any check, causing kernel panic.

This patch adds the required check to avoid it.

Change-Id: I0d6792608b33e674c217388aa57c4b7d680d9bc7
Signed-off-by: Shirish S <shirish.s@amd.com>
---
 drivers/gpu/drm/drm_plane.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 249c0ae..f675f8b 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -456,6 +456,12 @@ static int __setplane_internal(struct drm_plane *plane,
 {
 	int ret = 0;
 
+	if (plane->funcs->disable_plane == NULL ||
+				plane->funcs->update_plane == NULL) {
+		DRM_ERROR("plane funcs not implemented\n");
+		ret = -EPERM;
+		goto out;
+	}
 	/* No fb means shut it down */
 	if (!fb) {
 		plane->old_fb = plane->fb;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Ville Syrjälä]

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?

> 
> This patch adds the required check to avoid it.
> 
> Change-Id: I0d6792608b33e674c217388aa57c4b7d680d9bc7
> Signed-off-by: Shirish S <shirish.s@amd.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..f675f8b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -456,6 +456,12 @@ static int __setplane_internal(struct drm_plane *plane,
>  {
>  	int ret = 0;
>  
> +	if (plane->funcs->disable_plane == NULL ||
> +				plane->funcs->update_plane == NULL) {
> +		DRM_ERROR("plane funcs not implemented\n");
> +		ret = -EPERM;
> +		goto out;
> +	}
>  	/* No fb means shut it down */
>  	if (!fb) {
>  		plane->old_fb = plane->fb;
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Shirish S]

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 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.
>>
>> This patch adds the required check to avoid it.
>>
>> Change-Id: I0d6792608b33e674c217388aa57c4b7d680d9bc7
>> Signed-off-by: Shirish S <shirish.s@amd.com>
>> ---
>>  drivers/gpu/drm/drm_plane.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
>> index 249c0ae..f675f8b 100644
>> --- a/drivers/gpu/drm/drm_plane.c
>> +++ b/drivers/gpu/drm/drm_plane.c
>> @@ -456,6 +456,12 @@ static int __setplane_internal(struct drm_plane *plane,
>>  {
>>       int ret = 0;
>>
>> +     if (plane->funcs->disable_plane == NULL ||
>> +                             plane->funcs->update_plane == NULL) {
>> +             DRM_ERROR("plane funcs not implemented\n");
>> +             ret = -EPERM;
>> +             goto out;
>> +     }
>>       /* No fb means shut it down */
>>       if (!fb) {
>>               plane->old_fb = plane->fb;
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Ville Syrjälä]

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.

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

> >>
> >> This patch adds the required check to avoid it.
> >>
> >> Change-Id: I0d6792608b33e674c217388aa57c4b7d680d9bc7
> >> Signed-off-by: Shirish S <shirish.s@amd.com>
> >> ---
> >>  drivers/gpu/drm/drm_plane.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >> index 249c0ae..f675f8b 100644
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >> @@ -456,6 +456,12 @@ static int __setplane_internal(struct drm_plane *plane,
> >>  {
> >>       int ret = 0;
> >>
> >> +     if (plane->funcs->disable_plane == NULL ||
> >> +                             plane->funcs->update_plane == NULL) {
> >> +             DRM_ERROR("plane funcs not implemented\n");
> >> +             ret = -EPERM;
> >> +             goto out;
> >> +     }
> >>       /* No fb means shut it down */
> >>       if (!fb) {
> >>               plane->old_fb = plane->fb;
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Daniel Vetter]

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

Note that for the helpers we allow you to not have a lot of the hooks,
since there's indeed piles of hw where they are not needed. But this one
here really is.

Also, with an atomic driver you get it for free, so there's really never
any reason at all not too hook this up.

Add this to the list of reasons why you _really_ should have an atomic
driver :-)
-Daniel

> 
> > >>
> > >> This patch adds the required check to avoid it.
> > >>
> > >> Change-Id: I0d6792608b33e674c217388aa57c4b7d680d9bc7
> > >> Signed-off-by: Shirish S <shirish.s@amd.com>
> > >> ---
> > >>  drivers/gpu/drm/drm_plane.c | 6 ++++++
> > >>  1 file changed, 6 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> > >> index 249c0ae..f675f8b 100644
> > >> --- a/drivers/gpu/drm/drm_plane.c
> > >> +++ b/drivers/gpu/drm/drm_plane.c
> > >> @@ -456,6 +456,12 @@ static int __setplane_internal(struct drm_plane *plane,
> > >>  {
> > >>       int ret = 0;
> > >>
> > >> +     if (plane->funcs->disable_plane == NULL ||
> > >> +                             plane->funcs->update_plane == NULL) {
> > >> +             DRM_ERROR("plane funcs not implemented\n");
> > >> +             ret = -EPERM;
> > >> +             goto out;
> > >> +     }
> > >>       /* No fb means shut it down */
> > >>       if (!fb) {
> > >>               plane->old_fb = plane->fb;
> > >> --
> > >> 2.7.4
> > >>
> > >> _______________________________________________
> > >> dri-devel mailing list
> > >> dri-devel@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2924 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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.)

>> 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@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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Shirish S]

On Mon, Mar 20, 2017 at 1:51 PM, Daniel Vetter <daniel@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@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
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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Harry Wentland]

On 2017-03-20 05:42 AM, Shirish S wrote:
> On Mon, Mar 20, 2017 at 1:51 PM, Daniel Vetter <daniel@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@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
> 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.

I was MIA yesterday so I'm a bit late to the discussion. I like Ville's 
idea, though.

Just to rephrase: the only function pointers we want to NULL check at 
runtime are those that are deliberately left to be optional. All 
mandatory functions are allowed to crash at runtime.

Ville's suggestion is to make apparent inside our _init functions which 
function pointers are mandatory.

You can still keep the existing patch for your own benefit but I don't 
think it'll fly for upstream.

I'll leave it up to you whether you want to implement the NULL function 
pointer checks in the _init functions.

Harry

>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
> Regards,
> Shirish S
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm: add check for plane functions

[Harry Wentland]

On 2017-03-17 03:55 AM, Shirish S wrote:
> update_plane() and disable_plane() functions
> assoiciated with setting plane are called
> without any check, causing kernel panic.
>
> This patch adds the required check to avoid it.
>
> Change-Id: I0d6792608b33e674c217388aa57c4b7d680d9bc7
> Signed-off-by: Shirish S <shirish.s@amd.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 249c0ae..f675f8b 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -456,6 +456,12 @@ static int __setplane_internal(struct drm_plane *plane,
>  {
>  	int ret = 0;
>
> +	if (plane->funcs->disable_plane == NULL ||
> +				plane->funcs->update_plane == NULL) {

Thanks, Shirish, for sending these out on amd-gfx and dri-devel.

I've had a second look at this. It seems like there are a bunch of other 
places that assume these functions are implemented by drivers. Does this 
check serve any purpose other than as an immediate step when 
implementing universal planes?

Harry

> +		DRM_ERROR("plane funcs not implemented\n");
> +		ret = -EPERM;
> +		goto out;
> +	}
>  	/* No fb means shut it down */
>  	if (!fb) {
>  		plane->old_fb = plane->fb;
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx