Re: [PATCH v3 03/13] drm: bridge: Link encoder and bridge in core code

[Archit Taneja <architt@codeaurora.org>]

On 11/30/2016 03:53 PM, Laurent Pinchart wrote:
> Hi Archit,
>
> On Wednesday 30 Nov 2016 10:35:02 Archit Taneja wrote:
>> On 11/29/2016 11:27 PM, Laurent Pinchart wrote:
>>> On Tuesday 29 Nov 2016 15:57:06 Archit Taneja wrote:
>>>> On 11/29/2016 02:34 PM, Laurent Pinchart wrote:
>>>>> Instead of linking encoders and bridges in every driver (and getting it
>>>>> wrong half of the time, as many drivers forget to set the drm_bridge
>>>>> encoder pointer), do so in core code. The drm_bridge_attach() function
>>>>> needs the encoder and optional previous bridge to perform that task,
>>>>> update all the callers.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <laurent.pinchart+renesas@ideasonboard.com>
>>>>> ---
>>>>>
>>>>>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  4 +-
>>>>>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |  4 +-
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c                   |  3 +-
>>>>>  drivers/gpu/drm/drm_bridge.c                       | 46 ++++++++++-----
>>>>>  drivers/gpu/drm/drm_simple_kms_helper.c            |  4 +-
>>>>>  drivers/gpu/drm/exynos/exynos_dp.c                 |  5 +--
>>>>>  drivers/gpu/drm/exynos/exynos_drm_dsi.c            |  6 +--
>>>>>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c          |  5 +--
>>>>>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c       |  5 +--
>>>>>  drivers/gpu/drm/imx/imx-ldb.c                      |  6 +--
>>>>>  drivers/gpu/drm/imx/parallel-display.c             |  4 +-
>>>>>  drivers/gpu/drm/mediatek/mtk_dpi.c                 |  8 ++--
>>>>>  drivers/gpu/drm/mediatek/mtk_dsi.c                 | 24 ++---------
>>>>>  drivers/gpu/drm/mediatek/mtk_hdmi.c                | 11 +++---
>>>>>  drivers/gpu/drm/msm/dsi/dsi_manager.c              | 17 +++++---
>>>>>  drivers/gpu/drm/msm/edp/edp_bridge.c               |  2 +-
>>>>>  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c             |  2 +-
>>>>>  drivers/gpu/drm/rcar-du/rcar_du_hdmienc.c          |  5 +--
>>>>>  drivers/gpu/drm/sti/sti_dvo.c                      |  3 +-
>>>>>  drivers/gpu/drm/sti/sti_hda.c                      |  3 +-
>>>>>  drivers/gpu/drm/sti/sti_hdmi.c                     |  3 +-
>>>>>  drivers/gpu/drm/sun4i/sun4i_rgb.c                  | 13 +++---
>>>>>  include/drm/drm_bridge.h                           |  3 +-
>>>>>  23 files changed, 83 insertions(+), 103 deletions(-)
>>>
>>> [snip]
>>>
>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>> index 0ee052b7c21a..850bd6509ef1 100644
>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>
>>> [snip]
>>>
>>>>> @@ -92,32 +93,53 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>>>>>  EXPORT_SYMBOL(drm_bridge_remove);
>>>>>
>>>>>  /**
>>>>> - * drm_bridge_attach - associate given bridge to our DRM device
>>>>> + * drm_bridge_attach - attach the bridge to an encoder's chain
>>>>>   *
>>>>> - * @dev: DRM device
>>>>> - * @bridge: bridge control structure
>>>>> + * @encoder: DRM encoder
>>>>> + * @bridge: bridge to attach
>>>>> + * @previous: previous bridge in the chain (optional)
>>>>>   *
>>>>> - * Called by a kms driver to link one of our encoder/bridge to the
>>>>> given
>>>>> - * bridge.
>>>>> + * Called by a kms driver to link the bridge to an encoder's chain. The
>>>>> previous
>>>>> + * argument specifies the previous bridge in the chain. If NULL, the
>>>>> bridge is
>>>>> + * linked directly at the encoder's output. Otherwise it is linked at
>>>>> the
>>>>> + * previous bridge's output.
>>>>>   *
>>>>> - * Note that setting up links between the bridge and our encoder/bridge
>>>>> - * objects needs to be handled by the kms driver itself.
>>>>> + * If non-NULL the previous bridge must be already attached by a call
>>>>> to this
>>>>> + * function.
>>>>>   *
>>>>>   * RETURNS:
>>>>>   * Zero on success, error code on failure
>>>>>   */
>>>>> -int drm_bridge_attach(struct drm_device *dev, struct drm_bridge
>>>>> *bridge)
>>>>> +int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge
>>>>> *bridge,
>>>>> +		      struct drm_bridge *previous)
>>>>>  {
>>>>> -	if (!dev || !bridge)
>>>>> +	int ret;
>>>>> +
>>>>> +	if (!encoder || !bridge)
>>>>> +		return -EINVAL;
>>>>
>>>> I think we could derive previous from the encoder itself. Something like:
>>>> 	previous = encoder->bridge;
>>>> 	while (previous && previous->next)
>>>> 	
>>>> 		previous = previous->next;
>>>
>>> That's a very good point. It would however prevent us from catching
>>> drivers that attach bridges in the wrong order, which the !previous->dev
>>> currently allows us to do (and it should be turned into a WARN_ON as
>>> Daniel proposed).
>>
>> With the simpler API, I don't think we will ever hit the case of
>> !previous->dev. The previous bridge (if it exists) in the chain would
>> already have a dev attached to it. In other words, we would remove the
>> risk of the chance of the 'previous' bridge being unattached.
>>
>> I'm a bit unclear about what you mean about the order part. If a kms driver
>> wants to create a chain: encoder->bridge1->bridge2, it should ideally do:
>>
>> drm_bridge_attach(encoder, bridge1, NULL);
>> drm_bridge_attach(encoder, bridge2, bridge1);
>
> Correct.
>
>> We can't do much if the kms driver does the opposite:
>>
>> drm_bridge_attach(encoder, bridge2, NULL);
>> drm_bridge_attach(encoder, bridge2, bridge1);
>
> That would certainly be a very stupid thing for a driver to do :-) The problem
> that we could catch with my current proposal is
>
> drm_bridge_attach(encoder, bridge2, bridge1);
> ...
> drm_bridge_attach(encoder, bridge1, NULL);
>
> which I expect to happen from time to time as the two bridge can be attached
> through separate code paths sometimes a bit difficult to trace. It's not a big
> deal though, you could convince me that the advantages of a simpler API exceed
> its drawbacks.

Having no 'previous' argument would prevent the possibility of this altogether,
won't it?

With no 'previous' arg in the API, the driver can only do:

drm_bridge_attach(encoder, bridge1);
drm_bridge_attach(encoder, bridge2);

or

drm_bridge_attach(encoder, bridge2);
drm_bridge_attach(encoder, bridge1);

For the latter, we can't do much as discussed above.

Archit

>
>>> I'm fine losing that ability, as your proposal makes the API simpler. I'll
>>> let you decide, which option do you prefer ?
>>
>> I prefer the simpler API. I guess the main aim of the patch was to prevent
>> the driver setting up the encoder<->bridge links, which will be done
>> anyway.
>
> If you still prefer the simpler API after reading the above, I'll update my
> patch.
>
>>>>> +
>>>>> +	if (previous && (!previous->dev || previous->encoder != encoder))
>>>>>  		return -EINVAL;
>>>>>  	
>>>>>  	if (bridge->dev)
>>>>>  		return -EBUSY;
>>>>>
>>>>> -	bridge->dev = dev;
>>>>> +	bridge->dev = encoder->dev;
>>>>> +	bridge->encoder = encoder;
>>>>> +
>>>>> +	if (bridge->funcs->attach) {
>>>>> +		ret = bridge->funcs->attach(bridge);
>>>>> +		if (ret < 0) {
>>>>> +			bridge->dev = NULL;
>>>>> +			bridge->encoder = NULL;
>>>>> +			return ret;
>>>>> +		}
>>>>> +	}
>>>>>
>>>>> -	if (bridge->funcs->attach)
>>>>> -		return bridge->funcs->attach(bridge);
>>>>> +	if (previous)
>>>>> +		previous->next = bridge;
>>>>> +	else
>>>>> +		encoder->bridge = bridge;
>>>>>
>>>>>  	return 0;
>>>>>  }
>>>>
>>>> <snip>
>

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