Re: [PATCH] fbdev: atafb: Restrict SuperBlitter to supported formats

[Helge Deller <deller@gmx.de>]

On 9/7/26 22:14, Michael Schmitz wrote:
> Hi Miro,
> 
> On 7/09/26 23:14, Miro Kropáček wrote:
>> Hi Linmao, hi Michael,
>>
>> On Sun, 6 Sept 2026 at 11:49, Michael Schmitz <schmitzmic@gmail.com> wrote:
>>> Hi Linmao,
>>>
>>> thanks for your patch!
>>>
>>> Am 27.08.2026 um 21:39 schrieb Linmao Li:
>>>> The SuperBlitter operations derive an integer byte count per pixel.  The
>>>> accelerated fill path handles only one-, two- and four-byte pixels.
>>>> However, the operations are currently installed for every external
>>>> framebuffer in SuperVidel RAM, including planar 1/2/4/8-bpp and 24-bpp
>>>> truecolor modes accepted by the external video parser.
>>>>
>>>> For 1/2/4-bpp modes, the byte count becomes zero, so accelerated copies do
>>>> nothing and fills fall through to 32-bit stores.  Planar 8-bpp uses an
>>>> incompatible memory layout.  For 24-bpp modes, fills also use 32-bit stores
>>>> despite advancing addresses by three bytes per pixel.  These cases can
>>>> corrupt the framebuffer beyond the requested rectangle.
>>> I believe 24 bpp mode can be rescued using something like this (entirely
>>> untested):
>>>
>>> --- a/drivers/video/fbdev/atafb.c
>>> +++ b/drivers/video/fbdev/atafb.c
>>> @@ -2463,6 +2463,11 @@ static void svblit_fillrect(struct fb_info *info,
>>>           case 2:
>>>                   memset16((u16 *)line, pix, rect->width);
>>>                   break;
>>> +       case 3:
>>> +               memset(line, pix, ((rect->width * bytespp) % 4));
>>> +               line += ((rect->width * bytespp) % 4);
>>> +               memset32((u32 *)line, pix, (rect->width * bytespp) / 4);
>>> +               break;
>>>           default:
>>>                   memset32((u32 *)line, pix, rect->width);
>>>                   break;
>>>
>>> Can't test this on hardware, and would need Miro to confirm it works as
>>> intended.
>> Linmao's patch: I ran it on a Falcon with SuperVidel at
>> 1920x1080x32bpp, all good.
>>
>> Michael's 24bpp fill: I cannot test it because the SuperVidel has no
>> 24bpp video mode. It was planned but never implemented. Its mode
>> register offers 1/2/4/8-bit bitplane, 8-bit chunky, 16-bit highcolor
>> and 32-bit truecolor (24-bit RGB plus an alpha byte), with the
>> remaining code reserved - and TOS offers no 24bpp mode to set.
> Without a 24 bpp mode, the 24 bpp fill is pointless, so no point trying to keep that mode alive.
>> Unless I overlooked something, the fill also looks wrong on paper: the
>> first line is filled with a repeated byte and then with repeated
>> 32-bit words of the same value, which cannot reproduce a 3-byte pixel
>> pattern unless all three bytes are equal.
> 
> You're correct - we'd need a memset24() to account for that. Without hardware support this is an academic exercise that I don't have time for :-)
> 
>> On the fbset question: no, the pixel format, depth and geometry of an
>> external framebuffer cannot be changed at runtime here. fbset -depth
>> 16 and fbset -xres 1024 -yres 768 both return success and change
>> nothing, because ext_decode_var() only validates against the values
>> from the boot argument, ext_encode_var() re-derives every field from
>> the external_* globals and ext_set_par() is empty. Only a reboot with
>> a different atafb:external: argument changes the mode.
> 
> All good then - do you need my Acked-by for Linmao's patch, Helge?

No, I think your Reviewed-by is sufficient.

But I added a
Tested-by: Miro Kropáček <miro.kropacek@gmail.com>
tag, if that's Ok for you, Miro?
It's always good to know if a patch has been tested.

With that, I just added the patch to the fbdev git tree.

Thanks!
Helge