Received: by 2002:a05:7412:251c:b0:e2:908c:2ebd with SMTP id w28csp2805047rda; Wed, 25 Oct 2023 12:50:17 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFROFUaBi0lSrJ2JGX2eAbwqkL69n8MNaXcUZfaeMoKmbBsJcr3Sz28YsKYB97aQe1PhBxr X-Received: by 2002:a05:6870:2e07:b0:1e9:9833:daad with SMTP id oi7-20020a0568702e0700b001e99833daadmr21031900oab.4.1698263416891; Wed, 25 Oct 2023 12:50:16 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698263416; cv=none; d=google.com; s=arc-20160816; b=0jj0mW05vUjUOXlZRCu7gC+o8zQM/miR7h4lBj36glQCpqFFR/WcVFqST8IlnJJ0TE imr7boepID/3se8COrAvbL79k61S8ynfXBfC+fnuqFjUf0N6dIOTB7JGeC8/kSOvcYW5 hcResop7fUpmd2dU0l1xggkYGYf265OIRj6I3UAiNrT6XRfRyBWB/Cd8Akntq9bYaXDL MphBzntBlF/UpCqrfzIlS1LZg1xZA510Mr6s73Eb0slqR6j1dxggRh7shBZxVb9NZP1u 3tlKdz7P4XaDMkrbSGRLi3miGIC3xr0hvhavdIq1sqcfei3D/pFMxRB53s/1XkvCytcN EqlQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=VbYQgVmHg0mnPCKakv2463QTv9MRiEO5tk2Izif2Bc4=; fh=xByeMVmH8e5Qiytx2vCtNC+DlNyZSVfsG9fEAjeokbM=; b=CXtQLPeqBkSgP8/qGoiMxEfrZCUzTPzrVLZNDKaF8Yq97BE02DbbXsuj0U1cKsOkFY miKh2d5Y6I8U6lkA+6tpJL5CO9Vx5LU1JdQNsJKyK2ypDYq38Dx2PsV1K1sJefokJyXF A1IdRo39NVv33V9L2HVSxfy4Up7KxCtTCZONrfAcqAoGJTnLcei77zfZxtWq93Rrg9lp iyhGS58r+8nLInJE8a+Mwv9AefJOwTMFU3OtvJIin4jhci7n6+wyJQ1AkqhlaQo43p2h wQFUONqBsoHAcS8ZLdoE3z1JObCmSp4OH4AWR0z5vhGeu7040HA54FpN48LoDTF5pIdf 83PQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=F7H5vgnL; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from lipwig.vger.email (lipwig.vger.email. [23.128.96.33]) by mx.google.com with ESMTPS id v17-20020a81a551000000b005a80fab4f07si13094964ywg.35.2023.10.25.12.50.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 25 Oct 2023 12:50:16 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=F7H5vgnL; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by lipwig.vger.email (Postfix) with ESMTP id 9CEBB817890D; Wed, 25 Oct 2023 12:50:12 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at lipwig.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230026AbjJYTt7 (ORCPT + 99 others); Wed, 25 Oct 2023 15:49:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:34984 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229800AbjJYTt6 (ORCPT ); Wed, 25 Oct 2023 15:49:58 -0400 Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.20]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 14B60186; Wed, 25 Oct 2023 12:49:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1698263396; x=1729799396; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=6bjyCn+5RGDR80Oxqpw3WAaYkuJ8eyFL07jKhBVi4rA=; b=F7H5vgnLjZNnfCybWZh1/7tlYQk9jNN26dixy1wTEEmU3ufBTh8EhRrX DfHuTA2a7LauCeVXN7tOLGO3Gv1q3DCHDsJfpEIj+LhnYJ3lWVUB5NU8B m4mxPX3UCYJV+xBwlnmqyCVgVxrVuA+UzvFGO/Mkg5O5RhFA/NqieRykR YnGtCxX+MflOcaXdAncw9FhEQdWEBGF3l3bOF3Cu/WNUQ5ALkQClMBfDx vZPuquFmAA6z236R0mldplDC7vcpEmcaoq3Tcba7VUWiXb3KNwq8LzpcI 4ehPuW+o5EV0dWwcR0wO+EWTgoo5nvZHMsgxO1anoaT4DakgiYoDZGW6n Q==; X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="377752950" X-IronPort-AV: E=Sophos;i="6.03,250,1694761200"; d="scan'208";a="377752950" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Oct 2023 12:49:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6600,9927,10874"; a="735494248" X-IronPort-AV: E=Sophos;i="6.03,250,1694761200"; d="scan'208";a="735494248" Received: from stinkpipe.fi.intel.com (HELO stinkbox) ([10.237.72.74]) by orsmga006.jf.intel.com with SMTP; 25 Oct 2023 12:49:47 -0700 Received: by stinkbox (sSMTP sendmail emulation); Wed, 25 Oct 2023 22:49:46 +0300 Date: Wed, 25 Oct 2023 22:49:46 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Dmitry Baryshkov Cc: Keith Zhao , 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, Conor Dooley , Albert Ou , Emil Renner Berthing , christian.koenig@amd.com, 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 Subject: Re: [PATCH v2 5/6] drm/vs: Add KMS crtc&plane Message-ID: References: <20231025103957.3776-1-keith.zhao@starfivetech.com> <20231025103957.3776-6-keith.zhao@starfivetech.com> <6db09f77-31e8-4f2e-a987-e3745d0e8c24@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <6db09f77-31e8-4f2e-a987-e3745d0e8c24@linaro.org> X-Patchwork-Hint: comment X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS 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, 25 Oct 2023 12:50:12 -0700 (PDT) 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. > > > + > > + 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? > > > + > > + 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() > > > + .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. > > > + .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. 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. > > > + } > > +} > > + > > +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_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); > > + } > > + } > > +} -- Ville Syrj?l? Intel