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 X-Spam-Level: X-Spam-Status: No, score=-15.8 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2B744C433DB for ; Mon, 22 Feb 2021 11:02:01 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id EB77364E3F for ; Mon, 22 Feb 2021 11:02:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230224AbhBVLBy (ORCPT ); Mon, 22 Feb 2021 06:01:54 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:42496 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230315AbhBVLBk (ORCPT ); Mon, 22 Feb 2021 06:01:40 -0500 Received: from mail-vs1-xe31.google.com (mail-vs1-xe31.google.com [IPv6:2607:f8b0:4864:20::e31]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 583CCC06174A for ; Mon, 22 Feb 2021 03:01:00 -0800 (PST) Received: by mail-vs1-xe31.google.com with SMTP id u142so3876668vsu.7 for ; Mon, 22 Feb 2021 03:01:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=bgFHY4hV1gsz8pgEmyxYxqT5LdN1M6Rn37TYsGGZMr0=; b=QGOfC1OxJtyzhn/Dqh9x46vUR0wedl0F6e2YLebzF7a2LSj9jBLLI0PtMzqfOTNvY2 1zISBI7qQJRWsEByoOWAsCA5uT/NmuX1iJutbxhkjBxxCMp5FaU1olopGZcY/zpbiVb7 5aTe64cskNMRi/sSIcu/D8s1yj6PVS08su5Es= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=bgFHY4hV1gsz8pgEmyxYxqT5LdN1M6Rn37TYsGGZMr0=; b=qR8K2CJum7s5AIPVn1otL6SgdGwRDr6MRnoY7xF1cwzI9/RjAtU64yzDah9r8A6XHI LoK8r+PbQeVkL8WS9O4asz5SgrbZpdzPrJuIyIQA8vIbDQ6VDD/8cQuGJLUhwX28qfuI IyiH8mJ/Z4iKlMZ8N6RGbmwVhQ0ojAsCHwWzMrhyMD3JTua24dEH9339ZnaRASiEgFkb Y+GkP0SidHbkYCyVhLjqnkphR3TtvTTbvy00mkUQcmE4GdojsuBX+bveOSWI9GvhLqf1 dhJ0oiZxTPV90pCTOJz70LgdyXQLsUkZfT3jrOeGX6sjewtOXTn5Z8UcVCX5hh0wXvj+ lx+w== X-Gm-Message-State: AOAM531XJGRwgyqrjMptCEruaRUqffuiTOLjlsxDUZXFvKOr8FMDtxmp kfoY8+C8SigylF/FVp2h9gkrdbwyrEiH2KToEY2Wig== X-Google-Smtp-Source: ABdhPJxtvbO7yc7JIxcXC+vdsfZmlOTELtCIG4dyQr0KPt0s8Qz9jqkc/ShnqA459sEHTxSt78vabRBPoAMMyMx2ZKQ= X-Received: by 2002:a67:8945:: with SMTP id l66mr12716289vsd.48.1613991659266; Mon, 22 Feb 2021 03:00:59 -0800 (PST) MIME-Version: 1.0 References: <20210211113309.1.I629b2366a6591410359c7fcf6d385b474b705ca2@changeid> In-Reply-To: From: Nicolas Boichat Date: Mon, 22 Feb 2021 19:00:48 +0800 Message-ID: Subject: Re: [PATCH] drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling features To: Andrzej Hajda Cc: Laurent Pinchart , Neil Armstrong , David Airlie , Viresh Kumar , dri-devel , Thierry Reding , Sam Ravnborg , Emil Velikov , linux-samsung-soc@vger.kernel.org, Joonyoung Shim , Krzysztof Kozlowski , Chun-Kuang Hu , Jonas Karlman , linux-arm-msm@vger.kernel.org, Rikard Falkeborn , "moderated list:ARM/Mediatek SoC support" , Matthias Brugger , Sean Paul , Xin Ji , linux-arm Mailing List , Jernej Skrabec , Rajendra Nayak , Seung-Woo Kim , lkml , Robert Foss , Kyungmin Park , Thomas Zimmermann , freedreno@lists.freedesktop.org Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org On Mon, Feb 22, 2021 at 3:21 PM Andrzej Hajda wrote: > > Hi Nicolas, > > W dniu 22.02.2021 o 06:31, Nicolas Boichat pisze: > > On Mon, Feb 22, 2021 at 3:08 AM Laurent Pinchart > > wrote: > >> Hi Nicolas, > >> > >> Thank you for the patch. > >> > >> On Thu, Feb 11, 2021 at 11:33:55AM +0800, Nicolas Boichat wrote: > >>> Many of the DSI flags have names opposite to their actual effects, > >>> e.g. MIPI_DSI_MODE_EOT_PACKET means that EoT packets will actually > >>> be disabled. Fix this by including _NO_ in the flag names, e.g. > >>> MIPI_DSI_MODE_NO_EOT_PACKET. > >>> > >>> Signed-off-by: Nicolas Boichat > >> This looks good to me, it increases readability. > >> > >> Reviewed-by: Laurent Pinchart > >> > >> Please however see the end of the mail for a comment. > > > Reviewed-by: Andrzej Hajda > > And comment at the end. > > >> > >>> --- > >>> I considered adding _DISABLE_ instead, but that'd make the > >>> flag names a big too long. > >>> > >>> Generated with: > >>> flag=MIPI_DSI_MODE_VIDEO_HFP; git grep $flag | cut -f1 -d':' | \ > >>> xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HFP/" {} > >>> flag=MIPI_DSI_MODE_VIDEO_HBP; git grep $flag | cut -f1 -d':' | \ > >>> xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HBP/" {} > >>> flag=MIPI_DSI_MODE_VIDEO_HSA; git grep $flag | cut -f1 -d':' | \ > >>> xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HSA/" {} > >>> flag=MIPI_DSI_MODE_EOT_PACKET; git grep $flag | cut -f1 -d':' | \ > >>> xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_NO_EOT_PACKET/" {} > >>> (then minor format changes) > >> Ever tried coccinelle ? :-) > > Fun project for next time ,-) > > > >>> drivers/gpu/drm/bridge/adv7511/adv7533.c | 2 +- > >>> drivers/gpu/drm/bridge/analogix/anx7625.c | 2 +- > >>> drivers/gpu/drm/bridge/cdns-dsi.c | 4 ++-- > >>> drivers/gpu/drm/bridge/tc358768.c | 2 +- > >>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8 ++++---- > >>> drivers/gpu/drm/mcde/mcde_dsi.c | 2 +- > >>> drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +- > >>> drivers/gpu/drm/msm/dsi/dsi_host.c | 8 ++++---- > >>> drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c | 2 +- > >>> drivers/gpu/drm/panel/panel-dsi-cm.c | 2 +- > >>> drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +- > >>> drivers/gpu/drm/panel/panel-khadas-ts050.c | 2 +- > >>> drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c | 2 +- > >>> drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c | 2 +- > >>> drivers/gpu/drm/panel/panel-novatek-nt35510.c | 2 +- > >>> drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c | 2 +- > >>> drivers/gpu/drm/panel/panel-samsung-s6d16d0.c | 2 +- > >>> drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +- > >>> drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c | 2 +- > >>> drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c | 4 ++-- > >>> drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 2 +- > >>> drivers/gpu/drm/panel/panel-simple.c | 2 +- > >>> drivers/gpu/drm/panel/panel-sony-acx424akp.c | 2 +- > >>> drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c | 2 +- > >>> include/drm/drm_mipi_dsi.h | 8 ++++---- > >>> 25 files changed, 36 insertions(+), 36 deletions(-) > >>> > >>> [] > >>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h > >>> index 360e6377e84b..ba91cf22af51 100644 > >>> --- a/include/drm/drm_mipi_dsi.h > >>> +++ b/include/drm/drm_mipi_dsi.h > >>> @@ -119,15 +119,15 @@ struct mipi_dsi_host *of_find_mipi_dsi_host_by_node(struct device_node *node); > >>> /* enable hsync-end packets in vsync-pulse and v-porch area */ > >>> #define MIPI_DSI_MODE_VIDEO_HSE BIT(4) > >> We're mixing bits that enable a feature and bits that disable a feature. > >> Are these bits defined in the DSI spec, or internal to DRM ? In the > >> latter case, would it make sense to standardize on one "polarity" ? That > >> would be a more intrusive change in drivers though. > > Yes, that'd require auditing every single code path and reverse the > > logic as needed. I'm not volunteering for that ,-P (hopefully the > > current change is still an improvement). > > > > Hopefully real DSI experts can comment (Andrzej?), I think the default > > are sensible settings? > > Hehe, "real DSI expert" :), ok I've read spec few times :) > > If I remember correctly the spec did not prioritizes these modes, specs > are publicly available if somebody want to check it feel free. > > These values were taken from defaults for Exynos DSI, as nobody at the > time has better idea. > > We could try to optimize it by looking for example in different dsi > hosts defaults, or maybe dsi devices, but I am not sure if it is worth time. Little git grep experiment: # git grep compatible -- drivers/gpu/drm/panel | wc -l 219 panels in total # sed -n 's/.*\(MIPI_DSI[^ \t]*\).*/\1/p' include/drm/drm_mipi_dsi.h | xargs -I{} sh -c 'echo -n {}:; git grep {} | wc -l' MIPI_DSI_MODE_VIDEO:68 MIPI_DSI_MODE_VIDEO_BURST:23 MIPI_DSI_MODE_VIDEO_SYNC_PULSE:20 MIPI_DSI_MODE_VIDEO_AUTO_VERT:1 MIPI_DSI_MODE_VIDEO_HSE:6 MIPI_DSI_MODE_VIDEO_NO_HFP:1 MIPI_DSI_MODE_VIDEO_NO_HBP:1 MIPI_DSI_MODE_VIDEO_NO_HSA:1 MIPI_DSI_MODE_VSYNC_FLUSH:1 MIPI_DSI_MODE_NO_EOT_PACKET:16 MIPI_DSI_CLOCK_NON_CONTINUOUS:19 MIPI_DSI_MODE_LPM:54 At least, there is no regret flipping the polarity for MIPI_DSI_MODE_VIDEO_NO_HFP/HBP/HSA. I guess we could consider flipping the default for MIPI_DSI_MODE_VIDEO and MIPI_DSI_MODE_LPM (some drivers set the flags in code, instead of a structure, so I think MIPI_DSI_MODE_VIDEO is almost always set). Still not volunteering ,-P > > This solution is good for me. > > > Regards > > Andrzej > > > > > > > >>> /* disable hfront-porch area */ > >>> -#define MIPI_DSI_MODE_VIDEO_HFP BIT(5) > >>> +#define MIPI_DSI_MODE_VIDEO_NO_HFP BIT(5) > >>> /* disable hback-porch area */ > >>> -#define MIPI_DSI_MODE_VIDEO_HBP BIT(6) > >>> +#define MIPI_DSI_MODE_VIDEO_NO_HBP BIT(6) > >>> /* disable hsync-active area */ > >>> -#define MIPI_DSI_MODE_VIDEO_HSA BIT(7) > >>> +#define MIPI_DSI_MODE_VIDEO_NO_HSA BIT(7) > >>> /* flush display FIFO on vsync pulse */ > >>> #define MIPI_DSI_MODE_VSYNC_FLUSH BIT(8) > >>> /* disable EoT packets in HS mode */ > >>> -#define MIPI_DSI_MODE_EOT_PACKET BIT(9) > >>> +#define MIPI_DSI_MODE_NO_EOT_PACKET BIT(9) > >>> /* device supports non-continuous clock behavior (DSI spec 5.6.1) */ > >>> #define MIPI_DSI_CLOCK_NON_CONTINUOUS BIT(10) > >>> /* transmit data in low power */ > >> -- > >> Regards, > >> > >> Laurent Pinchart > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://protect2.fireeye.com/v1/url?k=e6f0d6d2-b96befef-e6f15d9d-0cc47a31309a-f4be6a0935319c2d&q=1&e=5e175166-1972-4f28-a483-e9a65c07e25f&u=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel