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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id BAFC9EB64DD for ; Thu, 22 Jun 2023 23:15:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231544AbjFVXPG (ORCPT ); Thu, 22 Jun 2023 19:15:06 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42892 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231561AbjFVXPF (ORCPT ); Thu, 22 Jun 2023 19:15:05 -0400 Received: from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com [IPv6:2a00:1450:4864:20::22e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7D5A51738 for ; Thu, 22 Jun 2023 16:14:59 -0700 (PDT) Received: by mail-lj1-x22e.google.com with SMTP id 38308e7fff4ca-2b47742de92so1594961fa.0 for ; Thu, 22 Jun 2023 16:14:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1687475697; x=1690067697; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=g1JY0yeL3CeoNuEhq9uRKX956qL6nT7MiCllyM/47ik=; b=pDgvUF90fHQm5F2DkDPo2iXIdKKXGkIoGOpgdvMdlg9jB+cag1pl8EJ76CM6XD2nHO T/Q7vGDxcWWFG/NCZhQKgzHFCiHfb4v/LmF+PkScYz8AYs3kq7IS8rVWVLAquwAMJ+Ed GBsfXR5QVrglU+R1oHBgKOEtpPPkgIeQXHbkZEwjYrnIVkuHoXcKuVnGPXZw/gU3ZdFl JLpbeMwU7NWfkW4l9L2Y0JNb0ZdRERyG9J6nXkUQNT/NQvOm4wpWkGp+1A2rwSgg0MZM lw7Q+oCIgiRJCW5wZ7mPkDsuQW0Nh7DghXimKMB9fFshVDQLcr9chYGJ8GdrL4+bUzOR YZkQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1687475697; x=1690067697; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=g1JY0yeL3CeoNuEhq9uRKX956qL6nT7MiCllyM/47ik=; b=ZJppSOWFqJI8AkETIFOdJP1NrNSYXdIpuu2O+hRI2kQ03sOArDiYAoNYpy8xRYzdY+ +6TSYASncRbK2FcskUR0Oqj1VGWjQUfWEqONK5ScAAqmcHBL5CDQpte8LYeXv8K7bRA9 h2Up2Hy3IxhsjvdH+kygqmlKaoQD46BVmKUpe9Fb40QOnRSF/VPKCKW+D3zfmdWmh0sZ RyL5kAOscH/Ehzw+/CRSMUJpuyyTCss25fGk09yKfFRaAmIvVl88RL07XIXtwngpcfsy XhbR+J8M0tZJiWlG2W0fs8XihrKn9ZKQvDMObQ7z0S5LkAHbQLuUj2LWRsHB1S2q/pRB y8+w== X-Gm-Message-State: AC+VfDz0OcS7tb/urHSl9zdPOZtj7yoqfYrBkNy5GhWBsAJOmOJToQf+ 2rzzMapQHEKrASx41xJ5/HQMgx19eel7scyUqA8= X-Google-Smtp-Source: ACHHUZ51Vi7gdrXGLA0oOJ1dXh3HiLjBGA0Ks/lCXN75Wlh+vv5jSmCwxP1WSZc2IkK17o30KM3N7g== X-Received: by 2002:a05:6512:55a:b0:4f7:669f:7da8 with SMTP id h26-20020a056512055a00b004f7669f7da8mr12998738lfl.7.1687475697468; Thu, 22 Jun 2023 16:14:57 -0700 (PDT) Received: from ?IPV6:2001:14ba:a0db:1f00::8a5? (dzdqv0yyyyyyyyyyybcwt-3.rev.dnainternet.fi. [2001:14ba:a0db:1f00::8a5]) by smtp.gmail.com with ESMTPSA id g15-20020a19ee0f000000b004f86e5918a6sm1237614lfb.189.2023.06.22.16.14.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 22 Jun 2023 16:14:56 -0700 (PDT) Message-ID: Date: Fri, 23 Jun 2023 02:14:56 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.11.0 Subject: Re: [PATCH 2/3] drm/msm/dpu: Set DATABUS_WIDEN on command mode encoders Content-Language: en-GB To: Abhinav Kumar , Marijn Suijten , Jessica Zhang Cc: Rob Clark , Sean Paul , David Airlie , Daniel Vetter , linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org References: <20230525-add-widebus-support-v1-0-c7069f2efca1@quicinc.com> <20230525-add-widebus-support-v1-2-c7069f2efca1@quicinc.com> <81a5e241-ec82-7414-8752-4ce3cb084959@linaro.org> <26tvhvqpxtxz5tqc6jbjixadpae34k7uc7fyec2u5o2ccj4tdq@tjvguzlolc3g> <8dcd643f-9644-a4e7-a0d5-eefa28084a88@linaro.org> <7d5256cd-c0bd-36e3-9b59-63ad8595f0ce@quicinc.com> From: Dmitry Baryshkov In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 23/06/2023 01:37, Abhinav Kumar wrote: > > > On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote: >> On 22/06/2023 02:01, Abhinav Kumar wrote: >>> >>> >>> On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote: >>>> On 21/06/2023 18:17, Marijn Suijten wrote: >>>>> On 2023-06-20 14:38:34, Jessica Zhang wrote: >>>>> >>>>>>>>>>> +    if (phys_enc->hw_intf->ops.enable_widebus) >>>>>>>>>>> + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf); >>>>>>>>>> >>>>>>>>>> No. Please provide a single function which takes necessary >>>>>>>>>> configuration, including compression and wide_bus_enable. >>>>>>>>>> >>>>>>>>> >>>>>>>>> There are two ways to look at this. Your point is coming from the >>>>>>>>> perspective that its programming the same register but just a >>>>>>>>> different >>>>>>>>> bit. But that will also make it a bit confusing. >>>>>>> >>>>>>> My point is to have a high-level function that configures the >>>>>>> INTF for >>>>>>> the CMD mode. This way it can take a structure with necessary >>>>>>> configuration bits. >>>>>> >>>>>> Hi Dmitry, >>>>>> >>>>>> After discussing this approach with Abhinav, we still have a few >>>>>> questions about it: >>>>>> >>>>>> Currently, only 3 of the 32 bits for INTF_CONFIG2 are being used (the >>>>>> rest are reserved with no plans of being programmed in the >>>>>> future). Does >>>>>> this still justify the use of a struct to pass in the necessary >>>>>> configuration? >>>>> >>>>> No.  The point Dmitry is making is **not** about this concidentally >>>>> using the same register, but about adding a common codepath to enable >>>>> compression on this hw_intf (regardless of the registers it needs to >>>>> touch). >>>> >>>> Actually to setup INTF for CMD stream (which is equal to setting up >>>> compression at this point). >>>> >>> >>> Yes it should be setup intf for cmd and not enable compression. >>> >>> Widebus and compression are different features and we should be able >>> to control them independently. >>> >>> We just enable them together for DSI. So a separation is necessary. >>> >>> But I am still not totally convinced we even need to go down the path >>> for having an op called setup_intf_cmd() which takes in a struct like >>> >>> struct dpu_cmd_intf_cfg { >>>      bool data_compress; >>>      bool widebus_en; >>> }; >>> >>> As we have agreed that we will not touch the video mode timing engine >>> path, it leaves us with only two bits. >>> >>> And like I said, its not that these two bits always go together. We >>> want to be able to control them independently which means that its >>> not necessary both bits program the same register one by one. We >>> might just end up programming one of them if we just use widebus. >>> >>> Thats why I am still leaning on keeping this approach. >> >> I do not like the idea of having small functions being called between >> modules. So, yes there will a config of two booleans, but it is >> preferable (and more scalable) compared to separate callbacks. >> > > I definitely agree with the scalable part and I even cross checked that > the number of usable bitfields of this register is going up from one > chipset to the other although once again that depends on whether we will > use those features. > > For that reason I am not opposed to the struct idea. > > But there is also another pattern i am seeing which worries me. Usable > bitfields sometimes even reduce. For those cases, if we go with a > pre-defined struct it ends up with redundant members as those bitfields > go away. > > With the function op based approach, we just call the op if the feature > bit / core revision. > > So I wanted to check once more about the fact that we should consider > not just expansion but also reduction. As we have to support all generations, there is no actual reduction. Newer SoCs do not have particular feature/bit, but older ones do. By having multiple optional ops we just move this knowledge from ops->complex_callback() to _setup_block_ops(). But more importantly the caller gets more complicated. Instead of just calling ops->set_me_up(), it has to check all the optional callbacks and call the one by one. > >> Not to mention that it allows us to program required registers >> directly (by setting values) rather than using RMW cycles and thus >> depending on the value being previously programmed to these registers. >> > > This will not change. We will still have to use RMW cycles to preserve > the reset values of some of the fields as those are the right values for > the registers and shouldnt be touched. I'd like to point to the dpu_hw_intf_setup_timing_engine(), a close rival callback, setting up the INTF for video mode. It does not do RMW cycles, it just writes all the registers. In the worst case, there will be a single RMW instead of having multiple of them. > >>> >>>>>  Similar to how dpu_hw_intf_setup_timing_engine() programs the >>>>> hw_intf - including widebus! - for video-mode. >>>>> >>>>> Or even more generically, have a struct similar to intf_timing_params >>>>> that says how the intf needs to be configured - without the caller >>>>> knowing about INTF_CONFIG2. >>>>> >>>>> struct dpu_hw_intf_cfg is a very good example of how we can use a >>>>> single >>>>> struct and a single callback to configure multiple registers at once >>>>> based on some input parameters. >>>>> >>>>>> In addition, it seems that video mode does all its INTF_CONFIG2 >>>>>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we >>>>>> have a generic set_intf_config2() op, it might be good to have it as >>>>>> part of a larger cleanup where we have both video and command mode >>>>>> use >>>>>> the generic op. What are your thoughts on this? >>>>> >>>>> Not in that way, but if there is a generic enable_compression() or >>>>> configure_compression() callback (or even more generic, similar to >>>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and >>>>> command-mode, maybe that is beneficial. >>>> >>>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup >>>> CMD'. For example, it might also include setting up other INTF >>>> parameters for CMD mode (if anything is required later on). >>>> >>> >>> Agreed on setup CMD but I dont know whether we need a setup CMD at all. >>> Seems like an overkill. >>> >>>>> >>>>> - Marijn >>>> >> -- With best wishes Dmitry