On Thu, Jul 25, 2024 at 04:51:08PM -0400, Hamza Mahfooz wrote:
> We would like to be able to enable vblank_disable_immediate
> unconditionally, however there are a handful of cases where a small off
> delay is necessary (e.g. with PSR enabled). So, we would like to be able
> to adjust the vblank off delay and disable imminent values dynamically
> for a given CRTC. Since, it will allow drivers to apply static screen
> optimizations more quickly and consequently allow users to benefit more
> so from the power savings afforded by the aforementioned optimizations,
> while avoiding issues in cases where an off delay is still warranted.
> In particular, the PSR case requires a small off delay of 2 frames,
> otherwise display firmware isn't able to keep up with all of the
> requests made to amdgpu. So, introduce drm_crtc_vblank_on_config() which
> is like drm_crtc_vblank_on(), but it allows drivers to specify the
> vblank CRTC configuration before enabling vblanking support for a given
> CRTC.
>
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@xxxxxxx>
Yeah looks reasonable, bunch of details below. With those addressed:
Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/drm_vblank.c | 57 ++++++++++++++++++++++++++----------
> include/drm/drm_vblank.h | 25 ++++++++++++++++
> 2 files changed, 66 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index cc3571e25a9a..c9de7d18389a 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
The intro kerneldoc at the top of this needs to be updated. Might be good
to grep for vblank_disable_immediate to make sure you haven't missed any
reference.
> @@ -1241,6 +1241,7 @@ EXPORT_SYMBOL(drm_crtc_vblank_get);
> void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
> {
> struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
> + int vblank_offdelay = vblank->config.offdelay_ms;
>
> if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> return;
> @@ -1250,13 +1251,13 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>
> /* Last user schedules interrupt disable */
> if (atomic_dec_and_test(&vblank->refcount)) {
> - if (drm_vblank_offdelay == 0)
> + if (!vblank_offdelay)
> return;
> - else if (drm_vblank_offdelay < 0)
> + else if (vblank_offdelay < 0)
> vblank_disable_fn(&vblank->disable_timer);
> - else if (!dev->vblank_disable_immediate)
> + else if (!vblank->config.disable_immediate)
> mod_timer(&vblank->disable_timer,
> - jiffies + ((drm_vblank_offdelay * HZ)/1000));
> + jiffies + ((vblank_offdelay * HZ) / 1000));
> }
> }
The kerneldoc for drm_crtc_vblank_put also needs updating to point at
drm_vblank_config.offdelay_ms.
>
> @@ -1466,16 +1467,16 @@ void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
> EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
>
> /**
> - * drm_crtc_vblank_on - enable vblank events on a CRTC
> + * drm_crtc_vblank_on_config - enable vblank events on a CRTC with custom
> + * configuration options
> * @crtc: CRTC in question
> + * @config: Vblank configuration value
> *
> - * This functions restores the vblank interrupt state captured with
> - * drm_crtc_vblank_off() again and is generally called when enabling @crtc. Note
> - * that calls to drm_crtc_vblank_on() and drm_crtc_vblank_off() can be
> - * unbalanced and so can also be unconditionally called in driver load code to
> - * reflect the current hardware state of the crtc.
> + * See drm_crtc_vblank_on(). In addition, this function allows you to provide a
> + * custom vblank configuration for a given CRTC.
I'd add
"Note that @config is copied, the pointer does not need to stay
valid beyond this function call. For details of the parameters see struct
drm_vblank_crtc_config."
to make the kerneldoc more meaningful.
> */
> -void drm_crtc_vblank_on(struct drm_crtc *crtc)
> +void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
> + const struct drm_vblank_crtc_config *config)
> {
> struct drm_device *dev = crtc->dev;
> unsigned int pipe = drm_crtc_index(crtc);
> @@ -1488,6 +1489,8 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
> drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
> pipe, vblank->enabled, vblank->inmodeset);
>
> + vblank->config = *config;
> +
> /* Drop our private "prevent drm_vblank_get" refcount */
> if (vblank->inmodeset) {
> atomic_dec(&vblank->refcount);
> @@ -1500,10 +1503,31 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
> * re-enable interrupts if there are users left, or the
> * user wishes vblank interrupts to be enabled all the time.
> */
> - if (atomic_read(&vblank->refcount) != 0 || drm_vblank_offdelay == 0)
> + if (atomic_read(&vblank->refcount) != 0 || !vblank->config.offdelay_ms)
> drm_WARN_ON(dev, drm_vblank_enable(dev, pipe));
> spin_unlock_irq(&dev->vbl_lock);
> }
> +EXPORT_SYMBOL(drm_crtc_vblank_on_config);
> +
> +/**
> + * drm_crtc_vblank_on - enable vblank events on a CRTC
> + * @crtc: CRTC in question
> + *
> + * This functions restores the vblank interrupt state captured with
> + * drm_crtc_vblank_off() again and is generally called when enabling @crtc. Note
> + * that calls to drm_crtc_vblank_on() and drm_crtc_vblank_off() can be
> + * unbalanced and so can also be unconditionally called in driver load code to
> + * reflect the current hardware state of the crtc.
I'd add something like "Unlike drm_crtc_vblank_on_config() default values
are used." here.
> + */
> +void drm_crtc_vblank_on(struct drm_crtc *crtc)
> +{
> + const struct drm_vblank_crtc_config config = {
> + .offdelay_ms = drm_vblank_offdelay,
> + .disable_immediate = crtc->dev->vblank_disable_immediate
> + };
> +
> + drm_crtc_vblank_on_config(crtc, &config);
> +}
> EXPORT_SYMBOL(drm_crtc_vblank_on);
>
> static void drm_vblank_restore(struct drm_device *dev, unsigned int pipe)
You don't update the reference to dev->vblank_disable_immediate in this
function. Might also be good to add an assert here that we're not in a
modeset as indicated by vbl->inmodeset, since without that the check
breaks.
> @@ -1754,7 +1778,7 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
> /* If the counter is currently enabled and accurate, short-circuit
> * queries to return the cached timestamp of the last vblank.
> */
> - if (dev->vblank_disable_immediate &&
> + if (vblank->config.disable_immediate &&
> drm_wait_vblank_is_query(vblwait) &&
> READ_ONCE(vblank->enabled)) {
> drm_wait_vblank_reply(dev, pipe, &vblwait->reply);
> @@ -1918,8 +1942,8 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
> * been signaled. The disable has to be last (after
> * drm_handle_vblank_events) so that the timestamp is always accurate.
> */
> - disable_irq = (dev->vblank_disable_immediate &&
> - drm_vblank_offdelay > 0 &&
> + disable_irq = (vblank->config.disable_immediate &&
> + vblank->config.offdelay_ms > 0 &&
> !atomic_read(&vblank->refcount));
>
> drm_handle_vblank_events(dev, pipe);
> @@ -1992,7 +2016,8 @@ int drm_crtc_get_sequence_ioctl(struct drm_device *dev, void *data,
> pipe = drm_crtc_index(crtc);
>
> vblank = drm_crtc_vblank_crtc(crtc);
> - vblank_enabled = dev->vblank_disable_immediate && READ_ONCE(vblank->enabled);
> + vblank_enabled = READ_ONCE(vblank->config.disable_immediate) &&
> + READ_ONCE(vblank->enabled);
>
> if (!vblank_enabled) {
> ret = drm_crtc_vblank_get(crtc);
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index c8f829b4307c..e70d4383a674 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -78,6 +78,24 @@ struct drm_pending_vblank_event {
> } event;
> };
>
> +/**
> + * struct drm_vblank_config - vblank configuration for a CRTC
> + */
> +struct drm_vblank_crtc_config {
> + /**
> + * @offdelay_ms: Vblank off delay in ms, used to determine how long
> + * @disable_timer waits before disabling.
Please add kerneldoc here and below that explains what
drm_crtc_vblank_on() uses as the default value, including working
kerneldoc hyperlinks to other structs/functions.
> + */
> + int offdelay_ms;
> +
> + /**
> + * @disable_immediate: See @struct drm_device.vblank_disable_immediate.
Just struct drm_device.vblank_disable_immediate is the right way to link.
Maybe check the html output to make sure it's all formatted correctly.
Also please add here that people should look in the linked kerneldoc for
details of the exact semantics of immediate vblank disabling.
> + * Additonally, this tracks the disable_immediate value per crtc, just
> + * in case it needs to differ from the default value for a given device.
> + */
> + bool disable_immediate;
> +};
> +
> /**
> * struct drm_vblank_crtc - vblank tracking for a CRTC
> *
> @@ -198,6 +216,11 @@ struct drm_vblank_crtc {
> */
> struct drm_display_mode hwmode;
>
> + /**
> + * config: Stores vblank configuration values for a given CRTC.
Might be good to link to drm_crtc_vblank_on_config() here.
> + */
> + struct drm_vblank_crtc_config config;
> +
> /**
> * @enabled: Tracks the enabling state of the corresponding &drm_crtc to
> * avoid double-disabling and hence corrupting saved state. Needed by
> @@ -247,6 +270,8 @@ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe);
> void drm_crtc_wait_one_vblank(struct drm_crtc *crtc);
> void drm_crtc_vblank_off(struct drm_crtc *crtc);
> void drm_crtc_vblank_reset(struct drm_crtc *crtc);
> +void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
> + const struct drm_vblank_crtc_config *config);
> void drm_crtc_vblank_on(struct drm_crtc *crtc);
> u64 drm_crtc_accurate_vblank_count(struct drm_crtc *crtc);
> void drm_crtc_vblank_restore(struct drm_crtc *crtc);
Also please update the kerneldoc for drm_device.vblank_disable_immediate.
Cheers, Sima
> --
> 2.45.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
