Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-07 Thread Joonas Lahtinen 
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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-06 Thread Ville Syrjälä 
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 
> >>
> >> Use types of more appropriate size in struct
> >> intel_watermark_params to save 512 bytes of .rodata.
> >>
> >> Signed-off-by: Tvrtko Ursulin 
> >> ---
> >>   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ä 
> 
> 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.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-06 Thread Tvrtko Ursulin 


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 

Use types of more appropriate size in struct
intel_watermark_params to save 512 bytes of .rodata.

Signed-off-by: Tvrtko Ursulin 
---
  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ä 


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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-06 Thread Tvrtko Ursulin 


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 

Use types of more appropriate size in struct
intel_watermark_params to save 512 bytes of .rodata.

Signed-off-by: Tvrtko Ursulin 

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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-06 Thread Joonas Lahtinen 
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 
> > > > 
> > > > Use types of more appropriate size in struct
> > > > intel_watermark_params to save 512 bytes of .rodata.
> > > > 
> > > > Signed-off-by: Tvrtko Ursulin 
> > > 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
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-05 Thread Ville Syrjälä 
On Wed, Oct 05, 2016 at 01:33:30PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Use types of more appropriate size in struct
> intel_watermark_params to save 512 bytes of .rodata.
> 
> Signed-off-by: Tvrtko Ursulin 
> ---
>  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ä 

>  
>  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

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-05 Thread Tvrtko Ursulin 


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 

Use types of more appropriate size in struct
intel_watermark_params to save 512 bytes of .rodata.

Signed-off-by: Tvrtko Ursulin 

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


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-05 Thread Tvrtko Ursulin 


On 05/10/2016 16:07, Joonas Lahtinen wrote:

On ke, 2016-10-05 at 13:33 +0100, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

Use types of more appropriate size in struct
intel_watermark_params to save 512 bytes of .rodata.

Signed-off-by: Tvrtko Ursulin 

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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-05 Thread Joonas Lahtinen 
On ke, 2016-10-05 at 13:33 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin 
> 
> Use types of more appropriate size in struct
> intel_watermark_params to save 512 bytes of .rodata.
> 
> Signed-off-by: Tvrtko Ursulin 

The code does seem to use datatypes very sloppily. Where did you pick
up these?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 03/22] drm/i915: Shrink per-platform watermark configuration

2016-10-05 Thread Tvrtko Ursulin 
From: Tvrtko Ursulin 

Use types of more appropriate size in struct
intel_watermark_params to save 512 bytes of .rodata.

Signed-off-by: Tvrtko Ursulin 
---
 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;
 };
 
 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