Re: [PATCH] drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled
On 11/03/17 19:25, Jyri Sarha wrote: > Touching HW while clocks are off is a serious error and for instance > breaks suspend functionality. After this patch tilcdc_crtc_update_fb() > always updates the primary plane's framebuffer pointer, increases fb's > reference count and stores vblank event. tilcdc_crtc_update_fb() only > writes the fb's DMA address to HW if the crtc is enabled, as > tilcdc_crtc_enable() takes care of writing the address on enable. > > This patch also refactors the tilcdc_crtc_update_fb() a bit. Number of > subsequent small changes to had made it almost unreadable. There Something is missing from the line above... > should be no other functional changes but checking the CRTC's enable > state. However, the locking goes a bit differently and some of the > redundant checks have been removed in this new version. > > The enable_lock should be enough to protect the access to > tilcdc_crtc->enabled. The irq_lock protects the access to last_vblank > and next_fb. The check for vrefresh and last_vblank being valid is > redundant, as the vrefresh should be always valid if the CRTC is > enabled and now last_vblank should be too, because it is initialized > to current time when CRTC raster is enabled. If for some reason the > values are not corretly initialized the division by zero waning is 2 typoes above. > quite appropriate. > > Signed-off-by: Jyri Sarha > --- > drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 37 > +++- > 1 file changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index f80bf93..bd92c89 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -1,4 +1,4 @@ > -/* > +* This breaks the compilation of the whole driver... Tomi signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled
On 03/11/17 19:25, Jyri Sarha wrote: > diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > index f80bf93..bd92c89 100644 > --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c > @@ -1,4 +1,4 @@ > -/* > +* > * Copyright (C) 2012 Texas Instruments > * Author: Rob Clark > * > @@ -464,6 +464,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc) Sorry, I have no idea how this little change slipped into the patch. Anyway, the rest of the patch is still good so I won't send a new version just because of that. Best regards, Jyri ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled
Hi Jyri,
[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Jyri-Sarha/drm-tilcdc-Set-framebuffer-DMA-address-to-HW-only-if-CRTC-is-enabled/20170312-201749
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm
All error/warnings (new ones prefixed by >>):
^~~
include/linux/string.h:42:12: error: storage class specified for parameter
'strcmp'
extern int strcmp(const char *,const char *);
^~
include/linux/string.h:45:46: error: expected declaration specifiers or
'...' before '__kernel_size_t'
extern int strncmp(const char *,const char *,__kernel_size_t);
^~~
include/linux/string.h:48:12: error: storage class specified for parameter
'strcasecmp'
extern int strcasecmp(const char *s1, const char *s2);
^~
include/linux/string.h:51:56: error: unknown type name 'size_t'
extern int strncasecmp(const char *s1, const char *s2, size_t n);
^~
include/linux/string.h:57:15: error: storage class specified for parameter
'strchrnul'
extern char * strchrnul(const char *,int);
^
include/linux/string.h:60:37: error: unknown type name 'size_t'
extern char * strnchr(const char *, size_t, int);
^~
include/linux/string.h:65:28: error: storage class specified for parameter
'skip_spaces'
extern char * __must_check skip_spaces(const char *);
^~~
include/linux/string.h:67:14: error: storage class specified for parameter
'strim'
extern char *strim(char *);
^
include/linux/string.h:70:1: error: expected '=', ',', ';', 'asm' or
'__attribute__' before '{' token
{
^
include/linux/string.h:75:15: error: storage class specified for parameter
'strstr'
extern char * strstr(const char *, const char *);
^~
include/linux/string.h:78:51: error: unknown type name 'size_t'
extern char * strnstr(const char *, const char *, size_t);
^~
include/linux/string.h:81:24: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'strlen'
extern __kernel_size_t strlen(const char *);
^~
include/linux/string.h:84:24: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'strnlen'
extern __kernel_size_t strnlen(const char *,__kernel_size_t);
^~~
include/linux/string.h:87:15: error: storage class specified for parameter
'strpbrk'
extern char * strpbrk(const char *,const char *);
^~~
include/linux/string.h:90:15: error: storage class specified for parameter
'strsep'
extern char * strsep(char **,const char *);
^~
include/linux/string.h:93:24: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'strspn'
extern __kernel_size_t strspn(const char *,const char *);
^~
include/linux/string.h:96:24: error: expected '=', ',', ';', 'asm' or
'__attribute__' before 'strcspn'
extern __kernel_size_t strcspn(const char *,const char *);
^~~
include/linux/string.h:109:34: error: expected declaration specifiers or
'...' before '__kernel_size_t'
extern void * memscan(void *,int,__kernel_size_t);
^~~
include/linux/string.h:112:45: error: expected declaration specifiers or
'...' before '__kernel_size_t'
extern int memcmp(const void *,const void *,__kernel_size_t);
^~~
include/linux/string.h:117:40: error: unknown type name 'size_t'
void *memchr_inv(const void *s, int c, size_t n);
^~
include/linux/string.h:120:13: error: storage class specified for parameter
'kfree_const'
extern void kfree_const(const void *x);
^~~
include/linux/string.h:122:37: error: expected declaration specifiers or
'...' before 'gfp_t'
extern char *kstrdup(const char *s, gfp_t gfp) __malloc;
^
include/linux/string.h:123:49: error: expected declaration specifi
Re: [PATCH] drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled
Hi Jyri, [auto build test ERROR on drm/drm-next] [also build test ERROR on v4.11-rc1 next-20170310] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Jyri-Sarha/drm-tilcdc-Set-framebuffer-DMA-address-to-HW-only-if-CRTC-is-enabled/20170312-201749 base: git://people.freedesktop.org/~airlied/linux.git drm-next config: arm-davinci_all_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=arm All error/warnings (new ones prefixed by >>): drivers/gpu/drm/tilcdc/tilcdc_crtc.c:2:4: error: return type defaults to 'int' [-Werror=return-type] * Copyright (C) 2012 Texas Instruments ^ >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c:2:4: error: function declaration isn't >> a prototype [-Werror=strict-prototypes] drivers/gpu/drm/tilcdc/tilcdc_crtc.c: In function 'Copyright': >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c:2:18: error: expected declaration >> specifiers before numeric constant * Copyright (C) 2012 Texas Instruments ^~~~ >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c:3:32: error: stray '@' in program * Author: Rob Clark ^ >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c:5:35: error: unknown type name 'you' * This program is free software; you can redistribute it and/or modify it ^~~ >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c:5:43: error: expected '=', ',', ';', >> 'asm' or '__attribute__' before 'redistribute' * This program is free software; you can redistribute it and/or modify it ^~~~ >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c:10:18: error: unknown type name >> 'without' * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or ^~~ >> drivers/gpu/drm/tilcdc/tilcdc_crtc.c:10:31: error: expected '=', ',', ';', >> 'asm' or '__attribute__' before 'the' * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or ^~~ In file included from include/asm-generic/int-ll64.h:10:0, from arch/arm/include/uapi/asm/types.h:4, from include/uapi/linux/types.h:4, from include/linux/types.h:5, from include/linux/mod_devicetable.h:11, from include/linux/i2c.h:29, from include/drm/drm_crtc.h:28, from include/drm/drm_atomic.h:31, from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:18: >> include/uapi/asm-generic/int-ll64.h:20:23: error: storage class specified >> for parameter '__u8' typedef unsigned char __u8; ^~~~ >> include/uapi/asm-generic/int-ll64.h:22:26: error: storage class specified >> for parameter '__s16' typedef __signed__ short __s16; ^ >> include/uapi/asm-generic/int-ll64.h:23:24: error: storage class specified >> for parameter '__u16' typedef unsigned short __u16; ^ >> include/uapi/asm-generic/int-ll64.h:25:24: error: storage class specified >> for parameter '__s32' typedef __signed__ int __s32; ^ >> include/uapi/asm-generic/int-ll64.h:26:22: error: storage class specified >> for parameter '__u32' typedef unsigned int __u32; ^ >> include/uapi/asm-generic/int-ll64.h:29:1: error: expected declaration >> specifiers before '__extension__' __extension__ typedef __signed__ long long __s64; ^ include/uapi/asm-generic/int-ll64.h:30:1: error: expected declaration specifiers before '__extension__' __extension__ typedef unsigned long long __u64; ^ In file included from arch/arm/include/uapi/asm/types.h:4:0, from include/uapi/linux/types.h:4, from include/linux/types.h:5, from include/linux/mod_devicetable.h:11, from include/linux/i2c.h:29, from include/drm/drm_crtc.h:28, from include/drm/drm_atomic.h:31, from drivers/gpu/drm/tilcdc/tilcdc_crtc.c:18: >> include/asm-generic/int-ll64.h:15:21: error: storage class specified for >> parameter 's8' typedef signed char s8; ^~ >> include/asm-generic/int-ll64.h:16:23: error: storage class specified for >> parameter 'u8' typedef unsigned char u8; ^~ >> include/asm-generic/int-ll64.h:18:22: error: storage class specified for >> parameter 's16' typedef signed short s16;
[PATCH] drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled
Touching HW while clocks are off is a serious error and for instance
breaks suspend functionality. After this patch tilcdc_crtc_update_fb()
always updates the primary plane's framebuffer pointer, increases fb's
reference count and stores vblank event. tilcdc_crtc_update_fb() only
writes the fb's DMA address to HW if the crtc is enabled, as
tilcdc_crtc_enable() takes care of writing the address on enable.
This patch also refactors the tilcdc_crtc_update_fb() a bit. Number of
subsequent small changes to had made it almost unreadable. There
should be no other functional changes but checking the CRTC's enable
state. However, the locking goes a bit differently and some of the
redundant checks have been removed in this new version.
The enable_lock should be enough to protect the access to
tilcdc_crtc->enabled. The irq_lock protects the access to last_vblank
and next_fb. The check for vrefresh and last_vblank being valid is
redundant, as the vrefresh should be always valid if the CRTC is
enabled and now last_vblank should be too, because it is initialized
to current time when CRTC raster is enabled. If for some reason the
values are not corretly initialized the division by zero waning is
quite appropriate.
Signed-off-by: Jyri Sarha
---
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 37 +++-
1 file changed, 24 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index f80bf93..bd92c89 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -1,4 +1,4 @@
-/*
+*
* Copyright (C) 2012 Texas Instruments
* Author: Rob Clark
*
@@ -464,6 +464,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+ unsigned long flags;
WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
mutex_lock(&tilcdc_crtc->enable_lock);
@@ -484,7 +485,17 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG,
LCDC_PALETTE_LOAD_MODE(DATA_ONLY),
LCDC_PALETTE_LOAD_MODE_MASK);
+
+ /* There is no real chance for a race here as the time stamp
+* is taken before the raster DMA is started. The spin-lock is
+* taken to have a memory barrier after taking the time-stamp
+* and to avoid a context switch between taking the stamp and
+* enabling the raster.
+*/
+ spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
+ tilcdc_crtc->last_vblank = ktime_get();
tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+ spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
drm_crtc_vblank_on(crtc);
@@ -539,7 +550,6 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool
shutdown)
}
drm_flip_work_commit(&tilcdc_crtc->unref_work, priv->wq);
- tilcdc_crtc->last_vblank = 0;
tilcdc_crtc->enabled = false;
mutex_unlock(&tilcdc_crtc->enable_lock);
@@ -602,7 +612,6 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
{
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
struct drm_device *dev = crtc->dev;
- unsigned long flags;
WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
@@ -614,28 +623,30 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
drm_framebuffer_reference(fb);
crtc->primary->fb = fb;
+ tilcdc_crtc->event = event;
- spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
+ mutex_lock(&tilcdc_crtc->enable_lock);
- if (crtc->hwmode.vrefresh && ktime_to_ns(tilcdc_crtc->last_vblank)) {
+ if (tilcdc_crtc->enabled) {
+ unsigned long flags;
ktime_t next_vblank;
s64 tdiff;
+ spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
+
next_vblank = ktime_add_us(tilcdc_crtc->last_vblank,
- 100 / crtc->hwmode.vrefresh);
-
+ 100 / crtc->hwmode.vrefresh);
tdiff = ktime_to_us(ktime_sub(next_vblank, ktime_get()));
if (tdiff < TILCDC_VBLANK_SAFETY_THRESHOLD_US)
tilcdc_crtc->next_fb = fb;
+ else
+ set_scanout(crtc, fb);
+
+ spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
}
- if (tilcdc_crtc->next_fb != fb)
- set_scanout(crtc, fb);
-
- tilcdc_crtc->event = event;
-
- spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
+ mutex_unlock(&tilcdc_crtc->enable_lock);
return 0;
}
--
1.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled
Touching HW while clocks are of is a serious error and for instance
breaks suspend functionality. After the patch tilcdc_crtc_update_fb()
always updates the primary planes framebuffer pointer, increases fb's
reference count and stores vblank event. The framebuffer's DMA address
is written to HW only if the CRTC is enabled at the moment. The
primary plane's DMA address is written to HW when the CRTC is enabled.
This patch also refactors the tilcdc_crtc_update_fb() a bit. Number of
subsequent small changes to had made it almost unreadable. There
should be no other functional changes but checking the CRTC's enable
state. However, the locking goes a bit differently and some of the
redundant checks have been removed in this new version.
The enable_lock should be enough to protect the access to
tilcdc_crtc->enabled. The irq_lock protects the access to last_vblank
and next_fb. The check for vrefresh and last_vblank being valid is
redundant, as they should now always be valid if the crtc is
enabled. If they are not a division by zero waning is quite
appropriate.
Signed-off-by: Jyri Sarha
---
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 27 ---
1 file changed, 16 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index f80bf93..26cb509 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -464,6 +464,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
{
struct drm_device *dev = crtc->dev;
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+ unsigned long flags;
WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
mutex_lock(&tilcdc_crtc->enable_lock);
@@ -484,7 +485,11 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG,
LCDC_PALETTE_LOAD_MODE(DATA_ONLY),
LCDC_PALETTE_LOAD_MODE_MASK);
+
+ spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+ tilcdc_crtc->last_vblank = ktime_get();
+ spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
drm_crtc_vblank_on(crtc);
@@ -539,7 +544,6 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool
shutdown)
}
drm_flip_work_commit(&tilcdc_crtc->unref_work, priv->wq);
- tilcdc_crtc->last_vblank = 0;
tilcdc_crtc->enabled = false;
mutex_unlock(&tilcdc_crtc->enable_lock);
@@ -602,7 +606,6 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
{
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
struct drm_device *dev = crtc->dev;
- unsigned long flags;
WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
@@ -615,27 +618,29 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
crtc->primary->fb = fb;
- spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
+ mutex_lock(&tilcdc_crtc->enable_lock);
- if (crtc->hwmode.vrefresh && ktime_to_ns(tilcdc_crtc->last_vblank)) {
+ if (tilcdc_crtc->enabled) {
+ unsigned long flags;
ktime_t next_vblank;
s64 tdiff;
+ spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
+
next_vblank = ktime_add_us(tilcdc_crtc->last_vblank,
- 100 / crtc->hwmode.vrefresh);
-
+ 100 / crtc->hwmode.vrefresh);
tdiff = ktime_to_us(ktime_sub(next_vblank, ktime_get()));
if (tdiff < TILCDC_VBLANK_SAFETY_THRESHOLD_US)
tilcdc_crtc->next_fb = fb;
+ else
+ set_scanout(crtc, fb);
+
+ spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
}
-
- if (tilcdc_crtc->next_fb != fb)
- set_scanout(crtc, fb);
-
tilcdc_crtc->event = event;
- spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
+ mutex_unlock(&tilcdc_crtc->enable_lock);
return 0;
}
--
1.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
