From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 81FB6C433EF for ; Wed, 23 Mar 2022 20:07:45 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Date:Cc:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=6rNaOjLUu65PW365udAQPvMudPI6bd34hA7st6SW3lE=; b=RhVCxdDDaihmr8 f5fJW3XPNb7dqgSq2cvAvGkLm3b5bfB6Bdrq1/2M0DFs+Yx7YFsPPaLrWTNlLTg4cN3ngpVh6Cn9U ZT7YPb52AHFXRVWv1ra0r6PnqX4ZnOKiAfB2ESZ8Rgb5H7uPMKEI1NEepUKvPD1J+nS2BRFN95vdN s3AZq6AUKtQUyZ/taqhZxkl/CCMvvVfJQkF9bXkWqcgvxu3VJgOmvRiChBJJTJJKbIP5jcMDzUrPu 6bJv2uno17HfW2IxCnsRTzEJ82KsTOnh5AWUbvInINGncoINg0ntjV9iPsFH+VwM1AbL/+PxAQ7cV LjG87pnYPegwLbO/7FyA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nX7FW-00EmdG-FX; Wed, 23 Mar 2022 20:06:30 +0000 Received: from mail-ed1-x52f.google.com ([2a00:1450:4864:20::52f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nX7FR-00EmcN-35 for linux-arm-kernel@lists.infradead.org; Wed, 23 Mar 2022 20:06:27 +0000 Received: by mail-ed1-x52f.google.com with SMTP id y10so3185495edv.7 for ; Wed, 23 Mar 2022 13:06:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:subject:from:to:cc:date:in-reply-to:references :user-agent:mime-version:content-transfer-encoding; bh=0meBw1PFo/61ZIFhBS5OBWfvMeyRYDdihNqds4tJj2w=; b=pUfXYawsuww0SW3Jz284kck7niEROJjRyztknyIwcaOwxwhM4ORa7YBpE0t1f2Iyok ttVazoxjnaRqgJpa/8peW1uXYjg4mPHjasc5JvvbLalY4HrqoJSqYDXDHnkq1hyx8ffq 12ZTEO6+iB89XfrB3dqWpg7eRTtklvaQb/5A1MDDVEeRgSoUAI5Pxu/KruIcGO+Ikkne RAj3t0hphxFyYXxX8DYP5hqsHmF3MZKHAn3XRdhgRMObLoUqkRYzKq04gLAjBgSjOnBs M5fM0Gg+pBR35312Snm3atUVELX6NAqMJXh6WNe0XWzICgjlDtOqm3PCnRgHLQGAorAe fwOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:user-agent:mime-version:content-transfer-encoding; bh=0meBw1PFo/61ZIFhBS5OBWfvMeyRYDdihNqds4tJj2w=; b=qrCTkzbZ9etiAnsM+qFV7AFXbphPe5bzH/5Pe91toJkaxFYfTuNcnadjYx3O2UgqT0 aCFkUu8vTQ53gyjMJ4Uz0RK+SOwsJKrx3VYpD3sNXXG2SYhLlwq3//Q0gdpv4dDs3FjN nhgYNzhAoD6lMH5yLYyt83vx58fYykQzwhmoYzfLnfLrQ1LbydTnpvm3xIIq7yJHrz6W SAoZShMLSLOti4l8aWoLiSoctuKokLSTCYZ//xGACMOAOBdrt/S8jtzf9ImIORqsnnhr szjjZWyN0B2XHMZU6u19VElrYKdNDOIbsFvEQcOQ01lzou5L8p7VmW8w3Fl9ZX6kN5pH 8RQA== X-Gm-Message-State: AOAM531HETLf+wbG8VFmbazEp4QaPU43eNFnzv1yVWFa7pSlbMWdZWb0 mur+wSgiRcvFHmFU3EoSUq8= X-Google-Smtp-Source: ABdhPJx6yisZ2ZRH0No/tK+z6ABSrVeT+l5zyANSiInUuoI6R7qaz/WBqVNpRCc5lrA5STgC/zvFXw== X-Received: by 2002:a05:6402:370b:b0:413:3bcd:3d0e with SMTP id ek11-20020a056402370b00b004133bcd3d0emr2358800edb.178.1648065981596; Wed, 23 Mar 2022 13:06:21 -0700 (PDT) Received: from Saturn.fritz.box ([194.191.225.82]) by smtp.googlemail.com with ESMTPSA id w22-20020a056402269600b004194f4eb3e7sm444683edd.19.2022.03.23.13.06.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Mar 2022 13:06:21 -0700 (PDT) Message-ID: Subject: Re: [RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format From: Max Krummenacher To: Maxime Ripard Cc: Dave Stevenson , Marek Vasut , Christoph Niedermaier , Max Krummenacher , Pengutronix Kernel Team , David Airlie , Sam Ravnborg , Sascha Hauer , DRI Development , DenysDrozdov , Laurent Pinchart , Shawn Guo , Linux ARM , NXP Linux Team Date: Wed, 23 Mar 2022 21:06:18 +0100 In-Reply-To: <20220323155817.xcsqxothziot7ba3@houat> References: <20220302142142.zroy464l5etide2g@houat> <9c9a10ca-e6a1-c310-c0a5-37d4fed6efd6@denx.de> <20220318163549.5a5v3lex4btnnvgb@houat> <20220318171642.y72eqf5qbmuu2ln2@houat> <5ae44b7cd1f7577c98f316a7d288aa4cf423da2d.camel@active.ch> <20220323155817.xcsqxothziot7ba3@houat> User-Agent: Evolution 3.34.4 MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220323_130625_915052_CDD873E2 X-CRM114-Status: GOOD ( 66.76 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Am Mittwoch, den 23.03.2022, 16:58 +0100 schrieb Maxime Ripard: > On Wed, Mar 23, 2022 at 09:42:11AM +0100, Max Krummenacher wrote: > > Am Freitag, den 18.03.2022, 17:53 +0000 schrieb Dave Stevenson: > > > On Fri, 18 Mar 2022 at 17:16, Maxime Ripard wrote: > > > > On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote: > > > > > Hi Maxime > > > > > > > > > > On Fri, 18 Mar 2022 at 16:35, Maxime Ripard wrote: > > > > > > On Mon, Mar 07, 2022 at 04:26:56PM +0100, Max Krummenacher wrote: > > > > > > > On Wed, Mar 2, 2022 at 5:22 PM Marek Vasut wrote: > > > > > > > > On 3/2/22 15:21, Maxime Ripard wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > Please try to avoid top posting > > > > > > > Sorry. > > > > > > > > > > > > > > > > On Wed, Feb 23, 2022 at 04:25:19PM +0100, Max Krummenacher wrote: > > > > > > > > > > The goal here is to set the element bus_format in the struct > > > > > > > > > > panel_desc. This is an enum with the possible values defined in > > > > > > > > > > include/uapi/linux/media-bus-format.h. > > > > > > > > > > > > > > > > > > > > The enum values are not constructed in a way that you could calculate > > > > > > > > > > the value from color channel width/shift/mapping/whatever. You rather > > > > > > > > > > would have to check if the combination of color channel > > > > > > > > > > width/shift/mapping/whatever maps to an existing value and otherwise > > > > > > > > > > EINVAL out. > > > > > > > > > > > > > > > > > > > > I don't see the value in having yet another way of how this > > > > > > > > > > information can be specified and then having to write a more > > > > > > > > > > complicated parser which maps the dt data to bus_format. > > > > > > > > > > > > > > > > > > Generally speaking, sending an RFC without explicitly stating what you > > > > > > > > > want a comment on isn't very efficient. > > > > > > > > > > > > > > > > Isn't that what RFC stands for -- Request For Comment ? > > > > > > > > > > > > > > I hoped that the link to the original discussion was enough. > > > > > > > > > > > > > > panel-simple used to have a finite number of hardcoded panels selected > > > > > > > by their compatible. > > > > > > > The following patchsets added a compatible 'panel-dpi' which should > > > > > > > allow to specify the panel in the device tree with timing etc. > > > > > > > > > > > > > > https://patchwork.kernel.org/project/dri-devel/patch/20200216181513.28109-6-sam@ravnborg.org/ > > > > > > > In the same release cycle part of it got reverted: > > > > > > > > > > > > > > https://patchwork.kernel.org/project/dri-devel/patch/20200314153047.2486-3-sam@ravnborg.org/ > > > > > > > With this it is no longer possible to set bus_format. > > > > > > > > > > > > > > The explanation what makes the use of a property "data-mapping" not a > > > > > > > suitable way in that revert > > > > > > > is a bit vague. > > > > > > > > > > > > Indeed, but I can only guess. BGR666 in itself doesn't mean much for > > > > > > example. Chances are the DPI interface will use a 24 bit bus, so where > > > > > > is the padding? > > > > > > > > > > > > I think that's what Sam and Laurent were talking about: there wasn't > > > > > > enough information encoded in that property to properly describe the > > > > > > format, hence the revert. > > > > I agree that the strings used to set "data-mapping" weren't self explaining. > > However, as there was a > > clear 1:1 relation to the bus_format value the meaning > > wasn't ambiguous at all. > > > > > > > MEDIA_BUS_FMT_RGB666_1X18 defines an 18bit bus, therefore there is no > > > > > padding. "bgr666" was selecting that media bus code (I won't ask about > > > > > the rgb/bgr swap). > > > > > > > > > > If there is padding on a 24 bit bus, then you'd use (for example) > > > > > MEDIA_BUS_FMT_RGB666_1X24_CPADHI to denote that the top 2 bits of each > > > > > colour are the padding. Define and use a PADLO variant if the padding > > > > > is the low bits. > > > > > > > > Yeah, that's kind of my point actually :) > > > > > > Ah, OK :) > > > > > > > Just having a rgb666 string won't allow to differentiate between > > > > MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are > > > > RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and > > > > then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new > > > > string but that usually leads to inconsistent or weird names, so this > > > > isn't ideal. > > > > We're on the same page that the strings that were used aren't self > > explaining and do not follow a pattern which would make it easy to > > extend. However that is something I addressed in my RFC proposal, not? > > > > > > > The string matching would need to be extended to have some string to > > > > > select those codes ("lvds666" is a weird choice from the original > > > > > patch). > > > > > > > > > > Taking those media bus codes and handling them appropriately is > > > > > already done in vc4_dpi [1], and the vendor tree has gained > > > > > BGR666_1X18 and BGR666_1X24_CPADHI [2] as they aren't defined in > > > > > mainline. > > > > > > > > > > Now this does potentially balloon out the number of MEDIA_BUS_FMT_xxx > > > > > defines needed, but that's the downside of having defines for all > > > > > formats. > > > > > > > > > > (I will admit to having a similar change in the Pi vendor tree that > > > > > allows the media bus code to be selected explicitly by hex value). > > > > > > > > I think having an integer value is indeed better: it doesn't change much > > > > in the device tree if we're using a header, it makes the driver simpler > > > > since we don't have to parse a string, and we can easily extend it or > > > > rename the define, it won't change the ABI. > > > > Fine with me. > > > > > > I'm not sure using the raw media bus format value is ideal though, since > > > > that value could then be used by any OS, and it would effectively force > > > > the mbus stuff down their throat. > > > > I disagree here, this forces us to use code to map the device tree enum > > to the kernel enum for Linux, i.e. adds complexity and maintenance work > > if additional bus_formats are needed. > > Assuming there is another OS which uses the device tree it would not > > make a difference, that OS would still need to map the device tree enum > > to the corresponding representation in their kernel. > > So, you don't want to do something in Linux, but would expect someone > else to be completely ok with that? Yes, sort of. Recycling the values as used currently in the Linux kernel rather than inventing a new numbering will make the Linux code a little easier to write, read and maintain without any negative effect on how that other OSs would have to map the DT representation to their internal representation. Would you rather have something like: DT_BUS_FMT_RGB666_1X18 1 DT_BUS_FMT_RGB888_1X24 2 ... switch (bus-format) { case DT_BUS_FMT_RGB666_1X18: bus_format = MEDIA_BUS_FMT_RGB666_1X18; break; case DT_BUS_FMT_RGB888_1X24: bus_format = MEDIA_BUS_FMT_RGB888_1X24; break; ... > > > I would copy the definitions of media-bus-format.h into a header in > > include/dt-bindings similarly as it is done for > > include/dt-bindings/display/sdtv-standards.h for TV standards. > > That might not be an option: that header is licensed under the GPL, > device trees are usually licensed under GPL+MIT, and we don't have any > requirements on the license for other projects using a DT (hence the > dual license). That one I didn't consider. That would be solved by a newly invented enum. Max > > Maxime _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel