From mboxrd@z Thu Jan 1 00:00:00 1970 From: Rob Clark Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support Date: Thu, 2 Apr 2015 14:42:47 -0400 Message-ID: References: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> <1427967609.10518.33.camel@x220> <1d1ffeacb66f0a24212f83bbe86996d9.squirrel@www.codeaurora.org> <1427999042.10518.60.camel@x220> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1427999042.10518.60.camel@x220> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Paul Bolle Cc: linux-arm-msm , Linux Kernel Mailing List , "dri-devel@lists.freedesktop.org" List-Id: linux-arm-msm@vger.kernel.org T24gVGh1LCBBcHIgMiwgMjAxNSBhdCAyOjI0IFBNLCBQYXVsIEJvbGxlIDxwZWJvbGxlQHRpc2Nh bGkubmw+IHdyb3RlOgo+IEhpIEppbGFpLAo+Cj4gT24gVGh1LCAyMDE1LTA0LTAyIGF0IDE3OjU4 ICswMDAwLCBqaWxhaXdAY29kZWF1cm9yYS5vcmcgd3JvdGU6Cj4+IFRoYW5rcyBQYXVsLiBTb21l IGNvbW1lbnRzIGVtYmVkZGVkIGFuZCBmb3IgdGhlIHJlc3QgSSB3aWxsIHVwZGF0ZSB0aGUKPj4g Y29kZSBhY2NvcmRpbmdseS4KPgo+IE1vc3Qgb2YgbXkgY29tbWVudHMgYXBwZWFyIHRvIGJlIGls bCBpbmZvcm1lZCwgc28gSSB3b3VsZG4ndCBtaW5kIGlmCj4geW91J2Qgc3BlY2lmeSB3aGljaCB1 cGRhdGVzIHlvdSBhY3R1YWxseSBwbGFuIHRvIGRvLgo+Cj4+ID4+IC0tLSBhL2RyaXZlcnMvZ3B1 L2RybS9tc20vTWFrZWZpbGUKPj4gPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL21zbS9NYWtlZmls ZQo+PiA+Cj4+ID4+ICttc20tJChDT05GSUdfRFJNX01TTV9XQikgKz0gXAo+PiA+PiArICBtZHAv bWRwNS9tZHA1X3diX2VuY29kZXIubyBcCj4+ID4+ICsgIG1kcC9tZHBfd2IvbWRwX3diLm8gXAo+ PiA+PiArICBtZHAvbWRwX3diL21kcF93Yl9jb25uZWN0b3IubyBcCj4+ID4+ICsgIG1kcC9tZHBf d2IvbWRwX3diX3Y0bDIubwo+PiA+Cj4+ID4gc28gbWRwX3diX3Y0bDIubyB3aWxsIG5ldmVyIGJl IHBhcnQgb2YgYSBtb2R1bGUuCj4+IElmIENPTkZJRy1EUk1fTVNNX1dCIGlzIHksIHRoZW4gYWxs IHdiIHJlbGF0ZWQgZmlsZXMgKGluY2x1ZGluZwo+PiBtZHBfd2JfdjRsMi5vKSB3aWxsIGJlIGFk ZGVkIHRvIG1zbS15LCB0aGVuIGJlIGxpbmtlZCB0byBtc20ubwo+PiBvYmotJChDT05GSUdfRFJN X01TTSkgICArPSBtc20ubwo+Cj4gKEEgdGVsbCB0YWxlIHdhcyB0aGF0IEkgZGlkbid0IHF1b3Rl IHRoYXQgbGluZS4pCj4KPj4gUmVmZXIgdG8gZG9jdW1lbnQgaHR0cDovL2x3bi5uZXQvQXJ0aWNs ZXMvMjE4MzUvIChzZWN0aW9uIDMuMyksCj4+IGl0IHNob3VsZCBiZSBhYmxlIHRvIGJlIGJ1aWx0 LWluIHRvIGtlcm5lbCBvciBhcyBhIG1vZHVsZS4KPgo+IEl0J3MgaGFyZCB0eXBpbmcgd2l0aCBh IGJyb3duIHBhcGVyIGJhZyBmb3IgaGVhZHdhcmU6IEkgc3RpbGwgaGF2ZQo+IHRyb3VibGUgc3Bl YWtpbmcgTWFrZWZpbGUsIGV2ZW4gYWZ0ZXIgYWxsIHRoZXNlIHllYXJzLCBidXQgSSdtIGFmcmFp ZAo+IHlvdSdyZSByaWdodC4KPgo+PiA+PiAtLS0gL2Rldi9udWxsCj4+ID4+ICsrKyBiL2RyaXZl cnMvZ3B1L2RybS9tc20vbWRwL21kcF93Yi9tZHBfd2JfdjRsMi5jCj4+ID4KPj4gPj4gKyNpbmNs dWRlIDxsaW51eC9tb2R1bGUuaD4KPj4gPgo+PiA+IFRoaXMgaW5jbHVkZSBpcyBuZWVkZWQgbW9z dGx5IGZvciBtb2R1bGVfcGFyYW0oKSwgcmlnaHQ/Cj4+ID4KPj4gPj4gKyNkZWZpbmUgTVNNX1dC X01PRFVMRV9OQU1FICJtc21fd2IiCj4+ID4KPj4gPiBNU01fV0JfRFJJVkVSX05BTUU/IEJ1dCBz ZWUgYmVsb3cuCj4+ID4KPj4gPj4gK3N0YXRpYyB1bnNpZ25lZCBkZWJ1ZzsKPj4gPj4gK21vZHVs ZV9wYXJhbShkZWJ1ZywgdWludCwgMDY0NCk7Cj4+ID4KPj4gPiBkZWJ1ZyBpcyBiYXNpY2FsbHkg YSBib29sZWFuIHR5cGUgb2YgZmxhZy4gV291bGQKPj4gPiAgICAgc3RhdGljIGJvb2wgZGVidWc7 Cj4+ID4gICAgIG1vZHVsZV9wYXJhbShkZWJ1ZywgYm9vbCwgMDY0NCk7Cj4+ID4KPj4gPiB3b3Jr Pwo+PiA+Cj4+ID4+ICtNT0RVTEVfUEFSTV9ERVNDKGRlYnVnLCAiYWN0aXZhdGVzIGRlYnVnIGlu Zm8iKTsKPj4gPgo+PiA+IE5vIG9uZSBpcyBldmVyIGdvaW5nIHRvIHNlZSB0aGlzIGRlc2NyaXB0 aW9uLgo+PiA+Cj4+ID4+ICsjZGVmaW5lIGRwcmludGsoZGV2LCBsZXZlbCwgZm10LCBhcmcuLi4p IFwKPj4gPj4gKyAgdjRsMl9kYmcobGV2ZWwsIGRlYnVnLCAmZGV2LT52NGwyX2RldiwgZm10LCAj IyBhcmcpCj4+ID4KPj4gPiBBbGwgaW5zdGFuY2VzIG9mIGRwcmludGsoKSB0aGF0IEkgZm91bmQg aGFkIGxldmVsIHNldCB0byAxLCBzbyB0aGUgYWJvdmUKPj4gPiBjb3VsZCBiZSBzaW1wbGlmaWVk IGEgYml0Ogo+PiA+ICAgICAjZGVmaW5lIGRwcmludGsoZGV2LCBmbXQsIGFyZy4uLikgXAo+PiA+ ICAgICAgICAgICAgIHY0bDJfZGJnKDEsIGRlYnVnLCAmZGV2LT52NGwyX2RldiwgZm10LCAjIyBh cmcpCj4+ID4KPj4gPiBCdXQgcGVyaGFwcyBwcl9kZWJ1ZygpIGFuZCB0aGUgZHluYW1pYyBkZWJ1 ZyBmYWNpbGl0eSBjb3VsZCBiZSB1c2VkCj4+ID4gaW5zdGVhZCBvZiBtb2R1bGVfcGFyYW0oKSwg ZHByaW50aygpIGFuZCB2NGwyX2RiZygpLiBOb3Qgc3VyZSB3aGljaAo+PiA+IGFwcHJvYWNoIGlz IGVhc2llci4KPj4gPgo+PiA+PiArc3RhdGljIGNvbnN0IHN0cnVjdCB2NGwyX2ZpbGVfb3BlcmF0 aW9ucyBtc21fd2JfdjRsMl9mb3BzID0gewo+PiA+PiArICAub3duZXIgPSBUSElTX01PRFVMRSwK Pj4gPgo+PiA+IFRISVNfTU9EVUxFIHdpbGwgYmFzaWNhbGx5IGJlIGVxdWl2YWxlbnQgdG8gTlVM TC4KPj4gPgo+PiA+PiArICAub3BlbiA9IHY0bDJfZmhfb3BlbiwKPj4gPj4gKyAgLnJlbGVhc2Ug PSB2YjJfZm9wX3JlbGVhc2UsCj4+ID4+ICsgIC5wb2xsID0gdmIyX2ZvcF9wb2xsLAo+PiA+PiAr ICAudW5sb2NrZWRfaW9jdGwgPSB2aWRlb19pb2N0bDIsCj4+ID4+ICt9Owo+PiA+Cj4+ID4+ICtp bnQgbXNtX3diX3Y0bDJfaW5pdChzdHJ1Y3QgbXNtX3diICp3YikKPj4gPj4gK3sKPj4gPj4gKyAg c3RydWN0IG1zbV93Yl92NGwyX2RldiAqZGV2Owo+PiA+PiArICBzdHJ1Y3QgdmlkZW9fZGV2aWNl ICp2ZmQ7Cj4+ID4+ICsgIHN0cnVjdCB2YjJfcXVldWUgKnE7Cj4+ID4+ICsgIGludCByZXQ7Cj4+ ID4+ICsKPj4gPj4gKyAgZGV2ID0ga3phbGxvYyhzaXplb2YoKmRldiksIEdGUF9LRVJORUwpOwo+ PiA+PiArICBpZiAoIWRldikKPj4gPj4gKyAgICAgICAgICByZXR1cm4gLUVOT01FTTsKPj4gPj4g Kwo+PiA+PiArICBzbnByaW50ZihkZXYtPnY0bDJfZGV2Lm5hbWUsIHNpemVvZihkZXYtPnY0bDJf ZGV2Lm5hbWUpLAo+PiA+PiArICAgICAgICAgICAgICAgICAgIiVzIiwgTVNNX1dCX01PRFVMRV9O QU1FKTsKPj4gPgo+PiA+IEl0IGxvb2tzIGxpa2UgeW91IGNhbiBhY3R1YWxseSBkcm9wIHRoZSAj ZGVmaW5lIGFuZCBtZXJnZSB0aGUgbGFzdCB0d28KPj4gPiBhcmd1bWVudHMgdG8gc25wcmludGYo KSBpbnRvIGp1c3QgIm1zbV93YiIuCj4+ID4KPj4gPj4gKyAgcmV0ID0gdjRsMl9kZXZpY2VfcmVn aXN0ZXIoTlVMTCwgJmRldi0+djRsMl9kZXYpOwo+PiA+PiArICBpZiAocmV0KQo+PiA+PiArICAg ICAgICAgIGdvdG8gZnJlZV9kZXY7Cj4+ID4+ICsKPj4gPj4gKyAgLyogZGVmYXVsdCBBUkdCODg4 OCA2NDB4NDgwICovCj4+ID4+ICsgIGRldi0+Zm10ID0gZ2V0X2Zvcm1hdChWNEwyX1BJWF9GTVRf UkdCMzIpOwo+PiA+PiArICBkZXYtPndpZHRoID0gNjQwOwo+PiA+PiArICBkZXYtPmhlaWdodCA9 IDQ4MDsKPj4gPj4gKwo+PiA+PiArICAvKiBpbml0aWFsaXplIHF1ZXVlICovCj4+ID4+ICsgIHEg PSAmZGV2LT52Yl92aWRxOwo+PiA+PiArICBxLT50eXBlID0gVjRMMl9CVUZfVFlQRV9WSURFT19D QVBUVVJFX01QTEFORTsKPj4gPj4gKyAgcS0+aW9fbW9kZXMgPSBWQjJfRE1BQlVGOwo+PiA+PiAr ICBxLT5kcnZfcHJpdiA9IGRldjsKPj4gPj4gKyAgcS0+YnVmX3N0cnVjdF9zaXplID0gc2l6ZW9m KHN0cnVjdCBtc21fd2JfdjRsMl9idWZmZXIpOwo+PiA+PiArICBxLT5vcHMgPSAmbXNtX3diX3Zi Ml9vcHM7Cj4+ID4+ICsgIHEtPm1lbV9vcHMgPSAmbXNtX3diX3ZiMl9tZW1fb3BzOwo+PiA+PiAr ICBxLT50aW1lc3RhbXBfdHlwZSA9IFY0TDJfQlVGX0ZMQUdfVElNRVNUQU1QX01PTk9UT05JQzsK Pj4gPj4gKwo+PiA+PiArICByZXQgPSB2YjJfcXVldWVfaW5pdChxKTsKPj4gPj4gKyAgaWYgKHJl dCkKPj4gPj4gKyAgICAgICAgICBnb3RvIHVucmVnX2RldjsKPj4gPj4gKwo+PiA+PiArICBtdXRl eF9pbml0KCZkZXYtPm11dGV4KTsKPj4gPj4gKwo+PiA+PiArICB2ZmQgPSAmZGV2LT52ZGV2Owo+ PiA+PiArICAqdmZkID0gbXNtX3diX3Y0bDJfdGVtcGxhdGU7Cj4+ID4+ICsgIHZmZC0+ZGVidWcg PSBkZWJ1ZzsKPj4gPgo+PiA+IEkgY291bGRuJ3QgZmluZCBhIG1lbWJlciBvZiBzdHJ1Y3Qgdmlk ZW9fZGV2aWNlIG5hbWVkIGRlYnVnLiBXaGVyZSBkb2VzCj4+ID4gdGhhdCBjb21lIGZyb20/IEJl Y2F1c2UsIGFzIGZhciBhcyBJIGNhbiBzZWUsIHRoaXMgd29uJ3QgY29tcGlsZS4KPj4geWVzLCBp dCdzIHRoZXJlOgo+PiBodHRwOi8vbHhyLmZyZWUtZWxlY3Ryb25zLmNvbS9zb3VyY2UvaW5jbHVk ZS9tZWRpYS92NGwyLWRldi5oI0wxMjcKPgo+IFByb2JhYmx5IGluIHNvbWUgdHJlZSBJJ20gbm90 IGF3YXJlIG9mLiBJIG9ubHkgZGlkOgo+ICAgICAkIGdpdCBjYXQtZmlsZSBibG9iIHY0LjAtcmM2 OmluY2x1ZGUvbWVkaWEvdjRsMi1kZXYuaCB8IGdyZXAgZGVidWcKPiAgICAgICAgIC8qIEludGVy bmFsIGRldmljZSBkZWJ1ZyBmbGFncywgbm90IGZvciB1c2UgYnkgZHJpdmVycyAqLwo+ICAgICAg ICAgaW50IGRldl9kZWJ1ZzsKPgo+IGFuZAo+ICAgICAkIGdpdCBjYXQtZmlsZSBibG9iIG5leHQt MjAxNTA0MDI6aW5jbHVkZS9tZWRpYS92NGwyLWRldi5oIHwgZ3JlcCBkZWJ1Zwo+ICAgICAgICAg LyogSW50ZXJuYWwgZGV2aWNlIGRlYnVnIGZsYWdzLCBub3QgZm9yIHVzZSBieSBkcml2ZXJzICov Cj4gICAgICAgICBpbnQgZGV2X2RlYnVnOwo+Cj4gV2hpY2ggdHJlZSBkb2VzIGRlYnVnIGNvbWUg ZnJvbT8KCmZ3aXcsIGxvb2tzIGxpa2UgMTcwMjhjZCByZW5hbWVkIGRlYnVnIC0+IGRldl9kZWJ1 ZwoKQlIsCi1SCgoKPgo+IFRoYW5rcywKPgo+Cj4gUGF1bCBCb2xsZQo+Cl9fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QK ZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Au b3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753693AbbDBSmy (ORCPT ); Thu, 2 Apr 2015 14:42:54 -0400 Received: from mail-qc0-f176.google.com ([209.85.216.176]:34881 "EHLO mail-qc0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753660AbbDBSms (ORCPT ); Thu, 2 Apr 2015 14:42:48 -0400 MIME-Version: 1.0 In-Reply-To: <1427999042.10518.60.camel@x220> References: <1427922770-13721-1-git-send-email-jilaiw@codeaurora.org> <1427967609.10518.33.camel@x220> <1d1ffeacb66f0a24212f83bbe86996d9.squirrel@www.codeaurora.org> <1427999042.10518.60.camel@x220> Date: Thu, 2 Apr 2015 14:42:47 -0400 Message-ID: Subject: Re: [PATCH 2/3] drm:msm: Initial Add Writeback Support From: Rob Clark To: Paul Bolle Cc: Jilai Wang , "dri-devel@lists.freedesktop.org" , linux-arm-msm , Linux Kernel Mailing List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 2, 2015 at 2:24 PM, Paul Bolle wrote: > Hi Jilai, > > On Thu, 2015-04-02 at 17:58 +0000, jilaiw@codeaurora.org wrote: >> Thanks Paul. Some comments embedded and for the rest I will update the >> code accordingly. > > Most of my comments appear to be ill informed, so I wouldn't mind if > you'd specify which updates you actually plan to do. > >> >> --- a/drivers/gpu/drm/msm/Makefile >> >> +++ b/drivers/gpu/drm/msm/Makefile >> > >> >> +msm-$(CONFIG_DRM_MSM_WB) += \ >> >> + mdp/mdp5/mdp5_wb_encoder.o \ >> >> + mdp/mdp_wb/mdp_wb.o \ >> >> + mdp/mdp_wb/mdp_wb_connector.o \ >> >> + mdp/mdp_wb/mdp_wb_v4l2.o >> > >> > so mdp_wb_v4l2.o will never be part of a module. >> If CONFIG-DRM_MSM_WB is y, then all wb related files (including >> mdp_wb_v4l2.o) will be added to msm-y, then be linked to msm.o >> obj-$(CONFIG_DRM_MSM) += msm.o > > (A tell tale was that I didn't quote that line.) > >> Refer to document http://lwn.net/Articles/21835/ (section 3.3), >> it should be able to be built-in to kernel or as a module. > > It's hard typing with a brown paper bag for headware: I still have > trouble speaking Makefile, even after all these years, but I'm afraid > you're right. > >> >> --- /dev/null >> >> +++ b/drivers/gpu/drm/msm/mdp/mdp_wb/mdp_wb_v4l2.c >> > >> >> +#include >> > >> > This include is needed mostly for module_param(), right? >> > >> >> +#define MSM_WB_MODULE_NAME "msm_wb" >> > >> > MSM_WB_DRIVER_NAME? But see below. >> > >> >> +static unsigned debug; >> >> +module_param(debug, uint, 0644); >> > >> > debug is basically a boolean type of flag. Would >> > static bool debug; >> > module_param(debug, bool, 0644); >> > >> > work? >> > >> >> +MODULE_PARM_DESC(debug, "activates debug info"); >> > >> > No one is ever going to see this description. >> > >> >> +#define dprintk(dev, level, fmt, arg...) \ >> >> + v4l2_dbg(level, debug, &dev->v4l2_dev, fmt, ## arg) >> > >> > All instances of dprintk() that I found had level set to 1, so the above >> > could be simplified a bit: >> > #define dprintk(dev, fmt, arg...) \ >> > v4l2_dbg(1, debug, &dev->v4l2_dev, fmt, ## arg) >> > >> > But perhaps pr_debug() and the dynamic debug facility could be used >> > instead of module_param(), dprintk() and v4l2_dbg(). Not sure which >> > approach is easier. >> > >> >> +static const struct v4l2_file_operations msm_wb_v4l2_fops = { >> >> + .owner = THIS_MODULE, >> > >> > THIS_MODULE will basically be equivalent to NULL. >> > >> >> + .open = v4l2_fh_open, >> >> + .release = vb2_fop_release, >> >> + .poll = vb2_fop_poll, >> >> + .unlocked_ioctl = video_ioctl2, >> >> +}; >> > >> >> +int msm_wb_v4l2_init(struct msm_wb *wb) >> >> +{ >> >> + struct msm_wb_v4l2_dev *dev; >> >> + struct video_device *vfd; >> >> + struct vb2_queue *q; >> >> + int ret; >> >> + >> >> + dev = kzalloc(sizeof(*dev), GFP_KERNEL); >> >> + if (!dev) >> >> + return -ENOMEM; >> >> + >> >> + snprintf(dev->v4l2_dev.name, sizeof(dev->v4l2_dev.name), >> >> + "%s", MSM_WB_MODULE_NAME); >> > >> > It looks like you can actually drop the #define and merge the last two >> > arguments to snprintf() into just "msm_wb". >> > >> >> + ret = v4l2_device_register(NULL, &dev->v4l2_dev); >> >> + if (ret) >> >> + goto free_dev; >> >> + >> >> + /* default ARGB8888 640x480 */ >> >> + dev->fmt = get_format(V4L2_PIX_FMT_RGB32); >> >> + dev->width = 640; >> >> + dev->height = 480; >> >> + >> >> + /* initialize queue */ >> >> + q = &dev->vb_vidq; >> >> + q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE; >> >> + q->io_modes = VB2_DMABUF; >> >> + q->drv_priv = dev; >> >> + q->buf_struct_size = sizeof(struct msm_wb_v4l2_buffer); >> >> + q->ops = &msm_wb_vb2_ops; >> >> + q->mem_ops = &msm_wb_vb2_mem_ops; >> >> + q->timestamp_type = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC; >> >> + >> >> + ret = vb2_queue_init(q); >> >> + if (ret) >> >> + goto unreg_dev; >> >> + >> >> + mutex_init(&dev->mutex); >> >> + >> >> + vfd = &dev->vdev; >> >> + *vfd = msm_wb_v4l2_template; >> >> + vfd->debug = debug; >> > >> > I couldn't find a member of struct video_device named debug. Where does >> > that come from? Because, as far as I can see, this won't compile. >> yes, it's there: >> http://lxr.free-electrons.com/source/include/media/v4l2-dev.h#L127 > > Probably in some tree I'm not aware of. I only did: > $ git cat-file blob v4.0-rc6:include/media/v4l2-dev.h | grep debug > /* Internal device debug flags, not for use by drivers */ > int dev_debug; > > and > $ git cat-file blob next-20150402:include/media/v4l2-dev.h | grep debug > /* Internal device debug flags, not for use by drivers */ > int dev_debug; > > Which tree does debug come from? fwiw, looks like 17028cd renamed debug -> dev_debug BR, -R > > Thanks, > > > Paul Bolle >