Message-ID: <126de7f5-c92a-9e5c-cd36-5484f43f0f6b@xs4all.nl>
Date:   Tue, 11 Jun 2019 15:10:47 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Cheng-yi Chiang <cychiang@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Takashi Iwai <tiwai@...e.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        David Airlie <airlied@...ux.ie>,
        Rob Herring <robh+dt@...nel.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Doug Anderson <dianders@...omium.org>,
        Dylan Reid <dgreid@...omium.org>, tzungbi@...omium.org,
        linux-media@...r.kernel.org,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@...a-project.org>, dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org, devicetree@...r.kernel.org,
        Dariusz Marcinkiewicz <darekm@...gle.com>
Cc:     Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH 1/7] video: add HDMI state notifier support

On 6/11/19 2:10 PM, Cheng-yi Chiang wrote:
> On Tue, Jun 4, 2019 at 3:24 PM Daniel Vetter <daniel@...ll.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@...ll.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@...co.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.

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

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

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

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

Powered by blists - more mailing lists