[v3,2/4] drm/rockchip: add an common abstracted PSR driver
diff mbox

Message ID 1467364754-21674-1-git-send-email-ykk@rock-chips.com
State New
Headers show

Commit Message

Yakir Yang July 1, 2016, 9:19 a.m. UTC
The PSR driver have exported four symbols for specific device driver:
- rockchip_drm_psr_register()
- rockchip_drm_psr_unregister()
- rockchip_drm_psr_enable()
- rockchip_drm_psr_disable()
- rockchip_drm_psr_flush()

Encoder driver should call the register/unregister interfaces to hook
itself into common PSR driver, encoder have implement the 'psr_set'
callback which use the set PSR state in hardware side.

Crtc driver would call the enable/disable interfaces when vblank is
enable/disable, after that the common PSR driver would call the encoder
registered callback to set the PSR state.

Fb driver would call the flush interface in 'fb->dirty' callback, this
helper function would force all PSR enabled encoders to exit from PSR
for 3 seconds.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v3:
- split the psr flow into an common abstracted PSR driver
- implement the 'fb->dirty' callback function (Daniel)
- avoid to use notify to acqiure for vact event (Daniel)
- remove psr_active() callback which introduce in v2

Changes in v2: None

 drivers/gpu/drm/rockchip/Makefile           |   2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  12 ++
 drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 200 ++++++++++++++++++++++++++++
 drivers/gpu/drm/rockchip/rockchip_drm_psr.h |  12 ++
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  24 ++++
 5 files changed, 249 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
 create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h

Comments

Sean Paul July 1, 2016, 6 p.m. UTC | #1
On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> The PSR driver have exported four symbols for specific device driver:
> - rockchip_drm_psr_register()
> - rockchip_drm_psr_unregister()
> - rockchip_drm_psr_enable()
> - rockchip_drm_psr_disable()
> - rockchip_drm_psr_flush()
>
> Encoder driver should call the register/unregister interfaces to hook
> itself into common PSR driver, encoder have implement the 'psr_set'
> callback which use the set PSR state in hardware side.
>
> Crtc driver would call the enable/disable interfaces when vblank is
> enable/disable, after that the common PSR driver would call the encoder
> registered callback to set the PSR state.
>

This feels overly complicated. It seems like you could cut out a bunch
of code by just coding the psr functions into vop and
analogix_dp-rockchip. I suppose the only reason to keep it abstracted
would be if you plan on supporting psr in a different encoder or crtc
in rockchip, or if you're planning on moving this into drm core.

Perhaps others will disagree with this sentiment and this is the right
thing to do.

> Fb driver would call the flush interface in 'fb->dirty' callback, this
> helper function would force all PSR enabled encoders to exit from PSR
> for 3 seconds.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v3:
> - split the psr flow into an common abstracted PSR driver
> - implement the 'fb->dirty' callback function (Daniel)
> - avoid to use notify to acqiure for vact event (Daniel)
> - remove psr_active() callback which introduce in v2
>
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/Makefile           |   2 +-
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  12 ++
>  drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 200 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/rockchip/rockchip_drm_psr.h |  12 ++
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  24 ++++
>  5 files changed, 249 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
>
> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> index 05d0713..9746365 100644
> --- a/drivers/gpu/drm/rockchip/Makefile
> +++ b/drivers/gpu/drm/rockchip/Makefile
> @@ -3,7 +3,7 @@
>  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>
>  rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> -               rockchip_drm_gem.o rockchip_drm_vop.o
> +               rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o
>  rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>
>  obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 20f12bc..0fec18f 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -21,6 +21,7 @@
>
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_gem.h"
> +#include "rockchip_drm_psr.h"
>
>  #define to_rockchip_fb(x) container_of(x, struct rockchip_drm_fb, fb)
>
> @@ -66,9 +67,20 @@ static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
>                                      rockchip_fb->obj[0], handle);
>  }
>
> +static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> +                                struct drm_file *file,
> +                                unsigned int flags, unsigned int color,
> +                                struct drm_clip_rect *clips,
> +                                unsigned int num_clips)
> +{
> +       rockchip_drm_psr_flush();
> +       return 0;
> +}
> +
>  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>         .destroy        = rockchip_drm_fb_destroy,
>         .create_handle  = rockchip_drm_fb_create_handle,
> +       .dirty          = rockchip_drm_fb_dirty,
>  };
>
>  static struct rockchip_drm_fb *
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> new file mode 100644
> index 0000000..c044443
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> @@ -0,0 +1,200 @@
> +#include <drm/drm_crtc_helper.h>
> +
> +#include "rockchip_drm_psr.h"
> +
> +#define PSR_FLUSH_TIMEOUT      msecs_to_jiffies(3000) /* 3 seconds */
> +
> +static LIST_HEAD(psr_list);
> +static DEFINE_MUTEX(psr_list_mutex);

I'm not crazy about these globals. Perhaps you can initialize them
with the rockchip driver and tuck them in a driver-level struct
(rockchip_drm_private or something).


> +
> +enum psr_state {
> +       PSR_FLUSH,
> +       PSR_ENABLE,
> +       PSR_DISABLE,
> +};
> +
> +struct psr_drv {
> +       struct list_head list;
> +       enum psr_state state;
> +       struct mutex state_mutex;
> +
> +       struct timer_list flush_timer;
> +
> +       struct drm_encoder *encoder;
> +       int (*set)(struct drm_encoder *encoder, bool enable);
> +};
> +
> +static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
> +{
> +       struct psr_drv *psr;
> +
> +       mutex_lock(&psr_list_mutex);
> +       list_for_each_entry(psr, &psr_list, list) {
> +               if (psr->encoder->crtc == crtc) {
> +                       mutex_unlock(&psr_list_mutex);
> +                       return psr;
> +               }
> +       }
> +       mutex_unlock(&psr_list_mutex);
> +
> +       return ERR_PTR(-ENODEV);
> +}
> +
> +static void psr_enable(struct psr_drv *psr)
> +{
> +       if (psr->state == PSR_ENABLE)
> +               return;

Should you be worried about races by accessing this outside of the state mutex?

> +
> +       mutex_lock(&psr->state_mutex);
> +       psr->state = PSR_ENABLE;
> +       psr->set(psr->encoder, true);
> +       mutex_unlock(&psr->state_mutex);
> +}
> +
> +static void psr_disable(struct psr_drv *psr)
> +{
> +       if (psr->state == PSR_DISABLE)
> +               return;
> +
> +       mutex_lock(&psr->state_mutex);
> +       psr->state = PSR_DISABLE;
> +       psr->set(psr->encoder, false);
> +       mutex_unlock(&psr->state_mutex);
> +}
> +
> +static void psr_flush_handler(unsigned long data)
> +{
> +       struct psr_drv *psr = (struct psr_drv *)data;
> +
> +       if (!psr || psr->state != PSR_FLUSH)
> +               return;
> +
> +       psr_enable(psr);
> +}
> +
> +/**
> + * rockchip_drm_psr_enable - enable the encoder PSR which bind to given CRTC
> + * @crtc: CRTC to obtain the PSR encoder
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int rockchip_drm_psr_enable(struct drm_crtc *crtc)
> +{
> +       struct psr_drv *psr = find_psr_by_crtc(crtc);
> +
> +       if (IS_ERR(psr))
> +               return PTR_ERR(psr);
> +
> +       psr_enable(psr);
> +       return 0;
> +}
> +EXPORT_SYMBOL(rockchip_drm_psr_enable);
> +
> +/**
> + * rockchip_drm_psr_disable - disable the encoder PSR which bind to given CRTC
> + * @crtc: CRTC to obtain the PSR encoder
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int rockchip_drm_psr_disable(struct drm_crtc *crtc)
> +{
> +       struct psr_drv *psr = find_psr_by_crtc(crtc);
> +
> +       if (IS_ERR(psr))
> +               return PTR_ERR(psr);
> +
> +       psr_disable(psr);
> +       return 0;
> +}
> +EXPORT_SYMBOL(rockchip_drm_psr_disable);
> +
> +/**
> + * rockchip_drm_psr_flush - force to flush all registered PSR encoders
> + *
> + * Disable the PSR function for all registered encoders, and then enable the
> + * PSR function back after 5 second. If encoder PSR state have been changed

s/5 second/PSR_FLUSH_TIMEOUT/

> + * during flush time, then keep the state no change after flush timeout.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +void rockchip_drm_psr_flush(void)
> +{
> +       struct psr_drv *psr;
> +
> +       mutex_lock(&psr_list_mutex);
> +       list_for_each_entry(psr, &psr_list, list) {
> +               if (psr->state == PSR_DISABLE)
> +                       continue;
> +
> +               mod_timer(&psr->flush_timer,
> +                         round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
> +
> +               psr_disable(psr);
> +               psr->state = PSR_FLUSH;

This is set outside of state_mutex, is that intentional?

> +       }
> +       mutex_unlock(&psr_list_mutex);
> +}
> +EXPORT_SYMBOL(rockchip_drm_psr_flush);
> +
> +/**
> + * rockchip_drm_psr_register - register encoder to psr driver
> + * @encoder: encoder that obtain the PSR function
> + * @psr_set: call back to set PSR state
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +int rockchip_drm_psr_register(struct drm_encoder *encoder,
> +                             int (*psr_set)(struct drm_encoder *, bool enable))
> +{
> +       struct psr_drv *psr;
> +
> +       if (!encoder || !psr_set)
> +               return -EINVAL;
> +
> +       psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
> +       if (!psr)
> +               return -ENOMEM;
> +
> +       setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
> +
> +       mutex_init(&psr->state_mutex);
> +
> +       psr->state = PSR_DISABLE;
> +       psr->encoder = encoder;
> +       psr->set = psr_set;
> +
> +       mutex_lock(&psr_list_mutex);
> +       list_add_tail(&psr->list, &psr_list);
> +       mutex_unlock(&psr_list_mutex);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(rockchip_drm_psr_register);
> +
> +/**
> + * rockchip_drm_psr_unregister - unregister encoder to psr driver
> + * @encoder: encoder that obtain the PSR function
> + * @psr_set: call back to set PSR state
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */
> +void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
> +{
> +       struct psr_drv *psr;
> +
> +       mutex_lock(&psr_list_mutex);
> +       list_for_each_entry(psr, &psr_list, list) {
> +               if (psr->encoder == encoder) {
> +                       del_timer(&psr->flush_timer);
> +                       list_del(&psr->list);
> +                       kfree(psr);
> +               }
> +       }
> +       mutex_unlock(&psr_list_mutex);
> +}
> +EXPORT_SYMBOL(rockchip_drm_psr_unregister);
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> new file mode 100644
> index 0000000..622f605
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> @@ -0,0 +1,12 @@
> +#ifndef __ROCKCHIP_DRM_PSR___
> +#define __ROCKCHIP_DRM_PSR___
> +
> +void rockchip_drm_psr_flush(void);
> +int rockchip_drm_psr_enable(struct drm_crtc *crtc);
> +int rockchip_drm_psr_disable(struct drm_crtc *crtc);
> +
> +int rockchip_drm_psr_register(struct drm_encoder *encoder,
> +                           int (*psr_set)(struct drm_encoder *, bool enable));
> +void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
> +
> +#endif /* __ROCKCHIP_DRM_PSR__ */
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index cd3cac5..3c6dfc5 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -34,6 +34,7 @@
>  #include "rockchip_drm_drv.h"
>  #include "rockchip_drm_gem.h"
>  #include "rockchip_drm_fb.h"
> +#include "rockchip_drm_psr.h"
>  #include "rockchip_drm_vop.h"
>
>  #define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
> @@ -121,6 +122,9 @@ struct vop {
>         /* protected by dev->event_lock */
>         struct drm_pending_vblank_event *event;
>
> +       bool psr_enabled;
> +       struct delayed_work psr_work;
> +
>         struct completion line_flag_completion;
>
>         const struct vop_data *data;
> @@ -911,6 +915,16 @@ static const struct drm_plane_funcs vop_plane_funcs = {
>         .atomic_destroy_state = vop_atomic_plane_destroy_state,
>  };
>
> +static void vop_psr_work(struct work_struct *work)
> +{
> +       struct vop *vop = container_of(work, typeof(*vop), psr_work.work);
> +
> +       if (vop->psr_enabled)
> +               rockchip_drm_psr_enable(&vop->crtc);
> +       else
> +               rockchip_drm_psr_disable(&vop->crtc);
> +}
> +
>  static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>  {
>         struct vop *vop = to_vop(crtc);
> @@ -925,6 +939,9 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>
>         spin_unlock_irqrestore(&vop->irq_lock, flags);
>
> +       vop->psr_enabled = false;
> +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
> +
>         return 0;
>  }
>
> @@ -941,6 +958,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
>         VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 0);
>
>         spin_unlock_irqrestore(&vop->irq_lock, flags);
> +
> +       vop->psr_enabled = true;
> +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
>  }
>
>  static void vop_crtc_wait_for_update(struct drm_crtc *crtc)
> @@ -1582,6 +1602,10 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>                 return ret;
>
>         pm_runtime_enable(&pdev->dev);
> +
> +       vop->psr_enabled = false;
> +       INIT_DELAYED_WORK(&vop->psr_work, vop_psr_work);
> +
>         return 0;
>  }
>
> --
> 1.9.1
>
>
Yakir Yang July 8, 2016, 2:12 a.m. UTC | #2
Sean,

On 07/02/2016 02:00 AM, Sean Paul wrote:
> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> The PSR driver have exported four symbols for specific device driver:
>> - rockchip_drm_psr_register()
>> - rockchip_drm_psr_unregister()
>> - rockchip_drm_psr_enable()
>> - rockchip_drm_psr_disable()
>> - rockchip_drm_psr_flush()
>>
>> Encoder driver should call the register/unregister interfaces to hook
>> itself into common PSR driver, encoder have implement the 'psr_set'
>> callback which use the set PSR state in hardware side.
>>
>> Crtc driver would call the enable/disable interfaces when vblank is
>> enable/disable, after that the common PSR driver would call the encoder
>> registered callback to set the PSR state.
>>
> This feels overly complicated. It seems like you could cut out a bunch
> of code by just coding the psr functions into vop and
> analogix_dp-rockchip. I suppose the only reason to keep it abstracted
> would be if you plan on supporting psr in a different encoder or crtc
> in rockchip, or if you're planning on moving this into drm core.

I split the psr code into a separate driver, cause it seems make things 
more clearly.

'rockchip_drm_vop.c' need to call the PSR enable/disable interfaces, and 
'rockchip_drm_drv.c' need to call the PSR flush interface. If I put the 
whole psr code into analogix_dp-rockchip driver, then the Rockchip drm 
core driver would tie to analogix_dp-rockchip driver very deeply, and 
hard to expand if there are two PSR device in further.

> Perhaps others will disagree with this sentiment and this is the right
> thing to do.
>
>> Fb driver would call the flush interface in 'fb->dirty' callback, this
>> helper function would force all PSR enabled encoders to exit from PSR
>> for 3 seconds.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v3:
>> - split the psr flow into an common abstracted PSR driver
>> - implement the 'fb->dirty' callback function (Daniel)
>> - avoid to use notify to acqiure for vact event (Daniel)
>> - remove psr_active() callback which introduce in v2
>>
>> Changes in v2: None
>>
>>   drivers/gpu/drm/rockchip/Makefile           |   2 +-
>>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  12 ++
>>   drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 200 ++++++++++++++++++++++++++++
>>   drivers/gpu/drm/rockchip/rockchip_drm_psr.h |  12 ++
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  24 ++++
>>   5 files changed, 249 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
>>
>> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
>> index 05d0713..9746365 100644
>> --- a/drivers/gpu/drm/rockchip/Makefile
>> +++ b/drivers/gpu/drm/rockchip/Makefile
>> @@ -3,7 +3,7 @@
>>   # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>>
>>   rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>> -               rockchip_drm_gem.o rockchip_drm_vop.o
>> +               rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o
>>   rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>>
>>   obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> index 20f12bc..0fec18f 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> @@ -21,6 +21,7 @@
>>
>>   #include "rockchip_drm_drv.h"
>>   #include "rockchip_drm_gem.h"
>> +#include "rockchip_drm_psr.h"
>>
>>   #define to_rockchip_fb(x) container_of(x, struct rockchip_drm_fb, fb)
>>
>> @@ -66,9 +67,20 @@ static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
>>                                       rockchip_fb->obj[0], handle);
>>   }
>>
>> +static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
>> +                                struct drm_file *file,
>> +                                unsigned int flags, unsigned int color,
>> +                                struct drm_clip_rect *clips,
>> +                                unsigned int num_clips)
>> +{
>> +       rockchip_drm_psr_flush();
>> +       return 0;
>> +}
>> +
>>   static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>>          .destroy        = rockchip_drm_fb_destroy,
>>          .create_handle  = rockchip_drm_fb_create_handle,
>> +       .dirty          = rockchip_drm_fb_dirty,
>>   };
>>
>>   static struct rockchip_drm_fb *
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>> new file mode 100644
>> index 0000000..c044443
>> --- /dev/null
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>> @@ -0,0 +1,200 @@
>> +#include <drm/drm_crtc_helper.h>
>> +
>> +#include "rockchip_drm_psr.h"
>> +
>> +#define PSR_FLUSH_TIMEOUT      msecs_to_jiffies(3000) /* 3 seconds */
>> +
>> +static LIST_HEAD(psr_list);
>> +static DEFINE_MUTEX(psr_list_mutex);
> I'm not crazy about these globals. Perhaps you can initialize them
> with the rockchip driver and tuck them in a driver-level struct
> (rockchip_drm_private or something).

Okay, done.

>
>> +
>> +enum psr_state {
>> +       PSR_FLUSH,
>> +       PSR_ENABLE,
>> +       PSR_DISABLE,
>> +};
>> +
>> +struct psr_drv {
>> +       struct list_head list;
>> +       enum psr_state state;
>> +       struct mutex state_mutex;
>> +
>> +       struct timer_list flush_timer;
>> +
>> +       struct drm_encoder *encoder;
>> +       int (*set)(struct drm_encoder *encoder, bool enable);
>> +};
>> +
>> +static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
>> +{
>> +       struct psr_drv *psr;
>> +
>> +       mutex_lock(&psr_list_mutex);
>> +       list_for_each_entry(psr, &psr_list, list) {
>> +               if (psr->encoder->crtc == crtc) {
>> +                       mutex_unlock(&psr_list_mutex);
>> +                       return psr;
>> +               }
>> +       }
>> +       mutex_unlock(&psr_list_mutex);
>> +
>> +       return ERR_PTR(-ENODEV);
>> +}
>> +
>> +static void psr_enable(struct psr_drv *psr)
>> +{
>> +       if (psr->state == PSR_ENABLE)
>> +               return;
> Should you be worried about races by accessing this outside of the state mutex?
>

Yes, this should under the protect of state_mutex.

>> +
>> +       mutex_lock(&psr->state_mutex);
>> +       psr->state = PSR_ENABLE;
>> +       psr->set(psr->encoder, true);
>> +       mutex_unlock(&psr->state_mutex);
>> +}
>> +
>> +static void psr_disable(struct psr_drv *psr)
>> +{
>> +       if (psr->state == PSR_DISABLE)
>> +               return;
>> +
>> +       mutex_lock(&psr->state_mutex);
>> +       psr->state = PSR_DISABLE;
>> +       psr->set(psr->encoder, false);
>> +       mutex_unlock(&psr->state_mutex);
>> +}
>> +
>> +static void psr_flush_handler(unsigned long data)
>> +{
>> +       struct psr_drv *psr = (struct psr_drv *)data;
>> +
>> +       if (!psr || psr->state != PSR_FLUSH)
>> +               return;
>> +
>> +       psr_enable(psr);
>> +}
>> +
>> +/**
>> + * rockchip_drm_psr_enable - enable the encoder PSR which bind to given CRTC
>> + * @crtc: CRTC to obtain the PSR encoder
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int rockchip_drm_psr_enable(struct drm_crtc *crtc)
>> +{
>> +       struct psr_drv *psr = find_psr_by_crtc(crtc);
>> +
>> +       if (IS_ERR(psr))
>> +               return PTR_ERR(psr);
>> +
>> +       psr_enable(psr);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(rockchip_drm_psr_enable);
>> +
>> +/**
>> + * rockchip_drm_psr_disable - disable the encoder PSR which bind to given CRTC
>> + * @crtc: CRTC to obtain the PSR encoder
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int rockchip_drm_psr_disable(struct drm_crtc *crtc)
>> +{
>> +       struct psr_drv *psr = find_psr_by_crtc(crtc);
>> +
>> +       if (IS_ERR(psr))
>> +               return PTR_ERR(psr);
>> +
>> +       psr_disable(psr);
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(rockchip_drm_psr_disable);
>> +
>> +/**
>> + * rockchip_drm_psr_flush - force to flush all registered PSR encoders
>> + *
>> + * Disable the PSR function for all registered encoders, and then enable the
>> + * PSR function back after 5 second. If encoder PSR state have been changed
> s/5 second/PSR_FLUSH_TIMEOUT/

Done

>
>> + * during flush time, then keep the state no change after flush timeout.
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +void rockchip_drm_psr_flush(void)
>> +{
>> +       struct psr_drv *psr;
>> +
>> +       mutex_lock(&psr_list_mutex);
>> +       list_for_each_entry(psr, &psr_list, list) {
>> +               if (psr->state == PSR_DISABLE)
>> +                       continue;
>> +
>> +               mod_timer(&psr->flush_timer,
>> +                         round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
>> +
>> +               psr_disable(psr);
>> +               psr->state = PSR_FLUSH;
> This is set outside of state_mutex, is that intentional?

No, I just don't want to create another interface like psr_flush(). But 
it's a bad idea to let 'state' change outside the protect of 'state_mutex'.

Thanks,
- Yakir

>> +       }
>> +       mutex_unlock(&psr_list_mutex);
>> +}
>> +EXPORT_SYMBOL(rockchip_drm_psr_flush);
>> +
>> +/**
>> + * rockchip_drm_psr_register - register encoder to psr driver
>> + * @encoder: encoder that obtain the PSR function
>> + * @psr_set: call back to set PSR state
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +int rockchip_drm_psr_register(struct drm_encoder *encoder,
>> +                             int (*psr_set)(struct drm_encoder *, bool enable))
>> +{
>> +       struct psr_drv *psr;
>> +
>> +       if (!encoder || !psr_set)
>> +               return -EINVAL;
>> +
>> +       psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
>> +       if (!psr)
>> +               return -ENOMEM;
>> +
>> +       setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
>> +
>> +       mutex_init(&psr->state_mutex);
>> +
>> +       psr->state = PSR_DISABLE;
>> +       psr->encoder = encoder;
>> +       psr->set = psr_set;
>> +
>> +       mutex_lock(&psr_list_mutex);
>> +       list_add_tail(&psr->list, &psr_list);
>> +       mutex_unlock(&psr_list_mutex);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(rockchip_drm_psr_register);
>> +
>> +/**
>> + * rockchip_drm_psr_unregister - unregister encoder to psr driver
>> + * @encoder: encoder that obtain the PSR function
>> + * @psr_set: call back to set PSR state
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
>> +void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
>> +{
>> +       struct psr_drv *psr;
>> +
>> +       mutex_lock(&psr_list_mutex);
>> +       list_for_each_entry(psr, &psr_list, list) {
>> +               if (psr->encoder == encoder) {
>> +                       del_timer(&psr->flush_timer);
>> +                       list_del(&psr->list);
>> +                       kfree(psr);
>> +               }
>> +       }
>> +       mutex_unlock(&psr_list_mutex);
>> +}
>> +EXPORT_SYMBOL(rockchip_drm_psr_unregister);
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
>> new file mode 100644
>> index 0000000..622f605
>> --- /dev/null
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
>> @@ -0,0 +1,12 @@
>> +#ifndef __ROCKCHIP_DRM_PSR___
>> +#define __ROCKCHIP_DRM_PSR___
>> +
>> +void rockchip_drm_psr_flush(void);
>> +int rockchip_drm_psr_enable(struct drm_crtc *crtc);
>> +int rockchip_drm_psr_disable(struct drm_crtc *crtc);
>> +
>> +int rockchip_drm_psr_register(struct drm_encoder *encoder,
>> +                           int (*psr_set)(struct drm_encoder *, bool enable));
>> +void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
>> +
>> +#endif /* __ROCKCHIP_DRM_PSR__ */
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index cd3cac5..3c6dfc5 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -34,6 +34,7 @@
>>   #include "rockchip_drm_drv.h"
>>   #include "rockchip_drm_gem.h"
>>   #include "rockchip_drm_fb.h"
>> +#include "rockchip_drm_psr.h"
>>   #include "rockchip_drm_vop.h"
>>
>>   #define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
>> @@ -121,6 +122,9 @@ struct vop {
>>          /* protected by dev->event_lock */
>>          struct drm_pending_vblank_event *event;
>>
>> +       bool psr_enabled;
>> +       struct delayed_work psr_work;
>> +
>>          struct completion line_flag_completion;
>>
>>          const struct vop_data *data;
>> @@ -911,6 +915,16 @@ static const struct drm_plane_funcs vop_plane_funcs = {
>>          .atomic_destroy_state = vop_atomic_plane_destroy_state,
>>   };
>>
>> +static void vop_psr_work(struct work_struct *work)
>> +{
>> +       struct vop *vop = container_of(work, typeof(*vop), psr_work.work);
>> +
>> +       if (vop->psr_enabled)
>> +               rockchip_drm_psr_enable(&vop->crtc);
>> +       else
>> +               rockchip_drm_psr_disable(&vop->crtc);
>> +}
>> +
>>   static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>>   {
>>          struct vop *vop = to_vop(crtc);
>> @@ -925,6 +939,9 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>>
>>          spin_unlock_irqrestore(&vop->irq_lock, flags);
>>
>> +       vop->psr_enabled = false;
>> +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
>> +
>>          return 0;
>>   }
>>
>> @@ -941,6 +958,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
>>          VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 0);
>>
>>          spin_unlock_irqrestore(&vop->irq_lock, flags);
>> +
>> +       vop->psr_enabled = true;
>> +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
>>   }
>>
>>   static void vop_crtc_wait_for_update(struct drm_crtc *crtc)
>> @@ -1582,6 +1602,10 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>                  return ret;
>>
>>          pm_runtime_enable(&pdev->dev);
>> +
>> +       vop->psr_enabled = false;
>> +       INIT_DELAYED_WORK(&vop->psr_work, vop_psr_work);
>> +
>>          return 0;
>>   }
>>
>> --
>> 1.9.1
>>
>>
>
>
Daniel Vetter July 12, 2016, 12:38 p.m. UTC | #3
On Fri, Jul 01, 2016 at 02:00:00PM -0400, Sean Paul wrote:
> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> > The PSR driver have exported four symbols for specific device driver:
> > - rockchip_drm_psr_register()
> > - rockchip_drm_psr_unregister()
> > - rockchip_drm_psr_enable()
> > - rockchip_drm_psr_disable()
> > - rockchip_drm_psr_flush()
> >
> > Encoder driver should call the register/unregister interfaces to hook
> > itself into common PSR driver, encoder have implement the 'psr_set'
> > callback which use the set PSR state in hardware side.
> >
> > Crtc driver would call the enable/disable interfaces when vblank is
> > enable/disable, after that the common PSR driver would call the encoder
> > registered callback to set the PSR state.
> >
> 
> This feels overly complicated. It seems like you could cut out a bunch
> of code by just coding the psr functions into vop and
> analogix_dp-rockchip. I suppose the only reason to keep it abstracted
> would be if you plan on supporting psr in a different encoder or crtc
> in rockchip, or if you're planning on moving this into drm core.

Agreed on the layers of indirection. Also, you end up with 3 delayed
timers in total:
- defio timer from fbdev emulation
- timer in this abstraction
- delayed work in the psr backend driver

I'd cut out at least the middle one.

But since this seems to correctly use the ->dirty callback it gets my Ack
either way ;-)

Cheers, Daniel

> 
> Perhaps others will disagree with this sentiment and this is the right
> thing to do.
> 
> > Fb driver would call the flush interface in 'fb->dirty' callback, this
> > helper function would force all PSR enabled encoders to exit from PSR
> > for 3 seconds.
> >
> > Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> > ---
> > Changes in v3:
> > - split the psr flow into an common abstracted PSR driver
> > - implement the 'fb->dirty' callback function (Daniel)
> > - avoid to use notify to acqiure for vact event (Daniel)
> > - remove psr_active() callback which introduce in v2
> >
> > Changes in v2: None
> >
> >  drivers/gpu/drm/rockchip/Makefile           |   2 +-
> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  12 ++
> >  drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 200 ++++++++++++++++++++++++++++
> >  drivers/gpu/drm/rockchip/rockchip_drm_psr.h |  12 ++
> >  drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  24 ++++
> >  5 files changed, 249 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> >  create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> >
> > diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
> > index 05d0713..9746365 100644
> > --- a/drivers/gpu/drm/rockchip/Makefile
> > +++ b/drivers/gpu/drm/rockchip/Makefile
> > @@ -3,7 +3,7 @@
> >  # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
> >
> >  rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
> > -               rockchip_drm_gem.o rockchip_drm_vop.o
> > +               rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o
> >  rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
> >
> >  obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index 20f12bc..0fec18f 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -21,6 +21,7 @@
> >
> >  #include "rockchip_drm_drv.h"
> >  #include "rockchip_drm_gem.h"
> > +#include "rockchip_drm_psr.h"
> >
> >  #define to_rockchip_fb(x) container_of(x, struct rockchip_drm_fb, fb)
> >
> > @@ -66,9 +67,20 @@ static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
> >                                      rockchip_fb->obj[0], handle);
> >  }
> >
> > +static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
> > +                                struct drm_file *file,
> > +                                unsigned int flags, unsigned int color,
> > +                                struct drm_clip_rect *clips,
> > +                                unsigned int num_clips)
> > +{
> > +       rockchip_drm_psr_flush();
> > +       return 0;
> > +}
> > +
> >  static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
> >         .destroy        = rockchip_drm_fb_destroy,
> >         .create_handle  = rockchip_drm_fb_create_handle,
> > +       .dirty          = rockchip_drm_fb_dirty,
> >  };
> >
> >  static struct rockchip_drm_fb *
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> > new file mode 100644
> > index 0000000..c044443
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> > @@ -0,0 +1,200 @@
> > +#include <drm/drm_crtc_helper.h>
> > +
> > +#include "rockchip_drm_psr.h"
> > +
> > +#define PSR_FLUSH_TIMEOUT      msecs_to_jiffies(3000) /* 3 seconds */
> > +
> > +static LIST_HEAD(psr_list);
> > +static DEFINE_MUTEX(psr_list_mutex);
> 
> I'm not crazy about these globals. Perhaps you can initialize them
> with the rockchip driver and tuck them in a driver-level struct
> (rockchip_drm_private or something).
> 
> 
> > +
> > +enum psr_state {
> > +       PSR_FLUSH,
> > +       PSR_ENABLE,
> > +       PSR_DISABLE,
> > +};
> > +
> > +struct psr_drv {
> > +       struct list_head list;
> > +       enum psr_state state;
> > +       struct mutex state_mutex;
> > +
> > +       struct timer_list flush_timer;
> > +
> > +       struct drm_encoder *encoder;
> > +       int (*set)(struct drm_encoder *encoder, bool enable);
> > +};
> > +
> > +static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
> > +{
> > +       struct psr_drv *psr;
> > +
> > +       mutex_lock(&psr_list_mutex);
> > +       list_for_each_entry(psr, &psr_list, list) {
> > +               if (psr->encoder->crtc == crtc) {
> > +                       mutex_unlock(&psr_list_mutex);
> > +                       return psr;
> > +               }
> > +       }
> > +       mutex_unlock(&psr_list_mutex);
> > +
> > +       return ERR_PTR(-ENODEV);
> > +}
> > +
> > +static void psr_enable(struct psr_drv *psr)
> > +{
> > +       if (psr->state == PSR_ENABLE)
> > +               return;
> 
> Should you be worried about races by accessing this outside of the state mutex?
> 
> > +
> > +       mutex_lock(&psr->state_mutex);
> > +       psr->state = PSR_ENABLE;
> > +       psr->set(psr->encoder, true);
> > +       mutex_unlock(&psr->state_mutex);
> > +}
> > +
> > +static void psr_disable(struct psr_drv *psr)
> > +{
> > +       if (psr->state == PSR_DISABLE)
> > +               return;
> > +
> > +       mutex_lock(&psr->state_mutex);
> > +       psr->state = PSR_DISABLE;
> > +       psr->set(psr->encoder, false);
> > +       mutex_unlock(&psr->state_mutex);
> > +}
> > +
> > +static void psr_flush_handler(unsigned long data)
> > +{
> > +       struct psr_drv *psr = (struct psr_drv *)data;
> > +
> > +       if (!psr || psr->state != PSR_FLUSH)
> > +               return;
> > +
> > +       psr_enable(psr);
> > +}
> > +
> > +/**
> > + * rockchip_drm_psr_enable - enable the encoder PSR which bind to given CRTC
> > + * @crtc: CRTC to obtain the PSR encoder
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int rockchip_drm_psr_enable(struct drm_crtc *crtc)
> > +{
> > +       struct psr_drv *psr = find_psr_by_crtc(crtc);
> > +
> > +       if (IS_ERR(psr))
> > +               return PTR_ERR(psr);
> > +
> > +       psr_enable(psr);
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(rockchip_drm_psr_enable);
> > +
> > +/**
> > + * rockchip_drm_psr_disable - disable the encoder PSR which bind to given CRTC
> > + * @crtc: CRTC to obtain the PSR encoder
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int rockchip_drm_psr_disable(struct drm_crtc *crtc)
> > +{
> > +       struct psr_drv *psr = find_psr_by_crtc(crtc);
> > +
> > +       if (IS_ERR(psr))
> > +               return PTR_ERR(psr);
> > +
> > +       psr_disable(psr);
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(rockchip_drm_psr_disable);
> > +
> > +/**
> > + * rockchip_drm_psr_flush - force to flush all registered PSR encoders
> > + *
> > + * Disable the PSR function for all registered encoders, and then enable the
> > + * PSR function back after 5 second. If encoder PSR state have been changed
> 
> s/5 second/PSR_FLUSH_TIMEOUT/
> 
> > + * during flush time, then keep the state no change after flush timeout.
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +void rockchip_drm_psr_flush(void)
> > +{
> > +       struct psr_drv *psr;
> > +
> > +       mutex_lock(&psr_list_mutex);
> > +       list_for_each_entry(psr, &psr_list, list) {
> > +               if (psr->state == PSR_DISABLE)
> > +                       continue;
> > +
> > +               mod_timer(&psr->flush_timer,
> > +                         round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
> > +
> > +               psr_disable(psr);
> > +               psr->state = PSR_FLUSH;
> 
> This is set outside of state_mutex, is that intentional?
> 
> > +       }
> > +       mutex_unlock(&psr_list_mutex);
> > +}
> > +EXPORT_SYMBOL(rockchip_drm_psr_flush);
> > +
> > +/**
> > + * rockchip_drm_psr_register - register encoder to psr driver
> > + * @encoder: encoder that obtain the PSR function
> > + * @psr_set: call back to set PSR state
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +int rockchip_drm_psr_register(struct drm_encoder *encoder,
> > +                             int (*psr_set)(struct drm_encoder *, bool enable))
> > +{
> > +       struct psr_drv *psr;
> > +
> > +       if (!encoder || !psr_set)
> > +               return -EINVAL;
> > +
> > +       psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
> > +       if (!psr)
> > +               return -ENOMEM;
> > +
> > +       setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
> > +
> > +       mutex_init(&psr->state_mutex);
> > +
> > +       psr->state = PSR_DISABLE;
> > +       psr->encoder = encoder;
> > +       psr->set = psr_set;
> > +
> > +       mutex_lock(&psr_list_mutex);
> > +       list_add_tail(&psr->list, &psr_list);
> > +       mutex_unlock(&psr_list_mutex);
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL(rockchip_drm_psr_register);
> > +
> > +/**
> > + * rockchip_drm_psr_unregister - unregister encoder to psr driver
> > + * @encoder: encoder that obtain the PSR function
> > + * @psr_set: call back to set PSR state
> > + *
> > + * Returns:
> > + * Zero on success, negative errno on failure.
> > + */
> > +void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
> > +{
> > +       struct psr_drv *psr;
> > +
> > +       mutex_lock(&psr_list_mutex);
> > +       list_for_each_entry(psr, &psr_list, list) {
> > +               if (psr->encoder == encoder) {
> > +                       del_timer(&psr->flush_timer);
> > +                       list_del(&psr->list);
> > +                       kfree(psr);
> > +               }
> > +       }
> > +       mutex_unlock(&psr_list_mutex);
> > +}
> > +EXPORT_SYMBOL(rockchip_drm_psr_unregister);
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> > new file mode 100644
> > index 0000000..622f605
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
> > @@ -0,0 +1,12 @@
> > +#ifndef __ROCKCHIP_DRM_PSR___
> > +#define __ROCKCHIP_DRM_PSR___
> > +
> > +void rockchip_drm_psr_flush(void);
> > +int rockchip_drm_psr_enable(struct drm_crtc *crtc);
> > +int rockchip_drm_psr_disable(struct drm_crtc *crtc);
> > +
> > +int rockchip_drm_psr_register(struct drm_encoder *encoder,
> > +                           int (*psr_set)(struct drm_encoder *, bool enable));
> > +void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
> > +
> > +#endif /* __ROCKCHIP_DRM_PSR__ */
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > index cd3cac5..3c6dfc5 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> > @@ -34,6 +34,7 @@
> >  #include "rockchip_drm_drv.h"
> >  #include "rockchip_drm_gem.h"
> >  #include "rockchip_drm_fb.h"
> > +#include "rockchip_drm_psr.h"
> >  #include "rockchip_drm_vop.h"
> >
> >  #define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
> > @@ -121,6 +122,9 @@ struct vop {
> >         /* protected by dev->event_lock */
> >         struct drm_pending_vblank_event *event;
> >
> > +       bool psr_enabled;
> > +       struct delayed_work psr_work;
> > +
> >         struct completion line_flag_completion;
> >
> >         const struct vop_data *data;
> > @@ -911,6 +915,16 @@ static const struct drm_plane_funcs vop_plane_funcs = {
> >         .atomic_destroy_state = vop_atomic_plane_destroy_state,
> >  };
> >
> > +static void vop_psr_work(struct work_struct *work)
> > +{
> > +       struct vop *vop = container_of(work, typeof(*vop), psr_work.work);
> > +
> > +       if (vop->psr_enabled)
> > +               rockchip_drm_psr_enable(&vop->crtc);
> > +       else
> > +               rockchip_drm_psr_disable(&vop->crtc);
> > +}
> > +
> >  static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
> >  {
> >         struct vop *vop = to_vop(crtc);
> > @@ -925,6 +939,9 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
> >
> >         spin_unlock_irqrestore(&vop->irq_lock, flags);
> >
> > +       vop->psr_enabled = false;
> > +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
> > +
> >         return 0;
> >  }
> >
> > @@ -941,6 +958,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
> >         VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 0);
> >
> >         spin_unlock_irqrestore(&vop->irq_lock, flags);
> > +
> > +       vop->psr_enabled = true;
> > +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
> >  }
> >
> >  static void vop_crtc_wait_for_update(struct drm_crtc *crtc)
> > @@ -1582,6 +1602,10 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
> >                 return ret;
> >
> >         pm_runtime_enable(&pdev->dev);
> > +
> > +       vop->psr_enabled = false;
> > +       INIT_DELAYED_WORK(&vop->psr_work, vop_psr_work);
> > +
> >         return 0;
> >  }
> >
> > --
> > 1.9.1
> >
> >
Tomasz Figa July 12, 2016, 3:10 p.m. UTC | #4
On Tue, Jul 12, 2016 at 9:38 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jul 01, 2016 at 02:00:00PM -0400, Sean Paul wrote:
>> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> > The PSR driver have exported four symbols for specific device driver:
>> > - rockchip_drm_psr_register()
>> > - rockchip_drm_psr_unregister()
>> > - rockchip_drm_psr_enable()
>> > - rockchip_drm_psr_disable()
>> > - rockchip_drm_psr_flush()
>> >
>> > Encoder driver should call the register/unregister interfaces to hook
>> > itself into common PSR driver, encoder have implement the 'psr_set'
>> > callback which use the set PSR state in hardware side.
>> >
>> > Crtc driver would call the enable/disable interfaces when vblank is
>> > enable/disable, after that the common PSR driver would call the encoder
>> > registered callback to set the PSR state.
>> >
>>
>> This feels overly complicated. It seems like you could cut out a bunch
>> of code by just coding the psr functions into vop and
>> analogix_dp-rockchip. I suppose the only reason to keep it abstracted
>> would be if you plan on supporting psr in a different encoder or crtc
>> in rockchip, or if you're planning on moving this into drm core.
>
> Agreed on the layers of indirection. Also, you end up with 3 delayed
> timers in total:
> - defio timer from fbdev emulation
> - timer in this abstraction
> - delayed work in the psr backend driver

Maybe I'm missing something obvious (don't know how the PSR is
implemented in hardware or in other drivers), but why do we need all
these timers? Couldn't we just trigger a fake page flip on fb dirty
call?

Best regards,
Tomasz
Daniel Vetter July 12, 2016, 4:10 p.m. UTC | #5
On Wed, Jul 13, 2016 at 12:10:13AM +0900, Tomasz Figa wrote:
> On Tue, Jul 12, 2016 at 9:38 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Fri, Jul 01, 2016 at 02:00:00PM -0400, Sean Paul wrote:
> >> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> >> > The PSR driver have exported four symbols for specific device driver:
> >> > - rockchip_drm_psr_register()
> >> > - rockchip_drm_psr_unregister()
> >> > - rockchip_drm_psr_enable()
> >> > - rockchip_drm_psr_disable()
> >> > - rockchip_drm_psr_flush()
> >> >
> >> > Encoder driver should call the register/unregister interfaces to hook
> >> > itself into common PSR driver, encoder have implement the 'psr_set'
> >> > callback which use the set PSR state in hardware side.
> >> >
> >> > Crtc driver would call the enable/disable interfaces when vblank is
> >> > enable/disable, after that the common PSR driver would call the encoder
> >> > registered callback to set the PSR state.
> >> >
> >>
> >> This feels overly complicated. It seems like you could cut out a bunch
> >> of code by just coding the psr functions into vop and
> >> analogix_dp-rockchip. I suppose the only reason to keep it abstracted
> >> would be if you plan on supporting psr in a different encoder or crtc
> >> in rockchip, or if you're planning on moving this into drm core.
> >
> > Agreed on the layers of indirection. Also, you end up with 3 delayed
> > timers in total:
> > - defio timer from fbdev emulation
> > - timer in this abstraction
> > - delayed work in the psr backend driver
> 
> Maybe I'm missing something obvious (don't know how the PSR is
> implemented in hardware or in other drivers), but why do we need all
> these timers? Couldn't we just trigger a fake page flip on fb dirty
> call?

Page flips are ratelimited to vrefresh, and not all drivers support it.
The other issue is that dirty ioctl supports dirty rectangles, but page
flip is still a full-screen upload. We discussed adding a dirty rectangle
to the kms properties to fix this gap, but until that's done I don't think
it makes much sense to remap dirty to an (atomic) page flip. One small
technicality is that dirty works on framebuffers, but page flips
(especially atomic ones) work on crtcs. That makes the remapping a notch
more tricky.

But in principle, fully agreed. And once we have atomic dirty rectangles
we could type up a small helper that drivers can plug into the dirty
ioctl. Since new properties requires new userspace we could just track the
dirty rectangle internally to get this helper landed, if there's
interested for that. I'll certainly be happy to review patches.
-Daniel
Yakir Yang July 13, 2016, 3:36 a.m. UTC | #6
Daniel,

On 07/12/2016 08:38 PM, Daniel Vetter wrote:
> On Fri, Jul 01, 2016 at 02:00:00PM -0400, Sean Paul wrote:
>> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>> The PSR driver have exported four symbols for specific device driver:
>>> - rockchip_drm_psr_register()
>>> - rockchip_drm_psr_unregister()
>>> - rockchip_drm_psr_enable()
>>> - rockchip_drm_psr_disable()
>>> - rockchip_drm_psr_flush()
>>>
>>> Encoder driver should call the register/unregister interfaces to hook
>>> itself into common PSR driver, encoder have implement the 'psr_set'
>>> callback which use the set PSR state in hardware side.
>>>
>>> Crtc driver would call the enable/disable interfaces when vblank is
>>> enable/disable, after that the common PSR driver would call the encoder
>>> registered callback to set the PSR state.
>>>
>> This feels overly complicated. It seems like you could cut out a bunch
>> of code by just coding the psr functions into vop and
>> analogix_dp-rockchip. I suppose the only reason to keep it abstracted
>> would be if you plan on supporting psr in a different encoder or crtc
>> in rockchip, or if you're planning on moving this into drm core.
> Agreed on the layers of indirection. Also, you end up with 3 delayed
> timers in total:
> - defio timer from fbdev emulation
> - timer in this abstraction
> - delayed work in the psr backend driver
>
> I'd cut out at least the middle one.
>
> But since this seems to correctly use the ->dirty callback it gets my Ack
> either way ;-)

Aha, thanks :-D

- Yakir

> Cheers, Daniel
>
>> Perhaps others will disagree with this sentiment and this is the right
>> thing to do.
>>
>>> Fb driver would call the flush interface in 'fb->dirty' callback, this
>>> helper function would force all PSR enabled encoders to exit from PSR
>>> for 3 seconds.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> ---
>>> Changes in v3:
>>> - split the psr flow into an common abstracted PSR driver
>>> - implement the 'fb->dirty' callback function (Daniel)
>>> - avoid to use notify to acqiure for vact event (Daniel)
>>> - remove psr_active() callback which introduce in v2
>>>
>>> Changes in v2: None
>>>
>>>   drivers/gpu/drm/rockchip/Makefile           |   2 +-
>>>   drivers/gpu/drm/rockchip/rockchip_drm_fb.c  |  12 ++
>>>   drivers/gpu/drm/rockchip/rockchip_drm_psr.c | 200 ++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/rockchip/rockchip_drm_psr.h |  12 ++
>>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  24 ++++
>>>   5 files changed, 249 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>>>   create mode 100644 drivers/gpu/drm/rockchip/rockchip_drm_psr.h
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
>>> index 05d0713..9746365 100644
>>> --- a/drivers/gpu/drm/rockchip/Makefile
>>> +++ b/drivers/gpu/drm/rockchip/Makefile
>>> @@ -3,7 +3,7 @@
>>>   # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
>>>
>>>   rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
>>> -               rockchip_drm_gem.o rockchip_drm_vop.o
>>> +               rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o
>>>   rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
>>>
>>>   obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> index 20f12bc..0fec18f 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>>> @@ -21,6 +21,7 @@
>>>
>>>   #include "rockchip_drm_drv.h"
>>>   #include "rockchip_drm_gem.h"
>>> +#include "rockchip_drm_psr.h"
>>>
>>>   #define to_rockchip_fb(x) container_of(x, struct rockchip_drm_fb, fb)
>>>
>>> @@ -66,9 +67,20 @@ static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
>>>                                       rockchip_fb->obj[0], handle);
>>>   }
>>>
>>> +static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
>>> +                                struct drm_file *file,
>>> +                                unsigned int flags, unsigned int color,
>>> +                                struct drm_clip_rect *clips,
>>> +                                unsigned int num_clips)
>>> +{
>>> +       rockchip_drm_psr_flush();
>>> +       return 0;
>>> +}
>>> +
>>>   static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>>>          .destroy        = rockchip_drm_fb_destroy,
>>>          .create_handle  = rockchip_drm_fb_create_handle,
>>> +       .dirty          = rockchip_drm_fb_dirty,
>>>   };
>>>
>>>   static struct rockchip_drm_fb *
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>>> new file mode 100644
>>> index 0000000..c044443
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>>> @@ -0,0 +1,200 @@
>>> +#include <drm/drm_crtc_helper.h>
>>> +
>>> +#include "rockchip_drm_psr.h"
>>> +
>>> +#define PSR_FLUSH_TIMEOUT      msecs_to_jiffies(3000) /* 3 seconds */
>>> +
>>> +static LIST_HEAD(psr_list);
>>> +static DEFINE_MUTEX(psr_list_mutex);
>> I'm not crazy about these globals. Perhaps you can initialize them
>> with the rockchip driver and tuck them in a driver-level struct
>> (rockchip_drm_private or something).
>>
>>
>>> +
>>> +enum psr_state {
>>> +       PSR_FLUSH,
>>> +       PSR_ENABLE,
>>> +       PSR_DISABLE,
>>> +};
>>> +
>>> +struct psr_drv {
>>> +       struct list_head list;
>>> +       enum psr_state state;
>>> +       struct mutex state_mutex;
>>> +
>>> +       struct timer_list flush_timer;
>>> +
>>> +       struct drm_encoder *encoder;
>>> +       int (*set)(struct drm_encoder *encoder, bool enable);
>>> +};
>>> +
>>> +static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
>>> +{
>>> +       struct psr_drv *psr;
>>> +
>>> +       mutex_lock(&psr_list_mutex);
>>> +       list_for_each_entry(psr, &psr_list, list) {
>>> +               if (psr->encoder->crtc == crtc) {
>>> +                       mutex_unlock(&psr_list_mutex);
>>> +                       return psr;
>>> +               }
>>> +       }
>>> +       mutex_unlock(&psr_list_mutex);
>>> +
>>> +       return ERR_PTR(-ENODEV);
>>> +}
>>> +
>>> +static void psr_enable(struct psr_drv *psr)
>>> +{
>>> +       if (psr->state == PSR_ENABLE)
>>> +               return;
>> Should you be worried about races by accessing this outside of the state mutex?
>>
>>> +
>>> +       mutex_lock(&psr->state_mutex);
>>> +       psr->state = PSR_ENABLE;
>>> +       psr->set(psr->encoder, true);
>>> +       mutex_unlock(&psr->state_mutex);
>>> +}
>>> +
>>> +static void psr_disable(struct psr_drv *psr)
>>> +{
>>> +       if (psr->state == PSR_DISABLE)
>>> +               return;
>>> +
>>> +       mutex_lock(&psr->state_mutex);
>>> +       psr->state = PSR_DISABLE;
>>> +       psr->set(psr->encoder, false);
>>> +       mutex_unlock(&psr->state_mutex);
>>> +}
>>> +
>>> +static void psr_flush_handler(unsigned long data)
>>> +{
>>> +       struct psr_drv *psr = (struct psr_drv *)data;
>>> +
>>> +       if (!psr || psr->state != PSR_FLUSH)
>>> +               return;
>>> +
>>> +       psr_enable(psr);
>>> +}
>>> +
>>> +/**
>>> + * rockchip_drm_psr_enable - enable the encoder PSR which bind to given CRTC
>>> + * @crtc: CRTC to obtain the PSR encoder
>>> + *
>>> + * Returns:
>>> + * Zero on success, negative errno on failure.
>>> + */
>>> +int rockchip_drm_psr_enable(struct drm_crtc *crtc)
>>> +{
>>> +       struct psr_drv *psr = find_psr_by_crtc(crtc);
>>> +
>>> +       if (IS_ERR(psr))
>>> +               return PTR_ERR(psr);
>>> +
>>> +       psr_enable(psr);
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(rockchip_drm_psr_enable);
>>> +
>>> +/**
>>> + * rockchip_drm_psr_disable - disable the encoder PSR which bind to given CRTC
>>> + * @crtc: CRTC to obtain the PSR encoder
>>> + *
>>> + * Returns:
>>> + * Zero on success, negative errno on failure.
>>> + */
>>> +int rockchip_drm_psr_disable(struct drm_crtc *crtc)
>>> +{
>>> +       struct psr_drv *psr = find_psr_by_crtc(crtc);
>>> +
>>> +       if (IS_ERR(psr))
>>> +               return PTR_ERR(psr);
>>> +
>>> +       psr_disable(psr);
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(rockchip_drm_psr_disable);
>>> +
>>> +/**
>>> + * rockchip_drm_psr_flush - force to flush all registered PSR encoders
>>> + *
>>> + * Disable the PSR function for all registered encoders, and then enable the
>>> + * PSR function back after 5 second. If encoder PSR state have been changed
>> s/5 second/PSR_FLUSH_TIMEOUT/
>>
>>> + * during flush time, then keep the state no change after flush timeout.
>>> + *
>>> + * Returns:
>>> + * Zero on success, negative errno on failure.
>>> + */
>>> +void rockchip_drm_psr_flush(void)
>>> +{
>>> +       struct psr_drv *psr;
>>> +
>>> +       mutex_lock(&psr_list_mutex);
>>> +       list_for_each_entry(psr, &psr_list, list) {
>>> +               if (psr->state == PSR_DISABLE)
>>> +                       continue;
>>> +
>>> +               mod_timer(&psr->flush_timer,
>>> +                         round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
>>> +
>>> +               psr_disable(psr);
>>> +               psr->state = PSR_FLUSH;
>> This is set outside of state_mutex, is that intentional?
>>
>>> +       }
>>> +       mutex_unlock(&psr_list_mutex);
>>> +}
>>> +EXPORT_SYMBOL(rockchip_drm_psr_flush);
>>> +
>>> +/**
>>> + * rockchip_drm_psr_register - register encoder to psr driver
>>> + * @encoder: encoder that obtain the PSR function
>>> + * @psr_set: call back to set PSR state
>>> + *
>>> + * Returns:
>>> + * Zero on success, negative errno on failure.
>>> + */
>>> +int rockchip_drm_psr_register(struct drm_encoder *encoder,
>>> +                             int (*psr_set)(struct drm_encoder *, bool enable))
>>> +{
>>> +       struct psr_drv *psr;
>>> +
>>> +       if (!encoder || !psr_set)
>>> +               return -EINVAL;
>>> +
>>> +       psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
>>> +       if (!psr)
>>> +               return -ENOMEM;
>>> +
>>> +       setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
>>> +
>>> +       mutex_init(&psr->state_mutex);
>>> +
>>> +       psr->state = PSR_DISABLE;
>>> +       psr->encoder = encoder;
>>> +       psr->set = psr_set;
>>> +
>>> +       mutex_lock(&psr_list_mutex);
>>> +       list_add_tail(&psr->list, &psr_list);
>>> +       mutex_unlock(&psr_list_mutex);
>>> +
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(rockchip_drm_psr_register);
>>> +
>>> +/**
>>> + * rockchip_drm_psr_unregister - unregister encoder to psr driver
>>> + * @encoder: encoder that obtain the PSR function
>>> + * @psr_set: call back to set PSR state
>>> + *
>>> + * Returns:
>>> + * Zero on success, negative errno on failure.
>>> + */
>>> +void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
>>> +{
>>> +       struct psr_drv *psr;
>>> +
>>> +       mutex_lock(&psr_list_mutex);
>>> +       list_for_each_entry(psr, &psr_list, list) {
>>> +               if (psr->encoder == encoder) {
>>> +                       del_timer(&psr->flush_timer);
>>> +                       list_del(&psr->list);
>>> +                       kfree(psr);
>>> +               }
>>> +       }
>>> +       mutex_unlock(&psr_list_mutex);
>>> +}
>>> +EXPORT_SYMBOL(rockchip_drm_psr_unregister);
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
>>> new file mode 100644
>>> index 0000000..622f605
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
>>> @@ -0,0 +1,12 @@
>>> +#ifndef __ROCKCHIP_DRM_PSR___
>>> +#define __ROCKCHIP_DRM_PSR___
>>> +
>>> +void rockchip_drm_psr_flush(void);
>>> +int rockchip_drm_psr_enable(struct drm_crtc *crtc);
>>> +int rockchip_drm_psr_disable(struct drm_crtc *crtc);
>>> +
>>> +int rockchip_drm_psr_register(struct drm_encoder *encoder,
>>> +                           int (*psr_set)(struct drm_encoder *, bool enable));
>>> +void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
>>> +
>>> +#endif /* __ROCKCHIP_DRM_PSR__ */
>>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> index cd3cac5..3c6dfc5 100644
>>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>>> @@ -34,6 +34,7 @@
>>>   #include "rockchip_drm_drv.h"
>>>   #include "rockchip_drm_gem.h"
>>>   #include "rockchip_drm_fb.h"
>>> +#include "rockchip_drm_psr.h"
>>>   #include "rockchip_drm_vop.h"
>>>
>>>   #define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
>>> @@ -121,6 +122,9 @@ struct vop {
>>>          /* protected by dev->event_lock */
>>>          struct drm_pending_vblank_event *event;
>>>
>>> +       bool psr_enabled;
>>> +       struct delayed_work psr_work;
>>> +
>>>          struct completion line_flag_completion;
>>>
>>>          const struct vop_data *data;
>>> @@ -911,6 +915,16 @@ static const struct drm_plane_funcs vop_plane_funcs = {
>>>          .atomic_destroy_state = vop_atomic_plane_destroy_state,
>>>   };
>>>
>>> +static void vop_psr_work(struct work_struct *work)
>>> +{
>>> +       struct vop *vop = container_of(work, typeof(*vop), psr_work.work);
>>> +
>>> +       if (vop->psr_enabled)
>>> +               rockchip_drm_psr_enable(&vop->crtc);
>>> +       else
>>> +               rockchip_drm_psr_disable(&vop->crtc);
>>> +}
>>> +
>>>   static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>>>   {
>>>          struct vop *vop = to_vop(crtc);
>>> @@ -925,6 +939,9 @@ static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
>>>
>>>          spin_unlock_irqrestore(&vop->irq_lock, flags);
>>>
>>> +       vop->psr_enabled = false;
>>> +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
>>> +
>>>          return 0;
>>>   }
>>>
>>> @@ -941,6 +958,9 @@ static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
>>>          VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 0);
>>>
>>>          spin_unlock_irqrestore(&vop->irq_lock, flags);
>>> +
>>> +       vop->psr_enabled = true;
>>> +       schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
>>>   }
>>>
>>>   static void vop_crtc_wait_for_update(struct drm_crtc *crtc)
>>> @@ -1582,6 +1602,10 @@ static int vop_bind(struct device *dev, struct device *master, void *data)
>>>                  return ret;
>>>
>>>          pm_runtime_enable(&pdev->dev);
>>> +
>>> +       vop->psr_enabled = false;
>>> +       INIT_DELAYED_WORK(&vop->psr_work, vop_psr_work);
>>> +
>>>          return 0;
>>>   }
>>>
>>> --
>>> 1.9.1
>>>
>>>

Patch
diff mbox

diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile
index 05d0713..9746365 100644
--- a/drivers/gpu/drm/rockchip/Makefile
+++ b/drivers/gpu/drm/rockchip/Makefile
@@ -3,7 +3,7 @@ 
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
 rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \
-		rockchip_drm_gem.o rockchip_drm_vop.o
+		rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o
 rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o
 
 obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 20f12bc..0fec18f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -21,6 +21,7 @@ 
 
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_gem.h"
+#include "rockchip_drm_psr.h"
 
 #define to_rockchip_fb(x) container_of(x, struct rockchip_drm_fb, fb)
 
@@ -66,9 +67,20 @@  static int rockchip_drm_fb_create_handle(struct drm_framebuffer *fb,
 				     rockchip_fb->obj[0], handle);
 }
 
+static int rockchip_drm_fb_dirty(struct drm_framebuffer *fb,
+				 struct drm_file *file,
+				 unsigned int flags, unsigned int color,
+				 struct drm_clip_rect *clips,
+				 unsigned int num_clips)
+{
+	rockchip_drm_psr_flush();
+	return 0;
+}
+
 static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.destroy	= rockchip_drm_fb_destroy,
 	.create_handle	= rockchip_drm_fb_create_handle,
+	.dirty		= rockchip_drm_fb_dirty,
 };
 
 static struct rockchip_drm_fb *
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.c b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
new file mode 100644
index 0000000..c044443
--- /dev/null
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
@@ -0,0 +1,200 @@ 
+#include <drm/drm_crtc_helper.h>
+
+#include "rockchip_drm_psr.h"
+
+#define PSR_FLUSH_TIMEOUT	msecs_to_jiffies(3000) /* 3 seconds */
+
+static LIST_HEAD(psr_list);
+static DEFINE_MUTEX(psr_list_mutex);
+
+enum psr_state {
+	PSR_FLUSH,
+	PSR_ENABLE,
+	PSR_DISABLE,
+};
+
+struct psr_drv {
+	struct list_head list;
+	enum psr_state state;
+	struct mutex state_mutex;
+
+	struct timer_list flush_timer;
+
+	struct drm_encoder *encoder;
+	int (*set)(struct drm_encoder *encoder, bool enable);
+};
+
+static struct psr_drv *find_psr_by_crtc(struct drm_crtc *crtc)
+{
+	struct psr_drv *psr;
+
+	mutex_lock(&psr_list_mutex);
+	list_for_each_entry(psr, &psr_list, list) {
+		if (psr->encoder->crtc == crtc) {
+			mutex_unlock(&psr_list_mutex);
+			return psr;
+		}
+	}
+	mutex_unlock(&psr_list_mutex);
+
+	return ERR_PTR(-ENODEV);
+}
+
+static void psr_enable(struct psr_drv *psr)
+{
+	if (psr->state == PSR_ENABLE)
+		return;
+
+	mutex_lock(&psr->state_mutex);
+	psr->state = PSR_ENABLE;
+	psr->set(psr->encoder, true);
+	mutex_unlock(&psr->state_mutex);
+}
+
+static void psr_disable(struct psr_drv *psr)
+{
+	if (psr->state == PSR_DISABLE)
+		return;
+
+	mutex_lock(&psr->state_mutex);
+	psr->state = PSR_DISABLE;
+	psr->set(psr->encoder, false);
+	mutex_unlock(&psr->state_mutex);
+}
+
+static void psr_flush_handler(unsigned long data)
+{
+	struct psr_drv *psr = (struct psr_drv *)data;
+
+	if (!psr || psr->state != PSR_FLUSH)
+		return;
+
+	psr_enable(psr);
+}
+
+/**
+ * rockchip_drm_psr_enable - enable the encoder PSR which bind to given CRTC
+ * @crtc: CRTC to obtain the PSR encoder
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int rockchip_drm_psr_enable(struct drm_crtc *crtc)
+{
+	struct psr_drv *psr = find_psr_by_crtc(crtc);
+
+	if (IS_ERR(psr))
+		return PTR_ERR(psr);
+
+	psr_enable(psr);
+	return 0;
+}
+EXPORT_SYMBOL(rockchip_drm_psr_enable);
+
+/**
+ * rockchip_drm_psr_disable - disable the encoder PSR which bind to given CRTC
+ * @crtc: CRTC to obtain the PSR encoder
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int rockchip_drm_psr_disable(struct drm_crtc *crtc)
+{
+	struct psr_drv *psr = find_psr_by_crtc(crtc);
+
+	if (IS_ERR(psr))
+		return PTR_ERR(psr);
+
+	psr_disable(psr);
+	return 0;
+}
+EXPORT_SYMBOL(rockchip_drm_psr_disable);
+
+/**
+ * rockchip_drm_psr_flush - force to flush all registered PSR encoders
+ *
+ * Disable the PSR function for all registered encoders, and then enable the
+ * PSR function back after 5 second. If encoder PSR state have been changed
+ * during flush time, then keep the state no change after flush timeout.
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+void rockchip_drm_psr_flush(void)
+{
+	struct psr_drv *psr;
+
+	mutex_lock(&psr_list_mutex);
+	list_for_each_entry(psr, &psr_list, list) {
+		if (psr->state == PSR_DISABLE)
+			continue;
+
+		mod_timer(&psr->flush_timer,
+			  round_jiffies_up(jiffies + PSR_FLUSH_TIMEOUT));
+
+		psr_disable(psr);
+		psr->state = PSR_FLUSH;
+	}
+	mutex_unlock(&psr_list_mutex);
+}
+EXPORT_SYMBOL(rockchip_drm_psr_flush);
+
+/**
+ * rockchip_drm_psr_register - register encoder to psr driver
+ * @encoder: encoder that obtain the PSR function
+ * @psr_set: call back to set PSR state
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+int rockchip_drm_psr_register(struct drm_encoder *encoder,
+			      int (*psr_set)(struct drm_encoder *, bool enable))
+{
+	struct psr_drv *psr;
+
+	if (!encoder || !psr_set)
+		return -EINVAL;
+
+	psr = kzalloc(sizeof(struct psr_drv), GFP_KERNEL);
+	if (!psr)
+		return -ENOMEM;
+
+	setup_timer(&psr->flush_timer, psr_flush_handler, (unsigned long)psr);
+
+	mutex_init(&psr->state_mutex);
+
+	psr->state = PSR_DISABLE;
+	psr->encoder = encoder;
+	psr->set = psr_set;
+
+	mutex_lock(&psr_list_mutex);
+	list_add_tail(&psr->list, &psr_list);
+	mutex_unlock(&psr_list_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL(rockchip_drm_psr_register);
+
+/**
+ * rockchip_drm_psr_unregister - unregister encoder to psr driver
+ * @encoder: encoder that obtain the PSR function
+ * @psr_set: call back to set PSR state
+ *
+ * Returns:
+ * Zero on success, negative errno on failure.
+ */
+void rockchip_drm_psr_unregister(struct drm_encoder *encoder)
+{
+	struct psr_drv *psr;
+
+	mutex_lock(&psr_list_mutex);
+	list_for_each_entry(psr, &psr_list, list) {
+		if (psr->encoder == encoder) {
+			del_timer(&psr->flush_timer);
+			list_del(&psr->list);
+			kfree(psr);
+		}
+	}
+	mutex_unlock(&psr_list_mutex);
+}
+EXPORT_SYMBOL(rockchip_drm_psr_unregister);
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_psr.h b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
new file mode 100644
index 0000000..622f605
--- /dev/null
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.h
@@ -0,0 +1,12 @@ 
+#ifndef __ROCKCHIP_DRM_PSR___
+#define __ROCKCHIP_DRM_PSR___
+
+void rockchip_drm_psr_flush(void);
+int rockchip_drm_psr_enable(struct drm_crtc *crtc);
+int rockchip_drm_psr_disable(struct drm_crtc *crtc);
+
+int rockchip_drm_psr_register(struct drm_encoder *encoder,
+			    int (*psr_set)(struct drm_encoder *, bool enable));
+void rockchip_drm_psr_unregister(struct drm_encoder *encoder);
+
+#endif /* __ROCKCHIP_DRM_PSR__ */
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index cd3cac5..3c6dfc5 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -34,6 +34,7 @@ 
 #include "rockchip_drm_drv.h"
 #include "rockchip_drm_gem.h"
 #include "rockchip_drm_fb.h"
+#include "rockchip_drm_psr.h"
 #include "rockchip_drm_vop.h"
 
 #define __REG_SET_RELAXED(x, off, mask, shift, v, write_mask) \
@@ -121,6 +122,9 @@  struct vop {
 	/* protected by dev->event_lock */
 	struct drm_pending_vblank_event *event;
 
+	bool psr_enabled;
+	struct delayed_work psr_work;
+
 	struct completion line_flag_completion;
 
 	const struct vop_data *data;
@@ -911,6 +915,16 @@  static const struct drm_plane_funcs vop_plane_funcs = {
 	.atomic_destroy_state = vop_atomic_plane_destroy_state,
 };
 
+static void vop_psr_work(struct work_struct *work)
+{
+	struct vop *vop = container_of(work, typeof(*vop), psr_work.work);
+
+	if (vop->psr_enabled)
+		rockchip_drm_psr_enable(&vop->crtc);
+	else
+		rockchip_drm_psr_disable(&vop->crtc);
+}
+
 static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
@@ -925,6 +939,9 @@  static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
 
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
 
+	vop->psr_enabled = false;
+	schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
+
 	return 0;
 }
 
@@ -941,6 +958,9 @@  static void vop_crtc_disable_vblank(struct drm_crtc *crtc)
 	VOP_INTR_SET_TYPE(vop, enable, FS_INTR, 0);
 
 	spin_unlock_irqrestore(&vop->irq_lock, flags);
+
+	vop->psr_enabled = true;
+	schedule_delayed_work(&vop->psr_work, msecs_to_jiffies(10));
 }
 
 static void vop_crtc_wait_for_update(struct drm_crtc *crtc)
@@ -1582,6 +1602,10 @@  static int vop_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 
 	pm_runtime_enable(&pdev->dev);
+
+	vop->psr_enabled = false;
+	INIT_DELAYED_WORK(&vop->psr_work, vop_psr_work);
+
 	return 0;
 }