On Tue, 29 Aug 2023, Alex Hung <alex.hung@xxxxxxx> wrote: > On 2023-08-29 11:03, Jani Nikula wrote: >> On Tue, 29 Aug 2023, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: >>> On Tue, 29 Aug 2023, Alex Deucher <alexdeucher@xxxxxxxxx> wrote: >>>> On Tue, Aug 29, 2023 at 6:48 AM Jani Nikula <jani.nikula@xxxxxxxxx> wrote: >>>>> >>>>> On Wed, 23 Aug 2023, Jani Nikula <jani.nikula@xxxxxxxxx> wrote: >>>>>> On Tue, 22 Aug 2023, Alex Hung <alex.hung@xxxxxxx> 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. 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. >> -- Jani Nikula, Intel Open Source Graphics Center
