Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755837AbbLAJWH (ORCPT ); Tue, 1 Dec 2015 04:22:07 -0500 Received: from regular1.263xmail.com ([211.150.99.137]:37718 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755589AbbLAJWB (ORCPT ); Tue, 1 Dec 2015 04:22:01 -0500 X-263anti-spam: KSV:0;BIG:0;ABS:1;DNS:0;ATT:0;SPF:S; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 1 X-SKE-CHECKED: 1 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: <7bf294405a0a267e84df0354f97be14e> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <565D66A1.7030900@rock-chips.com> Date: Tue, 01 Dec 2015 17:21:37 +0800 From: Mark yao User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Daniel Stone CC: David Airlie , Heiko Stuebner , dri-devel , "linux-arm-kernel@lists.infradead.org" , linux-rockchip , Linux Kernel Mailing List , Tomasz Figa Subject: Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API References: <1448940391-23333-1-git-send-email-mark.yao@rock-chips.com> <1448940391-23333-4-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 Content-Length: 9475 Lines: 238 On 2015年12月01日 16:18, Daniel Stone wrote: > Hi Mark, > > On 1 December 2015 at 03:26, Mark Yao wrote: >> +static void rockchip_atomic_wait_for_complete(struct drm_atomic_state *state) >> +{ >> + struct drm_crtc_state *crtc_state; >> + struct drm_crtc *crtc; >> + int i; >> + >> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + if (!crtc->state->active) >> + continue; >> + >> + WARN_ON(drm_crtc_vblank_get(crtc)); >> + } >> + >> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + if (!crtc->state->active) >> + continue; >> + >> + rockchip_crtc_wait_for_update(crtc); >> + } > I'd be much more comfortable if this passed in an explicit pointer to > state, or an address to wait for, rather than have wait_for_complete > dig out state with no locking. The latter is potentially racy for > async operations. > >> +struct vop_plane_state { >> + struct drm_plane_state base; >> + dma_addr_t dma_addr[ROCKCHIP_MAX_FB_BUFFER]; > Can you get rid of this by just using the pointer to a > rockchip_gem_object already provided? Good idea, I will do that. >> -static int vop_update_plane_event(struct drm_plane *plane, >> - struct drm_crtc *crtc, >> - struct drm_framebuffer *fb, int crtc_x, >> - int crtc_y, unsigned int crtc_w, >> - unsigned int crtc_h, uint32_t src_x, >> - uint32_t src_y, uint32_t src_w, >> - uint32_t src_h, >> - struct drm_pending_vblank_event *event) >> +static int vop_plane_atomic_check(struct drm_plane *plane, >> + struct drm_plane_state *state) >> { >> + struct drm_crtc *crtc = state->crtc; >> + struct drm_framebuffer *fb = state->fb; >> struct vop_win *vop_win = to_vop_win(plane); >> + struct vop_plane_state *vop_plane_state = to_vop_plane_state(state); >> const struct vop_win_data *win = vop_win->data; >> - 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 = 0; >> - dma_addr_t yrgb_mst; >> - dma_addr_t uv_mst = 0; >> - enum vop_data_format format; >> - uint32_t val; >> - bool is_alpha; >> - bool rb_swap; >> bool is_yuv; >> bool visible; >> int ret; >> - struct drm_rect dest = { >> - .x1 = crtc_x, >> - .y1 = crtc_y, >> - .x2 = crtc_x + crtc_w, >> - .y2 = crtc_y + crtc_h, >> - }; >> - struct drm_rect src = { >> - /* 16.16 fixed point */ >> - .x1 = src_x, >> - .y1 = src_y, >> - .x2 = src_x + src_w, >> - .y2 = src_y + src_h, >> - }; >> - const struct drm_rect clip = { >> - .x2 = crtc->mode.hdisplay, >> - .y2 = crtc->mode.vdisplay, >> - }; >> - bool can_position = plane->type != DRM_PLANE_TYPE_PRIMARY; >> + struct drm_rect *dest = &vop_plane_state->dest; >> + struct drm_rect *src = &vop_plane_state->src; >> + struct drm_rect clip; >> int min_scale = win->phy->scl ? FRAC_16_16(1, 8) : >> DRM_PLANE_HELPER_NO_SCALING; >> int max_scale = win->phy->scl ? FRAC_16_16(8, 1) : >> DRM_PLANE_HELPER_NO_SCALING; >> >> - ret = drm_plane_helper_check_update(plane, crtc, fb, >> - &src, &dest, &clip, >> + crtc = crtc ? crtc : plane->state->crtc; >> + /* >> + * Both crtc or plane->state->crtc can be null. >> + */ >> + if (!crtc || !fb) >> + goto out_disable; >> + src->x1 = state->src_x; >> + src->y1 = state->src_y; >> + src->x2 = state->src_x + state->src_w; >> + src->y2 = state->src_y + state->src_h; >> + dest->x1 = state->crtc_x; >> + dest->y1 = state->crtc_y; >> + dest->x2 = state->crtc_x + state->crtc_w; >> + dest->y2 = state->crtc_y + state->crtc_h; >> + >> + clip.x1 = 0; >> + clip.y1 = 0; >> + clip.x2 = crtc->mode.hdisplay; >> + clip.y2 = crtc->mode.vdisplay; >> + >> + ret = drm_plane_helper_check_update(plane, crtc, state->fb, >> + src, dest, &clip, >> min_scale, >> max_scale, >> - can_position, false, &visible); >> + true, true, &visible); >> if (ret) >> return ret; >> >> if (!visible) >> - return 0; >> - >> - is_alpha = is_alpha_support(fb->pixel_format); >> - rb_swap = has_rb_swapped(fb->pixel_format); >> - is_yuv = is_yuv_support(fb->pixel_format); >> + goto out_disable; >> >> - format = vop_convert_format(fb->pixel_format); >> - if (format < 0) >> - return format; >> + vop_plane_state->format = vop_convert_format(fb->pixel_format); >> + if (vop_plane_state->format < 0) >> + return vop_plane_state->format; >> >> - obj = rockchip_fb_get_gem_obj(fb, 0); >> - if (!obj) { >> - DRM_ERROR("fail to get rockchip gem object from framebuffer\n"); >> - return -EINVAL; >> - } >> - >> - rk_obj = to_rockchip_obj(obj); >> + is_yuv = is_yuv_support(fb->pixel_format); >> >> if (is_yuv) { >> /* >> * Src.x1 can be odd when do clip, but yuv plane start point >> * need align with 2 pixel. >> */ >> - val = (src.x1 >> 16) % 2; >> - src.x1 += val << 16; >> - src.x2 += val << 16; >> + uint32_t temp = (src->x1 >> 16) % 2; >> + >> + src->x1 += temp << 16; >> + src->x2 += temp << 16; >> } > I know this isn't new, but moving the plane around is bad. If the user > gives you a pixel boundary that you can't actually use, please reject > the configuration rather than silently trying to fix it up. the origin src.x1 would align with 2 pixel, but when we move the dest window, and do clip by output, the src.x1 may be clipped to odd. regect this configuration may let user confuse, sometimes good, sometimes bad. >> -static void vop_plane_destroy(struct drm_plane *plane) >> +static void vop_atomic_plane_destroy_state(struct drm_plane *plane, >> + struct drm_plane_state *state) >> { >> - vop_disable_plane(plane); >> - drm_plane_cleanup(plane); >> + struct vop_plane_state *vop_state = to_vop_plane_state(state); >> + >> + if (state->fb) >> + drm_framebuffer_unreference(state->fb); >> + >> + kfree(vop_state); >> } > You can replace this hook with drm_atomic_helper_plane_destroy_state. Hmm, only can hook with __drm_atomic_helper_plane_destroy_state. >> -static void vop_win_state_complete(struct vop_win *vop_win, >> - struct vop_win_state *state) >> -{ >> - struct vop *vop = vop_win->vop; >> - struct drm_crtc *crtc = &vop->crtc; >> - struct drm_device *drm = crtc->dev; >> - unsigned long flags; >> - >> - if (state->event) { >> - spin_lock_irqsave(&drm->event_lock, flags); >> - drm_crtc_send_vblank_event(crtc, state->event); >> - spin_unlock_irqrestore(&drm->event_lock, flags); >> - } > Ah, I see this is based on top of the patches I sent to fix pageflips? > If they have been merged, I would like to send you some others to > merge as well, which fix an OOPS when you close Weston. I didn't send > them yet as I didn't get a response to the previous patchset, so did > not know you had merged it. There are a lot of other correctness fixes > I had in there (e.g. using the CRTC vblank functions, some locking > fixes), but if this code is going to be thrown away then I will just > discard most of them as well. > > Still, I would like to prepare another series for you to merge through > drm-fixes, to be able to run Weston (the same-fb-flip patch I sent > along with the drm_crtc_send_vblank_event conversion, also adding a > preclose hook to discard events when clients exit). > > Cheers, > Daniel > > > Hi Daniel Can you share your Weston environment to me, I'm interesting to test drm rockchip on weston. -- Ｍark Yao -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/