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

["Ville Syrjälä" <ville.syrjala-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>]

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