| Message ID | 20210521090959.1663703-10-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] drm/amdgpu: Comply with implicit fencing rules | expand |
> It's tedious to review this all the time, and my audit showed that > arcpgu actually forgot to set this. > > Make this the default and stop worrying. > > Again I sprinkled WARN_ON_ONCE on top to make sure we don't have > strange combinations of hooks: cleanup_fb without prepare_fb doesn't > make sense, and since simpler drivers are all new they better be GEM > based drivers. > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Maxime Ripard <mripard@kernel.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: David Airlie <airlied@linux.ie> > Cc: Daniel Vetter <daniel@ffwll.ch> > --- > drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++-- > include/drm/drm_simple_kms_helper.h | 7 +++++-- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > index 0b095a313c44..1a97571d97d9 100644 > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > @@ -9,6 +9,8 @@ > #include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_bridge.h> > +#include <drm/drm_drv.h> > +#include <drm/drm_gem_atomic_helper.h> > #include <drm/drm_managed.h> > #include <drm/drm_plane_helper.h> > #include <drm/drm_probe_helper.h> > @@ -225,8 +227,14 @@ static int drm_simple_kms_plane_prepare_fb(struct drm_plane *plane, > struct drm_simple_display_pipe *pipe; > > pipe = container_of(plane, struct drm_simple_display_pipe, plane); > - if (!pipe->funcs || !pipe->funcs->prepare_fb) > - return 0; > + if (!pipe->funcs || !pipe->funcs->prepare_fb) { > + if (WARN_ON_ONCE(drm_core_check_feature(plane->dev, DRIVER_GEM))) Shouldn't this check be inverted? Looks like it warns on GEM drivers. With that considered: Acked-by: Noralf Trønnes <noralf@tronnes.org> Hopefully this reply will thread correctly, I had to reply from lore (I wasn't cc'ed) and I don't know if Thunderbird sets In-Reply-To. I'm not subscribed to dri-devel anymore since I'm winding down my Linux work and dri-devel is such a high volume list. Noralf > + return 0; > + > + WARN_ON_ONCE(pipe->funcs && pipe->funcs->cleanup_fb); > + > + return drm_gem_simple_display_pipe_prepare_fb(pipe, state); > + } > > return pipe->funcs->prepare_fb(pipe, state); > }
On Tue, May 25, 2021 at 07:48:12PM +0200, Noralf Trønnes wrote: > > It's tedious to review this all the time, and my audit showed that > > arcpgu actually forgot to set this. > > > > Make this the default and stop worrying. > > > > Again I sprinkled WARN_ON_ONCE on top to make sure we don't have > > strange combinations of hooks: cleanup_fb without prepare_fb doesn't > > make sense, and since simpler drivers are all new they better be GEM > > based drivers. > > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Maxime Ripard <mripard@kernel.org> > > Cc: Thomas Zimmermann <tzimmermann@suse.de> > > Cc: David Airlie <airlied@linux.ie> > > Cc: Daniel Vetter <daniel@ffwll.ch> > > --- > > drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++-- > > include/drm/drm_simple_kms_helper.h | 7 +++++-- > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c > b/drivers/gpu/drm/drm_simple_kms_helper.c > > index 0b095a313c44..1a97571d97d9 100644 > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > > @@ -9,6 +9,8 @@ > > #include <drm/drm_atomic.h> > > #include <drm/drm_atomic_helper.h> > > #include <drm/drm_bridge.h> > > +#include <drm/drm_drv.h> > > +#include <drm/drm_gem_atomic_helper.h> > > #include <drm/drm_managed.h> > > #include <drm/drm_plane_helper.h> > > #include <drm/drm_probe_helper.h> > > @@ -225,8 +227,14 @@ static int drm_simple_kms_plane_prepare_fb(struct > drm_plane *plane, > > struct drm_simple_display_pipe *pipe; > > > > pipe = container_of(plane, struct drm_simple_display_pipe, plane); > > - if (!pipe->funcs || !pipe->funcs->prepare_fb) > > - return 0; > > + if (!pipe->funcs || !pipe->funcs->prepare_fb) { > > + if (WARN_ON_ONCE(drm_core_check_feature(plane->dev, DRIVER_GEM))) > > Shouldn't this check be inverted? Looks like it warns on GEM drivers. Ah yes, I'll fix. > With that considered: > > Acked-by: Noralf Trønnes <noralf@tronnes.org> > > Hopefully this reply will thread correctly, I had to reply from lore (I > wasn't cc'ed) and I don't know if Thunderbird sets In-Reply-To. I'm not > subscribed to dri-devel anymore since I'm winding down my Linux work and > dri-devel is such a high volume list. Thanks a lot for taking a look, threaded all correctly. -Daniel > Noralf > > > + return 0; > > + > > + WARN_ON_ONCE(pipe->funcs && pipe->funcs->cleanup_fb); > > + > > + return drm_gem_simple_display_pipe_prepare_fb(pipe, state); > > + } > > > > return pipe->funcs->prepare_fb(pipe, state); > > }
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 0b095a313c44..1a97571d97d9 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -9,6 +9,8 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_drv.h> +#include <drm/drm_gem_atomic_helper.h> #include <drm/drm_managed.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> @@ -225,8 +227,14 @@ static int drm_simple_kms_plane_prepare_fb(struct drm_plane *plane, struct drm_simple_display_pipe *pipe; pipe = container_of(plane, struct drm_simple_display_pipe, plane); - if (!pipe->funcs || !pipe->funcs->prepare_fb) - return 0; + if (!pipe->funcs || !pipe->funcs->prepare_fb) { + if (WARN_ON_ONCE(drm_core_check_feature(plane->dev, DRIVER_GEM))) + return 0; + + WARN_ON_ONCE(pipe->funcs && pipe->funcs->cleanup_fb); + + return drm_gem_simple_display_pipe_prepare_fb(pipe, state); + } return pipe->funcs->prepare_fb(pipe, state); } diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index ef9944e9c5fc..363a9a8c3587 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -116,8 +116,11 @@ struct drm_simple_display_pipe_funcs { * the documentation for the &drm_plane_helper_funcs.prepare_fb hook for * more details. * - * Drivers which always have their buffers pinned should use - * drm_gem_simple_display_pipe_prepare_fb() for this hook. + * For GEM drivers who neither have a @prepare_fb not @cleanup_fb hook + * set drm_gem_simple_display_pipe_prepare_fb() is called automatically + * to implement this. Other drivers which need additional plane + * processing can call drm_gem_simple_display_pipe_prepare_fb() from + * their @prepare_fb hook. */ int (*prepare_fb)(struct drm_simple_display_pipe *pipe, struct drm_plane_state *plane_state);
It's tedious to review this all the time, and my audit showed that arcpgu actually forgot to set this. Make this the default and stop worrying. Again I sprinkled WARN_ON_ONCE on top to make sure we don't have strange combinations of hooks: cleanup_fb without prepare_fb doesn't make sense, and since simpler drivers are all new they better be GEM based drivers. Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Maxime Ripard <mripard@kernel.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: David Airlie <airlied@linux.ie> Cc: Daniel Vetter <daniel@ffwll.ch> --- drivers/gpu/drm/drm_simple_kms_helper.c | 12 ++++++++++-- include/drm/drm_simple_kms_helper.h | 7 +++++-- 2 files changed, 15 insertions(+), 4 deletions(-)