| Message ID | 20191211180832.7159-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vram-helper: Various cleanups | expand |
Hi Thomas, On Wed, Dec 11, 2019 at 07:08:32PM +0100, Thomas Zimmermann wrote: > Drivers that what to allocate VRAM GEM objects with additional fields > can now do this by implementing struct drm_driver.gem_create_object. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > index b760fd27f3c0..d475d94e2e3e 100644 > --- a/drivers/gpu/drm/drm_gem_vram_helper.c > +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > @@ -2,6 +2,7 @@ > > #include <drm/drm_debugfs.h> > #include <drm/drm_device.h> > +#include <drm/drm_drv.h> > #include <drm/drm_file.h> > #include <drm/drm_framebuffer.h> > #include <drm/drm_gem_ttm_helper.h> > @@ -142,13 +143,19 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev, > size_t size, > unsigned long pg_align) > { > + struct drm_gem_object *gem; > struct drm_gem_vram_object *gbo; > int ret; > > - gbo = kzalloc(sizeof(*gbo), GFP_KERNEL); > - if (!gbo) > + if (dev->driver->gem_create_object) > + gem = dev->driver->gem_create_object(dev, size); > + else > + gem = kzalloc(sizeof(*gbo), GFP_KERNEL); The size is (*gbo) but you assume it is a gem. Looks wrong at first glance??? Sam > + if (!gem) > return ERR_PTR(-ENOMEM); > > + gbo = drm_gem_vram_of_gem(gem); > + > ret = drm_gem_vram_init(dev, gbo, size, pg_align); > if (ret < 0) > goto err_kfree; > -- > 2.24.0
Hi Sam Am 11.12.19 um 20:11 schrieb Sam Ravnborg: > Hi Thomas, > > On Wed, Dec 11, 2019 at 07:08:32PM +0100, Thomas Zimmermann wrote: >> Drivers that what to allocate VRAM GEM objects with additional fields >> can now do this by implementing struct drm_driver.gem_create_object. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c >> index b760fd27f3c0..d475d94e2e3e 100644 >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c >> @@ -2,6 +2,7 @@ >> >> #include <drm/drm_debugfs.h> >> #include <drm/drm_device.h> >> +#include <drm/drm_drv.h> >> #include <drm/drm_file.h> >> #include <drm/drm_framebuffer.h> >> #include <drm/drm_gem_ttm_helper.h> >> @@ -142,13 +143,19 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev, >> size_t size, >> unsigned long pg_align) >> { >> + struct drm_gem_object *gem; >> struct drm_gem_vram_object *gbo; >> int ret; >> >> - gbo = kzalloc(sizeof(*gbo), GFP_KERNEL); >> - if (!gbo) >> + if (dev->driver->gem_create_object) >> + gem = dev->driver->gem_create_object(dev, size); >> + else >> + gem = kzalloc(sizeof(*gbo), GFP_KERNEL); > The size is (*gbo) but you assume it is a gem. > Looks wrong at first glance??? The conversion to gbo is... > > Sam > > >> + if (!gem) >> return ERR_PTR(-ENOMEM); >> >> + gbo = drm_gem_vram_of_gem(gem); >> + ...here. This could be pushed into the if branch, but I found it more readable to put the statement here. For the else branch, it doesn't matter as 'gem' goes first in the structure. I'll move it into the if branch, so it's more obvious what's going on and doesn't break accidentally. Best regards Thomas >> ret = drm_gem_vram_init(dev, gbo, size, pg_align); >> if (ret < 0) >> goto err_kfree; >> -- >> 2.24.0
On Thu, Dec 12, 2019 at 06:39:04AM +0100, Thomas Zimmermann wrote: > Hi Sam > > Am 11.12.19 um 20:11 schrieb Sam Ravnborg: > > Hi Thomas, > > > > On Wed, Dec 11, 2019 at 07:08:32PM +0100, Thomas Zimmermann wrote: > >> Drivers that what to allocate VRAM GEM objects with additional fields > >> can now do this by implementing struct drm_driver.gem_create_object. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++-- > >> 1 file changed, 9 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c > >> index b760fd27f3c0..d475d94e2e3e 100644 > >> --- a/drivers/gpu/drm/drm_gem_vram_helper.c > >> +++ b/drivers/gpu/drm/drm_gem_vram_helper.c > >> @@ -2,6 +2,7 @@ > >> > >> #include <drm/drm_debugfs.h> > >> #include <drm/drm_device.h> > >> +#include <drm/drm_drv.h> > >> #include <drm/drm_file.h> > >> #include <drm/drm_framebuffer.h> > >> #include <drm/drm_gem_ttm_helper.h> > >> @@ -142,13 +143,19 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev, > >> size_t size, > >> unsigned long pg_align) > >> { > >> + struct drm_gem_object *gem; > >> struct drm_gem_vram_object *gbo; > >> int ret; > >> > >> - gbo = kzalloc(sizeof(*gbo), GFP_KERNEL); > >> - if (!gbo) > >> + if (dev->driver->gem_create_object) > >> + gem = dev->driver->gem_create_object(dev, size); > >> + else > >> + gem = kzalloc(sizeof(*gbo), GFP_KERNEL); > > The size is (*gbo) but you assume it is a gem. > > Looks wrong at first glance??? > > The conversion to gbo is... > > > > > Sam > > > > > >> + if (!gem) > >> return ERR_PTR(-ENOMEM); > >> > >> + gbo = drm_gem_vram_of_gem(gem); > >> + > > ...here. This could be pushed into the if branch, but I found it more > readable to put the statement here. For the else branch, it doesn't > matter as 'gem' goes first in the structure. > > I'll move it into the if branch, so it's more obvious what's going on > and doesn't break accidentally. If you push this into the if() branch them imo also push the gem temp variable there. I think that would be quite readable again. Also, probably means you need to push the if (!gem) check too, and make it if (!gbo) in the else branch. Either way: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Best regards > Thomas > > >> ret = drm_gem_vram_init(dev, gbo, size, pg_align); > >> if (ret < 0) > >> goto err_kfree; > >> -- > >> 2.24.0 > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer >
diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index b760fd27f3c0..d475d94e2e3e 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -2,6 +2,7 @@ #include <drm/drm_debugfs.h> #include <drm/drm_device.h> +#include <drm/drm_drv.h> #include <drm/drm_file.h> #include <drm/drm_framebuffer.h> #include <drm/drm_gem_ttm_helper.h> @@ -142,13 +143,19 @@ struct drm_gem_vram_object *drm_gem_vram_create(struct drm_device *dev, size_t size, unsigned long pg_align) { + struct drm_gem_object *gem; struct drm_gem_vram_object *gbo; int ret; - gbo = kzalloc(sizeof(*gbo), GFP_KERNEL); - if (!gbo) + if (dev->driver->gem_create_object) + gem = dev->driver->gem_create_object(dev, size); + else + gem = kzalloc(sizeof(*gbo), GFP_KERNEL); + if (!gem) return ERR_PTR(-ENOMEM); + gbo = drm_gem_vram_of_gem(gem); + ret = drm_gem_vram_init(dev, gbo, size, pg_align); if (ret < 0) goto err_kfree;
Drivers that what to allocate VRAM GEM objects with additional fields can now do this by implementing struct drm_driver.gem_create_object. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)