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

[Vivek Gautam]

Hi Tomasz,

On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa <tfiga@xxxxxxxxxxxx> wrote:
> 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.

The entire point was to avoid the slowpath that pm_runtime_get/put_sync()
would add in map/unmap. It would not be correct to add a slowpath in irq_ctx
for taking care of non-irq_ctx and for the situations where master is already
powered-off.

>
>>
>> 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.
Yea, i point to the thread in cover letter and [PATCH 1/6].
Thanks.

regards
Vivek

>
> Best regards,
> Tomasz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation