Hi Daniel,
Thansk for reviewing!
å¨ 2016/10/26 13:56, Daniel Vetter åé:
> On Wed, Oct 26, 2016 at 10:37:00AM +0800, Rongrong Zou wrote:
>> Add support for fbdev and kms fb management.
>>
>> Signed-off-by: Rongrong Zou <zourongrong at gmail.com>
>
> Small drive-by comment below.
>
>> diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> index db8d80e..d41138a 100644
>> --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h
>> @@ -20,9 +20,23 @@
>> #define HIBMC_DRM_DRV_H
>>
>> #include <drm/drmP.h>
>> +#include <drm/drm_fb_helper.h>
>> #include <drm/ttm/ttm_bo_driver.h>
>> #include <drm/drm_gem.h>
>>
>> +struct hibmc_framebuffer {
>> + struct drm_framebuffer fb;
>> + struct drm_gem_object *obj;
>> + bool is_fbdev_fb;
>> +};
>> +
>> +struct hibmc_fbdev {
>> + struct drm_fb_helper helper;
>> + struct hibmc_framebuffer fb;
>
> I wouldn't embed the single framebuffer here, but instead have a pointer
> and just refcount it. This here is a pattern that predates framebuffer
> refcounting, and it leads to plenty of surprises.
will allocate fbdev in next patch version, thanks.
>
> Maybe we should update the documentation of
> drm_framebuffer_unregister_private() to mention that it is deprecated? The
> overview doc in drm_framebuffer.c already explains that, but I guess
> that's not obvious enough.
Just replace drm_framebuffer_unregister_private() with
drm_framebuffer_remove()?
I found many other drivers use the following two functions in their
own xxx_fbdev_destroy():
drm_framebuffer_unregister_private(fbdev->fb);
drm_framebuffer_remove(fbdev->fb);
so the former is redundant and can be removed directly?
>
> Can you pls do that patch? And pls make sure it all looks pretty when
> building the docs with
No problem, i'll send another patch later.
Regards,
Rongrong
>
> $ make htmldocs
>
> Thanks, Daniel
>
