Re: [PATCH 3/3] drm/vram-helper: Support struct drm_driver.gem_create_object

[Daniel Vetter <daniel@ffwll.ch>]

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
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel