Received: by 2002:a05:7412:a9a2:b0:e2:908c:2ebd with SMTP id o34csp317323rdh; Thu, 26 Oct 2023 03:10:18 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEBT66uMCdDvk5/zNikEAHEsCXXqp6bPuDR4WjgqoXn1tyJITB2ZmTq6s40XTxgnDVm/vyC X-Received: by 2002:a05:6102:4719:b0:457:cc32:39c6 with SMTP id ei25-20020a056102471900b00457cc3239c6mr17668975vsb.16.1698315017782; Thu, 26 Oct 2023 03:10:17 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698315017; cv=none; d=google.com; s=arc-20160816; b=nuToifjvD106rRBbM0hvkgg96Xm4b13dS6LN42i35xFKy20h45eSRied7+o/iPD38A Fez3kBrvT6hxxpDqZbmGKlt8+yWaJRVHNprBDnKCkG8nhu8n4ZV6lbPnc7RZ1slYyqxz AcD2i9iyPyCTs1T94680b8uvx3fe9BsLIgLu96wEnC9qG160HwB0KuF4qjRtBq2MNsyW +wizs5Tp0TDbwBhKd8IWz5cT9CBYgDCPt91dmKSJhsH31A4UHAd6ibDLmYRTeVGDn51q CTVDvZYlVH2+VV9JIA8S8NW3Uj8abeEd2RbZRFStu4pinAQIdTEyTvXMUu1qKGi4lT8f r30g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to:references :cc:to:from:content-language:subject:user-agent:mime-version:date :message-id; bh=/RqvkzSVGavSQYrWQ26Bsepy4C9gxsNqxvW4d8rKtfM=; fh=n6j8uZPvz+ZAVuqAS2fp+U3B6foYtDrkffQ3seBzkJM=; b=0dTQr9z6t857eUhgsfOfWxQ00dQkJ9XHNx5RyNwYg3S+AGUtNAafSHwZqcb4KNSXtR XB2ExZWlGMPEZ4D3hqS7YRMcgX/x52Jwn5M/7WvU13DV3pIBEN9WmysCjVj7G0hePdKh +bngukn789Q7U1XNJzGiixdZQAVTHg/7ebUoghPGtEWshqr2gOTvVMGbpsybh1SiBMNT boFNa06okNU7IvVIOkUK/1M+qopf8B3eoM9WZ+R8z+sMLj4uTYITYSiWSl5b0mc/hBqC KH2yYhERaPibcwBX+pAkdDBjdISLzfIQDRB/78ftJfQXzTUaRqxY3RIZEknNO787tR9u r7YA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id t98-20020a25aaeb000000b00d9cab69db82si14804430ybi.91.2023.10.26.03.10.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 26 Oct 2023 03:10:17 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 88A1482159D0; Thu, 26 Oct 2023 03:10:16 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230030AbjJZKKN convert rfc822-to-8bit (ORCPT + 99 others); Thu, 26 Oct 2023 06:10:13 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56030 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229611AbjJZKKM (ORCPT ); Thu, 26 Oct 2023 06:10:12 -0400 Received: from ex01.ufhost.com (ex01.ufhost.com [61.152.239.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 4F552183; Thu, 26 Oct 2023 03:10:07 -0700 (PDT) Received: from EXMBX166.cuchost.com (unknown [175.102.18.54]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "EXMBX166", Issuer "EXMBX166" (not verified)) by ex01.ufhost.com (Postfix) with ESMTP id E605124E214; Thu, 26 Oct 2023 18:10:00 +0800 (CST) Received: from EXMBX061.cuchost.com (172.16.6.61) by EXMBX166.cuchost.com (172.16.6.76) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Thu, 26 Oct 2023 18:10:01 +0800 Received: from [192.168.60.126] (180.164.60.184) by EXMBX061.cuchost.com (172.16.6.61) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Thu, 26 Oct 2023 18:10:00 +0800 Message-ID: Date: Thu, 26 Oct 2023 18:09:59 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.15.1 Subject: Re: [PATCH v2 5/6] drm/vs: Add KMS crtc&plane Content-Language: en-US From: Keith Zhao To: Dmitry Baryshkov CC: , , , , , , Conor Dooley , Albert Ou , "Emil Renner Berthing" , , Thomas Zimmermann , Bjorn Andersson , Chris Morgan , Maxime Ripard , Jagan Teki , Jack Zhu , Rob Herring , Palmer Dabbelt , "Krzysztof Kozlowski" , Paul Walmsley , Shengyang Chen , Changhuang Liang , Shawn Guo , Sumit Semwal References: <20231025103957.3776-1-keith.zhao@starfivetech.com> <20231025103957.3776-6-keith.zhao@starfivetech.com> <6db09f77-31e8-4f2e-a987-e3745d0e8c24@linaro.org> <627a0f60-7da0-6683-a451-42801c513308@starfivetech.com> In-Reply-To: <627a0f60-7da0-6683-a451-42801c513308@starfivetech.com> Content-Type: text/plain; charset="UTF-8" X-Originating-IP: [180.164.60.184] X-ClientProxiedBy: EXCAS066.cuchost.com (172.16.6.26) To EXMBX061.cuchost.com (172.16.6.61) X-YovoleRuleAgent: yovoleflag Content-Transfer-Encoding: 8BIT X-Spam-Status: No, score=-5.2 required=5.0 tests=BAYES_00,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Thu, 26 Oct 2023 03:10:16 -0700 (PDT) sorry Dmitry ,accidentally wrote the wrong name Take no offense On 2023/10/26 17:42, Keith Zhao wrote: > hi Ville: > very glad to receive your feedback > Some of them are very good ideas. > Some are not very clear and hope to get your further reply! > > > On 2023/10/26 3:49, Ville Syrjälä wrote: >> On Wed, Oct 25, 2023 at 10:28:56PM +0300, Dmitry Baryshkov wrote: >>> On 25/10/2023 13:39, Keith Zhao wrote: >>> > add 2 crtcs and 8 planes in vs-drm >>> > >>> > Signed-off-by: Keith Zhao >>> > --- >>> > drivers/gpu/drm/verisilicon/Makefile | 8 +- >>> > drivers/gpu/drm/verisilicon/vs_crtc.c | 257 ++++ >>> > drivers/gpu/drm/verisilicon/vs_crtc.h | 43 + >>> > drivers/gpu/drm/verisilicon/vs_dc.c | 1002 ++++++++++++ >>> > drivers/gpu/drm/verisilicon/vs_dc.h | 80 + >>> > drivers/gpu/drm/verisilicon/vs_dc_hw.c | 1959 ++++++++++++++++++++++++ >>> > drivers/gpu/drm/verisilicon/vs_dc_hw.h | 492 ++++++ >>> > drivers/gpu/drm/verisilicon/vs_drv.c | 2 + >>> > drivers/gpu/drm/verisilicon/vs_plane.c | 526 +++++++ >>> > drivers/gpu/drm/verisilicon/vs_plane.h | 58 + >>> > drivers/gpu/drm/verisilicon/vs_type.h | 69 + >>> > 11 files changed, 4494 insertions(+), 2 deletions(-) >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_crtc.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_dc_hw.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.c >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_plane.h >>> > create mode 100644 drivers/gpu/drm/verisilicon/vs_type.h >>> > >>> > diff --git a/drivers/gpu/drm/verisilicon/Makefile b/drivers/gpu/drm/verisilicon/Makefile >>> > index 7d3be305b..1d48016ca 100644 >>> > --- a/drivers/gpu/drm/verisilicon/Makefile >>> > +++ b/drivers/gpu/drm/verisilicon/Makefile >>> > @@ -1,7 +1,11 @@ >>> > # SPDX-License-Identifier: GPL-2.0 >>> > >>> > -vs_drm-objs := vs_drv.o \ >>> > - vs_modeset.o >>> > +vs_drm-objs := vs_dc_hw.o \ >>> > + vs_dc.o \ >>> > + vs_crtc.o \ >>> > + vs_drv.o \ >>> > + vs_modeset.o \ >>> > + vs_plane.o >>> > >>> > obj-$(CONFIG_DRM_VERISILICON) += vs_drm.o >>> > >>> > diff --git a/drivers/gpu/drm/verisilicon/vs_crtc.c b/drivers/gpu/drm/verisilicon/vs_crtc.c >>> > new file mode 100644 >>> > index 000000000..8a658ea77 >>> > --- /dev/null >>> > +++ b/drivers/gpu/drm/verisilicon/vs_crtc.c >>> > @@ -0,0 +1,257 @@ >>> > +// SPDX-License-Identifier: GPL-2.0 >>> > +/* >>> > + * Copyright (C) 2023 VeriSilicon Holdings Co., Ltd. >>> > + * >>> > + */ >>> > + >>> > +#include >>> > +#include >>> > +#include >>> > + >>> > +#include >>> > +#include >>> > +#include >>> > +#include >>> > +#include >>> > +#include >>> > + >>> > +#include "vs_crtc.h" >>> > +#include "vs_dc.h" >>> > +#include "vs_drv.h" >>> > + >>> > +static void vs_crtc_reset(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc_state *state; >>> > + >>> > + if (crtc->state) { >>> > + __drm_atomic_helper_crtc_destroy_state(crtc->state); >>> > + >>> > + state = to_vs_crtc_state(crtc->state); >>> > + kfree(state); >>> > + crtc->state = NULL; >>> > + } >>> >>> You can call your crtc_destroy_state function directly here. > ok got it ! >>> >>> > + >>> > + state = kzalloc(sizeof(*state), GFP_KERNEL); >>> > + if (!state) >>> > + return; >>> > + >>> > + __drm_atomic_helper_crtc_reset(crtc, &state->base); >>> > +} >>> > + >>> > +static struct drm_crtc_state * >>> > +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc_state *ori_state; >>> >>> It might be a matter of taste, but it is usually old_state. >>> >>> > + struct vs_crtc_state *state; >>> > + >>> > + if (!crtc->state) >>> > + return NULL; >>> > + >>> > + ori_state = to_vs_crtc_state(crtc->state); >>> > + state = kzalloc(sizeof(*state), GFP_KERNEL); >>> > + if (!state) >>> > + return NULL; >>> > + >>> > + __drm_atomic_helper_crtc_duplicate_state(crtc, &state->base); >>> > + >>> > + state->output_fmt = ori_state->output_fmt; >>> > + state->encoder_type = ori_state->encoder_type; >>> > + state->bpp = ori_state->bpp; >>> > + state->underflow = ori_state->underflow; >>> >>> Can you use kmemdup instead? > ok >>> >>> > + >>> > + return &state->base; >>> > +} >>> > + >>> > +static void vs_crtc_atomic_destroy_state(struct drm_crtc *crtc, >>> > + struct drm_crtc_state *state) >>> > +{ >>> > + __drm_atomic_helper_crtc_destroy_state(state); >>> > + kfree(to_vs_crtc_state(state)); >>> > +} >>> > + >>> > +static int vs_crtc_enable_vblank(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + >>> > + vs_dc_enable_vblank(dc, true); >>> > + >>> > + return 0; >>> > +} >>> > + >>> > +static void vs_crtc_disable_vblank(struct drm_crtc *crtc) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + >>> > + vs_dc_enable_vblank(dc, false); >>> > +} >>> > + >>> > +static const struct drm_crtc_funcs vs_crtc_funcs = { >>> > + .set_config = drm_atomic_helper_set_config, >>> > + .page_flip = drm_atomic_helper_page_flip, >>> >>> destroy is required, see drm_mode_config_cleanup() > will add destory >>> >>> > + .reset = vs_crtc_reset, >>> > + .atomic_duplicate_state = vs_crtc_atomic_duplicate_state, >>> > + .atomic_destroy_state = vs_crtc_atomic_destroy_state, >>> >>> please consider adding atomic_print_state to output driver-specific bits. >>> > will add atomic_print_state to print my private definitions >>> > + .enable_vblank = vs_crtc_enable_vblank, >>> > + .disable_vblank = vs_crtc_disable_vblank, >>> > +}; >>> > + >>> > +static u8 cal_pixel_bits(u32 bus_format) >>> >>> This looks like a generic helper code, which can go to a common place. >>> >>> > +{ >>> > + u8 bpp; >>> > + >>> > + switch (bus_format) { >>> > + case MEDIA_BUS_FMT_RGB565_1X16: >>> > + case MEDIA_BUS_FMT_UYVY8_1X16: >>> > + bpp = 16; >>> > + break; >>> > + case MEDIA_BUS_FMT_RGB666_1X18: >>> > + case MEDIA_BUS_FMT_RGB666_1X24_CPADHI: >>> > + bpp = 18; >>> > + break; >>> > + case MEDIA_BUS_FMT_UYVY10_1X20: >>> > + bpp = 20; >>> > + break; >>> > + case MEDIA_BUS_FMT_BGR888_1X24: >>> > + case MEDIA_BUS_FMT_UYYVYY8_0_5X24: >>> > + case MEDIA_BUS_FMT_YUV8_1X24: >>> > + bpp = 24; >>> > + break; >>> > + case MEDIA_BUS_FMT_RGB101010_1X30: >>> > + case MEDIA_BUS_FMT_UYYVYY10_0_5X30: >>> > + case MEDIA_BUS_FMT_YUV10_1X30: >>> > + bpp = 30; >>> > + break; >>> > + default: >>> > + bpp = 24; >>> > + break; >>> > + } >>> > + >>> > + return bpp; >>> > +} >>> > + >>> > +static void vs_crtc_atomic_enable(struct drm_crtc *crtc, >>> > + struct drm_atomic_state *state) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + struct vs_crtc_state *vs_crtc_state = to_vs_crtc_state(crtc->state); >>> > + >>> > + vs_crtc_state->bpp = cal_pixel_bits(vs_crtc_state->output_fmt); >>> > + >>> > + vs_dc_enable(dc, crtc); >>> > + drm_crtc_vblank_on(crtc); >>> > +} >>> > + >>> > +static void vs_crtc_atomic_disable(struct drm_crtc *crtc, >>> > + struct drm_atomic_state *state) >>> > +{ >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct vs_dc *dc = dev_get_drvdata(vs_crtc->dev); >>> > + >>> > + drm_crtc_vblank_off(crtc); >>> > + >>> > + vs_dc_disable(dc, crtc); >>> > + >>> > + if (crtc->state->event && !crtc->state->active) { >>> > + spin_lock_irq(&crtc->dev->event_lock); >>> > + drm_crtc_send_vblank_event(crtc, crtc->state->event); >>> > + spin_unlock_irq(&crtc->dev->event_lock); >>> > + >>> > + crtc->state->event = NULL; >>> >>> I think even should be cleared within the lock. >> >> event_lock doesn't protect anything in the crtc state. > > how about match like this: > spin_lock_irq(&crtc->dev->event_lock); > if (crtc->state->event) { > drm_crtc_send_vblank_event(crtc, crtc->state->event); > crtc->state->event = NULL; > } > spin_unlock_irq(&crtc->dev->event_lock); > >> >> But the bigger problem in this code is the prevalent crtc->state >> usage. That should pretty much never be done, especially in anything >> that can get called from the actual commit phase where you no longer >> have the locks held. Instead one should almost always use the >> get_{new,old}_state() stuff, or the old/new/oldnew state iterators. > > the prevalent crtc->state usage : > crtc->state should not be used directly? > need replace it by get_{new,old}_state() > > for example: > drm_crtc_send_vblank_event(crtc, crtc->state->event); > > > should do like this : > struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc); > ... > drm_crtc_send_vblank_event(crtc, crtc_state->event); > ... > > If there is a problem, help further correct > wish give a example > Thanks > >> >>> >>> > + } >>> > +} >>> > + >>> > +static void vs_crtc_atomic_begin(struct drm_crtc *crtc, >>> > + struct drm_atomic_state *state) >>> > +{ >>> > + struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, >>> > + crtc); >>> > + >>> > + struct vs_crtc *vs_crtc = to_vs_crtc(crtc); >>> > + struct device *dev = vs_crtc->dev; >>> > + struct drm_property_blob *blob = crtc->state->gamma_lut; >> >> Eg. here you are using drm_atomic_get_new_crtc_state() correctly, but >> then proceed to directly access crtc->state anyway. >> > struct drm_property_blob *blob = crtc->state->gamma_lut; > change to > struct drm_property_blob *blob = crtc_state ->gamma_lut; > >>> > + struct drm_color_lut *lut; >>> > + struct vs_dc *dc = dev_get_drvdata(dev); >>> > + >>> > + if (crtc_state->color_mgmt_changed) { >>> > + if (blob && blob->length) { >>> > + lut = blob->data; >>> > + vs_dc_set_gamma(dc, crtc, lut, >>> > + blob->length / sizeof(*lut)); >>> > + vs_dc_enable_gamma(dc, crtc, true); >>> > + } else { >>> > + vs_dc_enable_gamma(dc, crtc, false); >>> > + } >>> > + } >>> > +} >>