[PATCH 0/6] drm/omap: Misc fixes

[PATCH 0/6] drm/omap: Misc fixes

[Archit Taneja]

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(-)

-- 
1.8.3.2

[PATCH 1/6] drm/omap: Fix memory leak in omap_gem_op_async

[Archit Taneja]

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.. */
-- 
1.8.3.2

[PATCH 2/6] drm/omap: gem sync: wait on correct events

[Archit Taneja]

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;
 }
-- 
1.8.3.2

[PATCH 3/6] drm/omap: Fix crash when using LCD3 overlay manager

[Archit Taneja]

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)
-- 
1.8.3.2

[PATCH 4/6] drm/omap: Allow allocation of larger buffers

[Archit Taneja]

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);
 
-- 
1.8.3.2

Re: [PATCH 4/6] drm/omap: Allow allocation of larger buffers

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2076 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/6] drm/omap: Allow allocation of larger buffers

[Archit Taneja]

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

Re: [PATCH 4/6] drm/omap: Allow allocation of larger buffers

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 2834 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 4/6] drm/omap: Allow allocation of larger buffers

[Archit Taneja]

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

Re: [PATCH 4/6] drm/omap: Allow allocation of larger buffers

[Rob Clark]

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
>

[PATCH 5/6] drm/omap: Use old_fb to synchronize between successive page flips

[Archit Taneja]

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
-- 
1.8.3.2

[PATCH 6/6] drm/omap: protect omap_crtc's event with event_lock spinlock

[Archit Taneja]

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
-- 
1.8.3.2

Re: [PATCH 6/6] drm/omap: protect omap_crtc's event with event_lock spinlock

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1583 bytes --]

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




[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 6/6] drm/omap: protect omap_crtc's event with event_lock spinlock

[Archit Taneja]

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

Re: [PATCH 0/6] drm/omap: Misc fixes

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 685 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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