From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark yao Subject: Re: [PATCH v2 2/5] drm/rockchip: vop: fix yuv plane support Date: Thu, 02 Jul 2015 14:53:38 +0800 Message-ID: <5594DFF2.8020609@rock-chips.com> References: <1435313249-4549-1-git-send-email-mark.yao@rock-chips.com> <1435313249-4549-3-git-send-email-mark.yao@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Tomasz Figa Cc: xw@rock-chips.com, zwl@rock-chips.com, "linux-kernel@vger.kernel.org" , "open list:ARM/Rockchip SoC..." , dri-devel , dkm@rock-chips.com, sandy.huang@rock-chips.com, "linux-arm-kernel@lists.infradead.org" List-Id: linux-rockchip.vger.kernel.org SGkgVG9tYXN6CiAgICAgVGhhbmtzIGZvciB5b3VyIHJldmlldywgSSB3aWxsIGZpeCBpdCBzb29u LgpPbiAyMDE15bm0MDfmnIgwMuaXpSAxNDowMCwgVG9tYXN6IEZpZ2Egd3JvdGU6Cj4gSGkgTWFy aywKPgo+IFBsZWFzZSBzZWUgbXkgY29tbWVudHMgaW5saW5lLgo+Cj4gT24gRnJpLCBKdW4gMjYs IDIwMTUgYXQgNzowNyBQTSwgTWFyayBZYW8gPG1hcmsueWFvQHJvY2stY2hpcHMuY29tPiB3cm90 ZToKPj4gdm9wIHN1cHBvcnQgeXV2IHdpdGggTlYxMiwgTlYxNiBhbmQgTlYyNCwgb25seSAyIHBs YW5lIHl1di4KPj4KPj4gU2lnbmVkLW9mZi1ieTogTWFyayBZYW8gPG1hcmsueWFvQHJvY2stY2hp cHMuY29tPgo+Pgo+PiBDaGFuZ2VzIGluIHYyOgo+PiAtIFV2IGJ1ZmZlciBub3Qgc3VwcG9ydCBv ZGQgb2Zmc2V0LCBhbGlnbiBpdC4KPj4gLSBGaXggZXJyb3IgZGlzcGxheSB3aGVuIG1vdmUgeXV2 IGltYWdlLgo+Pgo+PiAtLS0KPj4gICBkcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBf ZHJtX3ZvcC5jIHwgICA2MyArKysrKysrKysrKysrKysrKysrKysrKystLS0KPj4gICAxIGZpbGUg Y2hhbmdlZCwgNTcgaW5zZXJ0aW9ucygrKSwgNiBkZWxldGlvbnMoLSkKPj4KPj4gZGlmZiAtLWdp dCBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fdm9wLmMgYi9kcml2ZXJz L2dwdS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX3ZvcC5jCj4+IGluZGV4IDNjOWY0ZjMuLjZj YTA4ZjggMTAwNjQ0Cj4+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9k cm1fdm9wLmMKPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV92 b3AuYwo+PiBAQCAtMzczLDYgKzM3MywxOCBAQCBzdGF0aWMgZW51bSB2b3BfZGF0YV9mb3JtYXQg dm9wX2NvbnZlcnRfZm9ybWF0KHVpbnQzMl90IGZvcm1hdCkKPj4gICAgICAgICAgfQo+PiAgIH0K Pj4KPj4gK3N0YXRpYyBib29sIGlzX3l1dl9zdXBwb3J0KHVpbnQzMl90IGZvcm1hdCkKPj4gK3sK Pj4gKyAgICAgICBzd2l0Y2ggKGZvcm1hdCkgewo+PiArICAgICAgIGNhc2UgRFJNX0ZPUk1BVF9O VjEyOgo+PiArICAgICAgIGNhc2UgRFJNX0ZPUk1BVF9OVjE2Ogo+PiArICAgICAgIGNhc2UgRFJN X0ZPUk1BVF9OVjI0Ogo+PiArICAgICAgICAgICAgICAgcmV0dXJuIHRydWU7Cj4+ICsgICAgICAg ZGVmYXVsdDoKPj4gKyAgICAgICAgICAgICAgIHJldHVybiBmYWxzZTsKPj4gKyAgICAgICB9Cj4+ ICt9Cj4+ICsKPj4gICBzdGF0aWMgYm9vbCBpc19hbHBoYV9zdXBwb3J0KHVpbnQzMl90IGZvcm1h dCkKPj4gICB7Cj4+ICAgICAgICAgIHN3aXRjaCAoZm9ybWF0KSB7Cj4+IEBAIC01NzcsMTYgKzU4 OSwyMSBAQCBzdGF0aWMgaW50IHZvcF91cGRhdGVfcGxhbmVfZXZlbnQoc3RydWN0IGRybV9wbGFu ZSAqcGxhbmUsCj4+ICAgICAgICAgIHN0cnVjdCB2b3AgKnZvcCA9IHRvX3ZvcChjcnRjKTsKPj4g ICAgICAgICAgc3RydWN0IGRybV9nZW1fb2JqZWN0ICpvYmo7Cj4+ICAgICAgICAgIHN0cnVjdCBy b2NrY2hpcF9nZW1fb2JqZWN0ICpya19vYmo7Cj4+ICsgICAgICAgc3RydWN0IGRybV9nZW1fb2Jq ZWN0ICp1dl9vYmo7Cj4+ICsgICAgICAgc3RydWN0IHJvY2tjaGlwX2dlbV9vYmplY3QgKnJrX3V2 X29iajsKPj4gICAgICAgICAgdW5zaWduZWQgbG9uZyBvZmZzZXQ7Cj4+ICAgICAgICAgIHVuc2ln bmVkIGludCBhY3R1YWxfdzsKPj4gICAgICAgICAgdW5zaWduZWQgaW50IGFjdHVhbF9oOwo+PiAg ICAgICAgICB1bnNpZ25lZCBpbnQgZHNwX3N0eDsKPj4gICAgICAgICAgdW5zaWduZWQgaW50IGRz cF9zdHk7Cj4+ICAgICAgICAgIHVuc2lnbmVkIGludCB5X3Zpcl9zdHJpZGU7Cj4+ICsgICAgICAg dW5zaWduZWQgaW50IHV2X3Zpcl9zdHJpZGU7Cj4+ICAgICAgICAgIGRtYV9hZGRyX3QgeXJnYl9t c3Q7Cj4+ICsgICAgICAgZG1hX2FkZHJfdCB1dl9tc3Q7Cj4+ICAgICAgICAgIGVudW0gdm9wX2Rh dGFfZm9ybWF0IGZvcm1hdDsKPj4gICAgICAgICAgdWludDMyX3QgdmFsOwo+PiAgICAgICAgICBi b29sIGlzX2FscGhhOwo+PiArICAgICAgIGJvb2wgaXNfeXV2Owo+PiAgICAgICAgICBib29sIHZp c2libGU7Cj4+ICAgICAgICAgIGludCByZXQ7Cj4+ICAgICAgICAgIHN0cnVjdCBkcm1fcmVjdCBk ZXN0ID0gewo+PiBAQCAtNjA4LDYgKzYyNSwxMiBAQCBzdGF0aWMgaW50IHZvcF91cGRhdGVfcGxh bmVfZXZlbnQoc3RydWN0IGRybV9wbGFuZSAqcGxhbmUsCj4+ICAgICAgICAgIH07Cj4+ICAgICAg ICAgIGJvb2wgY2FuX3Bvc2l0aW9uID0gcGxhbmUtPnR5cGUgIT0gRFJNX1BMQU5FX1RZUEVfUFJJ TUFSWTsKPj4KPj4gKyAgICAgICBpZiAoZHJtX2Zvcm1hdF9udW1fcGxhbmVzKGZiLT5waXhlbF9m b3JtYXQpID4gMikgewo+PiArICAgICAgICAgICAgICAgRFJNX0VSUk9SKCJ1bnN1cHBvcnQgbW9y ZSB0aGFuIDIgcGxhbmUgZm9ybWF0WyUwOHhdXG4iLAo+PiArICAgICAgICAgICAgICAgICAgICAg ICAgIGZiLT5waXhlbF9mb3JtYXQpOwo+PiArICAgICAgICAgICAgICAgcmV0dXJuIC1FSU5WQUw7 Cj4+ICsgICAgICAgfQo+IEhtbSwgZG8geW91IG5lZWQgdG8gY2hlY2sgdGhpcz8gRG9lc24ndCB0 aGUgY29yZSBndWFyYW50ZWUgdGhhdCB3aXRoCj4gZ2l2ZW4gcGl4ZWxfZm9ybWF0IHlvdSBhbHdh eXMgZ2V0IHRoZSByaWdodCBwbGFuZSBjb3VudD8gKFBvc3NpYmx5IGF0Cj4gZmIgY3JlYXRpb24g dGltZSwgYnV0IEkgaGF2ZW4ndCBjaGVja2VkIHRoYXQuKQoKSSBqdXN0IHdhbnQgdG8gcG9pbnQg b3V0IHRoYXQgdXBkYXRlX3BsYW5lIGNhbid0IGhhbmRsZSBidWZmZXIgbnVtYmVyID4gCjIgY2Fz ZS4KCkJ1dCBzaW5jZSBhbGwgd2luZG93cyBjYW4ndCBzdXBwb3J0IDMgYnVmZmVyIGNvdW50IGZv cm1hdCwgdGhpcyBjaGVjayAKY2FuIHJlbW92ZS4KCj4+ICsKPj4gICAgICAgICAgcmV0ID0gZHJt X3BsYW5lX2hlbHBlcl9jaGVja191cGRhdGUocGxhbmUsIGNydGMsIGZiLAo+PiAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAmc3JjLCAmZGVzdCwgJmNsaXAsCj4+ ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIERSTV9QTEFORV9I RUxQRVJfTk9fU0NBTElORywKPj4gQEAgLTYyNCwyOCArNjQ3LDUyIEBAIHN0YXRpYyBpbnQgdm9w X3VwZGF0ZV9wbGFuZV9ldmVudChzdHJ1Y3QgZHJtX3BsYW5lICpwbGFuZSwKPj4gICAgICAgICAg aWYgKGZvcm1hdCA8IDApCj4+ICAgICAgICAgICAgICAgICAgcmV0dXJuIGZvcm1hdDsKPj4KPj4g KyAgICAgICBpc195dXYgPSBpc195dXZfc3VwcG9ydChmYi0+cGl4ZWxfZm9ybWF0KTsKPiBuaXQ6 IENvdWxkIHlvdSBncm91cCB0aGlzIHRvZ2V0aGVyIHdpdGggb3RoZXIgaXNfKiBhc3NpZ25tZW50 cywgYWJvdmUKPiB0aGUgY2FsbCB0byB2b3BfY29udmVydF9mb3JtYXQoKT8KT0suCj4+ICsKPj4g ICAgICAgICAgb2JqID0gcm9ja2NoaXBfZmJfZ2V0X2dlbV9vYmooZmIsIDApOwo+PiAgICAgICAg ICBpZiAoIW9iaikgewo+PiAgICAgICAgICAgICAgICAgIERSTV9FUlJPUigiZmFpbCB0byBnZXQg cm9ja2NoaXAgZ2VtIG9iamVjdCBmcm9tIGZyYW1lYnVmZmVyXG4iKTsKPj4gICAgICAgICAgICAg ICAgICByZXR1cm4gLUVJTlZBTDsKPj4gICAgICAgICAgfQo+Pgo+PiArICAgICAgIGlmIChpc195 dXYpIHsKPj4gKyAgICAgICAgICAgICAgIHNyYy54MSAmPSAofjEpIDw8IDE2Owo+PiArICAgICAg ICAgICAgICAgc3JjLnkxICY9ICh+MSkgPDwgMTY7Cj4gSG1tLCBpZiB5b3UgYWxpZ24geDEgYW5k IHkxLCBzaG91bGRuJ3QgeW91IGFsc28gb2Zmc2V0IHgyIGFuZCB5Miwgc28KPiB0aGUgd2lkdGgg YW5kIGhlaWdodCBvZiB0aGUgcmVjdGFuZ2xlIGFyZSBwcmVzZXJ2ZWQ/IEFsc28gSSBjb3VsZG4n dAo+IGZpbmQgYW55IGRldGFpbHMgb24gdGhpcywgYnV0IHdoYXQgYXJlIHRoZSBzZW1hbnRpY3Mg b2YKPiAudXBkYXRlX3BsYW5lKCksIHNob3VsZCBpdCByZWFsbHkgYWxpZ24gdGhlIHZhbHVlcyBv ciBtYXliZSBqdXN0IGZhaWw/Cgpmb3IgeXV2IGZvcm1hdCwgdGhlIGJ1ZmZlciBzdGFydCBwb2lu dCBuZWVkIGFsaWduLCBjYW4ndCBiZSBvZGQuCgpPSywgSSB3aWxsIGZpeCB0aGUgeDIgYW5kIHky IG9mZnNldC4KCj4+ICsgICAgICAgfQo+PiArCj4+ICAgICAgICAgIHJrX29iaiA9IHRvX3JvY2tj aGlwX29iaihvYmopOwo+Pgo+PiAgICAgICAgICBhY3R1YWxfdyA9IChzcmMueDIgLSBzcmMueDEp ID4+IDE2Owo+PiAgICAgICAgICBhY3R1YWxfaCA9IChzcmMueTIgLSBzcmMueTEpID4+IDE2Owo+ PiAtICAgICAgIGNydGNfeCA9IG1heCgwLCBjcnRjX3gpOwo+PiAtICAgICAgIGNydGNfeSA9IG1h eCgwLCBjcnRjX3kpOwo+Pgo+PiAtICAgICAgIGRzcF9zdHggPSBjcnRjX3ggKyBjcnRjLT5tb2Rl Lmh0b3RhbCAtIGNydGMtPm1vZGUuaHN5bmNfc3RhcnQ7Cj4+IC0gICAgICAgZHNwX3N0eSA9IGNy dGNfeSArIGNydGMtPm1vZGUudnRvdGFsIC0gY3J0Yy0+bW9kZS52c3luY19zdGFydDsKPj4gKyAg ICAgICBkc3Bfc3R4ID0gZGVzdC54MSArIGNydGMtPm1vZGUuaHRvdGFsIC0gY3J0Yy0+bW9kZS5o c3luY19zdGFydDsKPj4gKyAgICAgICBkc3Bfc3R5ID0gZGVzdC55MSArIGNydGMtPm1vZGUudnRv dGFsIC0gY3J0Yy0+bW9kZS52c3luY19zdGFydDsKPiBUaGlzIGNoYW5nZSBjb3VsZCBiZSBzcGxp dCBpbnRvIHNlcGFyYXRlIHBhdGNoLCB3aGljaCBhY3R1YWxseSBmaXhlcwo+IHRoZSBjb29yZGlu YXRlcyB1c2VkIGZvciB0aGlzIGNhbGN1bGF0aW9uLCBiZWNhdXNlIHRoYXQncyB3aHkgd2UgZG8K PiBjbGlwcGluZyB3aXRoIGRybV9wbGFuZV9oZWxwZXJfY2hlY2tfdXBkYXRlKCkgZmlyc3QgdG8g dXNlIGRlc3Qgbm90Cj4gY3J0Y197eCx5fS4KT0sKPj4gLSAgICAgICBvZmZzZXQgPSAoc3JjLngx ID4+IDE2KSAqIChmYi0+Yml0c19wZXJfcGl4ZWwgPj4gMyk7Cj4+ICsgICAgICAgb2Zmc2V0ID0g KHNyYy54MSA+PiAxNikgKiBkcm1fZm9ybWF0X3BsYW5lX2NwcChmYi0+cGl4ZWxfZm9ybWF0LCAw KTsKPj4gICAgICAgICAgb2Zmc2V0ICs9IChzcmMueTEgPj4gMTYpICogZmItPnBpdGNoZXNbMF07 Cj4+IC0gICAgICAgeXJnYl9tc3QgPSBya19vYmotPmRtYV9hZGRyICsgb2Zmc2V0Owo+Pgo+PiAr ICAgICAgIHlyZ2JfbXN0ID0gcmtfb2JqLT5kbWFfYWRkciArIG9mZnNldCArIGZiLT5vZmZzZXRz WzBdOwo+IFRoaXMgKG1pc3Npbmcgb2Zmc2V0c1swXSBhZGRpdGlvbikgc2hvdWxkIGFsc28gYmUg YSBzZXBhcmF0ZSBwYXRjaCwKPiBiZWNhdXNlIGl0IHdhcyBvYnZpb3VzbHkgaW5jb3JyZWN0IGJl Zm9yZSB0aGlzIHBhdGNoLgpPSy4KPgo+IEJlc3QgcmVnYXJkcywKPiBUb21hc3oKPgo+Cj4KCgot LSAK77ytYXJrIFlhbwoKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9w Lm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRl dmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: mark.yao@rock-chips.com (Mark yao) Date: Thu, 02 Jul 2015 14:53:38 +0800 Subject: [PATCH v2 2/5] drm/rockchip: vop: fix yuv plane support In-Reply-To: References: <1435313249-4549-1-git-send-email-mark.yao@rock-chips.com> <1435313249-4549-3-git-send-email-mark.yao@rock-chips.com> Message-ID: <5594DFF2.8020609@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Tomasz Thanks for your review, I will fix it soon. On 2015?07?02? 14:00, Tomasz Figa wrote: > Hi Mark, > > Please see my comments inline. > > On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao wrote: >> vop support yuv with NV12, NV16 and NV24, only 2 plane yuv. >> >> Signed-off-by: Mark Yao >> >> Changes in v2: >> - Uv buffer not support odd offset, align it. >> - Fix error display when move yuv image. >> >> --- >> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 63 ++++++++++++++++++++++++--- >> 1 file changed, 57 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> index 3c9f4f3..6ca08f8 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> @@ -373,6 +373,18 @@ static enum vop_data_format vop_convert_format(uint32_t format) >> } >> } >> >> +static bool is_yuv_support(uint32_t format) >> +{ >> + switch (format) { >> + case DRM_FORMAT_NV12: >> + case DRM_FORMAT_NV16: >> + case DRM_FORMAT_NV24: >> + return true; >> + default: >> + return false; >> + } >> +} >> + >> static bool is_alpha_support(uint32_t format) >> { >> switch (format) { >> @@ -577,16 +589,21 @@ static int vop_update_plane_event(struct drm_plane *plane, >> struct vop *vop = to_vop(crtc); >> struct drm_gem_object *obj; >> struct rockchip_gem_object *rk_obj; >> + struct drm_gem_object *uv_obj; >> + struct rockchip_gem_object *rk_uv_obj; >> unsigned long offset; >> unsigned int actual_w; >> unsigned int actual_h; >> unsigned int dsp_stx; >> unsigned int dsp_sty; >> unsigned int y_vir_stride; >> + unsigned int uv_vir_stride; >> dma_addr_t yrgb_mst; >> + dma_addr_t uv_mst; >> enum vop_data_format format; >> uint32_t val; >> bool is_alpha; >> + bool is_yuv; >> bool visible; >> int ret; >> struct drm_rect dest = { >> @@ -608,6 +625,12 @@ static int vop_update_plane_event(struct drm_plane *plane, >> }; >> bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY; >> >> + if (drm_format_num_planes(fb->pixel_format) > 2) { >> + DRM_ERROR("unsupport more than 2 plane format[%08x]\n", >> + fb->pixel_format); >> + return -EINVAL; >> + } > Hmm, do you need to check this? Doesn't the core guarantee that with > given pixel_format you always get the right plane count? (Possibly at > fb creation time, but I haven't checked that.) I just want to point out that update_plane can't handle buffer number > 2 case. But since all windows can't support 3 buffer count format, this check can remove. >> + >> ret = drm_plane_helper_check_update(plane, crtc, fb, >> &src, &dest, &clip, >> DRM_PLANE_HELPER_NO_SCALING, >> @@ -624,28 +647,52 @@ static int vop_update_plane_event(struct drm_plane *plane, >> if (format < 0) >> return format; >> >> + is_yuv = is_yuv_support(fb->pixel_format); > nit: Could you group this together with other is_* assignments, above > the call to vop_convert_format()? OK. >> + >> obj = rockchip_fb_get_gem_obj(fb, 0); >> if (!obj) { >> DRM_ERROR("fail to get rockchip gem object from framebuffer\n"); >> return -EINVAL; >> } >> >> + if (is_yuv) { >> + src.x1 &= (~1) << 16; >> + src.y1 &= (~1) << 16; > Hmm, if you align x1 and y1, shouldn't you also offset x2 and y2, so > the width and height of the rectangle are preserved? Also I couldn't > find any details on this, but what are the semantics of > .update_plane(), should it really align the values or maybe just fail? for yuv format, the buffer start point need align, can't be odd. OK, I will fix the x2 and y2 offset. >> + } >> + >> rk_obj = to_rockchip_obj(obj); >> >> actual_w = (src.x2 - src.x1) >> 16; >> actual_h = (src.y2 - src.y1) >> 16; >> - crtc_x = max(0, crtc_x); >> - crtc_y = max(0, crtc_y); >> >> - dsp_stx = crtc_x + crtc->mode.htotal - crtc->mode.hsync_start; >> - dsp_sty = crtc_y + crtc->mode.vtotal - crtc->mode.vsync_start; >> + dsp_stx = dest.x1 + crtc->mode.htotal - crtc->mode.hsync_start; >> + dsp_sty = dest.y1 + crtc->mode.vtotal - crtc->mode.vsync_start; > This change could be split into separate patch, which actually fixes > the coordinates used for this calculation, because that's why we do > clipping with drm_plane_helper_check_update() first to use dest not > crtc_{x,y}. OK >> - offset = (src.x1 >> 16) * (fb->bits_per_pixel >> 3); >> + offset = (src.x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0); >> offset += (src.y1 >> 16) * fb->pitches[0]; >> - yrgb_mst = rk_obj->dma_addr + offset; >> >> + yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0]; > This (missing offsets[0] addition) should also be a separate patch, > because it was obviously incorrect before this patch. OK. > > Best regards, > Tomasz > > > -- ?ark Yao From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752883AbbGBGxx (ORCPT ); Thu, 2 Jul 2015 02:53:53 -0400 Received: from regular1.263xmail.com ([211.150.99.130]:50998 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752355AbbGBGxr (ORCPT ); Thu, 2 Jul 2015 02:53:47 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: mark.yao@rock-chips.com X-FST-TO: tfiga@chromium.org X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: mark.yao@rock-chips.com X-UNIQUE-TAG: <0160e5bf544a0de1306ddc64678805a8> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <5594DFF2.8020609@rock-chips.com> Date: Thu, 02 Jul 2015 14:53:38 +0800 From: Mark yao User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.2.0 MIME-Version: 1.0 To: Tomasz Figa CC: dri-devel , David Airlie , Heiko Stuebner , Daniel Kurtz , Philipp Zabel , Daniel Vetter , Rob Clark , "linux-arm-kernel@lists.infradead.org" , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" , sandy.huang@rock-chips.com, dkm@rock-chips.com, zwl@rock-chips.com, xw@rock-chips.com Subject: Re: [PATCH v2 2/5] drm/rockchip: vop: fix yuv plane support References: <1435313249-4549-1-git-send-email-mark.yao@rock-chips.com> <1435313249-4549-3-git-send-email-mark.yao@rock-chips.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Tomasz Thanks for your review, I will fix it soon. On 2015年07月02日 14:00, Tomasz Figa wrote: > Hi Mark, > > Please see my comments inline. > > On Fri, Jun 26, 2015 at 7:07 PM, Mark Yao wrote: >> vop support yuv with NV12, NV16 and NV24, only 2 plane yuv. >> >> Signed-off-by: Mark Yao >> >> Changes in v2: >> - Uv buffer not support odd offset, align it. >> - Fix error display when move yuv image. >> >> --- >> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 63 ++++++++++++++++++++++++--- >> 1 file changed, 57 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> index 3c9f4f3..6ca08f8 100644 >> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c >> @@ -373,6 +373,18 @@ static enum vop_data_format vop_convert_format(uint32_t format) >> } >> } >> >> +static bool is_yuv_support(uint32_t format) >> +{ >> + switch (format) { >> + case DRM_FORMAT_NV12: >> + case DRM_FORMAT_NV16: >> + case DRM_FORMAT_NV24: >> + return true; >> + default: >> + return false; >> + } >> +} >> + >> static bool is_alpha_support(uint32_t format) >> { >> switch (format) { >> @@ -577,16 +589,21 @@ static int vop_update_plane_event(struct drm_plane *plane, >> struct vop *vop = to_vop(crtc); >> struct drm_gem_object *obj; >> struct rockchip_gem_object *rk_obj; >> + struct drm_gem_object *uv_obj; >> + struct rockchip_gem_object *rk_uv_obj; >> unsigned long offset; >> unsigned int actual_w; >> unsigned int actual_h; >> unsigned int dsp_stx; >> unsigned int dsp_sty; >> unsigned int y_vir_stride; >> + unsigned int uv_vir_stride; >> dma_addr_t yrgb_mst; >> + dma_addr_t uv_mst; >> enum vop_data_format format; >> uint32_t val; >> bool is_alpha; >> + bool is_yuv; >> bool visible; >> int ret; >> struct drm_rect dest = { >> @@ -608,6 +625,12 @@ static int vop_update_plane_event(struct drm_plane *plane, >> }; >> bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY; >> >> + if (drm_format_num_planes(fb->pixel_format) > 2) { >> + DRM_ERROR("unsupport more than 2 plane format[%08x]\n", >> + fb->pixel_format); >> + return -EINVAL; >> + } > Hmm, do you need to check this? Doesn't the core guarantee that with > given pixel_format you always get the right plane count? (Possibly at > fb creation time, but I haven't checked that.) I just want to point out that update_plane can't handle buffer number > 2 case. But since all windows can't support 3 buffer count format, this check can remove. >> + >> ret = drm_plane_helper_check_update(plane, crtc, fb, >> &src, &dest, &clip, >> DRM_PLANE_HELPER_NO_SCALING, >> @@ -624,28 +647,52 @@ static int vop_update_plane_event(struct drm_plane *plane, >> if (format < 0) >> return format; >> >> + is_yuv = is_yuv_support(fb->pixel_format); > nit: Could you group this together with other is_* assignments, above > the call to vop_convert_format()? OK. >> + >> obj = rockchip_fb_get_gem_obj(fb, 0); >> if (!obj) { >> DRM_ERROR("fail to get rockchip gem object from framebuffer\n"); >> return -EINVAL; >> } >> >> + if (is_yuv) { >> + src.x1 &= (~1) << 16; >> + src.y1 &= (~1) << 16; > Hmm, if you align x1 and y1, shouldn't you also offset x2 and y2, so > the width and height of the rectangle are preserved? Also I couldn't > find any details on this, but what are the semantics of > .update_plane(), should it really align the values or maybe just fail? for yuv format, the buffer start point need align, can't be odd. OK, I will fix the x2 and y2 offset. >> + } >> + >> rk_obj = to_rockchip_obj(obj); >> >> actual_w = (src.x2 - src.x1) >> 16; >> actual_h = (src.y2 - src.y1) >> 16; >> - crtc_x = max(0, crtc_x); >> - crtc_y = max(0, crtc_y); >> >> - dsp_stx = crtc_x + crtc->mode.htotal - crtc->mode.hsync_start; >> - dsp_sty = crtc_y + crtc->mode.vtotal - crtc->mode.vsync_start; >> + dsp_stx = dest.x1 + crtc->mode.htotal - crtc->mode.hsync_start; >> + dsp_sty = dest.y1 + crtc->mode.vtotal - crtc->mode.vsync_start; > This change could be split into separate patch, which actually fixes > the coordinates used for this calculation, because that's why we do > clipping with drm_plane_helper_check_update() first to use dest not > crtc_{x,y}. OK >> - offset = (src.x1 >> 16) * (fb->bits_per_pixel >> 3); >> + offset = (src.x1 >> 16) * drm_format_plane_cpp(fb->pixel_format, 0); >> offset += (src.y1 >> 16) * fb->pitches[0]; >> - yrgb_mst = rk_obj->dma_addr + offset; >> >> + yrgb_mst = rk_obj->dma_addr + offset + fb->offsets[0]; > This (missing offsets[0] addition) should also be a separate patch, > because it was obviously incorrect before this patch. OK. > > Best regards, > Tomasz > > > -- Ｍark Yao