Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Cheng-yi Chiang <cychiang@chromium.org>
To: Hans Verkuil <hverkuil@xs4all.nl>
Cc: "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER
	MANAGEM..." <alsa-devel@alsa-project.org>,
	Heiko Stuebner <heiko@sntech.de>,
	Liam Girdwood <lgirdwood@gmail.com>,
	David Airlie <airlied@linux.ie>,
	dri-devel@lists.freedesktop.org, Takashi Iwai <tiwai@suse.com>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
	linux-rockchip@lists.infradead.org,
	Dylan Reid <dgreid@chromium.org>,
	linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	tzungbi@chromium.org, Daniel Vetter <daniel@ffwll.ch>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>,
	Russell King <rmk+kernel@armlinux.org.uk>,
	Rob Herring <robh+dt@kernel.org>,
	Jaroslav Kysela <perex@perex.cz>,
	linux-arm-kernel@lists.infradead.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Doug Anderson <dianders@chromium.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Mark Brown <broonie@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Dariusz Marcinkiewicz <darekm@google.com>
Subject: Re: [PATCH 1/7] video: add HDMI state notifier support
Date: Tue, 18 Jun 2019 19:54:56 +0800
Message-ID: <CAFv8NwK3kLkWqeTdCaC9mOf7z3uv0qXOVjjfhDtogoVb3CN8dw@mail.gmail.com> (raw)
In-Reply-To: <126de7f5-c92a-9e5c-cd36-5484f43f0f6b@xs4all.nl>

On Tue, Jun 11, 2019 at 9:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 6/11/19 2:10 PM, Cheng-yi Chiang wrote:
> > On Tue, Jun 4, 2019 at 3:24 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>
> >> On Tue, Jun 04, 2019 at 10:32:50AM +0800, Cheng-yi Chiang wrote:
> >>> On Mon, Jun 3, 2019 at 4:09 PM Daniel Vetter <daniel@ffwll.ch> wrote:
> >>>>
> >>>> On Mon, Jun 03, 2019 at 09:45:49AM +0200, Hans Verkuil wrote:
> >>>>> On 6/3/19 6:32 AM, Cheng-Yi Chiang wrote:
> >>>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
> >>>>>>
> >>>>>> Add support for HDMI hotplug and EDID notifiers, which is used to convey
> >>>>>> information from HDMI drivers to their CEC and audio counterparts.
> >>>>>>
> >>>>>> Based on an earlier version from Russell King:
> >>>>>>
> >>>>>> https://patchwork.kernel.org/patch/9277043/
> >>>>>>
> >>>>>> The hdmi_notifier is a reference counted object containing the HDMI state
> >>>>>> of an HDMI device.
> >>>>>>
> >>>>>> When a new notifier is registered the current state will be reported to
> >>>>>> that notifier at registration time.
> >>>>>>
> >>>>>> Based on Hans Verkuil's patch:
> >>>>>>
> >>>>>> https://patchwork.kernel.org/patch/9472521/
> >>>>>
> >>>>> Erm, you are aware that this patch morphed into a CEC-specific notifier
> >>>>> found in drivers/media/cec/cec-notifier.c?
> >>>>>
> >>>>> I don't think it makes sense to have two notifier implementations in the kernel.
> >>>>> The original intention was to have the notifier deal with both CEC and ASoC
> >>>>> notifications, but there was not enough interest for the ASoC bits at the time
> >>>>> and it was dropped.
> >>>>>
> >>>>> I am planning changes to the cec-notifier API, I hope to work on that this
> >>>>> week. I'll CC you when I post those. Those might be a good starting point
> >>>>> to convert the cec-notifier to an hdmi-notifier as was originally intended.
> >>>>>
> >>>>> I've added your colleague Dariusz Marcinkiewicz to the CC list since he's been
> >>>>> working on some nice cec-notifier improvements as well.
> >>>>
> >>>> We also have some interfaces for drm/alsa interactions around hdmi
> >>>> already in drm/drm_audio_component.h, but it's not used by anything
> >>>> outside of i915. Imo we should extend that, not reinvent a new wheel.
> >>>>
> >>> Hi Daniel,
> >>> Thank you for the pointer. Looking at the ops, it seems that it is
> >>> specific to HDA.
> >>> I am not familiar with drm and HDA. I am not sure how applicable it
> >>> would be to report jack status to ASoC.
> >>> There is a use case in sound/soc/codecs/hdac_hdmi.c though so it
> >>> should be possible.
> >>
> >> Currently hda is the only user, but the idea was to make it more generic.
> >> Jack status in alsa is what drm calls connector status btw.
> >>
> >> So if we can take that as a baseline and extend it (probably needs some
> >> registration boilerplate and helpers to look up the right endpoint using
> >> of/dt for soc systems, we use component.c in i915/hda for this), that
> >> would be great I think.
> >>
> >>>> Another note: notifiers considered evil, imo. Gets the job done for one
> >>>> case, as soon as you have multiple devices and need to make sure you get
> >>>> the update for the right one it all comes crashing down. Please create an
> >>>> api which registers for updates from a specific device only, plus
> >>>> something that has real callbacks (like the drm_audio_component.h thing we
> >>>> started already).
> >>>
> >>> To clarify a bit, this hdmi-notifier indeed supports updating from a
> >>> specific device only.
> >>> hdmi_notifier_get takes a device and return the notifier.
> >>
> >> Hm I missed that, I thought it's global, so one of my usual notifier
> >> concerns addressed.
> >>
> >>> It seems that a major difference between drm_audio_components and
> >>> hdmi-notifier is that
> >>> drm_audio_components defines all supported ops in drm_audio_component_audio_ops.
> >>> On the other hand, hdmi-notifier passes different events using an enum
> >>> like HDMI_CONNECTED and let listener handle different events.
> >>> In this regard I agree with you that drm_audio_component is cleaner.
> >>> Anyway, I will look into it a bit more and see how it works.
> >>
> >> Yeah I think if we could combine the approach, i.e. notifier side for
> >> registration, some _ops structure for the actual notifications, then
> >> there's a solid interface. I just really don't like the opaque void *
> >> interface notifier provides, it encourages abuse way too much.
> >>
> >> Ofc the registration side would then no longer be based on the notifier
> >> datastructure, list_head (like cec-notifier.c) of registeres devices with
> >> their _ops structure should be enough.
> >> -Daniel
> >
> > Hi Daniel,
> > Yes, I agree the above statement that we should have a more solid interface.
> >
> > Hi Hans,
> > I am not sure if I missed the patch.
>
> You haven't :-)
>
> > Do you have a estimated timeline for new cec-notifier interface you
> > are working on?
>
> I've started work on this, but I to find at least one more full day
> to finish it.
>
> Current status is here:
>
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=cec-conn
>
> What needs to be changed is that cec-notifier API is split into functions
> used by HDMI connectors to register/unregister themselves and functions
> used by CEC adapters (and possibly ALSA devices, if you decide to reuse
> this API) to do the same.
>
> Right now the same functions (notifier_get/put) are used by both, but that
> doesn't scale if we want to have multiple notifiers connected to the same
> HDMI drm_connector.
>
> Now, this is done, but the next step is on the CEC side where you need
> proper (un)register callbacks that are called when the HDMI connector is
> removed. That I still have to implement.
>
> I'd like to finish this some time next week, but I can't promise anything.
>
Hi Hans,
Sorry for the late reply. I have also spent some time digesting your
patch series.
I think your change makes sense for CEC.

> > It seems that your PoC patch needs Dariusz's patch to work.
> > I would like to seek your advice on whether I can proceed without your
> > patch and Dariusz's patch.
>
> If you want to reuse the cec-notifier framework for this as well, then you
> need to wait. It is currently not possible to have more than one notifier
> for an HDMI connector, and you'll need that.
>
> >
> > I looked through the patch from Dariusz
> >
> > https://lkml.org/lkml/2019/5/21/389
> >
> > , and saw that you were thinking whether we should use cec-notifier
> > for both HDMI and CEC.
> >
> > https://lkml.org/lkml/2019/5/24/298
> >
> > Could you please let me know your latest thought on whether we should
> > reuse cec-notifier?
>
> I don't know :-) I have no experience with ALSA, so I can't tell if generalizing
> the CEC notifier is the right approach, or if another approach would be better.
>
> The current design is not quite good enough for what you (or Dariusz, for that
> matter) want. What I am working towards is this:
>
> When a new HDMI connector appears, it calls cec_notifier_conn_register (and
> _unregister when it is removed).
>
> When a new CEC adapter appears, it calls cec_notifier_cec_adap_(un)register.
> The plan is that eventually there may be more than on CEC adapter registered
> to the same HDMI connector.
>
> The cec notifier framework will detect when HDMI connectors or CEC adapters
> appear/disappear and call the (un)register callbacks of the CEC adapter(s)
> accordingly.
>
> And what works for CEC adapters, should also work for ALSA devices.
>
> Obviously, if this framework is extended to ALSA devices, then it needs to
> be renamed.
>

Thank you for the explanation.
I can imagine how this path works.
But as I replied in the previous mail to Daniel, after some
investigation, I would like to try drm_audio_component approach and
see how that goes.
If that does not work out well, I will follow this approach.
Thanks again!

> > I agree with you that I should not proceed with hdmi-notifier. Reasons include:
> > 1. Method like cec_notifier_parse_hdmi_phandle can be reused. It is
> > error prone to memory leak if it is implemented by user, like the
> > patch in hdmi-codec.c in this series did not handle the ref count.
> > 2. cec-notifier has a simpler implementation of register / unregister
> > because there is no call chain. I am not aware of the need for
> > hdmi-notifier to support a chain of callbacks. So I think that call
> > chain support can be removed.
> >
> > If I go ahead and add a new interface to register ops to handle
> > connector status report from cec-notifer, based on current
> > cec-notifier, do you think that would work ?
>
> No. The cec-notifier doesn't support multiple notifiers connected to
> the same HDMI connector device. That's the main limitation that needs
> to be lifted first.
ACK
>
> > I think it might work if I add another cec_notifier object inside
> > dw-hdmi.c, but only for HDMI jack reporting, not for CEC related
> > reporting.
>
> That won't work.
>
> For testing you can reuse the current cec-notifier, but now for alsa.
> Disable the CEC support so it won't be used for CEC, then hook it up
> to the alsa device. That should be good enough to do a proof-of-concept.
>
ACK
> >
> > And after some investigation, I realize that my requirement is even
> > simpler. I don't need hdmi_event_new_edid and hdmi_event_new_eld in my
> > use case.
> > I just need to report the connector status from synopsys/dw-hdmi.c to
> > codecs/hdmi-codec.c for codec driver to update the jack status.
> > Do you think I can proceed in this direction ? Or do you prefer I wait
> > for a while and work on it based on your new patch.
>
> You can hack around as described above for testing the idea. If you
> really want to use it, then you'll have to wait.
>
> >
> > Thanks a lot!
>
> No problem.
>
> Regards,
>
>         Hans

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

  reply index

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-03  4:32 [PATCH 0/7] Add HDMI audio support on RK3288 veyron board Cheng-Yi Chiang
2019-06-03  4:32 ` [PATCH 1/7] video: add HDMI state notifier support Cheng-Yi Chiang
2019-06-03  7:45   ` Hans Verkuil
2019-06-03  8:09     ` Daniel Vetter
2019-06-03  9:05       ` Hans Verkuil
2019-06-04  7:19         ` Daniel Vetter
2019-06-04  2:32       ` Cheng-yi Chiang
2019-06-04  7:24         ` Daniel Vetter
2019-06-11 12:10           ` Cheng-yi Chiang
2019-06-11 12:34             ` Daniel Vetter
2019-06-18 11:48               ` Cheng-yi Chiang
2019-06-18 12:12                 ` Daniel Vetter
2019-06-19 11:48                   ` Cheng-yi Chiang
2019-06-20  9:25                     ` Daniel Vetter
2019-06-20 13:23                       ` Cheng-yi Chiang
2019-06-20 21:12                         ` Daniel Vetter
2019-06-21  9:39                           ` Cheng-yi Chiang
2019-06-11 13:10             ` Hans Verkuil
2019-06-18 11:54               ` Cheng-yi Chiang [this message]
2019-06-04  1:48     ` Cheng-yi Chiang
2019-06-03  4:32 ` [PATCH 2/7] ASoC: hdmi-codec: use HDMI state notifier to add jack support Cheng-Yi Chiang
2019-06-03  8:03   ` [alsa-devel] " Amadeusz Sławiński
2019-06-03  8:53     ` Cheng-yi Chiang
2019-06-03  4:32 ` [PATCH 3/7] drm/bridge/synopsys: dw-hdmi: Add HDMI notifier support Cheng-Yi Chiang
2019-06-03  4:32 ` [PATCH 4/7] ASoC: rockchip_max98090: Add dai_link for HDMI Cheng-Yi Chiang
2019-06-03  4:32 ` [PATCH 5/7] ASoC: rockchip: rockchip-max98090: Add node " Cheng-Yi Chiang
2019-07-09 20:01   ` Rob Herring
2019-07-10  4:11     ` Cheng-yi Chiang
2019-06-03  4:32 ` [PATCH 6/7] ASoC: rockchip_max98090: Add HDMI jack support Cheng-Yi Chiang
2019-06-03  4:32 ` [PATCH 7/7] ARM: dts: rockchip: Specify HDMI node to sound card node Cheng-Yi Chiang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFv8NwK3kLkWqeTdCaC9mOf7z3uv0qXOVjjfhDtogoVb3CN8dw@mail.gmail.com \
    --to=cychiang@chromium.org \
    --cc=Laurent.pinchart@ideasonboard.com \
    --cc=a.hajda@samsung.com \
    --cc=airlied@linux.ie \
    --cc=alsa-devel@alsa-project.org \
    --cc=b.zolnierkie@samsung.com \
    --cc=broonie@kernel.org \
    --cc=daniel@ffwll.ch \
    --cc=darekm@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dgreid@chromium.org \
    --cc=dianders@chromium.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=heiko@sntech.de \
    --cc=hverkuil@xs4all.nl \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=p.zabel@pengutronix.de \
    --cc=perex@perex.cz \
    --cc=rmk+kernel@armlinux.org.uk \
    --cc=robh+dt@kernel.org \
    --cc=tiwai@suse.com \
    --cc=tzungbi@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org infradead-linux-arm-kernel@archiver.kernel.org
	public-inbox-index linux-arm-kernel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox