From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S968096AbdADNqA (ORCPT ); Wed, 4 Jan 2017 08:46:00 -0500 Received: from mail-wm0-f41.google.com ([74.125.82.41]:36662 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967153AbdADNpw (ORCPT ); Wed, 4 Jan 2017 08:45:52 -0500 Subject: Re: [PATCH] drm/meson: Fix plane atomic check when no crtc for the plane To: =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= References: <1483435254-27955-1-git-send-email-narmstrong@baylibre.com> <20170104132737.GF31595@intel.com> Cc: airlied@linux.ie, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, dri-devel@lists.freedesktop.org From: Neil Armstrong Organization: Baylibre Message-ID: <50f16b43-331a-5549-478c-83583f275f91@baylibre.com> Date: Wed, 4 Jan 2017 14:45:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-Version: 1.0 In-Reply-To: <20170104132737.GF31595@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/04/2017 02:27 PM, Ville Syrjälä wrote: > On Tue, Jan 03, 2017 at 10:20:54AM +0100, Neil Armstrong wrote: >> When no CRTC is associated with the plane, the meson_plane_atomic_check() >> call breaks the kernel with an Oops. >> >> Fixes: bbbe775ec5b5 ("drm: Add support for Amlogic Meson Graphic Controller") >> Signed-off-by: Neil Armstrong >> --- >> drivers/gpu/drm/meson/meson_plane.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c >> index 4942ca0..7890e30 100644 >> --- a/drivers/gpu/drm/meson/meson_plane.c >> +++ b/drivers/gpu/drm/meson/meson_plane.c >> @@ -51,6 +51,9 @@ static int meson_plane_atomic_check(struct drm_plane *plane, >> struct drm_crtc_state *crtc_state; >> struct drm_rect clip = { 0, }; >> >> + if (!state->crtc) >> + return 0; >> + > > Since you're not going to call drm_plane_helper_check_state() you will > fail to update plane_state->visible. Indeed I forgot about that, but i's not necessary yet since visibility is not yet handled. > > What i915 does in this case is look for state->crtc first, and if that's > NULL it'll pick the crtc from the old state (plane->state->crtc). It > looks a bit ugly, but maybe we should hide it in a small helper function > that could be used by all drivers? Yes, an helper would be helpful. > >> crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); >> if (IS_ERR(crtc_state)) >> return PTR_ERR(crtc_state); >> -- >> 1.9.1 Thanks, Neil