[PATCH] drm/framebuffer: Expose only modifiers that support at least a format

[PATCH] drm/framebuffer: Expose only modifiers that support at least a format

[Dhinakaran Pandiyan]

Allows drivers to pass a larger modifier array, thereby avoiding
declarations of static modifier arrays that are only slight different
for each plane.

Cc: dri-devel@lists.freedesktop.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_plane.c | 35 +++++++++++++++++++++++++++--------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 1fa98bd12003..1546ffbf8e36 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -179,8 +179,8 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 			     const char *name, ...)
 {
 	struct drm_mode_config *config = &dev->mode_config;
-	unsigned int format_modifier_count = 0;
-	int ret;
+	unsigned int format_modifier_count, in_modifier_count = 0;
+	int ret, i;
 
 	/* plane index is used with 32bit bitmasks */
 	if (WARN_ON(config->num_total_plane >= 32))
@@ -216,22 +216,43 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	if (format_modifiers) {
 		const uint64_t *temp_modifiers = format_modifiers;
+
 		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
-			format_modifier_count++;
+			in_modifier_count++;
 	}
 
-	plane->modifier_count = format_modifier_count;
-	plane->modifiers = kmalloc_array(format_modifier_count,
+	plane->modifiers = kmalloc_array(in_modifier_count,
 					 sizeof(format_modifiers[0]),
 					 GFP_KERNEL);
 
-	if (format_modifier_count && !plane->modifiers) {
+	if (in_modifier_count && !plane->modifiers) {
 		DRM_DEBUG_KMS("out of memory when allocating plane\n");
 		kfree(plane->format_types);
 		drm_mode_object_unregister(dev, &plane->base);
 		return -ENOMEM;
 	}
 
+	for (i = 0, format_modifier_count = 0; i < in_modifier_count; i++) {
+		int j;
+
+		for (j = 0; funcs->format_mod_supported && j < format_count; j++)
+			if (funcs->format_mod_supported(plane, formats[j],
+							format_modifiers[i]))
+				break;
+
+		if (j < format_count)
+			plane->modifiers[format_modifier_count++] =
+				format_modifiers[i];
+	}
+
+	if (format_modifier_count < in_modifier_count) {
+		size_t size;
+
+		size = format_modifier_count * sizeof(format_modifiers[0]);
+		plane->modifiers = krealloc(plane->modifiers, size, GFP_KERNEL);
+	}
+	plane->modifier_count = format_modifier_count;
+
 	if (name) {
 		va_list ap;
 
@@ -251,8 +272,6 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
 
 	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
 	plane->format_count = format_count;
-	memcpy(plane->modifiers, format_modifiers,
-	       format_modifier_count * sizeof(format_modifiers[0]));
 	plane->possible_crtcs = possible_crtcs;
 	plane->type = type;
 
-- 
2.14.1

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

✗ Fi.CI.CHECKPATCH: warning for drm/framebuffer: Expose only modifiers that support at least a format

[Patchwork]

== Series Details ==

Series: drm/framebuffer: Expose only modifiers that support at least a format
URL   : https://patchwork.freedesktop.org/series/52064/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
dcf2012746d9 drm/framebuffer: Expose only modifiers that support at least a format
-:75: WARNING:KREALLOC_ARG_REUSE: Reusing the krealloc arg is almost always a bug
#75: FILE: drivers/gpu/drm/drm_plane.c:252:
+		plane->modifiers = krealloc(plane->modifiers, size, GFP_KERNEL);

total: 0 errors, 1 warnings, 0 checks, 65 lines checked

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

✓ Fi.CI.BAT: success for drm/framebuffer: Expose only modifiers that support at least a format

[Patchwork]

== Series Details ==

Series: drm/framebuffer: Expose only modifiers that support at least a format
URL   : https://patchwork.freedesktop.org/series/52064/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5090 -> Patchwork_10732 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/52064/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10732 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_switch@basic-default:
      fi-icl-u:           NOTRUN -> DMESG-FAIL (fdo#108535)

    igt@kms_pipe_crc_basic@read-crc-pipe-a:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362)

    igt@kms_pipe_crc_basic@read-crc-pipe-b:
      fi-snb-2520m:       NOTRUN -> DMESG-FAIL (fdo#103713)

    igt@kms_pipe_crc_basic@read-crc-pipe-b-frame-sequence:
      fi-snb-2520m:       NOTRUN -> INCOMPLETE (fdo#103713)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s4-devices:
      fi-blb-e6850:       INCOMPLETE (fdo#107718) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a:
      fi-snb-2520m:       DMESG-FAIL (fdo#103713) -> PASS

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107718 https://bugs.freedesktop.org/show_bug.cgi?id=107718
  fdo#108535 https://bugs.freedesktop.org/show_bug.cgi?id=108535


== Participating hosts (50 -> 46) ==

  Additional (1): fi-icl-u 
  Missing    (5): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 


== Build changes ==

    * Linux: CI_DRM_5090 -> Patchwork_10732

  CI_DRM_5090: 756a0fd616c3ea0486f5c239f7801f71303ff389 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4709: 15dff9353621d0746b80fae534c20621e03a9f01 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10732: dcf2012746d903fe3fee1df7f69b31c1a54f7471 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

dcf2012746d9 drm/framebuffer: Expose only modifiers that support at least a format

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10732/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/framebuffer: Expose only modifiers that support at least a format

[Patchwork]

== Series Details ==

Series: drm/framebuffer: Expose only modifiers that support at least a format
URL   : https://patchwork.freedesktop.org/series/52064/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5090_full -> Patchwork_10732_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10732_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10732_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10732_full:

  === IGT changes ===

    ==== Warnings ====

    igt@perf_pmu@rc6:
      shard-kbl:          PASS -> SKIP

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_10732_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_suspend@shrink:
      shard-apl:          PASS -> INCOMPLETE (fdo#106886, fdo#103927)

    igt@gem_cpu_reloc@full:
      shard-skl:          PASS -> INCOMPLETE (fdo#108073)

    igt@kms_atomic_transition@1x-modeset-transitions-fencing:
      shard-skl:          PASS -> FAIL (fdo#107815, fdo#108470)

    igt@kms_busy@extended-modeset-hang-newfb-render-a:
      shard-glk:          NOTRUN -> DMESG-WARN (fdo#107956)

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-b:
      shard-snb:          NOTRUN -> DMESG-WARN (fdo#107956) +2

    igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
      shard-glk:          PASS -> FAIL (fdo#108145)

    igt@kms_color@pipe-b-degamma:
      shard-skl:          NOTRUN -> FAIL (fdo#104782)

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-skl:          PASS -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-64x21-random:
      shard-apl:          PASS -> FAIL (fdo#103232) +2
      shard-glk:          PASS -> FAIL (fdo#103232) +1

    igt@kms_flip_tiling@flip-changes-tiling-yf:
      shard-skl:          PASS -> FAIL (fdo#108303, fdo#108228)

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-skl:          PASS -> FAIL (fdo#107931)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-move:
      shard-apl:          PASS -> FAIL (fdo#103167)

    igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
      shard-glk:          PASS -> FAIL (fdo#103167) +1

    igt@kms_frontbuffer_tracking@fbc-stridechange:
      shard-skl:          NOTRUN -> FAIL (fdo#105683)

    igt@kms_plane@plane-panning-bottom-right-suspend-pipe-a-planes:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#107773)

    igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
      shard-skl:          NOTRUN -> FAIL (fdo#108145) +1

    igt@kms_plane_alpha_blend@pipe-b-alpha-7efc:
      shard-skl:          NOTRUN -> FAIL (fdo#107815, fdo#108145)

    igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
      shard-skl:          PASS -> FAIL (fdo#107815)

    igt@kms_plane_multiple@atomic-pipe-a-tiling-yf:
      shard-apl:          PASS -> FAIL (fdo#103166)

    igt@kms_rotation_crc@primary-rotation-180:
      shard-skl:          PASS -> FAIL (fdo#103925, fdo#107815)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)
      shard-snb:          NOTRUN -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_suspend@shrink:
      shard-snb:          INCOMPLETE (fdo#106886, fdo#105411) -> PASS

    igt@kms_busy@extended-pageflip-modeset-hang-oldfb-render-c:
      shard-kbl:          DMESG-WARN (fdo#107956) -> PASS

    igt@kms_cursor_crc@cursor-128x128-random:
      shard-apl:          FAIL (fdo#103232) -> PASS +2

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
      shard-apl:          FAIL (fdo#103167) -> PASS +3

    igt@kms_frontbuffer_tracking@fbc-1p-rte:
      shard-apl:          FAIL (fdo#103167, fdo#105682) -> PASS

    igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-indfb-msflip-blt:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
      shard-glk:          FAIL (fdo#108145) -> PASS

    igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
      shard-apl:          FAIL (fdo#103166) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
      shard-glk:          FAIL (fdo#103166) -> PASS

    igt@kms_rotation_crc@primary-rotation-180:
      shard-glk:          INCOMPLETE (fdo#103359, k.org#198133) -> PASS

    igt@kms_sysfs_edid_timing:
      shard-apl:          FAIL (fdo#100047) -> PASS

    
  fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#104782 https://bugs.freedesktop.org/show_bug.cgi?id=104782
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#105682 https://bugs.freedesktop.org/show_bug.cgi?id=105682
  fdo#105683 https://bugs.freedesktop.org/show_bug.cgi?id=105683
  fdo#106886 https://bugs.freedesktop.org/show_bug.cgi?id=106886
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107931 https://bugs.freedesktop.org/show_bug.cgi?id=107931
  fdo#107956 https://bugs.freedesktop.org/show_bug.cgi?id=107956
  fdo#108073 https://bugs.freedesktop.org/show_bug.cgi?id=108073
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108228 https://bugs.freedesktop.org/show_bug.cgi?id=108228
  fdo#108303 https://bugs.freedesktop.org/show_bug.cgi?id=108303
  fdo#108470 https://bugs.freedesktop.org/show_bug.cgi?id=108470
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
  k.org#198133 https://bugzilla.kernel.org/show_bug.cgi?id=198133


== Participating hosts (6 -> 6) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_5090 -> Patchwork_10732

  CI_DRM_5090: 756a0fd616c3ea0486f5c239f7801f71303ff389 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4709: 15dff9353621d0746b80fae534c20621e03a9f01 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10732: dcf2012746d903fe3fee1df7f69b31c1a54f7471 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10732/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/framebuffer: Expose only modifiers that support at least a format

[Ville Syrjälä]

On Mon, Nov 05, 2018 at 06:44:34PM -0800, Dhinakaran Pandiyan wrote:
> Allows drivers to pass a larger modifier array, thereby avoiding
> declarations of static modifier arrays that are only slight different
> for each plane.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_plane.c | 35 +++++++++++++++++++++++++++--------
>  1 file changed, 27 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index 1fa98bd12003..1546ffbf8e36 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -179,8 +179,8 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  			     const char *name, ...)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
> -	unsigned int format_modifier_count = 0;
> -	int ret;
> +	unsigned int format_modifier_count, in_modifier_count = 0;
> +	int ret, i;
>  
>  	/* plane index is used with 32bit bitmasks */
>  	if (WARN_ON(config->num_total_plane >= 32))
> @@ -216,22 +216,43 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	if (format_modifiers) {
>  		const uint64_t *temp_modifiers = format_modifiers;
> +
>  		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> -			format_modifier_count++;
> +			in_modifier_count++;
>  	}
>  
> -	plane->modifier_count = format_modifier_count;
> -	plane->modifiers = kmalloc_array(format_modifier_count,
> +	plane->modifiers = kmalloc_array(in_modifier_count,
>  					 sizeof(format_modifiers[0]),
>  					 GFP_KERNEL);
>  
> -	if (format_modifier_count && !plane->modifiers) {
> +	if (in_modifier_count && !plane->modifiers) {
>  		DRM_DEBUG_KMS("out of memory when allocating plane\n");
>  		kfree(plane->format_types);
>  		drm_mode_object_unregister(dev, &plane->base);
>  		return -ENOMEM;
>  	}
>  
> +	for (i = 0, format_modifier_count = 0; i < in_modifier_count; i++) {
> +		int j;
> +
> +		for (j = 0; funcs->format_mod_supported && j < format_count; j++)
> +			if (funcs->format_mod_supported(plane, formats[j],
> +							format_modifiers[i]))
> +				break;
> +
> +		if (j < format_count)
> +			plane->modifiers[format_modifier_count++] =
> +				format_modifiers[i];
> +	}
> +
> +	if (format_modifier_count < in_modifier_count) {
> +		size_t size;
> +
> +		size = format_modifier_count * sizeof(format_modifiers[0]);
> +		plane->modifiers = krealloc(plane->modifiers, size, GFP_KERNEL);

Should check that the realloc actually succeeded.

And I think we might want to give this same treatment to plane->formats[]
as well.

And perhaps we could even throw out plane->modifiers[] and just rely on
the IN_FORMATS blob exclusively? Hmm. Looks like that is not getting fully
populated unless the driver has provided .format_mod_supported(). Not
sure why that is, and not sure what userspace is supposed to do with a
partially filled blob like that. I'm thinking we shouldn't even attach
the property to the plane in that case.

> +	}
> +	plane->modifier_count = format_modifier_count;
> +
>  	if (name) {
>  		va_list ap;
>  
> @@ -251,8 +272,6 @@ int drm_universal_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  
>  	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
>  	plane->format_count = format_count;
> -	memcpy(plane->modifiers, format_modifiers,
> -	       format_modifier_count * sizeof(format_modifiers[0]));
>  	plane->possible_crtcs = possible_crtcs;
>  	plane->type = type;
>  
> -- 
> 2.14.1

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

Re: [PATCH] drm/framebuffer: Expose only modifiers that support at least a format

[Dhinakaran Pandiyan]

On Tue, 2018-11-06 at 16:13 +0200, Ville Syrjälä wrote:
> On Mon, Nov 05, 2018 at 06:44:34PM -0800, Dhinakaran Pandiyan wrote:
> > Allows drivers to pass a larger modifier array, thereby avoiding
> > declarations of static modifier arrays that are only slight
> > different
> > for each plane.
> > 
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/drm_plane.c | 35 +++++++++++++++++++++++++++----
> > ----
> >  1 file changed, 27 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_plane.c
> > b/drivers/gpu/drm/drm_plane.c
> > index 1fa98bd12003..1546ffbf8e36 100644
> > --- a/drivers/gpu/drm/drm_plane.c
> > +++ b/drivers/gpu/drm/drm_plane.c
> > @@ -179,8 +179,8 @@ int drm_universal_plane_init(struct drm_device
> > *dev, struct drm_plane *plane,
> >  			     const char *name, ...)
> >  {
> >  	struct drm_mode_config *config = &dev->mode_config;
> > -	unsigned int format_modifier_count = 0;
> > -	int ret;
> > +	unsigned int format_modifier_count, in_modifier_count = 0;
> > +	int ret, i;
> >  
> >  	/* plane index is used with 32bit bitmasks */
> >  	if (WARN_ON(config->num_total_plane >= 32))
> > @@ -216,22 +216,43 @@ int drm_universal_plane_init(struct
> > drm_device *dev, struct drm_plane *plane,
> >  
> >  	if (format_modifiers) {
> >  		const uint64_t *temp_modifiers = format_modifiers;
> > +
> >  		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> > -			format_modifier_count++;
> > +			in_modifier_count++;
> >  	}
> >  
> > -	plane->modifier_count = format_modifier_count;
> > -	plane->modifiers = kmalloc_array(format_modifier_count,
> > +	plane->modifiers = kmalloc_array(in_modifier_count,
> >  					 sizeof(format_modifiers[0]),
> >  					 GFP_KERNEL);
> >  
> > -	if (format_modifier_count && !plane->modifiers) {
> > +	if (in_modifier_count && !plane->modifiers) {
> >  		DRM_DEBUG_KMS("out of memory when allocating plane\n");
> >  		kfree(plane->format_types);
> >  		drm_mode_object_unregister(dev, &plane->base);
> >  		return -ENOMEM;
> >  	}
> >  
> > +	for (i = 0, format_modifier_count = 0; i < in_modifier_count;
> > i++) {
> > +		int j;
> > +
> > +		for (j = 0; funcs->format_mod_supported && j <
> > format_count; j++)
> > +			if (funcs->format_mod_supported(plane,
> > formats[j],
> > +							format_modifier
> > s[i]))
> > +				break;
> > +
> > +		if (j < format_count)
> > +			plane->modifiers[format_modifier_count++] =
> > +				format_modifiers[i];
> > +	}
> > +
> > +	if (format_modifier_count < in_modifier_count) {
> > +		size_t size;
> > +
> > +		size = format_modifier_count *
> > sizeof(format_modifiers[0]);
> > +		plane->modifiers = krealloc(plane->modifiers, size,
> > GFP_KERNEL);
> 
> Should check that the realloc actually succeeded.
Didn't see a failure path for new size smaller than old, the return is
the same pointer passed to krealloc().

> 
> And I think we might want to give this same treatment to plane-
> >formats[]
> as well.
> 
> And perhaps we could even throw out plane->modifiers[] and just rely
> on
> the IN_FORMATS blob exclusively? Hmm. Looks like that is not getting
> fully
> populated unless the driver has provided .format_mod_supported(). Not
> sure why that is, and not sure what userspace is supposed to do with
> a
> partially filled blob like that. I'm thinking we shouldn't even
> attach
> the property to the plane in that case.

Shouldn't it copy the modifier array into the blob and mark all formats
as supported? drm_plane_check_pixel_format() seems to allow any valid
format for a modifier in this case.


-DK

> 
> > +	}
> > +	plane->modifier_count = format_modifier_count;
> > +
> >  	if (name) {
> >  		va_list ap;
> >  
> > @@ -251,8 +272,6 @@ int drm_universal_plane_init(struct drm_device
> > *dev, struct drm_plane *plane,
> >  
> >  	memcpy(plane->format_types, formats, format_count *
> > sizeof(uint32_t));
> >  	plane->format_count = format_count;
> > -	memcpy(plane->modifiers, format_modifiers,
> > -	       format_modifier_count * sizeof(format_modifiers[0]));
> >  	plane->possible_crtcs = possible_crtcs;
> >  	plane->type = type;
> >  
> > -- 
> > 2.14.1
> 
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/framebuffer: Expose only modifiers that support at least a format

[Ville Syrjälä]

On Tue, Nov 06, 2018 at 11:54:45AM -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-11-06 at 16:13 +0200, Ville Syrjälä wrote:
> > On Mon, Nov 05, 2018 at 06:44:34PM -0800, Dhinakaran Pandiyan wrote:
> > > Allows drivers to pass a larger modifier array, thereby avoiding
> > > declarations of static modifier arrays that are only slight
> > > different
> > > for each plane.
> > > 
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_plane.c | 35 +++++++++++++++++++++++++++----
> > > ----
> > >  1 file changed, 27 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > b/drivers/gpu/drm/drm_plane.c
> > > index 1fa98bd12003..1546ffbf8e36 100644
> > > --- a/drivers/gpu/drm/drm_plane.c
> > > +++ b/drivers/gpu/drm/drm_plane.c
> > > @@ -179,8 +179,8 @@ int drm_universal_plane_init(struct drm_device
> > > *dev, struct drm_plane *plane,
> > >  			     const char *name, ...)
> > >  {
> > >  	struct drm_mode_config *config = &dev->mode_config;
> > > -	unsigned int format_modifier_count = 0;
> > > -	int ret;
> > > +	unsigned int format_modifier_count, in_modifier_count = 0;
> > > +	int ret, i;
> > >  
> > >  	/* plane index is used with 32bit bitmasks */
> > >  	if (WARN_ON(config->num_total_plane >= 32))
> > > @@ -216,22 +216,43 @@ int drm_universal_plane_init(struct
> > > drm_device *dev, struct drm_plane *plane,
> > >  
> > >  	if (format_modifiers) {
> > >  		const uint64_t *temp_modifiers = format_modifiers;
> > > +
> > >  		while (*temp_modifiers++ != DRM_FORMAT_MOD_INVALID)
> > > -			format_modifier_count++;
> > > +			in_modifier_count++;
> > >  	}
> > >  
> > > -	plane->modifier_count = format_modifier_count;
> > > -	plane->modifiers = kmalloc_array(format_modifier_count,
> > > +	plane->modifiers = kmalloc_array(in_modifier_count,
> > >  					 sizeof(format_modifiers[0]),
> > >  					 GFP_KERNEL);
> > >  
> > > -	if (format_modifier_count && !plane->modifiers) {
> > > +	if (in_modifier_count && !plane->modifiers) {
> > >  		DRM_DEBUG_KMS("out of memory when allocating plane\n");
> > >  		kfree(plane->format_types);
> > >  		drm_mode_object_unregister(dev, &plane->base);
> > >  		return -ENOMEM;
> > >  	}
> > >  
> > > +	for (i = 0, format_modifier_count = 0; i < in_modifier_count;
> > > i++) {
> > > +		int j;
> > > +
> > > +		for (j = 0; funcs->format_mod_supported && j <
> > > format_count; j++)
> > > +			if (funcs->format_mod_supported(plane,
> > > formats[j],
> > > +							format_modifier
> > > s[i]))
> > > +				break;
> > > +
> > > +		if (j < format_count)
> > > +			plane->modifiers[format_modifier_count++] =
> > > +				format_modifiers[i];
> > > +	}
> > > +
> > > +	if (format_modifier_count < in_modifier_count) {
> > > +		size_t size;
> > > +
> > > +		size = format_modifier_count *
> > > sizeof(format_modifiers[0]);
> > > +		plane->modifiers = krealloc(plane->modifiers, size,
> > > GFP_KERNEL);
> > 
> > Should check that the realloc actually succeeded.
> Didn't see a failure path for new size smaller than old, the return is
> the same pointer passed to krealloc().

I don't know if we want to rely on an implementation detail that
hevily. But that does raise the interesting point that trying to
shrink our memory footprint with krealloc() is futile. I suppose
there is still some kind of debugging benefit from the kasan
stuff.

> 
> > 
> > And I think we might want to give this same treatment to plane-
> > >formats[]
> > as well.
> > 
> > And perhaps we could even throw out plane->modifiers[] and just rely
> > on
> > the IN_FORMATS blob exclusively? Hmm. Looks like that is not getting
> > fully
> > populated unless the driver has provided .format_mod_supported(). Not
> > sure why that is, and not sure what userspace is supposed to do with
> > a
> > partially filled blob like that. I'm thinking we shouldn't even
> > attach
> > the property to the plane in that case.
> 
> Shouldn't it copy the modifier array into the blob and mark all formats
> as supported? drm_plane_check_pixel_format() seems to allow any valid
> format for a modifier in this case.

Yeah, I guess that might be the better approach.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/framebuffer: Expose only modifiers that support at least a format

[Dhinakaran Pandiyan]

On Tue, 2018-11-06 at 22:21 +0200, Ville Syrjälä wrote:
> On Tue, Nov 06, 2018 at 11:54:45AM -0800, Dhinakaran Pandiyan wrote:
> > On Tue, 2018-11-06 at 16:13 +0200, Ville Syrjälä wrote:
> > > On Mon, Nov 05, 2018 at 06:44:34PM -0800, Dhinakaran Pandiyan
> > > wrote:
> > > > Allows drivers to pass a larger modifier array, thereby
> > > > avoiding
> > > > declarations of static modifier arrays that are only slight
> > > > different
> > > > for each plane.
> > > > 
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_plane.c | 35 +++++++++++++++++++++++++++
> > > > ----
> > > > ----
> > > >  1 file changed, 27 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_plane.c
> > > > b/drivers/gpu/drm/drm_plane.c
> > > > index 1fa98bd12003..1546ffbf8e36 100644
> > > > --- a/drivers/gpu/drm/drm_plane.c
> > > > +++ b/drivers/gpu/drm/drm_plane.c
> > > > @@ -179,8 +179,8 @@ int drm_universal_plane_init(struct
> > > > drm_device
> > > > *dev, struct drm_plane *plane,
> > > >  			     const char *name, ...)
> > > >  {
> > > >  	struct drm_mode_config *config = &dev->mode_config;
> > > > -	unsigned int format_modifier_count = 0;
> > > > -	int ret;
> > > > +	unsigned int format_modifier_count, in_modifier_count =
> > > > 0;
> > > > +	int ret, i;
> > > >  
> > > >  	/* plane index is used with 32bit bitmasks */
> > > >  	if (WARN_ON(config->num_total_plane >= 32))
> > > > @@ -216,22 +216,43 @@ int drm_universal_plane_init(struct
> > > > drm_device *dev, struct drm_plane *plane,
> > > >  
> > > >  	if (format_modifiers) {
> > > >  		const uint64_t *temp_modifiers =
> > > > format_modifiers;
> > > > +
> > > >  		while (*temp_modifiers++ !=
> > > > DRM_FORMAT_MOD_INVALID)
> > > > -			format_modifier_count++;
> > > > +			in_modifier_count++;
> > > >  	}
> > > >  
> > > > -	plane->modifier_count = format_modifier_count;
> > > > -	plane->modifiers = kmalloc_array(format_modifier_count,
> > > > +	plane->modifiers = kmalloc_array(in_modifier_count,
> > > >  					 sizeof(format_modifier
> > > > s[0]),
> > > >  					 GFP_KERNEL);
> > > >  
> > > > -	if (format_modifier_count && !plane->modifiers) {
> > > > +	if (in_modifier_count && !plane->modifiers) {
> > > >  		DRM_DEBUG_KMS("out of memory when allocating
> > > > plane\n");
> > > >  		kfree(plane->format_types);
> > > >  		drm_mode_object_unregister(dev, &plane->base);
> > > >  		return -ENOMEM;
> > > >  	}
> > > >  
> > > > +	for (i = 0, format_modifier_count = 0; i <
> > > > in_modifier_count;
> > > > i++) {
> > > > +		int j;
> > > > +
> > > > +		for (j = 0; funcs->format_mod_supported && j <
> > > > format_count; j++)
> > > > +			if (funcs->format_mod_supported(plane,
> > > > formats[j],
> > > > +							format_
> > > > modifier
> > > > s[i]))
> > > > +				break;
> > > > +
> > > > +		if (j < format_count)
> > > > +			plane-
> > > > >modifiers[format_modifier_count++] =
> > > > +				format_modifiers[i];
> > > > +	}
> > > > +
> > > > +	if (format_modifier_count < in_modifier_count) {
> > > > +		size_t size;
> > > > +
> > > > +		size = format_modifier_count *
> > > > sizeof(format_modifiers[0]);
> > > > +		plane->modifiers = krealloc(plane->modifiers,
> > > > size,
> > > > GFP_KERNEL);
> > > 
> > > Should check that the realloc actually succeeded.
> > 
> > Didn't see a failure path for new size smaller than old, the return
> > is
> > the same pointer passed to krealloc().
> 
> I don't know if we want to rely on an implementation detail that
> hevily.
Fair enough.

>  But that does raise the interesting point that trying to
> shrink our memory footprint with krealloc() is futile. I suppose
> there is still some kind of debugging benefit from the kasan
> stuff.
> 
> > 
> > > 
> > > And I think we might want to give this same treatment to plane-
> > > > formats[]
> > > 
> > > as well.
> > > 
> > > And perhaps we could even throw out plane->modifiers[] and just
> > > rely
> > > on
> > > the IN_FORMATS blob exclusively? Hmm. Looks like that is not
> > > getting
> > > fully
> > > populated unless the driver has provided .format_mod_supported().
> > > Not
> > > sure why that is, and not sure what userspace is supposed to do
> > > with
> > > a
> > > partially filled blob like that. I'm thinking we shouldn't even
> > > attach
> > > the property to the plane in that case.
> > 
> > Shouldn't it copy the modifier array into the blob and mark all
> > formats
> > as supported? drm_plane_check_pixel_format() seems to allow any
> > valid
> > format for a modifier in this case.
> 
> Yeah, I guess that might be the better approach.
I'll go with approach in the next version.

-DK
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel