On Mon, Aug 7, 2017 at 4:27 AM, Vivek Gautam
<vivek.gautam@xxxxxxxxxxxxxx> wrote:
> On Thu, Jul 13, 2017 at 5:20 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
>> On Thu, Jul 13, 2017 at 1:35 AM, Sricharan R <sricharan@xxxxxxxxxxxxxx> wrote:
>>> Hi Vivek,
>>>
>>> On 7/13/2017 10:43 AM, Vivek Gautam wrote:
>>>> Hi Stephen,
>>>>
>>>>
>>>> On 07/13/2017 04:24 AM, Stephen Boyd wrote:
>>>>> On 07/06, Vivek Gautam wrote:
>>>>>> @@ -1231,12 +1237,18 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
>>>>>> static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>>>>>> size_t size)
>>>>>> {
>>>>>> - struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
>>>>>> + struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>>>>>> + struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>>>>>> + size_t ret;
>>>>>> if (!ops)
>>>>>> return 0;
>>>>>> - return ops->unmap(ops, iova, size);
>>>>>> + pm_runtime_get_sync(smmu_domain->smmu->dev);
>>>>> Can these map/unmap ops be called from an atomic context? I seem
>>>>> to recall that being a problem before.
>>>>
>>>> That's something which was dropped in the following patch merged in master:
>>>> 523d7423e21b iommu/arm-smmu: Remove io-pgtable spinlock
>>>>
>>>> Looks like we don't need locks here anymore?
>>>
>>> Apart from the locking, wonder why a explicit pm_runtime is needed
>>> from unmap. Somehow looks like some path in the master using that
>>> should have enabled the pm ?
>>>
>>
>> Yes, there are a bunch of scenarios where unmap can happen with
>> disabled master (but not in atomic context).
>
> I would like to understand whether there is a situation where an unmap is
> called in atomic context without an enabled master?
>
> Let's say we have the case where all the unmap calls in atomic context happen
> only from the master's context (in which case the device link should
> take care of
> the pm state of smmu), and the only unmap that happen in non-atomic context
> is the one with master disabled. In such a case doesn it make sense to
> distinguish
> the atomic/non-atomic context and add pm_runtime_get_sync()/put_sync() only
> for the non-atomic context since that would be the one with master disabled.
>
At least drm/msm needs to hold obj->lock (a mutex) in unmap, so it
won't unmap anything in atomic ctx (but it can unmap w/ master
disabled). I can't really comment about other non-gpu drivers. It
seems like a reasonable constraint that either master is enabled or
not in atomic ctx.
Currently we actually wrap unmap w/ pm_runtime_get/put_sync(), but I'd
like to drop that to avoid powering up the gpu.
BR,
-R
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
