Re: [Intel-gfx] [PATCH 0/4] drm/amd/display: stop using drm_edid_override_connector_update()

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 29 Aug 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> On Tue, 29 Aug 2023, Alex Deucher <alexdeucher@gmail.com> wrote:
>> On Tue, Aug 29, 2023 at 6:48 AM Jani Nikula <jani.nikula@intel.com> wrote:
>>>
>>> On Wed, 23 Aug 2023, Jani Nikula <jani.nikula@intel.com> wrote:
>>> > On Tue, 22 Aug 2023, Alex Hung <alex.hung@amd.com> wrote:
>>> >> On 2023-08-22 06:01, Jani Nikula wrote:
>>> >>> Over the past years I've been trying to unify the override and firmware
>>> >>> EDID handling as well as EDID property updates. It won't work if drivers
>>> >>> do their own random things.
>>> >> Let's check how to replace these references by appropriate ones or fork
>>> >> the function as reverting these patches causes regressions.
>>> >
>>> > I think the fundamental problem you have is conflating connector forcing
>>> > with EDID override. They're orthogonal. The .force callback has no
>>> > business basing the decisions on connector->edid_override. Force is
>>> > force, override is override.
>>> >
>>> > The driver isn't even supposed to know or care if the EDID originates
>>> > from the firmware loader or override EDID debugfs. drm_get_edid() will
>>> > handle that for you transparently. It'll return the EDID, and you
>>> > shouldn't look at connector->edid_blob_ptr either. Using that will make
>>> > future work in drm_edid.c harder.
>>> >
>>> > You can't fix that with minor tweaks. I think you'll be better off
>>> > starting from scratch.
>>> >
>>> > Also, connector->edid_override is debugfs. You actually can change the
>>> > behaviour. If your userspace, whatever it is, has been written to assume
>>> > connector forcing if EDID override is set, you *do* have to fix that,
>>> > and set both.
>>>
>>> Any updates on fixing this, or shall we proceed with the reverts?
>>
>> What is the goal of the reverts?  I don't disagree that we may be
>> using the interfaces wrong, but reverting them will regess
>> functionality in the driver.
>
> The commits are in v6.5-rc1, but not yet in a release. No user depends
> on them yet. I'd strongly prefer them not reaching v6.5 final and users.

Sorry for confusion here, that's obviously come and gone already. :(

> The firmware EDID, override EDID, connector forcing, the EDID property,
> etc. have been and somewhat still are a hairy mess that we must keep
> untangling, and this isn't helping.
>
> I've put in crazy amounts of work on this, and I've added kernel-doc
> comments about stuff that should and should not be done, but they go
> unread and ignored.
>
> I really don't want to end up having to clean this up myself before I
> can embark on further cleanups and refactoring.
>
> And again, if the functionality in the driver depends on conflating two
> things that should be separate, it's probably not such a hot idea to let
> it reach users either. Even if it's just debugfs.
>
>
> BR,
> Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center