Received: by 2002:a05:7412:b101:b0:e2:908c:2ebd with SMTP id az1csp2646011rdb; Wed, 15 Nov 2023 06:53:01 -0800 (PST) X-Google-Smtp-Source: AGHT+IEbAmalJq/GC8lO4dHeEsKzteTk+RyMoWg2yt2xuC/Mcf5Gw0+VBbXEBp0csPq7XbqOP9Gj X-Received: by 2002:a05:6a20:8e1c:b0:187:932f:e249 with SMTP id y28-20020a056a208e1c00b00187932fe249mr557144pzj.4.1700059981500; Wed, 15 Nov 2023 06:53:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1700059981; cv=none; d=google.com; s=arc-20160816; b=I9dUqKaNuZcUApQgsmlJrh8RFof4mMs+Q2SnfD/xRi/2g7PWj0LuqWZ3K/D2H787wK imqqcy8HRht/tDIRs+OBkpdowxzBIp6p7O+XFhsljV01fi/QBwWn0WhNDox6NAx4zlYk 2Kr5Aw3ADg+QoKduNXgU9Z+zxWUhMoe97BzQx9szzeBZ94mv57BbKLS7nwMdGXD+cleZ XBsUMtcXgid7di8Hd0QatfhWxOLa/gs8APTUtwqZlvuNx41UbWvRo73A1DRQRiylmV0Z 5n9627UTcgnu47bdGahc28koN6rqQ7KcizT1rqX5CkY4VPfAZseUokFyRmKCLCRvpCo7 N47A== 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:from :references:cc:to:content-language:subject:user-agent:mime-version :date:message-id; bh=8UzqZj9Wht5hbs2ixjqIzUQfl9d7KOPhh/qli33F6ec=; fh=0Ln8YQ7h4GEbFr51t7HmIM2lVvF1duCuT4Aq6Fbu+Lc=; b=rFW0/EXd5D95qdLaMC6zvkgsC0tWP4VjgMB8cjzkGFywmGSDe31+gAvQ0cmjIoWTzc zDX158advpbva0KD0cbkjJV/P2AmBRXGOMRYmw0dxaUGW5aH3O56uJ+bUFlWwCc1+C0f I4Pvtfikz9m0ag75KwS6iY8kNpmHBg9peNwHqWrBOye9zx4ohsGHTkbKQpeeVaOuONm3 3IrLoSmlTgu7t1rpUeJbixstp4kas1MgbgJV6ex5RIoZ31dnX3L970amntPrnrv341Rw cs1qDloBsI3r3NGQ8HhQiFcwxNFM8ZL81rSds5k3JgDEtQP05REo73Q9TLCMqvHlMk8b wdoQ== 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:3 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from lipwig.vger.email (lipwig.vger.email. [2620:137:e000::3:3]) by mx.google.com with ESMTPS id z128-20020a633386000000b0057745d87b53si10461854pgz.686.2023.11.15.06.53.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Nov 2023 06:53:01 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 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 lipwig.vger.email (Postfix) with ESMTP id 8E37280209C3; Wed, 15 Nov 2023 06:52:58 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1344238AbjKOOwr (ORCPT + 99 others); Wed, 15 Nov 2023 09:52:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35324 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1344173AbjKOOwq (ORCPT ); Wed, 15 Nov 2023 09:52:46 -0500 Received: from ex01.ufhost.com (ex01.ufhost.com [61.152.239.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2289E8E; Wed, 15 Nov 2023 06:52:35 -0800 (PST) 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 3CE7C24E20F; Wed, 15 Nov 2023 22:52:28 +0800 (CST) Received: from EXMBX161.cuchost.com (172.16.6.71) by EXMBX166.cuchost.com (172.16.6.76) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Wed, 15 Nov 2023 22:52:28 +0800 Received: from [192.168.1.115] (180.164.60.184) by EXMBX161.cuchost.com (172.16.6.71) with Microsoft SMTP Server (TLS) id 15.0.1497.42; Wed, 15 Nov 2023 22:52:27 +0800 Message-ID: <21e14a40-e707-4925-b43b-6656ae59d680@starfivetech.com> Date: Wed, 15 Nov 2023 22:52:27 +0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 5/6] drm/vs: Add KMS crtc&plane Content-Language: en-US To: Maxime Ripard CC: "dri-devel@lists.freedesktop.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-riscv@lists.infradead.org" , "linux-media@vger.kernel.org" , "linaro-mm-sig@lists.linaro.org" , "David Airlie" , Daniel Vetter , Rob Herring , Krzysztof Kozlowski , Conor Dooley , "Emil Renner Berthing" , Paul Walmsley , Palmer Dabbelt , Albert Ou , Maarten Lankhorst , Thomas Zimmermann , Philipp Zabel , Sumit Semwal , "christian.koenig@amd.com" , Bjorn Andersson , "Heiko Stuebner" , Shawn Guo , Jagan Teki , Chris Morgan , Jack Zhu , Shengyang Chen , Changhuang Liang References: <20231025103957.3776-1-keith.zhao@starfivetech.com> <20231025103957.3776-6-keith.zhao@starfivetech.com> From: Keith Zhao In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [180.164.60.184] X-ClientProxiedBy: EXCAS062.cuchost.com (172.16.6.22) To EXMBX161.cuchost.com (172.16.6.71) X-YovoleRuleAgent: yovoleflag X-Spam-Status: No, score=-0.8 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email 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 (lipwig.vger.email [0.0.0.0]); Wed, 15 Nov 2023 06:52:58 -0800 (PST) On 2023/10/25 21:57, Maxime Ripard wrote: > On Wed, Oct 25, 2023 at 06:39:56PM +0800, Keith Zhao wrote: >> +static struct drm_crtc_state * >> +vs_crtc_atomic_duplicate_state(struct drm_crtc *crtc) >> +{ >> + struct vs_crtc_state *ori_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; > > That field is never set in your patch. > >> + state->encoder_type = ori_state->encoder_type; > > That isn't either, and it's not clear why you would need the > encoder_type stored in the CRTC? > >> + state->bpp = ori_state->bpp; > > You seem to derive that from output_fmt, it doesn't need to be in the > CRTC state. > >> + state->underflow = ori_state->underflow; > > Assuming you're setting this from the interrupt handler, it's unsafe, > you shouldn't do that. What are you using it for? I am going to use the crtc_debugfs function for printing. crtc_debugfs will use it But now I'd better delete it > >> +static const struct drm_prop_enum_list vs_sync_mode_enum_list[] = { >> + { VS_SINGLE_DC, "single dc mode" }, >> + { VS_MULTI_DC_PRIMARY, "primary dc for multi dc mode" }, >> + { VS_MULTI_DC_SECONDARY, "secondary dc for multi dc mode" }, >> +}; > > Custom driver properties are a no-go: > https://docs.kernel.org/gpu/drm-kms.html#requirements > > And > > https://docs.kernel.org/gpu/drm-uapi.html#open-source-userspace-requirements > >> +void vs_dc_enable(struct vs_dc *dc, struct drm_crtc *crtc) >> +{ >> + struct vs_crtc_state *crtc_state = to_vs_crtc_state(crtc->state); >> + struct drm_display_mode *mode = &crtc->state->adjusted_mode; >> + struct dc_hw_display display; > > Why are you rolling your own structure here, if it's exactly equivalent > to what drm_display_mode and the crtc_state provide? My original intention was to make the hardware part purer. and want to decouple hardware from drm struct. so I define the own structure between drm and hardware. Maybe doing this will make both the hardware and drm happy > >> +void vs_dc_commit(struct vs_dc *dc) >> +{ >> + dc_hw_enable_shadow_register(&dc->hw, false); >> + >> + dc_hw_commit(&dc->hw); >> + >> + if (dc->first_frame) >> + dc->first_frame = false; >> + >> + dc_hw_enable_shadow_register(&dc->hw, true); >> +} > > It's not clear to me what you're trying to do here, does the hardware > have latched registers that are only updated during vblank? > >> +static int dc_bind(struct device *dev, struct device *master, void *data) >> +{ >> + struct drm_device *drm_dev = data; >> + struct vs_dc *dc = dev_get_drvdata(dev); >> + struct device_node *port; >> + struct vs_crtc *crtc; >> + struct vs_dc_info *dc_info; >> + struct vs_plane *plane; >> + struct vs_plane_info *plane_info; >> + int i, ret; >> + u32 ctrc_mask = 0; >> + >> + if (!drm_dev || !dc) { >> + dev_err(dev, "devices are not created.\n"); >> + return -ENODEV; >> + } >> + >> + ret = dc_init(dev); >> + if (ret < 0) { >> + drm_err(drm_dev, "Failed to initialize DC hardware.\n"); >> + return ret; >> + } >> + >> + port = of_get_child_by_name(dev->of_node, "port"); >> + if (!port) { >> + drm_err(drm_dev, "no port node found\n"); >> + return -ENODEV; >> + } >> + of_node_put(port); >> + >> + dc_info = dc->hw.info; >> + >> + for (i = 0; i < dc_info->panel_num; i++) { >> + crtc = vs_crtc_create(drm_dev, dc_info); >> + if (!crtc) { >> + drm_err(drm_dev, "Failed to create CRTC.\n"); >> + ret = -ENOMEM; >> + return ret; >> + } >> + >> + crtc->base.port = port; >> + crtc->dev = dev; >> + dc->crtc[i] = crtc; >> + ctrc_mask |= drm_crtc_mask(&crtc->base); >> + } >> + >> + for (i = 0; i < dc_info->plane_num; i++) { >> + plane_info = (struct vs_plane_info *)&dc_info->planes[i]; >> + >> + if (!strcmp(plane_info->name, "Primary") || !strcmp(plane_info->name, "Cursor")) { >> + plane = vs_plane_create(drm_dev, plane_info, dc_info->layer_num, >> + drm_crtc_mask(&dc->crtc[0]->base)); >> + } else if (!strcmp(plane_info->name, "Primary_1") || >> + !strcmp(plane_info->name, "Cursor_1")) { > > Please use an enum and an id there. > >> +static int vs_plane_atomic_set_property(struct drm_plane *plane, >> + struct drm_plane_state *state, >> + struct drm_property *property, >> + uint64_t val) >> +{ >> + struct drm_device *dev = plane->dev; >> + struct vs_plane *vs_plane = to_vs_plane(plane); >> + struct vs_plane_state *vs_plane_state = to_vs_plane_state(state); >> + int ret = 0; >> + >> + if (property == vs_plane->degamma_mode) { >> + if (vs_plane_state->degamma != val) { >> + vs_plane_state->degamma = val; >> + vs_plane_state->degamma_changed = true; >> + } else { >> + vs_plane_state->degamma_changed = false; >> + } >> + } else if (property == vs_plane->watermark_prop) { >> + ret = _vs_plane_set_property_blob_from_id(dev, >> + &vs_plane_state->watermark, >> + val, >> + sizeof(struct drm_vs_watermark)); >> + return ret; >> + } else if (property == vs_plane->color_mgmt_prop) { >> + ret = _vs_plane_set_property_blob_from_id(dev, >> + &vs_plane_state->color_mgmt, >> + val, >> + sizeof(struct drm_vs_color_mgmt)); >> + return ret; >> + } else if (property == vs_plane->roi_prop) { >> + ret = _vs_plane_set_property_blob_from_id(dev, >> + &vs_plane_state->roi, >> + val, >> + sizeof(struct drm_vs_roi)); >> + return ret; >> + } else { >> + return -EINVAL; >> + } >> + >> + return 0; >> +} > > Same story than above for properties > > > Honestly, that driver is pretty massive, and you should be simplifying > it a lot of you want the initial contribution to be as smooth as > possible. > > Things like all the tiling formats, the underflowing handling, all those > properties, etc can (and should) be added in a second step once the > foundations are in. > > Maxime ok , Thanks for reminding me. I will clarify my next goal and be more likely to simplify features.