Message-ID: <616d2aa4-96a7-4ed1-afd8-9fce85b45438@amd.com>
Date: Mon, 20 Oct 2025 14:24:33 +0100
From: Alejandro Lucero Palau <alucerop@....com>
To: "Cheatham, Benjamin" <benjamin.cheatham@....com>,
 Dave Jiang <dave.jiang@...el.com>, alejandro.lucero-palau@....com
Cc: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
 linux-cxl@...r.kernel.org, netdev@...r.kernel.org, dan.j.williams@...el.com,
 edward.cree@....com, davem@...emloft.net, kuba@...nel.org,
 pabeni@...hat.com, edumazet@...gle.com
Subject: Re: [PATCH v19 18/22] cxl: Allow region creation by type2 drivers


On 10/16/25 14:23, Cheatham, Benjamin wrote:
> On 10/15/2025 4:42 PM, Dave Jiang wrote:
>>
>> On 10/9/25 1:56 PM, Cheatham, Benjamin wrote:
>>> On 10/6/2025 5:01 AM, alejandro.lucero-palau@....com wrote:
>>>> From: Alejandro Lucero <alucerop@....com>
>>>>
>>>> Creating a CXL region requires userspace intervention through the cxl
>>>> sysfs files. Type2 support should allow accelerator drivers to create
>>>> such cxl region from kernel code.
>>>>
>>>> Adding that functionality and integrating it with current support for
>>>> memory expanders.
>>>>
>>>> Support an action by the type2 driver to be linked to the created region
>>>> for unwinding the resources allocated properly.
>>>>
>>>> Based on https://lore.kernel.org/linux-cxl/168592159835.1948938.1647215579839222774.stgit@dwillia2-xfh.jf.intel.com/
>>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@....com>
>>>> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>>>> ---
>>> Fix for this one should be split between 13/22 and this patch, but the majority of it is in this one. The idea is
>>> if we don't find a free decoder we check for pre-programmed decoders and use that instead. Unfortunately, this
>>> invalidates some of the assumptions made by __construct_new_region().
>> Wouldn't you look for a pre-programmed decoder first and construct the auto region before you try to manually create one? Also for a type 2 device, would the driver know what it wants and what the region configuration should look like? Would it be a single region either it's auto or manual, or would there be a configuration of multiple regions possible? To me a type 2 region is more intentional where the driver would know exactly what it needs and thus trying to get that from the cxl core.
>>
> Since this is a fix I didn't want to supersede the current behavior. A better solution would've been to add a flag to allow the type 2 driver
> to set up an expected region type.
>
> As for multiple regions, I have no clue. I haven't heard of any reason why a type 2 device would need multiple regions, but it's still very
> early days. I don't think there's anything in this set that keeps you from using multiple regions though.


What Dave says is correct, so Type2 should  support these two 
possibilities, an HDM decoder already programmed by the BIOS and the 
BIOS doing nothing, at least with the Type2 HDM decoders. This patchset 
supports the latter, but the former is more than possible, even if the 
logic and what we have discussed since the RFC points to type2 driver 
having the full control.


However, I would prefer to do that other support as a follow-up as the 
functionality added is enough for the initial client, the sfc driver, 
requiring this new Type2 support. The reason is clear: I do not want to 
delay this "basic Type2 support" more than necessary, and as I stated in 
another comment, I bet we will see other things to support soon, so 
better to increasingly add those after a first work set the base. Of 
course, the base needs to be right.


Thanks,

Alejandro


> Thanks,
> Ben
>
>> DJ
>>

Powered by blists - more mailing lists