[v1,0/2] Break out as separate driver from boe-tv101wum-nl6 panel driver

Message

cong yang Oct. 7, 2023, 6:06 a.m. UTC

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.

[1]: https://lore.kernel.org/all/[e-mail]

Cong Yang (1):
  drm/panel: ili9882t: Avoid blurred screen from fast sleep

Linus Walleij (1):
  drm/panel: ili9882t: Break out as separate driver

 drivers/gpu/drm/panel/Kconfig                 |   9 +
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../gpu/drm/panel/panel-boe-tv101wum-nl6.c    | 371 ---------
 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c | 771 ++++++++++++++++++
 4 files changed, 781 insertions(+), 371 deletions(-)
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9882t.c

Comments

Linus Walleij Oct. 8, 2023, 7:33 p.m. UTC | #1

On Sat, Oct 7, 2023 at 8:06 AM Cong Yang
<[e-mail]> 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 <[e-mail]>

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

Yours,
Linus Walleij

Doug Anderson Oct. 9, 2023, 8:53 p.m. UTC | #2

Hi,

On Sun, Oct 8, 2023 at 12:33 PM Linus Walleij <[e-mail]> wrote:
>
> On Sat, Oct 7, 2023 at 8:06 AM Cong Yang
> <[e-mail]> 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 <[e-mail]>
>
> 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 <[e-mail]>
x-Signed-off-by: Linus Walleij <[e-mail]>
x-Reviewed-by: Linus Walleij <[e-mail]>
x-Signed-off-by: Cong Yang <[e-mail]>

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

Linus Walleij Oct. 9, 2023, 9:02 p.m. UTC | #3

On Mon, Oct 9, 2023 at 10:53 PM Doug Anderson <[e-mail]> 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

Doug Anderson Oct. 9, 2023, 9:12 p.m. UTC | #4

Hi,

On Mon, Oct 9, 2023 at 2:02 PM Linus Walleij <[e-mail]> wrote:
>
> On Mon, Oct 9, 2023 at 10:53 PM Doug Anderson <[e-mail]> 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