Daniel Vetter
2019-Oct-22 15:55 UTC
[PATCH] drm/simple-kms: Standardize arguments for callbacks
Passing the wrong type feels icky, everywhere else we use the pipe as the first parameter. Spotted while discussing patches with Thomas Zimmermann. Cc: Thomas Zimmermann <tzimmermann at suse.de> Cc: Noralf Tr?nnes <noralf at tronnes.org> Cc: Gerd Hoffmann <kraxel at redhat.com> Cc: Eric Anholt <eric at anholt.net> Cc: Emil Velikov <emil.velikov at collabora.com> Cc: virtualization at lists.linux-foundation.org Cc: Linus Walleij <linus.walleij at linaro.org> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com> --- drivers/gpu/drm/cirrus/cirrus.c | 2 +- drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- drivers/gpu/drm/pl111/pl111_display.c | 4 ++-- include/drm/drm_simple_kms_helper.h | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c index 7d08d067e1a4..248c9f765c45 100644 --- a/drivers/gpu/drm/cirrus/cirrus.c +++ b/drivers/gpu/drm/cirrus/cirrus.c @@ -390,7 +390,7 @@ static int cirrus_conn_init(struct cirrus_device *cirrus) /* ------------------------------------------------------------------ */ /* cirrus (simple) display pipe */ -static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_crtc *crtc, +static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_simple_display_pipe *pipe, const struct drm_display_mode *mode) { if (cirrus_check_size(mode->hdisplay, mode->vdisplay, NULL) < 0) diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c index 046055719245..15fb516ae2d8 100644 --- a/drivers/gpu/drm/drm_simple_kms_helper.c +++ b/drivers/gpu/drm/drm_simple_kms_helper.c @@ -43,7 +43,7 @@ drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc, /* Anything goes */ return MODE_OK; - return pipe->funcs->mode_valid(crtc, mode); + return pipe->funcs->mode_valid(pipe, mode); } static int drm_simple_kms_crtc_check(struct drm_crtc *crtc, diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c index 024771a4083e..703ddc803c55 100644 --- a/drivers/gpu/drm/pl111/pl111_display.c +++ b/drivers/gpu/drm/pl111/pl111_display.c @@ -48,10 +48,10 @@ irqreturn_t pl111_irq(int irq, void *data) } static enum drm_mode_status -pl111_mode_valid(struct drm_crtc *crtc, +pl111_mode_valid(struct drm_simple_display_pipe *pipe, const struct drm_display_mode *mode) { - struct drm_device *drm = crtc->dev; + struct drm_device *drm = pipe->crtc.dev; struct pl111_drm_dev_private *priv = drm->dev_private; u32 cpp = priv->variant->fb_bpp / 8; u64 bw; diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h index 4d89cd0a60db..15afee9cf049 100644 --- a/include/drm/drm_simple_kms_helper.h +++ b/include/drm/drm_simple_kms_helper.h @@ -49,7 +49,7 @@ struct drm_simple_display_pipe_funcs { * * drm_mode_status Enum */ - enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, + enum drm_mode_status (*mode_valid)(struct drm_simple_display_pipe *pipe, const struct drm_display_mode *mode); /** -- 2.23.0
Thomas Zimmermann
2019-Oct-22 17:16 UTC
[PATCH] drm/simple-kms: Standardize arguments for callbacks
Hi, there are two types of callbacks in struct drm_simple_display_pipe_funcs: functions that are genuine to simple KMS, and functions that are merely forwarded from another structure (crtc, plane, etc). In the former category are enable(), disable(), check(), and update(). Those should probably receive a simple display pipe as their first argument. In the latter category are mode_valid(), prepare_fb(), cleanup_fb(), enable_vblank(), and disable_vblank(). IMHO those functions should receive a pointer to the original structure as their first argument. This type provides the context in which the operations make sense. (Even their documentation already refers to the original structure.) I admit that not all are as shareable as prepare_fb() and enable_fb(). But what else than boiler-plate wrappers do we get from simple display pipe here? Best regards Thomas Am 22.10.19 um 17:55 schrieb Daniel Vetter:> Passing the wrong type feels icky, everywhere else we use the pipe as > the first parameter. Spotted while discussing patches with Thomas > Zimmermann. > > Cc: Thomas Zimmermann <tzimmermann at suse.de> > Cc: Noralf Tr?nnes <noralf at tronnes.org> > Cc: Gerd Hoffmann <kraxel at redhat.com> > Cc: Eric Anholt <eric at anholt.net> > Cc: Emil Velikov <emil.velikov at collabora.com> > Cc: virtualization at lists.linux-foundation.org > Cc: Linus Walleij <linus.walleij at linaro.org> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com> > --- > drivers/gpu/drm/cirrus/cirrus.c | 2 +- > drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- > drivers/gpu/drm/pl111/pl111_display.c | 4 ++-- > include/drm/drm_simple_kms_helper.h | 2 +- > 4 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c > index 7d08d067e1a4..248c9f765c45 100644 > --- a/drivers/gpu/drm/cirrus/cirrus.c > +++ b/drivers/gpu/drm/cirrus/cirrus.c > @@ -390,7 +390,7 @@ static int cirrus_conn_init(struct cirrus_device *cirrus) > /* ------------------------------------------------------------------ */ > /* cirrus (simple) display pipe */ > > -static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_crtc *crtc, > +static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_simple_display_pipe *pipe, > const struct drm_display_mode *mode) > { > if (cirrus_check_size(mode->hdisplay, mode->vdisplay, NULL) < 0) > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > index 046055719245..15fb516ae2d8 100644 > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > @@ -43,7 +43,7 @@ drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc, > /* Anything goes */ > return MODE_OK; > > - return pipe->funcs->mode_valid(crtc, mode); > + return pipe->funcs->mode_valid(pipe, mode); > } > > static int drm_simple_kms_crtc_check(struct drm_crtc *crtc, > diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c > index 024771a4083e..703ddc803c55 100644 > --- a/drivers/gpu/drm/pl111/pl111_display.c > +++ b/drivers/gpu/drm/pl111/pl111_display.c > @@ -48,10 +48,10 @@ irqreturn_t pl111_irq(int irq, void *data) > } > > static enum drm_mode_status > -pl111_mode_valid(struct drm_crtc *crtc, > +pl111_mode_valid(struct drm_simple_display_pipe *pipe, > const struct drm_display_mode *mode) > { > - struct drm_device *drm = crtc->dev; > + struct drm_device *drm = pipe->crtc.dev; > struct pl111_drm_dev_private *priv = drm->dev_private; > u32 cpp = priv->variant->fb_bpp / 8; > u64 bw; > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h > index 4d89cd0a60db..15afee9cf049 100644 > --- a/include/drm/drm_simple_kms_helper.h > +++ b/include/drm/drm_simple_kms_helper.h > @@ -49,7 +49,7 @@ struct drm_simple_display_pipe_funcs { > * > * drm_mode_status Enum > */ > - enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > + enum drm_mode_status (*mode_valid)(struct drm_simple_display_pipe *pipe, > const struct drm_display_mode *mode); > > /** >-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 N?rnberg, Germany (HRB 36809, AG N?rnberg) Gesch?ftsf?hrer: Felix Imend?rffer -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20191022/29fad445/attachment-0001.sig>
Daniel Vetter
2019-Oct-22 19:03 UTC
[PATCH] drm/simple-kms: Standardize arguments for callbacks
On Tue, Oct 22, 2019 at 7:16 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:> > Hi, > > there are two types of callbacks in struct > drm_simple_display_pipe_funcs: functions that are genuine to simple KMS, > and functions that are merely forwarded from another structure (crtc, > plane, etc). > > In the former category are enable(), disable(), check(), and update(). > Those should probably receive a simple display pipe as their first argument.mode_valid _very_ much belongs to this category too, since there's mode_valid hooks also on other objects. But simple pipe helper condenses that down to one mode_valid hook (we could also put the mode_valid onto encoder, wouldn't change anything).> In the latter category are mode_valid(), prepare_fb(), cleanup_fb(), > enable_vblank(), and disable_vblank(). IMHO those functions should > receive a pointer to the original structure as their first argument. > This type provides the context in which the operations make sense. (Even > their documentation already refers to the original structure.)Now on those you can maybe make a case that they only exist in one object. But the entire point of simple helpers was to condense the zoo of drm types down to one. Only reason you don't also get a drm_simple_display_pipe_state is that this one would be a bit more work to make work correctly. If we full on leak all the underlying objects, then you might as well set them up yourself and set up all the hooks, it's just a few more lines of code. Imo for simple pipe we should go more into that direction, not less.> I admit that not all are as shareable as prepare_fb() and enable_fb(). > But what else than boiler-plate wrappers do we get from simple display > pipe here?Boiler plate wrappers is pretty much the entire point of simple pipe helpers. Anytime you're interested in the things it abstracts away (crtc, plane, encoder) you probably want your own atomic implementation. That's why I don't think using simple pipe for fbdev conversion is a good fit, it's not meant to be useful for all small drivers. Only for the _really_ simple ones. Otherwise if we readd all the bells and whistles to simple pipe helpers, then we just end back where we started. That's also why I personally think explicit simple wrappers would fit better, instead of wrestling the prepare/cleanup_fb functions to match full atomic helpers. -Daniel> > Best regards > Thomas > > Am 22.10.19 um 17:55 schrieb Daniel Vetter: > > Passing the wrong type feels icky, everywhere else we use the pipe as > > the first parameter. Spotted while discussing patches with Thomas > > Zimmermann. > > > > Cc: Thomas Zimmermann <tzimmermann at suse.de> > > Cc: Noralf Tr?nnes <noralf at tronnes.org> > > Cc: Gerd Hoffmann <kraxel at redhat.com> > > Cc: Eric Anholt <eric at anholt.net> > > Cc: Emil Velikov <emil.velikov at collabora.com> > > Cc: virtualization at lists.linux-foundation.org > > Cc: Linus Walleij <linus.walleij at linaro.org> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com> > > --- > > drivers/gpu/drm/cirrus/cirrus.c | 2 +- > > drivers/gpu/drm/drm_simple_kms_helper.c | 2 +- > > drivers/gpu/drm/pl111/pl111_display.c | 4 ++-- > > include/drm/drm_simple_kms_helper.h | 2 +- > > 4 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c > > index 7d08d067e1a4..248c9f765c45 100644 > > --- a/drivers/gpu/drm/cirrus/cirrus.c > > +++ b/drivers/gpu/drm/cirrus/cirrus.c > > @@ -390,7 +390,7 @@ static int cirrus_conn_init(struct cirrus_device *cirrus) > > /* ------------------------------------------------------------------ */ > > /* cirrus (simple) display pipe */ > > > > -static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_crtc *crtc, > > +static enum drm_mode_status cirrus_pipe_mode_valid(struct drm_simple_display_pipe *pipe, > > const struct drm_display_mode *mode) > > { > > if (cirrus_check_size(mode->hdisplay, mode->vdisplay, NULL) < 0) > > diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c b/drivers/gpu/drm/drm_simple_kms_helper.c > > index 046055719245..15fb516ae2d8 100644 > > --- a/drivers/gpu/drm/drm_simple_kms_helper.c > > +++ b/drivers/gpu/drm/drm_simple_kms_helper.c > > @@ -43,7 +43,7 @@ drm_simple_kms_crtc_mode_valid(struct drm_crtc *crtc, > > /* Anything goes */ > > return MODE_OK; > > > > - return pipe->funcs->mode_valid(crtc, mode); > > + return pipe->funcs->mode_valid(pipe, mode); > > } > > > > static int drm_simple_kms_crtc_check(struct drm_crtc *crtc, > > diff --git a/drivers/gpu/drm/pl111/pl111_display.c b/drivers/gpu/drm/pl111/pl111_display.c > > index 024771a4083e..703ddc803c55 100644 > > --- a/drivers/gpu/drm/pl111/pl111_display.c > > +++ b/drivers/gpu/drm/pl111/pl111_display.c > > @@ -48,10 +48,10 @@ irqreturn_t pl111_irq(int irq, void *data) > > } > > > > static enum drm_mode_status > > -pl111_mode_valid(struct drm_crtc *crtc, > > +pl111_mode_valid(struct drm_simple_display_pipe *pipe, > > const struct drm_display_mode *mode) > > { > > - struct drm_device *drm = crtc->dev; > > + struct drm_device *drm = pipe->crtc.dev; > > struct pl111_drm_dev_private *priv = drm->dev_private; > > u32 cpp = priv->variant->fb_bpp / 8; > > u64 bw; > > diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h > > index 4d89cd0a60db..15afee9cf049 100644 > > --- a/include/drm/drm_simple_kms_helper.h > > +++ b/include/drm/drm_simple_kms_helper.h > > @@ -49,7 +49,7 @@ struct drm_simple_display_pipe_funcs { > > * > > * drm_mode_status Enum > > */ > > - enum drm_mode_status (*mode_valid)(struct drm_crtc *crtc, > > + enum drm_mode_status (*mode_valid)(struct drm_simple_display_pipe *pipe, > > const struct drm_display_mode *mode); > > > > /** > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 N?rnberg, Germany > (HRB 36809, AG N?rnberg) > Gesch?ftsf?hrer: Felix Imend?rffer >-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
kbuild test robot
2019-Oct-23 00:43 UTC
[PATCH] drm/simple-kms: Standardize arguments for callbacks
Hi Daniel, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.4-rc4 next-20191022] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-simple-kms-Standardize-arguments-for-callbacks/20191023-073731 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3b7c59a1950c75f2c0152e5a9cd77675b09233d6 config: x86_64-allyesconfig (attached as .config) compiler: gcc-7 (Debian 7.4.0-14) 7.4.0 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp at intel.com> All errors (new ones prefixed by >>):>> drivers/gpu//drm/xen/xen_drm_front_kms.c:289:16: error: initialization from incompatible pointer type [-Werror=incompatible-pointer-types].mode_valid = display_mode_valid, ^~~~~~~~~~~~~~~~~~ drivers/gpu//drm/xen/xen_drm_front_kms.c:289:16: note: (near initialization for 'display_funcs.mode_valid') cc1: some warnings being treated as errors vim +289 drivers/gpu//drm/xen/xen_drm_front_kms.c c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 287 c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 288 static const struct drm_simple_display_pipe_funcs display_funcs = { c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 @289 .mode_valid = display_mode_valid, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 290 .enable = display_enable, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 291 .disable = display_disable, dd388ee1ecbb8c Daniel Vetter 2018-04-09 292 .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 293 .update = display_update, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 294 }; c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 295 :::::: The code at line 289 was first introduced by commit :::::: c575b7eeb89f94356997abd62d6d5a0590e259b7 drm/xen-front: Add support for Xen PV display frontend :::::: TO: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com> :::::: CC: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation -------------- next part -------------- A non-text attachment was scrubbed... Name: .config.gz Type: application/gzip Size: 70172 bytes Desc: not available URL: <http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20191023/d4e8f945/attachment-0001.bin>
kbuild test robot
2019-Oct-23 05:55 UTC
[PATCH] drm/simple-kms: Standardize arguments for callbacks
Hi Daniel, I love your patch! Perhaps something to improve: [auto build test WARNING on linus/master] [cannot apply to v5.4-rc4 next-20191022] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-simple-kms-Standardize-arguments-for-callbacks/20191023-073731 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 3b7c59a1950c75f2c0152e5a9cd77675b09233d6 reproduce: # apt-get install sparse # sparse version: v0.6.1-dirty make ARCH=x86_64 allmodconfig make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp at intel.com> sparse warnings: (new ones prefixed by >>)>> drivers/gpu/drm/xen/xen_drm_front_kms.c:289:23: sparse: sparse: incorrect type in initializer (incompatible argument 1 (different base types)) @@ expected int enum drm_mode_status ( *mode_valid )( ... ) @@ got int enum drm_mode_status ( *mode_valid )( ... ) @@ >> drivers/gpu/drm/xen/xen_drm_front_kms.c:289:23: sparse: expected int enum drm_mode_status ( *mode_valid )( ... ) >> drivers/gpu/drm/xen/xen_drm_front_kms.c:289:23: sparse: got int enum drm_mode_status ( * )( ... )vim +289 drivers/gpu/drm/xen/xen_drm_front_kms.c c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 287 c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 288 static const struct drm_simple_display_pipe_funcs display_funcs = { c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 @289 .mode_valid = display_mode_valid, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 290 .enable = display_enable, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 291 .disable = display_disable, dd388ee1ecbb8c Daniel Vetter 2018-04-09 292 .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 293 .update = display_update, c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 294 }; c575b7eeb89f94 Oleksandr Andrushchenko 2018-04-03 295 :::::: The code at line 289 was first introduced by commit :::::: c575b7eeb89f94356997abd62d6d5a0590e259b7 drm/xen-front: Add support for Xen PV display frontend :::::: TO: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com> :::::: CC: Oleksandr Andrushchenko <oleksandr_andrushchenko at epam.com> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
Maybe Matching Threads
- [PATCH] drm/simple-kms: Standardize arguments for callbacks
- [PATCH] drm/simple-kms: Standardize arguments for callbacks
- [PATCH] drm/simple-kms: Standardize arguments for callbacks
- [PATCH] drm/simple-kms: Standardize arguments for callbacks
- [PATCH] drm/simple-kms: Standardize arguments for callbacks