[Freedreno] [RFT PATCH] drm/msm: Trigger fence completion from GPU

[Bjorn Andersson]

Interrupt commands causes the CP to trigger an interrupt as the command
is processed, regardless of the GPU being done processing previous
commands. This is seen by the interrupt being delivered before the
fence is written on 8974 and is likely the cause of the additional
CP_WAIT_FOR_IDLE workaround found for a306, which would cause the CP to
wait for the GPU to go idle before triggering the interrupt.

Instead we can set the (undocumented) BIT(31) of the CACHE_FLUSH_TS
which will cause a special CACHE_FLUSH_TS interrupt to be triggered from
the GPU as the write event is processed.

Add CACHE_FLUSH_TS to the IRQ masks of A3xx and A4xx and remove the
workaround for A306.

Suggested-by: Jordan Crouse 
Signed-off-by: Bjorn Andersson 
---

This is only tested on 8974.

 drivers/gpu/drm/msm/adreno/a3xx_gpu.c   |  1 +
 drivers/gpu/drm/msm/adreno/a4xx_gpu.c   |  1 +
 drivers/gpu/drm/msm/adreno/adreno_gpu.c | 18 ++
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
index 4baef2738178..a3a43be920d0 100644
--- a/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a3xx_gpu.c
@@ -35,6 +35,7 @@
 A3XX_INT0_CP_RB_INT | \
 A3XX_INT0_CP_REG_PROTECT_FAULT |  \
 A3XX_INT0_CP_AHB_ERROR_HALT | \
+A3XX_INT0_CACHE_FLUSH_TS |\
 A3XX_INT0_UCHE_OOB_ACCESS)
 
 extern bool hang_debug;
diff --git a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c 
b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
index 8199a4b9f2fa..b44cd0d90621 100644
--- a/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a4xx_gpu.c
@@ -27,6 +27,7 @@
 A4XX_INT0_CP_RB_INT | \
 A4XX_INT0_CP_REG_PROTECT_FAULT |  \
 A4XX_INT0_CP_AHB_ERROR_HALT | \
+A4XX_INT0_CACHE_FLUSH_TS |\
 A4XX_INT0_UCHE_OOB_ACCESS)
 
 extern bool hang_debug;
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c 
b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
index de63ff26a062..5806f9942514 100644
--- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
@@ -293,26 +293,12 @@ void adreno_submit(struct msm_gpu *gpu, struct 
msm_gem_submit *submit,
OUT_RING(ring, 0x);
}
 
+   /* BIT(31) of CACHE_FLUSH_TS triggers CACHE_FLUSH_TS IRQ from GPU */
OUT_PKT3(ring, CP_EVENT_WRITE, 3);
-   OUT_RING(ring, CACHE_FLUSH_TS);
+   OUT_RING(ring, CACHE_FLUSH_TS | BIT(31));
OUT_RING(ring, rbmemptr(ring, fence));
OUT_RING(ring, submit->seqno);
 
-   /* we could maybe be clever and only CP_COND_EXEC the interrupt: */
-   OUT_PKT3(ring, CP_INTERRUPT, 1);
-   OUT_RING(ring, 0x8000);
-
-   /* Workaround for missing irq issue on 8x16/a306.  Unsure if the
-* root cause is a platform issue or some a306 quirk, but this
-* keeps things humming along:
-*/
-   if (adreno_is_a306(adreno_gpu)) {
-   OUT_PKT3(ring, CP_WAIT_FOR_IDLE, 1);
-   OUT_RING(ring, 0x);
-   OUT_PKT3(ring, CP_INTERRUPT, 1);
-   OUT_RING(ring, 0x8000);
-   }
-
 #if 0
if (adreno_is_a3xx(adreno_gpu)) {
/* Dummy set-constant to trigger context rollover */
-- 
2.15.0

___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Tomasz Figa]

On Wed, Feb 14, 2018 at 1:17 PM, Vivek Gautam
 wrote:
> Hi Tomasz,
>
> On Wed, Feb 14, 2018 at 8:31 AM, Tomasz Figa  wrote:
>> On Wed, Feb 14, 2018 at 11:13 AM, Rob Clark  wrote:
>>> On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa  wrote:
 On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark  wrote:
> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa  wrote:
>> Hi Vivek,
>>
>> Thanks for the patch. Please see my comments inline.
>>
>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>>  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() with
>>> pm_runtime_get/put_suppliers() 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 
>>> ---
>>>  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.

Correct me if I'm wrong, but I believe that with what I'm 

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

[Tomasz Figa]

Hi Jordan,

On Wed, Feb 14, 2018 at 1:42 AM, Jordan Crouse  wrote:
> On Tue, Feb 13, 2018 at 06:10:38PM +0900, Tomasz Figa wrote:
>> Hi Vivek,
>>
>> Thanks for the patch. Please see my comments inline.
>>
>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>>  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() with
>> > pm_runtime_get/put_suppliers() 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 
>> > ---
>> >  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).
>
> This whole thing is confused by the fact that on MSM the GPU and the GPU IOMMU
> share some of the same clocks and power rail so turning on the GPU also
> turned on the IOMMU register banks by extension.

This is surprisingly not a very surprising case. Exactly the same can
be seen on Rockchip SoCs and we're solving the problem using the
solution I suggested. In fact, my suggestions to this thread are based
on the design we chose for Rockchip, due to the high level of
similarity (+/- the GPU directly programming IOMMU registers, which is
not present there, but AFAICT it doesn't pose a problem here).

>
> But if we put that aside the question is who should be responsible for
> controlling the power in this relationship and there are several good reasons 
> to
> leave it up to the client device. The most important reason is when we move to
> the per-instance model where the GPU self-programmings the SMMU registers. In
> that case, the driver will need to make sure that the SMMU is powered up 
> before
> submitting the command and then removing the power vote when the commands
> are done to save energy.

I might need more insight on what's going on in your hardware, but
with my current understanding I'd argue that that is not right,
because:

- When submitting commands to the GPU, the GPU driver will
pm_runtime_get_sync() on the GPU device, which will automatically do
the same on all the linked suppliers, which would also include the
SMMU itself. The role of device links here is exactly that the GPU
driver doesn't have to care which other devices need to be brought up.

- When the GPU is operating, the SMMU power must be supplied anyway,
because it needs to be doing the translations, right? Note that by
"power" I really mean the physical power supply in the SoC, e.g. as
for a power domain. The runtime PM API in its current form (e.g.
binary off or on operation) is unsuitable for managing other things,
such as clocks (and there is ongoing work on improving it, e.g. by
adding support for multiple power states).

   ^^ The above would be actually guaranteed by your hardware design,
where SMMU and GPU share the power domain and clocks. (We used to rely
on this in old downstream implementation of Rockchip IOMMU and master
drivers in Chromium OS kernel, before we moved to handling the clocks
explicitly in the IOMMU driver and properly using device links to
manage the power domain and state restoration.)

>
> Additionally, there might be legitimate reasons in the driver to batch
> operations - you may wish to attach the device and then map several global
> buffers immediately - having driver side control prevents several unneeded 
> power
> transitions.

As I mentioned before, these operations wouldn't normally need any
power transitions, since mapping with the TLB powered down boils down
to just updating the page tables in memory. 

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

[Rob Clark]

On Tue, Feb 13, 2018 at 8:59 PM, Tomasz Figa  wrote:
> On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark  wrote:
>> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa  wrote:
>>> Hi Vivek,
>>>
>>> Thanks for the patch. Please see my comments inline.
>>>
>>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>>>  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() with
 pm_runtime_get/put_suppliers() 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 
 ---
  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.  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.

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

BR,
-R
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Tomasz Figa]

On Wed, Feb 14, 2018 at 3:03 AM, Rob Clark  wrote:
> On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa  wrote:
>> Hi Vivek,
>>
>> Thanks for the patch. Please see my comments inline.
>>
>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>>  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() with
>>> pm_runtime_get/put_suppliers() 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 
>>> ---
>>>  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.

Best regards,
Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Clark]

On Tue, Feb 13, 2018 at 7:02 PM, Jordan Crouse  wrote:
> On Tue, Feb 13, 2018 at 02:18:13PM -0500, Sean Paul wrote:
>> Hi dri-devel,
>> Qualcomm has been working for the past few weeks on forward porting their
>> downstream drm driver from 4.14 to mainline. Please consider this PR as a
>> request for review, rather than an attempt at mainlining the code as it
>> currently stands. The goal is get this driver in shape over the next coming
>> months.
>>
>> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
>> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things 
>> look
>> good, I'll send another pull 4realz.
>>
>>  drivers/gpu/drm/msm/msm_rd.c   |   58 +-
>
> These changes are somewhat overzealous pointer verification, If
> struct file * and/or struct inode * are NULL in a fops function your system is
> already having a bad day. The only check that might be somewhat reasonable is
> for gpu->funcs->get_param but that hook is defined on all targets so I don't
> think it is unreasonable to assume that it should be there (and if it isn't 
> the
> unit test sure better catch it). We can safely drop these changes.
>

thanks, this was something I didn't notice (or test) yet.. a couple of
the other 'elfring fixes' I encountered (I guess there are probably
some coding standards or static analysis behind this?) actually broke
things and I have already reverted in the course of unbreaking
display/gpu on db410c..

But yeah, ->get_param() is mandatory in the ioctl path (which is
something you don't need debugfs access for).. for the paranoid
probably the better thing would be a WARN_ON(!gpu->get_param) in gpu
init path.

BR,
-R
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845

[Jordan Crouse]

On Tue, Feb 13, 2018 at 02:18:13PM -0500, Sean Paul wrote:
> Hi dri-devel,
> Qualcomm has been working for the past few weeks on forward porting their
> downstream drm driver from 4.14 to mainline. Please consider this PR as a
> request for review, rather than an attempt at mainlining the code as it
> currently stands. The goal is get this driver in shape over the next coming
> months.
> 
> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things look
> good, I'll send another pull 4realz.
> 
>  drivers/gpu/drm/msm/msm_rd.c   |   58 +-

These changes are somewhat overzealous pointer verification, If
struct file * and/or struct inode * are NULL in a fops function your system is
already having a bad day. The only check that might be somewhat reasonable is
for gpu->funcs->get_param but that hook is defined on all targets so I don't
think it is unreasonable to assume that it should be there (and if it isn't the
unit test sure better catch it). We can safely drop these changes.

Jordan

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [RFC PULL] Add Display Support for Qualcomm SDM845

[Rob Clark]

On Tue, Feb 13, 2018 at 2:18 PM, Sean Paul  wrote:
> Hi dri-devel,
> Qualcomm has been working for the past few weeks on forward porting their
> downstream drm driver from 4.14 to mainline. Please consider this PR as a
> request for review, rather than an attempt at mainlining the code as it
> currently stands. The goal is get this driver in shape over the next coming
> months.
>
> In the meantime, I'll be hosting a tree here [1] to stage the fixes. Patches
> will be posted and reviewed on linux-arm-...@vger.kernel.org. Once things look
> good, I'll send another pull 4realz.
>
> To get the ball rolling, I've done some review on the new connector code, my
> comments are below.
>
> Thanks in advance for your constructive feedback :)
>
> Sean
>
> [1]- git://people.freedesktop.org/~seanpaul/dpu-staging
>
> Review feedback:
> 

just so others aren't confused, s/sde/dpu/g in the list below (we did
our DAL->DC rename before sending first RFC :-P)

> - Solve the splash screen handling (or remove it)
> - Simplify devicetree binding (remove register offsets)
> feedback from reviewing sde_connector.c:
> - Rationalize backlight implementation in sde_connector (display_count static)
> - Sort out the dsi event passing between dsi/encoder/connector (move to 
> encoder)
> - include/uapi/drm/msm_drm_pp.h needs opensource userspace (or removal)
> - connector->state access violations reading/writing mode_info
> - s/sde_rect/drm_rect/
> - sde_kms_info usage needs to be replaced with formal data structures (not
>   stringified keypairs)
> - sde_connector_ops needs to be trimmed, duplicates connector helpers, info
>   hooks circumvent state, and other hooks should be stored in state or
>   prepopulated (get_dst_format)
> - sde_connector_get_dpms unused
> - esd status check should migrate to encoder from connector
> - backlight should be handled in panel drivers, not in the generic 
> connector/dsi
>   encoder
> - sde_connector_helper_bridge_disable is called from encoder and calls back 
> into
>   set_power encoder function. if backlight, and esd status are removed,
>   pre_kickoff can probably go away
> - sde_connector_clk_ctrl is another example of encoder->connector->encoder 
> call
> - RETIRE_FENCE connector property should be removed, opting for the native
>   atomic fences
> - ROI (regions of interest) should be expressed per-plane instead of 
> connector.
>   there is work ongoing to support dirty_rects per-plane by Deepak Singh Rawat
>    and Lukasz Spintzyk 
> - Uma Shankar  has proposed HDR source metadata
>   properties on the list, we should pivot to those instead of hand-rolling 
> them
>   in the sde driver
> - Convert HDCP implementation to upstream Content Protection property
> - Merge dsi and dsi_staging into one driver
> - Writeback connector has been proposed by ARM (Liviu Dudau and Brian 
> Starkey),
>   we should work with their proposal instead of rolling OUT_FB ourselves
> - sde_connector_set_property should be replaced with atomic helper
> - dsi hotplug can probably be punted to the panel driver
> - dpms should switch to enable/disable (or at least use the atomic helpers)
> - dsi mode handling should also defer to the panel driver
> - SDE_WB_CONFIG ioctl should be removed in favor of the existing ioctl to add
>   user-defined modes
> - dp implementation should use the existing dp helpers wherever possible
> - lots of duplicated structures in dsi_defs.h that can be replaced with 
> existing
>   drm structs
> - mode_valid should be split up and implemented directly in connector/encoder 
> as
>   appropriate
> - sde_connector->aspace seems like it's unused?
>

To add to that, a few things I've noticed (but I've mostly only gotten
as far as the front-end of the pipeline, ie. dpu_plane):

 - It looks like, as was the case with mdp4/mdp5 (and really most other hw)
   there are still unequal capabilities for planes (ie. some can do YUV,
   scaling, etc).

   But drm-hwc (or weston) isn't going to know about that, so I think we'll
   need to add support for dynamically assigning dpu_plane::pipe, similar
   to what mdp5 does w/ plane<->hwpipe.  (Which I actually added specifically
   for drm-hwc ;-))

 - I *think* we also need the same trick for combining mixers under one CRTC
   for 4k modes too?

 - in dpu_plane, and perhaps elsewhere, userspace pointers for things like CSC
   and scaling coefficients need to be annotated w/ __user.  (Except the should
   probably be blob properties instead.. and we probably need to strip out the
   custom properties and re-introduce them separately with some userspace +
   review.)

 - dpu_formats.c looks mostly like a superset of mdp_format.c, ie there is a
   similar concept to how you describe the format to hw as mdp4/mdp5.  Probably
   the additions in dpu_formats should be folded back in mdp_format.

I'm not sure how we want to balance adding new features 

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

[Rob Clark]

On Tue, Feb 13, 2018 at 4:10 AM, Tomasz Figa  wrote:
> Hi Vivek,
>
> Thanks for the patch. Please see my comments inline.
>
> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>  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() with
>> pm_runtime_get/put_suppliers() 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 
>> ---
>>  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..

So I'm happy with the pm_runtime_get/put_suppliers() approach as a
reasonable compromise.

(Perhaps specifically, attach/detach this could move inside the iommu
driver, but we still need to get/put_suppliers() for unmap(), so meh)

BR,
-R

> This is also important for the reasons I stated in my comments to
> "[PATCH v7 1/6] base: power: runtime: Export
> pm_runtime_get/put_suppliers". Quoting for everyone's convenience:
>
>>> There are however cases in which the consumer wants to power-on
>>> the supplier, but not itself.
>>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>>> to unmap a buffer and finish the TLB operations without powering
>>> on itself.
>>
>>This sounds strange to me. If the SMMU is powered down, wouldn't the
>>TLB lose its contents as well (and so no flushing needed)?
>>
>>Other than that, what kind of hardware operations would be needed
>>besides just updating the page tables from the CPU?
>>
>
> In other words, the SMMU driver can deal with hardware state based on
> return value of pm_runtime_get_sync() or pm_runtime_get_if_in_use()
> and decide whether some operations are necessary or not, e.g.
> - a state restore is necessary if the domain was powered off, but we
> are bringing the master on,
> - a flush may not be required when (un)mapping with the domain powered off,
> - etc.
>
> Best regards,
> Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v7 1/6] base: power: runtime: Export pm_runtime_get/put_suppliers

[Tomasz Figa]

On Tue, Feb 13, 2018 at 9:00 PM, Robin Murphy  wrote:
> On 13/02/18 07:44, Tomasz Figa wrote:
>>
>> Hi Vivek,
>>
>> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>>  wrote:
>>>
>>> The device link allows the pm framework to tie the supplier and
>>> consumer. So, whenever the consumer is powered-on the supplier
>>> is powered-on first.
>>>
>>> There are however cases in which the consumer wants to power-on
>>> the supplier, but not itself.
>>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>>> to unmap a buffer and finish the TLB operations without powering
>>> on itself.
>>
>>
>> This sounds strange to me. If the SMMU is powered down, wouldn't the
>> TLB lose its contents as well (and so no flushing needed)?
>
>
> Depends on implementation details - if runtime PM is actually implemented
> via external clock gating (in the absence of fine-grained power domains),
> then "suspended" TLBs might both retain state and not receive invalidation
> requests, which is really the worst case.

Agreed. That's why in "[PATCH v7 3/6] iommu/arm-smmu: Invoke
pm_runtime during probe, add/remove device" I actually suggested
managing clocks separately from runtime PM. At least until runtime PM
framework arrives at a state, where multiple power states can be
managed, i.e. full power state, clock-gated state, domain-off state.
(I think I might have seen some ongoing work on this on LWN though...)

>
>> Other than that, what kind of hardware operations would be needed
>> besides just updating the page tables from the CPU?
>
>
> Domain attach/detach also require updating SMMU hardware state (and possibly
> TLB maintenance), but don't logically require the master device itself to be
> active at the time.

Wouldn't this hardware state need to be reinitialized anyway after
respective power domain power cycles? (In other words, hardware would
only need programming if it's powered on at the moment.)

Best regards,
Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v7 3/6] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device

[Robin Murphy]

On 13/02/18 08:24, Tomasz Figa wrote:

Hi Vivek,

Thanks for the patch. Please see my comments inline.

On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
 wrote:

From: Sricharan R 

The smmu device probe/remove and add/remove master device callbacks
gets called when the smmu is not linked to its master, that is without
the context of the master device. So calling runtime apis in those places
separately.

Signed-off-by: Sricharan R 
[vivek: Cleanup pm runtime calls]
Signed-off-by: Vivek Gautam 
---
  drivers/iommu/arm-smmu.c | 42 ++
  1 file changed, 38 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 9e2f917e16c2..c024f69c1682 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -913,11 +913,15 @@ static void arm_smmu_destroy_domain_context(struct 
iommu_domain *domain)
 struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 struct arm_smmu_device *smmu = smmu_domain->smmu;
 struct arm_smmu_cfg *cfg = _domain->cfg;
-   int irq;
+   int ret, irq;

 if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
 return;

+   ret = pm_runtime_get_sync(smmu->dev);
+   if (ret)
+   return;


pm_runtime_get_sync() will return 0 if the device was powered off, 1
if it was already/still powered on or runtime PM is not compiled in,
or a negative value on error, so shouldn't the test be (ret < 0)?

Moreover, I'm actually wondering if it makes any sense to power up the
hardware just to program it and power it down again. In a system where
the IOMMU is located within a power domain, it would cause the IOMMU
block to lose its state anyway.


This is generally for the case where the SMMU internal state remains 
active, but the programming interface needs to be powered up in order to 
access it.



Actually, reflecting back on "[PATCH v7 2/6] iommu/arm-smmu: Add
pm_runtime/sleep ops", perhaps it would make more sense to just
control the clocks independently of runtime PM? Then, runtime PM could
be used for real power management, e.g. really powering the block up
and down, for further power saving.


Unfortunately that ends up pretty much unmanageable, because there are 
numerous different SMMU microarchitectures with fundamentally different 
clock/power domain schemes (multiplied by individual SoC integration 
possibilities). Since this is fundamentally a generic architectural 
driver, adding explicit clock support would probably make the whole 
thing about 50% clock code, with complicated decision trees around every 
hardware access calculating which clocks are necessary for a given 
operation on a given system. That maintainability aspect is why we've 
already nacked such a fine-grained approach in the past.


Robin.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v7 1/6] base: power: runtime: Export pm_runtime_get/put_suppliers

[Robin Murphy]

On 13/02/18 07:44, Tomasz Figa wrote:

Hi Vivek,

On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
 wrote:

The device link allows the pm framework to tie the supplier and
consumer. So, whenever the consumer is powered-on the supplier
is powered-on first.

There are however cases in which the consumer wants to power-on
the supplier, but not itself.
E.g., A Graphics or multimedia driver wants to power-on the SMMU
to unmap a buffer and finish the TLB operations without powering
on itself.


This sounds strange to me. If the SMMU is powered down, wouldn't the
TLB lose its contents as well (and so no flushing needed)?


Depends on implementation details - if runtime PM is actually 
implemented via external clock gating (in the absence of fine-grained 
power domains), then "suspended" TLBs might both retain state and not 
receive invalidation requests, which is really the worst case.



Other than that, what kind of hardware operations would be needed
besides just updating the page tables from the CPU?


Domain attach/detach also require updating SMMU hardware state (and 
possibly TLB maintenance), but don't logically require the master device 
itself to be active at the time.


Robin.
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v8 5/6] iommu/arm-smmu: Add support for qcom, smmu-v2 variant

[Tomasz Figa]

Hi Vivek,

Thanks for the patch. Please see my comments inline.

On Fri, Feb 9, 2018 at 7:57 PM, Vivek Gautam
 wrote:
> qcom,smmu-v2 is an arm,smmu-v2 implementation with specific
> clock and power requirements. This smmu core is used with
> multiple masters on msm8996, viz. mdss, video, etc.
> Add bindings for the same.
>
> Signed-off-by: Vivek Gautam 
> Reviewed-by: Rob Herring 
> ---
>
> Changes in v8:
>  - Added the missing IOMMU_OF_DECLARE declaration for "qcom,smmu-v2"
>
>  .../devicetree/bindings/iommu/arm,smmu.txt | 43 
> ++
>  drivers/iommu/arm-smmu.c   | 14 +++
>  2 files changed, 57 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.txt 
> b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> index 8a6ffce12af5..169222ae2706 100644
> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.txt
> @@ -17,10 +17,19 @@ conditions.
>  "arm,mmu-401"
>  "arm,mmu-500"
>  "cavium,smmu-v2"
> +"qcom,-smmu-v2", "qcom,smmu-v2"
>
>depending on the particular implementation and/or the
>version of the architecture implemented.
>
> +  A number of Qcom SoCs use qcom,smmu-v2 version of the IP.
> +  "qcom,-smmu-v2" represents a soc specific compatible
> +  string that should be present along with the "qcom,smmu-v2"
> +  to facilitate SoC specific clocks/power connections and to
> +  address specific bug fixes.
> +  An example string would be -
> +  "qcom,msm8996-smmu-v2", "qcom,smmu-v2".

Hmm, I remember that for  and similar wildcards we required
explicitly listing allowed values. Rob, has it changed since I
stumbled upon such thing last time (or I just got it wrong before)?

> +
>  - reg   : Base address and size of the SMMU.
>
>  - #global-interrupts : The number of global interrupts exposed by the
> @@ -71,6 +80,23 @@ conditions.
>or using stream matching with #iommu-cells = <2>, and
>may be ignored if present in such cases.
>
> +- clock-names:Should be "bus", and "iface" for "qcom,smmu-v2"
> +  implementation.
> +
> +  "bus" clock for "qcom,smmu-v2" is required for downstream
> +  bus access and for the smmu ptw.
> +
> +  "iface" clock is required to access smmu's registers 
> through
> +  the TCU's programming interface.

nit: Could we have it in a bit more structured way? E.g.

- clock-names: List of names of clocks input to the device. The
required list depends on particular implementation and is as follows:
 - for "qcom,smmu-v2":
   - "bus": clock required for downstream bus access and for the smmu ptw,
   - "iface":  clock required to access smmu's registers through the
TCU's programming interface.
 - unspecified for other implementations.

(+/- wrapping)

> +
> +- clocks: Phandles for respective clocks described by clock-names.

Phandle is just a pointer to another node. Clocks are however
specified using a phandle and a number of arguments, depending on the
clock controller. I'd change it to:

- clocks: Specifiers for all clocks listed in the clock-names
property, as per generic clock bindings.

> +
> +- power-domains:  Phandles to SMMU's power domain specifier. This is
> +  required even if SMMU belongs to the master's power
> +  domain, as the SMMU will have to be enabled and
> +  accessed before master gets enabled and linked to its
> +  SMMU.

From DT point of view, the relationship of SMMU belonging to a master
device doesn't exist. The power-domains property needs to be properly
specified for all devices within power domains represented in DT, with
an exception of the case when the parent-child relationship is
explicitly stated in DT by child devices being represented by child
nodes of the parent device node.

- power-domains: Specifiers for power domains required to be powered
on for the SMMU to operate, as per generic power domain bindings.

Best regards,
Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Tomasz Figa]

Hi Vivek,

Thanks for the patch. Please see my comments inline.

On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
 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() with
> pm_runtime_get/put_suppliers() 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 
> ---
>  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).

This is also important for the reasons I stated in my comments to
"[PATCH v7 1/6] base: power: runtime: Export
pm_runtime_get/put_suppliers". Quoting for everyone's convenience:

>> There are however cases in which the consumer wants to power-on
>> the supplier, but not itself.
>> E.g., A Graphics or multimedia driver wants to power-on the SMMU
>> to unmap a buffer and finish the TLB operations without powering
>> on itself.
>
>This sounds strange to me. If the SMMU is powered down, wouldn't the
>TLB lose its contents as well (and so no flushing needed)?
>
>Other than that, what kind of hardware operations would be needed
>besides just updating the page tables from the CPU?
>

In other words, the SMMU driver can deal with hardware state based on
return value of pm_runtime_get_sync() or pm_runtime_get_if_in_use()
and decide whether some operations are necessary or not, e.g.
- a state restore is necessary if the domain was powered off, but we
are bringing the master on,
- a flush may not be required when (un)mapping with the domain powered off,
- etc.

Best regards,
Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v7 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Tomasz Figa]

Hi Vivek,

Thanks for the patch. Please see my comments inline.

On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
 wrote:
> From: Sricharan R 
>
> Finally add the device link between the master device and
> smmu, so that the smmu gets runtime enabled/disabled only when the
> master needs it. This is done from add_device callback which gets
> called once when the master is added to the smmu.
>
> Signed-off-by: Sricharan R 
> Signed-off-by: Vivek Gautam 
> ---
>  drivers/iommu/arm-smmu.c | 16 
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index c024f69c1682..c7e924d553bd 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -215,6 +215,9 @@ struct arm_smmu_device {
>
> /* IOMMU core code handle */
> struct iommu_device iommu;
> +
> +   /* runtime PM link to master */
> +   struct device_link *link;
>  };
>
>  enum arm_smmu_context_fmt {
> @@ -1425,6 +1428,17 @@ static int arm_smmu_add_device(struct device *dev)
>
> pm_runtime_put_sync(smmu->dev);
>
> +   /*
> +* Establish the link between smmu and master, so that the
> +* smmu gets runtime enabled/disabled as per the master's
> +* needs.
> +*/
> +   smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
> +   if (!smmu->link)
> +   dev_warn(smmu->dev,
> +"Unable to create device link between %s and %s\n",
> +dev_name(smmu->dev), dev_name(dev));

How likely it is that the master can work normally even if the link
add fails? Perhaps we should just return an error here?

> +
> return 0;
>
>  out_rpm_put:
> @@ -1449,6 +1463,8 @@ static void arm_smmu_remove_device(struct device *dev)
> cfg  = fwspec->iommu_priv;
> smmu = cfg->smmu;
>
> +   device_link_del(smmu->link);

We allowed smmu->link in arm_smmu_add_device(), but here we don't
check it. Looking at the code, device_link_del() doesn't seem to check
either.

Note that this problem would go away if we fail add_device on
device_link_add() failure, as I suggested above, so no change would be
necessary.

Best regards,
Tomasz
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v7 2/6] iommu/arm-smmu: Add pm_runtime/sleep ops

[Vivek Gautam]

Hi Tomasz,


Please find my response inline below.

On Tue, Feb 13, 2018 at 1:33 PM, Tomasz Figa  wrote:
> Hi Vivek,
>
> Thanks for the patch. Please see some comments inline.
>
> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>  wrote:
>> From: Sricharan R 
>>
>> The smmu needs to be functional only when the respective
>> master's using it are active. The device_link feature
>> helps to track such functional dependencies, so that the
>> iommu gets powered when the master device enables itself
>> using pm_runtime. So by adapting the smmu driver for
>> runtime pm, above said dependency can be addressed.
>>
>> This patch adds the pm runtime/sleep callbacks to the
>> driver and also the functions to parse the smmu clocks
>> from DT and enable them in resume/suspend.
>>
>> Signed-off-by: Sricharan R 
>> Signed-off-by: Archit Taneja 
>> [vivek: Clock rework to request bulk of clocks]
>> Signed-off-by: Vivek Gautam 
>> ---
>>  drivers/iommu/arm-smmu.c | 56 
>> ++--
>>  1 file changed, 54 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 69e7c60792a8..9e2f917e16c2 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -48,6 +48,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  #include 
>>
>> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>> u32 num_global_irqs;
>> u32 num_context_irqs;
>> unsigned int*irqs;
>> +   struct clk_bulk_data*clocks;
>> +   int num_clks;
>
> nit: Perhaps "num_clocks" to be consistent with "clocks"?
>
>>
>> u32 cavium_id_base; /* Specific to 
>> Cavium */
>>
>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct 
>> arm_smmu_device *smmu)
>>  struct arm_smmu_match_data {
>> enum arm_smmu_arch_version version;
>> enum arm_smmu_implementation model;
>> +   const char * const *clks;
>> +   int num_clks;
>
> nit: Perhaps s/clks/clocks/ here or s/clocks/clks/ in struct arm_smmu_device?

Sure. Will change to s/clocks/clks/ in struct arm_smmu_device.

>
>>  };
>>
>>  #define ARM_SMMU_MATCH_DATA(name, ver, imp)\
>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
>> +static const struct arm_smmu_match_data name = { .version = ver, .model = 
>> imp }
>>
>>  ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>  ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>> @@ -2001,6 +2006,7 @@ static int arm_smmu_device_dt_probe(struct 
>> platform_device *pdev,
>> data = of_device_get_match_data(dev);
>> smmu->version = data->version;
>> smmu->model = data->model;
>> +   smmu->num_clks = data->num_clks;
>>
>> parse_driver_options(smmu);
>>
>> @@ -2039,6 +2045,28 @@ static void arm_smmu_bus_init(void)
>>  #endif
>>  }
>>
>> +static int arm_smmu_init_clks(struct arm_smmu_device *smmu)
>> +{
>> +   int i;
>> +   int num = smmu->num_clks;
>> +   const struct arm_smmu_match_data *data;
>> +
>> +   if (num < 1)
>> +   return 0;
>> +
>> +   smmu->clocks = devm_kcalloc(smmu->dev, num,
>> +   sizeof(*smmu->clocks), GFP_KERNEL);
>> +   if (!smmu->clocks)
>> +   return -ENOMEM;
>> +
>> +   data = of_device_get_match_data(smmu->dev);
>> +
>> +   for (i = 0; i < num; i++)
>> +   smmu->clocks[i].id = data->clks[i];
>
> I'd argue that arm_smmu_device_dt_probe() is a better place for all
> the code above, since this function is called regardless of whether
> the device is probed from DT or not. Going further,
> arm_smmu_device_acpi_probe() could fill smmu->num_clks and ->clocks
> using ACPI-like way (as opposed to OF match data) if necessary.

Right, it's valid to fill the data in arm_smmu_device_dt_probe().
Perhaps we can just keep the devm_clk_bulk_get() in arm_smmu_device_probe()
at the point where we are currently doing arm_smmu_init_clks().

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 majord...@vger.kernel.org
> 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
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [Freedreno] [PATCH v7 4/6] iommu/arm-smmu: Add the device_link between masters and smmu

[Vivek Gautam]

Hi Tomasz,


On Tue, Feb 13, 2018 at 2:01 PM, Tomasz Figa  wrote:
> Hi Vivek,
>
> Thanks for the patch. Please see my comments inline.

Thanks for reviewing the patch series.

>
> On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam
>  wrote:
>> From: Sricharan R 
>>
>> Finally add the device link between the master device and
>> smmu, so that the smmu gets runtime enabled/disabled only when the
>> master needs it. This is done from add_device callback which gets
>> called once when the master is added to the smmu.
>>
>> Signed-off-by: Sricharan R 
>> Signed-off-by: Vivek Gautam 
>> ---
>>  drivers/iommu/arm-smmu.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index c024f69c1682..c7e924d553bd 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -215,6 +215,9 @@ struct arm_smmu_device {
>>
>> /* IOMMU core code handle */
>> struct iommu_device iommu;
>> +
>> +   /* runtime PM link to master */
>> +   struct device_link *link;
>>  };
>>
>>  enum arm_smmu_context_fmt {
>> @@ -1425,6 +1428,17 @@ static int arm_smmu_add_device(struct device *dev)
>>
>> pm_runtime_put_sync(smmu->dev);
>>
>> +   /*
>> +* Establish the link between smmu and master, so that the
>> +* smmu gets runtime enabled/disabled as per the master's
>> +* needs.
>> +*/
>> +   smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME);
>> +   if (!smmu->link)
>> +   dev_warn(smmu->dev,
>> +"Unable to create device link between %s and %s\n",
>> +dev_name(smmu->dev), dev_name(dev));
>
> How likely it is that the master can work normally even if the link
> add fails? Perhaps we should just return an error here?

Right. We are assuming that the power is handled for most of the
smmu operations, after we add the master with smmu, based on the fact
that the device link is successful.
We should return error code here. Will make the necessary change.

>
>> +
>> return 0;
>>
>>  out_rpm_put:
>> @@ -1449,6 +1463,8 @@ static void arm_smmu_remove_device(struct device *dev)
>> cfg  = fwspec->iommu_priv;
>> smmu = cfg->smmu;
>>
>> +   device_link_del(smmu->link);
>
> We allowed smmu->link in arm_smmu_add_device(), but here we don't
> check it. Looking at the code, device_link_del() doesn't seem to check
> either.
>
> Note that this problem would go away if we fail add_device on
> device_link_add() failure, as I suggested above, so no change would be
> necessary.

Sure. After making the above change, this should also be handled.

Best regards
Vivek

>
> Best regards,
> Tomasz



-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
___
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno