Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <4171ca35-3d82-eaa3-5812-9c038f1c0415@baylibre.com>
Date:   Mon, 7 Feb 2022 09:22:30 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH v3] drm/bridge: dw-hdmi: use safe format when first in
 bridge chain
Content-Language: en-US
To:     Sam Ravnborg <sam@ravnborg.org>
Cc:     robert.foss@linaro.org, jernej.skrabec@gmail.com, jonas@kwiboo.se,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        kieran.bingham@ideasonboard.com,
        Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>,
        Laurent.pinchart@ideasonboard.com, biju.das.jz@bp.renesas.com
References: <20220204143337.89221-1-narmstrong@baylibre.com>
 <YgA+lcSNaQ2x4Wef@ravnborg.org>
From:   Neil Armstrong <narmstrong@baylibre.com>
Organization: Baylibre
In-Reply-To: <YgA+lcSNaQ2x4Wef@ravnborg.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Sam,

On 06/02/2022 22:33, Sam Ravnborg wrote:
> Hi Neail,
> 
> On Fri, Feb 04, 2022 at 03:33:37PM +0100, Neil Armstrong wrote:
>> When the dw-hdmi bridge is in first place of the bridge chain, this
>> means there is no way to select an input format of the dw-hdmi HW
>> component.
>>
>> Since introduction of display-connector, negotiation was broken since
>> the dw-hdmi negotiation code only worked when the dw-hdmi bridge was
>> in last position of the bridge chain or behind another bridge also
>> supporting input & output format negotiation.
>>
>> Commit 7cd70656d128 ("drm/bridge: display-connector: implement bus fmts callbacks")
>> was introduced to make negotiation work again by making display-connector
>> act as a pass-through concerning input & output format negotiation.
>>
>> But in the case where the dw-hdmi is single in the bridge chain, for
>> example on Renesas SoCs, with the display-connector bridge the dw-hdmi
>> is no more single, breaking output format.
> 
> I have forgotten all the details during my leave from drm, so I
> may miss something obvious.
> This fix looks like it papers over some general thingy with the
> format negotiation.
> 
> We do not want to see the below in all display drivers, so
> I assume the right fix is to stuff it in somewhere in the framework.

The main issue is there is rule about the encoder in display driver having
a companion bridge to support format negotiation.

To solve this cleanly, the first bridge tied to an encoder should register
with some caps or flags.

For now very few bridge supports negotiation so the rules yet needs to be defined.

Since we are getting into a better support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
clarifies the bridge chain, we should have more cards in our hand in a near future.

Anyway, in the meantime there is no fix in the framework for this case.

Neil

> 
> Or do I miss something?
> 
> 	Sam
> 
> 
>>
>> Reported-by: Biju Das <biju.das.jz@bp.renesas.com>
>> Bisected-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> Fixes: 7cd70656d128 ("drm/bridge: display-connector: implement bus fmts callbacks").
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> Reviewed-by: Robert Foss <robert.foss@linaro.org>
>> ---
>> Changes since v2:
>> - Add rob's r-b
>> - Fix invalid Fixes commit hash
>>
>> Changes since v1:
>> - Remove bad fix in dw_hdmi_bridge_atomic_get_input_bus_fmts
>> - Fix typos in commit message
>>
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 54d8fdad395f..97cdc61b57f6 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -2551,8 +2551,9 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>>   	if (!output_fmts)
>>   		return NULL;
>>   
>> -	/* If dw-hdmi is the only bridge, avoid negociating with ourselves */
>> -	if (list_is_singular(&bridge->encoder->bridge_chain)) {
>> +	/* If dw-hdmi is the first or only bridge, avoid negociating with ourselves */
>> +	if (list_is_singular(&bridge->encoder->bridge_chain) ||
>> +	    list_is_first(&bridge->chain_node, &bridge->encoder->bridge_chain)) {
>>   		*num_output_fmts = 1;
>>   		output_fmts[0] = MEDIA_BUS_FMT_FIXED;
>>   
>> -- 
>> 2.25.1