2016-10-01 09:24:59

by Sekhar Nori

[permalink] [raw]
Subject: Re: [PATCH 6/6] ARM: da850: adjust memory settings for tilcdc

On Saturday 01 October 2016 12:49 AM, Peter Ujfalusi wrote:
> On 09/30/2016 06:06 PM, Bartosz Golaszewski wrote:
>> 2016-09-30 14:59 GMT+02:00 Peter Ujfalusi <[email protected]>:
>>> On 09/29/16 19:31, Bartosz Golaszewski wrote:
>>>> Default memory settings of da850 do not meet the throughput/latency
>>>> requirements of tilcdc. This results in the image displayed being
>>>> incorrect and the following warning being displayed by the LCDC
>>>> drm driver:
>>>>
>>>> tilcdc da8xx_lcdc.0: tilcdc_crtc_irq(0x00000020): FIFO underfow
>>>>
>>>> Reconfigure the LCDC priority to the highest. This is a workaround
>>>> for the da850-lcdk board which has the LCD controller enabled in
>>>> the device tree, but a long-term, system-wide fix is needed for
>>>> all davinci boards.
>>>>
>>>> This patch has been modified for mainline linux. It comes from a
>>>> downstream TI release for da850[1].
>>>>
>>>> Original author: Vishwanathrao Badarkhe, Manish <[email protected]>
>>>>
>>
>> [snip]
>>
>>>
>>> Is this safe to do for all da850 boards (to change PR_OLD_COUNT from 0xff to
>>> 0x20)? Most probably it is, but this setting has nothing to do with LCDC.
>>>
>>> The whole priority configuration has nothing to do with the LCDC, it is a
>>> system level priority.
>>>
>>> Now you have lowered the eDMA3_0-TPTC0/1 priority. Audio is serviced by
>>> eDMA3_0-TPTC1. So are we going to see issues in audio if LCDC is taking the
>>> highest priority?
>>>
>>
>> Just ran a quick test with speaker-test -c2 -twav. Besides the fact
>> that the left and right channels are inverted (I'm looking into that),
>> I didn't notice any problems. Even at 1024x768 resolution, playing
>> audio at the same time seems to work fine.
>
> That's good to hear, but I think the priorities should be set:
> LCDC and EDMA30TC1 to highest priority
> EDMA30TC0 to priority 2
>
> The 0TC0 is used by MMC and if you want to play a video you might need the
> servicing TC to be higher priority then other masters.
>
> If audio playback would trigger sync losts in lcdc then we might need to move
> 0TC1 to priority 1.
>
> I agree that LCDC priority needs to be higher, but I do wonder why the default
> (5) is not working and if it is not working why it is 5...
>
> My guess is that the change in the PBBPR register is the one actually helping
> here.

Good point, Peter. If you are booting off NFS and not playing any audio,
then there is pretty much no EDMA generated traffic on the interconnect.

I would guess too that its the PBBPR setting that is making a
difference. The EDMA vs LCDC priority adjustment might be needed in
particular situations too, but specific experiments should be done to
narrow down on that being the cause.

In any case, to configure the PBBR, you will have to introduce a driver
for it in drivers/memory. Then you can set it up per board using a DT
parameter.

Thanks,
Sekhar







2016-10-03 07:14:20

by Peter Ujfalusi

[permalink] [raw]
Subject: Re: [PATCH 6/6] ARM: da850: adjust memory settings for tilcdc

On 10/01/16 12:24, Sekhar Nori wrote:
>> That's good to hear, but I think the priorities should be set:
>> LCDC and EDMA30TC1 to highest priority
>> EDMA30TC0 to priority 2
>>
>> The 0TC0 is used by MMC and if you want to play a video you might need the
>> servicing TC to be higher priority then other masters.
>>
>> If audio playback would trigger sync losts in lcdc then we might need to move
>> 0TC1 to priority 1.
>>
>> I agree that LCDC priority needs to be higher, but I do wonder why the default
>> (5) is not working and if it is not working why it is 5...
>>
>> My guess is that the change in the PBBPR register is the one actually helping
>> here.
>
> Good point, Peter. If you are booting off NFS and not playing any audio,
> then there is pretty much no EDMA generated traffic on the interconnect.

Yes, this is my understanding as well.

> I would guess too that its the PBBPR setting that is making a
> difference. The EDMA vs LCDC priority adjustment might be needed in
> particular situations too, but specific experiments should be done to
> narrow down on that being the cause.

True, we can use some conservative values for the priority, but the PBBPR
register for sure needs to be updated.

> In any case, to configure the PBBR, you will have to introduce a driver
> for it in drivers/memory. Then you can set it up per board using a DT
> parameter.

and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On
OMAP the legacy DMA API provided a call to raise the priority of the sDMA in
EMIF :o That needs to be removed and replaced.

--
P?ter

2016-10-04 13:02:52

by Kevin Hilman

[permalink] [raw]
Subject: Re: [PATCH 6/6] ARM: da850: adjust memory settings for tilcdc

Peter Ujfalusi <[email protected]> writes:

> On 10/01/16 12:24, Sekhar Nori wrote:

[...]

>> In any case, to configure the PBBR, you will have to introduce a driver
>> for it in drivers/memory. Then you can set it up per board using a DT
>> parameter.
>
> and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On
> OMAP the legacy DMA API provided a call to raise the priority of the sDMA in
> EMIF :o That needs to be removed and replaced.

Can you point us to the bindings you're referring to?

Also, a new driver in drivers/memory is fine for setting the PBBR, but
what about the SYSCFG0 registers. Are you OK with leaving those in the
init code as proposed in $SUBJECT patch?

Kevin

2016-10-05 08:22:47

by Peter Ujfalusi

[permalink] [raw]
Subject: Re: [PATCH 6/6] ARM: da850: adjust memory settings for tilcdc

On 10/04/16 16:02, Kevin Hilman wrote:
> Peter Ujfalusi <[email protected]> writes:
>
>> On 10/01/16 12:24, Sekhar Nori wrote:
>
> [...]
>
>>> In any case, to configure the PBBR, you will have to introduce a driver
>>> for it in drivers/memory. Then you can set it up per board using a DT
>>> parameter.
>>
>> and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On
>> OMAP the legacy DMA API provided a call to raise the priority of the sDMA in
>> EMIF :o That needs to be removed and replaced.
>
> Can you point us to the bindings you're referring to?

We don't have one atm.

And the DMA priority hack in legacy sDMA code is for OMAP1:
omap_set_dma_priority(). Basically it can change the sDMA priority in
OCPT1_PRIOR, OCPT2_PRIOR, EMIFF_PRIOR and EMIFS_PRIOR registers.

> Also, a new driver in drivers/memory is fine for setting the PBBR, but
> what about the SYSCFG0 registers. Are you OK with leaving those in the
> init code as proposed in $SUBJECT patch?

My problem is - as I described it in reply to Bartosz - is that for example I
don't want the LCDC to get high priority on OMAP-L138 EVM from Logic as it
does not have LCD/VGA by default. ifdef for LCDC is not good either since my
kernel have LCDC compiled in, but it is disabled.

The easiest way would be to have pdata quirk to handle the LCDK until we have
proper handling of the priority configuration.

--
P?ter

2016-10-06 17:57:59

by Sekhar Nori

[permalink] [raw]
Subject: Re: [PATCH 6/6] ARM: da850: adjust memory settings for tilcdc

On Wednesday 05 October 2016 01:52 PM, Peter Ujfalusi wrote:
> On 10/04/16 16:02, Kevin Hilman wrote:
>> Peter Ujfalusi <[email protected]> writes:
>>
>>> On 10/01/16 12:24, Sekhar Nori wrote:
>>
>> [...]
>>
>>>> In any case, to configure the PBBR, you will have to introduce a driver
>>>> for it in drivers/memory. Then you can set it up per board using a DT
>>>> parameter.
>>>
>>> and we can reuse the introduced bindings for am335x and OMAP1/2 as well. On
>>> OMAP the legacy DMA API provided a call to raise the priority of the sDMA in
>>> EMIF :o That needs to be removed and replaced.
>>
>> Can you point us to the bindings you're referring to?
>
> We don't have one atm.
>
> And the DMA priority hack in legacy sDMA code is for OMAP1:
> omap_set_dma_priority(). Basically it can change the sDMA priority in
> OCPT1_PRIOR, OCPT2_PRIOR, EMIFF_PRIOR and EMIFS_PRIOR registers.
>
>> Also, a new driver in drivers/memory is fine for setting the PBBR, but
>> what about the SYSCFG0 registers. Are you OK with leaving those in the
>> init code as proposed in $SUBJECT patch?
>
> My problem is - as I described it in reply to Bartosz - is that for example I
> don't want the LCDC to get high priority on OMAP-L138 EVM from Logic as it
> does not have LCD/VGA by default. ifdef for LCDC is not good either since my
> kernel have LCDC compiled in, but it is disabled.
>
> The easiest way would be to have pdata quirk to handle the LCDK until we have
> proper handling of the priority configuration.

We don't have pdata quirks handling in mach-davinci. And I think instead
of investing time in adding pdata quirks which are anyway a short term
solution, it is better to work on a drivers/bus/ based driver which can
help adjust master priorities via DT.

Although as Peter asked, it is indeed intriguing as to why LCDC priority
has to be raised even in supposed absence of any competing traffic.

Thanks,
Sekhar