2023-10-08 19:44:10

by Linus Walleij

[permalink] [raw]
Subject: Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

On Sat, Oct 7, 2023 at 8:06 AM Cong Yang
<[email protected]> wrote:

> Linus series proposed to break out ili9882t as separate driver,
> but he didn't have time for that extensive rework of the driver.
> As discussed by Linus and Doug [1], keep macro using the "struct panel_init_cmd"
> until we get some resolution about the binary size issue.

OK works for me:
Reviewed-by: Linus Walleij <[email protected]>

Dough, if it looks OK to you too, can you apply the patches?

Yours,
Linus Walleij


2023-10-09 20:53:46

by Doug Anderson

[permalink] [raw]
Subject: Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

Hi,

On Sun, Oct 8, 2023 at 12:33 PM Linus Walleij <[email protected]> wrote:
>
> On Sat, Oct 7, 2023 at 8:06 AM Cong Yang
> <[email protected]> wrote:
>
> > Linus series proposed to break out ili9882t as separate driver,
> > but he didn't have time for that extensive rework of the driver.
> > As discussed by Linus and Doug [1], keep macro using the "struct panel_init_cmd"
> > until we get some resolution about the binary size issue.
>
> OK works for me:
> Reviewed-by: Linus Walleij <[email protected]>
>
> Dough, if it looks OK to you too, can you apply the patches?

Thanks for the review. I had a few small comments so I'd expect a v2.

I suspect it would look a little weird to add your Reviewed-by to
patch #1 since the way Cong has it you're the direct patch author. :-P
Cong, I suspect you may want to change the tagging on patch #1. I'd
suggest setting yourself as the patch author (git commit --amend
--reset-author), then tag the first patch like this (I put "x-" first
to make it obvious to any bots reading this that these are not tags to
actually apply--remove that when you tag your patch):

x-Co-developed-by: Linus Walleij <[email protected]>
x-Signed-off-by: Linus Walleij <[email protected]>
x-Reviewed-by: Linus Walleij <[email protected]>
x-Signed-off-by: Cong Yang <[email protected]>

Also: just as a heads up, Hsin-Yi measured the impact of removing the
"command table" for init and replacing it with a whole pile of direct
function calls. She found that it added over 100K to the driver (!!!).
I believe it went from a 45K driver to a 152K driver. Something to
keep in mind. ;-)

-Doug

2023-10-09 21:02:59

by Linus Walleij

[permalink] [raw]
Subject: Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

On Mon, Oct 9, 2023 at 10:53 PM Doug Anderson <[email protected]> wrote:

> Also: just as a heads up, Hsin-Yi measured the impact of removing the
> "command table" for init and replacing it with a whole pile of direct
> function calls. She found that it added over 100K to the driver (!!!).
> I believe it went from a 45K driver to a 152K driver. Something to
> keep in mind. ;-)

Sounds like Aarch64 code. I would love a comparison of the same
driver compiled to ARMv7t thumb code. Just for the academic
interest. Because I have heard about people running ARM32
kernels on Aarch64 hardware for this exact reason: so they can
have thumb, which is compact.

OK OK we definitely need command sequence tables in the core,
what we have now is each driver rolling its own which is looking bad.

Yours,
Linus Walleij

2023-10-09 21:13:18

by Doug Anderson

[permalink] [raw]
Subject: Re: [v1 0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

Hi,

On Mon, Oct 9, 2023 at 2:02 PM Linus Walleij <[email protected]> wrote:
>
> On Mon, Oct 9, 2023 at 10:53 PM Doug Anderson <[email protected]> wrote:
>
> > Also: just as a heads up, Hsin-Yi measured the impact of removing the
> > "command table" for init and replacing it with a whole pile of direct
> > function calls. She found that it added over 100K to the driver (!!!).
> > I believe it went from a 45K driver to a 152K driver. Something to
> > keep in mind. ;-)
>
> Sounds like Aarch64 code. I would love a comparison of the same
> driver compiled to ARMv7t thumb code. Just for the academic
> interest. Because I have heard about people running ARM32
> kernels on Aarch64 hardware for this exact reason: so they can
> have thumb, which is compact.

Yeah, thumb2 was the best.

I suspect that in addition to the aarch64 vs thumb2 part of the
problem is that mipi_dsi_dcs_write_seq() is a macro, so this wasn't
just a whole ton of function calls, but a whole ton of inline function
calls. ;-) Still, even if we fixed that, I'm not sure it we'll ever be
able to beat the space efficiency of command sequence tables.


> OK OK we definitely need command sequence tables in the core,
> what we have now is each driver rolling its own which is looking bad.

Agreed. I'd love to see someone tackle this (though not blocking
Cong's series on it). Hsin-Yi took a quick look at it and noticed that
some drivers have slightly different cases for how they handle command
sequences, which is a bit annoying. For instance, at least one driver
had an extra NOP between commands and said it was important not to
remove that. ...so we'd have to figure out how to abstract some of
these differences without it getting too ugly...

-Doug