Re: [PATCH v1 1/4] video: simplefb-client: switch to dev_get_resource

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Ivaylo,

On 7/30/25 13:28, Ivaylo Ivanov wrote:
> On 7/30/25 11:11, Ahmad Fatoum wrote:
>> Hi Ivaylo,
>>
>> thanks for your patch.
>>
>> On 7/29/25 22:36, Ivaylo Ivanov wrote:
>>> Since the framebuffer memory resource resides in ram, it has already
>>> been requested and mapped, so only get the resource to avoid
>>> requesting a busy resource. This is also the approach for linux.
>> even if it's regular RAM (as opposed to video memory) we aren't
>> guaranteed, it's mapped suitably.
> 
> In my case at least, the mainline device tree covers all of dram -
> (0x80000000 + n GB)
> 
>>
>> I think a remap of the region to writecombine is in order here. It
>> probably works right now, because we flush caches before shutting down
>> the MMU, but if you were to run fbtest, I'd expect that you will see
>> artifacts due to some pixels remaining in dirty cache line.
> 
> So you suggest remapping it, like ipufb for example?
> 
>     fbi->info.screen_base = dma_alloc_writecombine(DMA_DEVICE_BROKEN,
>                            info->line_length * info->yres,
>                            DMA_ADDRESS_BROKEN);

Except for dma_alloc_writecombine allocating a new buffer instead of
remapping an existing region. See the patch I just Cc'd you on.

>>> While at it, use IOMEM for mem->start and drop an unnecessary newline.
>>>
>>> Signed-off-by: Ivaylo Ivanov <ivo.ivanov.ivanov1@gmail.com>
>> Patch is correct, except for that aspect of the commit message:
> 
> I don't get it, do I have to change the commit message only, or
> code logic too?

Just correct the commit message: s/Since/If/.

The writecombine thing is a separate issue.

Cheers,
Ahmad

> 
> Thanks and best regards,
> Ivaylo
> 
>>
>> Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>
>> Thanks,
>> Ahmad
>>
>>> ---
>>>  drivers/video/simplefb-client.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/video/simplefb-client.c b/drivers/video/simplefb-client.c
>>> index dafec617..41ad8ffa 100644
>>> --- a/drivers/video/simplefb-client.c
>>> +++ b/drivers/video/simplefb-client.c
>>> @@ -96,7 +96,7 @@ static int simplefb_probe(struct device *dev)
>>>  	if (ret)
>>>  		return ret;
>>>  
>>> -	mem = dev_request_mem_resource(dev, 0);
>>> +	mem = dev_get_resource(dev, IORESOURCE_MEM, 0);
>>>  	if (IS_ERR(mem)) {
>>>  		dev_err(dev, "No memory resource\n");
>>>  		return PTR_ERR(mem);
>>> @@ -116,10 +116,9 @@ static int simplefb_probe(struct device *dev)
>>>  	info->blue = params.format->blue;
>>>  	info->transp = params.format->transp;
>>>  
>>> -	info->screen_base = (void *)mem->start;
>>> +	info->screen_base = IOMEM(mem->start);
>>>  	info->screen_size = resource_size(mem);
>>>  
>>> -
>>>  	info->fbops = &simplefb_ops;
>>>  
>>>  	info->dev.parent = dev;
> 
> 

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |