| Message ID | 1544541373-30044-6-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: adv748x: Implement dynamic routing support | expand |
Hi Jacopo, On 11/12/2018 15:16, Jacopo Mondi wrote: > When the adv748x driver is informed about a link being created from HDMI or > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > sure to implement proper routing management at link setup time, to route > the selected video stream to the desired TX output. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++- > drivers/media/i2c/adv748x/adv748x.h | 1 + > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > index f3aabbccdfb5..08dc0e89b053 100644 > --- a/drivers/media/i2c/adv748x/adv748x-core.c > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > /* ----------------------------------------------------------------------------- > * Media Operations > */ > +static int adv748x_link_setup(struct media_entity *entity, > + const struct media_pad *local, > + const struct media_pad *remote, u32 flags) > +{ > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > + struct adv748x_csi2 *tx; > + struct media_link *link; > + u8 io10; > + > + /* > + * For each link setup from [HDMI|AFE] to TX we receive two > + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]". > + * > + * Use the second notification form to make sure we're linking > + * to a TX and find out from where, to set up routing properly. > + */ > + if ((sd != &state->txa.sd && sd != &state->txb.sd) || I'm starting to think an 'is_txb(tx)' would help clean up some code ... Then we could do the assignment of tx above, and then this line would read if ( (!(is_txa(tx) && !(is_txb(tx))) || !(flags & MEDIA_LNK_FL_ENABLED) ) It shouldn't matter that the adv748x_sd_to_csi2(sd) could be called on non-TX SD's as they will then simply fail to match the above is_txa/is_txb. > + !(flags & MEDIA_LNK_FL_ENABLED)) > + return 0; Don't we need to clear some local references when disabling links? (or actually perhaps it doesn't matter if we keep stale references in a disabled object, because it's disabled) > + tx = adv748x_sd_to_csi2(sd); > + > + /* > + * Now that we're sure we're operating on one of the two TXs, > + * make sure there are no enabled links ending there from > + * either HDMI or AFE (this can only happens for TXA though). > + */ > + if (is_txa(tx)) > + list_for_each_entry(link, &entity->links, list) > + if (link->sink->entity == entity && > + link->flags & MEDIA_LNK_FL_ENABLED) > + return -EINVAL; > + What does this protect? Doesn't this code read as: if (is TXA) for each entity Check all links - and if any are enabled, -EINVAL Don't we ever want a link to be enabled on TXA? (I must surely be mis-reading this - and it's nearly mid-night - so I'm going to say I'm confused and it's time for me to stop and go to bed :D) > + /* Change video stream routing, according to the newly created link. */ > + io10 = io_read(state, ADV748X_IO_10); > + if (rsd == &state->afe.sd) { > + state->afe.tx = tx; > + > + /* > + * If AFE is routed to TXA, make sure TXB is off; > + * If AFE goes to TXB, we need TXA powered on. > + */ > + if (is_txa(tx)) { > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > + io10 &= ~ADV748X_IO_10_CSI1_EN; > + } else { > + io10 |= ADV748X_IO_10_CSI4_EN | > + ADV748X_IO_10_CSI1_EN; > + } > + } else { > + state->hdmi.tx = tx; > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > + } > + io_write(state, ADV748X_IO_10, io10); > + > + tx->rsd = rsd; > + > + return 0; > +} > > static const struct media_entity_operations adv748x_media_ops = { > - .link_validate = v4l2_subdev_link_validate, > + .link_setup = adv748x_link_setup, > + .link_validate = v4l2_subdev_link_validate, > }; > > /* ----------------------------------------------------------------------------- > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > index 0ee3b8d5c795..63a17c31c169 100644 > --- a/drivers/media/i2c/adv748x/adv748x.h > +++ b/drivers/media/i2c/adv748x/adv748x.h > @@ -220,6 +220,7 @@ struct adv748x_state { > #define ADV748X_IO_10_CSI4_EN BIT(7) > #define ADV748X_IO_10_CSI1_EN BIT(6) > #define ADV748X_IO_10_PIX_OUT_EN BIT(5) > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE 0x08 Should this be BIT(3)? > > #define ADV748X_IO_CHIP_REV_ID_1 0xdf > #define ADV748X_IO_CHIP_REV_ID_2 0xe0 >
Hi Kieran, On Tue, Dec 11, 2018 at 11:43:08PM +0000, Kieran Bingham wrote: > Hi Jacopo, > > On 11/12/2018 15:16, Jacopo Mondi wrote: > > When the adv748x driver is informed about a link being created from HDMI or > > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > > sure to implement proper routing management at link setup time, to route > > the selected video stream to the desired TX output. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++- > > drivers/media/i2c/adv748x/adv748x.h | 1 + > > 2 files changed, 63 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c > > index f3aabbccdfb5..08dc0e89b053 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > > /* ----------------------------------------------------------------------------- > > * Media Operations > > */ > > +static int adv748x_link_setup(struct media_entity *entity, > > + const struct media_pad *local, > > + const struct media_pad *remote, u32 flags) > > +{ > > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > > + struct adv748x_csi2 *tx; > > + struct media_link *link; > > + u8 io10; > > + > > + /* > > + * For each link setup from [HDMI|AFE] to TX we receive two > > + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]". > > + * > > + * Use the second notification form to make sure we're linking > > + * to a TX and find out from where, to set up routing properly. > > + */ > > > > + if ((sd != &state->txa.sd && sd != &state->txb.sd) || > > I'm starting to think an 'is_txb(tx)' would help clean up some code ... > Then we could do the assignment of tx above, and then this line would read > > if ( (!(is_txa(tx) && !(is_txb(tx))) > || !(flags & MEDIA_LNK_FL_ENABLED) ) > > > It shouldn't matter that the adv748x_sd_to_csi2(sd) could be called on > non-TX SD's as they will then simply fail to match the above is_txa/is_txb. > Checking for is_txa() and is_txb() would require to call 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually represent a csi2_tx. I would keep it as it is. > > > > + !(flags & MEDIA_LNK_FL_ENABLED)) > > + return 0; > > Don't we need to clear some local references when disabling links? > I don't think so, if the link is disabled the pipeline would never start and s_stream() (where the reference to the connected tx is used) will never be called the AFE or HDMI backends. > (or actually perhaps it doesn't matter if we keep stale references in a > disabled object, because it's disabled) Yes. Even if both HDMI and AFE have 'TXA' as their connected TX, only one of them has an actually enabled link, and to enable that link, the previously existing one has to be disabled first, otherwise this function fails (see the -EINVAL a few lines below) > > > + tx = adv748x_sd_to_csi2(sd); > > > > + > > + /* > > + * Now that we're sure we're operating on one of the two TXs, > > + * make sure there are no enabled links ending there from > > + * either HDMI or AFE (this can only happens for TXA though). > > + */ > > + if (is_txa(tx)) > > + list_for_each_entry(link, &entity->links, list) > > + if (link->sink->entity == entity && > > + link->flags & MEDIA_LNK_FL_ENABLED) > > + return -EINVAL; > > + > > What does this protect? > > Doesn't this code read as: > > if (is TXA) > for each entity > Check all links - and if any are enabled, -EINVAL > > Don't we ever want a link to be enabled on TXA? Not if we are enabling another one. One should first disable the existing link, then create a new one. > > (I must surely be mis-reading this - and it's nearly mid-night - so I'm > going to say I'm confused and it's time for me to stop and go to bed :D) > > > > + /* Change video stream routing, according to the newly created link. */ > > + io10 = io_read(state, ADV748X_IO_10); > > + if (rsd == &state->afe.sd) { > > + state->afe.tx = tx; > > + > > + /* > > + * If AFE is routed to TXA, make sure TXB is off; > > + * If AFE goes to TXB, we need TXA powered on. > > + */ > > + if (is_txa(tx)) { > > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + io10 &= ~ADV748X_IO_10_CSI1_EN; > > + } else { > > + io10 |= ADV748X_IO_10_CSI4_EN | > > + ADV748X_IO_10_CSI1_EN; > > + } > > + } else { > > + state->hdmi.tx = tx; > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + } > > + io_write(state, ADV748X_IO_10, io10); > > + > > + tx->rsd = rsd; > > + > > + return 0; > > +} > > > > static const struct media_entity_operations adv748x_media_ops = { > > - .link_validate = v4l2_subdev_link_validate, > > + .link_setup = adv748x_link_setup, > > + .link_validate = v4l2_subdev_link_validate, > > }; > > > > /* ----------------------------------------------------------------------------- > > diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h > > index 0ee3b8d5c795..63a17c31c169 100644 > > --- a/drivers/media/i2c/adv748x/adv748x.h > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > @@ -220,6 +220,7 @@ struct adv748x_state { > > #define ADV748X_IO_10_CSI4_EN BIT(7) > > #define ADV748X_IO_10_CSI1_EN BIT(6) > > #define ADV748X_IO_10_PIX_OUT_EN BIT(5) > > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE 0x08 > > Should this be BIT(3)? > It surely read better. See, you were not that sleepy as you said, after all :p Thanks for review, I'll wait some more time to receive more comments and will resend. Thanks j > > > > #define ADV748X_IO_CHIP_REV_ID_1 0xdf > > #define ADV748X_IO_CHIP_REV_ID_2 0xe0 > > > > -- > Regards > -- > Kieran
Hi Jacopo, On 12/12/2018 08:27, jacopo mondi wrote: > Hi Kieran, > > On Tue, Dec 11, 2018 at 11:43:08PM +0000, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 11/12/2018 15:16, Jacopo Mondi wrote: >>> When the adv748x driver is informed about a link being created from HDMI or >>> AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make >>> sure to implement proper routing management at link setup time, to route >>> the selected video stream to the desired TX output. >>> >>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >>> --- >>> drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++- >>> drivers/media/i2c/adv748x/adv748x.h | 1 + >>> 2 files changed, 63 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c >>> index f3aabbccdfb5..08dc0e89b053 100644 >>> --- a/drivers/media/i2c/adv748x/adv748x-core.c >>> +++ b/drivers/media/i2c/adv748x/adv748x-core.c >>> @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) >>> /* ----------------------------------------------------------------------------- >>> * Media Operations >>> */ >>> +static int adv748x_link_setup(struct media_entity *entity, >>> + const struct media_pad *local, >>> + const struct media_pad *remote, u32 flags) >>> +{ >>> + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); >>> + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); >>> + struct adv748x_state *state = v4l2_get_subdevdata(sd); >>> + struct adv748x_csi2 *tx; >>> + struct media_link *link; >>> + u8 io10; >>> + >>> + /* >>> + * For each link setup from [HDMI|AFE] to TX we receive two >>> + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]". >>> + * >>> + * Use the second notification form to make sure we're linking >>> + * to a TX and find out from where, to set up routing properly. >>> + */ >> >> >>> + if ((sd != &state->txa.sd && sd != &state->txb.sd) || >> >> I'm starting to think an 'is_txb(tx)' would help clean up some code ... >> Then we could do the assignment of tx above, and then this line would read >> >> if ( (!(is_txa(tx) && !(is_txb(tx))) >> || !(flags & MEDIA_LNK_FL_ENABLED) ) >> >> >> It shouldn't matter that the adv748x_sd_to_csi2(sd) could be called on >> non-TX SD's as they will then simply fail to match the above is_txa/is_txb. >> > > Checking for is_txa() and is_txb() would require to call > 'adv_sd_to_csi2(sd)' before having made sure the 'sd' actually > represent a csi2_tx. I would keep it as it is. Indeed - but the is_txa() / is_txb() would then guard against their usage. We only use !is_txa(tx) once currently. If we add more - then it might be worth a separate patch to add in the is_txb() anyway if you feel like adding to your patch count ;-) >> >> >>> + !(flags & MEDIA_LNK_FL_ENABLED)) >>> + return 0; >> >> Don't we need to clear some local references when disabling links? >> > > I don't think so, if the link is disabled the pipeline would never > start and s_stream() (where the reference to the connected tx is used) > will never be called the AFE or HDMI backends. Ok - that's fine then. >> (or actually perhaps it doesn't matter if we keep stale references in a >> disabled object, because it's disabled) > > Yes. Even if both HDMI and AFE have 'TXA' as their connected TX, only one > of them has an actually enabled link, and to enable that link, the > previously existing one has to be disabled first, otherwise this > function fails (see the -EINVAL a few lines below) Good. > >> >>> + tx = adv748x_sd_to_csi2(sd); >> >> >>> + >>> + /* >>> + * Now that we're sure we're operating on one of the two TXs, >>> + * make sure there are no enabled links ending there from >>> + * either HDMI or AFE (this can only happens for TXA though). >>> + */ >>> + if (is_txa(tx)) >>> + list_for_each_entry(link, &entity->links, list) >>> + if (link->sink->entity == entity && >>> + link->flags & MEDIA_LNK_FL_ENABLED) >>> + return -EINVAL; >>> + >> >> What does this protect? >> >> Doesn't this code read as: >> >> if (is TXA) >> for each entity >> Check all links - and if any are enabled, -EINVAL >> >> Don't we ever want a link to be enabled on TXA? > > Not if we are enabling another one. One should first disable the > existing link, then create a new one. Ah - I read the code correctly - but mis-interpreted where the links were coming from. I incorrectly thought they were 'new' links - not checking the existing links (which is obvious from the whole 'entity->links' parameter in the for_each() - but ... well :) I (probably incorrectly) /assume/ then that we could drop the if(is_txa(tx)) conditional and indent here? As for TXB we will know that it's links are not enabled - and will pass ? I'm not sure if that would make for cleaner code (reduced indent) or less obvious intent (not acting on TXA) though - so ... up to you :) >> >> (I must surely be mis-reading this - and it's nearly mid-night - so I'm >> going to say I'm confused and it's time for me to stop and go to bed :D) To: Me - "You were. Good job you went to bed :)" >> >> >>> + /* Change video stream routing, according to the newly created link. */ >>> + io10 = io_read(state, ADV748X_IO_10); >>> + if (rsd == &state->afe.sd) { >>> + state->afe.tx = tx; >>> + >>> + /* >>> + * If AFE is routed to TXA, make sure TXB is off; >>> + * If AFE goes to TXB, we need TXA powered on. >>> + */ >>> + if (is_txa(tx)) { >>> + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; >>> + io10 &= ~ADV748X_IO_10_CSI1_EN; >>> + } else { >>> + io10 |= ADV748X_IO_10_CSI4_EN | >>> + ADV748X_IO_10_CSI1_EN; >>> + } >>> + } else { >>> + state->hdmi.tx = tx; >>> + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; >>> + } >>> + io_write(state, ADV748X_IO_10, io10); >>> + >>> + tx->rsd = rsd; >>> + >>> + return 0; >>> +} >>> >>> static const struct media_entity_operations adv748x_media_ops = { >>> - .link_validate = v4l2_subdev_link_validate, >>> + .link_setup = adv748x_link_setup, >>> + .link_validate = v4l2_subdev_link_validate, >>> }; >>> >>> /* ----------------------------------------------------------------------------- >>> diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h >>> index 0ee3b8d5c795..63a17c31c169 100644 >>> --- a/drivers/media/i2c/adv748x/adv748x.h >>> +++ b/drivers/media/i2c/adv748x/adv748x.h >>> @@ -220,6 +220,7 @@ struct adv748x_state { >>> #define ADV748X_IO_10_CSI4_EN BIT(7) >>> #define ADV748X_IO_10_CSI1_EN BIT(6) >>> #define ADV748X_IO_10_PIX_OUT_EN BIT(5) >>> +#define ADV748X_IO_10_CSI4_IN_SEL_AFE 0x08 >> >> Should this be BIT(3)? >> > > It surely read better. See, you were not that sleepy as you said, > after all :p > > Thanks for review, I'll wait some more time to receive more comments > and will resend. > > Thanks > j > >>> >>> #define ADV748X_IO_CHIP_REV_ID_1 0xdf >>> #define ADV748X_IO_CHIP_REV_ID_2 0xe0 >>> >> >> -- >> Regards >> -- >> Kieran
Hi Jacopo, Thank you for the patch. On Tuesday, 11 December 2018 17:16:13 EET Jacopo Mondi wrote: > When the adv748x driver is informed about a link being created from HDMI or > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > sure to implement proper routing management at link setup time, to route > the selected video stream to the desired TX output. > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > drivers/media/i2c/adv748x/adv748x-core.c | 63 ++++++++++++++++++++++++++++- > drivers/media/i2c/adv748x/adv748x.h | 1 + > 2 files changed, 63 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c > b/drivers/media/i2c/adv748x/adv748x-core.c index f3aabbccdfb5..08dc0e89b053 > 100644 > --- a/drivers/media/i2c/adv748x/adv748x-core.c > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > /* ------------------------------------------------------------------------ > * Media Operations > */ > +static int adv748x_link_setup(struct media_entity *entity, > + const struct media_pad *local, > + const struct media_pad *remote, u32 flags) > +{ > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > + struct adv748x_csi2 *tx; > + struct media_link *link; > + u8 io10; > + > + /* > + * For each link setup from [HDMI|AFE] to TX we receive two > + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]". > + * > + * Use the second notification form to make sure we're linking > + * to a TX and find out from where, to set up routing properly. > + */ Why don't you implement the link handler just for the TX entities then ? > + if ((sd != &state->txa.sd && sd != &state->txb.sd) || > + !(flags & MEDIA_LNK_FL_ENABLED)) When disabling the link you should reset the ->source and ->tx pointers. > + return 0; > + tx = adv748x_sd_to_csi2(sd); > + > + /* > + * Now that we're sure we're operating on one of the two TXs, > + * make sure there are no enabled links ending there from > + * either HDMI or AFE (this can only happens for TXA though). > + */ > + if (is_txa(tx)) > + list_for_each_entry(link, &entity->links, list) > + if (link->sink->entity == entity && > + link->flags & MEDIA_LNK_FL_ENABLED) > + return -EINVAL; You can simplify this by checking if tx->source == NULL (after resetting tx- >source when disabling the link of course). > + /* Change video stream routing, according to the newly created link. */ > + io10 = io_read(state, ADV748X_IO_10); > + if (rsd == &state->afe.sd) { > + state->afe.tx = tx; > + > + /* > + * If AFE is routed to TXA, make sure TXB is off; > + * If AFE goes to TXB, we need TXA powered on. > + */ > + if (is_txa(tx)) { > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > + io10 &= ~ADV748X_IO_10_CSI1_EN; > + } else { > + io10 |= ADV748X_IO_10_CSI4_EN | > + ADV748X_IO_10_CSI1_EN; > + } > + } else { > + state->hdmi.tx = tx; > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > + } > + io_write(state, ADV748X_IO_10, io10); Is it guaranteed that the chip will be powered on at this point ? How about writing the register at stream on time instead ? > + tx->rsd = rsd; > + > + return 0; > +} > > static const struct media_entity_operations adv748x_media_ops = { > - .link_validate = v4l2_subdev_link_validate, > + .link_setup = adv748x_link_setup, > + .link_validate = v4l2_subdev_link_validate, > }; > > /* ------------------------------------------------------------------------ > -- diff --git a/drivers/media/i2c/adv748x/adv748x.h > b/drivers/media/i2c/adv748x/adv748x.h index 0ee3b8d5c795..63a17c31c169 > 100644 > --- a/drivers/media/i2c/adv748x/adv748x.h > +++ b/drivers/media/i2c/adv748x/adv748x.h > @@ -220,6 +220,7 @@ struct adv748x_state { > #define ADV748X_IO_10_CSI4_EN BIT(7) > #define ADV748X_IO_10_CSI1_EN BIT(6) > #define ADV748X_IO_10_PIX_OUT_EN BIT(5) > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE 0x08 > > #define ADV748X_IO_CHIP_REV_ID_1 0xdf > #define ADV748X_IO_CHIP_REV_ID_2 0xe0
Hi Laurent, Kieran, thanks for the review On Thu, Dec 13, 2018 at 11:40:00AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Tuesday, 11 December 2018 17:16:13 EET Jacopo Mondi wrote: > > When the adv748x driver is informed about a link being created from HDMI or > > AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make > > sure to implement proper routing management at link setup time, to route > > the selected video stream to the desired TX output. > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > > --- > > drivers/media/i2c/adv748x/adv748x-core.c | 63 ++++++++++++++++++++++++++++- > > drivers/media/i2c/adv748x/adv748x.h | 1 + > > 2 files changed, 63 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/adv748x/adv748x-core.c > > b/drivers/media/i2c/adv748x/adv748x-core.c index f3aabbccdfb5..08dc0e89b053 > > 100644 > > --- a/drivers/media/i2c/adv748x/adv748x-core.c > > +++ b/drivers/media/i2c/adv748x/adv748x-core.c > > @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) > > /* ------------------------------------------------------------------------ > > * Media Operations > > */ > > +static int adv748x_link_setup(struct media_entity *entity, > > + const struct media_pad *local, > > + const struct media_pad *remote, u32 flags) > > +{ > > + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); > > + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); > > + struct adv748x_state *state = v4l2_get_subdevdata(sd); > > + struct adv748x_csi2 *tx; > > + struct media_link *link; > > + u8 io10; > > + > > + /* > > + * For each link setup from [HDMI|AFE] to TX we receive two > > + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]". > > + * > > + * Use the second notification form to make sure we're linking > > + * to a TX and find out from where, to set up routing properly. > > + */ > > Why don't you implement the link handler just for the TX entities then ? > Done. Much better > > + if ((sd != &state->txa.sd && sd != &state->txb.sd) || > > + !(flags & MEDIA_LNK_FL_ENABLED)) > > When disabling the link you should reset the ->source and ->tx pointers. > right > > + return 0; > > + tx = adv748x_sd_to_csi2(sd); > > + > > + /* > > + * Now that we're sure we're operating on one of the two TXs, > > + * make sure there are no enabled links ending there from > > + * either HDMI or AFE (this can only happens for TXA though). > > + */ > > + if (is_txa(tx)) > > + list_for_each_entry(link, &entity->links, list) > > + if (link->sink->entity == entity && > > + link->flags & MEDIA_LNK_FL_ENABLED) > > + return -EINVAL; > > You can simplify this by checking if tx->source == NULL (after resetting tx- > >source when disabling the link of course). > > > + /* Change video stream routing, according to the newly created link. */ > > + io10 = io_read(state, ADV748X_IO_10); > > + if (rsd == &state->afe.sd) { > > + state->afe.tx = tx; > > + > > + /* > > + * If AFE is routed to TXA, make sure TXB is off; > > + * If AFE goes to TXB, we need TXA powered on. > > + */ > > + if (is_txa(tx)) { > > + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + io10 &= ~ADV748X_IO_10_CSI1_EN; > > + } else { > > + io10 |= ADV748X_IO_10_CSI4_EN | > > + ADV748X_IO_10_CSI1_EN; > > + } > > + } else { > > + state->hdmi.tx = tx; > > + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; > > + } > > + io_write(state, ADV748X_IO_10, io10); > > Is it guaranteed that the chip will be powered on at this point ? How about > writing the register at stream on time instead ? You know, I struggle to find a better place where to do so... - Right now the register gets written at reset time only - s_stream: Each subdev (afe, hdmi, txa and txb) has its own s_stream so it's hard to find a centralized place where to write this register that impacts both TXes - afe/hdmi s_stream() ops call tx_power_up, but again this is per-TX - There doesn't seems to be much concern in the driver in synchronizing register writes with power states already actually. Not that's an excuse, but when I played around and tried to power off the chip's PLLs for real I got into LP-11 state detection issues (again :) I would leave it here if that's fine with you :) Thanks j > > > + tx->rsd = rsd; > > + > > + return 0; > > +} > > > > static const struct media_entity_operations adv748x_media_ops = { > > - .link_validate = v4l2_subdev_link_validate, > > + .link_setup = adv748x_link_setup, > > + .link_validate = v4l2_subdev_link_validate, > > }; > > > > /* ------------------------------------------------------------------------ > > -- diff --git a/drivers/media/i2c/adv748x/adv748x.h > > b/drivers/media/i2c/adv748x/adv748x.h index 0ee3b8d5c795..63a17c31c169 > > 100644 > > --- a/drivers/media/i2c/adv748x/adv748x.h > > +++ b/drivers/media/i2c/adv748x/adv748x.h > > @@ -220,6 +220,7 @@ struct adv748x_state { > > #define ADV748X_IO_10_CSI4_EN BIT(7) > > #define ADV748X_IO_10_CSI1_EN BIT(6) > > #define ADV748X_IO_10_PIX_OUT_EN BIT(5) > > +#define ADV748X_IO_10_CSI4_IN_SEL_AFE 0x08 > > > > #define ADV748X_IO_CHIP_REV_ID_1 0xdf > > #define ADV748X_IO_CHIP_REV_ID_2 0xe0 > > -- > Regards, > > Laurent Pinchart > > >
diff --git a/drivers/media/i2c/adv748x/adv748x-core.c b/drivers/media/i2c/adv748x/adv748x-core.c index f3aabbccdfb5..08dc0e89b053 100644 --- a/drivers/media/i2c/adv748x/adv748x-core.c +++ b/drivers/media/i2c/adv748x/adv748x-core.c @@ -335,9 +335,70 @@ int adv748x_tx_power(struct adv748x_csi2 *tx, bool on) /* ----------------------------------------------------------------------------- * Media Operations */ +static int adv748x_link_setup(struct media_entity *entity, + const struct media_pad *local, + const struct media_pad *remote, u32 flags) +{ + struct v4l2_subdev *rsd = media_entity_to_v4l2_subdev(remote->entity); + struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity); + struct adv748x_state *state = v4l2_get_subdevdata(sd); + struct adv748x_csi2 *tx; + struct media_link *link; + u8 io10; + + /* + * For each link setup from [HDMI|AFE] to TX we receive two + * notifications: "[HDMI|AFE]->TX" and "TX<-[HDMI|AFE]". + * + * Use the second notification form to make sure we're linking + * to a TX and find out from where, to set up routing properly. + */ + if ((sd != &state->txa.sd && sd != &state->txb.sd) || + !(flags & MEDIA_LNK_FL_ENABLED)) + return 0; + tx = adv748x_sd_to_csi2(sd); + + /* + * Now that we're sure we're operating on one of the two TXs, + * make sure there are no enabled links ending there from + * either HDMI or AFE (this can only happens for TXA though). + */ + if (is_txa(tx)) + list_for_each_entry(link, &entity->links, list) + if (link->sink->entity == entity && + link->flags & MEDIA_LNK_FL_ENABLED) + return -EINVAL; + + /* Change video stream routing, according to the newly created link. */ + io10 = io_read(state, ADV748X_IO_10); + if (rsd == &state->afe.sd) { + state->afe.tx = tx; + + /* + * If AFE is routed to TXA, make sure TXB is off; + * If AFE goes to TXB, we need TXA powered on. + */ + if (is_txa(tx)) { + io10 |= ADV748X_IO_10_CSI4_IN_SEL_AFE; + io10 &= ~ADV748X_IO_10_CSI1_EN; + } else { + io10 |= ADV748X_IO_10_CSI4_EN | + ADV748X_IO_10_CSI1_EN; + } + } else { + state->hdmi.tx = tx; + io10 &= ~ADV748X_IO_10_CSI4_IN_SEL_AFE; + } + io_write(state, ADV748X_IO_10, io10); + + tx->rsd = rsd; + + return 0; +} static const struct media_entity_operations adv748x_media_ops = { - .link_validate = v4l2_subdev_link_validate, + .link_setup = adv748x_link_setup, + .link_validate = v4l2_subdev_link_validate, }; /* ----------------------------------------------------------------------------- diff --git a/drivers/media/i2c/adv748x/adv748x.h b/drivers/media/i2c/adv748x/adv748x.h index 0ee3b8d5c795..63a17c31c169 100644 --- a/drivers/media/i2c/adv748x/adv748x.h +++ b/drivers/media/i2c/adv748x/adv748x.h @@ -220,6 +220,7 @@ struct adv748x_state { #define ADV748X_IO_10_CSI4_EN BIT(7) #define ADV748X_IO_10_CSI1_EN BIT(6) #define ADV748X_IO_10_PIX_OUT_EN BIT(5) +#define ADV748X_IO_10_CSI4_IN_SEL_AFE 0x08 #define ADV748X_IO_CHIP_REV_ID_1 0xdf #define ADV748X_IO_CHIP_REV_ID_2 0xe0
When the adv748x driver is informed about a link being created from HDMI or AFE to a CSI-2 TX output, the 'link_setup()' callback is invoked. Make sure to implement proper routing management at link setup time, to route the selected video stream to the desired TX output. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- drivers/media/i2c/adv748x/adv748x-core.c | 63 +++++++++++++++++++++++++++++++- drivers/media/i2c/adv748x/adv748x.h | 1 + 2 files changed, 63 insertions(+), 1 deletion(-)