Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230120114313.2087015-1-john@metanate.com>
In-Reply-To: <20230120114313.2087015-1-john@metanate.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 20 Jan 2023 13:44:38 -0800
Message-ID: <CAD=FV=UPD6c+NY8Ub37N7LmrRFpcr6gKOh0Os14DaKrf3bKo2A@mail.gmail.com>
Subject: Re: [PATCH] drm/bridge: panel: Set orientation on panel_bridge connector
To:     John Keeping <john@metanate.com>
Cc:     dri-devel@lists.freedesktop.org,
        Andrzej Hajda <andrzej.hajda@intel.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        Jonas Karlman <jonas@kwiboo.se>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Stephen Boyd <swboyd@chromium.org>,
        Hsin-Yi Wang <hsinyi@chromium.org>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Fri, Jan 20, 2023 at 3:43 AM John Keeping <john@metanate.com> wrote:
>
> Commit 15b9ca1641f0 ("drm: Config orientation property if panel provides
> it") added a helper to set the panel panel orientation early but only
> connected this for drm_bridge_connector, which constructs a panel bridge
> with DRM_BRIDGE_ATTACH_NO_CONNECTOR and creates the connector itself.
>
> When the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is not specified and the
> panel_bridge creates its own connector the orientation is not set unless
> the panel does it in .get_modes which is too late and leads to a warning
> splat from __drm_mode_object_add() because the device is already
> registered.
>
> Call the necessary function to set add the orientation property when the
> connector is created so that it is available before the device is
> registered.

I have no huge objection to your patch and it looks OK to me. That
being said, my understanding is that:

1. DRM_BRIDGE_ATTACH_NO_CONNECTOR is "the future" and not using the
flag is "deprecated".

2. In general, if something about the deprecated method used to work
and a patch broke it then we should fix it until we can finish fully
deprecating. However, we should avoid adding new features to the old
deprecated method and instead encourage people to move to "the
future".

3. I don't think any of the orientation patches broke the deprecated
path. Before those patches, nothing used to configure the orientation
property properly. After those patches, "the future" method (AKA
DRM_BRIDGE_ATTACH_NO_CONNECTOR) did configure the orientation property
properly.

...so by those arguments I would say that we shouldn't land your patch
and that instead you should work to get your drivers moving to
DRM_BRIDGE_ATTACH_NO_CONNECTOR.

Now, all that being said, your patch adds one line of code and really
doesn't seem like a big deal. I'd personally be OK with landing it,
but I'd prefer to hear an opinion from someone more senior in the DRM
world before doing so. I'm still fairly low on the totem pole. ;-)


> Fixes: 15b9ca1641f0 ("drm: Config orientation property if panel provides it")

Maybe remove the "Fixes" tag here. That patch didn't break you, right?
It just didn't happen to _also_ fix you.