[PATCH 00/65] OMAPDSS: manager/apply improvements

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

The get/set info functions for overlays and managers only modify the 
omap_overlay_info or manager_info structs, these aren't really a part of 
'dss_data', they only become a part of dss_data only when we call 
mgr->apply().

So, are we protecting these functions so that 2 users of the same 
overlay don't see incorrect info values?

Archit

  int dss_ovl_set_manager(struct omap_overlay *ovl,

On Wed, 2011-11-23 at 15:26 +0530, Archit Taneja wrote:

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

quoted

  int dss_mgr_set_device(struct omap_overlay_manager *mgr,

@@ -745,16 +762,28 @@ err:
  int dss_ovl_set_info(struct omap_overlay *ovl,
  		struct omap_overlay_info *info)
  {
+	unsigned long flags;
+
+	spin_lock_irqsave(&data_lock, flags);
+
  	ovl->info = *info;
  	ovl->info_dirty = true;

+	spin_unlock_irqrestore(&data_lock, flags);
+
  	return 0;
  }

  void dss_ovl_get_info(struct omap_overlay *ovl,
  		struct omap_overlay_info *info)
  {
+	unsigned long flags;
+
+	spin_lock_irqsave(&data_lock, flags);
+
  	*info = ovl->info;
+
+	spin_unlock_irqrestore(&data_lock, flags);
  }

The get/set info functions for overlays and managers only modify the 
omap_overlay_info or manager_info structs, these aren't really a part of 
'dss_data', they only become a part of dss_data only when we call 
mgr->apply().

So, are we protecting these functions so that 2 users of the same 
overlay don't see incorrect info values?

True, at this point the data_lock is a bit vague, and is protecting also
the info fields in omap_overlay and omap_overlay_manager.

A lock is needed, though, as otherwise the info struct may be only
partial. E.g. somebody calls set_info, which is half way copying the
values, and somebody else calls apply or get_info.

In the next patches the infos will be moved into the dss_data, and then
using dss_lock spin lock makes more sense.

 Tomi

On Wed, 2011-11-23 at 15:18 +0530, Archit Taneja wrote:

Hi,

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

quoted

The functions in apply.c, called mostly via function pointers in overlay
and overlay_manager structs, will be divided into two groups. The other
group will not sleep and can be called from interrupts, and the other
group may sleep.

Small sentence issue above, both groups are called the 'other group'.

Thanks, fixed.

quoted

The idea is that the non-sleeping functions may only change certain
settings in overlays and managers, and those settings may only affect
the particular overlay/manager. For example, set the base address of the
overlay.

The blocking functions, however, will handle more complex configuration
changes. For example, when an overlay is enabled and fifo-merge feature
is used, we need to do the enable in multiple steps, waiting in between,
and the change affects multiple overlays and managers.

This patch adds the mutex which is used in the blocking functions to
have exclusive access to overlays and overlay managers.

Previously, when we changed the links between 'overlay->managers' and 
'manager->devices', it wasn't protected by a lock. Why is it needed now?

Previously many places were missing a lock =).

As an example, suppose we are changing a manager's device to some other 
display. Is this lock preventing someone else to get the older 
'mgr->device' rather than the new one?

Hmm. We need some lock there, that's for sure, as set/unset manager are
changing the manager's list of overlays. However, it is also protected
by the spinlock, so in that sense mutex is not necessary.

I have to say I'm not sure if mutex is needed at this point. However,
consider the end result when fifo-merge is used:

dss_ovl_enable() will take the mutex, then it does the configuration in
multiple steps, doing multiple spin_lock & spin_unlocks, waiting in
between.

If we had only a spinlock in set/unset_manager, the manager could be
changed while dss_ovl_enable is doing the process of enabling the
overlay.

So I may have added mutexes or spinlocks a bit early in the series to
some places, but I don't see any harm in that. It'd be rather difficult
to try to find the exact spots where a lock becomes a requirement.

 Tomi

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

The current code uses dsi_video_mode_enable/disable functions to
enable/disable DISPC output for video mode displays. For command mode
displays we have no notion in the DISPC side of whether the panel is
enabled, except when a dss_start_update() call is made.

However, to properly maintain the DISPC state in apply.c, we need to
know if a manager used for a manual update display is currently in use.

This patch achieves that by changing dsi_video_mode_enable/disable to
dsi_enable/disable_video_output, which is called by both video and
command mode displays. For video mode displays it starts the actual
pixel stream, as it did before. For command mode displays it doesn't do
anything else than mark that the manager is currently in use.

dsi_video_mode_enable() doesn't only enable the DISPC output, it also 
sends the long packet header to start video mode transfer.

I think it would be better if we had 2 separate functions, one which 
starts/stops DSI video mode, and the other which enables/disables the 
DISPC video port.

This way, a manual update panel would need to call only 
dsi_enable/disable_video_output(which just enables or disables the 
manager), whereas a video mode panel will need to call both.

This is just a suggestion though. It's probably okay to have both in the 
same function too. We might have to separate them out later if we were 
planning to standardise mipi dsi across SoCs.

Archit

On 11/22/2011 11:21 AM, Tomi Valkeinen wrote:

dss_cache struct contains a spinlock used to protect the struct. A more
logical place for the spinlock is outside the struct that it is
protecting. So move it there.

a small question: isn't it clearer to keep lock inside struct, so it would be easier to read code? Say, if we meet

spin_lock_irqsave(&dss_cache.lock, flags);

in code we already aware of what struct being actually protected, and in case of external lock it's not that obvious

-- 
regards,
Sergey

On Wed, 2011-11-23 at 15:40 +0530, Archit Taneja wrote:

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

quoted

The current code uses dsi_video_mode_enable/disable functions to
enable/disable DISPC output for video mode displays. For command mode
displays we have no notion in the DISPC side of whether the panel is
enabled, except when a dss_start_update() call is made.

However, to properly maintain the DISPC state in apply.c, we need to
know if a manager used for a manual update display is currently in use.

This patch achieves that by changing dsi_video_mode_enable/disable to
dsi_enable/disable_video_output, which is called by both video and
command mode displays. For video mode displays it starts the actual
pixel stream, as it did before. For command mode displays it doesn't do
anything else than mark that the manager is currently in use.

dsi_video_mode_enable() doesn't only enable the DISPC output, it also 
sends the long packet header to start video mode transfer.

I think it would be better if we had 2 separate functions, one which 
starts/stops DSI video mode, and the other which enables/disables the 
DISPC video port.

This way, a manual update panel would need to call only 
dsi_enable/disable_video_output(which just enables or disables the 
manager), whereas a video mode panel will need to call both.

This is just a suggestion though. It's probably okay to have both in the 
same function too. We might have to separate them out later if we were 
planning to standardise mipi dsi across SoCs.

If you think from the panel driver's point of view, it doesn't know
about DISPC. It just wants to enable the video stream (on video mode
displays).

If we had two functions, could only the first be used? I.e. is it
possible to just enable the video mode transfer, without enabling DISPC?
If not, I'm not sure what would be the use for two separate functions.
And even if it can, I'm not sure what use it would be to enable only the
video mode output without the actual pixel data from DISPC.

It is true that the function in thsi patch is not the best one. For
command mode display it's more about reserving the ovl manager for use
than actually enabling it. Then again, how I thought the function's
purpose was that it enables the DSI video output, but because for
command mode there's need to trigger the actual frame transfer later,
the function doesn't start the pixel feed for command mode displays. It
just "prepares" the output.

But even if we had the functions separated, the function called by video
mode and command mode displays would be different, as for the former one
it enables the pixel stream, and for latter one it just reserves the
output.

So, I'm fine with changing the function, but the reasoning for what
functions we have and what they do should come from the panel driver's
perspective, not because of OMAP DSS's HW details.

 Tomi

On Wed, 2011-11-23 at 12:29 +0200, Sergey Kibrik wrote:

On 11/22/2011 11:21 AM, Tomi Valkeinen wrote:

quoted

dss_cache struct contains a spinlock used to protect the struct. A more
logical place for the spinlock is outside the struct that it is
protecting. So move it there.

a small question: isn't it clearer to keep lock inside struct, so it would be easier to read code? Say, if we meet

quoted

spin_lock_irqsave(&dss_cache.lock, flags);

in code we already aware of what struct being actually protected, and in case of external lock it's not that obvious

But if you meet code like:

op = get_ovl_priv(ovl);

You don't see that the data is inside the struct protected with the
spinlock. So you still need to understand what it protects, and what the
above function returns.

But I see your point. I'm not sure which way is better. I thought it
like this: the lock protects a struct, but if the lock is inside the
struct, the lock would protect also itself. Which it doesn't.

That said, I'm fine with both ways. It doesn't matter much. I didn't
really look for any established patterns for this in the kernel code,
but if everybody else have their locks inside the structs they protect,
then obviously we should also.

 Tomi

On Wednesday 23 November 2011 04:12 PM, Tomi Valkeinen wrote:

On Wed, 2011-11-23 at 15:40 +0530, Archit Taneja wrote:

quoted

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

quoted

The current code uses dsi_video_mode_enable/disable functions to
enable/disable DISPC output for video mode displays. For command mode
displays we have no notion in the DISPC side of whether the panel is
enabled, except when a dss_start_update() call is made.

However, to properly maintain the DISPC state in apply.c, we need to
know if a manager used for a manual update display is currently in use.

This patch achieves that by changing dsi_video_mode_enable/disable to
dsi_enable/disable_video_output, which is called by both video and
command mode displays. For video mode displays it starts the actual
pixel stream, as it did before. For command mode displays it doesn't do
anything else than mark that the manager is currently in use.

dsi_video_mode_enable() doesn't only enable the DISPC output, it also
sends the long packet header to start video mode transfer.

I think it would be better if we had 2 separate functions, one which
starts/stops DSI video mode, and the other which enables/disables the
DISPC video port.

This way, a manual update panel would need to call only
dsi_enable/disable_video_output(which just enables or disables the
manager), whereas a video mode panel will need to call both.

This is just a suggestion though. It's probably okay to have both in the
same function too. We might have to separate them out later if we were
planning to standardise mipi dsi across SoCs.

If you think from the panel driver's point of view, it doesn't know
about DISPC. It just wants to enable the video stream (on video mode
displays).

If we had two functions, could only the first be used? I.e. is it
possible to just enable the video mode transfer, without enabling DISPC?
If not, I'm not sure what would be the use for two separate functions.
And even if it can, I'm not sure what use it would be to enable only the
video mode output without the actual pixel data from DISPC.

It is true that the function in thsi patch is not the best one. For
command mode display it's more about reserving the ovl manager for use
than actually enabling it. Then again, how I thought the function's
purpose was that it enables the DSI video output, but because for
command mode there's need to trigger the actual frame transfer later,
the function doesn't start the pixel feed for command mode displays. It
just "prepares" the output.

But even if we had the functions separated, the function called by video
mode and command mode displays would be different, as for the former one
it enables the pixel stream, and for latter one it just reserves the
output.

So, I'm fine with changing the function, but the reasoning for what
functions we have and what they do should come from the panel driver's
perspective, not because of OMAP DSS's HW details.

If you look from the panel driver's perspective, we shouldn't split this.

I think it would be best to stuff the 'video mode enabling and manager 
enabling' functionality in omapdss_dsi_display_enable() itself, the 
panel driver shouldn't need to call a function separately to enable 
video mode for the panel. This way we would be more along the lines of 
the dpi driver, where dpi_display_enable() enables the manager in the end.

I guess we can leave this the way you are doing it currently, and move 
this code into dsi_display_enable() code later on.

Archit

  Tomi

On Wed, 2011-11-23 at 16:38 +0530, Archit Taneja wrote:

I think it would be best to stuff the 'video mode enabling and
manager 
enabling' functionality in omapdss_dsi_display_enable() itself, the 
panel driver shouldn't need to call a function separately to enable 
video mode for the panel. This way we would be more along the lines
of 
the dpi driver, where dpi_display_enable() enables the manager in the
end. 

But we need to configure the panel between enabling the DSI interface
and enabling the video output, so we can't combine those two functions.

For DPI things are simpler, as enabling the interface and the video
output are more or less the same thing.

 Tomi

On Wednesday 23 November 2011 04:57 PM, Tomi Valkeinen wrote:

On Wed, 2011-11-23 at 16:38 +0530, Archit Taneja wrote:

quoted

I think it would be best to stuff the 'video mode enabling and
manager
enabling' functionality in omapdss_dsi_display_enable() itself, the
panel driver shouldn't need to call a function separately to enable
video mode for the panel. This way we would be more along the lines
of
the dpi driver, where dpi_display_enable() enables the manager in the
end.

But we need to configure the panel between enabling the DSI interface
and enabling the video output, so we can't combine those two functions.

Oh okay, that's right, we can't start video mode before preparing the panel.

Archit

For DPI things are simpler, as enabling the interface and the video
output are more or less the same thing.

  Tomi

On Wed, 2011-12-07 at 18:37 +0530, Archit Taneja wrote:

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

quoted

+/* wait until no extra_info updates are pending */
+static void wait_pending_extra_info_updates(void)
+{
+	bool updating;
+	unsigned long flags;
+	unsigned long t;
+
+	spin_lock_irqsave(&data_lock, flags);
+
+	updating = extra_info_update_ongoing();
+
+	if (!updating) {
+		spin_unlock_irqrestore(&data_lock, flags);
+		return;
+	}
+
+	init_completion(&extra_updated_completion);
+
+	spin_unlock_irqrestore(&data_lock, flags);
+
+	t = msecs_to_jiffies(500);
+	wait_for_completion_timeout(&extra_updated_completion, t);
+
+	updating = extra_info_update_ongoing();
+
+	WARN_ON(updating);
+}

This function isn't used, are you planning to use it later?

Yes, it's used with the fifo-merge stuff. It could be added later, but
as it's related to the extra_info stuff, I think it fits better here
than with the fifo merge series.

 Tomi

Hi,

On Tuesday 22 November 2011 02:52 PM, Tomi Valkeinen wrote:

Add checks for overlay and manager settings. The checks are a bit
complex, as we need to observe the bigger picture instead of overlays
and managers independently. Things like the used display and the zorder
of other overlays affect the validity of the settings.

Minor comment:

dss_ovl_check, dss_mgr_check and dss_mgr_check_zorder don't really 
qualify as functions which do actual applying of configurations, they 
could be moved from apply.c to manager.c and overlay.c.

Archit

On Tuesday 22 November 2011 02:51 PM, Tomi Valkeinen wrote:

This function isn't used, are you planning to use it later?

Archit

On Wed, 2011-12-07 at 18:35 +0530, Archit Taneja wrote:

Hi,

On Tuesday 22 November 2011 02:52 PM, Tomi Valkeinen wrote:

quoted

Add checks for overlay and manager settings. The checks are a bit
complex, as we need to observe the bigger picture instead of overlays
and managers independently. Things like the used display and the zorder
of other overlays affect the validity of the settings.

Minor comment:

dss_ovl_check, dss_mgr_check and dss_mgr_check_zorder don't really 
qualify as functions which do actual applying of configurations, they 
could be moved from apply.c to manager.c and overlay.c.

I had the check functions in apply.c because they used apply.c's
internal datastructures.

However, looking the functions now, only dss_mgr_check_zorder() used the
internal datas, and the function doesn't even use those variables for
anything =).

So you're right, they can be moved to manager.c and overlay.c, thanks.

 Tomi

Hi,

On Thursday 08 December 2011 01:59 PM, Tomi Valkeinen wrote:

On Wed, 2011-12-07 at 18:35 +0530, Archit Taneja wrote:

quoted

Hi,

On Tuesday 22 November 2011 02:52 PM, Tomi Valkeinen wrote:

quoted

Add checks for overlay and manager settings. The checks are a bit
complex, as we need to observe the bigger picture instead of overlays
and managers independently. Things like the used display and the zorder
of other overlays affect the validity of the settings.

Minor comment:

dss_ovl_check, dss_mgr_check and dss_mgr_check_zorder don't really
qualify as functions which do actual applying of configurations, they
could be moved from apply.c to manager.c and overlay.c.

I had the check functions in apply.c because they used apply.c's
internal datastructures.

However, looking the functions now, only dss_mgr_check_zorder() used the
internal datas, and the function doesn't even use those variables for
anything =).

Also, the function dss_mgr_check() takes 'struct 
omap_overlay_manager_info *info' as a parameter but doesn't use it. We 
might want to remove that too.

Archit

So you're right, they can be moved to manager.c and overlay.c, thanks.

  Tomi

On Tue, 2011-12-13 at 15:32 +0530, Archit Taneja wrote:

Hi,

On Thursday 08 December 2011 01:59 PM, Tomi Valkeinen wrote:

quoted

On Wed, 2011-12-07 at 18:35 +0530, Archit Taneja wrote:

quoted

Hi,

On Tuesday 22 November 2011 02:52 PM, Tomi Valkeinen wrote:

quoted

Add checks for overlay and manager settings. The checks are a bit
complex, as we need to observe the bigger picture instead of overlays
and managers independently. Things like the used display and the zorder
of other overlays affect the validity of the settings.

Minor comment:

dss_ovl_check, dss_mgr_check and dss_mgr_check_zorder don't really
qualify as functions which do actual applying of configurations, they
could be moved from apply.c to manager.c and overlay.c.

I had the check functions in apply.c because they used apply.c's
internal datastructures.

However, looking the functions now, only dss_mgr_check_zorder() used the
internal datas, and the function doesn't even use those variables for
anything =).

Also, the function dss_mgr_check() takes 'struct 
omap_overlay_manager_info *info' as a parameter but doesn't use it. We 
might want to remove that too.

Right. And the dss_ovl/mgr_simple_check() functions could also be moved
outside apply.c.

 Tomi