[v2] drm/stm: drv: Improve data transfers

Commit Message

Philippe CORNU Jan. 30, 2018, 10:42 a.m. UTC

To optimize data transfers, align pitch on 128 bytes & height
on 4 bytes. This optimization is not applicable on hw without MMU.

Signed-off-by: Yannick Fertre <[e-mail]>
Signed-off-by: Vincent Abriou <[e-mail]>
Signed-off-by: Philippe Cornu <[e-mail]>
---
Changes in v2: Rename stm_dumb_create() to stm_gem_cma_dumb_create() and
  move CONFIG_MMU inside the function following comments from Benjamin
  Gaignard.

 drivers/gpu/drm/stm/drv.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Benjamin Gaignard Jan. 30, 2018, 10:51 a.m. UTC | #1

2018-01-30 11:42 GMT+01:00 Philippe Cornu <[e-mail]>:
> To optimize data transfers, align pitch on 128 bytes & height
> on 4 bytes. This optimization is not applicable on hw without MMU.
>
> Signed-off-by: Yannick Fertre <[e-mail]>
> Signed-off-by: Vincent Abriou <[e-mail]>
> Signed-off-by: Philippe Cornu <[e-mail]>

Applied on drm-misc-next.

Regards,
Benjamin
> ---
> Changes in v2: Rename stm_dumb_create() to stm_gem_cma_dumb_create() and
>   move CONFIG_MMU inside the function following comments from Benjamin
>   Gaignard.
>
>  drivers/gpu/drm/stm/drv.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 8fe954c27fba..8bc7e8418b8d 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -31,6 +31,24 @@ static const struct drm_mode_config_funcs drv_mode_config_funcs = {
>         .atomic_commit = drm_atomic_helper_commit,
>  };
>
> +static int stm_gem_cma_dumb_create(struct drm_file *file,
> +                                  struct drm_device *dev,
> +                                  struct drm_mode_create_dumb *args)
> +{
> +#ifdef CONFIG_MMU
> +       unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +
> +       /*
> +        * in order to optimize data transfer, pitch is aligned on
> +        * 128 bytes, height is aligned on 4 bytes
> +        */
> +       args->pitch = roundup(min_pitch, 128);
> +       args->height = roundup(args->height, 4);
> +#endif
> +
> +       return drm_gem_cma_dumb_create_internal(file, dev, args);
> +}
> +
>  DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
>
>  static struct drm_driver drv_driver = {
> @@ -44,7 +62,7 @@ static struct drm_driver drv_driver = {
>         .minor = 0,
>         .patchlevel = 0,
>         .fops = &drv_driver_fops,
> -       .dumb_create = drm_gem_cma_dumb_create,
> +       .dumb_create = stm_gem_cma_dumb_create,
>         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>         .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>         .gem_free_object_unlocked = drm_gem_cma_free_object,
> --
> 2.15.1
>

Laurent Pinchart Jan. 30, 2018, 2:58 p.m. UTC | #2

Hi Philippe,

Thank you for the patch.

On Tuesday, 30 January 2018 12:42:00 EET Philippe Cornu wrote:
> To optimize data transfers, align pitch on 128 bytes & height
> on 4 bytes. This optimization is not applicable on hw without MMU.
> 
> Signed-off-by: Yannick Fertre <[e-mail]>
> Signed-off-by: Vincent Abriou <[e-mail]>
> Signed-off-by: Philippe Cornu <[e-mail]>
> ---
> Changes in v2: Rename stm_dumb_create() to stm_gem_cma_dumb_create() and
>   move CONFIG_MMU inside the function following comments from Benjamin
>   Gaignard.
> 
>  drivers/gpu/drm/stm/drv.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index 8fe954c27fba..8bc7e8418b8d 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -31,6 +31,24 @@ static const struct drm_mode_config_funcs
> drv_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit,
>  };
> 
> +static int stm_gem_cma_dumb_create(struct drm_file *file,
> +				   struct drm_device *dev,
> +				   struct drm_mode_create_dumb *args)
> +{
> +#ifdef CONFIG_MMU
> +	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> +
> +	/*
> +	 * in order to optimize data transfer, pitch is aligned on
> +	 * 128 bytes, height is aligned on 4 bytes
> +	 */
> +	args->pitch = roundup(min_pitch, 128);
> +	args->height = roundup(args->height, 4);
> +#endif
> +
> +	return drm_gem_cma_dumb_create_internal(file, dev, args);

In the !CONFIG_MMU case you now bypass the pitch and size calculations 
performed by drm_gem_cma_dumb_create(), allowing userspace to allocate 
arbitrarily large buffers. Is that intentional ?

> +}
> +
>  DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
> 
>  static struct drm_driver drv_driver = {
> @@ -44,7 +62,7 @@ static struct drm_driver drv_driver = {
>  	.minor = 0,
>  	.patchlevel = 0,
>  	.fops = &drv_driver_fops,
> -	.dumb_create = drm_gem_cma_dumb_create,
> +	.dumb_create = stm_gem_cma_dumb_create,
>  	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>  	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>  	.gem_free_object_unlocked = drm_gem_cma_free_object,

Laurent Pinchart Jan. 30, 2018, 3:01 p.m. UTC | #3

Hi Benjamin,

On Tuesday, 30 January 2018 12:51:25 EET Benjamin Gaignard wrote:
> 2018-01-30 11:42 GMT+01:00 Philippe Cornu <[e-mail]>:
> > To optimize data transfers, align pitch on 128 bytes & height
> > on 4 bytes. This optimization is not applicable on hw without MMU.
> > 
> > Signed-off-by: Yannick Fertre <[e-mail]>
> > Signed-off-by: Vincent Abriou <[e-mail]>
> > Signed-off-by: Philippe Cornu <[e-mail]>
> 
> Applied on drm-misc-next.

A 9 minutes review window is pretty small... I've been told before that I 
should then review v1 instead of complaining about short review times for v2, 
but in this case v2 introduces a bug that wasn't present in v1.

> > ---
> > Changes in v2: Rename stm_dumb_create() to stm_gem_cma_dumb_create() and
> > 
> >   move CONFIG_MMU inside the function following comments from Benjamin
> >   Gaignard.
> >  
> >  drivers/gpu/drm/stm/drv.c | 20 +++++++++++++++++++-
> >  1 file changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> > index 8fe954c27fba..8bc7e8418b8d 100644
> > --- a/drivers/gpu/drm/stm/drv.c
> > +++ b/drivers/gpu/drm/stm/drv.c
> > @@ -31,6 +31,24 @@ static const struct drm_mode_config_funcs
> > drv_mode_config_funcs = {> 
> >         .atomic_commit = drm_atomic_helper_commit,
> >  
> >  };
> > 
> > +static int stm_gem_cma_dumb_create(struct drm_file *file,
> > +                                  struct drm_device *dev,
> > +                                  struct drm_mode_create_dumb *args)
> > +{
> > +#ifdef CONFIG_MMU
> > +       unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> > +
> > +       /*
> > +        * in order to optimize data transfer, pitch is aligned on
> > +        * 128 bytes, height is aligned on 4 bytes
> > +        */
> > +       args->pitch = roundup(min_pitch, 128);
> > +       args->height = roundup(args->height, 4);
> > +#endif
> > +
> > +       return drm_gem_cma_dumb_create_internal(file, dev, args);
> > +}
> > +
> > 
> >  DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
> >  
> >  static struct drm_driver drv_driver = {
> > 
> > @@ -44,7 +62,7 @@ static struct drm_driver drv_driver = {
> > 
> >         .minor = 0,
> >         .patchlevel = 0,
> >         .fops = &drv_driver_fops,
> > 
> > -       .dumb_create = drm_gem_cma_dumb_create,
> > +       .dumb_create = stm_gem_cma_dumb_create,
> > 
> >         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> >         .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> >         .gem_free_object_unlocked = drm_gem_cma_free_object,
> > 
> > --
> > 2.15.1

Benjamin Gaignard Jan. 30, 2018, 3:08 p.m. UTC | #4

2018-01-30 15:58 GMT+01:00 Laurent Pinchart <[e-mail]>:
> Hi Philippe,
>
> Thank you for the patch.
>
> On Tuesday, 30 January 2018 12:42:00 EET Philippe Cornu wrote:
>> To optimize data transfers, align pitch on 128 bytes & height
>> on 4 bytes. This optimization is not applicable on hw without MMU.
>>
>> Signed-off-by: Yannick Fertre <[e-mail]>
>> Signed-off-by: Vincent Abriou <[e-mail]>
>> Signed-off-by: Philippe Cornu <[e-mail]>
>> ---
>> Changes in v2: Rename stm_dumb_create() to stm_gem_cma_dumb_create() and
>>   move CONFIG_MMU inside the function following comments from Benjamin
>>   Gaignard.
>>
>>  drivers/gpu/drm/stm/drv.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
>> index 8fe954c27fba..8bc7e8418b8d 100644
>> --- a/drivers/gpu/drm/stm/drv.c
>> +++ b/drivers/gpu/drm/stm/drv.c
>> @@ -31,6 +31,24 @@ static const struct drm_mode_config_funcs
>> drv_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit,
>>  };
>>
>> +static int stm_gem_cma_dumb_create(struct drm_file *file,
>> +                                struct drm_device *dev,
>> +                                struct drm_mode_create_dumb *args)
>> +{
>> +#ifdef CONFIG_MMU
>> +     unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>> +
>> +     /*
>> +      * in order to optimize data transfer, pitch is aligned on
>> +      * 128 bytes, height is aligned on 4 bytes
>> +      */
>> +     args->pitch = roundup(min_pitch, 128);
>> +     args->height = roundup(args->height, 4);
>> +#endif
>> +
>> +     return drm_gem_cma_dumb_create_internal(file, dev, args);
>
> In the !CONFIG_MMU case you now bypass the pitch and size calculations
> performed by drm_gem_cma_dumb_create(), allowing userspace to allocate
> arbitrarily large buffers. Is that intentional ?

My bad, I will send a fix

>
>> +}
>> +
>>  DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
>>
>>  static struct drm_driver drv_driver = {
>> @@ -44,7 +62,7 @@ static struct drm_driver drv_driver = {
>>       .minor = 0,
>>       .patchlevel = 0,
>>       .fops = &drv_driver_fops,
>> -     .dumb_create = drm_gem_cma_dumb_create,
>> +     .dumb_create = stm_gem_cma_dumb_create,
>>       .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>       .gem_free_object_unlocked = drm_gem_cma_free_object,
>
> --
> Regards,
>
> Laurent Pinchart
>

Laurent Pinchart Jan. 30, 2018, 3:10 p.m. UTC | #5

Hi Benjamin,

On Tuesday, 30 January 2018 17:08:48 EET Benjamin Gaignard wrote:
> 2018-01-30 15:58 GMT+01:00 Laurent Pinchart:
> > On Tuesday, 30 January 2018 12:42:00 EET Philippe Cornu wrote:
> >> To optimize data transfers, align pitch on 128 bytes & height
> >> on 4 bytes. This optimization is not applicable on hw without MMU.
> >> 
> >> Signed-off-by: Yannick Fertre <[e-mail]>
> >> Signed-off-by: Vincent Abriou <[e-mail]>
> >> Signed-off-by: Philippe Cornu <[e-mail]>
> >> ---
> >> Changes in v2: Rename stm_dumb_create() to stm_gem_cma_dumb_create() and
> >> 
> >>   move CONFIG_MMU inside the function following comments from Benjamin
> >>   Gaignard.
> >>  
> >>  drivers/gpu/drm/stm/drv.c | 20 +++++++++++++++++++-
> >>  1 file changed, 19 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> >> index 8fe954c27fba..8bc7e8418b8d 100644
> >> --- a/drivers/gpu/drm/stm/drv.c
> >> +++ b/drivers/gpu/drm/stm/drv.c
> >> @@ -31,6 +31,24 @@ static const struct drm_mode_config_funcs
> >> drv_mode_config_funcs = { .atomic_commit = drm_atomic_helper_commit,
> >> 
> >>  };
> >> 
> >> +static int stm_gem_cma_dumb_create(struct drm_file *file,
> >> +                                struct drm_device *dev,
> >> +                                struct drm_mode_create_dumb *args)
> >> +{
> >> +#ifdef CONFIG_MMU
> >> +     unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
> >> +
> >> +     /*
> >> +      * in order to optimize data transfer, pitch is aligned on
> >> +      * 128 bytes, height is aligned on 4 bytes
> >> +      */
> >> +     args->pitch = roundup(min_pitch, 128);
> >> +     args->height = roundup(args->height, 4);
> >> +#endif
> >> +
> >> +     return drm_gem_cma_dumb_create_internal(file, dev, args);
> > 
> > In the !CONFIG_MMU case you now bypass the pitch and size calculations
> > performed by drm_gem_cma_dumb_create(), allowing userspace to allocate
> > arbitrarily large buffers. Is that intentional ?
> 
> My bad, I will send a fix

Thank you.

> >> +}
> >> +
> >> 
> >>  DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
> >>  
> >>  static struct drm_driver drv_driver = {
> >> 
> >> @@ -44,7 +62,7 @@ static struct drm_driver drv_driver = {
> >> 
> >>       .minor = 0,
> >>       .patchlevel = 0,
> >>       .fops = &drv_driver_fops,
> >> 
> >> -     .dumb_create = drm_gem_cma_dumb_create,
> >> +     .dumb_create = stm_gem_cma_dumb_create,
> >> 
> >>       .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> >>       .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> >>       .gem_free_object_unlocked = drm_gem_cma_free_object,

Jani Nikula Jan. 30, 2018, 3:28 p.m. UTC | #6

On Tue, 30 Jan 2018, Laurent Pinchart <[e-mail]> wrote:
> Hi Benjamin,
>
> On Tuesday, 30 January 2018 12:51:25 EET Benjamin Gaignard wrote:
>> 2018-01-30 11:42 GMT+01:00 Philippe Cornu <[e-mail]>:
>> > To optimize data transfers, align pitch on 128 bytes & height
>> > on 4 bytes. This optimization is not applicable on hw without MMU.
>> > 
>> > Signed-off-by: Yannick Fertre <[e-mail]>
>> > Signed-off-by: Vincent Abriou <[e-mail]>
>> > Signed-off-by: Philippe Cornu <[e-mail]>
>> 
>> Applied on drm-misc-next.
>
> A 9 minutes review window is pretty small... I've been told before that I 
> should then review v1 instead of complaining about short review times for v2, 
> but in this case v2 introduces a bug that wasn't present in v1.

IMO no patch ever needs to be applied this fast. Give more eyeballs a
chance.

BR,
Jani.


>
>> > ---
>> > Changes in v2: Rename stm_dumb_create() to stm_gem_cma_dumb_create() and
>> > 
>> >   move CONFIG_MMU inside the function following comments from Benjamin
>> >   Gaignard.
>> >  
>> >  drivers/gpu/drm/stm/drv.c | 20 +++++++++++++++++++-
>> >  1 file changed, 19 insertions(+), 1 deletion(-)
>> > 
>> > diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
>> > index 8fe954c27fba..8bc7e8418b8d 100644
>> > --- a/drivers/gpu/drm/stm/drv.c
>> > +++ b/drivers/gpu/drm/stm/drv.c
>> > @@ -31,6 +31,24 @@ static const struct drm_mode_config_funcs
>> > drv_mode_config_funcs = {> 
>> >         .atomic_commit = drm_atomic_helper_commit,
>> >  
>> >  };
>> > 
>> > +static int stm_gem_cma_dumb_create(struct drm_file *file,
>> > +                                  struct drm_device *dev,
>> > +                                  struct drm_mode_create_dumb *args)
>> > +{
>> > +#ifdef CONFIG_MMU
>> > +       unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
>> > +
>> > +       /*
>> > +        * in order to optimize data transfer, pitch is aligned on
>> > +        * 128 bytes, height is aligned on 4 bytes
>> > +        */
>> > +       args->pitch = roundup(min_pitch, 128);
>> > +       args->height = roundup(args->height, 4);
>> > +#endif
>> > +
>> > +       return drm_gem_cma_dumb_create_internal(file, dev, args);
>> > +}
>> > +
>> > 
>> >  DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
>> >  
>> >  static struct drm_driver drv_driver = {
>> > 
>> > @@ -44,7 +62,7 @@ static struct drm_driver drv_driver = {
>> > 
>> >         .minor = 0,
>> >         .patchlevel = 0,
>> >         .fops = &drv_driver_fops,
>> > 
>> > -       .dumb_create = drm_gem_cma_dumb_create,
>> > +       .dumb_create = stm_gem_cma_dumb_create,
>> > 
>> >         .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>> >         .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> >         .gem_free_object_unlocked = drm_gem_cma_free_object,
>> > 
>> > --
>> > 2.15.1

Patch

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index 8fe954c27fba..8bc7e8418b8d 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -31,6 +31,24 @@  static const struct drm_mode_config_funcs drv_mode_config_funcs = {
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
+static int stm_gem_cma_dumb_create(struct drm_file *file,
+				   struct drm_device *dev,
+				   struct drm_mode_create_dumb *args)
+{
+#ifdef CONFIG_MMU
+	unsigned int min_pitch = DIV_ROUND_UP(args->width * args->bpp, 8);
+
+	/*
+	 * in order to optimize data transfer, pitch is aligned on
+	 * 128 bytes, height is aligned on 4 bytes
+	 */
+	args->pitch = roundup(min_pitch, 128);
+	args->height = roundup(args->height, 4);
+#endif
+
+	return drm_gem_cma_dumb_create_internal(file, dev, args);
+}
+
 DEFINE_DRM_GEM_CMA_FOPS(drv_driver_fops);
 
 static struct drm_driver drv_driver = {
@@ -44,7 +62,7 @@  static struct drm_driver drv_driver = {
 	.minor = 0,
 	.patchlevel = 0,
 	.fops = &drv_driver_fops,
-	.dumb_create = drm_gem_cma_dumb_create,
+	.dumb_create = stm_gem_cma_dumb_create,
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_free_object_unlocked = drm_gem_cma_free_object,