| Message ID | cover.1692705543.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/amd/display: stop using drm_edid_override_connector_update() | expand |
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. Cheers, Alex > > BR, > Jani. > > > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Alex Hung <alex.hung@amd.com> > Cc: Chao-kai Wang <Stylon.Wang@amd.com> > Cc: Daniel Wheeler <daniel.wheeler@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Hersen Wu <hersenxs.wu@amd.com> > Cc: Leo Li <sunpeng.li@amd.com> > Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> > Cc: Wenchieh Chien <wenchieh.chien@amd.com> > Cc: David Airlie <airlied@gmail.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > > Jani Nikula (4): > Revert "drm/amd/display: drop unused count variable in > create_eml_sink()" > Revert "drm/amd/display: assign edid_blob_ptr with edid from debugfs" > Revert "drm/amd/display: mark amdgpu_dm_connector_funcs_force static" > Revert "drm/amd/display: implement force function in > amdgpu_dm_connector_funcs" > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++---------------- > 1 file changed, 5 insertions(+), 39 deletions(-) >
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. BR, Jani. > > Cheers, > Alex > >> >> BR, >> Jani. >> >> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Cc: Alex Hung <alex.hung@amd.com> >> Cc: Chao-kai Wang <Stylon.Wang@amd.com> >> Cc: Daniel Wheeler <daniel.wheeler@amd.com> >> Cc: Harry Wentland <harry.wentland@amd.com> >> Cc: Hersen Wu <hersenxs.wu@amd.com> >> Cc: Leo Li <sunpeng.li@amd.com> >> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> >> Cc: Wenchieh Chien <wenchieh.chien@amd.com> >> Cc: David Airlie <airlied@gmail.com> >> Cc: Daniel Vetter <daniel@ffwll.ch> >> >> Jani Nikula (4): >> Revert "drm/amd/display: drop unused count variable in >> create_eml_sink()" >> Revert "drm/amd/display: assign edid_blob_ptr with edid from debugfs" >> Revert "drm/amd/display: mark amdgpu_dm_connector_funcs_force static" >> Revert "drm/amd/display: implement force function in >> amdgpu_dm_connector_funcs" >> >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++---------------- >> 1 file changed, 5 insertions(+), 39 deletions(-) >>
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? BR, Jani. > > BR, > Jani. > > >> >> Cheers, >> Alex >> >>> >>> BR, >>> Jani. >>> >>> >>> Cc: Alex Deucher <alexander.deucher@amd.com> >>> Cc: Alex Hung <alex.hung@amd.com> >>> Cc: Chao-kai Wang <Stylon.Wang@amd.com> >>> Cc: Daniel Wheeler <daniel.wheeler@amd.com> >>> Cc: Harry Wentland <harry.wentland@amd.com> >>> Cc: Hersen Wu <hersenxs.wu@amd.com> >>> Cc: Leo Li <sunpeng.li@amd.com> >>> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> >>> Cc: Wenchieh Chien <wenchieh.chien@amd.com> >>> Cc: David Airlie <airlied@gmail.com> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> >>> Jani Nikula (4): >>> Revert "drm/amd/display: drop unused count variable in >>> create_eml_sink()" >>> Revert "drm/amd/display: assign edid_blob_ptr with edid from debugfs" >>> Revert "drm/amd/display: mark amdgpu_dm_connector_funcs_force static" >>> Revert "drm/amd/display: implement force function in >>> amdgpu_dm_connector_funcs" >>> >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++---------------- >>> 1 file changed, 5 insertions(+), 39 deletions(-) >>>
[AMD Official Use Only - General] + Charlie -----Original Message----- From: Jani Nikula <jani.nikula@intel.com> Sent: Tuesday, August 29, 2023 6:49 AM To: Hung, Alex <Alex.Hung@amd.com>; dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org Cc: Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; David Airlie <airlied@gmail.com>; intel-gfx@lists.freedesktop.org; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Wheeler, Daniel <Daniel.Wheeler@amd.com>; Wu, Hersen <hersenxs.wu@amd.com>; Daniel Vetter <daniel@ffwll.ch>; Chien, WenChieh (Jay) <WenChieh.Chien@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Wentland, Harry <Harry.Wentland@amd.com> Subject: Re: [Intel-gfx] [PATCH 0/4] drm/amd/display: stop using drm_edid_override_connector_update() 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? BR, Jani. > > BR, > Jani. > > >> >> Cheers, >> Alex >> >>> >>> BR, >>> Jani. >>> >>> >>> Cc: Alex Deucher <alexander.deucher@amd.com> >>> Cc: Alex Hung <alex.hung@amd.com> >>> Cc: Chao-kai Wang <Stylon.Wang@amd.com> >>> Cc: Daniel Wheeler <daniel.wheeler@amd.com> >>> Cc: Harry Wentland <harry.wentland@amd.com> >>> Cc: Hersen Wu <hersenxs.wu@amd.com> >>> Cc: Leo Li <sunpeng.li@amd.com> >>> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> >>> Cc: Wenchieh Chien <wenchieh.chien@amd.com> >>> Cc: David Airlie <airlied@gmail.com> >>> Cc: Daniel Vetter <daniel@ffwll.ch> >>> >>> Jani Nikula (4): >>> Revert "drm/amd/display: drop unused count variable in >>> create_eml_sink()" >>> Revert "drm/amd/display: assign edid_blob_ptr with edid from debugfs" >>> Revert "drm/amd/display: mark amdgpu_dm_connector_funcs_force static" >>> Revert "drm/amd/display: implement force function in >>> amdgpu_dm_connector_funcs" >>> >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++---------------- >>> 1 file changed, 5 insertions(+), 39 deletions(-) >>> -- Jani Nikula, Intel Open Source Graphics Center
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. Alex
[AMD Official Use Only - General] + Charlie Wang -----Original Message----- From: Alex Deucher <alexdeucher@gmail.com> Sent: Tuesday, August 29, 2023 11:44 AM To: Jani Nikula <jani.nikula@intel.com> Cc: Hung, Alex <Alex.Hung@amd.com>; dri-devel@lists.freedesktop.org; amd-gfx@lists.freedesktop.org; Li, Sun peng (Leo) <Sunpeng.Li@amd.com>; intel-gfx@lists.freedesktop.org; Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; Wheeler, Daniel <Daniel.Wheeler@amd.com>; Wu, Hersen <hersenxs.wu@amd.com>; Chien, WenChieh (Jay) <WenChieh.Chien@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com> Subject: Re: [Intel-gfx] [PATCH 0/4] drm/amd/display: stop using drm_edid_override_connector_update() 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. Alex
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. 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.
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.
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. 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. >
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. 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. >>
On Wed, Aug 30, 2023 at 10:29:46AM +0300, Jani Nikula wrote:
> Upstream code should be reviewed in public.
Yup
-Sima
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. >>> >
On Thu, 31 Aug 2023, Alex Hung <alex.hung@amd.com> wrote: > On 2023-08-30 01:29, Jani Nikula wrote: >> On Tue, 29 Aug 2023, Alex Hung <alex.hung@amd.com> wrote: >>> 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. The point about public review is that there's no transparency to the steps before 4. The patches are posted for public review with Reviewed-by and Acked-by already added, based on internal review, and there is de facto no public review taking place on the code drops. There is zero visibility to the discussions taking place. We don't know if it's just rubber stamping, we don't know what concerns were raised, if any. I'm mainly disappointed about the double standards here, given that we post most patches directly upstream (especially ones that have zero reason to be embargoed like the ones being discussed here), and the ones that have gone through internal review will be stripped of all prior internal Reviewed-by's and Acked-by's before posting. Because that's the upstream expectation. BR, Jani.
On Thu, Aug 31, 2023 at 6:01 PM Alex Hung <alex.hung@amd.com> wrote: > > > > 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. > This sort of thing is fine for unreleased chips or new IP prior public exposure, but for released hardware, you really need to do the reviews on the mailing lists. Alex > > > > > > 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. > >>> > >
On Fri, 1 Sept 2023 at 21:00, Alex Deucher <alexdeucher@gmail.com> wrote: > > On Thu, Aug 31, 2023 at 6:01 PM Alex Hung <alex.hung@amd.com> wrote: > > > > > > > > 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. > > > > This sort of thing is fine for unreleased chips or new IP prior public > exposure, but for released hardware, you really need to do the reviews > on the mailing lists. Aye. Maybe with the clarification that if the embargoed code touches areas that are common code (or really should be handled in common code), then the cross-driver parts also need to be reviewed in public as upfront prep patches. If that's not possible (try to fix your process to make that possible please), at least ping stakeholders in private to give them a heads up, so that when the IP enabling gets published it's not going to be held up in the review for the necessary common changes. What's not good is if code that should be reviewed on dri-devel bypasses all that just because it's part of a hardware enabling series. Cheers, Sima > Alex > > > > > > > > > > > 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. > > >>> > > >
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. BR, Jani. Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Alex Hung <alex.hung@amd.com> Cc: Chao-kai Wang <Stylon.Wang@amd.com> Cc: Daniel Wheeler <daniel.wheeler@amd.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Hersen Wu <hersenxs.wu@amd.com> Cc: Leo Li <sunpeng.li@amd.com> Cc: Rodrigo Siqueira <Rodrigo.Siqueira@amd.com> Cc: Wenchieh Chien <wenchieh.chien@amd.com> Cc: David Airlie <airlied@gmail.com> Cc: Daniel Vetter <daniel@ffwll.ch> Jani Nikula (4): Revert "drm/amd/display: drop unused count variable in create_eml_sink()" Revert "drm/amd/display: assign edid_blob_ptr with edid from debugfs" Revert "drm/amd/display: mark amdgpu_dm_connector_funcs_force static" Revert "drm/amd/display: implement force function in amdgpu_dm_connector_funcs" .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 44 +++---------------- 1 file changed, 5 insertions(+), 39 deletions(-)