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@xxxxxxx>
> >> ---
> >> 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@xxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
