From mboxrd@z Thu Jan 1 00:00:00 1970 From: zourongrong@huawei.com (Rongrong Zou) Date: Thu, 27 Oct 2016 11:01:10 +0800 Subject: [PATCH v5 3/9] drm/hisilicon/hibmc: Add support for frame buffer In-Reply-To: <20161026125910.j6bfc4tq4gjrrmtv@phenom.ffwll.local> References: <1477449426-69018-1-git-send-email-zourongrong@gmail.com> <1477449426-69018-4-git-send-email-zourongrong@gmail.com> <20161026055648.bipks7wthiu4gmlf@phenom.ffwll.local> <58107523.20409@huawei.com> <20161026125910.j6bfc4tq4gjrrmtv@phenom.ffwll.local> Message-ID: <58116DF6.4090709@huawei.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org ? 2016/10/26 20:59, Daniel Vetter ??: > On Wed, Oct 26, 2016 at 05:19:31PM +0800, Rongrong Zou wrote: >> 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 >>> >>> 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 >>>> +#include >>>> #include >>>> #include >>>> >>>> +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. > > Not the fbdev, the hibmc_framebuffer. Understood, 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? > > They all embed the fb instead of having a pointer, because those drivers > are all older than the fb refcounting support. In general good practice is > to look at the most recently merged driver, not at all of them. Only the > most recently one has a good chance to be up-to-date with latest best > practices. The function to call would be drm_framebuffer_unreference(), > and your struct above should be > > struct hibmc_fbdev { > struct drm_fb_helper helper; > struct hibmc_framebuffer *fb; > ... > }; > > Note how fb is a pointer here, not an embedded struct. And in hibmc_user_framebuffer_destroy(), it should call kfree(hibmc_framebuffer *hibmc_fb) not kfree(drm_framebuffer *fb), thanks. regards, Rongrong > -Daniel > >> >>> >>> 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 >>> >> >> >> > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rongrong Zou Subject: Re: [PATCH v5 3/9] drm/hisilicon/hibmc: Add support for frame buffer Date: Thu, 27 Oct 2016 11:01:10 +0800 Message-ID: <58116DF6.4090709@huawei.com> References: <1477449426-69018-1-git-send-email-zourongrong@gmail.com> <1477449426-69018-4-git-send-email-zourongrong@gmail.com> <20161026055648.bipks7wthiu4gmlf@phenom.ffwll.local> <58107523.20409@huawei.com> <20161026125910.j6bfc4tq4gjrrmtv@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20161026125910.j6bfc4tq4gjrrmtv@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Daniel Vetter Cc: mark.rutland@arm.com, architt@codeaurora.org, lijianhua@huawei.com, shenhui@huawei.com, tomeu.vizoso@collabora.com, corbet@lwn.net, airlied@linux.ie, catalin.marinas@arm.com, emil.l.velikov@gmail.com, linuxarm@huawei.com, dri-devel@lists.freedesktop.org, xinliang.liu@linaro.org, james.xiong@huawei.com, benjamin.gaignard@linaro.org, Rongrong Zou , daniel@fooishbar.org, will.deacon@arm.com, linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org 5ZyoIDIwMTYvMTAvMjYgMjA6NTksIERhbmllbCBWZXR0ZXIg5YaZ6YGTOgo+IE9uIFdlZCwgT2N0 IDI2LCAyMDE2IGF0IDA1OjE5OjMxUE0gKzA4MDAsIFJvbmdyb25nIFpvdSB3cm90ZToKPj4gSGkg RGFuaWVsLAo+Pgo+PiBUaGFuc2sgZm9yIHJldmlld2luZyEKPj4KPj4g5ZyoIDIwMTYvMTAvMjYg MTM6NTYsIERhbmllbCBWZXR0ZXIg5YaZ6YGTOgo+Pj4gT24gV2VkLCBPY3QgMjYsIDIwMTYgYXQg MTA6Mzc6MDBBTSArMDgwMCwgUm9uZ3JvbmcgWm91IHdyb3RlOgo+Pj4+IEFkZCBzdXBwb3J0IGZv ciBmYmRldiBhbmQga21zIGZiIG1hbmFnZW1lbnQuCj4+Pj4KPj4+PiBTaWduZWQtb2ZmLWJ5OiBS b25ncm9uZyBab3UgPHpvdXJvbmdyb25nQGdtYWlsLmNvbT4KPj4+Cj4+PiBTbWFsbCBkcml2ZS1i eSBjb21tZW50IGJlbG93Lgo+Pj4KPj4+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2hp c2lsaWNvbi9oaWJtYy9oaWJtY19kcm1fZHJ2LmggYi9kcml2ZXJzL2dwdS9kcm0vaGlzaWxpY29u L2hpYm1jL2hpYm1jX2RybV9kcnYuaAo+Pj4+IGluZGV4IGRiOGQ4MGUuLmQ0MTEzOGEgMTAwNjQ0 Cj4+Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2hpc2lsaWNvbi9oaWJtYy9oaWJtY19kcm1fZHJ2 LmgKPj4+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vaGlzaWxpY29uL2hpYm1jL2hpYm1jX2RybV9k cnYuaAo+Pj4+IEBAIC0yMCw5ICsyMCwyMyBAQAo+Pj4+ICAgICNkZWZpbmUgSElCTUNfRFJNX0RS Vl9ICj4+Pj4KPj4+PiAgICAjaW5jbHVkZSA8ZHJtL2RybVAuaD4KPj4+PiArI2luY2x1ZGUgPGRy bS9kcm1fZmJfaGVscGVyLmg+Cj4+Pj4gICAgI2luY2x1ZGUgPGRybS90dG0vdHRtX2JvX2RyaXZl ci5oPgo+Pj4+ICAgICNpbmNsdWRlIDxkcm0vZHJtX2dlbS5oPgo+Pj4+Cj4+Pj4gK3N0cnVjdCBo aWJtY19mcmFtZWJ1ZmZlciB7Cj4+Pj4gKwlzdHJ1Y3QgZHJtX2ZyYW1lYnVmZmVyIGZiOwo+Pj4+ ICsJc3RydWN0IGRybV9nZW1fb2JqZWN0ICpvYmo7Cj4+Pj4gKwlib29sIGlzX2ZiZGV2X2ZiOwo+ Pj4+ICt9Owo+Pj4+ICsKPj4+PiArc3RydWN0IGhpYm1jX2ZiZGV2IHsKPj4+PiArCXN0cnVjdCBk cm1fZmJfaGVscGVyIGhlbHBlcjsKPj4+PiArCXN0cnVjdCBoaWJtY19mcmFtZWJ1ZmZlciBmYjsK Pj4+Cj4+PiBJIHdvdWxkbid0IGVtYmVkIHRoZSBzaW5nbGUgZnJhbWVidWZmZXIgaGVyZSwgYnV0 IGluc3RlYWQgaGF2ZSBhIHBvaW50ZXIKPj4+IGFuZCBqdXN0IHJlZmNvdW50IGl0LiBUaGlzIGhl cmUgaXMgYSBwYXR0ZXJuIHRoYXQgcHJlZGF0ZXMgZnJhbWVidWZmZXIKPj4+IHJlZmNvdW50aW5n LCBhbmQgaXQgbGVhZHMgdG8gcGxlbnR5IG9mIHN1cnByaXNlcy4KPj4KPj4gd2lsbCBhbGxvY2F0 ZSBmYmRldiBpbiBuZXh0IHBhdGNoIHZlcnNpb24sIHRoYW5rcy4KPgo+IE5vdCB0aGUgZmJkZXYs IHRoZSBoaWJtY19mcmFtZWJ1ZmZlci4KClVuZGVyc3Rvb2QsIHRoYW5rcy4KCj4KPj4+IE1heWJl IHdlIHNob3VsZCB1cGRhdGUgdGhlIGRvY3VtZW50YXRpb24gb2YKPj4+IGRybV9mcmFtZWJ1ZmZl cl91bnJlZ2lzdGVyX3ByaXZhdGUoKSB0byBtZW50aW9uIHRoYXQgaXQgaXMgZGVwcmVjYXRlZD8g VGhlCj4+PiBvdmVydmlldyBkb2MgaW4gZHJtX2ZyYW1lYnVmZmVyLmMgYWxyZWFkeSBleHBsYWlu cyB0aGF0LCBidXQgSSBndWVzcwo+Pj4gdGhhdCdzIG5vdCBvYnZpb3VzIGVub3VnaC4KPj4KPj4g SnVzdCByZXBsYWNlIGRybV9mcmFtZWJ1ZmZlcl91bnJlZ2lzdGVyX3ByaXZhdGUoKSB3aXRoCj4+ IGRybV9mcmFtZWJ1ZmZlcl9yZW1vdmUoKT8KPj4KPj4gSSBmb3VuZCBtYW55IG90aGVyIGRyaXZl cnMgdXNlIHRoZSBmb2xsb3dpbmcgdHdvIGZ1bmN0aW9ucyBpbiB0aGVpcgo+PiBvd24geHh4X2Zi ZGV2X2Rlc3Ryb3koKToKPj4gCWRybV9mcmFtZWJ1ZmZlcl91bnJlZ2lzdGVyX3ByaXZhdGUoZmJk ZXYtPmZiKTsKPj4gCWRybV9mcmFtZWJ1ZmZlcl9yZW1vdmUoZmJkZXYtPmZiKTsKPj4gc28gdGhl IGZvcm1lciBpcyByZWR1bmRhbnQgYW5kIGNhbiBiZSByZW1vdmVkIGRpcmVjdGx5Pwo+Cj4gVGhl eSBhbGwgZW1iZWQgdGhlIGZiIGluc3RlYWQgb2YgaGF2aW5nIGEgcG9pbnRlciwgYmVjYXVzZSB0 aG9zZSBkcml2ZXJzCj4gYXJlIGFsbCBvbGRlciB0aGFuIHRoZSBmYiByZWZjb3VudGluZyBzdXBw b3J0LiBJbiBnZW5lcmFsIGdvb2QgcHJhY3RpY2UgaXMKPiB0byBsb29rIGF0IHRoZSBtb3N0IHJl Y2VudGx5IG1lcmdlZCBkcml2ZXIsIG5vdCBhdCBhbGwgb2YgdGhlbS4gT25seSB0aGUKPiBtb3N0 IHJlY2VudGx5IG9uZSBoYXMgYSBnb29kIGNoYW5jZSB0byBiZSB1cC10by1kYXRlIHdpdGggbGF0 ZXN0IGJlc3QKPiBwcmFjdGljZXMuIFRoZSBmdW5jdGlvbiB0byBjYWxsIHdvdWxkIGJlIGRybV9m cmFtZWJ1ZmZlcl91bnJlZmVyZW5jZSgpLAo+IGFuZCB5b3VyIHN0cnVjdCBhYm92ZSBzaG91bGQg YmUKPgo+IHN0cnVjdCBoaWJtY19mYmRldiB7Cj4gCXN0cnVjdCBkcm1fZmJfaGVscGVyIGhlbHBl cjsKPiAJc3RydWN0IGhpYm1jX2ZyYW1lYnVmZmVyICpmYjsKPiAJLi4uCj4gfTsKPgo+IE5vdGUg aG93IGZiIGlzIGEgcG9pbnRlciBoZXJlLCBub3QgYW4gZW1iZWRkZWQgc3RydWN0LgoKQW5kIGlu IGhpYm1jX3VzZXJfZnJhbWVidWZmZXJfZGVzdHJveSgpLCBpdCBzaG91bGQgY2FsbAprZnJlZSho aWJtY19mcmFtZWJ1ZmZlciAqaGlibWNfZmIpIG5vdCBrZnJlZShkcm1fZnJhbWVidWZmZXIgKmZi KSwgdGhhbmtzLgoKcmVnYXJkcywKUm9uZ3JvbmcKCj4gLURhbmllbAo+Cj4+Cj4+Pgo+Pj4gQ2Fu IHlvdSBwbHMgZG8gdGhhdCBwYXRjaD8gQW5kIHBscyBtYWtlIHN1cmUgaXQgYWxsIGxvb2tzIHBy ZXR0eSB3aGVuCj4+PiBidWlsZGluZyB0aGUgZG9jcyB3aXRoCj4+Cj4+IE5vIHByb2JsZW0sIGkn bGwgc2VuZCBhbm90aGVyIHBhdGNoIGxhdGVyLgo+Pgo+PiBSZWdhcmRzLAo+PiBSb25ncm9uZwo+ Pgo+Pj4KPj4+ICQgbWFrZSBodG1sZG9jcwo+Pj4KPj4+IFRoYW5rcywgRGFuaWVsCj4+Pgo+Pgo+ Pgo+Pgo+CgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwps aW51eC1hcm0ta2VybmVsIG1haWxpbmcgbGlzdApsaW51eC1hcm0ta2VybmVsQGxpc3RzLmluZnJh ZGVhZC5vcmcKaHR0cDovL2xpc3RzLmluZnJhZGVhZC5vcmcvbWFpbG1hbi9saXN0aW5mby9saW51 eC1hcm0ta2VybmVsCg==