Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On 27.08.2018 19:59, Russell King - ARM Linux wrote:
> Hi Andrzej,
>
> On Mon, Aug 27, 2018 at 06:15:59PM +0200, Andrzej Hajda wrote:
>> On 30.07.2018 18:42, Russell King wrote:
>>> static void tda998x_destroy(struct tda998x_priv *priv)
>>> {
>>> + drm_bridge_remove(>bridge);
>>> +
>>> /* disable all IRQs and free the IRQ handler */
>>> cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
>>> reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
>>> @@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client,
>>> struct tda998x_priv *priv)
>>> mutex_init(>mutex); /* protect the page access */
>>> mutex_init(>audio_mutex); /* protect access from audio thread */
>>> mutex_init(>edid_mutex);
>>> + INIT_LIST_HEAD(>bridge.list);
>> This line can be probably removed, unless there is a reason I am not
>> aware of.
> The addition above of drm_bridge_remove() to tda998x_destroy() means
> that we end up calling this function in the error cleanup path. This
> avoids unnecessary complexity with lots of different gotos - tda998x
> has had a long history of not cleaning up stuff properly.
1. bridge.list is/should be a private field of drm_bridge framework, so
it's direct usage in driver looks like layer violation.
2. Calling drm_bridge_remove() without drm_bridge_add() is not strictly
forbidden, but at least looks very suspicious. Even if current
implementation tolerates it, it can change in the future.
Neither argument is a blocker IMO so if you prefer to stay with current
solution please add a comment in the code explaining why do you
initializes list field, the code at first sight looks suspicious.
> devm interfaces for bridge do not help avoid that - devm stuff only
> works if everything that is registered previously is cleaned up via
> devm mechanisms to ensure that a device's interface becomes unavailable
> before stuff (eg, edid timers, detect work) is started to be cleaned up.
> Otherwise, there's a chance of this stuff being triggered during
> tear-down.
>
>>> +static int tda998x_bind(struct device *dev, struct device *master, void
>>> *data)
>>> +{
>>> + struct i2c_client *client = to_i2c_client(dev);
>>> + struct drm_device *drm = data;
>>> + struct tda998x_priv *priv;
>>> + int ret;
>>> +
>>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> + if (!priv)
>>> + return -ENOMEM;
>>> +
>>> + dev_set_drvdata(dev, priv);
>>> +
>>> + ret = tda998x_create(client, priv);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = tda998x_encoder_init(dev, drm);
>>> + if (ret) {
>>> + tda998x_destroy(priv);
>>> + return ret;
>>> + }
>>> + return 0;
>> It could be replaced by:
>> ret = tda998x_encoder_init(dev, drm);
>> if (ret)
>> tda998x_destroy(priv);
>> return ret;
>>
>> but this is probably matter of taste.
> It's not clear to me what "It" is - I think you're suggesting combining
> tda998x_create() and tda998x_encoder_init() ?
No, just simplifying error path.
>
> The code is structured this way to make the following patches easier -
> there is no point of combining things only to have to then break them
> apart again in a later patch. Please see patch 7, where tda998x_create()
> moves out of this function, where exactly this happens.
OK. As I said: up to you.
>
>> Moreover I guess priv->is_on could be removed if enable/disable
>> callbacks are called only by drm_core, but this is for another patch.
> Is it guaranteed that a bridge ->enable or ->disable callback won't be
> called twice, even for legacy drivers? I think atomic guarantees this
> but I don't think it's guaranteed for legacy drivers.
>
> I'm guessing Rob had a reason why he added the check when he originally
> created the driver (encoder ->dpms can be called for the same dpms
> state multiple times?)
>
OK, my guess was incorrect.
Regards
Andrzej
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
Hi Andrzej,
On Mon, Aug 27, 2018 at 06:15:59PM +0200, Andrzej Hajda wrote:
> On 30.07.2018 18:42, Russell King wrote:
> > static void tda998x_destroy(struct tda998x_priv *priv)
> > {
> > + drm_bridge_remove(>bridge);
> > +
> > /* disable all IRQs and free the IRQ handler */
> > cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> > reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
> > @@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client,
> > struct tda998x_priv *priv)
> > mutex_init(>mutex); /* protect the page access */
> > mutex_init(>audio_mutex); /* protect access from audio thread */
> > mutex_init(>edid_mutex);
> > + INIT_LIST_HEAD(>bridge.list);
>
> This line can be probably removed, unless there is a reason I am not
> aware of.
The addition above of drm_bridge_remove() to tda998x_destroy() means
that we end up calling this function in the error cleanup path. This
avoids unnecessary complexity with lots of different gotos - tda998x
has had a long history of not cleaning up stuff properly.
devm interfaces for bridge do not help avoid that - devm stuff only
works if everything that is registered previously is cleaned up via
devm mechanisms to ensure that a device's interface becomes unavailable
before stuff (eg, edid timers, detect work) is started to be cleaned up.
Otherwise, there's a chance of this stuff being triggered during
tear-down.
> > +static int tda998x_bind(struct device *dev, struct device *master, void
> > *data)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct drm_device *drm = data;
> > + struct tda998x_priv *priv;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + dev_set_drvdata(dev, priv);
> > +
> > + ret = tda998x_create(client, priv);
> > + if (ret)
> > + return ret;
> > +
> > + ret = tda998x_encoder_init(dev, drm);
> > + if (ret) {
> > + tda998x_destroy(priv);
> > + return ret;
> > + }
> > + return 0;
>
> It could be replaced by:
> ret = tda998x_encoder_init(dev, drm);
> if (ret)
> tda998x_destroy(priv);
> return ret;
>
> but this is probably matter of taste.
It's not clear to me what "It" is - I think you're suggesting combining
tda998x_create() and tda998x_encoder_init() ?
The code is structured this way to make the following patches easier -
there is no point of combining things only to have to then break them
apart again in a later patch. Please see patch 7, where tda998x_create()
moves out of this function, where exactly this happens.
> Moreover I guess priv->is_on could be removed if enable/disable
> callbacks are called only by drm_core, but this is for another patch.
Is it guaranteed that a bridge ->enable or ->disable callback won't be
called twice, even for legacy drivers? I think atomic guarantees this
but I don't think it's guaranteed for legacy drivers.
I'm guessing Rob had a reason why he added the check when he originally
created the driver (encoder ->dpms can be called for the same dpms
state multiple times?)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On 30.07.2018 18:42, Russell King wrote:
> Convert tda998x to a bridge driver with built-in encoder support for
> compatibility with existing component drivers.
>
> Signed-off-by: Russell King
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 154
> +++---
> 1 file changed, 79 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 843078e9fbf3..1ea62052f3e0 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -69,6 +69,7 @@ struct tda998x_priv {
> bool edid_delay_active;
>
> struct drm_encoder encoder;
> + struct drm_bridge bridge;
> struct drm_connector connector;
>
> struct tda998x_audio_port audio_port[2];
> @@ -79,9 +80,10 @@ struct tda998x_priv {
>
> #define conn_to_tda998x_priv(x) \
> container_of(x, struct tda998x_priv, connector)
> -
> #define enc_to_tda998x_priv(x) \
> container_of(x, struct tda998x_priv, encoder)
> +#define bridge_to_tda998x_priv(x) \
> + container_of(x, struct tda998x_priv, bridge)
>
> /* The TDA9988 series of devices use a paged register scheme.. to simplify
> * things we encode the page # in upper bits of the register #. To read/
> @@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector
> *connector)
> {
> struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
>
> - return >encoder;
> + return priv->bridge.encoder;
> }
>
> static
> @@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> if (ret)
> return ret;
>
> - drm_mode_connector_attach_encoder(>connector, >encoder);
> + drm_mode_connector_attach_encoder(>connector,
> + priv->bridge.encoder);
>
> return 0;
> }
>
> -/* DRM encoder functions */
> +/* DRM bridge functions */
> +
> +static int tda998x_bridge_attach(struct drm_bridge *bridge)
> +{
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> + return tda998x_connector_init(priv, bridge->dev);
> +}
> +
> +static void tda998x_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> + drm_connector_cleanup(>connector);
> +}
>
> -static void tda998x_enable(struct tda998x_priv *priv)
> +static void tda998x_bridge_enable(struct drm_bridge *bridge)
> {
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> if (!priv->is_on) {
> /* enable video ports, audio will be enabled later */
> reg_write(priv, REG_ENA_VP_0, 0xff);
> @@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv)
> }
> }
>
> -static void tda998x_disable(struct tda998x_priv *priv)
> +static void tda998x_bridge_disable(struct drm_bridge *bridge)
> {
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> if (!priv->is_on) {
> /* disable video ports */
> reg_write(priv, REG_ENA_VP_0, 0x00);
> @@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv)
> }
> }
>
> -static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
> +static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> {
> - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> - bool on;
> -
> - /* we only care about on or off: */
> - on = mode == DRM_MODE_DPMS_ON;
> -
> - if (on == priv->is_on)
> - return;
> -
> - if (on)
> - tda998x_enable(priv);
> - else
> - tda998x_disable(priv);
> -}
> -
> -static void
> -tda998x_encoder_mode_set(struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> -{
> - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> u16 ref_pix, ref_line, n_pix, n_line;
> u16 hs_pix_s, hs_pix_e;
> u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
> @@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
> mutex_unlock(>audio_mutex);
> }
>
> +static const struct drm_bridge_funcs tda998x_bridge_funcs = {
> + .attach = tda998x_bridge_attach,
> + .detach = tda998x_bridge_detach,
> + .disable = tda998x_bridge_disable,
> + .mode_set = tda998x_bridge_mode_set,
> + .enable = tda998x_bridge_enable,
> +};
> +
> static void tda998x_destroy(struct tda998x_priv *priv)
> {
> + drm_bridge_remove(>bridge);
> +
> /* disable all IRQs and free the IRQ handler */
> cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
> reg_clear(priv, REG_INT_FLAGS_2,
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Tue, Aug 14, 2018 at 12:42:42PM +0200, Daniel Vetter wrote: > Given your past track record of handling other contributors I think it's > entirely understandably that people do not choose to collaborate with you > voluntarily. Fixing that is entirely up to you though. I do not work piecemeal. I work in blocks, so at some point, I'll switch to working on X, and then I'll work on X until that's complete before moving on to Y. It's more efficient use of my time, otherwise I have to constantly context switch - and that leads to mistakes. I'm sorry that you don't think that's a good way of working, and you find that unacceptable, and you blame this on lack of collaboration. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Tue, Aug 14, 2018 at 12:48 PM, Russell King - ARM Linux wrote: > On Tue, Aug 14, 2018 at 12:42:42PM +0200, Daniel Vetter wrote: >> Given your past track record of handling other contributors I think it's >> entirely understandably that people do not choose to collaborate with you >> voluntarily. Fixing that is entirely up to you though. > > I do not work piecemeal. I work in blocks, so at some point, I'll > switch to working on X, and then I'll work on X until that's complete > before moving on to Y. It's more efficient use of my time, otherwise > I have to constantly context switch - and that leads to mistakes. > > I'm sorry that you don't think that's a good way of working, and you > find that unacceptable, and you blame this on lack of collaboration. Maybe it's not clear, but I'm not referring to how you schedule your own time here. I've referred to your downright abuse behaviour in past years here on dri-devel, which only stopped once we've put a full formal Code of Conduct into place, including enforcement. Up to and including simply throwing existing maintainers out. You blaming others for not reviewing your patches in this context is simply victim blaming. You do not get to blame others for the consequence of your past actions. It took you years of abuse emails to get there, it will take you years of hard constructive work and showing your best behaviour only, with no blaming others, to restore your reputation. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Fri, Aug 10, 2018 at 06:16:30PM +0100, Russell King - ARM Linux wrote: > On Fri, Aug 10, 2018 at 01:02:29PM -0400, Sean Paul wrote: > > On Fri, Aug 10, 2018 at 05:50:37PM +0100, Russell King - ARM Linux wrote: > > > Almost none of my DRM specific patches on dri-devel this time around > > > received any feedback what so ever, even after myself and David chasing > > > them up. Over the Armada changes, David Airlie eventually said to me > > > "if it works for you, send it to me I suppose". > > > > > > David also tried poking the tda998x/component discussion thread as well > > > from the beginning of July (or so David told me), with seemingly the > > > same result. > > > > > > So, in summary, it's been almost impossible to get any feedback on any > > > of the patches and discussions I've sent - I think extracting blood > > > from a rock might be easier! ;) > > > > Yeah, I suspect a lot of people have blinders wrt tda given that it's off in > > i2c/. Feel free to ping me on irc in future if you're looking for a review. > > Once > > tda moves to bridge/ and drm-misc, it'll likely be much easier to get people > > engaged. > > I suspect that those who actually use it don't have such blinders, yet > they also have ignored the patches despite being copied with them. I > can't see that the location of the driver would make any difference > for them. Expecting review to magically happen doesn't work. You need to actively solicit that, and often that involves a bit of reviewing trading. Especially anywhere where a part of the kernel is officially group-maintained like drm_bridge. Given your past track record of handling other contributors I think it's entirely understandably that people do not choose to collaborate with you voluntarily. Fixing that is entirely up to you though. I good first step here would imo be to move the tda driver under bridge, formally put it under the drm-misc group maintainership, actively solict and ack for that from other bridge maintainers and then also suggest other authors and contributors to the tda driver to join as committers and co-maintainers. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Fri, Aug 10, 2018 at 12:11:05PM -0400, Sean Paul wrote: > On Wed, Aug 08, 2018 at 11:15:47PM +0100, Russell King - ARM Linux wrote: > > In any case, bridges are buggy with unbinding/rebinding as I've pointed > > out several times in the past, but TDA998x used with Armada and TI LCDC > > as it currently stands are not. So, to do this as a full conversion to > > bridge and pushing the encoders into the DRM drivers results in a > > regression for these two DRM drivers. I'm not willing to accept such > > a regression, sorry. > > > > Thanks for the other cleanup suggestions, they can be done with a > > later patch (these changes have already been been merged.) > > I still think you should get review from a bridge maintainer before it goes to > drm-next. They are free to review them any time they wish, all the patches are on dri-devel. However "before it goes to drm-next" has been impossible for a while now because David has already pulled it in. Almost none of my DRM specific patches on dri-devel this time around received any feedback what so ever, even after myself and David chasing them up. Over the Armada changes, David Airlie eventually said to me "if it works for you, send it to me I suppose". David also tried poking the tda998x/component discussion thread as well from the beginning of July (or so David told me), with seemingly the same result. So, in summary, it's been almost impossible to get any feedback on any of the patches and discussions I've sent - I think extracting blood from a rock might be easier! ;) I sent a patch about the "broadcast rgb" property - there are no replies to that email, and it was only in a completely different thread that I happened to notice a comment about that patch - it is not a sign of a healthy community to be providing feedback on patches by sending replies to other threads! Yet that seems to be happening on dri-devel! I guess everyone could've gone to the beach (lucky them) because of the hot weather for the last couple of months, and now that it's cooled down in some parts, people are starting to re-engage... If the bridge maintainers eventually get around to reviewing the changes, then, as I've said, any necessary changes can be made later, but what you ask is now impossible - they've been in drm-next since Tuesday evening, and by implication they've been in linux-next too - including last night's linux-next which is the final one before the merge window opens assuming it opens on Sunday. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Fri, Aug 10, 2018 at 01:02:29PM -0400, Sean Paul wrote: > On Fri, Aug 10, 2018 at 05:50:37PM +0100, Russell King - ARM Linux wrote: > > Almost none of my DRM specific patches on dri-devel this time around > > received any feedback what so ever, even after myself and David chasing > > them up. Over the Armada changes, David Airlie eventually said to me > > "if it works for you, send it to me I suppose". > > > > David also tried poking the tda998x/component discussion thread as well > > from the beginning of July (or so David told me), with seemingly the > > same result. > > > > So, in summary, it's been almost impossible to get any feedback on any > > of the patches and discussions I've sent - I think extracting blood > > from a rock might be easier! ;) > > Yeah, I suspect a lot of people have blinders wrt tda given that it's off in > i2c/. Feel free to ping me on irc in future if you're looking for a review. > Once > tda moves to bridge/ and drm-misc, it'll likely be much easier to get people > engaged. I suspect that those who actually use it don't have such blinders, yet they also have ignored the patches despite being copied with them. I can't see that the location of the driver would make any difference for them. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Wed, Aug 08, 2018 at 03:09:30PM -0400, Sean Paul wrote:
> > -static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs
> > = {
> > - .dpms = tda998x_encoder_dpms,
> > - .prepare = tda998x_encoder_prepare,
> > - .commit = tda998x_encoder_commit,
> > - .mode_set = tda998x_encoder_mode_set,
> > -};
>
> Now that encoder is a stub, it should really be removed from here. The encoder
> should be instantiated elsewhere and attach the bridge to itself. There are a
> bunch of examples of this in bridge/
That's not possible at present - this driver has to remain compatible
with Armada and TI LCDC, both of which expect this driver to create
the encoder.
In any case, bridges are buggy with unbinding/rebinding as I've pointed
out several times in the past, but TDA998x used with Armada and TI LCDC
as it currently stands are not. So, to do this as a full conversion to
bridge and pushing the encoders into the DRM drivers results in a
regression for these two DRM drivers. I'm not willing to accept such
a regression, sorry.
Thanks for the other cleanup suggestions, they can be done with a
later patch (these changes have already been been merged.)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Fri, Aug 10, 2018 at 05:50:37PM +0100, Russell King - ARM Linux wrote: > On Fri, Aug 10, 2018 at 12:11:05PM -0400, Sean Paul wrote: > > On Wed, Aug 08, 2018 at 11:15:47PM +0100, Russell King - ARM Linux wrote: > > > In any case, bridges are buggy with unbinding/rebinding as I've pointed > > > out several times in the past, but TDA998x used with Armada and TI LCDC > > > as it currently stands are not. So, to do this as a full conversion to > > > bridge and pushing the encoders into the DRM drivers results in a > > > regression for these two DRM drivers. I'm not willing to accept such > > > a regression, sorry. > > > > > > Thanks for the other cleanup suggestions, they can be done with a > > > later patch (these changes have already been been merged.) > > > > I still think you should get review from a bridge maintainer before it goes > > to > > drm-next. > > They are free to review them any time they wish, all the patches are > on dri-devel. However "before it goes to drm-next" has been impossible > for a while now because David has already pulled it in. > Oh, I didn't realize it was already pulled into -next. > Almost none of my DRM specific patches on dri-devel this time around > received any feedback what so ever, even after myself and David chasing > them up. Over the Armada changes, David Airlie eventually said to me > "if it works for you, send it to me I suppose". > > David also tried poking the tda998x/component discussion thread as well > from the beginning of July (or so David told me), with seemingly the > same result. > > So, in summary, it's been almost impossible to get any feedback on any > of the patches and discussions I've sent - I think extracting blood > from a rock might be easier! ;) Yeah, I suspect a lot of people have blinders wrt tda given that it's off in i2c/. Feel free to ping me on irc in future if you're looking for a review. Once tda moves to bridge/ and drm-misc, it'll likely be much easier to get people engaged. > > I sent a patch about the "broadcast rgb" property - there are no replies > to that email, and it was only in a completely different thread that I > happened to notice a comment about that patch - it is not a sign of a > healthy community to be providing feedback on patches by sending replies > to other threads! Yet that seems to be happening on dri-devel! > > I guess everyone could've gone to the beach (lucky them) because of the > hot weather for the last couple of months, and now that it's cooled down > in some parts, people are starting to re-engage... > > If the bridge maintainers eventually get around to reviewing the changes, > then, as I've said, any necessary changes can be made later, but what you > ask is now impossible - they've been in drm-next since Tuesday evening, > and by implication they've been in linux-next too - including last night's > linux-next which is the final one before the merge window opens assuming > it opens on Sunday. FYI, drm-misc (and by extension, bridge drivers) are under feature freeze after rc6 is cut. That soak time in linux-next has proven quite helpful for the stability of drm. Having the bridge conversion spend only a week in linux-next before merge window is not ideal. Again, nothing that can be done about that now, but keep that in mind for future. Sean > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up > According to speedtest.net: 13Mbps down 490kbps up -- Sean Paul, Software Engineer, Google / Chromium OS ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Wed, Aug 08, 2018 at 11:15:47PM +0100, Russell King - ARM Linux wrote:
> On Wed, Aug 08, 2018 at 03:09:30PM -0400, Sean Paul wrote:
> > > -static const struct drm_encoder_helper_funcs
> > > tda998x_encoder_helper_funcs = {
> > > - .dpms = tda998x_encoder_dpms,
> > > - .prepare = tda998x_encoder_prepare,
> > > - .commit = tda998x_encoder_commit,
> > > - .mode_set = tda998x_encoder_mode_set,
> > > -};
> >
> > Now that encoder is a stub, it should really be removed from here. The
> > encoder
> > should be instantiated elsewhere and attach the bridge to itself. There are
> > a
> > bunch of examples of this in bridge/
>
> That's not possible at present - this driver has to remain compatible
> with Armada and TI LCDC, both of which expect this driver to create
> the encoder.
Hmm, yeah, I just read that thread. I suppose once bridges play nice with
components the encoder can migrate into the drivers?
>
> In any case, bridges are buggy with unbinding/rebinding as I've pointed
> out several times in the past, but TDA998x used with Armada and TI LCDC
> as it currently stands are not. So, to do this as a full conversion to
> bridge and pushing the encoders into the DRM drivers results in a
> regression for these two DRM drivers. I'm not willing to accept such
> a regression, sorry.
>
> Thanks for the other cleanup suggestions, they can be done with a
> later patch (these changes have already been been merged.)
I still think you should get review from a bridge maintainer before it goes to
drm-next.
Sean
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
> According to speedtest.net: 13Mbps down 490kbps up
--
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
On Mon, Jul 30, 2018 at 05:42:21PM +0100, Russell King wrote:
> Convert tda998x to a bridge driver with built-in encoder support for
> compatibility with existing component drivers.
>
> Signed-off-by: Russell King
Hi Russell,
Thanks for doing the bridge conversion, it certainly seems a better fit. I've
cc'd the bridge maintainers/reviewer on this patch so that hopefully they will
see it.
We should probably also move this driver into bridge/ once it's been reviewed.
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 154
> +++---
> 1 file changed, 79 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 843078e9fbf3..1ea62052f3e0 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -69,6 +69,7 @@ struct tda998x_priv {
> bool edid_delay_active;
>
> struct drm_encoder encoder;
> + struct drm_bridge bridge;
> struct drm_connector connector;
>
> struct tda998x_audio_port audio_port[2];
> @@ -79,9 +80,10 @@ struct tda998x_priv {
>
> #define conn_to_tda998x_priv(x) \
> container_of(x, struct tda998x_priv, connector)
> -
> #define enc_to_tda998x_priv(x) \
> container_of(x, struct tda998x_priv, encoder)
> +#define bridge_to_tda998x_priv(x) \
> + container_of(x, struct tda998x_priv, bridge)
>
> /* The TDA9988 series of devices use a paged register scheme.. to simplify
> * things we encode the page # in upper bits of the register #. To read/
> @@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector
> *connector)
> {
> struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
>
> - return >encoder;
> + return priv->bridge.encoder;
> }
>
> static
> @@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> if (ret)
> return ret;
>
> - drm_mode_connector_attach_encoder(>connector, >encoder);
> + drm_mode_connector_attach_encoder(>connector,
> + priv->bridge.encoder);
>
> return 0;
> }
>
> -/* DRM encoder functions */
> +/* DRM bridge functions */
> +
> +static int tda998x_bridge_attach(struct drm_bridge *bridge)
> +{
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> + return tda998x_connector_init(priv, bridge->dev);
There doesn't seem to be any benefit to having tda998x_connector_init() as a
separate function. I'd suggest just rolling that code in here.
> +}
> +
> +static void tda998x_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> + drm_connector_cleanup(>connector);
> +}
>
> -static void tda998x_enable(struct tda998x_priv *priv)
> +static void tda998x_bridge_enable(struct drm_bridge *bridge)
> {
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> if (!priv->is_on) {
> /* enable video ports, audio will be enabled later */
> reg_write(priv, REG_ENA_VP_0, 0xff);
> @@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv)
> }
> }
>
> -static void tda998x_disable(struct tda998x_priv *priv)
> +static void tda998x_bridge_disable(struct drm_bridge *bridge)
> {
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> if (!priv->is_on) {
> /* disable video ports */
> reg_write(priv, REG_ENA_VP_0, 0x00);
> @@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv)
> }
> }
>
> -static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
> +static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> {
> - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> - bool on;
> -
> - /* we only care about on or off: */
> - on = mode == DRM_MODE_DPMS_ON;
> -
> - if (on == priv->is_on)
> - return;
> -
> - if (on)
> - tda998x_enable(priv);
> - else
> - tda998x_disable(priv);
> -}
> -
> -static void
> -tda998x_encoder_mode_set(struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> -{
> - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> u16 ref_pix, ref_line, n_pix, n_line;
> u16 hs_pix_s, hs_pix_e;
> u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
> @@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
> mutex_unlock(>audio_mutex);
> }
>
> +static const struct drm_bridge_funcs tda998x_bridge_funcs = {
> + .attach = tda998x_bridge_attach,
> + .detach =
Re: [PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
Hi!
This patch needs a refresh since commit
cde4c44d8769 ("drm: drop _mode_ from drm_mode_connector_attach_encoder")
interferes with hunk#4
On 2018-07-30 18:42, Russell King wrote:
> Convert tda998x to a bridge driver with built-in encoder support for
> compatibility with existing component drivers.
>
> Signed-off-by: Russell King
> ---
> drivers/gpu/drm/i2c/tda998x_drv.c | 154
> +++---
> 1 file changed, 79 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
> b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 843078e9fbf3..1ea62052f3e0 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -69,6 +69,7 @@ struct tda998x_priv {
> bool edid_delay_active;
>
> struct drm_encoder encoder;
> + struct drm_bridge bridge;
> struct drm_connector connector;
>
> struct tda998x_audio_port audio_port[2];
> @@ -79,9 +80,10 @@ struct tda998x_priv {
>
> #define conn_to_tda998x_priv(x) \
> container_of(x, struct tda998x_priv, connector)
> -
> #define enc_to_tda998x_priv(x) \
> container_of(x, struct tda998x_priv, encoder)
> +#define bridge_to_tda998x_priv(x) \
> + container_of(x, struct tda998x_priv, bridge)
>
> /* The TDA9988 series of devices use a paged register scheme.. to simplify
> * things we encode the page # in upper bits of the register #. To read/
> @@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector
> *connector)
> {
> struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
>
> - return >encoder;
> + return priv->bridge.encoder;
> }
>
> static
> @@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv
> *priv,
> if (ret)
> return ret;
>
> - drm_mode_connector_attach_encoder(>connector, >encoder);
> + drm_mode_connector_attach_encoder(>connector,
> + priv->bridge.encoder);
cde4c44d8769 ("drm: drop _mode_ from drm_mode_connector_attach_encoder")
conflicts with this hunk.
Cheers,
Peter
>
> return 0;
> }
>
> -/* DRM encoder functions */
> +/* DRM bridge functions */
> +
> +static int tda998x_bridge_attach(struct drm_bridge *bridge)
> +{
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> + return tda998x_connector_init(priv, bridge->dev);
> +}
> +
> +static void tda998x_bridge_detach(struct drm_bridge *bridge)
> +{
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> + drm_connector_cleanup(>connector);
> +}
>
> -static void tda998x_enable(struct tda998x_priv *priv)
> +static void tda998x_bridge_enable(struct drm_bridge *bridge)
> {
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> if (!priv->is_on) {
> /* enable video ports, audio will be enabled later */
> reg_write(priv, REG_ENA_VP_0, 0xff);
> @@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv)
> }
> }
>
> -static void tda998x_disable(struct tda998x_priv *priv)
> +static void tda998x_bridge_disable(struct drm_bridge *bridge)
> {
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> +
> if (!priv->is_on) {
> /* disable video ports */
> reg_write(priv, REG_ENA_VP_0, 0x00);
> @@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv)
> }
> }
>
> -static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
> +static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adjusted_mode)
> {
> - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> - bool on;
> -
> - /* we only care about on or off: */
> - on = mode == DRM_MODE_DPMS_ON;
> -
> - if (on == priv->is_on)
> - return;
> -
> - if (on)
> - tda998x_enable(priv);
> - else
> - tda998x_disable(priv);
> -}
> -
> -static void
> -tda998x_encoder_mode_set(struct drm_encoder *encoder,
> - struct drm_display_mode *mode,
> - struct drm_display_mode *adjusted_mode)
> -{
> - struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
> + struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
> u16 ref_pix, ref_line, n_pix, n_line;
> u16 hs_pix_s, hs_pix_e;
> u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
> @@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
> mutex_unlock(>audio_mutex);
> }
>
> +static const struct drm_bridge_funcs tda998x_bridge_funcs = {
> + .attach = tda998x_bridge_attach,
> + .detach = tda998x_bridge_detach,
> + .disable = tda998x_bridge_disable,
> + .mode_set = tda998x_bridge_mode_set,
> + .enable = tda998x_bridge_enable,
> +};
> +
>
[PATCH v2 4/7] drm/i2c: tda998x: convert to bridge driver
Convert tda998x to a bridge driver with built-in encoder support for
compatibility with existing component drivers.
Signed-off-by: Russell King
---
drivers/gpu/drm/i2c/tda998x_drv.c | 154 +++---
1 file changed, 79 insertions(+), 75 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 843078e9fbf3..1ea62052f3e0 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -69,6 +69,7 @@ struct tda998x_priv {
bool edid_delay_active;
struct drm_encoder encoder;
+ struct drm_bridge bridge;
struct drm_connector connector;
struct tda998x_audio_port audio_port[2];
@@ -79,9 +80,10 @@ struct tda998x_priv {
#define conn_to_tda998x_priv(x) \
container_of(x, struct tda998x_priv, connector)
-
#define enc_to_tda998x_priv(x) \
container_of(x, struct tda998x_priv, encoder)
+#define bridge_to_tda998x_priv(x) \
+ container_of(x, struct tda998x_priv, bridge)
/* The TDA9988 series of devices use a paged register scheme.. to simplify
* things we encode the page # in upper bits of the register #. To read/
@@ -1262,7 +1264,7 @@ tda998x_connector_best_encoder(struct drm_connector
*connector)
{
struct tda998x_priv *priv = conn_to_tda998x_priv(connector);
- return >encoder;
+ return priv->bridge.encoder;
}
static
@@ -1292,15 +1294,32 @@ static int tda998x_connector_init(struct tda998x_priv
*priv,
if (ret)
return ret;
- drm_mode_connector_attach_encoder(>connector, >encoder);
+ drm_mode_connector_attach_encoder(>connector,
+ priv->bridge.encoder);
return 0;
}
-/* DRM encoder functions */
+/* DRM bridge functions */
+
+static int tda998x_bridge_attach(struct drm_bridge *bridge)
+{
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+
+ return tda998x_connector_init(priv, bridge->dev);
+}
+
+static void tda998x_bridge_detach(struct drm_bridge *bridge)
+{
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+
+ drm_connector_cleanup(>connector);
+}
-static void tda998x_enable(struct tda998x_priv *priv)
+static void tda998x_bridge_enable(struct drm_bridge *bridge)
{
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+
if (!priv->is_on) {
/* enable video ports, audio will be enabled later */
reg_write(priv, REG_ENA_VP_0, 0xff);
@@ -1315,8 +1334,10 @@ static void tda998x_enable(struct tda998x_priv *priv)
}
}
-static void tda998x_disable(struct tda998x_priv *priv)
+static void tda998x_bridge_disable(struct drm_bridge *bridge)
{
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
+
if (!priv->is_on) {
/* disable video ports */
reg_write(priv, REG_ENA_VP_0, 0x00);
@@ -1327,29 +1348,11 @@ static void tda998x_disable(struct tda998x_priv *priv)
}
}
-static void tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_bridge_mode_set(struct drm_bridge *bridge,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
- bool on;
-
- /* we only care about on or off: */
- on = mode == DRM_MODE_DPMS_ON;
-
- if (on == priv->is_on)
- return;
-
- if (on)
- tda998x_enable(priv);
- else
- tda998x_disable(priv);
-}
-
-static void
-tda998x_encoder_mode_set(struct drm_encoder *encoder,
-struct drm_display_mode *mode,
-struct drm_display_mode *adjusted_mode)
-{
- struct tda998x_priv *priv = enc_to_tda998x_priv(encoder);
+ struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
u16 ref_pix, ref_line, n_pix, n_line;
u16 hs_pix_s, hs_pix_e;
u16 vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1556,8 +1559,18 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
mutex_unlock(>audio_mutex);
}
+static const struct drm_bridge_funcs tda998x_bridge_funcs = {
+ .attach = tda998x_bridge_attach,
+ .detach = tda998x_bridge_detach,
+ .disable = tda998x_bridge_disable,
+ .mode_set = tda998x_bridge_mode_set,
+ .enable = tda998x_bridge_enable,
+};
+
static void tda998x_destroy(struct tda998x_priv *priv)
{
+ drm_bridge_remove(>bridge);
+
/* disable all IRQs and free the IRQ handler */
cec_write(priv, REG_CEC_RXSHPDINTENA, 0);
reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
@@ -1650,6 +1663,7 @@ static int tda998x_create(struct i2c_client *client,
struct tda998x_priv *priv)
mutex_init(>mutex); /* protect the page access */
mutex_init(>audio_mutex);
