From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id AFA8EC433FE for ; Fri, 4 Mar 2022 16:47:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240817AbiCDQs0 convert rfc822-to-8bit (ORCPT ); Fri, 4 Mar 2022 11:48:26 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43492 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239119AbiCDQsW (ORCPT ); Fri, 4 Mar 2022 11:48:22 -0500 Received: from aposti.net (aposti.net [89.234.176.197]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1AD5B39A; Fri, 4 Mar 2022 08:47:31 -0800 (PST) Date: Fri, 04 Mar 2022 16:47:19 +0000 From: Paul Cercueil Subject: Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll() To: Neil Armstrong Cc: "H. Nikolaus Schaller" , Paul Boddie , Andrzej Hajda , Maxime Ripard , Jonas Karlman , David Airlie , Robert Foss , linux-mips , dri-devel , linux-kernel , Kieran Bingham , Jernej Skrabec , Discussions about the Letux Kernel , Laurent Pinchart Message-Id: In-Reply-To: References: <983e9064-17ad-e646-f37d-ca9173ba0967@baylibre.com> <3E620AF4-402E-45EA-9D92-92EAEA9647F5@goldelico.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Neil, Le ven., mars 4 2022 at 14:30:46 +0100, Neil Armstrong a écrit : > Hi, > > On 03/03/2022 18:59, H. Nikolaus Schaller wrote: >> Hi Paul, Neil, >> >>> Am 03.03.2022 um 18:20 schrieb Paul Cercueil : >>> >>> Hi Nikolaus, >>> >>> [snip] >>> >>>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do >>>>> have access to the main drm_device in the ingenic_drm_bind() >>>>> function, so you can add it there (with a cleanup function >>>>> calling drm_kms_helper_poll_fini() registered with >>>>> drmm_add_action_or_reset()). >>>> Well, do you really want to mix HPD detection between connector, >>>> Synopsys bridge and Ingenic DRM core? These are independent... >>>> Or should be accessed only through the bridge chain pointers. >>>> IMHO we should keep separate functions separate. >>> >>> The drm_kms_helper_poll_init() just says "this DRM device may have >>> connectors that need to be polled" so it very well fits inside the >>> main driver, IMHO. >> >> As far as I understand, it has the side-effect to always set >> dev->mode_config.poll_enabled and >> schedule_delayed_work() for all devices. >> I am not sure if this is intended for arbitrary ingenic-drm devices. >> But you know better than me. >> >> >> Hm. But wait, I think I now finally remember why I have proposed it >> the way it is! >> It is always better to go back to requirements and find the least >> invasive solution. >> >> - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding >> printk()] >> - it is just that the udevd is only notified if poll_enabled = true >> (but no polling takes place!). >> >> An earlier version (v4) to fix this proposed to add an explicit call >> to drm_kms_helper_hotplug_event() >> in dw_hdmi_irq() but that was rejected a while ago because >> drm_helper_hpd_irq_event() will already call it: >> >> https://www.spinics.net/lists/dri-devel/msg316846.html >> >> Since this did not take into account that >> dev->mode_config.poll_enabled must be set true, I then proposed the >> enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi >> specialization. >> >> So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq() >> and that calls drm_helper_hpd_irq_event() >> but not drm_kms_helper_hotplug_event() and user-space is not getting >> aware. >> >> It is all a hack because we mix the dw-hdmi driver which originally >> did register its own connector >> with an explicit connector... >> >> In summary I now thing that the v4 patch is the simplest and least >> invasive solution. >> >> We neither have to introduce a dw_hdmi_enable_poll() function or >> call drm_kms_helper_poll_init() anywhere. >> >> It is just a single line to add to dw-hdmi. And neither touches >> ingenic-dw-hdmi nor ingenic-drm-drv. >> >> So let's go back to v4 version (just modify commit message to better >> describe why we have to call >> drm_kms_helper_hotplug_event() explicitly) and forget about >> alternatives. > > Please don't and add drm_kms_helper_poll_init() from the > ingenic-drm-drv.c like every other DRM driver. > > Adding drm_kms_helper_hotplug_event() in dw-hdmi will impact other > drivers using dw-hdmi but correctly > calling drm_kms_helper_poll_init(). From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace. I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c. Cheers, -Paul From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from 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 65912C433EF for ; Fri, 4 Mar 2022 16:47:33 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 91C4B1128D4; Fri, 4 Mar 2022 16:47:32 +0000 (UTC) Received: from aposti.net (aposti.net [89.234.176.197]) by gabe.freedesktop.org (Postfix) with ESMTPS id 67E841128D4 for ; Fri, 4 Mar 2022 16:47:31 +0000 (UTC) Date: Fri, 04 Mar 2022 16:47:19 +0000 From: Paul Cercueil Subject: Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll() To: Neil Armstrong Message-Id: In-Reply-To: References: <983e9064-17ad-e646-f37d-ca9173ba0967@baylibre.com> <3E620AF4-402E-45EA-9D92-92EAEA9647F5@goldelico.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1; format=flowed Content-Transfer-Encoding: quoted-printable X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paul Boddie , Jonas Karlman , David Airlie , "H. Nikolaus Schaller" , dri-devel , linux-mips , Robert Foss , linux-kernel , Kieran Bingham , Maxime Ripard , Andrzej Hajda , Discussions about the Letux Kernel , Jernej Skrabec , Laurent Pinchart Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" Hi Neil, Le ven., mars 4 2022 at 14:30:46 +0100, Neil Armstrong=20 a =E9crit : > Hi, >=20 > On 03/03/2022 18:59, H. Nikolaus Schaller wrote: >> Hi Paul, Neil, >>=20 >>> Am 03.03.2022 um 18:20 schrieb Paul Cercueil : >>>=20 >>> Hi Nikolaus, >>>=20 >>> [snip] >>>=20 >>>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do=20 >>>>> have access to the main drm_device in the ingenic_drm_bind()=20 >>>>> function, so you can add it there (with a cleanup function=20 >>>>> calling drm_kms_helper_poll_fini() registered with=20 >>>>> drmm_add_action_or_reset()). >>>> Well, do you really want to mix HPD detection between connector,=20 >>>> Synopsys bridge and Ingenic DRM core? These are independent... >>>> Or should be accessed only through the bridge chain pointers. >>>> IMHO we should keep separate functions separate. >>>=20 >>> The drm_kms_helper_poll_init() just says "this DRM device may have=20 >>> connectors that need to be polled" so it very well fits inside the=20 >>> main driver, IMHO. >>=20 >> As far as I understand, it has the side-effect to always set=20 >> dev->mode_config.poll_enabled and >> schedule_delayed_work() for all devices. >> I am not sure if this is intended for arbitrary ingenic-drm devices.=20 >> But you know better than me. >>=20 >>=20 >> Hm. But wait, I think I now finally remember why I have proposed it=20 >> the way it is! >> It is always better to go back to requirements and find the least=20 >> invasive solution. >>=20 >> - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding=20 >> printk()] >> - it is just that the udevd is only notified if poll_enabled =3D true=20 >> (but no polling takes place!). >>=20 >> An earlier version (v4) to fix this proposed to add an explicit call=20 >> to drm_kms_helper_hotplug_event() >> in dw_hdmi_irq() but that was rejected a while ago because=20 >> drm_helper_hpd_irq_event() will already call it: >>=20 >> https://www.spinics.net/lists/dri-devel/msg316846.html >>=20 >> Since this did not take into account that=20 >> dev->mode_config.poll_enabled must be set true, I then proposed the >> enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi=20 >> specialization. >>=20 >> So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq()=20 >> and that calls drm_helper_hpd_irq_event() >> but not drm_kms_helper_hotplug_event() and user-space is not getting=20 >> aware. >>=20 >> It is all a hack because we mix the dw-hdmi driver which originally=20 >> did register its own connector >> with an explicit connector... >>=20 >> In summary I now thing that the v4 patch is the simplest and least=20 >> invasive solution. >>=20 >> We neither have to introduce a dw_hdmi_enable_poll() function or=20 >> call drm_kms_helper_poll_init() anywhere. >>=20 >> It is just a single line to add to dw-hdmi. And neither touches=20 >> ingenic-dw-hdmi nor ingenic-drm-drv. >>=20 >> So let's go back to v4 version (just modify commit message to better=20 >> describe why we have to call >> drm_kms_helper_hotplug_event() explicitly) and forget about=20 >> alternatives. >=20 > Please don't and add drm_kms_helper_poll_init() from the=20 > ingenic-drm-drv.c like every other DRM driver. >=20 > Adding drm_kms_helper_hotplug_event() in dw-hdmi will impact other=20 > drivers using dw-hdmi but correctly > calling drm_kms_helper_poll_init(). From what I understood in Nikolaus' last message, HDMI hotplug is=20 actually correctly detected, so there's no need for polling. What is=20 missing is the call to drm_kms_helper_hotplug_event *somewhere*, so=20 that the information is correctly relayed to userspace. I think this issue can be fixed by calling=20 drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c. Cheers, -Paul