Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758492AbbLBOSj (ORCPT ); Wed, 2 Dec 2015 09:18:39 -0500 Received: from mail-wm0-f53.google.com ([74.125.82.53]:35812 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754492AbbLBOSh convert rfc822-to-8bit (ORCPT ); Wed, 2 Dec 2015 09:18:37 -0500 MIME-Version: 1.0 In-Reply-To: <565D68F0.3010905@rock-chips.com> References: <1448940391-23333-1-git-send-email-mark.yao@rock-chips.com> <1448940391-23333-4-git-send-email-mark.yao@rock-chips.com> <565D68F0.3010905@rock-chips.com> Date: Wed, 2 Dec 2015 14:18:35 +0000 Message-ID: Subject: Re: [RFC PATCH 3/9] drm/rockchip: Convert to support atomic API From: Daniel Stone To: Mark yao Cc: David Airlie , Heiko Stuebner , dri-devel , "linux-arm-kernel@lists.infradead.org" , linux-rockchip , Linux Kernel Mailing List , Tomasz Figa Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4851 Lines: 120 Hi Mark, Thanks for getting back to this. On 1 December 2015 at 09:31, Mark yao wrote: > On 2015年12月01日 16:18, Daniel Stone wrote: >> On 1 December 2015 at 03:26, Mark Yao wrote: >>> >+ 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. >> > Hi Daniel > "if this passed in an explicit pointer to state, or an address to wait > for", I don't understand, can you point how it work? Ah, OK. I mean that rockchip_crtc_wait_for_update takes a drm_crtc pointer, and establishes the state from that (e.g. crtc->primary->state). This can easily lead to confusion in async contexts, as the states attached to a drm_crtc and a drm_plane can change here while you wait for it. It would be better if the call was: for_each_plane_in_state(state, plane, plane_state, i) { if (plane->type == DRM_PLANE_TYPE_PRIMARY) rockchip_crtc_wait_for_update(plane_state->crtc, plane_state); } This way it is very clear, and there is no confusion as to which request we are waiting to complete. In general, using crtc->state or plane->state is a very bad idea, _except_ in the atomic_check function where you are calculating changes (e.g. if (plane_state->fb->pitches[0] != plane->state->fb->pitches[0]) rockchip_plane_state->pitch_changed = true). After atomic_check, you should always pass around pointers to the plane state explicitly, and avoid using the pointers from drm_crtc and drm_plane. Does that help? >>> 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. For me, it is more confusing when the display shows something different to what I have requested. In some media usecases, doing this is a showstopper and will result in products failing acceptance testing. Userspace can make a policy decision to try different alignments to get _something_ to show (even if it's not what was explicitly requested), but doing this in the kernel is inappropriate: please just reject it, and have userspace fall back to another composition method (e.g. GL) in these cases. >>> -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. Ah yes, you're right. But still, using that would be better than duplicating it. > Can you share your Weston environment to me, I'm interesting to test drm > rockchip on weston. Of course. You can download Weston from http://wayland.freedesktop.org - the most interesting dependencies are libevdev, libinput, and wayland itself. If you are building newer Weston from git, you'll need the wayland-protocols repository as well, from anongit.freedesktop.org/git/wayland/wayland-protocols/. Please let me know privately if you need some more help with building these, but they should be quite straightforward. Cheers, Daniel -- 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/