[PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver
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 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
>>> ---
>>> 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 000..c03
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>>> @@ -0,0 +1,200 @@
>>> +#include
>>> +
>>> +#include "rockchip_drm_psr.h"
>>> +
>>>
[PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver
On Tue, Jul 12, 2016 at 9: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 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
[PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver
On Wed, Jul 13, 2016 at 12:10:13AM +0900, Tomasz Figa wrote: > On Tue, Jul 12, 2016 at 9: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 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver
On Fri, Jul 01, 2016 at 02:00:00PM -0400, Sean Paul wrote:
> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang 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
> > ---
> > 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 000..c03
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> > @@ -0,0 +1,200 @@
> > +#include
> > +
> > +#include "rockchip_drm_psr.h"
> > +
> > +#define PSR_FLUSH_TIMEOUT msecs_to_jiffies(3000) /* 3 seconds */
> > +
> > +static LIST_HEAD(psr_list);
> > +static
[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 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
>> ---
>> 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 000..c03
>> --- /dev/null
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
>> @@ -0,0 +1,200 @@
>> +#include
>> +
>> +#include "rockchip_drm_psr.h"
>> +
>> +#define PSR_FLUSH_TIMEOUT msecs_to_jiffies(3000) /* 3 seconds */
>> +
>> +static LIST_HEAD(psr_list);
>> +static
[PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver
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
---
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 000..c03
--- /dev/null
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
@@ -0,0 +1,200 @@
+#include
+
+#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(_list_mutex);
+ list_for_each_entry(psr, _list, list) {
+ if (psr->encoder->crtc == crtc) {
+ mutex_unlock(_list_mutex);
+ return psr;
+ }
+ }
+ mutex_unlock(_list_mutex);
+
+ return ERR_PTR(-ENODEV);
+}
+
+static void psr_enable(struct psr_drv *psr)
+{
+ if (psr->state == PSR_ENABLE)
+ return;
+
+ mutex_lock(>state_mutex);
+ psr->state = PSR_ENABLE;
+ psr->set(psr->encoder, true);
+ mutex_unlock(>state_mutex);
+}
+
+static void psr_disable(struct psr_drv *psr)
+{
+ if (psr->state == PSR_DISABLE)
+ return;
+
+ mutex_lock(>state_mutex);
+ psr->state = PSR_DISABLE;
+ psr->set(psr->encoder, false);
+
[PATCH v3 2/4] drm/rockchip: add an common abstracted PSR driver
On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang 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
> ---
> 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 000..c03
> --- /dev/null
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_psr.c
> @@ -0,0 +1,200 @@
> +#include
> +
> +#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
