These are based over Tomi's 3.15/omapdrm-fixes branch.
Archit Taneja (5): drm/omap: gem sync: wait on correct events drm/omap: Fix crash when using LCD3 overlay manager drm/omap: Allow allocation of larger buffers drm/omap: Use old_fb to synchronize between successive page flips drm/omap: protect omap_crtc's event with event_lock spinlock
Subhajit Paul (1): drm/omap: Fix memory leak in omap_gem_op_async
drivers/gpu/drm/omapdrm/omap_crtc.c | 7 ++++++- drivers/gpu/drm/omapdrm/omap_drv.c | 10 ++++++---- drivers/gpu/drm/omapdrm/omap_gem.c | 6 ++++-- drivers/video/omap2/dss/dispc.c | 12 ++++++++++++ include/video/omapdss.h | 2 ++ 5 files changed, 30 insertions(+), 7 deletions(-)
From: Subhajit Paul subhajit_paul@ti.com
In omap_gem_op_async(), if a waiter is not added to the wait list, it needs to be free'd in the function itself.
Make sure we free the waiter for this case.
Signed-off-by: Subhajit Paul subhajit_paul@ti.com Signed-off-by: Archit Taneja archit@ti.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 5aec3e8..3199e3f 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1229,6 +1229,8 @@ int omap_gem_op_async(struct drm_gem_object *obj, enum omap_gem_op op, }
spin_unlock(&sync_lock); + + kfree(waiter); }
/* no waiting.. */
A waiter of the type OMAP_GEM_READ should wait for a buffer to be completely written, and only then proceed with reading it. A similar logic applies for waiters with OMAP_GEM_WRITE flag.
Currently the function is_waiting() waits on the read_complete/read_target counts in the sync object.
This should be the other way round, as a reader should wait for users who are 'writing' to this buffer, and vice versa.
Make readers of the buffer(OMAP_GEM_READ) wait on the write counters, and writers to the buffer(OMAP_GEM_WRITE) wait on the read counters in is_waiting()
Signed-off-by: Archit Taneja archit@ti.com --- drivers/gpu/drm/omapdrm/omap_gem.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 3199e3f..002b89d7 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -1050,10 +1050,10 @@ static inline bool is_waiting(struct omap_gem_sync_waiter *waiter) { struct omap_gem_object *omap_obj = waiter->omap_obj; if ((waiter->op & OMAP_GEM_READ) && - (omap_obj->sync->read_complete < waiter->read_target)) + (omap_obj->sync->write_complete < waiter->write_target)) return true; if ((waiter->op & OMAP_GEM_WRITE) && - (omap_obj->sync->write_complete < waiter->write_target)) + (omap_obj->sync->read_complete < waiter->read_target)) return true; return false; }
The channel_names list didn't have a string populated for LCD3 manager, this results in a crash when the display's output is connected to LCD3. Add an entry for LCD3.
Reported-by: Somnath Mukherjee somnath@ti.com Signed-off-by: Archit Taneja archit@ti.com --- drivers/gpu/drm/omapdrm/omap_crtc.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 90916ab..62f28df 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -654,6 +654,7 @@ static const char *channel_names[] = { [OMAP_DSS_CHANNEL_LCD] = "lcd", [OMAP_DSS_CHANNEL_DIGIT] = "tv", [OMAP_DSS_CHANNEL_LCD2] = "lcd2", + [OMAP_DSS_CHANNEL_LCD3] = "lcd3", };
void omap_crtc_pre_init(void)
The drm ioctl DRM_IOCTL_MODE_ADDFB2 doesn't let us allocate buffers which are greater than what is specified in the driver through dev->mode_config.
Create helpers for DISPC which return the max manager width and height supported by the device. The maximum width for a framebuffer is set to the combined width of the all the crtcs, assuming they are arranged horizontally.
Signed-off-by: Archit Taneja archit@ti.com --- drivers/gpu/drm/omapdrm/omap_drv.c | 10 ++++++---- drivers/video/omap2/dss/dispc.c | 12 ++++++++++++ include/video/omapdss.h | 2 ++ 3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index c8270e4..55ec575 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -306,11 +306,13 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.min_width = 32; dev->mode_config.min_height = 32;
- /* note: eventually will need some cpu_is_omapXYZ() type stuff here - * to fill in these limits properly on different OMAP generations.. + /* + * Note: the maximum width is set to the combined width of all the + * crtcs. We could assume the same for the maximum height too, but + * we generally don't use such a configuration. */ - dev->mode_config.max_width = 2048; - dev->mode_config.max_height = 2048; + dev->mode_config.max_width = num_crtcs * dispc_mgr_max_width(); + dev->mode_config.max_height = dispc_mgr_max_height();
dev->mode_config.funcs = &omap_mode_config_funcs;
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c index 77d6221..47f9829 100644 --- a/drivers/video/omap2/dss/dispc.c +++ b/drivers/video/omap2/dss/dispc.c @@ -2999,6 +2999,18 @@ void dispc_mgr_set_timings(enum omap_channel channel, } EXPORT_SYMBOL(dispc_mgr_set_timings);
+u16 dispc_mgr_max_width(void) +{ + return dispc.feat->mgr_width_max; +} +EXPORT_SYMBOL(dispc_mgr_max_width); + +u16 dispc_mgr_max_height(void) +{ + return dispc.feat->mgr_height_max; +} +EXPORT_SYMBOL(dispc_mgr_max_height); + static void dispc_mgr_set_lcd_divisor(enum omap_channel channel, u16 lck_div, u16 pck_div) { diff --git a/include/video/omapdss.h b/include/video/omapdss.h index 3d7c51a..53637ac 100644 --- a/include/video/omapdss.h +++ b/include/video/omapdss.h @@ -949,6 +949,8 @@ void dispc_mgr_set_lcd_config(enum omap_channel channel, const struct dss_lcd_mgr_config *config); void dispc_mgr_set_timings(enum omap_channel channel, const struct omap_video_timings *timings); +u16 dispc_mgr_max_width(void); +u16 dispc_mgr_max_height(void); void dispc_mgr_setup(enum omap_channel channel, const struct omap_overlay_manager_info *info);
On 11/04/14 10:23, Archit Taneja wrote:
The drm ioctl DRM_IOCTL_MODE_ADDFB2 doesn't let us allocate buffers which are greater than what is specified in the driver through dev->mode_config.
Create helpers for DISPC which return the max manager width and height supported by the device. The maximum width for a framebuffer is set to the combined width of the all the crtcs, assuming they are arranged horizontally.
Signed-off-by: Archit Taneja archit@ti.com
drivers/gpu/drm/omapdrm/omap_drv.c | 10 ++++++---- drivers/video/omap2/dss/dispc.c | 12 ++++++++++++ include/video/omapdss.h | 2 ++ 3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index c8270e4..55ec575 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -306,11 +306,13 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.min_width = 32; dev->mode_config.min_height = 32;
- /* note: eventually will need some cpu_is_omapXYZ() type stuff here
* to fill in these limits properly on different OMAP generations..
- /*
* Note: the maximum width is set to the combined width of all the* crtcs. We could assume the same for the maximum height too, but* we generally don't use such a configuration.
- dev->mode_config.max_width = 2048;
- dev->mode_config.max_height = 2048;
- dev->mode_config.max_width = num_crtcs * dispc_mgr_max_width();
- dev->mode_config.max_height = dispc_mgr_max_height();
This looks very strange.
If the max size is supposed to be the maximum output size we support, then multiplying with num_crtcs doesn't make sense.
If, on the other hand, it tells the possible maximum size of the framebuffer in the memory, of which only small part is shown (where's the max size of that "part" defined, then?), then there should be no limits as the only limit is the size of the memory.
Tomi
On Monday 14 April 2014 02:55 PM, Tomi Valkeinen wrote:
On 11/04/14 10:23, Archit Taneja wrote:
The drm ioctl DRM_IOCTL_MODE_ADDFB2 doesn't let us allocate buffers which are greater than what is specified in the driver through dev->mode_config.
Create helpers for DISPC which return the max manager width and height supported by the device. The maximum width for a framebuffer is set to the combined width of the all the crtcs, assuming they are arranged horizontally.
Signed-off-by: Archit Taneja archit@ti.com
drivers/gpu/drm/omapdrm/omap_drv.c | 10 ++++++---- drivers/video/omap2/dss/dispc.c | 12 ++++++++++++ include/video/omapdss.h | 2 ++ 3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index c8270e4..55ec575 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -306,11 +306,13 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.min_width = 32; dev->mode_config.min_height = 32;
- /* note: eventually will need some cpu_is_omapXYZ() type stuff here
* to fill in these limits properly on different OMAP generations..
- /*
* Note: the maximum width is set to the combined width of all the* crtcs. We could assume the same for the maximum height too, but* we generally don't use such a configuration.
- dev->mode_config.max_width = 2048;
- dev->mode_config.max_height = 2048;
- dev->mode_config.max_width = num_crtcs * dispc_mgr_max_width();
- dev->mode_config.max_height = dispc_mgr_max_height();
This looks very strange.
If the max size is supposed to be the maximum output size we support, then multiplying with num_crtcs doesn't make sense.
If, on the other hand, it tells the possible maximum size of the framebuffer in the memory, of which only small part is shown (where's the max size of that "part" defined, then?), then there should be no limits as the only limit is the size of the memory.
These parameters are used to tell the max size of framebuffer we can allocate in memory.
I'm not sure why there is a limit in the first place, but if we have a really low minimum(like 2048 pixels right now), we can't have multiple displays showing different regions of the same buffer.
Archit
On 14/04/14 13:18, Archit Taneja wrote:
On Monday 14 April 2014 02:55 PM, Tomi Valkeinen wrote:
On 11/04/14 10:23, Archit Taneja wrote:
The drm ioctl DRM_IOCTL_MODE_ADDFB2 doesn't let us allocate buffers which are greater than what is specified in the driver through dev->mode_config.
Create helpers for DISPC which return the max manager width and height supported by the device. The maximum width for a framebuffer is set to the combined width of the all the crtcs, assuming they are arranged horizontally.
Signed-off-by: Archit Taneja archit@ti.com
drivers/gpu/drm/omapdrm/omap_drv.c | 10 ++++++---- drivers/video/omap2/dss/dispc.c | 12 ++++++++++++ include/video/omapdss.h | 2 ++ 3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index c8270e4..55ec575 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -306,11 +306,13 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.min_width = 32; dev->mode_config.min_height = 32;
- /* note: eventually will need some cpu_is_omapXYZ() type stuff here
* to fill in these limits properly on different OMAP generations..
- /*
* Note: the maximum width is set to the combined width of all the* crtcs. We could assume the same for the maximum height too, but* we generally don't use such a configuration. */
- dev->mode_config.max_width = 2048;
- dev->mode_config.max_height = 2048;
- dev->mode_config.max_width = num_crtcs * dispc_mgr_max_width();
- dev->mode_config.max_height = dispc_mgr_max_height();
This looks very strange.
If the max size is supposed to be the maximum output size we support, then multiplying with num_crtcs doesn't make sense.
If, on the other hand, it tells the possible maximum size of the framebuffer in the memory, of which only small part is shown (where's the max size of that "part" defined, then?), then there should be no limits as the only limit is the size of the memory.
These parameters are used to tell the max size of framebuffer we can allocate in memory.
I'm not sure why there is a limit in the first place, but if we have a really low minimum(like 2048 pixels right now), we can't have multiple displays showing different regions of the same buffer.
In that case shouldn't the limit be 0 (if that's allowed) or some surely high enough value, say, 32767. Why calculate it at all based on the dispc's maximum, if it has no relevance?
Tomi
On Monday 14 April 2014 04:00 PM, Tomi Valkeinen wrote:
On 14/04/14 13:18, Archit Taneja wrote:
On Monday 14 April 2014 02:55 PM, Tomi Valkeinen wrote:
On 11/04/14 10:23, Archit Taneja wrote:
The drm ioctl DRM_IOCTL_MODE_ADDFB2 doesn't let us allocate buffers which are greater than what is specified in the driver through dev->mode_config.
Create helpers for DISPC which return the max manager width and height supported by the device. The maximum width for a framebuffer is set to the combined width of the all the crtcs, assuming they are arranged horizontally.
Signed-off-by: Archit Taneja archit@ti.com
drivers/gpu/drm/omapdrm/omap_drv.c | 10 ++++++---- drivers/video/omap2/dss/dispc.c | 12 ++++++++++++ include/video/omapdss.h | 2 ++ 3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index c8270e4..55ec575 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -306,11 +306,13 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.min_width = 32; dev->mode_config.min_height = 32;
- /* note: eventually will need some cpu_is_omapXYZ() type stuff here
* to fill in these limits properly on different OMAP generations..
- /*
* Note: the maximum width is set to the combined width of all the* crtcs. We could assume the same for the maximum height too, but* we generally don't use such a configuration. */
- dev->mode_config.max_width = 2048;
- dev->mode_config.max_height = 2048;
- dev->mode_config.max_width = num_crtcs * dispc_mgr_max_width();
- dev->mode_config.max_height = dispc_mgr_max_height();
This looks very strange.
If the max size is supposed to be the maximum output size we support, then multiplying with num_crtcs doesn't make sense.
If, on the other hand, it tells the possible maximum size of the framebuffer in the memory, of which only small part is shown (where's the max size of that "part" defined, then?), then there should be no limits as the only limit is the size of the memory.
These parameters are used to tell the max size of framebuffer we can allocate in memory.
I'm not sure why there is a limit in the first place, but if we have a really low minimum(like 2048 pixels right now), we can't have multiple displays showing different regions of the same buffer.
In that case shouldn't the limit be 0 (if that's allowed) or some surely high enough value, say, 32767. Why calculate it at all based on the dispc's maximum, if it has no relevance?
I understand your point. When I wrote the patch, I thought more like: "I have 3 displays arranged horizontally, if I want a buffer large enough to fill the 3 displays, what should be it's width/height?"
Archit
On Fri, Apr 11, 2014 at 3:23 AM, Archit Taneja archit@ti.com wrote:
The drm ioctl DRM_IOCTL_MODE_ADDFB2 doesn't let us allocate buffers which are greater than what is specified in the driver through dev->mode_config.
Create helpers for DISPC which return the max manager width and height supported by the device. The maximum width for a framebuffer is set to the combined width of the all the crtcs, assuming they are arranged horizontally.
Signed-off-by: Archit Taneja archit@ti.com
drivers/gpu/drm/omapdrm/omap_drv.c | 10 ++++++---- drivers/video/omap2/dss/dispc.c | 12 ++++++++++++ include/video/omapdss.h | 2 ++ 3 files changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index c8270e4..55ec575 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -306,11 +306,13 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.min_width = 32; dev->mode_config.min_height = 32;
/* note: eventually will need some cpu_is_omapXYZ() type stuff here* to fill in these limits properly on different OMAP generations..
/** Note: the maximum width is set to the combined width of all the* crtcs. We could assume the same for the maximum height too, but* we generally don't use such a configuration. */
dev->mode_config.max_width = 2048;dev->mode_config.max_height = 2048;
dev->mode_config.max_width = num_crtcs * dispc_mgr_max_width();dev->mode_config.max_height = dispc_mgr_max_height();
the max_width should probably actually be the max_pitch..
BR, -R
dev->mode_config.funcs = &omap_mode_config_funcs;diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c index 77d6221..47f9829 100644 --- a/drivers/video/omap2/dss/dispc.c +++ b/drivers/video/omap2/dss/dispc.c @@ -2999,6 +2999,18 @@ void dispc_mgr_set_timings(enum omap_channel channel, } EXPORT_SYMBOL(dispc_mgr_set_timings);
+u16 dispc_mgr_max_width(void) +{
return dispc.feat->mgr_width_max;+} +EXPORT_SYMBOL(dispc_mgr_max_width);
+u16 dispc_mgr_max_height(void) +{
return dispc.feat->mgr_height_max;+} +EXPORT_SYMBOL(dispc_mgr_max_height);
static void dispc_mgr_set_lcd_divisor(enum omap_channel channel, u16 lck_div, u16 pck_div) { diff --git a/include/video/omapdss.h b/include/video/omapdss.h index 3d7c51a..53637ac 100644 --- a/include/video/omapdss.h +++ b/include/video/omapdss.h @@ -949,6 +949,8 @@ void dispc_mgr_set_lcd_config(enum omap_channel channel, const struct dss_lcd_mgr_config *config); void dispc_mgr_set_timings(enum omap_channel channel, const struct omap_video_timings *timings); +u16 dispc_mgr_max_width(void); +u16 dispc_mgr_max_height(void); void dispc_mgr_setup(enum omap_channel channel, const struct omap_overlay_manager_info *info);
-- 1.8.3.2
omap_crtc->old_fb is used to check whether the previous page flip has completed or not. However, it's never initialized to anything, so it's always NULL. This results in the check to always succeed, and the page_flip to proceed.
Initialize old_fb to the fb that we intend to flip to through page_flip, and therefore prevent a future page flip to proceed if the last one didn't complete.
Signed-off-by: Archit Taneja archit@ti.com --- drivers/gpu/drm/omapdrm/omap_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 62f28df..d74c72c 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -360,7 +360,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, }
omap_crtc->event = event; - crtc->fb = fb; + omap_crtc->old_fb = crtc->fb = fb;
/* * Hold a reference temporarily until the crtc is updated
The vblank_cb callback and the page_flip ioctl can occur together in different CPU contexts. vblank_cb uses takes tje drm device's event_lock spinlock when sending the vblank event and updating omap_crtc->event and omap_crtc->od_fb.
Use the same spinlock in page_flip, to make sure the above omap_crtc parameters are configured sequentially.
Signed-off-by: Archit Taneja archit@ti.com --- drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index d74c72c..9c7af42 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -350,6 +350,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct omap_crtc *omap_crtc = to_omap_crtc(crtc); struct drm_gem_object *bo; + unsigned long flags;
DBG("%d -> %d (event=%p)", crtc->fb ? crtc->fb->base.id : -1, fb->base.id, event); @@ -359,9 +360,12 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, return -EINVAL; }
+ spin_lock_irqsave(&dev->event_lock, flags); + omap_crtc->event = event; omap_crtc->old_fb = crtc->fb = fb;
+ spin_unlock_irqrestore(&dev->event_lock, flags); /* * Hold a reference temporarily until the crtc is updated * and takes the reference to the bo. This avoids it
On 11/04/14 10:23, Archit Taneja wrote:
The vblank_cb callback and the page_flip ioctl can occur together in different CPU contexts. vblank_cb uses takes tje drm device's event_lock spinlock when sending the vblank event and updating omap_crtc->event and omap_crtc->od_fb.
Use the same spinlock in page_flip, to make sure the above omap_crtc parameters are configured sequentially.
Signed-off-by: Archit Taneja archit@ti.com
drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index d74c72c..9c7af42 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -350,6 +350,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct omap_crtc *omap_crtc = to_omap_crtc(crtc); struct drm_gem_object *bo;
unsigned long flags;
DBG("%d -> %d (event=%p)", crtc->fb ? crtc->fb->base.id : -1, fb->base.id, event);
@@ -359,9 +360,12 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, return -EINVAL; }
spin_lock_irqsave(&dev->event_lock, flags);
omap_crtc->event = event; omap_crtc->old_fb = crtc->fb = fb;
spin_unlock_irqrestore(&dev->event_lock, flags);
There's
if (omap_crtc->old_fb)
just above the code you changed. Shouldn't that also be inside the spinlock?
Also, the description contains a few typos.
Tomi
Hi,
On Monday 14 April 2014 06:06 PM, Tomi Valkeinen wrote:
On 11/04/14 10:23, Archit Taneja wrote:
The vblank_cb callback and the page_flip ioctl can occur together in different CPU contexts. vblank_cb uses takes tje drm device's event_lock spinlock when sending the vblank event and updating omap_crtc->event and omap_crtc->od_fb.
Use the same spinlock in page_flip, to make sure the above omap_crtc parameters are configured sequentially.
Signed-off-by: Archit Taneja archit@ti.com
drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index d74c72c..9c7af42 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -350,6 +350,7 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct omap_crtc *omap_crtc = to_omap_crtc(crtc); struct drm_gem_object *bo;
unsigned long flags;
DBG("%d -> %d (event=%p)", crtc->fb ? crtc->fb->base.id : -1, fb->base.id, event);
@@ -359,9 +360,12 @@ static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, return -EINVAL; }
spin_lock_irqsave(&dev->event_lock, flags);
omap_crtc->event = event; omap_crtc->old_fb = crtc->fb = fb;
spin_unlock_irqrestore(&dev->event_lock, flags);
There's
if (omap_crtc->old_fb)
just above the code you changed. Shouldn't that also be inside the spinlock?
Yes, that should come within the spinlock too.
On a related note, the mdp5_crtc->event in mdp5_crtc_page_flip(and for mdp4 crtc) should also be inside the spinlock.
Also, the description contains a few typos.
I'll fix these.
Thanks, Archit
Hi,
On 11/04/14 10:23, Archit Taneja wrote:
These are based over Tomi's 3.15/omapdrm-fixes branch.
Archit Taneja (5): drm/omap: gem sync: wait on correct events drm/omap: Fix crash when using LCD3 overlay manager drm/omap: Allow allocation of larger buffers drm/omap: Use old_fb to synchronize between successive page flips drm/omap: protect omap_crtc's event with event_lock spinlock
Subhajit Paul (1): drm/omap: Fix memory leak in omap_gem_op_async
I've added all of these except "Allow allocation of larger buffers" to my omapdrm-fixes branch. I needed to do some small changes for the last two, but quite minor.
Tomi
dri-devel@lists.freedesktop.org
-
Archit Taneja -
Rob Clark -
Tomi Valkeinen