From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org>
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 D1629C433F5
	for <linux-mediatek@archiver.kernel.org>; Thu, 12 May 2022 07:45:18 +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:In-Reply-To:MIME-Version:References:
	Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:
	Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:
	List-Owner; bh=IBZQxtRWCM6VFlsdTsiahNloGA9LcA5H7W9Mgp+NRqg=; b=3ZKdl71qLDGSSA
	HcnjO73Y/iOLHbvA6Y4p2vHwFNpz6pJRl0E7iH9U/KXgV2AFCkAqUIpF74hZS+Ivq9mDvioaKGzEU
	aRBydgFWebCLHJZuq7ZrWKgTfZA/7OzCPSKCEojuXlhqYY8gyTDcJdEEZvsiwIWAnItNccLDIO3rT
	CaFfYtbbUUSbycyXS2NGFTYnGWJQrD8uTBakhY7KYC2xQ/rSzOt4Hll0zwwaKTXN0l9IKfw9fWXoz
	kFAX27kCAJGGVCUEGJlid4iSQBL5Dojn69JMJgxp6ctwemL1zdtf0OU5Zz6TPiQVDOLYHkJVrp7XH
	psnyi51F80smlyxT6BCw==;
Received: from localhost ([::1] helo=bombadil.infradead.org)
	by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux))
	id 1np3VX-00Anux-SC; Thu, 12 May 2022 07:45:11 +0000
Received: from wnew4-smtp.messagingengine.com ([64.147.123.18])
 by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux))
 id 1np3VL-00Anr5-MM; Thu, 12 May 2022 07:45:02 +0000
Received: from compute3.internal (compute3.nyi.internal [10.202.2.43])
 by mailnew.west.internal (Postfix) with ESMTP id DB9CC2B059AE;
 Thu, 12 May 2022 03:44:50 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
 by compute3.internal (MEProxy); Thu, 12 May 2022 03:44:53 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc
 :cc:content-transfer-encoding:content-type:date:date:from:from
 :in-reply-to:in-reply-to:message-id:mime-version:references
 :reply-to:sender:subject:subject:to:to; s=fm3; t=1652341490; x=
 1652348690; bh=F/3C1V38ahUzifyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=f
 oyo14B5q77HWBztYyB6jy0JiEQ9Ii/BiEb6AG0JsU2fDxdZqUcK68yNGAC7EoaS0
 MLdiGoW2wTpmdb51vgwhwQiNWtPftgTIU60n5uuzPE360fUVgjTUAya9IPqnwNKb
 BnviiRI1bsbrEa+UsHZFVi+kJ9LICkw7R3oWYCMgAfhrwml2d5eev3yTn9skCxBg
 VH8/RSJY4M5b81xtpsc+F9ey0/umPWOxbsk7XMP+7d9G9PDaBbhhl/f5taTsz5RB
 6dmN6bGq0hhSJ7jW2s6IqG39WvMgsRgpCq0P2vXnglINSYJVGREHl0DALEPDRllK
 ioGt2OrO1hw7MXf06E+fg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:cc:content-transfer-encoding
 :content-type:date:date:from:from:in-reply-to:in-reply-to
 :message-id:mime-version:references:reply-to:sender:subject
 :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
 :x-sasl-enc; s=fm1; t=1652341490; x=1652348690; bh=F/3C1V38ahUzi
 fyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=PwYCoCpllwsaeQDZ/UbNYqufIcU3D
 ZUWgY8F7k7Saqv2O6zyjaOoYF5xcDIWEnLSGiwdoo9z8lCBMr/tHJh5b0zfeFWE6
 /Z2RCJLc+r2pnrvNhImcW02ht7w/mv4bgobAnb9k0gXLDcWXAQYuz3K1z9POKN/y
 S/eqJzDh+501oGPp9exuG3dzomL2dp5vuGlGULmmWXC8cqRqYHf4z9q8S2BE/UHo
 6LyF8q2ZC7+aDXKLXASdRi94nP7is08wABRLovnl/szz04ritVADLELl1cAAm1+2
 90qcou1rCuxePxCOn11vTIOW78qBQGe0KsUAxnEyD9NN9GYZScntiuXrg==
X-ME-Sender: <xms:8bp8YqcK-fdwBudEcm5H1P_35GB8Y7LkLxZ4zJkH7TKJFPY--ts9wQ>
 <xme:8bp8YkOvhUAnmlQU_F1xaAbi92HOEjaEU2MhKVss9NVtJC1EMVj_Hk3wd-nXqt7XK
 R68JXUE9Mg8QkPMOf4>
X-ME-Received: <xmr:8bp8Yrgphjel3L9AUZeENPZqw0iDm42eTLMqQ_jugzMVv5Ex1vLAqdXFGcdo-J9RZVw2IZo3EkihHmK8LVQqL50inCCR6S8SR81EWos>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrgeeigdduvdduucetufdoteggodetrfdotf
 fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
 uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
 cujfgurhepfffhvfevuffkfhggtggugfgjsehtqhertddttddvnecuhfhrohhmpeforgig
 ihhmvgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrf
 grthhtvghrnhepheejffefgefgieevjedvfffgvdfghedtgfdvueejvdejgfffvdeuteei
 vedufedvnecuffhomhgrihhnpegsohhothhlihhnrdgtohhmnecuvehluhhsthgvrhfuih
 iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrgihimhgvsegtvghrnhhordht
 vggthh
X-ME-Proxy: <xmx:8bp8Yn9_Hnhd8voI202NNd_FlS4bmUHGt3SBFFkI5a5B-1Sfmlps8Q>
 <xmx:8bp8Ymsn4HLY6Dqe7h0m9lGhr8Ulxluora9B7319zC42M3vRgIlJhg>
 <xmx:8bp8YuGGZe5sCyNbypDOt_k8OycLAKInPJrliLcAmGL5wufwJB_p0w>
 <xmx:8rp8Yu2wRlaVZOp8yBGYU65RItWFtHlRMmFrvZtBCrOu7SA0hzrdwpMFxjs>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu,
 12 May 2022 03:44:48 -0400 (EDT)
Date: Thu, 12 May 2022 09:44:46 +0200
From: Maxime Ripard <maxime@cerno.tech>
To: Guillaume Ranquet <granquet@baylibre.com>
Cc: airlied@linux.ie, angelogioacchino.delregno@collabora.com,
 chunfeng.yun@mediatek.com, chunkuang.hu@kernel.org,
 ck.hu@mediatek.com, daniel@ffwll.ch, deller@gmx.de,
 jitao.shi@mediatek.com, kishon@ti.com, krzk+dt@kernel.org,
 maarten.lankhorst@linux.intel.com, matthias.bgg@gmail.com,
 p.zabel@pengutronix.de, robh+dt@kernel.org, tzimmermann@suse.de,
 vkoul@kernel.org, devicetree@vger.kernel.org,
 dri-devel@lists.freedesktop.org,
 linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
 linux-phy@lists.infradead.org, markyacoub@google.com,
 Markus Schneider-Pargmann <msp@baylibre.com>,
 kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
Message-ID: <20220512074446.ihilbbnbuwesxbbg@houat>
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
X-CRM114-CacheID: sfid-20220512_004500_313292_EC3548B9 
X-CRM114-Status: GOOD (  30.85  )
X-BeenThere: linux-mediatek@lists.infradead.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: <linux-mediatek.lists.infradead.org>
List-Unsubscribe: <http://lists.infradead.org/mailman/options/linux-mediatek>, 
 <mailto:linux-mediatek-request@lists.infradead.org?subject=unsubscribe>
List-Archive: <http://lists.infradead.org/pipermail/linux-mediatek/>
List-Post: <mailto:linux-mediatek@lists.infradead.org>
List-Help: <mailto:linux-mediatek-request@lists.infradead.org?subject=help>
List-Subscribe: <http://lists.infradead.org/mailman/listinfo/linux-mediatek>, 
 <mailto:linux-mediatek-request@lists.infradead.org?subject=subscribe>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: "Linux-mediatek" <linux-mediatek-bounces@lists.infradead.org>
Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org

Hi,

On Wed, May 11, 2022 at 05:59:13AM -0700, Guillaume Ranquet wrote:
> >> +#include <drm/drm_atomic_helper.h>
> >> +#include <drm/drm_bridge.h>
> >> +#include <drm/drm_crtc.h>
> >> +#include <drm/dp/drm_dp_helper.h>
> >> +#include <drm/drm_edid.h>
> >> +#include <drm/drm_of.h>
> >> +#include <drm/drm_panel.h>
> >> +#include <drm/drm_print.h>
> >> +#include <drm/drm_probe_helper.h>
> >> +#include <linux/arm-smccc.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/syscon.h>
> >> +#include <linux/nvmem-consumer.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/phy/phy.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/regmap.h>
> >> +#include <sound/hdmi-codec.h>
> >> +#include <video/videomode.h>
> >> +
> >> +#include "mtk_dp_reg.h"
> >> +
> >> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
> >> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
> >> +
> >> +//TODO: platform/device data or dts?
> >
> >DTS :)
> 
> It's probably going to be a platform_data struct for v10...
> If I have time, I'll change it to a dts property for v10.

I can't really imagine a case where we would need platform_data
nowadays. If you have a device tree, then it should be part of the
binding.

What issue would you like to address by using a platform_data?

> >> +static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
> >> +{
> >> +	return connector_status_connected;
> >> +}
> >
> >I'm not quite sure what's going on there. You seem to have some support
> >for HPD interrupts above, but you always report the display as
> >connected?
> >
> >I'd assume that either you don't have HPD support and then always report
> >it as connected, or you have HPD support and report the current status
> >in detect, but that combination seems weird.
> 
> The HPD logic needs more work, some things have been broken when I split
> the driver into three patches eDP - DP - Audio
> The assumption at first was that eDP didn't need any HPD handling... but it
> seems I was wrong and the eDP driver needs to be reworked.

That can be made into a patch of its own if you prefer.

You first introduce the driver without status reporting (always
returning connected or unknown), and then add the needed bits for HPD.

However, that first patch shouldn't contain the interrupt plumbing and
so on, it's just confusing.

> >> +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
> >> +				    struct drm_connector *connector)
> >> +{
> >> +	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
> >> +	bool enabled = mtk_dp->enabled;
> >> +	struct edid *new_edid = NULL;
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_pre_enable(bridge);
> >> +
> >> +	drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> >> +	usleep_range(2000, 5000);
> >> +
> >> +	if (mtk_dp_plug_state(mtk_dp))
> >> +		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_post_disable(bridge);
> >
> >Are you sure we can't get a mode set while get_edid is called?
> >
> >If we can, then you could end up disabling the device while it's being
> >powered on.
> 
> I'm a bit unsure, I need to spend more time in the drm stack to make sure.
> I'll get back to you when I have a definitive answer.

So, it looks like it's ok.

get_edid is your implementation of get_modes, which is called by
drm_helper_probe_single_connector_modes

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_probe_helper.c#L416

This is the standard implemantion of fill_modes, which is called
whenever the get_connector ioctl is called (or similar paths, like
drm_client_modeset_probe)

drm_helper_probe_single_connector_modes is under the assumption that the
mode_config.mutex is held though, and that the big lock. So it should be
serialized there.

Just for future proofing though, it would be better to use refcounting
there. Would runtime_pm work for you there?

> >> +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp,
> >> +					  struct drm_display_mode *mode)
> >> +{
> >> +	struct mtk_dp_timings *timings = &mtk_dp->info.timings;
> >> +
> >> +	drm_display_mode_to_videomode(mode, &timings->vm);
> >> +	timings->frame_rate = mode->clock * 1000 / mode->htotal / mode->vtotal;
> >
> >drm_mode_vrefresh()
> >
> >> +	timings->htotal = mode->htotal;
> >> +	timings->vtotal = mode->vtotal;
> >> +}
> >
> >It's not really clear to me why you need to duplicate drm_display_mode
> >here?
> >
> It's saved to be re-used in mtk_dp_set_msa().
> It's not ideal, I'll check if I can get the mode directly from mtk_dp_set_msa()

Yeah, it looks like mtk_dp_set_msa() uses fairly straightforward values,
this will be just as easy with drm_display_mode.

Maxime

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dri-devel-bounces@lists.freedesktop.org>
X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on
	aws-us-west-2-korg-lkml-1.web.codeaurora.org
Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177])
	(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 DD28BC433EF
	for <dri-devel@archiver.kernel.org>; Thu, 12 May 2022 07:44:58 +0000 (UTC)
Received: from gabe.freedesktop.org (localhost [127.0.0.1])
	by gabe.freedesktop.org (Postfix) with ESMTP id 3493010F230;
	Thu, 12 May 2022 07:44:58 +0000 (UTC)
Received: from wnew4-smtp.messagingengine.com (wnew4-smtp.messagingengine.com
 [64.147.123.18])
 by gabe.freedesktop.org (Postfix) with ESMTPS id CF68F10F230
 for <dri-devel@lists.freedesktop.org>; Thu, 12 May 2022 07:44:56 +0000 (UTC)
Received: from compute3.internal (compute3.nyi.internal [10.202.2.43])
 by mailnew.west.internal (Postfix) with ESMTP id DB9CC2B059AE;
 Thu, 12 May 2022 03:44:50 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
 by compute3.internal (MEProxy); Thu, 12 May 2022 03:44:53 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc
 :cc:content-transfer-encoding:content-type:date:date:from:from
 :in-reply-to:in-reply-to:message-id:mime-version:references
 :reply-to:sender:subject:subject:to:to; s=fm3; t=1652341490; x=
 1652348690; bh=F/3C1V38ahUzifyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=f
 oyo14B5q77HWBztYyB6jy0JiEQ9Ii/BiEb6AG0JsU2fDxdZqUcK68yNGAC7EoaS0
 MLdiGoW2wTpmdb51vgwhwQiNWtPftgTIU60n5uuzPE360fUVgjTUAya9IPqnwNKb
 BnviiRI1bsbrEa+UsHZFVi+kJ9LICkw7R3oWYCMgAfhrwml2d5eev3yTn9skCxBg
 VH8/RSJY4M5b81xtpsc+F9ey0/umPWOxbsk7XMP+7d9G9PDaBbhhl/f5taTsz5RB
 6dmN6bGq0hhSJ7jW2s6IqG39WvMgsRgpCq0P2vXnglINSYJVGREHl0DALEPDRllK
 ioGt2OrO1hw7MXf06E+fg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:cc:content-transfer-encoding
 :content-type:date:date:from:from:in-reply-to:in-reply-to
 :message-id:mime-version:references:reply-to:sender:subject
 :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
 :x-sasl-enc; s=fm1; t=1652341490; x=1652348690; bh=F/3C1V38ahUzi
 fyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=PwYCoCpllwsaeQDZ/UbNYqufIcU3D
 ZUWgY8F7k7Saqv2O6zyjaOoYF5xcDIWEnLSGiwdoo9z8lCBMr/tHJh5b0zfeFWE6
 /Z2RCJLc+r2pnrvNhImcW02ht7w/mv4bgobAnb9k0gXLDcWXAQYuz3K1z9POKN/y
 S/eqJzDh+501oGPp9exuG3dzomL2dp5vuGlGULmmWXC8cqRqYHf4z9q8S2BE/UHo
 6LyF8q2ZC7+aDXKLXASdRi94nP7is08wABRLovnl/szz04ritVADLELl1cAAm1+2
 90qcou1rCuxePxCOn11vTIOW78qBQGe0KsUAxnEyD9NN9GYZScntiuXrg==
X-ME-Sender: <xms:8bp8YqcK-fdwBudEcm5H1P_35GB8Y7LkLxZ4zJkH7TKJFPY--ts9wQ>
 <xme:8bp8YkOvhUAnmlQU_F1xaAbi92HOEjaEU2MhKVss9NVtJC1EMVj_Hk3wd-nXqt7XK
 R68JXUE9Mg8QkPMOf4>
X-ME-Received: <xmr:8bp8Yrgphjel3L9AUZeENPZqw0iDm42eTLMqQ_jugzMVv5Ex1vLAqdXFGcdo-J9RZVw2IZo3EkihHmK8LVQqL50inCCR6S8SR81EWos>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrgeeigdduvdduucetufdoteggodetrfdotf
 fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
 uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
 cujfgurhepfffhvfevuffkfhggtggugfgjsehtqhertddttddvnecuhfhrohhmpeforgig
 ihhmvgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrf
 grthhtvghrnhepheejffefgefgieevjedvfffgvdfghedtgfdvueejvdejgfffvdeuteei
 vedufedvnecuffhomhgrihhnpegsohhothhlihhnrdgtohhmnecuvehluhhsthgvrhfuih
 iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrgihimhgvsegtvghrnhhordht
 vggthh
X-ME-Proxy: <xmx:8bp8Yn9_Hnhd8voI202NNd_FlS4bmUHGt3SBFFkI5a5B-1Sfmlps8Q>
 <xmx:8bp8Ymsn4HLY6Dqe7h0m9lGhr8Ulxluora9B7319zC42M3vRgIlJhg>
 <xmx:8bp8YuGGZe5sCyNbypDOt_k8OycLAKInPJrliLcAmGL5wufwJB_p0w>
 <xmx:8rp8Yu2wRlaVZOp8yBGYU65RItWFtHlRMmFrvZtBCrOu7SA0hzrdwpMFxjs>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu,
 12 May 2022 03:44:48 -0400 (EDT)
Date: Thu, 12 May 2022 09:44:46 +0200
From: Maxime Ripard <maxime@cerno.tech>
To: Guillaume Ranquet <granquet@baylibre.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
Message-ID: <20220512074446.ihilbbnbuwesxbbg@houat>
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
X-BeenThere: dri-devel@lists.freedesktop.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Direct Rendering Infrastructure - Development
 <dri-devel.lists.freedesktop.org>
List-Unsubscribe: <https://lists.freedesktop.org/mailman/options/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <https://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <https://lists.freedesktop.org/mailman/listinfo/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=subscribe>
Cc: linux-fbdev@vger.kernel.org, devicetree@vger.kernel.org, airlied@linux.ie,
 dri-devel@lists.freedesktop.org, linux-phy@lists.infradead.org,
 kernel test robot <lkp@intel.com>, deller@gmx.de, kishon@ti.com,
 chunkuang.hu@kernel.org, jitao.shi@mediatek.com, tzimmermann@suse.de,
 Markus Schneider-Pargmann <msp@baylibre.com>, chunfeng.yun@mediatek.com,
 robh+dt@kernel.org, linux-mediatek@lists.infradead.org, matthias.bgg@gmail.com,
 linux-arm-kernel@lists.infradead.org, angelogioacchino.delregno@collabora.com,
 linux-kernel@vger.kernel.org, vkoul@kernel.org, krzk+dt@kernel.org,
 markyacoub@google.com
Errors-To: dri-devel-bounces@lists.freedesktop.org
Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>

Hi,

On Wed, May 11, 2022 at 05:59:13AM -0700, Guillaume Ranquet wrote:
> >> +#include <drm/drm_atomic_helper.h>
> >> +#include <drm/drm_bridge.h>
> >> +#include <drm/drm_crtc.h>
> >> +#include <drm/dp/drm_dp_helper.h>
> >> +#include <drm/drm_edid.h>
> >> +#include <drm/drm_of.h>
> >> +#include <drm/drm_panel.h>
> >> +#include <drm/drm_print.h>
> >> +#include <drm/drm_probe_helper.h>
> >> +#include <linux/arm-smccc.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/syscon.h>
> >> +#include <linux/nvmem-consumer.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/phy/phy.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/regmap.h>
> >> +#include <sound/hdmi-codec.h>
> >> +#include <video/videomode.h>
> >> +
> >> +#include "mtk_dp_reg.h"
> >> +
> >> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
> >> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
> >> +
> >> +//TODO: platform/device data or dts?
> >
> >DTS :)
>=20
> It's probably going to be a platform_data struct for v10...
> If I have time, I'll change it to a dts property for v10.

I can't really imagine a case where we would need platform_data
nowadays. If you have a device tree, then it should be part of the
binding.

What issue would you like to address by using a platform_data?

> >> +static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge =
*bridge)
> >> +{
> >> +	return connector_status_connected;
> >> +}
> >
> >I'm not quite sure what's going on there. You seem to have some support
> >for HPD interrupts above, but you always report the display as
> >connected?
> >
> >I'd assume that either you don't have HPD support and then always report
> >it as connected, or you have HPD support and report the current status
> >in detect, but that combination seems weird.
>=20
> The HPD logic needs more work, some things have been broken when I split
> the driver into three patches eDP - DP - Audio
> The assumption at first was that eDP didn't need any HPD handling... but =
it
> seems I was wrong and the eDP driver needs to be reworked.

That can be made into a patch of its own if you prefer.

You first introduce the driver without status reporting (always
returning connected or unknown), and then add the needed bits for HPD.

However, that first patch shouldn't contain the interrupt plumbing and
so on, it's just confusing.

> >> +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
> >> +				    struct drm_connector *connector)
> >> +{
> >> +	struct mtk_dp *mtk_dp =3D mtk_dp_from_bridge(bridge);
> >> +	bool enabled =3D mtk_dp->enabled;
> >> +	struct edid *new_edid =3D NULL;
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_pre_enable(bridge);
> >> +
> >> +	drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> >> +	usleep_range(2000, 5000);
> >> +
> >> +	if (mtk_dp_plug_state(mtk_dp))
> >> +		new_edid =3D drm_get_edid(connector, &mtk_dp->aux.ddc);
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_post_disable(bridge);
> >
> >Are you sure we can't get a mode set while get_edid is called?
> >
> >If we can, then you could end up disabling the device while it's being
> >powered on.
>=20
> I'm a bit unsure, I need to spend more time in the drm stack to make sure.
> I'll get back to you when I have a definitive answer.

So, it looks like it's ok.

get_edid is your implementation of get_modes, which is called by
drm_helper_probe_single_connector_modes

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_probe_he=
lper.c#L416

This is the standard implemantion of fill_modes, which is called
whenever the get_connector ioctl is called (or similar paths, like
drm_client_modeset_probe)

drm_helper_probe_single_connector_modes is under the assumption that the
mode_config.mutex is held though, and that the big lock. So it should be
serialized there.

Just for future proofing though, it would be better to use refcounting
there. Would runtime_pm work for you there?

> >> +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp,
> >> +					  struct drm_display_mode *mode)
> >> +{
> >> +	struct mtk_dp_timings *timings =3D &mtk_dp->info.timings;
> >> +
> >> +	drm_display_mode_to_videomode(mode, &timings->vm);
> >> +	timings->frame_rate =3D mode->clock * 1000 / mode->htotal / mode->vt=
otal;
> >
> >drm_mode_vrefresh()
> >
> >> +	timings->htotal =3D mode->htotal;
> >> +	timings->vtotal =3D mode->vtotal;
> >> +}
> >
> >It's not really clear to me why you need to duplicate drm_display_mode
> >here?
> >
> It's saved to be re-used in mtk_dp_set_msa().
> It's not ideal, I'll check if I can get the mode directly from mtk_dp_set=
_msa()

Yeah, it looks like mtk_dp_set_msa() uses fairly straightforward values,
this will be just as easy with drm_display_mode.

Maxime

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org>
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 942AFC433F5
	for <linux-phy@archiver.kernel.org>; Thu, 12 May 2022 07:45:13 +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:In-Reply-To:MIME-Version:References:
	Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:
	Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:
	List-Owner; bh=LEMsIhhoj0OZhbj07AH/kpFxICVkvCFL01Mqck5FIvk=; b=fjomHtAFJligXf
	7a9sEkrj1hSebLwlOS9vlAlwxHclAaGPo+8y6HmN1TUI8QzX8jCzzNLzAFP83mhFPUBSYu1fjJEzY
	924501PtPOheIMS2mKpWUKm5ZsfnIXAJ1Ve28YWgNmjz6M7O2wD5s2RYQNCZsJ/n05hy3/X8pX+ok
	Ob52BjXUprKliKH41xvIDs9NPqbyB3FzHc0v65JVPogqUmmkep8LFBgf9nAsZV5rh+mKhwmsU8vdp
	C4DJ6CKA+JqoUY0DcERoyJR2nD4OFO0BYFvq3mQMOLs+rEVKpTE86gBa3OyS7Mium/6JAMgGenNc2
	2VhMjInJhVdS+XfAF3Dg==;
Received: from localhost ([::1] helo=bombadil.infradead.org)
	by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux))
	id 1np3VY-00Anvw-U0; Thu, 12 May 2022 07:45:12 +0000
Received: from wnew4-smtp.messagingengine.com ([64.147.123.18])
 by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux))
 id 1np3VL-00Anr5-MM; Thu, 12 May 2022 07:45:02 +0000
Received: from compute3.internal (compute3.nyi.internal [10.202.2.43])
 by mailnew.west.internal (Postfix) with ESMTP id DB9CC2B059AE;
 Thu, 12 May 2022 03:44:50 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
 by compute3.internal (MEProxy); Thu, 12 May 2022 03:44:53 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc
 :cc:content-transfer-encoding:content-type:date:date:from:from
 :in-reply-to:in-reply-to:message-id:mime-version:references
 :reply-to:sender:subject:subject:to:to; s=fm3; t=1652341490; x=
 1652348690; bh=F/3C1V38ahUzifyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=f
 oyo14B5q77HWBztYyB6jy0JiEQ9Ii/BiEb6AG0JsU2fDxdZqUcK68yNGAC7EoaS0
 MLdiGoW2wTpmdb51vgwhwQiNWtPftgTIU60n5uuzPE360fUVgjTUAya9IPqnwNKb
 BnviiRI1bsbrEa+UsHZFVi+kJ9LICkw7R3oWYCMgAfhrwml2d5eev3yTn9skCxBg
 VH8/RSJY4M5b81xtpsc+F9ey0/umPWOxbsk7XMP+7d9G9PDaBbhhl/f5taTsz5RB
 6dmN6bGq0hhSJ7jW2s6IqG39WvMgsRgpCq0P2vXnglINSYJVGREHl0DALEPDRllK
 ioGt2OrO1hw7MXf06E+fg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:cc:content-transfer-encoding
 :content-type:date:date:from:from:in-reply-to:in-reply-to
 :message-id:mime-version:references:reply-to:sender:subject
 :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
 :x-sasl-enc; s=fm1; t=1652341490; x=1652348690; bh=F/3C1V38ahUzi
 fyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=PwYCoCpllwsaeQDZ/UbNYqufIcU3D
 ZUWgY8F7k7Saqv2O6zyjaOoYF5xcDIWEnLSGiwdoo9z8lCBMr/tHJh5b0zfeFWE6
 /Z2RCJLc+r2pnrvNhImcW02ht7w/mv4bgobAnb9k0gXLDcWXAQYuz3K1z9POKN/y
 S/eqJzDh+501oGPp9exuG3dzomL2dp5vuGlGULmmWXC8cqRqYHf4z9q8S2BE/UHo
 6LyF8q2ZC7+aDXKLXASdRi94nP7is08wABRLovnl/szz04ritVADLELl1cAAm1+2
 90qcou1rCuxePxCOn11vTIOW78qBQGe0KsUAxnEyD9NN9GYZScntiuXrg==
X-ME-Sender: <xms:8bp8YqcK-fdwBudEcm5H1P_35GB8Y7LkLxZ4zJkH7TKJFPY--ts9wQ>
 <xme:8bp8YkOvhUAnmlQU_F1xaAbi92HOEjaEU2MhKVss9NVtJC1EMVj_Hk3wd-nXqt7XK
 R68JXUE9Mg8QkPMOf4>
X-ME-Received: <xmr:8bp8Yrgphjel3L9AUZeENPZqw0iDm42eTLMqQ_jugzMVv5Ex1vLAqdXFGcdo-J9RZVw2IZo3EkihHmK8LVQqL50inCCR6S8SR81EWos>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrgeeigdduvdduucetufdoteggodetrfdotf
 fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
 uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
 cujfgurhepfffhvfevuffkfhggtggugfgjsehtqhertddttddvnecuhfhrohhmpeforgig
 ihhmvgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrf
 grthhtvghrnhepheejffefgefgieevjedvfffgvdfghedtgfdvueejvdejgfffvdeuteei
 vedufedvnecuffhomhgrihhnpegsohhothhlihhnrdgtohhmnecuvehluhhsthgvrhfuih
 iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrgihimhgvsegtvghrnhhordht
 vggthh
X-ME-Proxy: <xmx:8bp8Yn9_Hnhd8voI202NNd_FlS4bmUHGt3SBFFkI5a5B-1Sfmlps8Q>
 <xmx:8bp8Ymsn4HLY6Dqe7h0m9lGhr8Ulxluora9B7319zC42M3vRgIlJhg>
 <xmx:8bp8YuGGZe5sCyNbypDOt_k8OycLAKInPJrliLcAmGL5wufwJB_p0w>
 <xmx:8rp8Yu2wRlaVZOp8yBGYU65RItWFtHlRMmFrvZtBCrOu7SA0hzrdwpMFxjs>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu,
 12 May 2022 03:44:48 -0400 (EDT)
Date: Thu, 12 May 2022 09:44:46 +0200
From: Maxime Ripard <maxime@cerno.tech>
To: Guillaume Ranquet <granquet@baylibre.com>
Cc: airlied@linux.ie, angelogioacchino.delregno@collabora.com,
 chunfeng.yun@mediatek.com, chunkuang.hu@kernel.org,
 ck.hu@mediatek.com, daniel@ffwll.ch, deller@gmx.de,
 jitao.shi@mediatek.com, kishon@ti.com, krzk+dt@kernel.org,
 maarten.lankhorst@linux.intel.com, matthias.bgg@gmail.com,
 p.zabel@pengutronix.de, robh+dt@kernel.org, tzimmermann@suse.de,
 vkoul@kernel.org, devicetree@vger.kernel.org,
 dri-devel@lists.freedesktop.org,
 linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
 linux-phy@lists.infradead.org, markyacoub@google.com,
 Markus Schneider-Pargmann <msp@baylibre.com>,
 kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
Message-ID: <20220512074446.ihilbbnbuwesxbbg@houat>
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
X-CRM114-CacheID: sfid-20220512_004500_313292_EC3548B9 
X-CRM114-Status: GOOD (  30.85  )
X-BeenThere: linux-phy@lists.infradead.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: Linux Phy Mailing list <linux-phy.lists.infradead.org>
List-Unsubscribe: <https://lists.infradead.org/mailman/options/linux-phy>,
 <mailto:linux-phy-request@lists.infradead.org?subject=unsubscribe>
List-Archive: <http://lists.infradead.org/pipermail/linux-phy/>
List-Post: <mailto:linux-phy@lists.infradead.org>
List-Help: <mailto:linux-phy-request@lists.infradead.org?subject=help>
List-Subscribe: <https://lists.infradead.org/mailman/listinfo/linux-phy>,
 <mailto:linux-phy-request@lists.infradead.org?subject=subscribe>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: "linux-phy" <linux-phy-bounces@lists.infradead.org>
Errors-To: linux-phy-bounces+linux-phy=archiver.kernel.org@lists.infradead.org

Hi,

On Wed, May 11, 2022 at 05:59:13AM -0700, Guillaume Ranquet wrote:
> >> +#include <drm/drm_atomic_helper.h>
> >> +#include <drm/drm_bridge.h>
> >> +#include <drm/drm_crtc.h>
> >> +#include <drm/dp/drm_dp_helper.h>
> >> +#include <drm/drm_edid.h>
> >> +#include <drm/drm_of.h>
> >> +#include <drm/drm_panel.h>
> >> +#include <drm/drm_print.h>
> >> +#include <drm/drm_probe_helper.h>
> >> +#include <linux/arm-smccc.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/syscon.h>
> >> +#include <linux/nvmem-consumer.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/phy/phy.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/regmap.h>
> >> +#include <sound/hdmi-codec.h>
> >> +#include <video/videomode.h>
> >> +
> >> +#include "mtk_dp_reg.h"
> >> +
> >> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
> >> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
> >> +
> >> +//TODO: platform/device data or dts?
> >
> >DTS :)
> 
> It's probably going to be a platform_data struct for v10...
> If I have time, I'll change it to a dts property for v10.

I can't really imagine a case where we would need platform_data
nowadays. If you have a device tree, then it should be part of the
binding.

What issue would you like to address by using a platform_data?

> >> +static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
> >> +{
> >> +	return connector_status_connected;
> >> +}
> >
> >I'm not quite sure what's going on there. You seem to have some support
> >for HPD interrupts above, but you always report the display as
> >connected?
> >
> >I'd assume that either you don't have HPD support and then always report
> >it as connected, or you have HPD support and report the current status
> >in detect, but that combination seems weird.
> 
> The HPD logic needs more work, some things have been broken when I split
> the driver into three patches eDP - DP - Audio
> The assumption at first was that eDP didn't need any HPD handling... but it
> seems I was wrong and the eDP driver needs to be reworked.

That can be made into a patch of its own if you prefer.

You first introduce the driver without status reporting (always
returning connected or unknown), and then add the needed bits for HPD.

However, that first patch shouldn't contain the interrupt plumbing and
so on, it's just confusing.

> >> +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
> >> +				    struct drm_connector *connector)
> >> +{
> >> +	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
> >> +	bool enabled = mtk_dp->enabled;
> >> +	struct edid *new_edid = NULL;
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_pre_enable(bridge);
> >> +
> >> +	drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> >> +	usleep_range(2000, 5000);
> >> +
> >> +	if (mtk_dp_plug_state(mtk_dp))
> >> +		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_post_disable(bridge);
> >
> >Are you sure we can't get a mode set while get_edid is called?
> >
> >If we can, then you could end up disabling the device while it's being
> >powered on.
> 
> I'm a bit unsure, I need to spend more time in the drm stack to make sure.
> I'll get back to you when I have a definitive answer.

So, it looks like it's ok.

get_edid is your implementation of get_modes, which is called by
drm_helper_probe_single_connector_modes

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_probe_helper.c#L416

This is the standard implemantion of fill_modes, which is called
whenever the get_connector ioctl is called (or similar paths, like
drm_client_modeset_probe)

drm_helper_probe_single_connector_modes is under the assumption that the
mode_config.mutex is held though, and that the big lock. So it should be
serialized there.

Just for future proofing though, it would be better to use refcounting
there. Would runtime_pm work for you there?

> >> +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp,
> >> +					  struct drm_display_mode *mode)
> >> +{
> >> +	struct mtk_dp_timings *timings = &mtk_dp->info.timings;
> >> +
> >> +	drm_display_mode_to_videomode(mode, &timings->vm);
> >> +	timings->frame_rate = mode->clock * 1000 / mode->htotal / mode->vtotal;
> >
> >drm_mode_vrefresh()
> >
> >> +	timings->htotal = mode->htotal;
> >> +	timings->vtotal = mode->vtotal;
> >> +}
> >
> >It's not really clear to me why you need to duplicate drm_display_mode
> >here?
> >
> It's saved to be re-used in mtk_dp_set_msa().
> It's not ideal, I'll check if I can get the mode directly from mtk_dp_set_msa()

Yeah, it looks like mtk_dp_set_msa() uses fairly straightforward values,
this will be just as easy with drm_display_mode.

Maxime

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org>
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 A9FDAC433EF
	for <linux-arm-kernel@archiver.kernel.org>; Thu, 12 May 2022 07:46:26 +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:In-Reply-To:MIME-Version:References:
	Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description:
	Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:
	List-Owner; bh=pZr7lGD4bNpPRbp45OOAY5gBh+k39QptrsN42W6gJ38=; b=c7XDSwoY8AGqR8
	V23SN7yxcHD8rfhVL06iWIbvGt6JItyvgH2hbemUR8yb3NX4WVkAtuVPLbm2V1Rf98+DD2XrY6r/L
	ot+vXhy02KqpFOAEA0E9LV5boGuNGyZNHnlTFzip2XMj3eytTzLurI1rPOSS7icCP5eK7leq4vo4s
	7nB0xVDfGsQ7LgWZQoWVodS6KKsqYr1i82urteAi87CoRgabV41EGj/eGRzR9Tg9DpdqJHtWvlO97
	g6i9tQeDx5wZIHlf3R+kfuWlbIzd3LNvXmaB7uQm3B967NSgIu0dJXZbb6plXwUJjeynkVbuTIOFZ
	vB+c8EzPdIOKcKMcU5mg==;
Received: from localhost ([::1] helo=bombadil.infradead.org)
	by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux))
	id 1np3VP-00AntE-Ju; Thu, 12 May 2022 07:45:03 +0000
Received: from wnew4-smtp.messagingengine.com ([64.147.123.18])
 by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux))
 id 1np3VL-00Anr5-MM; Thu, 12 May 2022 07:45:02 +0000
Received: from compute3.internal (compute3.nyi.internal [10.202.2.43])
 by mailnew.west.internal (Postfix) with ESMTP id DB9CC2B059AE;
 Thu, 12 May 2022 03:44:50 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
 by compute3.internal (MEProxy); Thu, 12 May 2022 03:44:53 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc
 :cc:content-transfer-encoding:content-type:date:date:from:from
 :in-reply-to:in-reply-to:message-id:mime-version:references
 :reply-to:sender:subject:subject:to:to; s=fm3; t=1652341490; x=
 1652348690; bh=F/3C1V38ahUzifyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=f
 oyo14B5q77HWBztYyB6jy0JiEQ9Ii/BiEb6AG0JsU2fDxdZqUcK68yNGAC7EoaS0
 MLdiGoW2wTpmdb51vgwhwQiNWtPftgTIU60n5uuzPE360fUVgjTUAya9IPqnwNKb
 BnviiRI1bsbrEa+UsHZFVi+kJ9LICkw7R3oWYCMgAfhrwml2d5eev3yTn9skCxBg
 VH8/RSJY4M5b81xtpsc+F9ey0/umPWOxbsk7XMP+7d9G9PDaBbhhl/f5taTsz5RB
 6dmN6bGq0hhSJ7jW2s6IqG39WvMgsRgpCq0P2vXnglINSYJVGREHl0DALEPDRllK
 ioGt2OrO1hw7MXf06E+fg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
 messagingengine.com; h=cc:cc:content-transfer-encoding
 :content-type:date:date:from:from:in-reply-to:in-reply-to
 :message-id:mime-version:references:reply-to:sender:subject
 :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
 :x-sasl-enc; s=fm1; t=1652341490; x=1652348690; bh=F/3C1V38ahUzi
 fyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=PwYCoCpllwsaeQDZ/UbNYqufIcU3D
 ZUWgY8F7k7Saqv2O6zyjaOoYF5xcDIWEnLSGiwdoo9z8lCBMr/tHJh5b0zfeFWE6
 /Z2RCJLc+r2pnrvNhImcW02ht7w/mv4bgobAnb9k0gXLDcWXAQYuz3K1z9POKN/y
 S/eqJzDh+501oGPp9exuG3dzomL2dp5vuGlGULmmWXC8cqRqYHf4z9q8S2BE/UHo
 6LyF8q2ZC7+aDXKLXASdRi94nP7is08wABRLovnl/szz04ritVADLELl1cAAm1+2
 90qcou1rCuxePxCOn11vTIOW78qBQGe0KsUAxnEyD9NN9GYZScntiuXrg==
X-ME-Sender: <xms:8bp8YqcK-fdwBudEcm5H1P_35GB8Y7LkLxZ4zJkH7TKJFPY--ts9wQ>
 <xme:8bp8YkOvhUAnmlQU_F1xaAbi92HOEjaEU2MhKVss9NVtJC1EMVj_Hk3wd-nXqt7XK
 R68JXUE9Mg8QkPMOf4>
X-ME-Received: <xmr:8bp8Yrgphjel3L9AUZeENPZqw0iDm42eTLMqQ_jugzMVv5Ex1vLAqdXFGcdo-J9RZVw2IZo3EkihHmK8LVQqL50inCCR6S8SR81EWos>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrgeeigdduvdduucetufdoteggodetrfdotf
 fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
 uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
 cujfgurhepfffhvfevuffkfhggtggugfgjsehtqhertddttddvnecuhfhrohhmpeforgig
 ihhmvgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrf
 grthhtvghrnhepheejffefgefgieevjedvfffgvdfghedtgfdvueejvdejgfffvdeuteei
 vedufedvnecuffhomhgrihhnpegsohhothhlihhnrdgtohhmnecuvehluhhsthgvrhfuih
 iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrgihimhgvsegtvghrnhhordht
 vggthh
X-ME-Proxy: <xmx:8bp8Yn9_Hnhd8voI202NNd_FlS4bmUHGt3SBFFkI5a5B-1Sfmlps8Q>
 <xmx:8bp8Ymsn4HLY6Dqe7h0m9lGhr8Ulxluora9B7319zC42M3vRgIlJhg>
 <xmx:8bp8YuGGZe5sCyNbypDOt_k8OycLAKInPJrliLcAmGL5wufwJB_p0w>
 <xmx:8rp8Yu2wRlaVZOp8yBGYU65RItWFtHlRMmFrvZtBCrOu7SA0hzrdwpMFxjs>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu,
 12 May 2022 03:44:48 -0400 (EDT)
Date: Thu, 12 May 2022 09:44:46 +0200
From: Maxime Ripard <maxime@cerno.tech>
To: Guillaume Ranquet <granquet@baylibre.com>
Cc: airlied@linux.ie, angelogioacchino.delregno@collabora.com,
 chunfeng.yun@mediatek.com, chunkuang.hu@kernel.org,
 ck.hu@mediatek.com, daniel@ffwll.ch, deller@gmx.de,
 jitao.shi@mediatek.com, kishon@ti.com, krzk+dt@kernel.org,
 maarten.lankhorst@linux.intel.com, matthias.bgg@gmail.com,
 p.zabel@pengutronix.de, robh+dt@kernel.org, tzimmermann@suse.de,
 vkoul@kernel.org, devicetree@vger.kernel.org,
 dri-devel@lists.freedesktop.org,
 linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
 linux-phy@lists.infradead.org, markyacoub@google.com,
 Markus Schneider-Pargmann <msp@baylibre.com>,
 kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
Message-ID: <20220512074446.ihilbbnbuwesxbbg@houat>
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
MIME-Version: 1.0
Content-Disposition: inline
In-Reply-To: <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
X-CRM114-CacheID: sfid-20220512_004500_313292_EC3548B9 
X-CRM114-Status: GOOD (  30.85  )
X-BeenThere: linux-arm-kernel@lists.infradead.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: <linux-arm-kernel.lists.infradead.org>
List-Unsubscribe: <http://lists.infradead.org/mailman/options/linux-arm-kernel>, 
 <mailto:linux-arm-kernel-request@lists.infradead.org?subject=unsubscribe>
List-Archive: <http://lists.infradead.org/pipermail/linux-arm-kernel/>
List-Post: <mailto:linux-arm-kernel@lists.infradead.org>
List-Help: <mailto:linux-arm-kernel-request@lists.infradead.org?subject=help>
List-Subscribe: <http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>, 
 <mailto:linux-arm-kernel-request@lists.infradead.org?subject=subscribe>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Sender: "linux-arm-kernel" <linux-arm-kernel-bounces@lists.infradead.org>
Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org

Hi,

On Wed, May 11, 2022 at 05:59:13AM -0700, Guillaume Ranquet wrote:
> >> +#include <drm/drm_atomic_helper.h>
> >> +#include <drm/drm_bridge.h>
> >> +#include <drm/drm_crtc.h>
> >> +#include <drm/dp/drm_dp_helper.h>
> >> +#include <drm/drm_edid.h>
> >> +#include <drm/drm_of.h>
> >> +#include <drm/drm_panel.h>
> >> +#include <drm/drm_print.h>
> >> +#include <drm/drm_probe_helper.h>
> >> +#include <linux/arm-smccc.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/syscon.h>
> >> +#include <linux/nvmem-consumer.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/phy/phy.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/regmap.h>
> >> +#include <sound/hdmi-codec.h>
> >> +#include <video/videomode.h>
> >> +
> >> +#include "mtk_dp_reg.h"
> >> +
> >> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
> >> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
> >> +
> >> +//TODO: platform/device data or dts?
> >
> >DTS :)
> 
> It's probably going to be a platform_data struct for v10...
> If I have time, I'll change it to a dts property for v10.

I can't really imagine a case where we would need platform_data
nowadays. If you have a device tree, then it should be part of the
binding.

What issue would you like to address by using a platform_data?

> >> +static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge *bridge)
> >> +{
> >> +	return connector_status_connected;
> >> +}
> >
> >I'm not quite sure what's going on there. You seem to have some support
> >for HPD interrupts above, but you always report the display as
> >connected?
> >
> >I'd assume that either you don't have HPD support and then always report
> >it as connected, or you have HPD support and report the current status
> >in detect, but that combination seems weird.
> 
> The HPD logic needs more work, some things have been broken when I split
> the driver into three patches eDP - DP - Audio
> The assumption at first was that eDP didn't need any HPD handling... but it
> seems I was wrong and the eDP driver needs to be reworked.

That can be made into a patch of its own if you prefer.

You first introduce the driver without status reporting (always
returning connected or unknown), and then add the needed bits for HPD.

However, that first patch shouldn't contain the interrupt plumbing and
so on, it's just confusing.

> >> +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
> >> +				    struct drm_connector *connector)
> >> +{
> >> +	struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge);
> >> +	bool enabled = mtk_dp->enabled;
> >> +	struct edid *new_edid = NULL;
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_pre_enable(bridge);
> >> +
> >> +	drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> >> +	usleep_range(2000, 5000);
> >> +
> >> +	if (mtk_dp_plug_state(mtk_dp))
> >> +		new_edid = drm_get_edid(connector, &mtk_dp->aux.ddc);
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_post_disable(bridge);
> >
> >Are you sure we can't get a mode set while get_edid is called?
> >
> >If we can, then you could end up disabling the device while it's being
> >powered on.
> 
> I'm a bit unsure, I need to spend more time in the drm stack to make sure.
> I'll get back to you when I have a definitive answer.

So, it looks like it's ok.

get_edid is your implementation of get_modes, which is called by
drm_helper_probe_single_connector_modes

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_probe_helper.c#L416

This is the standard implemantion of fill_modes, which is called
whenever the get_connector ioctl is called (or similar paths, like
drm_client_modeset_probe)

drm_helper_probe_single_connector_modes is under the assumption that the
mode_config.mutex is held though, and that the big lock. So it should be
serialized there.

Just for future proofing though, it would be better to use refcounting
there. Would runtime_pm work for you there?

> >> +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp,
> >> +					  struct drm_display_mode *mode)
> >> +{
> >> +	struct mtk_dp_timings *timings = &mtk_dp->info.timings;
> >> +
> >> +	drm_display_mode_to_videomode(mode, &timings->vm);
> >> +	timings->frame_rate = mode->clock * 1000 / mode->htotal / mode->vtotal;
> >
> >drm_mode_vrefresh()
> >
> >> +	timings->htotal = mode->htotal;
> >> +	timings->vtotal = mode->vtotal;
> >> +}
> >
> >It's not really clear to me why you need to duplicate drm_display_mode
> >here?
> >
> It's saved to be re-used in mtk_dp_set_msa().
> It's not ideal, I'll check if I can get the mode directly from mtk_dp_set_msa()

Yeah, it looks like mtk_dp_set_msa() uses fairly straightforward values,
this will be just as easy with drm_display_mode.

Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@kernel.org>
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 DDCE4C433F5
	for <linux-kernel@archiver.kernel.org>; Thu, 12 May 2022 07:51:39 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1350873AbiELHvf (ORCPT
        <rfc822;linux-kernel@archiver.kernel.org>);
        Thu, 12 May 2022 03:51:35 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37094 "EHLO
        lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1351176AbiELHvP (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 12 May 2022 03:51:15 -0400
X-Greylist: delayed 372 seconds by postgrey-1.37 at lindbergh.monkeyblade.net; Thu, 12 May 2022 00:51:09 PDT
Received: from wnew4-smtp.messagingengine.com (wnew4-smtp.messagingengine.com [64.147.123.18])
        by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5D6841838F;
        Thu, 12 May 2022 00:51:08 -0700 (PDT)
Received: from compute3.internal (compute3.nyi.internal [10.202.2.43])
        by mailnew.west.internal (Postfix) with ESMTP id DB9CC2B059AE;
        Thu, 12 May 2022 03:44:50 -0400 (EDT)
Received: from mailfrontend2 ([10.202.2.163])
  by compute3.internal (MEProxy); Thu, 12 May 2022 03:44:53 -0400
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cerno.tech; h=cc
        :cc:content-transfer-encoding:content-type:date:date:from:from
        :in-reply-to:in-reply-to:message-id:mime-version:references
        :reply-to:sender:subject:subject:to:to; s=fm3; t=1652341490; x=
        1652348690; bh=F/3C1V38ahUzifyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=f
        oyo14B5q77HWBztYyB6jy0JiEQ9Ii/BiEb6AG0JsU2fDxdZqUcK68yNGAC7EoaS0
        MLdiGoW2wTpmdb51vgwhwQiNWtPftgTIU60n5uuzPE360fUVgjTUAya9IPqnwNKb
        BnviiRI1bsbrEa+UsHZFVi+kJ9LICkw7R3oWYCMgAfhrwml2d5eev3yTn9skCxBg
        VH8/RSJY4M5b81xtpsc+F9ey0/umPWOxbsk7XMP+7d9G9PDaBbhhl/f5taTsz5RB
        6dmN6bGq0hhSJ7jW2s6IqG39WvMgsRgpCq0P2vXnglINSYJVGREHl0DALEPDRllK
        ioGt2OrO1hw7MXf06E+fg==
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=
        messagingengine.com; h=cc:cc:content-transfer-encoding
        :content-type:date:date:from:from:in-reply-to:in-reply-to
        :message-id:mime-version:references:reply-to:sender:subject
        :subject:to:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender
        :x-sasl-enc; s=fm1; t=1652341490; x=1652348690; bh=F/3C1V38ahUzi
        fyHzEs+CWDQJfKIOWf16zUshCZyYK4=; b=PwYCoCpllwsaeQDZ/UbNYqufIcU3D
        ZUWgY8F7k7Saqv2O6zyjaOoYF5xcDIWEnLSGiwdoo9z8lCBMr/tHJh5b0zfeFWE6
        /Z2RCJLc+r2pnrvNhImcW02ht7w/mv4bgobAnb9k0gXLDcWXAQYuz3K1z9POKN/y
        S/eqJzDh+501oGPp9exuG3dzomL2dp5vuGlGULmmWXC8cqRqYHf4z9q8S2BE/UHo
        6LyF8q2ZC7+aDXKLXASdRi94nP7is08wABRLovnl/szz04ritVADLELl1cAAm1+2
        90qcou1rCuxePxCOn11vTIOW78qBQGe0KsUAxnEyD9NN9GYZScntiuXrg==
X-ME-Sender: <xms:8bp8YqcK-fdwBudEcm5H1P_35GB8Y7LkLxZ4zJkH7TKJFPY--ts9wQ>
    <xme:8bp8YkOvhUAnmlQU_F1xaAbi92HOEjaEU2MhKVss9NVtJC1EMVj_Hk3wd-nXqt7XK
    R68JXUE9Mg8QkPMOf4>
X-ME-Received: <xmr:8bp8Yrgphjel3L9AUZeENPZqw0iDm42eTLMqQ_jugzMVv5Ex1vLAqdXFGcdo-J9RZVw2IZo3EkihHmK8LVQqL50inCCR6S8SR81EWos>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrgeeigdduvdduucetufdoteggodetrfdotf
    fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
    uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
    cujfgurhepfffhvfevuffkfhggtggugfgjsehtqhertddttddvnecuhfhrohhmpeforgig
    ihhmvgcutfhiphgrrhguuceomhgrgihimhgvsegtvghrnhhordhtvggthheqnecuggftrf
    grthhtvghrnhepheejffefgefgieevjedvfffgvdfghedtgfdvueejvdejgfffvdeuteei
    vedufedvnecuffhomhgrihhnpegsohhothhlihhnrdgtohhmnecuvehluhhsthgvrhfuih
    iivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepmhgrgihimhgvsegtvghrnhhordht
    vggthh
X-ME-Proxy: <xmx:8bp8Yn9_Hnhd8voI202NNd_FlS4bmUHGt3SBFFkI5a5B-1Sfmlps8Q>
    <xmx:8bp8Ymsn4HLY6Dqe7h0m9lGhr8Ulxluora9B7319zC42M3vRgIlJhg>
    <xmx:8bp8YuGGZe5sCyNbypDOt_k8OycLAKInPJrliLcAmGL5wufwJB_p0w>
    <xmx:8rp8Yu2wRlaVZOp8yBGYU65RItWFtHlRMmFrvZtBCrOu7SA0hzrdwpMFxjs>
Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu,
 12 May 2022 03:44:48 -0400 (EDT)
Date:   Thu, 12 May 2022 09:44:46 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Guillaume Ranquet <granquet@baylibre.com>
Cc:     airlied@linux.ie, angelogioacchino.delregno@collabora.com,
        chunfeng.yun@mediatek.com, chunkuang.hu@kernel.org,
        ck.hu@mediatek.com, daniel@ffwll.ch, deller@gmx.de,
        jitao.shi@mediatek.com, kishon@ti.com, krzk+dt@kernel.org,
        maarten.lankhorst@linux.intel.com, matthias.bgg@gmail.com,
        p.zabel@pengutronix.de, robh+dt@kernel.org, tzimmermann@suse.de,
        vkoul@kernel.org, devicetree@vger.kernel.org,
        dri-devel@lists.freedesktop.org,
        linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
        linux-phy@lists.infradead.org, markyacoub@google.com,
        Markus Schneider-Pargmann <msp@baylibre.com>,
        kernel test robot <lkp@intel.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
Message-ID: <20220512074446.ihilbbnbuwesxbbg@houat>
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

On Wed, May 11, 2022 at 05:59:13AM -0700, Guillaume Ranquet wrote:
> >> +#include <drm/drm_atomic_helper.h>
> >> +#include <drm/drm_bridge.h>
> >> +#include <drm/drm_crtc.h>
> >> +#include <drm/dp/drm_dp_helper.h>
> >> +#include <drm/drm_edid.h>
> >> +#include <drm/drm_of.h>
> >> +#include <drm/drm_panel.h>
> >> +#include <drm/drm_print.h>
> >> +#include <drm/drm_probe_helper.h>
> >> +#include <linux/arm-smccc.h>
> >> +#include <linux/clk.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/errno.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/mfd/syscon.h>
> >> +#include <linux/nvmem-consumer.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_irq.h>
> >> +#include <linux/of_platform.h>
> >> +#include <linux/phy/phy.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/regmap.h>
> >> +#include <sound/hdmi-codec.h>
> >> +#include <video/videomode.h>
> >> +
> >> +#include "mtk_dp_reg.h"
> >> +
> >> +#define MTK_DP_AUX_WAIT_REPLY_COUNT 20
> >> +#define MTK_DP_CHECK_SINK_CAP_TIMEOUT_COUNT 3
> >> +
> >> +//TODO: platform/device data or dts?
> >
> >DTS :)
>=20
> It's probably going to be a platform_data struct for v10...
> If I have time, I'll change it to a dts property for v10.

I can't really imagine a case where we would need platform_data
nowadays. If you have a device tree, then it should be part of the
binding.

What issue would you like to address by using a platform_data?

> >> +static enum drm_connector_status mtk_dp_bdg_detect(struct drm_bridge =
*bridge)
> >> +{
> >> +	return connector_status_connected;
> >> +}
> >
> >I'm not quite sure what's going on there. You seem to have some support
> >for HPD interrupts above, but you always report the display as
> >connected?
> >
> >I'd assume that either you don't have HPD support and then always report
> >it as connected, or you have HPD support and report the current status
> >in detect, but that combination seems weird.
>=20
> The HPD logic needs more work, some things have been broken when I split
> the driver into three patches eDP - DP - Audio
> The assumption at first was that eDP didn't need any HPD handling... but =
it
> seems I was wrong and the eDP driver needs to be reworked.

That can be made into a patch of its own if you prefer.

You first introduce the driver without status reporting (always
returning connected or unknown), and then add the needed bits for HPD.

However, that first patch shouldn't contain the interrupt plumbing and
so on, it's just confusing.

> >> +static struct edid *mtk_dp_get_edid(struct drm_bridge *bridge,
> >> +				    struct drm_connector *connector)
> >> +{
> >> +	struct mtk_dp *mtk_dp =3D mtk_dp_from_bridge(bridge);
> >> +	bool enabled =3D mtk_dp->enabled;
> >> +	struct edid *new_edid =3D NULL;
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_pre_enable(bridge);
> >> +
> >> +	drm_dp_dpcd_writeb(&mtk_dp->aux, DP_SET_POWER, DP_SET_POWER_D0);
> >> +	usleep_range(2000, 5000);
> >> +
> >> +	if (mtk_dp_plug_state(mtk_dp))
> >> +		new_edid =3D drm_get_edid(connector, &mtk_dp->aux.ddc);
> >> +
> >> +	if (!enabled)
> >> +		drm_bridge_chain_post_disable(bridge);
> >
> >Are you sure we can't get a mode set while get_edid is called?
> >
> >If we can, then you could end up disabling the device while it's being
> >powered on.
>=20
> I'm a bit unsure, I need to spend more time in the drm stack to make sure.
> I'll get back to you when I have a definitive answer.

So, it looks like it's ok.

get_edid is your implementation of get_modes, which is called by
drm_helper_probe_single_connector_modes

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_probe_he=
lper.c#L416

This is the standard implemantion of fill_modes, which is called
whenever the get_connector ioctl is called (or similar paths, like
drm_client_modeset_probe)

drm_helper_probe_single_connector_modes is under the assumption that the
mode_config.mutex is held though, and that the big lock. So it should be
serialized there.

Just for future proofing though, it would be better to use refcounting
there. Would runtime_pm work for you there?

> >> +static void mtk_dp_parse_drm_mode_timings(struct mtk_dp *mtk_dp,
> >> +					  struct drm_display_mode *mode)
> >> +{
> >> +	struct mtk_dp_timings *timings =3D &mtk_dp->info.timings;
> >> +
> >> +	drm_display_mode_to_videomode(mode, &timings->vm);
> >> +	timings->frame_rate =3D mode->clock * 1000 / mode->htotal / mode->vt=
otal;
> >
> >drm_mode_vrefresh()
> >
> >> +	timings->htotal =3D mode->htotal;
> >> +	timings->vtotal =3D mode->vtotal;
> >> +}
> >
> >It's not really clear to me why you need to duplicate drm_display_mode
> >here?
> >
> It's saved to be re-used in mtk_dp_set_msa().
> It's not ideal, I'll check if I can get the mode directly from mtk_dp_set=
_msa()

Yeah, it looks like mtk_dp_set_msa() uses fairly straightforward values,
this will be just as easy with drm_display_mode.

Maxime