| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <577F0C12.60208@rock-chips.com>
Date: Fri, 8 Jul 2016 10:12:34 +0800
From: Yakir Yang <ykk@...k-chips.com>
To: Sean Paul <seanpaul@...omium.org>
Cc: Mark Yao <yzq@...k-chips.com>, Inki Dae <inki.dae@...sung.com>,
Jingoo Han <jingoohan1@...il.com>,
Heiko Stuebner <heiko@...ech.de>,
Daniel Vetter <daniel.vetter@...ll.ch>,
Stéphane Marchesin <marcheu@...omium.org>,
Tomasz Figa <tfiga@...omium.org>,
Doug Anderson <dianders@...omium.org>,
Thierry Reding <treding@...dia.com>,
Krzysztof Kozlowski <k.kozlowski@...sung.com>,
Javier Martinez Canillas <javier@....samsung.com>,
David Airlie <airlied@...ux.ie>,
Emil Velikov <emil.l.velikov@...il.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
dri-devel <dri-devel@...ts.freedesktop.org>,
linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver
Sean,
On 07/02/2016 02:00 AM, Sean Paul wrote:
> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@...k-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@...k-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
>>
>>
>
>
Powered by blists - more mailing lists