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

[Alex Hung <alex.hung@amd.com>]

On 2023-08-30 01:29, Jani Nikula wrote:
> On Tue, 29 Aug 2023, Alex Hung <alex.hung@amd.com> wrote:
>> On 2023-08-29 11:03, Jani Nikula wrote:
>>> 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?
>>
>> There is a patch under internal reviews. It removes calls edid_override
>> and drm_edid_override_connector_update as intended in this patchset but
>> does not remove the functionality.
> 
> While I am happy to hear there's progress, I'm somewhat baffled the
> review is internal. The commits that I suggested to revert were also
> only reviewed internally, as far as I can see... And that's kind of the
> problem.
> 
> Upstream code should be reviewed in public.

Hi Jani,

All patches are sent for public reviews, the progress is summarized as 
the followings:

== internal ==

1. a patch or patches are tested by CI.
2. internal technical and IP reviews are performed to ensure no concerns 
before patches are merged to internal branch.

== public ==

3. a regression test and IP reviews are performed by engineers before 
sending to public mailing lists.
4. the patchset is sent for public reviews ex. 
https://patchwork.freedesktop.org/series/122498/
5. patches are merged to public repo.

> 
> 
> BR,
> Jani.
> 
> 
>>
>> With the patch. both following git grep commands return nothing in
>> amd-staging-drm-next.
>>
>> $ git grep drm_edid_override_connector_update -- drivers/gpu/drm/amd
>> $ git grep edid_override -- drivers/gpu/drm/amd
>>
>> Best regards,
>> Alex Hung
>>
>>>>>
>>>>> 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.
>>>
>