Re: [PATCH v2 6/8] drm: Introduce drm_bridge_mode_valid()

[Archit Taneja <architt@codeaurora.org>]

On 05/12/2017 04:31 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> On Friday 12 May 2017 16:20:07 Archit Taneja wrote:
>> On 05/12/2017 03:08 PM, Laurent Pinchart wrote:
>>> On Wednesday 10 May 2017 17:14:33 Daniel Vetter wrote:
>>>> On Wed, May 10, 2017 at 04:41:09PM +0300, Ville Syrjälä wrote:
>>>>> On Tue, May 09, 2017 at 06:00:13PM +0100, Jose Abreu wrote:
>>>>>> Introduce a new helper function which calls mode_valid() callback
>>>>>> for all bridges in an encoder chain.
>>>>>>
>>>>>> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
>>>>>> Cc: Carlos Palminha <palminha@synopsys.com>
>>>>>> Cc: Alexey Brodkin <abrodkin@synopsys.com>
>>>>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>> Cc: Dave Airlie <airlied@linux.ie>
>>>>>> Cc: Andrzej Hajda <a.hajda@samsung.com>
>>>>>> Cc: Archit Taneja <architt@codeaurora.org>
>>>>>> ---
>>>>>>
>>>>>>  drivers/gpu/drm/drm_bridge.c | 33 +++++++++++++++++++++++++++++++++
>>>>>>  include/drm/drm_bridge.h     |  2 ++
>>>>>>  2 files changed, 35 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c
>>>>>> b/drivers/gpu/drm/drm_bridge.c
>>>>>> index 86a7637..dc8cdfe 100644
>>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>>> @@ -206,6 +206,39 @@ bool drm_bridge_mode_fixup(struct drm_bridge
>>>>>> *bridge,
>>>>>>
>>>>>>  EXPORT_SYMBOL(drm_bridge_mode_fixup);
>>>>>>
>>>>>>  /**
>>>>>>
>>>>>> + * drm_bridge_mode_valid - validate the mode against all bridges in
>>>>>> the
>>>>>> + * 			   encoder chain.
>>>>>> + * @bridge: bridge control structure
>>>>>> + * @mode: desired mode to be validated
>>>>>> + *
>>>>>> + * Calls &drm_bridge_funcs.mode_valid for all the bridges in the
>>>>>> encoder
>>>>>> + * chain, starting from the first bridge to the last. If at least one
>>>>>> bridge + * does not accept the mode the function returns the error
>>>>>> code.
>>>>>> + *
>>>>>> + * Note: the bridge passed should be the one closest to the encoder.
>>>>>> + *
>>>>>> + * RETURNS:
>>>>>> + * MODE_OK on success, drm_mode_status Enum error code on failure
>>>>>> + */
>>>>>> +enum drm_mode_status drm_bridge_mode_valid(struct drm_bridge *bridge,
>>>>>> +					   const struct
> drm_display_mode
>>>
>>> *mode)
>>>
>>>>>> +{
>>>>>> +	enum drm_mode_status ret = MODE_OK;
>>>>>> +
>>>>>> +	if (!bridge)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (bridge->funcs->mode_valid)
>>>>>> +		ret = bridge->funcs->mode_valid(bridge, mode);
>>>>>> +
>>>>>> +	if (ret != MODE_OK)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	return drm_bridge_mode_valid(bridge->next, mode);
>>>>>
>>>>> Looks like it should be pretty trivial to avoid the recursion.
>>>>>
>>>>> Am I correct in interpreting this that bridges have some kind of
>>>>> a hand rolled linked list implementation? Reusing the standard
>>>>> linked lists would allow you to use list_for_each() etc.
>>>>
>>>> Yeah it's a hand-rolled list, but current hw also has a bridge nesting
>>>> depth of 2, so it really doesn't matter. I guess once we have real long
>>>> chains of bridges we can fix this (and just using list_head sounds like a
>>>> great idea).
>>>
>>> Even if not really needed right now, it's a pretty easy cleanup, if Jose
>>> has time to handle it in v3 of this series let's not postpone it ;-)
>>
>> jfyi, some of the bridge functions call the ops from the last bridge in the
>> chain to first, so we'd need to use list_for_each_entry_prev() (or something
>> like that) for them.
>
> And now that I think about it, for some of the operations (especially
> enable/disable) I believe that the bridge should be able to decide whether to
> call the next/previous bridge first or to configure its hardware first. I can
> image bridges that need the previous bridge in the chain to provide a valid
> clock before they get started, as well as bridges that need to be started with
> the incoming video signal stopped.

I guess converting into list would be a good start to achieve this. We'd probably
need to extend/redo the drm_bridge_attach() API to tweak the order in the which
the ops are called.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project