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@xxxxxxxxxx>
> > ---
> > 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
Attachment:
signature.asc
Description: PGP signature
