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@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.
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
--
Regards
--
Kieran
