From: Doug Anderson <dianders@chromium.org> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: Stephen Boyd <swboyd@chromium.org>, Sankeerth Billakanti <quic_sbillaka@quicinc.com>, dri-devel <dri-devel@lists.freedesktop.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, freedreno <freedreno@lists.freedesktop.org>, LKML <linux-kernel@vger.kernel.org>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Rob Clark <robdclark@gmail.com>, Sean Paul <seanpaul@chromium.org>, quic_kalyant <quic_kalyant@quicinc.com>, Abhinav Kumar <quic_abhinavk@quicinc.com>, Kuogee Hsieh <quic_khsieh@quicinc.com>, Bjorn Andersson <bjorn.andersson@linaro.org>, Sean Paul <sean@poorly.run>, David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, quic_vproddut <quic_vproddut@quicinc.com>, Aravind Venkateswaran <quic_aravindh@quicinc.com>, Steev Klimaszewski <steev@kali.org> Subject: Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus Date: Thu, 14 Apr 2022 14:51:15 -0700 [thread overview] Message-ID: <CAD=FV=XjVb7tP4acjOQgg2_8oCbOxqTopJE1PUKdf2noQCHg=Q@mail.gmail.com> (raw) In-Reply-To: <56453228-d4b2-c7e4-7b72-6de8637f2def@linaro.org> Hi, On Thu, Apr 14, 2022 at 2:16 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work? > > It's definitely worth confirming but from my reading of the code it > > _probably_ wouldn't hurt. > > > > One thing someone would want to confirm would be what would happen if > > we move this code and the panel code to implement DRM_BRIDGE_OP_EDID > > properly. It looks as if both actually ought to be implementing that > > instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a > > future day. Could we get into trouble if one moved before the other? > > Then the panel would no longer override the eDP controller and the eDP > > controller would try to read from a possibly unpowered panel? > > That would depend on the way the get_edid would be implemented in DP > driver. Currently the edid is cached via the > dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain. > > With this patchset, the dp_hpd_plug_handle() -> > dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be > called too late for the get_modes/get_edid (from dp_bridge's enable() op). > > There is another issue. drm_panel has only get_modes() callback, so > panel_bridge can not implement get_edid() unless we extend the panel > interface (which might be a good idea). Ah, that makes sense and explains why the current panel code does the EDID reading in its get_modes() function even though get_modes() is _documented_ that it doesn't read the EDID. ;-) I guess it's another of the "let's move some people over to the new way but we'll keep the old code working". Definitely makes it hard to understand at times. > > For hotplug/detect I'm even less confident that setting the bits would > > be harmless. I haven't sat down and traced everything, but from what I > > can see the panel _doesn't_ set these bits, does it? I believe that > > the rule is that when every bridge in the chain _doesn't_ implement > > detect/hotplug that the panel is always present. The moment someone > > says "hey, I can detect" then it suddenly becomes _not_ always > > present. Yes, I guess we could have the panel implement "detect" and > > return true, but I'm not convinced that's actually better... > > I think it makes sense to implement OP_DETECT in panel bridge (that > always returns connector_status_connected) at least to override the > possible detect ops in previous bridges. So I truly don't know the right answer, but are you sure that's the best design? I _think_ that panel_bridge is used for all kinds of panels, right? So what if there's some type of display that uses a panel but there's still a mechanism that supports physical detection of the panel? By implementing "detect" in the generic panel_bridge then you're _preventing_ anyone higher up in the chain from implementing it and you're forcing it to be "always connected". For instance, we could come up with a new display standard called "pluggable eDP" that is just like eDP except that you can physically detect it. This imaginary new display standard is different from DP because it has eDP power sequencing (fully powers the display off when the screen is off) but it's hot pluggable! It introduces a new pin that goes to the DP controller called RT-HPD for "really, truly hot plug detect" that works even when the panel is off. The existing "HPD" pin continues to mean that the panel is read to communicate. If the drm_panel hardcodes "always connected" then I can't implement my "pluggable eDP" system, right? However, if we leave it just like it is today then my new system would be easy to implement. ;-) The above example is obviously not truly a real one but I guess my point is that I find it more intuitive / useful to say that we should only implement "detect" if we truly think we can detect and that if nobody says they can detect then we must be always connected. As an aside; I think in general it's not always easy to fit every possible graphics system into these "bridge chains" and the simple sequence of pre-enable, enable, etc, so we have to do our best and accept the fact that sometimes we'll need special cases. Dave Stephenson's patches [1] should tell us that, at least. [1] https://lore.kernel.org/all/cover.1646406653.git.dave.stevenson@raspberrypi.com/ -Doug
WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders@chromium.org> To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Cc: quic_kalyant <quic_kalyant@quicinc.com>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Sankeerth Billakanti <quic_sbillaka@quicinc.com>, Abhinav Kumar <quic_abhinavk@quicinc.com>, quic_vproddut <quic_vproddut@quicinc.com>, David Airlie <airlied@linux.ie>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, LKML <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Stephen Boyd <swboyd@chromium.org>, Sean Paul <sean@poorly.run>, Sean Paul <seanpaul@chromium.org>, Steev Klimaszewski <steev@kali.org>, Aravind Venkateswaran <quic_aravindh@quicinc.com>, Kuogee Hsieh <quic_khsieh@quicinc.com>, freedreno <freedreno@lists.freedesktop.org> Subject: Re: [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus Date: Thu, 14 Apr 2022 14:51:15 -0700 [thread overview] Message-ID: <CAD=FV=XjVb7tP4acjOQgg2_8oCbOxqTopJE1PUKdf2noQCHg=Q@mail.gmail.com> (raw) In-Reply-To: <56453228-d4b2-c7e4-7b72-6de8637f2def@linaro.org> Hi, On Thu, Apr 14, 2022 at 2:16 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > Hmm, interesting. Probably for DRM_BRIDGE_OP_MODES that will work? > > It's definitely worth confirming but from my reading of the code it > > _probably_ wouldn't hurt. > > > > One thing someone would want to confirm would be what would happen if > > we move this code and the panel code to implement DRM_BRIDGE_OP_EDID > > properly. It looks as if both actually ought to be implementing that > > instead of DRM_BRIDGE_OP_MODES, at least in some cases. A fix for a > > future day. Could we get into trouble if one moved before the other? > > Then the panel would no longer override the eDP controller and the eDP > > controller would try to read from a possibly unpowered panel? > > That would depend on the way the get_edid would be implemented in DP > driver. Currently the edid is cached via the > dp_display_process_hpd_high() -> dp_panel_read_sink_caps() call chain. > > With this patchset, the dp_hpd_plug_handle() -> > dp_display_usbpd_configure_cb() -> dp_display_process_hpd_high() will be > called too late for the get_modes/get_edid (from dp_bridge's enable() op). > > There is another issue. drm_panel has only get_modes() callback, so > panel_bridge can not implement get_edid() unless we extend the panel > interface (which might be a good idea). Ah, that makes sense and explains why the current panel code does the EDID reading in its get_modes() function even though get_modes() is _documented_ that it doesn't read the EDID. ;-) I guess it's another of the "let's move some people over to the new way but we'll keep the old code working". Definitely makes it hard to understand at times. > > For hotplug/detect I'm even less confident that setting the bits would > > be harmless. I haven't sat down and traced everything, but from what I > > can see the panel _doesn't_ set these bits, does it? I believe that > > the rule is that when every bridge in the chain _doesn't_ implement > > detect/hotplug that the panel is always present. The moment someone > > says "hey, I can detect" then it suddenly becomes _not_ always > > present. Yes, I guess we could have the panel implement "detect" and > > return true, but I'm not convinced that's actually better... > > I think it makes sense to implement OP_DETECT in panel bridge (that > always returns connector_status_connected) at least to override the > possible detect ops in previous bridges. So I truly don't know the right answer, but are you sure that's the best design? I _think_ that panel_bridge is used for all kinds of panels, right? So what if there's some type of display that uses a panel but there's still a mechanism that supports physical detection of the panel? By implementing "detect" in the generic panel_bridge then you're _preventing_ anyone higher up in the chain from implementing it and you're forcing it to be "always connected". For instance, we could come up with a new display standard called "pluggable eDP" that is just like eDP except that you can physically detect it. This imaginary new display standard is different from DP because it has eDP power sequencing (fully powers the display off when the screen is off) but it's hot pluggable! It introduces a new pin that goes to the DP controller called RT-HPD for "really, truly hot plug detect" that works even when the panel is off. The existing "HPD" pin continues to mean that the panel is read to communicate. If the drm_panel hardcodes "always connected" then I can't implement my "pluggable eDP" system, right? However, if we leave it just like it is today then my new system would be easy to implement. ;-) The above example is obviously not truly a real one but I guess my point is that I find it more intuitive / useful to say that we should only implement "detect" if we truly think we can detect and that if nobody says they can detect then we must be always connected. As an aside; I think in general it's not always easy to fit every possible graphics system into these "bridge chains" and the simple sequence of pre-enable, enable, etc, so we have to do our best and accept the fact that sometimes we'll need special cases. Dave Stephenson's patches [1] should tell us that, at least. [1] https://lore.kernel.org/all/cover.1646406653.git.dave.stevenson@raspberrypi.com/ -Doug
next prev parent reply other threads:[~2022-04-14 21:51 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-14 12:19 [PATCH v7 0/4] Add support for the eDP panel over aux_bus Sankeerth Billakanti 2022-04-14 12:19 ` Sankeerth Billakanti 2022-04-14 12:19 ` [PATCH v7 1/4] drm/msm/dp: Add eDP support via aux_bus Sankeerth Billakanti 2022-04-14 12:19 ` Sankeerth Billakanti 2022-04-14 16:39 ` Doug Anderson 2022-04-14 16:39 ` Doug Anderson 2022-04-14 19:16 ` Dmitry Baryshkov 2022-04-14 19:16 ` Dmitry Baryshkov 2022-04-14 19:40 ` Stephen Boyd 2022-04-14 19:40 ` Stephen Boyd 2022-04-14 20:09 ` Doug Anderson 2022-04-14 20:09 ` Doug Anderson 2022-04-14 21:16 ` Dmitry Baryshkov 2022-04-14 21:16 ` Dmitry Baryshkov 2022-04-14 21:51 ` Doug Anderson [this message] 2022-04-14 21:51 ` Doug Anderson 2022-04-14 12:19 ` [PATCH v7 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP Sankeerth Billakanti 2022-04-14 12:19 ` Sankeerth Billakanti 2022-04-14 16:39 ` Doug Anderson 2022-04-14 16:39 ` Doug Anderson 2022-04-14 12:19 ` [PATCH v7 3/4] drm/msm/dp: wait for hpd high before aux transaction Sankeerth Billakanti 2022-04-14 12:19 ` Sankeerth Billakanti 2022-04-14 16:39 ` Doug Anderson 2022-04-14 16:39 ` Doug Anderson 2022-04-14 12:19 ` [PATCH v7 4/4] Support the eDP modes given by panel Sankeerth Billakanti 2022-04-14 12:19 ` Sankeerth Billakanti 2022-04-14 16:40 ` Doug Anderson 2022-04-14 16:40 ` Doug Anderson 2022-04-14 16:40 ` [PATCH v7 0/4] Add support for the eDP panel over aux_bus Doug Anderson 2022-04-14 16:40 ` Doug Anderson 2022-04-14 19:20 ` Dmitry Baryshkov 2022-04-14 19:20 ` Dmitry Baryshkov 2022-04-14 19:43 ` Stephen Boyd 2022-04-14 19:43 ` Stephen Boyd 2022-04-14 20:00 ` [Freedreno] " Abhinav Kumar 2022-04-14 20:00 ` Abhinav Kumar 2022-04-14 20:03 ` Dmitry Baryshkov 2022-04-14 20:03 ` Dmitry Baryshkov 2022-04-14 20:19 ` Abhinav Kumar 2022-04-14 20:19 ` Abhinav Kumar 2022-04-14 20:21 ` Dmitry Baryshkov 2022-04-14 20:21 ` Dmitry Baryshkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAD=FV=XjVb7tP4acjOQgg2_8oCbOxqTopJE1PUKdf2noQCHg=Q@mail.gmail.com' \ --to=dianders@chromium.org \ --cc=airlied@linux.ie \ --cc=bjorn.andersson@linaro.org \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=quic_aravindh@quicinc.com \ --cc=quic_kalyant@quicinc.com \ --cc=quic_khsieh@quicinc.com \ --cc=quic_sbillaka@quicinc.com \ --cc=quic_vproddut@quicinc.com \ --cc=robdclark@gmail.com \ --cc=sean@poorly.run \ --cc=seanpaul@chromium.org \ --cc=steev@kali.org \ --cc=swboyd@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.