| Message ID | 1483962987-19011-2-git-send-email-shawnguo@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 09, 2017 at 07:56:24PM +0800, Shawn Guo wrote: > From: Shawn Guo <shawn.guo@linaro.org> > > The vblank is mostly CRTC specific and implemented as part of CRTC > driver. So having vblank hooks in struct drm_crtc_funcs should > generally help to reduce code from client drivers in implementing > drm_driver's vblank callbacks. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++++++++++++++++++ > include/drm/drm_crtc.h | 21 +++++++++++++++++++++ > 2 files changed, 57 insertions(+) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 85a7452d0fb4..59ff00f48101 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -70,6 +70,42 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx) > EXPORT_SYMBOL(drm_crtc_from_index); > > /** > + * drm_crtc_enable_vblank - vblank enable callback helper > + * @dev: DRM device > + * @pipe: CRTC index > + * > + * It's a helper function as the generic vblank enable callback implementation, > + * which calls into &drm_crtc_funcs.enable_vblank function. > + */ > +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe) > +{ > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > + > + if (crtc && crtc->funcs && crtc->funcs->enable_vblank) > + return crtc->funcs->enable_vblank(crtc); > + > + return 0; > +} > +EXPORT_SYMBOL(drm_crtc_enable_vblank); With the helper approach here there's still a pile of boilerplate in drivers (well, 2 lines to fill out the legacy helpers). What if instead we wrap all callers of enable/disable_vblank in drm_irq.c into something like __enable_vblank(dev, pipe) { if (DRIVER_MODESET) /* otherwise we'll oops on legacy drivers */ { /* above code to call the new hook, if it's there. */ if (crtc->funcs->enable_vblank) return crtc->funcs->enable_vblank(crtc); } /* fallback for everyone else */ dev->driver->enable_vblank(dev, pipe); } > + > +/** > + * drm_crtc_disable_vblank - vblank disable callback helper > + * @dev: DRM device > + * @pipe: CRTC index > + * > + * It's a helper function as the generic vblank disable callback implementation, > + * which calls into &drm_crtc_funcs.disable_vblank function. > + */ > +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe) > +{ > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > + > + if (crtc && crtc->funcs && crtc->funcs->disable_vblank) > + return crtc->funcs->disable_vblank(crtc); > +} > +EXPORT_SYMBOL(drm_crtc_disable_vblank); > + > +/** > * drm_crtc_force_disable - Forcibly turn off a CRTC > * @crtc: CRTC to turn off > * > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index a5627eb8621f..20874b3903a6 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -599,6 +599,25 @@ struct drm_crtc_funcs { > */ > void (*atomic_print_state)(struct drm_printer *p, > const struct drm_crtc_state *state); > + > + /** > + * @enable_vblank: > + * > + * Enable vblank interrupts for the CRTC. > + * > + * Returns: > + * > + * Zero on success, appropriate errno if the vblank interrupt cannot > + * be enabled. > + */ > + int (*enable_vblank)(struct drm_crtc *crtc); > + > + /** > + * @disable_vblank: > + * > + * Disable vblank interrupts for the CRTC. > + */ > + void (*disable_vblank)(struct drm_crtc *crtc); Please also update the kerneldoc for these two hooks in drm_drv.h, and link between the two (using the &drm_driver.enable_vblank syntax). And then mention that the drm_driver one is deprecated. Also I think it'd would make sense to do the same for get_vblank_counter(), since those are the 3 core->driver interface functions. The other 2 are kinda just helpers for precise vblank timestamp support. -Daniel > }; > > /** > @@ -831,6 +850,8 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode, > > int drm_mode_set_config_internal(struct drm_mode_set *set); > struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx); > +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe); > +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe); > > /* Helpers */ > static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev, > -- > 1.9.1 >
On Tuesday 10 Jan 2017 11:39:03 Daniel Vetter wrote: > On Mon, Jan 09, 2017 at 07:56:24PM +0800, Shawn Guo wrote: > > From: Shawn Guo <shawn.guo@linaro.org> > > > > The vblank is mostly CRTC specific and implemented as part of CRTC > > driver. So having vblank hooks in struct drm_crtc_funcs should > > generally help to reduce code from client drivers in implementing > > drm_driver's vblank callbacks. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > --- > > > > drivers/gpu/drm/drm_crtc.c | 36 ++++++++++++++++++++++++++++++++++++ > > include/drm/drm_crtc.h | 21 +++++++++++++++++++++ > > 2 files changed, 57 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > index 85a7452d0fb4..59ff00f48101 100644 > > --- a/drivers/gpu/drm/drm_crtc.c > > +++ b/drivers/gpu/drm/drm_crtc.c > > @@ -70,6 +70,42 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device > > *dev, int idx)> > > EXPORT_SYMBOL(drm_crtc_from_index); > > > > /** > > > > + * drm_crtc_enable_vblank - vblank enable callback helper > > + * @dev: DRM device > > + * @pipe: CRTC index > > + * > > + * It's a helper function as the generic vblank enable callback > > implementation, + * which calls into &drm_crtc_funcs.enable_vblank > > function. > > + */ > > +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe) > > +{ > > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > > + > > + if (crtc && crtc->funcs && crtc->funcs->enable_vblank) > > + return crtc->funcs->enable_vblank(crtc); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(drm_crtc_enable_vblank); > > With the helper approach here there's still a pile of boilerplate in > drivers (well, 2 lines to fill out the legacy helpers). What if instead we > wrap all callers of enable/disable_vblank in drm_irq.c into something like > > __enable_vblank(dev, pipe) > { > if (DRIVER_MODESET) /* otherwise we'll oops on legacy drivers */ > { > /* above code to call the new hook, if it's there. */ > > if (crtc->funcs->enable_vblank) > return crtc->funcs->enable_vblank(crtc); > } > > /* fallback for everyone else */ > > dev->driver->enable_vblank(dev, pipe); > } FWIW I like that approach much better. I'd even go as far as saying that DRIVER_MODESET drivers should be mass-converted. > > + > > +/** > > + * drm_crtc_disable_vblank - vblank disable callback helper > > + * @dev: DRM device > > + * @pipe: CRTC index > > + * > > + * It's a helper function as the generic vblank disable callback > > implementation, > > + * which calls into &drm_crtc_funcs.disable_vblank function. > > + */ > > +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe) > > +{ > > + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); > > + > > + if (crtc && crtc->funcs && crtc->funcs->disable_vblank) > > + return crtc->funcs->disable_vblank(crtc); > > +} > > +EXPORT_SYMBOL(drm_crtc_disable_vblank); > > + > > +/** > > * drm_crtc_force_disable - Forcibly turn off a CRTC > > * @crtc: CRTC to turn off > > *
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 85a7452d0fb4..59ff00f48101 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -70,6 +70,42 @@ struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx) EXPORT_SYMBOL(drm_crtc_from_index); /** + * drm_crtc_enable_vblank - vblank enable callback helper + * @dev: DRM device + * @pipe: CRTC index + * + * It's a helper function as the generic vblank enable callback implementation, + * which calls into &drm_crtc_funcs.enable_vblank function. + */ +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe) +{ + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); + + if (crtc && crtc->funcs && crtc->funcs->enable_vblank) + return crtc->funcs->enable_vblank(crtc); + + return 0; +} +EXPORT_SYMBOL(drm_crtc_enable_vblank); + +/** + * drm_crtc_disable_vblank - vblank disable callback helper + * @dev: DRM device + * @pipe: CRTC index + * + * It's a helper function as the generic vblank disable callback implementation, + * which calls into &drm_crtc_funcs.disable_vblank function. + */ +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe) +{ + struct drm_crtc *crtc = drm_crtc_from_index(dev, pipe); + + if (crtc && crtc->funcs && crtc->funcs->disable_vblank) + return crtc->funcs->disable_vblank(crtc); +} +EXPORT_SYMBOL(drm_crtc_disable_vblank); + +/** * drm_crtc_force_disable - Forcibly turn off a CRTC * @crtc: CRTC to turn off * diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a5627eb8621f..20874b3903a6 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -599,6 +599,25 @@ struct drm_crtc_funcs { */ void (*atomic_print_state)(struct drm_printer *p, const struct drm_crtc_state *state); + + /** + * @enable_vblank: + * + * Enable vblank interrupts for the CRTC. + * + * Returns: + * + * Zero on success, appropriate errno if the vblank interrupt cannot + * be enabled. + */ + int (*enable_vblank)(struct drm_crtc *crtc); + + /** + * @disable_vblank: + * + * Disable vblank interrupts for the CRTC. + */ + void (*disable_vblank)(struct drm_crtc *crtc); }; /** @@ -831,6 +850,8 @@ void drm_crtc_get_hv_timing(const struct drm_display_mode *mode, int drm_mode_set_config_internal(struct drm_mode_set *set); struct drm_crtc *drm_crtc_from_index(struct drm_device *dev, int idx); +int drm_crtc_enable_vblank(struct drm_device *dev, unsigned int pipe); +void drm_crtc_disable_vblank(struct drm_device *dev, unsigned int pipe); /* Helpers */ static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,