| Message ID | 20181106024434.12322-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/framebuffer: Expose only modifiers that support at least a format | expand |
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
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 > >
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.
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 >
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;
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(-)