Re: [PATCH v7 6/6] drm/msm: iommu: Replace runtime calls with runtime suppliers

[Tomasz Figa]

On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark <robdclark@xxxxxxxxx> wrote:
> On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
>> On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark <robdclark@xxxxxxxxx> wrote:
>>> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
>>>> Hi Vivek,
>>>>
>>>> Thanks for the patch. Please see my comments inline.
>>>>
>>>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>>>> <vivek.gautam@xxxxxxxxxxxxxx> wrote:
>>>>> While handling the concerned iommu, there should not be a
>>>>> need to power control the drm devices from iommu interface.
>>>>> If these drm devices need to be powered around this time,
>>>>> the respective drivers should take care of this.
>>>>>
>>>>> Replace the pm_runtime_get/put_sync(<drm_device>) with
>>>>> pm_runtime_get/put_suppliers(<drm_device>) calls, to power-up
>>>>> the connected iommu through the device link interface.
>>>>> In case the device link is not setup these get/put_suppliers()
>>>>> calls will be a no-op, and the iommu driver should take care of
>>>>> powering on its devices accordingly.
>>>>>
>>>>> Signed-off-by: Vivek Gautam <vivek.gautam@xxxxxxxxxxxxxx>
>>>>> ---
>>>>>  drivers/gpu/drm/msm/msm_iommu.c | 16 ++++++++--------
>>>>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/msm_iommu.c b/drivers/gpu/drm/msm/msm_iommu.c
>>>>> index b23d33622f37..1ab629bbee69 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_iommu.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_iommu.c
>>>>> @@ -40,9 +40,9 @@ static int msm_iommu_attach(struct msm_mmu *mmu, const char * const *names,
>>>>>         struct msm_iommu *iommu = to_msm_iommu(mmu);
>>>>>         int ret;
>>>>>
>>>>> -       pm_runtime_get_sync(mmu->dev);
>>>>> +       pm_runtime_get_suppliers(mmu->dev);
>>>>>         ret = iommu_attach_device(iommu->domain, mmu->dev);
>>>>> -       pm_runtime_put_sync(mmu->dev);
>>>>> +       pm_runtime_put_suppliers(mmu->dev);
>>>>
>>>> For me, it looks like a wrong place to handle runtime PM of IOMMU
>>>> here. iommu_attach_device() calls into IOMMU driver's attach_device()
>>>> callback and that's where necessary runtime PM gets should happen, if
>>>> any. In other words, driver A (MSM DRM driver) shouldn't be dealing
>>>> with power state of device controlled by driver B (ARM SMMU).
>>>
>>> Note that we end up having to do the same, because of iommu_unmap()
>>> while DRM driver is powered off..  it might be cleaner if it was all
>>> self contained in the iommu driver, but that would make it so other
>>> drivers couldn't call iommu_unmap() from an irq handler, which is
>>> apparently something that some of them want to do..
>>
>> I'd assume that runtime PM status is already guaranteed to be active
>> when the IRQ handler is running, by some other means (e.g.
>> pm_runtime_get_sync() called earlier, when queuing some work to the
>> hardware). Otherwise, I'm not sure how a powered down device could
>> trigger an IRQ.
>>
>> So, if the master device power is already on, suppliers should be
>> powered on as well, thanks to device links.
>>
>
> umm, that is kindof the inverse of the problem..  the problem is
> things like gpu driver (and v4l2 drivers that import dma-buf's,
> afaict).. they will potentially call iommu->unmap() when device is not
> active (due to userspace or things beyond the control of the driver)..
> so *they* would want iommu to do pm get/put calls.

Which is fine and which is actually already done by one of the patches
in this series, not for map/unmap, but probe, add_device,
remove_device. Having parts of the API doing it inside the callback
and other parts outside sounds at least inconsistent.

> But other drivers
> trying to unmap from irq ctx would not.  Which is the contradictory
> requirement that lead to the idea of iommu user powering up iommu for
> unmap.

Sorry, maybe I wasn't clear. My last message was supposed to show that
it's not contradictory at all, because "other drivers trying to unmap
from irq ctx" would already have called pm_runtime_get_*() earlier
from a non-irq ctx, which would have also done the same on all the
linked suppliers, including the IOMMU. The ultimate result would be
that the map/unmap() of the IOMMU driver calling pm_runtime_get_sync()
would do nothing besides incrementing the reference count.

>
> There has already been some discussion about this on various earlier
> permutations of this patchset.  I think we have exhausted all other
> options.

I guess I should have read those. Let me do that now.

Best regards,
Tomasz