Re: [PATCH 00/12] drm: Put drm_display_mode on diet

Emil Velikov
2020-02-20T17:02:17Z

On Thu, 20 Feb 2020 at 14:28, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Thu, Feb 20, 2020 at 01:21:03PM +0000, Emil Velikov wrote:
> > On Wed, 19 Feb 2020 at 20:35, Ville Syrjala
> > <ville.syrjala@linux.intel.com> wrote:
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > struct drm_display_mode is extremely fat. Put it on diet.
> > >
> > > Some stats for the whole series:
> > >
> > > 64bit sizeof(struct drm_display_mode):
> > > 200 -> 136 bytes (-32%)
> > >
> > > 64bit bloat-o-meter -c drm.ko:
> > > add/remove: 1/0 grow/shrink: 29/47 up/down: 893/-1544 (-651)
> > > Function old new delta
> > > ...
> > > Total: Before=189430, After=188779, chg -0.34%
> > > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> > > Data old new delta
> > > Total: Before=11667, After=11667, chg +0.00%
> > > add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-16896 (-16896)
> > > RO Data old new delta
> > > edid_4k_modes 1000 680 -320
> > > edid_est_modes 3400 2312 -1088
> > > edid_cea_modes_193 5400 3672 -1728
> > > drm_dmt_modes 17600 11968 -5632
> > > edid_cea_modes_1 25400 17272 -8128
> > > Total: Before=71239, After=54343, chg -23.72%
> > >
> > >
> > > 64bit bloat-o-meter drm.ko:
> > > add/remove: 1/0 grow/shrink: 29/52 up/down: 893/-18440 (-17547)
> > > ...
> > > Total: Before=272336, After=254789, chg -6.44%
> > >
> > >
> > > 32bit sizeof(struct drm_display_mode):
> > > 184 -> 120 bytes (-34%)
> > >
> > > 32bit bloat-o-meter -c drm.ko
> > > add/remove: 1/0 grow/shrink: 19/21 up/down: 743/-1368 (-625)
> > > Function old new delta
> > > ...
> > > Total: Before=172359, After=171734, chg -0.36%
> > > add/remove: 0/0 grow/shrink: 0/0 up/down: 0/0 (0)
> > > Data old new delta
> > > Total: Before=4227, After=4227, chg +0.00%
> > > add/remove: 0/0 grow/shrink: 0/5 up/down: 0/-16896 (-16896)
> > > RO Data old new delta
> > > edid_4k_modes 920 600 -320
> > > edid_est_modes 3128 2040 -1088
> > > edid_cea_modes_193 4968 3240 -1728
> > > drm_dmt_modes 16192 10560 -5632
> > > edid_cea_modes_1 23368 15240 -8128
> > > Total: Before=59230, After=42334, chg -28.53%
> > >
> > > 32bit bloat-o-meter drm.ko:
> > > add/remove: 1/0 grow/shrink: 19/26 up/down: 743/-18264 (-17521)
> > > ...
> > > Total: Before=235816, After=218295, chg -7.43%
> > >
> > >
> > > Some ideas for further reduction:
> > > - Convert mode->name to a pointer (saves 24/28 bytes in the
> > > struct but would often require a heap alloc for the name (though
> > > typical mode name is <10 bytes so still overall win perhaps)
> > > - Get rid of mode->name entirely? I guess setcrtc & co. is the only
> > > place where we have to preserve the user provided name, elsewhere
> > > could pehaps just generate on demand? Not sure how tricky this
> > > would get.
> >
> > The series does some great work, with future work reaching the cache
> > line for 64bit.
> > Doing much more than that might be an overkill IMHO.
> >
> > In particular, if we change DRM_DISPLAY_MODE_LEN to 24 we get there,
> > avoiding the heap alloc/calc on demand fun.
> > While also ensuring the name is sufficiently large for the next decade or so.
>
> Unfortunately it's part of the uabi. So can't change it without some
> risk of userspace breakage.
>
Right the define is in the uABI. More importantly userspace can
provide a drm_mode_modeinfo blob, with a name[32], which we cannot
store in the kernel drm_display_mode::name[24].

One might get away with returning -EINVAL if the actual name given by
the user is > 24.
Since you've found a better way, there's on point in risking it.

-Emil