| Message ID | 1475670829-14025-4-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On ke, 2016-10-05 at 13:33 +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Use types of more appropriate size in struct > intel_watermark_params to save 512 bytes of .rodata. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> The code does seem to use datatypes very sloppily. Where did you pick up these? Regards, Joonas
On 05/10/2016 16:07, Joonas Lahtinen wrote: > On ke, 2016-10-05 at 13:33 +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Use types of more appropriate size in struct >> intel_watermark_params to save 512 bytes of .rodata. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > The code does seem to use datatypes very sloppily. Where did you pick > up these? I was just nosing around in .rodata and wondered why a huge table of zeros. :) Regards, Tvrtko
On 05/10/2016 17:50, Tvrtko Ursulin wrote: > > On 05/10/2016 16:07, Joonas Lahtinen wrote: >> On ke, 2016-10-05 at 13:33 +0100, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Use types of more appropriate size in struct >>> intel_watermark_params to save 512 bytes of .rodata. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> The code does seem to use datatypes very sloppily. Where did you pick >> up these? > > I was just nosing around in .rodata and wondered why a huge table of > zeros. :) > Oops no, that was the cxsr table which stood out so much. After I found that one I nosed around some more to see which ones look wasteful. Regards, Tvrtko
On Wed, Oct 05, 2016 at 01:33:30PM +0100, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Use types of more appropriate size in struct > intel_watermark_params to save 512 bytes of .rodata. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_drv.h | 10 +++++----- > drivers/gpu/drm/i915/intel_pm.c | 4 ++-- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index c52b1d3a7ba0..59a73f8ca7af 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -799,11 +799,11 @@ struct intel_plane { > }; > > struct intel_watermark_params { > - unsigned long fifo_size; > - unsigned long max_wm; > - unsigned long default_wm; > - unsigned long guard_size; > - unsigned long cacheline_size; > + u16 fifo_size; > + u16 max_wm; > + u8 default_wm; > + u8 guard_size; > + u8 cacheline_size; > }; This thing has been bugging me since forever. And yet I never sent out a fix. We could probably shrink things furher by tossing out a bunch of the data since it's not all that diverse. But this looks like a decent first step. The other thing that bugs me about these is the defines for the actual values. Makes it just that much harder to figure out what the actual values are. I'm pretty sure I have a branch or two where I kill the defines but naturally I can't find it right now. I'm too lazy to double check the values now with this indirection in place, but your choice of types does look reasonable, so Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > struct cxsr_latency { > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 3b1f0b40ccb9..96d0c57c816c 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -775,13 +775,13 @@ static bool g4x_check_srwm(struct drm_device *dev, > display_wm, cursor_wm); > > if (display_wm > display->max_wm) { > - DRM_DEBUG_KMS("display watermark is too large(%d/%ld), disabling\n", > + DRM_DEBUG_KMS("display watermark is too large(%d/%u), disabling\n", > display_wm, display->max_wm); > return false; > } > > if (cursor_wm > cursor->max_wm) { > - DRM_DEBUG_KMS("cursor watermark is too large(%d/%ld), disabling\n", > + DRM_DEBUG_KMS("cursor watermark is too large(%d/%u), disabling\n", > cursor_wm, cursor->max_wm); > return false; > } > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On ke, 2016-10-05 at 17:53 +0100, Tvrtko Ursulin wrote: > On 05/10/2016 17:50, Tvrtko Ursulin wrote: > > > > > > On 05/10/2016 16:07, Joonas Lahtinen wrote: > > > > > > On ke, 2016-10-05 at 13:33 +0100, Tvrtko Ursulin wrote: > > > > > > > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > > > > > Use types of more appropriate size in struct > > > > intel_watermark_params to save 512 bytes of .rodata. > > > > > > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > The code does seem to use datatypes very sloppily. Where did you > > > pick > > > up these? > > > > I was just nosing around in .rodata and wondered why a huge table > > of > > zeros. :) > > > > Oops no, that was the cxsr table which stood out so much. After I > found > that one I nosed around some more to see which ones look wasteful. > I rather meant how did you deduce the right datatype sizes? Regards, Joonas
On 06/10/2016 07:58, Joonas Lahtinen wrote: > On ke, 2016-10-05 at 17:53 +0100, Tvrtko Ursulin wrote: >> On 05/10/2016 17:50, Tvrtko Ursulin wrote: >>> >>> On 05/10/2016 16:07, Joonas Lahtinen wrote: >>>> On ke, 2016-10-05 at 13:33 +0100, Tvrtko Ursulin wrote: >>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>>> >>>>> Use types of more appropriate size in struct >>>>> intel_watermark_params to save 512 bytes of .rodata. >>>>> >>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> The code does seem to use datatypes very sloppily. Where did you >>>> pick >>>> up these? >>> I was just nosing around in .rodata and wondered why a huge table >>> of >>> zeros. :) >>> >> Oops no, that was the cxsr table which stood out so much. After I >> found >> that one I nosed around some more to see which ones look wasteful. >> > I rather meant how did you deduce the right datatype sizes? > Oh right, I've looked at the values stored in them and got extra confidence when the compiler did not complain. Regards, Tvrtko
On 05/10/2016 18:11, Ville Syrjälä wrote: > On Wed, Oct 05, 2016 at 01:33:30PM +0100, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Use types of more appropriate size in struct >> intel_watermark_params to save 512 bytes of .rodata. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/intel_drv.h | 10 +++++----- >> drivers/gpu/drm/i915/intel_pm.c | 4 ++-- >> 2 files changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index c52b1d3a7ba0..59a73f8ca7af 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -799,11 +799,11 @@ struct intel_plane { >> }; >> >> struct intel_watermark_params { >> - unsigned long fifo_size; >> - unsigned long max_wm; >> - unsigned long default_wm; >> - unsigned long guard_size; >> - unsigned long cacheline_size; >> + u16 fifo_size; >> + u16 max_wm; >> + u8 default_wm; >> + u8 guard_size; >> + u8 cacheline_size; >> }; > This thing has been bugging me since forever. And yet I never sent out a > fix. We could probably shrink things furher by tossing out a bunch of > the data since it's not all that diverse. But this looks like a decent > first step. > > The other thing that bugs me about these is the defines for the actual > values. Makes it just that much harder to figure out what the actual > values are. I'm pretty sure I have a branch or two where I kill the > defines but naturally I can't find it right now. I'm too lazy to > double check the values now with this indirection in place, but your > choice of types does look reasonable, so > > Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Thanks. Btw we can shave one more kilobyte by similarly shrinking "struct video_levels" and "struct tv_mode". u8/u16 are enough to replace most of the ints there. If you want to ack that in principle I can include a patch for that in the next round. Regards, Tvrtko
On Thu, Oct 06, 2016 at 05:38:12PM +0100, Tvrtko Ursulin wrote: > > On 05/10/2016 18:11, Ville Syrjälä wrote: > > On Wed, Oct 05, 2016 at 01:33:30PM +0100, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> > >> Use types of more appropriate size in struct > >> intel_watermark_params to save 512 bytes of .rodata. > >> > >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_drv.h | 10 +++++----- > >> drivers/gpu/drm/i915/intel_pm.c | 4 ++-- > >> 2 files changed, 7 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > >> index c52b1d3a7ba0..59a73f8ca7af 100644 > >> --- a/drivers/gpu/drm/i915/intel_drv.h > >> +++ b/drivers/gpu/drm/i915/intel_drv.h > >> @@ -799,11 +799,11 @@ struct intel_plane { > >> }; > >> > >> struct intel_watermark_params { > >> - unsigned long fifo_size; > >> - unsigned long max_wm; > >> - unsigned long default_wm; > >> - unsigned long guard_size; > >> - unsigned long cacheline_size; > >> + u16 fifo_size; > >> + u16 max_wm; > >> + u8 default_wm; > >> + u8 guard_size; > >> + u8 cacheline_size; > >> }; > > This thing has been bugging me since forever. And yet I never sent out a > > fix. We could probably shrink things furher by tossing out a bunch of > > the data since it's not all that diverse. But this looks like a decent > > first step. > > > > The other thing that bugs me about these is the defines for the actual > > values. Makes it just that much harder to figure out what the actual > > values are. I'm pretty sure I have a branch or two where I kill the > > defines but naturally I can't find it right now. I'm too lazy to > > double check the values now with this indirection in place, but your > > choice of types does look reasonable, so > > > > Acked-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Thanks. > > Btw we can shave one more kilobyte by similarly shrinking "struct > video_levels" and "struct tv_mode". u8/u16 are enough to replace most of > the ints there. If you want to ack that in principle I can include a > patch for that in the next round. Sure, go for it.
On to, 2016-10-06 at 08:37 +0100, Tvrtko Ursulin wrote: > On 06/10/2016 07:58, Joonas Lahtinen wrote: > > I rather meant how did you deduce the right datatype sizes? > > > > Oh right, I've looked at the values stored in them and got extra > confidence when the compiler did not complain. The code repeatedly does ugly casts, it could be fixed in proper manner, too. Add to your TODO ;) Regards, Joonas
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c52b1d3a7ba0..59a73f8ca7af 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -799,11 +799,11 @@ struct intel_plane { }; struct intel_watermark_params { - unsigned long fifo_size; - unsigned long max_wm; - unsigned long default_wm; - unsigned long guard_size; - unsigned long cacheline_size; + u16 fifo_size; + u16 max_wm; + u8 default_wm; + u8 guard_size; + u8 cacheline_size; }; struct cxsr_latency { diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3b1f0b40ccb9..96d0c57c816c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -775,13 +775,13 @@ static bool g4x_check_srwm(struct drm_device *dev, display_wm, cursor_wm); if (display_wm > display->max_wm) { - DRM_DEBUG_KMS("display watermark is too large(%d/%ld), disabling\n", + DRM_DEBUG_KMS("display watermark is too large(%d/%u), disabling\n", display_wm, display->max_wm); return false; } if (cursor_wm > cursor->max_wm) { - DRM_DEBUG_KMS("cursor watermark is too large(%d/%ld), disabling\n", + DRM_DEBUG_KMS("cursor watermark is too large(%d/%u), disabling\n", cursor_wm, cursor->max_wm); return false; }