| Message ID | 1499337186-24429-1-git-send-email-narmstrong@baylibre.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
Hi Neil, On 06-07-2017 11:33, Neil Armstrong wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > Add CEC notifier support to the HDMI bridge driver, so that the CEC > part of the IP can receive its physical address. > > Tested-by: Neil Armstrong <narmstrong@baylibre.com> > Acked-by: Neil Armstrong <narmstrong@baylibre.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > Hi Archit, Hans, > > This is repost of Russell's patch from his dw-hdmi CEC patchset. > > Since his CEC implementation will be integrated in the bridge driver, > this notifier patch won't be re-posted. > > But the Amlogic Platform needs a notifier since it uses a standalone CEC > controller. > > Thanks, > Neil > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index ead1124..9c03846 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -33,6 +33,8 @@ > #include <uapi/linux/media-bus-format.h> > #include <uapi/linux/videodev2.h> > > +#include <media/cec-notifier.h> Don't you also have to "select CEC_NOTIFIER" in Kconfig? Or is it not needed anymore with the recent Kconfig changes of CEC? Best regards, Jose Miguel Abreu > + > #include "dw-hdmi.h" > #include "dw-hdmi-audio.h" > > @@ -175,6 +177,8 @@ struct dw_hdmi { > struct regmap *regm; > void (*enable_audio)(struct dw_hdmi *hdmi); > void (*disable_audio)(struct dw_hdmi *hdmi); > + > + struct cec_notifier *cec_notifier; > }; > > #define HDMI_IH_PHY_STAT0_RX_SENSE \ > @@ -1896,6 +1900,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) > hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); > hdmi->sink_has_audio = drm_detect_monitor_audio(edid); > drm_mode_connector_update_edid_property(connector, edid); > + cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, edid); > ret = drm_add_edid_modes(connector, edid); > /* Store the ELD */ > drm_edid_to_eld(connector, edid); > @@ -2077,6 +2082,10 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense) > dw_hdmi_update_phy_mask(hdmi); > } > mutex_unlock(&hdmi->mutex); > + > + if (!rx_sense && !hpd) > + cec_notifier_set_phys_addr(hdmi->cec_notifier, > + CEC_PHYS_ADDR_INVALID); > } > > void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense) > @@ -2376,6 +2385,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > if (ret) > goto err_iahb; > > + hdmi->cec_notifier = cec_notifier_get(dev); > + if (!hdmi->cec_notifier) { > + ret = -ENOMEM; > + goto err_iahb; > + } > + > /* > * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator > * N and cts values before enabling phy > @@ -2452,6 +2467,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > hdmi->ddc = NULL; > } > > + if (hdmi->cec_notifier) > + cec_notifier_put(hdmi->cec_notifier); > + > clk_disable_unprepare(hdmi->iahb_clk); > err_isfr: > clk_disable_unprepare(hdmi->isfr_clk);
On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote: > From: Russell King <rmk+kernel@armlinux.org.uk> > > Add CEC notifier support to the HDMI bridge driver, so that the CEC > part of the IP can receive its physical address. > > Tested-by: Neil Armstrong <narmstrong@baylibre.com> > Acked-by: Neil Armstrong <narmstrong@baylibre.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > Hi Archit, Hans, > > This is repost of Russell's patch from his dw-hdmi CEC patchset. > > Since his CEC implementation will be integrated in the bridge driver, > this notifier patch won't be re-posted. > > But the Amlogic Platform needs a notifier since it uses a standalone CEC > controller. Without this, the dw-hdmi CEC support will be totally useless anyway, and it's pointless to merge it without this patch.
On 07/06/2017 12:43 PM, Jose Abreu wrote: > Hi Neil, > > > On 06-07-2017 11:33, Neil Armstrong wrote: >> From: Russell King <rmk+kernel@armlinux.org.uk> >> >> Add CEC notifier support to the HDMI bridge driver, so that the CEC >> part of the IP can receive its physical address. >> >> Tested-by: Neil Armstrong <narmstrong@baylibre.com> >> Acked-by: Neil Armstrong <narmstrong@baylibre.com> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> Hi Archit, Hans, >> >> This is repost of Russell's patch from his dw-hdmi CEC patchset. >> >> Since his CEC implementation will be integrated in the bridge driver, >> this notifier patch won't be re-posted. >> >> But the Amlogic Platform needs a notifier since it uses a standalone CEC >> controller. >> >> Thanks, >> Neil >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> index ead1124..9c03846 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -33,6 +33,8 @@ >> #include <uapi/linux/media-bus-format.h> >> #include <uapi/linux/videodev2.h> >> >> +#include <media/cec-notifier.h> > > Don't you also have to "select CEC_NOTIFIER" in Kconfig? Or is it > not needed anymore with the recent Kconfig changes of CEC? Hi Jose, Seems no, since the cec functions are protected with : #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER) and replaced with dummy inline functions. Neil > > Best regards, > Jose Miguel Abreu > >> + >> #include "dw-hdmi.h" >> #include "dw-hdmi-audio.h" >> >> @@ -175,6 +177,8 @@ struct dw_hdmi { >> struct regmap *regm; >> void (*enable_audio)(struct dw_hdmi *hdmi); >> void (*disable_audio)(struct dw_hdmi *hdmi); >> + >> + struct cec_notifier *cec_notifier; >> }; >> >> #define HDMI_IH_PHY_STAT0_RX_SENSE \ >> @@ -1896,6 +1900,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) >> hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); >> hdmi->sink_has_audio = drm_detect_monitor_audio(edid); >> drm_mode_connector_update_edid_property(connector, edid); >> + cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, edid); >> ret = drm_add_edid_modes(connector, edid); >> /* Store the ELD */ >> drm_edid_to_eld(connector, edid); >> @@ -2077,6 +2082,10 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense) >> dw_hdmi_update_phy_mask(hdmi); >> } >> mutex_unlock(&hdmi->mutex); >> + >> + if (!rx_sense && !hpd) >> + cec_notifier_set_phys_addr(hdmi->cec_notifier, >> + CEC_PHYS_ADDR_INVALID); >> } >> >> void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense) >> @@ -2376,6 +2385,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> if (ret) >> goto err_iahb; >> >> + hdmi->cec_notifier = cec_notifier_get(dev); >> + if (!hdmi->cec_notifier) { >> + ret = -ENOMEM; >> + goto err_iahb; >> + } >> + >> /* >> * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator >> * N and cts values before enabling phy >> @@ -2452,6 +2467,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) >> hdmi->ddc = NULL; >> } >> >> + if (hdmi->cec_notifier) >> + cec_notifier_put(hdmi->cec_notifier); >> + >> clk_disable_unprepare(hdmi->iahb_clk); >> err_isfr: >> clk_disable_unprepare(hdmi->isfr_clk); >
On 07/06/2017 01:05 PM, Russell King - ARM Linux wrote: > On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote: >> From: Russell King <rmk+kernel@armlinux.org.uk> >> >> Add CEC notifier support to the HDMI bridge driver, so that the CEC >> part of the IP can receive its physical address. >> >> Tested-by: Neil Armstrong <narmstrong@baylibre.com> >> Acked-by: Neil Armstrong <narmstrong@baylibre.com> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> Hi Archit, Hans, >> >> This is repost of Russell's patch from his dw-hdmi CEC patchset. >> >> Since his CEC implementation will be integrated in the bridge driver, >> this notifier patch won't be re-posted. >> >> But the Amlogic Platform needs a notifier since it uses a standalone CEC >> controller. > > Without this, the dw-hdmi CEC support will be totally useless anyway, > and it's pointless to merge it without this patch. > Hi Russell, While following discussion on your last patchset, it seems you agreed with Hans to no more rely on the cec-notifier but directly call the dw-hdmi-cec functions. Anyway, if this patch is merged separately and you still depend on it, you could still rebase on it when it appears on drm-misc-next... Neil
On Thu, Jul 06, 2017 at 01:29:54PM +0200, Neil Armstrong wrote: > On 07/06/2017 01:05 PM, Russell King - ARM Linux wrote: > > On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote: > >> From: Russell King <rmk+kernel@armlinux.org.uk> > >> > >> Add CEC notifier support to the HDMI bridge driver, so that the CEC > >> part of the IP can receive its physical address. > >> > >> Tested-by: Neil Armstrong <narmstrong@baylibre.com> > >> Acked-by: Neil Armstrong <narmstrong@baylibre.com> > >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> > >> --- > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++ > >> 1 file changed, 18 insertions(+) > >> > >> Hi Archit, Hans, > >> > >> This is repost of Russell's patch from his dw-hdmi CEC patchset. > >> > >> Since his CEC implementation will be integrated in the bridge driver, > >> this notifier patch won't be re-posted. > >> > >> But the Amlogic Platform needs a notifier since it uses a standalone CEC > >> controller. > > > > Without this, the dw-hdmi CEC support will be totally useless anyway, > > and it's pointless to merge it without this patch. > > > > Hi Russell, > > While following discussion on your last patchset, it seems you agreed with Hans > to no more rely on the cec-notifier but directly call the dw-hdmi-cec functions. Incorrect. I think you're looking at the discussion in "thread mode" and assuming that messages are stored in date order... There was a discussion about removing the cec-notifier which happened on June 1st/2nd, based off the covering email. However, there was a later discussion on June 9th (sparked by your requirement) which changed the resolution from "lets remove the notifier" to "we need to keep the notifier". This was based on patch 2/4. I never posted an updated patch set, because of the dependencies, but Hans decided on June 12th to do I-don't-know-what with the patches I sent, resulting in what we have queued up today. This brought the sub-thread containing the cec-notifier removal discussion to be _after_ the June 9th discussion, which is probably what's causing some confusion here. However, I can assure you that the resolution of the discussion with the cec-notifier was that it should remain in place - this is from the last few messages in the discussion: Hans wrote on 9th June: | You wrote on 9th June: | > It won't since the Meson platform needs it... | | Ah, I wasn't aware of that when I wrote my original comments. In that case | we do need the notifier. Which is fine, as long as the reason for that is | documented. From what you're saying, it sounds like the CEC dw-hdmi-cec.c still relies on the notifier, but the patch which adds the CEC notifier to dw-hdmi.c was omited from what Hans did, which will result in the whole thing being a total waste of time. > Anyway, if this patch is merged separately and you still depend on it, > you could still rebase on it when it appears on drm-misc-next... Well, from what I can see in 4.12, the cec-notifier stuff is rather broken (tda998x has stopped working as its stuck with a physical address of f.f.f.f) so I think the whole thing is rather moot right now. I don't yet know what's going on with that, other than the notifier stuff seems to not be working, despite being enabled in the .config.
On 07/06/2017 01:45 PM, Russell King - ARM Linux wrote: > On Thu, Jul 06, 2017 at 01:29:54PM +0200, Neil Armstrong wrote: >> On 07/06/2017 01:05 PM, Russell King - ARM Linux wrote: >>> On Thu, Jul 06, 2017 at 12:33:06PM +0200, Neil Armstrong wrote: >>>> From: Russell King <rmk+kernel@armlinux.org.uk> >>>> >>>> Add CEC notifier support to the HDMI bridge driver, so that the CEC >>>> part of the IP can receive its physical address. >>>> >>>> Tested-by: Neil Armstrong <narmstrong@baylibre.com> >>>> Acked-by: Neil Armstrong <narmstrong@baylibre.com> >>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> >>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com> >>>> --- >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> Hi Archit, Hans, >>>> >>>> This is repost of Russell's patch from his dw-hdmi CEC patchset. >>>> >>>> Since his CEC implementation will be integrated in the bridge driver, >>>> this notifier patch won't be re-posted. >>>> >>>> But the Amlogic Platform needs a notifier since it uses a standalone CEC >>>> controller. >>> >>> Without this, the dw-hdmi CEC support will be totally useless anyway, >>> and it's pointless to merge it without this patch. >>> >> >> Hi Russell, >> >> While following discussion on your last patchset, it seems you agreed with Hans >> to no more rely on the cec-notifier but directly call the dw-hdmi-cec functions. > > Incorrect. I think you're looking at the discussion in "thread mode" > and assuming that messages are stored in date order... > > There was a discussion about removing the cec-notifier which happened > on June 1st/2nd, based off the covering email. > > However, there was a later discussion on June 9th (sparked by your > requirement) which changed the resolution from "lets remove the > notifier" to "we need to keep the notifier". This was based on > patch 2/4. > > I never posted an updated patch set, because of the dependencies, but > Hans decided on June 12th to do I-don't-know-what with the patches I > sent, resulting in what we have queued up today. This brought the > sub-thread containing the cec-notifier removal discussion to be _after_ > the June 9th discussion, which is probably what's causing some > confusion here. > > However, I can assure you that the resolution of the discussion with > the cec-notifier was that it should remain in place - this is from > the last few messages in the discussion: > > Hans wrote on 9th June: > | You wrote on 9th June: > | > It won't since the Meson platform needs it... > | > | Ah, I wasn't aware of that when I wrote my original comments. In that case > | we do need the notifier. Which is fine, as long as the reason for that is > | documented. > > From what you're saying, it sounds like the CEC dw-hdmi-cec.c still > relies on the notifier, but the patch which adds the CEC notifier > to dw-hdmi.c was omited from what Hans did, which will result in the > whole thing being a total waste of time. > >> Anyway, if this patch is merged separately and you still depend on it, >> you could still rebase on it when it appears on drm-misc-next... > > Well, from what I can see in 4.12, the cec-notifier stuff is rather > broken (tda998x has stopped working as its stuck with a physical > address of f.f.f.f) so I think the whole thing is rather moot right > now. I don't yet know what's going on with that, other than the > notifier stuff seems to not be working, despite being enabled in > the .config. > Indeed, I missed some parts of the thread, sorry for the confusion. Anyway, is it a showstopper to have this patch merged separately ? If you prefer re-posting a serie with this patch inside, no problem, but the Amlogic platform still needs this patch to have CEC working. Maybe it's because of fixes introduced in 4.13, but using next-20170706 and this patch, make things works perfectly. Neil
On Thu, Jul 06, 2017 at 12:45:55PM +0100, Russell King - ARM Linux wrote: > Well, from what I can see in 4.12, the cec-notifier stuff is rather > broken (tda998x has stopped working as its stuck with a physical > address of f.f.f.f) so I think the whole thing is rather moot right > now. I don't yet know what's going on with that, other than the > notifier stuff seems to not be working, despite being enabled in > the .config. The problem there appears to be the changes that were made with the way the config works - which IMHO are totally broken. Let's take this scenario: - You have a HDMI bridge, and you build that into the kernel, because you want the display to come up early. - You have a CEC driver, which you build as a module. If the HDMI bridge driver selects CEC_NOTIFIER and the CEC driver selects both CEC_NOTIFIER and CEC_CORE, you end up with CEC_NOTIFIER=y and CEC_CORE=m. We now come to this: #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER) The definition of IS_REACHABLE() is that it is false if the config symbol is selected as a module. So, in this case, we end up compiling out all the CEC notifier functions from the HDMI bridge, and building them into the CEC driver. The CEC notifier also gets built as a module, meaning that there's no way for the built-in HDMI bridge could ever call the notifier. The overall result of this is that such a configuration completely breaks such a setup - a setup that worked fine before the CEC Kconfig changes. This isn't limited to tda998x - I'd expect the same to be true of dw-hdmi.
On Thu, Jul 06, 2017 at 01:55:58PM +0200, Neil Armstrong wrote: > On 07/06/2017 01:45 PM, Russell King - ARM Linux wrote: > > Well, from what I can see in 4.12, the cec-notifier stuff is rather > > broken (tda998x has stopped working as its stuck with a physical > > address of f.f.f.f) so I think the whole thing is rather moot right > > now. I don't yet know what's going on with that, other than the > > notifier stuff seems to not be working, despite being enabled in > > the .config. > > > > Indeed, I missed some parts of the thread, sorry for the confusion. > > Anyway, is it a showstopper to have this patch merged separately ? It shouldn't be provided it makes _this_ merge window or the -rc following, but it doesn't really comprise a "fix" in that the feature never worked in the previous kernel (and we are supposed to have the rule that features are merged prior to the merge window.) It's up to the maintainers to decide what to do at this point. > If you prefer re-posting a serie with this patch inside, no problem, > but the Amlogic platform still needs this patch to have CEC working. I suspect (because I've not seen what has been queued to be merged yet) that even the dw-hdmi "native" side will be similarly broken - unless Hans merged some other changes to make it work. So, I suggest we all sit tight and wait until what was merged appears in Linus' tree.
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index ead1124..9c03846 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -33,6 +33,8 @@ #include <uapi/linux/media-bus-format.h> #include <uapi/linux/videodev2.h> +#include <media/cec-notifier.h> + #include "dw-hdmi.h" #include "dw-hdmi-audio.h" @@ -175,6 +177,8 @@ struct dw_hdmi { struct regmap *regm; void (*enable_audio)(struct dw_hdmi *hdmi); void (*disable_audio)(struct dw_hdmi *hdmi); + + struct cec_notifier *cec_notifier; }; #define HDMI_IH_PHY_STAT0_RX_SENSE \ @@ -1896,6 +1900,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); hdmi->sink_has_audio = drm_detect_monitor_audio(edid); drm_mode_connector_update_edid_property(connector, edid); + cec_notifier_set_phys_addr_from_edid(hdmi->cec_notifier, edid); ret = drm_add_edid_modes(connector, edid); /* Store the ELD */ drm_edid_to_eld(connector, edid); @@ -2077,6 +2082,10 @@ void __dw_hdmi_setup_rx_sense(struct dw_hdmi *hdmi, bool hpd, bool rx_sense) dw_hdmi_update_phy_mask(hdmi); } mutex_unlock(&hdmi->mutex); + + if (!rx_sense && !hpd) + cec_notifier_set_phys_addr(hdmi->cec_notifier, + CEC_PHYS_ADDR_INVALID); } void dw_hdmi_setup_rx_sense(struct device *dev, bool hpd, bool rx_sense) @@ -2376,6 +2385,12 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) if (ret) goto err_iahb; + hdmi->cec_notifier = cec_notifier_get(dev); + if (!hdmi->cec_notifier) { + ret = -ENOMEM; + goto err_iahb; + } + /* * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator * N and cts values before enabling phy @@ -2452,6 +2467,9 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) hdmi->ddc = NULL; } + if (hdmi->cec_notifier) + cec_notifier_put(hdmi->cec_notifier); + clk_disable_unprepare(hdmi->iahb_clk); err_isfr: clk_disable_unprepare(hdmi->isfr_clk);