Re: [PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
---
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




_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux