On 2023-08-30 01:29, Jani Nikula wrote:
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.
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.
