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.syrj...@linux.intel.com>
> > > > Suggested-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > dhinakaran.pandi...@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
