Re: [PATCH 5/5] media: adv748x: Implement link_setup callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux