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@xxxxxx>
>> ---
>> 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
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
