linux-kernel - Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c6179f0b-c93f-483b-bfeb-322d800e5170@ideasonboard.com>
Date: Tue, 4 Feb 2025 17:33:32 +0200
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Devarsh Thakkar <devarsht@...com>, "jyri.sarha@....fi"
 <jyri.sarha@....fi>, "airlied@...il.com" <airlied@...il.com>,
 "maarten.lankhorst@...ux.intel.com" <maarten.lankhorst@...ux.intel.com>,
 "mripard@...nel.org" <mripard@...nel.org>,
 "tzimmermann@...e.de" <tzimmermann@...e.de>,
 "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
 "simona@...ll.ch" <simona@...ll.ch>,
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
 "robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
 <krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>
Cc: "Bajjuri, Praneeth" <praneeth@...com>,
 "Raghavendra, Vignesh" <vigneshr@...com>, "Jain, Swamil" <s-jain1@...com>,
 "Donadkar, Rishikesh" <r-donadkar@...com>,
 "Choudhary, Jayesh" <j-choudhary@...com>,
 "Shenoy, Harikrishna" <h-shenoy@...com>,
 Aradhya Bhatia <aradhya.bhatia@...ux.dev>
Subject: Re: [PATCH 2/2] drm/tidss: Add support for AM62L display subsystem

Hi,

On 28/01/2025 15:16, Devarsh Thakkar wrote:
> Hi Aradhya,
> 
> On 18/01/25 14:57, Aradhya Bhatia wrote:
>> Hi Devarsh,
>>
>> Thanks for the patches.
>>
> 
> Thanks for the review.
> 
>> On 31/12/24 14:34, Devarsh Thakkar wrote:
>>> Enable display for AM62L DSS [1] which supports only a single display
>>> pipeline using a single overlay manager, single video port and a single
>>> video lite pipeline which does not support scaling.
>>>
>>> The output of video port is routed to SoC boundary via DPI interface and
>>> the DPI signals from the video port are also routed to DSI Tx controller
>>> present within the SoC.
>>>
>>> [1]: Section 11.7 (Display Subsystem and Peripherals)
>>> Link : https://www.ti.com/lit/pdf/sprujb4
>>>
>>> Signed-off-by: Devarsh Thakkar <devarsht@...com>
>>> ---
> 
> <snip>
>>>   
>>> +const struct dispc_features dispc_am62l_feats = {
>>> +    .max_pclk_khz = {
>>> +            [DISPC_VP_DPI] = 165000,
>>
>> The TRM mentions that the max the VP PLL can go is 150MHz, so maybe you
>> might need to update this.
>>
>> That said, as far as I understand, the IP itself can support 165 MHz,
>> and I am wondering, what would we do if there comes out a new SoC that
>> uses AM62L DSS as is, but just bumps up the PLL capacity to 165MHz.
>>
>> It would be odd to have a ditto feats structure with just the frequency
>> updated.
> 
> TRM mentions max VP PLL frequency as 165 Mhz and not 150 Mhz. Please refer
> Table 11-343. DSS Signals for MIPI DPI 2.0 or BT.656/BT.1120 section in
> section 11.7.1.2.1 DSS Parallel Interface of  AM62L TRM [1].
>>
>>> +    },
>>> +
>>> +    .subrev = DISPC_AM62L,
>>> +
>>> +    .common = "common",
>>> +    .common_regs = tidss_am65x_common_regs,
>>
>> Also, I don't think we should utilize this as is.
>>
>> The AM62L common regions is different, and is, at best, a subset of the
>> AM65x/AM62x common register space.
>>
>> For example, registers VID_IRQ{STATUS, ENABLE}_0 have been dropped,
>> along with VP_IRQ{STATUS, ENABLE}_1.
>>
>> - Which brings to my next concern...
>>
> 
> Thanks for pointing out, I probably missed this since the use-case still
> worked since VP interrupts were still enabled and those were sufficient to
> drive the display
> but the VID underflow interrupts and VID specific bits were probably not
> enabled at-all due to above miss, so agreed
> we should probably go ahead with a different reg space for AM62L due to
> aforementioned differences.

I think I disagree here. Afaiu, AM62L has plane at hw index 1 (VIDL1), 
but the plane at hw index 0 (VID1) is not instantiated in the hardware. 
But the registers are the same, i.e. AM62L's registers for VIDL1 match 
AM65x/AM62x registers, right?

If so, we just need to tell the driver the hw index, instead of creating 
new register offsets as done in v2.

Or am I missing something here? (I haven't looked at the HW manual yet).

  Tomi