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 42A71C433FE
	for <linux-kernel@archiver.kernel.org>; Thu, 19 May 2022 16:27:11 +0000 (UTC)
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S242002AbiESQ1K (ORCPT <rfc822;linux-kernel@archiver.kernel.org>);
        Thu, 19 May 2022 12:27:10 -0400
Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60316 "EHLO
        lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S241997AbiESQ1E (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 19 May 2022 12:27:04 -0400
Received: from mail-vs1-xe34.google.com (mail-vs1-xe34.google.com [IPv6:2607:f8b0:4864:20::e34])
        by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 42EEED4117
        for <linux-kernel@vger.kernel.org>; Thu, 19 May 2022 09:27:01 -0700 (PDT)
Received: by mail-vs1-xe34.google.com with SMTP id b7so5968937vsq.1
        for <linux-kernel@vger.kernel.org>; Thu, 19 May 2022 09:27:01 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=baylibre-com.20210112.gappssmtp.com; s=20210112;
        h=from:user-agent:references:in-reply-to:mime-version:date:message-id
         :subject:to:cc;
        bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
        b=Wdbve9pVOQfTCQ8qHIZq21Qqz+xsUOAaavE8ZtUJ9EblcqE54EBHnNXjuCxpwYGpor
         x4JgBbxZg77ZEAnCTT0kqlEZ/o/n17D8A0eVw1bfm2qy1z6ki8Rk1os7osw5nwYQtmDJ
         dWe7PFxstS5Yi5ZvCBNqRNNi55N7BlBUXf4UbrdAgQFpVWfVo7+Qk0uy/cnuWKCmsn1F
         B7HDdWZgo2ml5tOL/yDMoTEHgTVr2MA4oll4/SoapPprGkcSufMBge8WouN5+m6vPQ49
         A+4KWf4Mi66zOuv/wwToUPvT+BZC3ilqbBHUO6PSVKpTz/Hmp8QRDEB6XzvnVrGrAt6Y
         o6eg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20210112;
        h=x-gm-message-state:from:user-agent:references:in-reply-to
         :mime-version:date:message-id:subject:to:cc;
        bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
        b=4NVsnFMhw5RNxW1vpa8qTatVR+rTM6mjBEwwRfvmlZG8ce7MNpmRe0p+vCeg+vTa4x
         4rhhKgFJaY0wt3+bOSZoZDPXKVDAsnx4o2kxe+hTNbW0yU1YCUx/i9JaAoXsDrmTvzwk
         CjhOuqZ5y7FsmgpgBP16e/H/NBwCT85nJvk6c8DYyQG/5tJBvbxrGjfQOEVcuDYqUEOO
         m5ZHcOjouax+Rkez1F4L6rDRoFAp6B4Vx2reIQDi0tsaIWw6DYKy5Aek1CWROmGXKrg9
         hex1HNby/dAVVZglavoW7LlXqABdq8Qhbwy40VIzcptiaqMnFA4a5KoTtEDobHZTDGgk
         dGsA==
X-Gm-Message-State: AOAM530oUvTHT2xLYis3Rxjlr9ACwyPDitt/x6jfS6x8VaaF04sih1U6
        WQKNL8HJ4S8gD7LI4PZLKyDsUNEyhzZJvJyURkBQRw==
X-Google-Smtp-Source: ABdhPJx+NNr1h6VgTuwMD1Bhi8EcRUBgWUvE0krCGgcnnrPzo4cs17ylt6Dfy/iIo/Qh4cdkDItrCl7KQ2kO/HmmW/M=
X-Received: by 2002:a67:d516:0:b0:335:c0b5:1e5d with SMTP id
 l22-20020a67d516000000b00335c0b51e5dmr2517279vsj.13.1652977620240; Thu, 19
 May 2022 09:27:00 -0700 (PDT)
Received: from 753933720722 named unknown by gmailapi.google.com with
 HTTPREST; Thu, 19 May 2022 09:26:59 -0700
From:   Guillaume Ranquet <granquet@baylibre.com>
User-Agent: meli 0.7.2
References: <20220327223927.20848-1-granquet@baylibre.com> <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat> <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
 <20220512074446.ihilbbnbuwesxbbg@houat>
In-Reply-To: <20220512074446.ihilbbnbuwesxbbg@houat>
MIME-Version: 1.0
Date:   Thu, 19 May 2022 09:26:59 -0700
Message-ID: <CABnWg9tHRc_7VZ1z6NeMEDfQvxam_xOimBnM=hzBUqkhzPOSCA@mail.gmail.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort driver
To:     Maxime Ripard <maxime@cerno.tech>
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, 12 May 2022 09:44, Maxime Ripard <maxime@cerno.tech> wrote:
>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?
>

Ok, I'll migrate to dt then. I didn't realize platform_data were depreciated.

Angelo wants the MAX_LINRATE and MAX_LANES defines to be configurable.
I imagined platform_data would be more appropriate as (per my understanding) the
limitation is associated with a specific SoC.

>> >> +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.
>

After discussing a while with Mediatek, it appears that hot plug detection
needs to be handled even for eDP... (which is always connected).
So I'll revert to the split I made earlier in v8 where hot plug detection was
part of the eDP patch the addition of the External display port was a
trivial patch [1].

>> >> +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?
>

Thx for looking into this for me.
Not sure runtime_pm works here as it would only refcount if compiled
with CONFIG_PM?
I'd rather use the enabled field as a refcounter instead of a boolean.

Unless I'm totally missing your point?

Thx,
Guillaume.

>> >> +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


[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.18563-17-granquet@baylibre.com/

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 93A9DC433F5
	for <linux-mediatek@archiver.kernel.org>; Thu, 19 May 2022 16:27:28 +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:Cc:To:Subject:Message-ID:Date:
	MIME-Version:In-Reply-To:References:From:Reply-To:Content-ID:
	Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc
	:Resent-Message-ID:List-Owner;
	bh=u0T2yZB/7/K3F2ri+MTI0f6nRWyAyUI5Q7XWM+GE/EQ=; b=fVHe+PxQN4Ky8WsOdcqq8WBhSr
	yoFn+7eiJsXlUToo9l7WzKWZBTa8rnwMAQdW2jtfhQgU+tFySwUsrn5C9JlIHG5KcJ+GJeMZRFe3b
	RXcJVUkn2S00WW0IWKqtFKTLFzJiQSwogJb8Sby6L+zsfVvoejslddiNMWX6q6pGoHtorH1pkF/s7
	0pNfEXy6yUtM7QexPa0Oy2iSg5TzM9Pi9Xf1OlgAQx/+Xngx7niejO7FvyTQL7IeidKYGwgSnO+Tg
	2K/Wfhae+nzZSo+z8oWPn/5gF0GAHd6k2wiMzarmasi33uQpgqj+CetaA9dwnlNqlid9878yTZb3D
	P5zjljIQ==;
Received: from localhost ([::1] helo=bombadil.infradead.org)
	by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux))
	id 1nrizg-008N6y-Fu; Thu, 19 May 2022 16:27:20 +0000
Received: from mail-vs1-xe2a.google.com ([2607:f8b0:4864:20::e2a])
 by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux))
 id 1nrizT-008N0l-6p
 for linux-mediatek@lists.infradead.org; Thu, 19 May 2022 16:27:09 +0000
Received: by mail-vs1-xe2a.google.com with SMTP id e19so5925933vsu.12
 for <linux-mediatek@lists.infradead.org>; Thu, 19 May 2022 09:27:01 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=baylibre-com.20210112.gappssmtp.com; s=20210112;
 h=from:user-agent:references:in-reply-to:mime-version:date:message-id
 :subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=Wdbve9pVOQfTCQ8qHIZq21Qqz+xsUOAaavE8ZtUJ9EblcqE54EBHnNXjuCxpwYGpor
 x4JgBbxZg77ZEAnCTT0kqlEZ/o/n17D8A0eVw1bfm2qy1z6ki8Rk1os7osw5nwYQtmDJ
 dWe7PFxstS5Yi5ZvCBNqRNNi55N7BlBUXf4UbrdAgQFpVWfVo7+Qk0uy/cnuWKCmsn1F
 B7HDdWZgo2ml5tOL/yDMoTEHgTVr2MA4oll4/SoapPprGkcSufMBge8WouN5+m6vPQ49
 A+4KWf4Mi66zOuv/wwToUPvT+BZC3ilqbBHUO6PSVKpTz/Hmp8QRDEB6XzvnVrGrAt6Y
 o6eg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20210112;
 h=x-gm-message-state:from:user-agent:references:in-reply-to
 :mime-version:date:message-id:subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=GMkYJ570GgkqcUjDSIA+EMBeH/61glf8o2cBkz0VZqNR0oHcoKmwhEHgWNtKmVXeFN
 hwJjPZZwz9ka6+mcOLEl6zt/ONnIfBjKKMKCzF9VYgqSDx+35MW4oWCtJWJo1OBSte1k
 gALEnjy/L+y8jhrl1U8A5JAvRGwC7St/2kwq7r8Xc4kOm1HQVXHkkqmay2tPPSEJ71hQ
 0xNKfn6jqB8O4UZVyzCqs40P1rL03I9q59F/hJXATOl9iHbJDjhyNy6K0evS0/fCYgDB
 c92cmsxE1sS3DsOWLrYy8QaOEP+eP5BTQMeolbqV86SIX6SvmioVvp01EigNHBs+q1zC
 NZKA==
X-Gm-Message-State: AOAM530ARuci6qklAEdaRBJr6NSy+MdreMrmcnodqTOajLBdMosBWznZ
 +GHMncu3Cz1T+mnewP08ble8Bmbk3CukheY1EOsJpw==
X-Google-Smtp-Source: ABdhPJx+NNr1h6VgTuwMD1Bhi8EcRUBgWUvE0krCGgcnnrPzo4cs17ylt6Dfy/iIo/Qh4cdkDItrCl7KQ2kO/HmmW/M=
X-Received: by 2002:a67:d516:0:b0:335:c0b5:1e5d with SMTP id
 l22-20020a67d516000000b00335c0b51e5dmr2517279vsj.13.1652977620240; Thu, 19
 May 2022 09:27:00 -0700 (PDT)
Received: from 753933720722 named unknown by gmailapi.google.com with
 HTTPREST; Thu, 19 May 2022 09:26:59 -0700
From: Guillaume Ranquet <granquet@baylibre.com>
User-Agent: meli 0.7.2
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
 <20220512074446.ihilbbnbuwesxbbg@houat>
In-Reply-To: <20220512074446.ihilbbnbuwesxbbg@houat>
MIME-Version: 1.0
Date: Thu, 19 May 2022 09:26:59 -0700
Message-ID: <CABnWg9tHRc_7VZ1z6NeMEDfQvxam_xOimBnM=hzBUqkhzPOSCA@mail.gmail.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
To: Maxime Ripard <maxime@cerno.tech>
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>
X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
X-CRM114-CacheID: sfid-20220519_092707_276055_911D06F4 
X-CRM114-Status: GOOD (  32.51  )
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

On Thu, 12 May 2022 09:44, Maxime Ripard <maxime@cerno.tech> wrote:
>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?
>

Ok, I'll migrate to dt then. I didn't realize platform_data were depreciated.

Angelo wants the MAX_LINRATE and MAX_LANES defines to be configurable.
I imagined platform_data would be more appropriate as (per my understanding) the
limitation is associated with a specific SoC.

>> >> +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.
>

After discussing a while with Mediatek, it appears that hot plug detection
needs to be handled even for eDP... (which is always connected).
So I'll revert to the split I made earlier in v8 where hot plug detection was
part of the eDP patch the addition of the External display port was a
trivial patch [1].

>> >> +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?
>

Thx for looking into this for me.
Not sure runtime_pm works here as it would only refcount if compiled
with CONFIG_PM?
I'd rather use the enabled field as a refcounter instead of a boolean.

Unless I'm totally missing your point?

Thx,
Guillaume.

>> >> +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


[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.18563-17-granquet@baylibre.com/

_______________________________________________
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 099D2C433EF
	for <dri-devel@archiver.kernel.org>; Thu, 19 May 2022 16:27:02 +0000 (UTC)
Received: from gabe.freedesktop.org (localhost [127.0.0.1])
	by gabe.freedesktop.org (Postfix) with ESMTP id 34ABF10F1BF;
	Thu, 19 May 2022 16:27:02 +0000 (UTC)
Received: from mail-vs1-xe2d.google.com (mail-vs1-xe2d.google.com
 [IPv6:2607:f8b0:4864:20::e2d])
 by gabe.freedesktop.org (Postfix) with ESMTPS id 3425110F1BF
 for <dri-devel@lists.freedesktop.org>; Thu, 19 May 2022 16:27:01 +0000 (UTC)
Received: by mail-vs1-xe2d.google.com with SMTP id o2so5713770vsd.13
 for <dri-devel@lists.freedesktop.org>; Thu, 19 May 2022 09:27:01 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=baylibre-com.20210112.gappssmtp.com; s=20210112;
 h=from:user-agent:references:in-reply-to:mime-version:date:message-id
 :subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=Wdbve9pVOQfTCQ8qHIZq21Qqz+xsUOAaavE8ZtUJ9EblcqE54EBHnNXjuCxpwYGpor
 x4JgBbxZg77ZEAnCTT0kqlEZ/o/n17D8A0eVw1bfm2qy1z6ki8Rk1os7osw5nwYQtmDJ
 dWe7PFxstS5Yi5ZvCBNqRNNi55N7BlBUXf4UbrdAgQFpVWfVo7+Qk0uy/cnuWKCmsn1F
 B7HDdWZgo2ml5tOL/yDMoTEHgTVr2MA4oll4/SoapPprGkcSufMBge8WouN5+m6vPQ49
 A+4KWf4Mi66zOuv/wwToUPvT+BZC3ilqbBHUO6PSVKpTz/Hmp8QRDEB6XzvnVrGrAt6Y
 o6eg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20210112;
 h=x-gm-message-state:from:user-agent:references:in-reply-to
 :mime-version:date:message-id:subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=M6NLxb/5DwlRp+SCNx5rjfwTuCn2s3RTkTj12vZ8DsyArDqze6ef92CJZqd+jEyLDl
 TlN8pirCfb3YnbB9Tmaqlk6ZaMusk7b7gybyk2ArXiB/9cLfpPf7W/QKQjxAfn651W1p
 3wjq0ic652Eo6oNWfjLYIm5MT2Cd5zr1F8K2as6oQlKteL0ORgbpJVwZpO1LO2Kn/d6Y
 cXCQTElUpn8BjD6Hmxkwi95MAQk4TFZuaUwIS855f9UFOSm6mcqelfcybpQEoqR5Y+Zy
 hZfmngIoe7UQzXle1SQs8GuLtiZ42899dppTk+bm4izWLU0NSryFTMjntzEjyrBwM18g
 cuAg==
X-Gm-Message-State: AOAM531VEXuqbV28Rlk1ER6NmwhoPcyA0QDHnRij/DXtheNSsm4d2DpF
 LJ/Q4m70bp+DzFbkHqdyUrqv9ikc7aGClNhWYxqseA==
X-Google-Smtp-Source: ABdhPJx+NNr1h6VgTuwMD1Bhi8EcRUBgWUvE0krCGgcnnrPzo4cs17ylt6Dfy/iIo/Qh4cdkDItrCl7KQ2kO/HmmW/M=
X-Received: by 2002:a67:d516:0:b0:335:c0b5:1e5d with SMTP id
 l22-20020a67d516000000b00335c0b51e5dmr2517279vsj.13.1652977620240; Thu, 19
 May 2022 09:27:00 -0700 (PDT)
Received: from 753933720722 named unknown by gmailapi.google.com with
 HTTPREST; Thu, 19 May 2022 09:26:59 -0700
From: Guillaume Ranquet <granquet@baylibre.com>
User-Agent: meli 0.7.2
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
 <20220512074446.ihilbbnbuwesxbbg@houat>
In-Reply-To: <20220512074446.ihilbbnbuwesxbbg@houat>
MIME-Version: 1.0
Date: Thu, 19 May 2022 09:26:59 -0700
Message-ID: <CABnWg9tHRc_7VZ1z6NeMEDfQvxam_xOimBnM=hzBUqkhzPOSCA@mail.gmail.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
To: Maxime Ripard <maxime@cerno.tech>
Content-Type: text/plain; charset="UTF-8"
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>

On Thu, 12 May 2022 09:44, Maxime Ripard <maxime@cerno.tech> wrote:
>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?
>

Ok, I'll migrate to dt then. I didn't realize platform_data were depreciated.

Angelo wants the MAX_LINRATE and MAX_LANES defines to be configurable.
I imagined platform_data would be more appropriate as (per my understanding) the
limitation is associated with a specific SoC.

>> >> +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.
>

After discussing a while with Mediatek, it appears that hot plug detection
needs to be handled even for eDP... (which is always connected).
So I'll revert to the split I made earlier in v8 where hot plug detection was
part of the eDP patch the addition of the External display port was a
trivial patch [1].

>> >> +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?
>

Thx for looking into this for me.
Not sure runtime_pm works here as it would only refcount if compiled
with CONFIG_PM?
I'd rather use the enabled field as a refcounter instead of a boolean.

Unless I'm totally missing your point?

Thx,
Guillaume.

>> >> +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


[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.18563-17-granquet@baylibre.com/

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 C1F91C433EF
	for <linux-phy@archiver.kernel.org>; Thu, 19 May 2022 16:27:21 +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:Cc:To:Subject:Message-ID:Date:
	MIME-Version:In-Reply-To:References:From:Reply-To:Content-ID:
	Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc
	:Resent-Message-ID:List-Owner;
	bh=+uQP2EvN0ZXHOp5kbf0dwLAmRiNVc+mIX9YxNg5gbvI=; b=i5qNNb3vfM+A9A7GNgk1HcTcma
	baWwVaHySJQppr7sNvcBAD9X0vCmG6OucCsroQUCh1eBDA16EK3IQSv5eWQWcxC/YOMIXBOWTxR1o
	IdQzI9luL8TRGH3tCikIhnNUWST1b0r+EiPbxwUKUkS8EXDpEW6y5TYUqR/rOl94y50N6XzmXbdcZ
	yC1xjNIgXC3M+1FSITiPvtvUTvtLOmAUlB08HOdUlMwdkghKgAzFeV8ZUADbuor9R0qa7Pu+6u/xc
	gWeGbZQ4Vks+pKjetd67K1KhmD9jYFgLZvJ1h+jfFihI6BPKcITn9rpCROwrY90w1GkmDADEDh4n3
	S5VYtMZA==;
Received: from localhost ([::1] helo=bombadil.infradead.org)
	by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux))
	id 1nrizh-008N79-5B; Thu, 19 May 2022 16:27:21 +0000
Received: from mail-vs1-xe2b.google.com ([2607:f8b0:4864:20::e2b])
 by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux))
 id 1nrizW-008N0m-OX
 for linux-phy@lists.infradead.org; Thu, 19 May 2022 16:27:12 +0000
Received: by mail-vs1-xe2b.google.com with SMTP id e19so5925935vsu.12
 for <linux-phy@lists.infradead.org>; Thu, 19 May 2022 09:27:01 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=baylibre-com.20210112.gappssmtp.com; s=20210112;
 h=from:user-agent:references:in-reply-to:mime-version:date:message-id
 :subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=Wdbve9pVOQfTCQ8qHIZq21Qqz+xsUOAaavE8ZtUJ9EblcqE54EBHnNXjuCxpwYGpor
 x4JgBbxZg77ZEAnCTT0kqlEZ/o/n17D8A0eVw1bfm2qy1z6ki8Rk1os7osw5nwYQtmDJ
 dWe7PFxstS5Yi5ZvCBNqRNNi55N7BlBUXf4UbrdAgQFpVWfVo7+Qk0uy/cnuWKCmsn1F
 B7HDdWZgo2ml5tOL/yDMoTEHgTVr2MA4oll4/SoapPprGkcSufMBge8WouN5+m6vPQ49
 A+4KWf4Mi66zOuv/wwToUPvT+BZC3ilqbBHUO6PSVKpTz/Hmp8QRDEB6XzvnVrGrAt6Y
 o6eg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20210112;
 h=x-gm-message-state:from:user-agent:references:in-reply-to
 :mime-version:date:message-id:subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=7azs9zwtJpQ50nKmxvOPvF7dd2+Udlo/lR1JV2jX9dtqgFmQ995rnCHFGTM7ghSvtW
 0Q9V0TB0EWJFNtxnCbge9HRvNmTiyVhN3x1THObHGbd/YuOo/XShyF+fyNB1YEYG4CU0
 YedmGYRcQp9abZBkBU6NOwRprlSwaLzahWY+A2uXgm9PcCJONTsleXV87AN4kRga2gwj
 lBsXCed5wH/QegxH71GbC29sqhlowW3AoQ8BkX5zklmId0An5Nb0Fj7nHkRfC3PPiph/
 uLHbVDFdM8t1vH171xvKm1HU/EJYjuTTFcf6h5LNiCF+/acCmPdTjepc7eTfBlCPlauT
 OOHQ==
X-Gm-Message-State: AOAM532gQpOgRoqUHYw+/AQtKCzxwlw9WS5PMsO/ldrrbqQHXH+15qQg
 SUVK8faS58D2QyB2DtSe90arrevMPu4LIBsCiFHJgQ==
X-Google-Smtp-Source: ABdhPJx+NNr1h6VgTuwMD1Bhi8EcRUBgWUvE0krCGgcnnrPzo4cs17ylt6Dfy/iIo/Qh4cdkDItrCl7KQ2kO/HmmW/M=
X-Received: by 2002:a67:d516:0:b0:335:c0b5:1e5d with SMTP id
 l22-20020a67d516000000b00335c0b51e5dmr2517279vsj.13.1652977620240; Thu, 19
 May 2022 09:27:00 -0700 (PDT)
Received: from 753933720722 named unknown by gmailapi.google.com with
 HTTPREST; Thu, 19 May 2022 09:26:59 -0700
From: Guillaume Ranquet <granquet@baylibre.com>
User-Agent: meli 0.7.2
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
 <20220512074446.ihilbbnbuwesxbbg@houat>
In-Reply-To: <20220512074446.ihilbbnbuwesxbbg@houat>
MIME-Version: 1.0
Date: Thu, 19 May 2022 09:26:59 -0700
Message-ID: <CABnWg9tHRc_7VZ1z6NeMEDfQvxam_xOimBnM=hzBUqkhzPOSCA@mail.gmail.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
To: Maxime Ripard <maxime@cerno.tech>
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>
X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
X-CRM114-CacheID: sfid-20220519_092710_824665_8D4BD698 
X-CRM114-Status: GOOD (  32.31  )
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

On Thu, 12 May 2022 09:44, Maxime Ripard <maxime@cerno.tech> wrote:
>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?
>

Ok, I'll migrate to dt then. I didn't realize platform_data were depreciated.

Angelo wants the MAX_LINRATE and MAX_LANES defines to be configurable.
I imagined platform_data would be more appropriate as (per my understanding) the
limitation is associated with a specific SoC.

>> >> +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.
>

After discussing a while with Mediatek, it appears that hot plug detection
needs to be handled even for eDP... (which is always connected).
So I'll revert to the split I made earlier in v8 where hot plug detection was
part of the eDP patch the addition of the External display port was a
trivial patch [1].

>> >> +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?
>

Thx for looking into this for me.
Not sure runtime_pm works here as it would only refcount if compiled
with CONFIG_PM?
I'd rather use the enabled field as a refcounter instead of a boolean.

Unless I'm totally missing your point?

Thx,
Guillaume.

>> >> +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


[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.18563-17-granquet@baylibre.com/

-- 
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 904C6C433EF
	for <linux-arm-kernel@archiver.kernel.org>; Thu, 19 May 2022 16:28:23 +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:Cc:To:Subject:Message-ID:Date:
	MIME-Version:In-Reply-To:References:From:Reply-To:Content-ID:
	Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc
	:Resent-Message-ID:List-Owner;
	bh=dReACWyvpoUINbjDQiXGrjjmtAlMsKKEVeIluMWDbr0=; b=FFTnar9afTC5ZyiCwOiGKikZpJ
	aFYxLbnRj1lK+zAPvvSJ8Khygs/D7R1T4ODC1NlqnZdQnqsmVwxU7heWByIxUAmP1CFefVhE9rPwz
	VQ/bJQIiYHgfZnbKCPt+Pp7dLpsCjcL5dGnYOkJrJ3RwlpeEOMZWFrwkXOjFbdt2yugESI35NREqx
	xZlJtfzyeJjlld8Wtsi1f+nNZKcIPlPo9VOvr7x4pR6livia2wSs4oEk+OLb/DzEzd38GVdJyiLSJ
	hGfNsVFWEtZq5a4hajZXUEC/TnIxabDbK3tsRK8qN8HqJwMD0YJwhxO4XdTUqgWPyUAPZ0SHZa/mF
	iIPAy9Wg==;
Received: from localhost ([::1] helo=bombadil.infradead.org)
	by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux))
	id 1nrizX-008N3p-Tz; Thu, 19 May 2022 16:27:12 +0000
Received: from mail-vs1-xe29.google.com ([2607:f8b0:4864:20::e29])
 by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux))
 id 1nrizT-008N0n-6t
 for linux-arm-kernel@lists.infradead.org; Thu, 19 May 2022 16:27:09 +0000
Received: by mail-vs1-xe29.google.com with SMTP id o2so5713773vsd.13
 for <linux-arm-kernel@lists.infradead.org>;
 Thu, 19 May 2022 09:27:01 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=baylibre-com.20210112.gappssmtp.com; s=20210112;
 h=from:user-agent:references:in-reply-to:mime-version:date:message-id
 :subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=Wdbve9pVOQfTCQ8qHIZq21Qqz+xsUOAaavE8ZtUJ9EblcqE54EBHnNXjuCxpwYGpor
 x4JgBbxZg77ZEAnCTT0kqlEZ/o/n17D8A0eVw1bfm2qy1z6ki8Rk1os7osw5nwYQtmDJ
 dWe7PFxstS5Yi5ZvCBNqRNNi55N7BlBUXf4UbrdAgQFpVWfVo7+Qk0uy/cnuWKCmsn1F
 B7HDdWZgo2ml5tOL/yDMoTEHgTVr2MA4oll4/SoapPprGkcSufMBge8WouN5+m6vPQ49
 A+4KWf4Mi66zOuv/wwToUPvT+BZC3ilqbBHUO6PSVKpTz/Hmp8QRDEB6XzvnVrGrAt6Y
 o6eg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20210112;
 h=x-gm-message-state:from:user-agent:references:in-reply-to
 :mime-version:date:message-id:subject:to:cc;
 bh=D3BsvTGZmL9aXn8JC2DlxbBhxOaQqmVN5f5pOf4WE4o=;
 b=BToUqxWtj19S4FzwD1Ux1zCdgiSy6u/a3PI+dNfi96T2xqjeq7MHfAC3lBiN4L+7pL
 ano8JkA00nFcf5g8gemwdn+3wqp00euXejfKarUWBXPhLsVVeGeFAZHWH0sq61zEJO5x
 sAPDEoS6xxlVSdcwe2CjfadX/6uhW2PGIyWU7b1cgq1zokxMOTyJlQ61dEH6YY3tiUcn
 cmkkg1hs33ov9B0FPZ900yo2tmevvFRbNxKeC/Zu5dgCOhxb5gmgBhVsAqUcVXJutB6O
 i+/Wk/TPDX6rcQyu4Wlo8GEdDhzXjSpxVgvXebRJT1iV0px0I4Q6SykdZW6IrKGQFvQ9
 XXTA==
X-Gm-Message-State: AOAM530+VrNp7Gov3yZY0ZAmZEZM57yc5LaVn9i4vyZqnIH1SuVyRBb1
 sJlnYnoXkYv55lvp+egTAS85kFl++xnoF/U/jTHUwA==
X-Google-Smtp-Source: ABdhPJx+NNr1h6VgTuwMD1Bhi8EcRUBgWUvE0krCGgcnnrPzo4cs17ylt6Dfy/iIo/Qh4cdkDItrCl7KQ2kO/HmmW/M=
X-Received: by 2002:a67:d516:0:b0:335:c0b5:1e5d with SMTP id
 l22-20020a67d516000000b00335c0b51e5dmr2517279vsj.13.1652977620240; Thu, 19
 May 2022 09:27:00 -0700 (PDT)
Received: from 753933720722 named unknown by gmailapi.google.com with
 HTTPREST; Thu, 19 May 2022 09:26:59 -0700
From: Guillaume Ranquet <granquet@baylibre.com>
User-Agent: meli 0.7.2
References: <20220327223927.20848-1-granquet@baylibre.com>
 <20220327223927.20848-19-granquet@baylibre.com>
 <20220429083933.q3w75q3zuyyuvo4w@houat>
 <CABnWg9tzhZjrdKT4chkDrY-uH8BMUoxyNLUSwfuG6Sv1J+8ddg@mail.gmail.com>
 <20220512074446.ihilbbnbuwesxbbg@houat>
In-Reply-To: <20220512074446.ihilbbnbuwesxbbg@houat>
MIME-Version: 1.0
Date: Thu, 19 May 2022 09:26:59 -0700
Message-ID: <CABnWg9tHRc_7VZ1z6NeMEDfQvxam_xOimBnM=hzBUqkhzPOSCA@mail.gmail.com>
Subject: Re: [PATCH v9 18/22] drm/mediatek: Add mt8195 Embedded DisplayPort
 driver
To: Maxime Ripard <maxime@cerno.tech>
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>
X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 
X-CRM114-CacheID: sfid-20220519_092707_296579_2B5A8E52 
X-CRM114-Status: GOOD (  33.78  )
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

On Thu, 12 May 2022 09:44, Maxime Ripard <maxime@cerno.tech> wrote:
>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?
>

Ok, I'll migrate to dt then. I didn't realize platform_data were depreciated.

Angelo wants the MAX_LINRATE and MAX_LANES defines to be configurable.
I imagined platform_data would be more appropriate as (per my understanding) the
limitation is associated with a specific SoC.

>> >> +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.
>

After discussing a while with Mediatek, it appears that hot plug detection
needs to be handled even for eDP... (which is always connected).
So I'll revert to the split I made earlier in v8 where hot plug detection was
part of the eDP patch the addition of the External display port was a
trivial patch [1].

>> >> +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?
>

Thx for looking into this for me.
Not sure runtime_pm works here as it would only refcount if compiled
with CONFIG_PM?
I'd rather use the enabled field as a refcounter instead of a boolean.

Unless I'm totally missing your point?

Thx,
Guillaume.

>> >> +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


[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220218145437.18563-17-granquet@baylibre.com/

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