On 08/12/2020 17:48, Laurent Pinchart wrote:
Hi Tomi,
Thank you for the patch.
On Tue, Dec 08, 2020 at 02:28:55PM +0200, Tomi Valkeinen wrote:
Panel drivers can send DSI commands in panel's prepare(), which happens before the bridge's enable() is called. The OMAP DSI driver currently only sets up the DSI interface at bridge's enable(), so prepare() cannot be used to send DSI commands.
This patch fixes the issue by making it possible to enable the DSI interface any time a command is about to be sent. Disabling the interface is be done via delayed work.
Signed-off-by: Tomi Valkeinen tomi.valkeinen@ti.com
drivers/gpu/drm/omapdrm/dss/dsi.c | 49 +++++++++++++++++++++++++++---- drivers/gpu/drm/omapdrm/dss/dsi.h | 3 ++ 2 files changed, 47 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c index 53a64bc91867..34f665aa9a59 100644 --- a/drivers/gpu/drm/omapdrm/dss/dsi.c +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c @@ -3503,6 +3503,9 @@ static void dsi_enable(struct dsi_data *dsi)
WARN_ON(!dsi_bus_is_locked(dsi));
if (WARN_ON(dsi->iface_enabled))
return;mutex_lock(&dsi->lock);
r = dsi_runtime_get(dsi);
@@ -3515,6 +3518,8 @@ static void dsi_enable(struct dsi_data *dsi) if (r) goto err_init_dsi;
dsi->iface_enabled = true;
mutex_unlock(&dsi->lock);
return;
@@ -3530,6 +3535,9 @@ static void dsi_disable(struct dsi_data *dsi) { WARN_ON(!dsi_bus_is_locked(dsi));
if (WARN_ON(!dsi->iface_enabled))
return;mutex_lock(&dsi->lock);
dsi_sync_vc(dsi, 0);
@@ -3541,6 +3549,8 @@ static void dsi_disable(struct dsi_data *dsi)
dsi_runtime_put(dsi);
- dsi->iface_enabled = false;
- mutex_unlock(&dsi->lock);
}
@@ -4229,10 +4239,12 @@ static ssize_t omap_dsi_host_transfer(struct mipi_dsi_host *host,
dsi_bus_lock(dsi);
- if (dsi->video_enabled)
r = _omap_dsi_host_transfer(dsi, vc, msg);- else
r = -EIO;
if (!dsi->iface_enabled) {
dsi_enable(dsi);schedule_delayed_work(&dsi->dsi_disable_work, msecs_to_jiffies(2000));}
r = _omap_dsi_host_transfer(dsi, vc, msg);
dsi_bus_unlock(dsi);
@@ -4397,6 +4409,14 @@ static int omap_dsi_host_detach(struct mipi_dsi_host *host, if (WARN_ON(dsi->dsidev != client)) return -EINVAL;
- cancel_delayed_work_sync(&dsi->dsi_disable_work);
- if (dsi->iface_enabled) {
dsi_bus_lock(dsi);dsi_disable(dsi);dsi_bus_unlock(dsi);- }
- omap_dsi_unregister_te_irq(dsi); dsi->dsidev = NULL; return 0;
@@ -4632,9 +4652,12 @@ static void dsi_bridge_enable(struct drm_bridge *bridge) struct dsi_data *dsi = drm_bridge_to_dsi(bridge); struct omap_dss_device *dssdev = &dsi->output;
- cancel_delayed_work_sync(&dsi->dsi_disable_work);
Is there a risk of a race condition if omap_dsi_host_transfer() is called right here, before locking the bus ? Or is there a guarantee that the two functions can't be executed concurrently ? Same for dsi_bridge_disable() below.
Yes, there's a possibility for a race, if the panel driver does dsi command transactions via, say, a timer, and doesn't take DRM locks that are shared with bridge-enable/disable/detach.
For bridge-enable, it shouldn't matter: If the disable callback is called just before bridge_enable takes the dsi_bus_lock, no problem, bridge_enable just enables the interface again. If the callback is ran just after bridge_enable's dsi_bus_unlock, no problem, dsi->video_enabled == true so the callback does nothing.
Similarly for bridge-disable, the callback won't do anything if video_enabled == true, and after bridge-disable has turned the video and the interface off, there's nothing to do for the callback.
The detach is a bit more unclear. Is the panel driver allowed to do "stuff" with bridges while bridge detach is going on? If yes, it's probably broken. We should move the bus_locks to cover the whole if() so that dsi->iface_enabled is inside the locks. With that change the delayed disable itself should work fine.
But we don't have anything stopping omap_dsi_host_transfer being called after the whole bridge has been detached (or called before attach). So, if we have a guarantee that the panels won't be doing dsi transfers before/during bridge attach or after/during bridge detach, we have no issue. If we don't have such a guarantee, it's broken.
I'll try to figure out if there's such a guarantee, but maybe it's safer to add a flag to indicate if the bridge is available, and check that during omap_dsi_host_transfer.
Tomi