Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On 13-04-22, 20:39, Liu Ying wrote: > On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > > On 13-04-22, 18:04, Liu Ying wrote: > > > Hi Vinod, > > > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > > On 02-04-22, 13:24, Liu Ying wrote: > > > > > This patch allows LVDS PHYs to be configured through > > > > > the generic functions and through a custom structure > > > > > added to the generic union. > > > > > > > > > > The parameters added here are based on common LVDS PHY > > > > > implementation practices. The set of parameters > > > > > should cover all potential users. > > > > > > > > > > Cc: Kishon Vijay Abraham I > > > > > Cc: Vinod Koul > > > > > Cc: NXP Linux Team > > > > > Signed-off-by: Liu Ying > > > > > --- > > [...] > > > > > > + */ > > > > > + > > > > > +#ifndef __PHY_LVDS_H_ > > > > > +#define __PHY_LVDS_H_ > > > > > + > > > > > +/** > > > > > + * struct phy_configure_opts_lvds - LVDS configuration set > > > > > + * @bits_per_lane_and_dclk_cycle:Number of bits per data > > > > > lane > > > > > and > > > > > + * differential clock > > > > > cycle. > > > > > > > > What does it mean by bits per data lane and differential clock > > > > cycle? > > > > > > Please check > > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the > > > number of slots. But, I don't find the word 'slot' in my lvds > > > relevant > > > specs which mentioned in lvds.yaml, so 'slots' is probably not a > > > generic name(lvds.yaml is for display panel). So, I use > > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > > > variable name is fine, explanation for bit per lane and differential > > clock cycle didnt help, maybe add better explanation of what this > > variable means > > I may add an example diagram as below... Not really a diagram, you can add if you like.. But something which explains in a sentence or few about the variable. bits per lane per differential clock cycle ? -- ~Vinod
[pull] amdgpu drm-fixes-5.18
Hi Dave, Daniel, Fixes for 5.18. The following changes since commit 88711fa9a14f6f473f4a7645155ca51386e36c21: Merge tag 'drm-misc-fixes-2022-04-07' of git://anongit.freedesktop.org/drm/drm-misc into drm-fixes (2022-04-08 09:22:16 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-fixes-5.18-2022-04-13 for you to fetch changes up to aadaeca46ce54af9f8f494792a1ba47a6fbda7ba: drm/amd/display: remove dtbclk_ss compensation for dcn316 (2022-04-13 22:23:54 -0400) amd-drm-fixes-5.18-2022-04-13: amdgpu: - Fix for alpha properly in pre-multiplied mode - Fix VCN 3.1.2 firmware name - Suspend/resume fix - Add a gfxoff quirk for Mac vega20 board - DCN 3.1.6 spread spectrum fix Alex Deucher (1): drm/amdgpu: fix VCN 3.1.2 firmware name Charlene Liu (1): drm/amd/display: remove dtbclk_ss compensation for dcn316 Kai-Heng Feng (1): drm/amdgpu: Ensure HDA function is suspended before ASIC reset Melissa Wen (1): drm/amd/display: don't ignore alpha property on pre-multiplied mode Tomasz Moń (1): drm/amdgpu: Enable gfxoff quirk on MacBook Pro drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c| 18 -- drivers/gpu/drm/amd/amdgpu/amdgpu_vcn.c| 2 +- drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 2 ++ .../drm/amd/display/dc/clk_mgr/dce100/dce_clk_mgr.c| 2 +- .../drm/amd/display/dc/clk_mgr/dcn316/dcn316_clk_mgr.c | 4 ++-- drivers/gpu/drm/amd/display/dc/dc.h| 2 +- .../gpu/drm/amd/display/dc/dcn10/dcn10_hw_sequencer.c | 14 +- drivers/gpu/drm/amd/display/dc/dcn20/dcn20_hwseq.c | 14 +- 8 files changed, 37 insertions(+), 21 deletions(-)
Re: [PATCH 3/4] dt-bindings: drm/bridge: anx7625: Change bus-type to 7 (DPI)
On Wed, Apr 13, 2022 at 04:28:51PM +0200, Robert Foss wrote:
> On Sat, 9 Apr 2022 at 06:47, Xin Ji wrote:
> >
> > On Mon, Apr 04, 2022 at 12:52:14PM -0500, Rob Herring wrote:
> > > On Mon, Mar 28, 2022 at 08:09:54PM +0800, Xin Ji wrote:
> > > > Change bus-type define for DPI.
> > > >
> > > > Fixes: a43661e7e819 ("dt-bindings:drm/bridge:anx7625:add vendor define")
> > > >
> > > > Signed-off-by: Xin Ji
> > > > ---
> > > > .../devicetree/bindings/display/bridge/analogix,anx7625.yaml | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > >
> > > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > > index 0d38d6fe3983..4590186c4a0b 100644
> > > > ---
> > > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > > +++
> > > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > > @@ -106,7 +106,7 @@ properties:
> > > >remote-endpoint: true
> > > >
> > > >bus-type:
> > > > -enum: [1, 5]
> > > > +enum: [7]
> > >
> > > Changing is an ABI break, but didn't we revert adding this?
> > Hi Rob, sorry, what do you mean about ABI break? Do I need remove this
> > patch in this serial? Or do I need revert patch
> > https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.freedesktop.org%2Fpatch%2F462331%2Fdata=04%7C01%7Cxji%40analogixsemi.com%7C10f5b0213f434592936008da1d59f566%7Cb099b0b4f26c4cf59a0fd5be9acab205%7C0%7C0%7C637854569490105386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=vK0Vmb9S425kHZc1WurfnNhnoXDMbUGkkdY1PVnfS9g%3Dreserved=0,
> > I don't know how to do
> > it.
> >
>
> I contributed to the confusion about this, let's see if we can clear it up.
>
> An issue was found related to which enum values were used to represent
> what late in the last release cycle. As a result a revert[1] patch for
> everything touching bus-type in anx7625 was merged.
>
> This patch, does not apply to drm-misc-next due to the revert, and if
> Xin Ji rebases his work on the drm-misc-next there should be no
> ABI-change as this patch when fixed up will introduce bus-type to the
> nax7625 ABI.
>
> Xin: Does this make sense to you?
Hi Robert Foss, yes, my work is based on drm-misc-next, all I can do,
just make a fix up patch(this patch) to change the bus-type define.
Thanks,
Xin
>
> [1]
> https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcgit.freedesktop.org%2Fdrm%2Fdrm-misc%2Fcommit%2F%3Fid%3D979452fbc43028675b5a5da156f91928b739dea8data=04%7C01%7Cxji%40analogixsemi.com%7C10f5b0213f434592936008da1d59f566%7Cb099b0b4f26c4cf59a0fd5be9acab205%7C0%7C0%7C637854569490105386%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=onmKAoEEP1XDbl%2FaeAIfVYJ4cSbiTfTBYCd%2BHCA9fzw%3Dreserved=0
RE: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
> From: Wang, Zhi A > Sent: Thursday, April 14, 2022 5:09 AM > > > Or is it that only the page table levels themselves are GFNs and the > > actual DMA's are IOVA? The unclear mixing of GFN as IOVA in the code > > makes it inscrutible. > > > > No. Even the HW is capable of controlling the level of translation, but > it's not used like this in the existing driver. It's definitely an > architecture open. > There is no open on this. Any guest memory that vGPU accesses must be IOVA including page table levels. There is only one address space per vRID.
RE: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
> From: Jason Gunthorpe > Sent: Thursday, April 14, 2022 7:12 AM > > On Wed, Apr 13, 2022 at 09:08:40PM +, Wang, Zhi A wrote: > > On 4/13/22 8:04 PM, Jason Gunthorpe wrote: > > > On Wed, Apr 13, 2022 at 07:17:52PM +, Wang, Zhi A wrote: > > >> On 4/13/22 5:37 PM, Jason Gunthorpe wrote: > > >>> On Wed, Apr 13, 2022 at 06:29:46PM +0200, Christoph Hellwig wrote: > > On Wed, Apr 13, 2022 at 01:18:14PM -0300, Jason Gunthorpe wrote: > > > Yeah, I was thinking about that too, but on the other hand I think it > > > is completely wrong that gvt requires kvm at all. A vfio_device is not > > > supposed to be tightly linked to KVM - the only exception possibly > > > being s390.. > > > > So i915/gvt uses it for: > > > > - poking into the KVM GFN translations The only user of this is is_2MB_gtt_possible() which I suppose should go through vfio instead of kvm as it actually means IOVA here. > > - using the KVM page track notifier This is the real reason which causes the mess as write-protecting CPU access to certain guest memory has to go through KVM. > > > > No idea how these could be solved in a more generic way. > > >>> > > >>> TBH I'm not sure how any of this works fully correctly.. > > >>> > > >>> I see this code getting something it calls a GFN and then passing > > >>> them to vfio - which makes no sense. Either a value is a GFN - the > > >>> physical memory address of the VM, or it is an IOVA. VFIO only takes > > >>> in IOVA and kvm only takes in GFN. So these are probably IOVAs really.. > > >>> > > >> Can you let me know the place? So that I can take a look. > > > > > > Well, for instance: > > > > > > static int gvt_pin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn, > > > unsigned long size, struct page **page) > > > > > > There is no way that is a GFN, it is an IOVA. > > > > > I see. The name is vague. There is an promised 1:1 mapping between guest > GFN > > and host IOVA when a PCI device is passed to a VM, I guess mdev is just > > leveraging it as they are sharing the same code path in QEMU. > > That has never been true. It happens to be the case in some common > scenarios. > > > > So if the page table in the guest has IOVA addreses then why can you > > > use them as GFNs? > > > > That's another problem. We don't support a guess enabling the guest > IOMMU > > (aka virtual IOMMU). The guest/virtual IOMMU is implemented in QEMU, > so > > does the translation between guest IOVA and GFN. For a mdev model > > implemented in the kernel, there isn't any mechanism so far to reach there. > > And this is the uncommon scenario, there is no way for the mdev driver > to know if viommu is turned on, and AFAIK, no way to block it from VFIO. > > > People were discussing it before. But none agreement was achieved. Is it > > possible to implement it in the kernel? Would like to discuss more about it > > if there is any good idea. > > I don't know of anything, VFIO and kvm are not intended to be tightly > linked like this, they don't have the same view of the world. > Yes this is the main problem. VFIO only cares about IOVA and KVM only cares about GPA. GVT as a mdev driver should follow VFIO in concept but due to the requirement of gpu page table shadowing it needs call into KVM for write-protecting CPU access to GPA. What about extending KVM page tracking interface to accept HVA? This is probably the only common denominator between VFIO and KVM to allow dissolve this conceptual disconnection... Thanks Kevin
Re: [PATCH v2] drm/msm/dp: enhance both connect and disconnect pending_timeout handle
The subject is still misleading. It is fixing something. It may be
enhancing it as well but it is clearly fixing it first.
Quoting Kuogee Hsieh (2022-04-06 14:28:13)
> dp_hpd_plug_handle() is responsible for setting up main link and send
> uevent to notify user space framework to start video stream. Similarly,
> dp_hdp_unplug_handle is responsible to send uevent to notify user space
> framework to stop video stream and then tear down main link.
> However there are rare cases, such as in the middle of system suspending,
> that uevent could not be delivered to user space framework. Therefore
> some kind of recover mechanism armed by timer need to be in place in the
> case of user space framework does not respond to uevent.
>
> This patch have both dp_conenct_pending_timeout and
> dp_disconnect_pending_timeout are used to stop video stream and tear down
> main link and eventually restore DP driver state to known default
> DISCONNECTED state in the case of timer fired due to framework does not
> respond to uevent so that DP driver can recover itself gracefully at next
> dongle unplug followed by plugin event.
>
> Changes in v2:
> -- replace dp_display_usbpd_disconnect_cb with dp_display_notify_disconnect
I'd prefer this part to be a different patch. It can come after the fix
to ease backporting.
Also, is there any response to Dmitry's question yet? I haven't seen
anything.
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 2433edb..ffafe17 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -22,6 +22,7 @@ struct dp_ctrl {
> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
> int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
> int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
> +int dp_ctrl_off_link(struct dp_ctrl *dp_ctrl);
> int dp_ctrl_off(struct dp_ctrl *dp_ctrl);
> void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl);
> void dp_ctrl_isr(struct dp_ctrl *dp_ctrl);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 178b774..a6200a5 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -451,11 +451,14 @@ static int dp_display_usbpd_configure_cb(struct device
> *dev)
>
> static int dp_display_usbpd_disconnect_cb(struct device *dev)
We shouldn't need to keep around an empty function.
> {
> + return 0;
> +}
> +
> +static void dp_display_notify_disconnect(struct device *dev)
> +{
> struct dp_display_private *dp = dev_get_dp_display_private(dev);
>
> dp_add_event(dp, EV_USER_NOTIFICATION, false, 0);
> -
> - return 0;
> }
>
> static void dp_display_handle_video_request(struct dp_display_private *dp)
> @@ -593,10 +596,16 @@ static int dp_connect_pending_timeout(struct
> dp_display_private *dp, u32 data)
>
> mutex_lock(>event_mutex);
>
> + /*
> +* main link had been setup but video is not ready yet
> +* only tear down main link
> +*/
> state = dp->hpd_state;
> if (state == ST_CONNECT_PENDING) {
> - dp->hpd_state = ST_CONNECTED;
> DRM_DEBUG_DP("type=%d\n", dp->dp_display.connector_type);
> + dp_ctrl_off_link(dp->ctrl);
> + dp_display_host_phy_exit(dp);
> + dp->hpd_state = ST_DISCONNECTED;
> }
>
> mutex_unlock(>event_mutex);
> @@ -645,6 +654,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private
> *dp, u32 data)
> if (dp->link->sink_count == 0) {
> dp_display_host_phy_exit(dp);
> }
> + dp_display_notify_disconnect(>pdev->dev);
> mutex_unlock(>event_mutex);
> return 0;
> }
> @@ -661,19 +671,22 @@ static int dp_hpd_unplug_handle(struct
> dp_display_private *dp, u32 data)
> return 0;
> }
>
> - dp->hpd_state = ST_DISCONNECT_PENDING;
> -
> /* disable HPD plug interrupts */
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK,
> false);
>
> /*
> * We don't need separate work for disconnect as
> * connect/attention interrupts are disabled
> -*/
> - dp_display_usbpd_disconnect_cb(>pdev->dev);
> + */
This comment end is wrong. It should be unchanged.
> + dp_display_notify_disconnect(>pdev->dev);
>
> - /* start sentinel checking in case of missing uevent */
> - dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0,
> DP_TIMEOUT_5_SECOND);
> + if (state == ST_DISPLAY_OFF) {
> + dp->hpd_state = ST_DISCONNECTED;
> + } else {
> + /* start sentinel checking in case of missing uevent */
> + dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0,
> DP_TIMEOUT_5_SECOND);
> + dp->hpd_state = ST_DISCONNECT_PENDING;
> + }
>
> /* signal the disconnect event early to
linux-next: manual merge of the drm-misc tree with the drm-misc-fixes tree
Hi all,
Today's linux-next merge of the drm-misc tree got a conflict in:
drivers/gpu/drm/radeon/radeon_sync.c
between commit:
022074918042 ("drm/radeon: fix logic inversion in radeon_sync_resv")
from the drm-misc-fixes tree and commit:
7bc80a5462c3 ("dma-buf: add enum dma_resv_usage v4")
from the drm-misc tree.
I fixed it up (I just used the latter version) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging. You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.
--
Cheers,
Stephen Rothwell
pgpfrGA6dhjVK.pgp
Description: OpenPGP digital signature
Re: [PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind
On 14/04/2022 00:04, Kuogee Hsieh wrote:
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 40 +++--
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..943e4f1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp *dp_display)
complete_all(>audio_comp);
}
+static int hpd_event_thread(void *data);
+
+static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
+{
+ init_waitqueue_head(_priv->event_q);
+ spin_lock_init(_priv->event_lock);
This can go to dp_probe()
+
+ dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
"dp_hpd_handler");
+
+ if (IS_ERR(dp_priv->ev_tsk))
+ DRM_ERROR("failed to create DP event thread\n");
+}
+
+static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
+{
+ if (IS_ERR(dp_priv->ev_tsk))
+ return;
+
+ kthread_stop(dp_priv->ev_tsk);
+
+ /* reset event q to empty */
+ dp_priv->event_gndx = 0;
+ dp_priv->event_pndx = 0;
+}
+
static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
@@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct
device *master,
if (rc)
DRM_ERROR("Audio registration Dp failed\n");
+ dp_hpd_event_setup(dp); /* start event thread */
end:
return rc;
}
@@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct
device *master,
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
+ disable_irq(dp->irq);
+ dp_hpd_event_stop(dp); /* stop event thread */
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp[dp->id] = NULL;
@@ -1054,7 +1083,7 @@ static int hpd_event_thread(void *data)
dp_priv = (struct dp_display_private *)data;
- while (1) {
+ while (!kthread_should_stop()) {
if (timeout_mode) {
wait_event_timeout(dp_priv->event_q,
(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,13 +1161,6 @@ static int hpd_event_thread(void *data)
return 0;
}
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
-{
- init_waitqueue_head(_priv->event_q);
- spin_lock_init(_priv->event_lock);
-
- kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
-}
static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
{
@@ -1441,8 +1463,6 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp = container_of(dp_display, struct dp_display_private, dp_display);
- dp_hpd_event_setup(dp);
-
dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
--
With best wishes
Dmitry
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Wed, Apr 13, 2022 at 11:13:06PM +, Wang, Zhi A wrote: > Hi folks: > > Thanks so much for the efforts. I prepared a branch which contains all our > patches.The aim of the branch is for the VFIO maintainers to pull the whole > bunch easily after the drm-intel-next got merged through drm (as one of the > MMIO patches depends on a patch in drm-intel-next). > > I dropped patch 4 and patch 5 as they have been covered by Jani's patches. > Some conflicts was solved. > QA is going to test it today. > > You can find it here: > > git clone https://github.com/intel/gvt-linux -b for-christoph There are alot of extra commits on there - is it possible to base this straight on rc1 not on some kind of existing DRM tree? Why did you choose drm/i915/fbc: Call intel_fbc_activate() directly from frontbuffer flush as a base? Jason
Re: [PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind
Quoting Kuogee Hsieh (2022-04-13 14:04:25)
> Current DP driver implementation, event thread is kept running
> after DP display is unbind. This patch fix this problem by disabling
> DP irq and stop event thread to exit gracefully at dp_display_unbind().
>
> Changes in v2:
> -- start event thread at dp_display_bind()
>
> Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
> Chipsets")
> Signed-off-by: Kuogee Hsieh
> Reported-by: Dmitry Baryshkov
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 40
> +++--
> 1 file changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 01453db..943e4f1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -113,6 +113,7 @@ struct dp_display_private {
> u32 hpd_state;
> u32 event_pndx;
> u32 event_gndx;
> + struct task_struct *ev_tsk;
> struct dp_event event_list[DP_EVENT_Q_MAX];
> spinlock_t event_lock;
>
> @@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp
> *dp_display)
> complete_all(>audio_comp);
> }
>
> +static int hpd_event_thread(void *data);
Is there a reason why this is needed vs. defining the kthread start
function after hpd_event_thread()?
> +
> +static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
Maybe dp_hpd_event_thread_start()?
> +{
> + init_waitqueue_head(_priv->event_q);
> + spin_lock_init(_priv->event_lock);
> +
> + dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
> "dp_hpd_handler");
> +
> + if (IS_ERR(dp_priv->ev_tsk))
> + DRM_ERROR("failed to create DP event thread\n");
Can we return an error from this function?
> +}
> +
> +static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
Maybe dp_hpd_event_thread_stop()?
> +{
> + if (IS_ERR(dp_priv->ev_tsk))
> + return;
If we handled the error then this check becomes impossible.
> +
> + kthread_stop(dp_priv->ev_tsk);
> +
> + /* reset event q to empty */
> + dp_priv->event_gndx = 0;
> + dp_priv->event_pndx = 0;
> +}
> +
> static int dp_display_bind(struct device *dev, struct device *master,
>void *data)
> {
> @@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct
> device *master,
> if (rc)
> DRM_ERROR("Audio registration Dp failed\n");
>
> + dp_hpd_event_setup(dp); /* start event thread */
The comment is useless, please remove.
> end:
> return rc;
> }
> @@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct
> device *master,
> struct drm_device *drm = dev_get_drvdata(master);
> struct msm_drm_private *priv = drm->dev_private;
>
> + disable_irq(dp->irq);
Is the disable_irq() necessary? It would be nicer to silence the
hardware and remove the disable_irq() so that we can reason about the
code assuming the irq is always enabled after it is requested.
> + dp_hpd_event_stop(dp); /* stop event thread */
> dp_power_client_deinit(dp->power);
> dp_aux_unregister(dp->aux);
> priv->dp[dp->id] = NULL;
[Bug 215618] vblank related lockup during start of SteamVR using Valve Index HMD
https://bugzilla.kernel.org/show_bug.cgi?id=215618 Pierre Choffet (ct@peuc.net) changed: What|Removed |Added CC||ct@peuc.net --- Comment #1 from Pierre Choffet (ct@peuc.net) --- I can reproduce the crash with a Radeon 6800XT in 5.17.1. GPU is then unstable after it resets and system must be rebooted. Here is my callstack - it's almost the same as the previous one: > [drm:dm_vblank_get_counter [amdgpu]] *ERROR* dc_stream_state is NULL for crtc > '1'! > [drm:dm_crtc_get_scanoutpos [amdgpu]] *ERROR* dc_stream_state is NULL for > crtc '1'! > [drm:dm_vblank_get_counter [amdgpu]] *ERROR* dc_stream_state is NULL for crtc > '1'! > [ cut here ] > amdgpu :0b:00.0: drm_WARN_ON_ONCE(drm_drv_uses_atomic_modeset(dev)) > WARNING: CPU: 3 PID: 2263 at drivers/gpu/drm/drm_vblank.c:728 > drm_crtc_vblank_helper_get_vblank_timestamp_internal+0x369/0x380 > Modules linked in: nf_tables nfnetlink snd_seq_dummy snd_hrtimer snd_seq > cfg80211 8021q garp mrp stp llc nct6775 hwmon_vid eeepc_wmi intel_ra> > crct10dif_pclmul crc32_pclmul crc32c_intel ghash_clmulni_intel aesni_intel > ccp sr_mod xhci_pci crypto_simd cryptd rng_core cdrom xhci_pci_re> > CPU: 3 PID: 2263 Comm: VulkanVblankThr Not tainted 5.17.1-arch1-1 #1 > 0ea933cb6bfe82a8dc16ab834a4bccdd297f98b7 > Hardware name: ASUS System Product Name/ROG CROSSHAIR VIII DARK HERO, BIOS > 3601 05/26/2021 > RIP: 0010:drm_crtc_vblank_helper_get_vblank_timestamp_internal+0x369/0x380 > Code: 4c 8b 6f 50 4d 85 ed 75 03 4c 8b 2f e8 f0 6b 01 00 48 c7 c1 40 f3 d1 ad > 4c 89 ea 48 c7 c7 c2 5d d1 ad 48 89 c6 e8 a3 43 3d 00 <0f> 0b e> > RSP: 0018:9beb86303b20 EFLAGS: 00010082 > RAX: RBX: c0b7e840 RCX: 0027 > RDX: 8dca0eae1728 RSI: 0001 RDI: 8dca0eae1720 > RBP: 9beb86303b90 R08: R09: 9beb86303950 > R10: 9beb86303948 R11: 8dca2f2a9b28 R12: > R13: 8dc3023dae30 R14: R15: 8dc3376b21d8 > FS: 7fa79444b640() GS:8dca0eac() knlGS: > CS: 0010 DS: ES: CR0: 80050033 > CR2: 7fa6ec001278 CR3: 0001e6984000 CR4: 00750ee0 > PKRU: 5554 > Call Trace: > > drm_get_last_vbltimestamp+0xb2/0xc0 > drm_update_vblank_count+0x91/0x3d0 > drm_vblank_enable+0x14b/0x180 > drm_vblank_get+0x95/0xe0 > drm_crtc_queue_sequence_ioctl+0xfd/0x2d0 > ? __check_object_size+0x46/0x140 > ? drm_crtc_get_sequence_ioctl+0x1a0/0x1a0 > drm_ioctl_kernel+0xb8/0x140 > drm_ioctl+0x22a/0x3d0 > ? drm_crtc_get_sequence_ioctl+0x1a0/0x1a0 > amdgpu_drm_ioctl+0x49/0x80 [amdgpu 08a70cd20fdf14582ce9165e3698aeaecdd8c8f8] > __x64_sys_ioctl+0x82/0xb0 > do_syscall_64+0x5c/0x80 > ? do_user_addr_fault+0x1d7/0x690 > ? do_syscall_64+0x69/0x80 > ? exc_page_fault+0x72/0x170 > entry_SYSCALL_64_after_hwframe+0x44/0xae > RIP: 0033:0x7fa7ac2a7e6f > Code: 00 48 89 44 24 18 31 c0 48 8d 44 24 60 c7 04 24 10 00 00 00 48 89 44 24 > 08 48 8d 44 24 20 48 89 44 24 10 b8 10 00 00 00 0f 05 <41> 89 c> > RSP: 002b:7fa79444ab00 EFLAGS: 0246 ORIG_RAX: 0010 > RAX: ffda RBX: 7fa79444ab90 RCX: 7fa7ac2a7e6f > RDX: 7fa79444ab90 RSI: c018643c RDI: 004a > RBP: c018643c R08: R09: 7fa6ec000be0 > R10: 006e R11: 0246 R12: 55d760b895b8 > R13: 004a R14: 55d760c41d00 R15: > > ---[ end trace ]--- > [drm:dm_vblank_get_counter [amdgpu]] *ERROR* dc_stream_state is NULL for crtc > '1'! > [drm:dm_crtc_get_scanoutpos [amdgpu]] *ERROR* dc_stream_state is NULL for > crtc '1'! > [drm:dm_vblank_get_counter [amdgpu]] *ERROR* dc_stream_state is NULL for crtc > '1'! -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: refactor the i915 GVT support and move to the modern mdev API v3
Hi folks: Thanks so much for the efforts. I prepared a branch which contains all our patches.The aim of the branch is for the VFIO maintainers to pull the whole bunch easily after the drm-intel-next got merged through drm (as one of the MMIO patches depends on a patch in drm-intel-next). I dropped patch 4 and patch 5 as they have been covered by Jani's patches. Some conflicts was solved. QA is going to test it today. You can find it here: git clone https://github.com/intel/gvt-linux -b for-christoph Thanks, Zhi. On 4/13/22 3:58 PM, Jani Nikula wrote: > On Wed, 13 Apr 2022, Christoph Hellwig wrote: >> On Wed, Apr 13, 2022 at 01:47:05PM +, Wang, Zhi A wrote: the GVT code in the i915 is a bit of a mess right now due to strange abstractions and lots of indirect calls. This series refactors various bits to clean that up. The main user visible change is that almost all of the GVT code moves out of the main i915 driver and into the kvmgt module. >>> >>> Hi Christoph: >>> >>> Do you want me to merge the GVT-g patches in this series? Or you want them >>> to get merged from your side? >> >> The two option here are drm tree via gvt and i915 trees or the vfio >> tree, neither of which really is my tree. >> >> We already have a fair bit of vfio changes at the tail end of the series, >> and Jason has some more that should sit on top of it, and I have some >> more that I haven't sent yet. >> >> So if we could get the MMIO table and Makefile cleanups into a topic >> branch that we could pull into the vfio tree and merge it through that >> that would seem easiest to me, assuming that is ok with the i915, drm >> and vfio maintainers. > > AFAICS the changes are mostly to gvt/, and at least I'm fine with the > minor changes to i915 (in this series and in my two patches) being > merged via whichever tree you all see fit. > > Acked-by: Jani Nikula > > Joonas, Tvrtko, Rodrigo, chime in now if you have any issues with that. > > > BR, > Jani. > >
Re: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
On Wed, Apr 13, 2022 at 09:08:40PM +, Wang, Zhi A wrote: > On 4/13/22 8:04 PM, Jason Gunthorpe wrote: > > On Wed, Apr 13, 2022 at 07:17:52PM +, Wang, Zhi A wrote: > >> On 4/13/22 5:37 PM, Jason Gunthorpe wrote: > >>> On Wed, Apr 13, 2022 at 06:29:46PM +0200, Christoph Hellwig wrote: > On Wed, Apr 13, 2022 at 01:18:14PM -0300, Jason Gunthorpe wrote: > > Yeah, I was thinking about that too, but on the other hand I think it > > is completely wrong that gvt requires kvm at all. A vfio_device is not > > supposed to be tightly linked to KVM - the only exception possibly > > being s390.. > > So i915/gvt uses it for: > > - poking into the KVM GFN translations > - using the KVM page track notifier > > No idea how these could be solved in a more generic way. > >>> > >>> TBH I'm not sure how any of this works fully correctly.. > >>> > >>> I see this code getting something it calls a GFN and then passing > >>> them to vfio - which makes no sense. Either a value is a GFN - the > >>> physical memory address of the VM, or it is an IOVA. VFIO only takes > >>> in IOVA and kvm only takes in GFN. So these are probably IOVAs really.. > >>> > >> Can you let me know the place? So that I can take a look. > > > > Well, for instance: > > > > static int gvt_pin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn, > > unsigned long size, struct page **page) > > > > There is no way that is a GFN, it is an IOVA. > > > I see. The name is vague. There is an promised 1:1 mapping between guest GFN > and host IOVA when a PCI device is passed to a VM, I guess mdev is just > leveraging it as they are sharing the same code path in QEMU. That has never been true. It happens to be the case in some common scenarios. > > So if the page table in the guest has IOVA addreses then why can you > > use them as GFNs? > > That's another problem. We don't support a guess enabling the guest IOMMU > (aka virtual IOMMU). The guest/virtual IOMMU is implemented in QEMU, so > does the translation between guest IOVA and GFN. For a mdev model > implemented in the kernel, there isn't any mechanism so far to reach there. And this is the uncommon scenario, there is no way for the mdev driver to know if viommu is turned on, and AFAIK, no way to block it from VFIO. > People were discussing it before. But none agreement was achieved. Is it > possible to implement it in the kernel? Would like to discuss more about it > if there is any good idea. I don't know of anything, VFIO and kvm are not intended to be tightly linked like this, they don't have the same view of the world. Jason
Re: [PATCH V2 3/3] dt-bindings: display: mediatek: Update disp_aal binding for MT8192 and MT8195
On Mon, 11 Apr 2022 11:58:43 +0800, Rex-BC Chen wrote: > Disp_aal of MT8192 and MT8195 are fully compatible with disp_aal of > MT8183. Therefore, we move the them to item "mediatek,mt8183-disp-aal". > > Signed-off-by: Rex-BC Chen > --- > .../devicetree/bindings/display/mediatek/mediatek,aal.yaml| 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Acked-by: Rob Herring
Re: [PATCH V2 1/3] dt-bindings: display: mediatek: Update disp_aal binding for MT8183
On Mon, 11 Apr 2022 11:58:41 +0800, Rex-BC Chen wrote: > The driver data of MT8183 and MT8173 are different. > > For MT8173, the gamma module is inside disp_aal. When we need to adjust > gamma value, we need to use "has_gamma" to control gamma function > inside disp_aal to adjust the gamma value. > > For successors like MT8183, disp_gamma is separated from disp_aal. We > just need to control disp_gamma directly and don't need to control gamma > function inside disp_aal. > > With this modification, the driver doesn't require any functional changes. > We only update the dt-binding and DTS node to make it clear. > > Signed-off-by: Rex-BC Chen > Reviewed-by: AngeloGioacchino Del Regno > > --- > .../devicetree/bindings/display/mediatek/mediatek,aal.yaml | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > Acked-by: Rob Herring
Re: [PATCH v8 2/2] drm/msm/disp/dpu1: add inline rotation support for sc7280
On 11/04/2022 19:37, Vinod Polimera wrote:
- Some DPU versions support inline rot90. It is supported only for
limited amount of UBWC formats.
- There are two versions of inline rotators, v1 (present on sm8250 and
sm7250) and v2 (sc7280). These versions differ in the list of supported
formats and in the scaler possibilities.
Co-developed-by: Kalyan Thota
Signed-off-by: Kalyan Thota
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 43 -
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 16 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 129 -
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 2 +
4 files changed, 161 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index a4fe77c..85b539d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -35,6 +35,9 @@
BIT(DPU_SSPP_TS_PREFILL) | BIT(DPU_SSPP_TS_PREFILL_REC1) |\
BIT(DPU_SSPP_CDP) | BIT(DPU_SSPP_EXCL_RECT))
+#define VIG_SC7280_MASK \
+ (VIG_SC7180_MASK | BIT(DPU_SSPP_INLINE_ROTATION))
+
#define DMA_SDM845_MASK \
(BIT(DPU_SSPP_SRC) | BIT(DPU_SSPP_QOS) | BIT(DPU_SSPP_QOS_8LVL) |\
BIT(DPU_SSPP_TS_PREFILL) | BIT(DPU_SSPP_TS_PREFILL_REC1) |\
@@ -203,6 +206,11 @@ static const uint32_t plane_formats_yuv[] = {
DRM_FORMAT_YVU420,
};
+static const u32 rotation_v2_formats[] = {
+ DRM_FORMAT_NV12,
+ /* TODO add formats after validation */
+};
+
/*
* DPU sub blocks config
*/
@@ -642,7 +650,6 @@ static const struct dpu_ctl_cfg qcm2290_ctl[] = {
*/
/* SSPP common configuration */
-
#define _VIG_SBLK(num, sdma_pri, qseed_ver) \
{ \
.maxdwnscale = MAX_DOWNSCALE_RATIO, \
@@ -660,6 +667,27 @@ static const struct dpu_ctl_cfg qcm2290_ctl[] = {
.num_formats = ARRAY_SIZE(plane_formats_yuv), \
.virt_format_list = plane_formats, \
.virt_num_formats = ARRAY_SIZE(plane_formats), \
+ .rotation_cfg = NULL, \
+ }
+
+#define _VIG_SBLK_ROT(num, sdma_pri, qseed_ver, rot_cfg) \
+ { \
+ .maxdwnscale = MAX_DOWNSCALE_RATIO, \
+ .maxupscale = MAX_UPSCALE_RATIO, \
+ .smart_dma_priority = sdma_pri, \
+ .src_blk = {.name = STRCAT("sspp_src_", num), \
+ .id = DPU_SSPP_SRC, .base = 0x00, .len = 0x150,}, \
+ .scaler_blk = {.name = STRCAT("sspp_scaler", num), \
+ .id = qseed_ver, \
+ .base = 0xa00, .len = 0xa0,}, \
+ .csc_blk = {.name = STRCAT("sspp_csc", num), \
+ .id = DPU_SSPP_CSC_10BIT, \
+ .base = 0x1a00, .len = 0x100,}, \
+ .format_list = plane_formats_yuv, \
+ .num_formats = ARRAY_SIZE(plane_formats_yuv), \
+ .virt_format_list = plane_formats, \
+ .virt_num_formats = ARRAY_SIZE(plane_formats), \
+ .rotation_cfg = rot_cfg, \
}
#define _DMA_SBLK(num, sdma_pri) \
@@ -684,6 +712,12 @@ static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
_VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);
+static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
+ .rot_maxheight = 1088,
+ .rot_num_formats = ARRAY_SIZE(rotation_v2_formats),
+ .rot_format_list = rotation_v2_formats,
+};
+
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_0 =
_VIG_SBLK("0", 5, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_1 =
@@ -751,6 +785,9 @@ static const struct dpu_sspp_cfg sdm845_sspp[] = {
static const struct dpu_sspp_sub_blks sc7180_vig_sblk_0 =
_VIG_SBLK("0", 4, DPU_SSPP_SCALER_QSEED4);
+static const struct dpu_sspp_sub_blks sc7280_vig_sblk_0 =
+ _VIG_SBLK_ROT("0", 4, DPU_SSPP_SCALER_QSEED4,
_rot_sc7280_cfg_v2);
+
static const struct dpu_sspp_cfg sc7180_sspp[] = {
SSPP_BLK("sspp_0", SSPP_VIG0, 0x4000, VIG_SC7180_MASK,
sc7180_vig_sblk_0, 0, SSPP_TYPE_VIG, DPU_CLK_CTRL_VIG0),
@@ -791,8 +828,8 @@ static const struct dpu_sspp_cfg sm8250_sspp[] = {
};
static const struct dpu_sspp_cfg sc7280_sspp[] = {
- SSPP_BLK("sspp_0", SSPP_VIG0, 0x4000, VIG_SC7180_MASK,
- sc7180_vig_sblk_0, 0, SSPP_TYPE_VIG, DPU_CLK_CTRL_VIG0),
+ SSPP_BLK("sspp_0", SSPP_VIG0, 0x4000, VIG_SC7280_MASK,
+ sc7280_vig_sblk_0, 0, SSPP_TYPE_VIG, DPU_CLK_CTRL_VIG0),
SSPP_BLK("sspp_8", SSPP_DMA0, 0x24000, DMA_SDM845_MASK,
sdm845_dma_sblk_0, 1, SSPP_TYPE_DMA, DPU_CLK_CTRL_DMA0),
Re: [PATCH v8 1/2] drm/msm/disp/dpu1: add inline function to validate format support
On 11/04/2022 19:37, Vinod Polimera wrote:
Check if the dpu format is supported or not using dpu_find_format.
Co-developed-by: Kalyan Thota
Signed-off-by: Kalyan Thota
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h | 22 ++
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 10 +++---
2 files changed, 25 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
index 418f5ae..84b8b32 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_formats.h
@@ -21,6 +21,28 @@ const struct dpu_format *dpu_get_dpu_format_ext(
#define dpu_get_dpu_format(f) dpu_get_dpu_format_ext(f, 0)
/**
+ * dpu_find_format - validate if the pixel format is supported
+ * @format:dpu format
+ * @supported_formats: supported formats by dpu HW
+ * @num_formatss: total number of formats
+ *
+ * Return: false if not valid format, true on success
+ */
+static inline bool dpu_find_format(u32 format, const u32 *supported_formats,
+ size_t num_formats)
+{
+ int i;
+
+ for (i = 0; i < num_formats; i++) {
+ /* check for valid formats supported */
+ if (format == supported_formats[i])
+ return true;
+ }
+
+ return false;
+}
+
+/**
* dpu_get_msm_format - get an dpu_format by its msm_format base
* callback function registers with the msm_kms layer
* @kms: kms driver
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 6565682..3216cda 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -1411,13 +1411,9 @@ static bool dpu_plane_format_mod_supported(struct
drm_plane *plane,
if (modifier == DRM_FORMAT_MOD_LINEAR)
return true;
- if (modifier == DRM_FORMAT_MOD_QCOM_COMPRESSED) {
- int i;
- for (i = 0; i < ARRAY_SIZE(qcom_compressed_supported_formats);
i++) {
- if (format == qcom_compressed_supported_formats[i])
- return true;
- }
- }
+ if (modifier == DRM_FORMAT_MOD_QCOM_COMPRESSED)
+ return dpu_find_format(format,
qcom_compressed_supported_formats,
+ ARRAY_SIZE(qcom_compressed_supported_formats));
return false;
}
--
With best wishes
Dmitry
Re: [PATCH v2] drm/tegra: Stop using iommu_present()
On 4/11/22 16:46, Robin Murphy wrote:
> Refactor the confusing logic to make it both clearer and more robust. If
> the host1x parent device does have an IOMMU domain then iommu_present()
> is redundantly true, while otherwise for the 32-bit DMA mask case it
> still doesn't say whether the IOMMU driver actually knows about the DRM
> device or not.
>
> Signed-off-by: Robin Murphy
> ---
>
> v2: Fix logic for older SoCs and clarify.
>
> drivers/gpu/drm/tegra/drm.c | 28
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 9464f522e257..4f2bdab31064 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -1092,6 +1092,19 @@ static bool host1x_drm_wants_iommu(struct
> host1x_device *dev)
> struct host1x *host1x = dev_get_drvdata(dev->dev.parent);
> struct iommu_domain *domain;
>
> + /* For starters, this is moot if no IOMMU is available */
> + if (!device_iommu_mapped(>dev))
> + return false;
> +
> + /*
> + * Tegra20 and Tegra30 don't support addressing memory beyond the
> + * 32-bit boundary, so the regular GATHER opcodes will always be
> + * sufficient and whether or not the host1x is attached to an IOMMU
> + * doesn't matter.
> + */
> + if (host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32))
> + return true;
> +
> /*
>* If the Tegra DRM clients are backed by an IOMMU, push buffers are
>* likely to be allocated beyond the 32-bit boundary if sufficient
> @@ -1122,14 +1135,13 @@ static bool host1x_drm_wants_iommu(struct
> host1x_device *dev)
> domain = iommu_get_domain_for_dev(dev->dev.parent);
>
> /*
> - * Tegra20 and Tegra30 don't support addressing memory beyond the
> - * 32-bit boundary, so the regular GATHER opcodes will always be
> - * sufficient and whether or not the host1x is attached to an IOMMU
> - * doesn't matter.
> + * At the moment, the exact type of domain doesn't actually matter.
> + * Only for 64-bit kernels might this be a managed DMA API domain, and
> + * then only on newer SoCs using arm-smmu, since tegra-smmu doesn't
> + * support default domains at all, and since those SoCs are the same
> + * ones with extended GATHER support, even if it's a passthrough domain
> + * it can still work out OK.
>*/
> - if (!domain && host1x_get_dma_mask(host1x) <= DMA_BIT_MASK(32))
> - return true;
> -
> return domain != NULL;
> }
>
> @@ -1149,7 +1161,7 @@ static int host1x_drm_probe(struct host1x_device *dev)
> goto put;
> }
>
> - if (host1x_drm_wants_iommu(dev) && iommu_present(_bus_type)) {
> + if (host1x_drm_wants_iommu(dev)) {
> tegra->domain = iommu_domain_alloc(_bus_type);
> if (!tegra->domain) {
> err = -ENOMEM;
Robin, thank you for the updated version. The patch looks okay to me.
Reviewed-by: Dmitry Osipenko
A bit later I'll also will give it a test, just to be sure because we
had problems with that function in the past.
[RFC PATCH 12/16] drm/rockchip: ebc: Add support for direct mode
Currently, 3-window mode causes some artifacting. Until the cause is
determined, provide an option to use direct mode instead. Direct mode
does the waveform lookups in software, so it has much higher CPU usage.
This limits the frame rate below the panel's ideal 85 Hz, so it leads to
slightly lower brightness accuracy. On the other hand, it doesn't leave
random lines all over the screen.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 97 ++---
1 file changed, 88 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index dcd8c8e8208e..93d689ff0933 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -162,6 +162,10 @@ static bool diff_mode = true;
module_param(diff_mode, bool, 0644);
MODULE_PARM_DESC(diff_mode, "only compute waveforms for changed pixels");
+static bool direct_mode = true;
+module_param(direct_mode, bool, 0444);
+MODULE_PARM_DESC(direct_mode, "compute waveforms in software (software LUT)");
+
static bool skip_reset;
module_param(skip_reset, bool, 0444);
MODULE_PARM_DESC(skip_reset, "skip the initial display reset");
@@ -370,6 +374,59 @@ static bool rockchip_ebc_schedule_area(struct list_head
*areas,
return true;
}
+static void rockchip_ebc_blit_direct(const struct rockchip_ebc_ctx *ctx,
+u8 *dst, u8 phase,
+const struct drm_epd_lut *lut,
+const struct drm_rect *clip)
+{
+ const u32 *phase_lut = (const u32 *)lut->buf + 16 * phase;
+ unsigned int dst_pitch = ctx->phase_pitch / 4;
+ unsigned int src_pitch = ctx->gray4_pitch;
+ unsigned int x, y;
+ u8 *dst_line;
+ u32 src_line;
+
+ dst_line = dst + clip->y1 * dst_pitch + clip->x1 / 4;
+ src_line = clip->y1 * src_pitch + clip->x1 / 2;
+
+ for (y = clip->y1; y < clip->y2; y++) {
+ u32 src_offset = src_line;
+ u8 *dbuf = dst_line;
+
+ for (x = clip->x1; x < clip->x2; x += 4) {
+ u8 prev0 = ctx->prev[src_offset];
+ u8 next0 = ctx->next[src_offset++];
+ u8 prev1 = ctx->prev[src_offset];
+ u8 next1 = ctx->next[src_offset++];
+
+ /*
+* The LUT is 256 phases * 16 next * 16 previous levels.
+* Each value is two bits, so the last dimension neatly
+* fits in a 32-bit word.
+*/
+ u8 data = ((phase_lut[next0 & 0xf] >> ((prev0 & 0xf) <<
1)) & 0x3) << 0 |
+ ((phase_lut[next0 >> 4] >> ((prev0 >> 4) <<
1)) & 0x3) << 2 |
+ ((phase_lut[next1 & 0xf] >> ((prev1 & 0xf) <<
1)) & 0x3) << 4 |
+ ((phase_lut[next1 >> 4] >> ((prev1 >> 4) <<
1)) & 0x3) << 6;
+
+ /* Diff mode ignores pixels that did not change
brightness. */
+ if (diff_mode) {
+ u8 mask = ((next0 ^ prev0) & 0x0f ? 0x03 : 0) |
+ ((next0 ^ prev0) & 0xf0 ? 0x0c : 0) |
+ ((next1 ^ prev1) & 0x0f ? 0x30 : 0) |
+ ((next1 ^ prev1) & 0xf0 ? 0xc0 : 0);
+
+ data &= mask;
+ }
+
+ *dbuf++ = data;
+ }
+
+ dst_line += dst_pitch;
+ src_line += src_pitch;
+ }
+}
+
static void rockchip_ebc_blit_phase(const struct rockchip_ebc_ctx *ctx,
u8 *dst, u8 phase,
const struct drm_rect *clip)
@@ -472,8 +529,13 @@ static void rockchip_ebc_partial_refresh(struct
rockchip_ebc *ebc,
* be neutral for every waveform.
*/
phase = frame_delta >= last_phase ? 0xff : frame_delta;
- rockchip_ebc_blit_phase(ctx, phase_buffer, phase,
- >clip);
+ if (direct_mode)
+ rockchip_ebc_blit_direct(ctx, phase_buffer,
+phase, >lut,
+>clip);
+ else
+ rockchip_ebc_blit_phase(ctx, phase_buffer,
+ phase, >clip);
/*
* Copy ctx->next to ctx->prev after the last phase.
@@ -513,7 +575,8 @@ static void rockchip_ebc_partial_refresh(struct
rockchip_ebc *ebc,
if (list_empty())
[RFC PATCH 16/16] [DO NOT MERGE] arm64: dts: rockchip: pinenote: Enable EBC display pipeline
The PineNote contains an eInk ED103TC2 panel connected to the EBC,
powered by a TI TPS651851 PMIC.
Signed-off-by: Samuel Holland
---
.../boot/dts/rockchip/rk3566-pinenote.dtsi| 80 +++
1 file changed, 80 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
b/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
index fea748adfa90..4a53931c3f92 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3566-pinenote.dtsi
@@ -72,6 +72,16 @@ led-0 {
};
};
+ panel {
+ compatible = "eink,ed103tc2";
+
+ port {
+ panel_in_ebc: endpoint {
+ remote-endpoint = <_out_panel>;
+ };
+ };
+ };
+
sdio_pwrseq: sdio-pwrseq {
compatible = "mmc-pwrseq-simple";
clocks = < 1>;
@@ -213,6 +223,20 @@ {
cpu-supply = <_cpu>;
};
+ {
+ io-channels = <_pmic 0>;
+ panel-supply = <>;
+ vcom-supply = <>;
+ vdrive-supply = <>;
+ status = "okay";
+
+ port {
+ ebc_out_panel: endpoint {
+ remote-endpoint = <_in_ebc>;
+ };
+ };
+};
+
{
status = "okay";
@@ -466,6 +490,47 @@ led@1 {
default-brightness = <0>;
};
};
+
+ /* TODO: write binding */
+ ebc_pmic: pmic@68 {
+ compatible = "ti,tps65185";
+ reg = <0x68>;
+ interrupt-parent = <>;
+ interrupts = ;
+ #io-channel-cells = <1>;
+ pinctrl-0 = <_pmic_pins>;
+ pinctrl-names = "default";
+ powerup-gpios = < RK_PB0 GPIO_ACTIVE_HIGH>;
+ pwr_good-gpios = < RK_PA7 GPIO_ACTIVE_HIGH>;
+ vcom_ctrl-gpios = < RK_PB2 GPIO_ACTIVE_HIGH>;
+ vin-supply = <_bat>;
+ vin3p3-supply = <_3v3>;
+ wakeup-gpios = < RK_PA5 GPIO_ACTIVE_HIGH>;
+ ti,up-sequence = <1>, <0>, <2>, <3>;
+ ti,up-delay-ms = <3>, <3>, <3>, <3>;
+ ti,down-sequence = <2>, <3>, <1>, <0>;
+ ti,down-delay-ms = <3>, <6>, <6>, <6>;
+
+ regulators {
+ v3p3: v3p3 {
+ regulator-name = "v3p3";
+ regulator-always-on;
+ regulator-min-microvolt = <330>;
+ regulator-max-microvolt = <330>;
+ };
+
+ vcom: vcom {
+ regulator-name = "vcom";
+ /* voltage range is board-specific */
+ };
+
+ vdrive: vdrive {
+ regulator-name = "vdrive";
+ regulator-min-microvolt = <1500>;
+ regulator-max-microvolt = <1500>;
+ };
+ };
+ };
};
_8ch {
@@ -508,6 +573,21 @@ bt_wake_h: bt-wake-h {
};
};
+ ebc-pmic {
+ ebc_pmic_pins: ebc-pmic-pins {
+ rockchip,pins = /* wakeup */
+ <3 RK_PA5 RK_FUNC_GPIO _pull_none>,
+ /* int */
+ <3 RK_PA6 RK_FUNC_GPIO _pull_up>,
+ /* pwr_good */
+ <3 RK_PA7 RK_FUNC_GPIO _pull_none>,
+ /* pwrup */
+ <3 RK_PB0 RK_FUNC_GPIO _pull_none>,
+ /* vcom_ctrl */
+ <4 RK_PB2 RK_FUNC_GPIO _pull_none>;
+ };
+ };
+
led {
led_pin: led-pin {
rockchip,pins = <3 RK_PC5 RK_FUNC_GPIO _pull_none>;
--
2.35.1
[RFC PATCH 15/16] arm64: dts: rockchip: rk356x: Add EBC node
The RK356x SoCs contain an EBC. Add its node.
Signed-off-by: Samuel Holland
---
arch/arm64/boot/dts/rockchip/rk356x.dtsi | 14 ++
1 file changed, 14 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 7cdef800cb3c..58c26f240af0 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -508,6 +508,20 @@ gpu: gpu@fde6 {
status = "disabled";
};
+ ebc: ebc@fdec {
+ compatible = "rockchip,rk3568-ebc";
+ reg = <0x0 0xfdec 0x0 0x5000>;
+ interrupts = ;
+ clocks = < HCLK_EBC>, < DCLK_EBC>;
+ clock-names = "hclk", "dclk";
+ pinctrl-0 = <_pins>;
+ pinctrl-names = "default";
+ power-domains = < RK3568_PD_RGA>;
+ resets = < SRST_H_EBC>, < SRST_D_EBC>;
+ reset-names = "hclk", "dclk";
+ status = "disabled";
+ };
+
sdmmc2: mmc@fe00 {
compatible = "rockchip,rk3568-dw-mshc",
"rockchip,rk3288-dw-mshc";
reg = <0x0 0xfe00 0x0 0x4000>;
--
2.35.1
[RFC PATCH 14/16] drm/panel-simple: Add eInk ED103TC2
ED103TC2 is a 10.3" 1872x1404 eInk panel which supports up to 16 levels
of grayscale and an 85 Hz frame rate. The timings and polarities here
were taken from the manufacturer's datasheet.
Since this panel is an electrophoretic display (EPD), the color depth is
independent from the bus width. Instead, it is largely determined by the
number of frames in the selected waveform. Each pixel uses two parallel
data lines to specify one of only three states each frame: positive,
negative, or no voltage.
This specific panel has a 16-bit data bus, allowing it to update 8
pixels during each source driver (horizontal) clock cycle. As a result,
the horizontal timings given in the datasheet were all multiplied by 8
to convert the units from clock cycles to pixels.
Since the 16-bit data bus is double the width of the usual 8-bit bus
used by eInk panels, the source driver clock will be half the usual
frequency. This is signified by the DRM_MODE_FLAG_CLKDIV2 flag.
The hskew parameter provides the spacing between the horizontal sync
puls and the gate driver (vertical) clock pulse. This spacing is
symmetrical on both sides, so it can be used to compute the gate
driver clock pulse width.
Datasheet:
https://files.pine64.org/doc/quartz64/Eink%20P-511-828-V1_ED103TC2%20Formal%20Spec%20V1.0_20190514.pdf
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/panel/panel-simple.c | 31
1 file changed, 31 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index a34f4198a534..c6b104ba01ee 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1686,6 +1686,34 @@ static const struct panel_desc edt_etmv570g2dhu = {
.connector_type = DRM_MODE_CONNECTOR_DPI,
};
+static const struct drm_display_mode eink_ed103tc2_mode = {
+ .clock = 266693,
+ .hdisplay = 1872,
+ .hsync_start = 1872 + 184,
+ .hsync_end = 1872 + 184 + 88,
+ .htotal = 1872 + 184 + 88 + 64,
+ .hskew = 136,
+ .vdisplay = 1404,
+ .vsync_start = 1404 + 12,
+ .vsync_end = 1404 + 12 + 1,
+ .vtotal = 1404 + 12 + 1 + 4,
+ .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC |
+DRM_MODE_FLAG_HSKEW | DRM_MODE_FLAG_CLKDIV2,
+};
+
+static const struct panel_desc eink_ed103tc2 = {
+ .modes = _ed103tc2_mode,
+ .num_modes = 1,
+ .bpc = 4,
+ .size = {
+ .width = 210,
+ .height = 157,
+ },
+ .bus_format = MEDIA_BUS_FMT_FIXED,
+ .bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
+ .connector_type = DRM_MODE_CONNECTOR_DPI,
+};
+
static const struct display_timing eink_vb3300_kca_timing = {
.pixelclock = { 4000, 4000, 4000 },
.hactive = { 334, 334, 334 },
@@ -3807,6 +3835,9 @@ static const struct of_device_id platform_of_match[] = {
}, {
.compatible = "edt,etmv570g2dhu",
.data = _etmv570g2dhu,
+ }, {
+ .compatible = "eink,ed103tc2",
+ .data = _ed103tc2,
}, {
.compatible = "eink,vb3300-kca",
.data = _vb3300_kca,
--
2.35.1
[RFC PATCH 10/16] drm/rockchip: ebc: Implement partial refreshes
Several areas of the display can be refreshed concurrently, but only if
they do not overlap. This commit adds a queue of damaged areas, and
schedules them for refresh based on collision with other areas. While
the queue is unbounded, there is logic to quickly drop duplicate areas.
Because three-window mode disables the hardware's frame counter, a
separate buffer is required for each frame. (In other words, there is no
automatic increment.) To minimize overhead, swap between two buffers for
phase numbers. This requires extending the loop for one extra frame to
clear the phase numbers in both buffers when an area completes. (This
extra frame is a no-op and is not sent to the hardware.)
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 346 +++-
1 file changed, 344 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index cb6dc567e94c..c3e4b65bdee6 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -123,10 +124,14 @@
#define EBC_WIN_MST2 0x0058
#define EBC_LUT_DATA 0x1000
+#define EBC_FRAME_PENDING (-1U)
+
#define EBC_MAX_PHASES 256
+
#define EBC_NUM_LUT_REGS 0x1000
#define EBC_NUM_SUPPLIES 3
+#define EBC_FRAME_TIMEOUT msecs_to_jiffies(25)
#define EBC_REFRESH_TIMEOUTmsecs_to_jiffies(3000)
#define EBC_SUSPEND_DELAY_MS 2000
@@ -177,10 +182,25 @@ static const struct drm_mode_config_funcs
rockchip_ebc_mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
};
+/**
+ * struct rockchip_ebc_area - describes a damaged area of the display
+ *
+ * @list: Used to put this area in the state/context/refresh thread list
+ * @clip: The rectangular clip of this damage area
+ * @frame_begin: The frame number when this damage area starts being refreshed
+ */
+struct rockchip_ebc_area {
+ struct list_headlist;
+ struct drm_rect clip;
+ u32 frame_begin;
+};
+
/**
* struct rockchip_ebc_ctx - context for performing display refreshes
*
* @kref: Reference count, maintained as part of the CRTC's atomic state
+ * @queue: Queue of damaged areas to be refreshed
+ * @queue_lock: Lock protecting access to @queue
* @prev: Display contents (Y4) before this refresh
* @next: Display contents (Y4) after this refresh
* @final: Display contents (Y4) after all pending refreshes
@@ -192,6 +212,8 @@ static const struct drm_mode_config_funcs
rockchip_ebc_mode_config_funcs = {
*/
struct rockchip_ebc_ctx {
struct kref kref;
+ struct list_headqueue;
+ spinlock_t queue_lock;
u8 *prev;
u8 *next;
u8 *final;
@@ -204,6 +226,10 @@ struct rockchip_ebc_ctx {
static void rockchip_ebc_ctx_free(struct rockchip_ebc_ctx *ctx)
{
+ struct rockchip_ebc_area *area;
+
+ list_for_each_entry(area, >queue, list)
+ kfree(area);
kfree(ctx->prev);
kfree(ctx->next);
kfree(ctx->final);
@@ -234,6 +260,8 @@ static struct rockchip_ebc_ctx *rockchip_ebc_ctx_alloc(u32
width, u32 height)
}
kref_init(>kref);
+ INIT_LIST_HEAD(>queue);
+ spin_lock_init(>queue_lock);
ctx->gray4_pitch = width / 2;
ctx->gray4_size = gray4_size;
ctx->phase_pitch = width;
@@ -291,12 +319,204 @@ static void rockchip_ebc_global_refresh(struct
rockchip_ebc *ebc,
memcpy(ctx->prev, ctx->next, gray4_size);
}
+static bool rockchip_ebc_schedule_area(struct list_head *areas,
+ struct rockchip_ebc_area *area,
+ u32 current_frame, u32 num_phases)
+{
+ struct rockchip_ebc_area *other;
+ u32 frame_begin = current_frame;
+
+ list_for_each_entry(other, areas, list) {
+ struct drm_rect intersection;
+ u32 other_end;
+
+ /* Only consider areas before this one in the list. */
+ if (other == area)
+ break;
+
+ /* Skip areas that finish refresh before this area begins. */
+ other_end = other->frame_begin + num_phases;
+ if (other_end <= frame_begin)
+ continue;
+
+ /* If there is no collision, the areas are independent. */
+ intersection = area->clip;
+ if (!drm_rect_intersect(, >clip))
+ continue;
+
+ /* If the other area already started, wait until it finishes. */
+ if
[RFC PATCH 09/16] drm/rockchip: ebc: Implement global refreshes
The global refresh mode is used to initialize and clear the screen.
It is the most efficient refresh mode. It uses two pixel buffers (old
and new) and a frame count. The frame count is set to the number of
phases in the waveform. The hardware then looks up the combination of
(old pixel value, new pixel value, frame number) in the LUT and sends
the resulting polarity value to the display.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 48 -
1 file changed, 47 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index ca3173b28d1c..cb6dc567e94c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -127,6 +127,7 @@
#define EBC_NUM_LUT_REGS 0x1000
#define EBC_NUM_SUPPLIES 3
+#define EBC_REFRESH_TIMEOUTmsecs_to_jiffies(3000)
#define EBC_SUSPEND_DELAY_MS 2000
struct rockchip_ebc {
@@ -269,8 +270,23 @@ static void rockchip_ebc_global_refresh(struct
rockchip_ebc *ebc,
{
struct drm_device *drm = >drm;
u32 gray4_size = ctx->gray4_size;
+ struct device *dev = drm->dev;
- drm_dbg(drm, "global refresh\n");
+ dma_sync_single_for_device(dev, virt_to_phys(ctx->next),
+ gray4_size, DMA_TO_DEVICE);
+ dma_sync_single_for_device(dev, virt_to_phys(ctx->prev),
+ gray4_size, DMA_TO_DEVICE);
+
+ reinit_completion(>display_end);
+ regmap_write(ebc->regmap, EBC_CONFIG_DONE,
+EBC_CONFIG_DONE_REG_CONFIG_DONE);
+ regmap_write(ebc->regmap, EBC_DSP_START,
+ebc->dsp_start |
+EBC_DSP_START_DSP_FRM_TOTAL(ebc->lut.num_phases - 1) |
+EBC_DSP_START_DSP_FRM_START);
+ if (!wait_for_completion_timeout(>display_end,
+EBC_REFRESH_TIMEOUT))
+ drm_err(drm, "Refresh timed out!\n");
memcpy(ctx->prev, ctx->next, gray4_size);
}
@@ -289,6 +305,7 @@ static void rockchip_ebc_refresh(struct rockchip_ebc *ebc,
enum drm_epd_waveform waveform)
{
struct drm_device *drm = >drm;
+ u32 dsp_ctrl = 0, epd_ctrl = 0;
struct device *dev = drm->dev;
int ret, temperature;
@@ -334,11 +351,40 @@ static void rockchip_ebc_refresh(struct rockchip_ebc *ebc,
ebc->lut.buf, EBC_NUM_LUT_REGS);
}
+ regmap_write(ebc->regmap, EBC_DSP_START,
+ebc->dsp_start);
+
+ /*
+* The hardware has a separate bit for each mode, with some priority
+* scheme between them. For clarity, only set one bit at a time.
+*/
+ if (global_refresh) {
+ dsp_ctrl |= EBC_DSP_CTRL_DSP_LUT_MODE;
+ } else {
+ epd_ctrl |= EBC_EPD_CTRL_DSP_THREE_WIN_MODE;
+ }
+ regmap_update_bits(ebc->regmap, EBC_EPD_CTRL,
+ EBC_EPD_CTRL_DSP_THREE_WIN_MODE,
+ epd_ctrl);
+ regmap_update_bits(ebc->regmap, EBC_DSP_CTRL,
+ EBC_DSP_CTRL_DSP_LUT_MODE,
+ dsp_ctrl);
+
+ regmap_write(ebc->regmap, EBC_WIN_MST0,
+virt_to_phys(ctx->next));
+ regmap_write(ebc->regmap, EBC_WIN_MST1,
+virt_to_phys(ctx->prev));
+
if (global_refresh)
rockchip_ebc_global_refresh(ebc, ctx);
else
rockchip_ebc_partial_refresh(ebc, ctx);
+ /* Drive the output pins low once the refresh is complete. */
+ regmap_write(ebc->regmap, EBC_DSP_START,
+ebc->dsp_start |
+EBC_DSP_START_DSP_OUT_LOW);
+
pm_runtime_mark_last_busy(dev);
pm_runtime_put_autosuspend(dev);
}
--
2.35.1
[RFC PATCH 13/16] drm/rockchip: ebc: Add a panel reflection option
Some panels, like the one in the PineNote, are reflected. Since the
driver already has to copy pixels, the driver can handle this with
little additional overhead.
Currently, there is no devicetree binding for this situation, so control
the behavior via a module parameter.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index 93d689ff0933..9d0b2cdc5fdc 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -166,6 +166,10 @@ static bool direct_mode = true;
module_param(direct_mode, bool, 0444);
MODULE_PARM_DESC(direct_mode, "compute waveforms in software (software LUT)");
+static bool panel_reflection = true;
+module_param(panel_reflection, bool, 0644);
+MODULE_PARM_DESC(panel_reflection, "reflect the image horizontally");
+
static bool skip_reset;
module_param(skip_reset, bool, 0444);
MODULE_PARM_DESC(skip_reset, "skip the initial display reset");
@@ -1046,23 +1050,29 @@ static bool rockchip_ebc_blit_fb(const struct
rockchip_ebc_ctx *ctx,
{
unsigned int dst_pitch = ctx->gray4_pitch;
unsigned int src_pitch = fb->pitches[0];
- unsigned int x, y;
+ unsigned int start_x, x, y;
const void *src;
u8 changed = 0;
+ int delta_x;
void *dst;
+ delta_x = panel_reflection ? -1 : 1;
+ start_x = panel_reflection ? src_clip->x2 - 1 : src_clip->x1;
+
dst = ctx->final + dst_clip->y1 * dst_pitch + dst_clip->x1 / 2;
- src = vaddr + src_clip->y1 * src_pitch + src_clip->x1 *
fb->format->cpp[0];
+ src = vaddr + src_clip->y1 * src_pitch + start_x * fb->format->cpp[0];
for (y = src_clip->y1; y < src_clip->y2; y++) {
const u32 *sbuf = src;
u8 *dbuf = dst;
for (x = src_clip->x1; x < src_clip->x2; x += 2) {
- u32 rgb0 = *sbuf++;
- u32 rgb1 = *sbuf++;
+ u32 rgb0, rgb1;
u8 gray;
+ rgb0 = *sbuf; sbuf += delta_x;
+ rgb1 = *sbuf; sbuf += delta_x;
+
/* Truncate the RGB values to 5 bits each. */
rgb0 &= 0x00f8f8f8U; rgb1 &= 0x00f8f8f8U;
/* Put the sum 2R+5G+B in bits 24-31. */
@@ -1136,6 +1146,13 @@ static void rockchip_ebc_plane_atomic_update(struct
drm_plane *plane,
dst_clip->x2 += adjust;
src_clip.x2 += adjust;
+ if (panel_reflection) {
+ int x1 = dst_clip->x1, x2 = dst_clip->x2;
+
+ dst_clip->x1 = plane_state->dst.x2 - x2;
+ dst_clip->x2 = plane_state->dst.x2 - x1;
+ }
+
if (!rockchip_ebc_blit_fb(ctx, dst_clip, vaddr,
plane_state->fb, _clip)) {
/* Drop the area if the FB didn't actually change. */
--
2.35.1
[RFC PATCH 05/16] drm/rockchip: ebc: Add CRTC mode setting
EPDs require some additional timing data beyond what is normally
provided by drm_display_mode, as that struct is designed for CRTs/LCDs.
For example, EPDs care about the width and position of the gate driver
(vertical) clock pulse within a line.
EPDs also update some number of pixels in parallel, based on the
interface width, which of course varies by panel. Only two data bits are
used for each pixel, to choose between driving it positive, negative, or
neither direction. Color depth is thus not limited by interface width,
but by time (the number of phases in the active waveform).
This additional timing information is packed inside drm_display_mode as
hskew and DRM_MODE_FLAG_CLKDIV2. This allows getting the complete mode
from a DRM bridge.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 102
1 file changed, 102 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index f75fd23adda2..5f9502313657 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -135,6 +135,7 @@ struct rockchip_ebc {
struct drm_planeplane;
struct regmap *regmap;
struct regulator_bulk_data supplies[EBC_NUM_SUPPLIES];
+ u32 dsp_start;
};
DEFINE_DRM_GEM_FOPS(rockchip_ebc_fops);
@@ -178,11 +179,112 @@ static inline struct rockchip_ebc *crtc_to_ebc(struct
drm_crtc *crtc)
static void rockchip_ebc_crtc_mode_set_nofb(struct drm_crtc *crtc)
{
+ struct rockchip_ebc *ebc = crtc_to_ebc(crtc);
+ struct drm_display_mode mode = crtc->state->adjusted_mode;
+ struct drm_display_mode sdck;
+ u16 hsync_width, vsync_width;
+ u16 hact_start, vact_start;
+ u16 pixels_per_sdck;
+ bool bus_16bit;
+
+ /*
+* Hardware needs horizontal timings in SDCK (source driver clock)
+* cycles, not pixels. Bus width is either 8 bits (normal) or 16 bits
+* (DRM_MODE_FLAG_CLKDIV2), and each pixel uses two data bits.
+*/
+ bus_16bit = !!(mode.flags & DRM_MODE_FLAG_CLKDIV2);
+ pixels_per_sdck = bus_16bit ? 8 : 4;
+ sdck.hdisplay = mode.hdisplay / pixels_per_sdck;
+ sdck.hsync_start = mode.hsync_start / pixels_per_sdck;
+ sdck.hsync_end = mode.hsync_end / pixels_per_sdck;
+ sdck.htotal = mode.htotal / pixels_per_sdck;
+ sdck.hskew = mode.hskew / pixels_per_sdck;
+
+ /*
+* Linux timing order is display/fp/sync/bp. Hardware timing order is
+* sync/bp/display/fp, aka sync/start/display/end.
+*/
+ hact_start = sdck.htotal - sdck.hsync_start;
+ vact_start = mode.vtotal - mode.vsync_start;
+
+ hsync_width = sdck.hsync_end - sdck.hsync_start;
+ vsync_width = mode.vsync_end - mode.vsync_start;
+
+ clk_set_rate(ebc->dclk, mode.clock * 1000);
+
+ ebc->dsp_start = EBC_DSP_START_DSP_SDCE_WIDTH(sdck.hdisplay) |
+EBC_DSP_START_SW_BURST_CTRL;
+ regmap_write(ebc->regmap, EBC_EPD_CTRL,
+EBC_EPD_CTRL_DSP_GD_END(sdck.htotal - sdck.hskew) |
+EBC_EPD_CTRL_DSP_GD_ST(hsync_width + sdck.hskew) |
+EBC_EPD_CTRL_DSP_SDDW_MODE * bus_16bit);
+ regmap_write(ebc->regmap, EBC_DSP_CTRL,
+/* no swap */
+EBC_DSP_CTRL_DSP_SWAP_MODE(bus_16bit ? 2 : 3) |
+EBC_DSP_CTRL_DSP_SDCLK_DIV(pixels_per_sdck - 1));
+ regmap_write(ebc->regmap, EBC_DSP_HTIMING0,
+EBC_DSP_HTIMING0_DSP_HTOTAL(sdck.htotal) |
+/* sync end == sync width */
+EBC_DSP_HTIMING0_DSP_HS_END(hsync_width));
+ regmap_write(ebc->regmap, EBC_DSP_HTIMING1,
+EBC_DSP_HTIMING1_DSP_HACT_END(hact_start + sdck.hdisplay) |
+/* minus 1 for fixed delay in timing sequence */
+EBC_DSP_HTIMING1_DSP_HACT_ST(hact_start - 1));
+ regmap_write(ebc->regmap, EBC_DSP_VTIMING0,
+EBC_DSP_VTIMING0_DSP_VTOTAL(mode.vtotal) |
+/* sync end == sync width */
+EBC_DSP_VTIMING0_DSP_VS_END(vsync_width));
+ regmap_write(ebc->regmap, EBC_DSP_VTIMING1,
+EBC_DSP_VTIMING1_DSP_VACT_END(vact_start + mode.vdisplay) |
+EBC_DSP_VTIMING1_DSP_VACT_ST(vact_start));
+ regmap_write(ebc->regmap, EBC_DSP_ACT_INFO,
+EBC_DSP_ACT_INFO_DSP_HEIGHT(mode.vdisplay) |
+EBC_DSP_ACT_INFO_DSP_WIDTH(mode.hdisplay));
+ regmap_write(ebc->regmap, EBC_WIN_CTRL,
+/* FIFO depth - 16 */
+EBC_WIN_CTRL_WIN2_FIFO_THRESHOLD(496) |
+EBC_WIN_CTRL_WIN_EN |
+/* INCR16 */
+EBC_WIN_CTRL_AHB_BURST_REG(7) |
+/* FIFO depth - 16 */
+
[RFC PATCH 11/16] drm/rockchip: ebc: Enable diff mode for partial refreshes
Some waveforms, such as GC16, cause the display to flash even when the
previous and next pixel values are the same. This can be helpful,
because it produces more consistent brightness, but usually it is more
distracting. Add an option, enabled by default, for the hardware to
ignore the LUT and always send zeroes for pixels with unchanged values.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index c3e4b65bdee6..dcd8c8e8208e 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -158,6 +158,10 @@ static int default_waveform = DRM_EPD_WF_GC16;
module_param(default_waveform, int, 0644);
MODULE_PARM_DESC(default_waveform, "waveform to use for display updates");
+static bool diff_mode = true;
+module_param(diff_mode, bool, 0644);
+MODULE_PARM_DESC(diff_mode, "only compute waveforms for changed pixels");
+
static bool skip_reset;
module_param(skip_reset, bool, 0444);
MODULE_PARM_DESC(skip_reset, "skip the initial display reset");
@@ -582,11 +586,14 @@ static void rockchip_ebc_refresh(struct rockchip_ebc *ebc,
dsp_ctrl |= EBC_DSP_CTRL_DSP_LUT_MODE;
} else {
epd_ctrl |= EBC_EPD_CTRL_DSP_THREE_WIN_MODE;
+ if (diff_mode)
+ dsp_ctrl |= EBC_DSP_CTRL_DSP_DIFF_MODE;
}
regmap_update_bits(ebc->regmap, EBC_EPD_CTRL,
EBC_EPD_CTRL_DSP_THREE_WIN_MODE,
epd_ctrl);
regmap_update_bits(ebc->regmap, EBC_DSP_CTRL,
+ EBC_DSP_CTRL_DSP_DIFF_MODE |
EBC_DSP_CTRL_DSP_LUT_MODE,
dsp_ctrl);
--
2.35.1
[RFC PATCH 06/16] drm/rockchip: ebc: Add CRTC refresh thread
EPD refreshes are extremely slow; they take anywhere between hundreds of
milliseconds and several seconds. To avoid blocking userspace, perform
these refreshes on a separate thread. The thread will also take care of
initializing the display before first use and clearing it when the CRTC
is disabled.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 82 -
1 file changed, 81 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index 5f9502313657..ebe60d5e011a 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -6,6 +6,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -135,9 +136,15 @@ struct rockchip_ebc {
struct drm_planeplane;
struct regmap *regmap;
struct regulator_bulk_data supplies[EBC_NUM_SUPPLIES];
+ struct task_struct *refresh_thread;
u32 dsp_start;
+ boolreset_complete;
};
+static bool skip_reset;
+module_param(skip_reset, bool, 0444);
+MODULE_PARM_DESC(skip_reset, "skip the initial display reset");
+
DEFINE_DRM_GEM_FOPS(rockchip_ebc_fops);
static const struct drm_driver rockchip_ebc_drm_driver = {
@@ -172,6 +179,42 @@ to_ebc_crtc_state(struct drm_crtc_state *crtc_state)
return container_of(crtc_state, struct ebc_crtc_state, base);
}
+static int rockchip_ebc_refresh_thread(void *data)
+{
+ struct rockchip_ebc *ebc = data;
+
+ while (!kthread_should_stop()) {
+ /*
+* LUTs use both the old and the new pixel values as inputs.
+* However, the initial contents of the display are unknown.
+* The special RESET waveform will initialize the display to
+* known contents (white) regardless of its current contents.
+*/
+ if (!ebc->reset_complete) {
+ ebc->reset_complete = true;
+ drm_dbg(>drm, "display reset\n");
+ }
+
+ while (!kthread_should_park()) {
+ drm_dbg(>drm, "display update\n");
+
+ set_current_state(TASK_IDLE);
+ schedule();
+ __set_current_state(TASK_RUNNING);
+ }
+
+ /*
+* Clear the display before disabling the CRTC. Use the
+* highest-quality waveform to minimize visible artifacts.
+*/
+ drm_dbg(>drm, "display clear\n");
+
+ kthread_parkme();
+ }
+
+ return 0;
+}
+
static inline struct rockchip_ebc *crtc_to_ebc(struct drm_crtc *crtc)
{
return container_of(crtc, struct rockchip_ebc, crtc);
@@ -296,11 +339,23 @@ static void rockchip_ebc_crtc_atomic_flush(struct
drm_crtc *crtc,
static void rockchip_ebc_crtc_atomic_enable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
+ struct rockchip_ebc *ebc = crtc_to_ebc(crtc);
+ struct drm_crtc_state *crtc_state;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ if (crtc_state->mode_changed)
+ kthread_unpark(ebc->refresh_thread);
}
static void rockchip_ebc_crtc_atomic_disable(struct drm_crtc *crtc,
struct drm_atomic_state *state)
{
+ struct rockchip_ebc *ebc = crtc_to_ebc(crtc);
+ struct drm_crtc_state *crtc_state;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ if (crtc_state->mode_changed)
+ kthread_park(ebc->refresh_thread);
}
static const struct drm_crtc_helper_funcs rockchip_ebc_crtc_helper_funcs = {
@@ -408,6 +463,14 @@ static int rockchip_ebc_plane_atomic_check(struct
drm_plane *plane,
static void rockchip_ebc_plane_atomic_update(struct drm_plane *plane,
struct drm_atomic_state *state)
{
+ struct rockchip_ebc *ebc = plane_to_ebc(plane);
+ struct drm_plane_state *plane_state;
+
+ plane_state = drm_atomic_get_new_plane_state(state, plane);
+ if (!plane_state->crtc)
+ return;
+
+ wake_up_process(ebc->refresh_thread);
}
static const struct drm_plane_helper_funcs rockchip_ebc_plane_helper_funcs = {
@@ -673,6 +736,7 @@ static int rockchip_ebc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, ebc);
init_completion(>display_end);
+ ebc->reset_complete = skip_reset;
base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(base))
@@ -716,12 +780,26 @@ static int rockchip_ebc_probe(struct platform_device
*pdev)
return ret;
}
+ ebc->refresh_thread = kthread_create(rockchip_ebc_refresh_thread,
[RFC PATCH 07/16] drm/rockchip: ebc: Add CRTC buffer management
This commit adds the "context" structure which holds all buffers needed
to refresh the display. It is allocated separately from the CRTC state
because it is reused as long as no mode change occurs.
There are three buffers holding Y4 (grayscale 4 bits/pixel) pixel data:
- "prev" - contents of the display at the beginning of a refresh.
- "next" - contents of the display at the end of that refresh. When a
refresh finishes, the "next" buffer is copied into "prev".
- "final" - contents of the display at the end of the final refresh.
This buffer is necessary because a refresh waveform cannot be
modified or interrupted once it is started. If a pixel's value is
changed while it is already being refreshed, the "next" buffer
cannot be updated until the first waveform completes. At that time,
the "final" buffer is copied into "next", and another refresh is
started. The name "final" refers to the write-combining behavior of
this buffer; any number of plane updates can change this buffer
while waiting for the current refresh to complete.
Then there are two buffers holding "phase" data. These are only used
during partial refreshes. The phase represents the time component (the X
coordinate) of the waveform. Since the EBC supports a maximum of 256
phases in a waveform, the phase number requires one byte per pixel. The
driver swaps between two buffers to minimize the delay between phases,
as these buffers must be updated for every phase in the waveform.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 166 +++-
1 file changed, 163 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index ebe60d5e011a..095d66e67c2f 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -141,6 +142,10 @@ struct rockchip_ebc {
boolreset_complete;
};
+static int default_waveform = DRM_EPD_WF_GC16;
+module_param(default_waveform, int, 0644);
+MODULE_PARM_DESC(default_waveform, "waveform to use for display updates");
+
static bool skip_reset;
module_param(skip_reset, bool, 0444);
MODULE_PARM_DESC(skip_reset, "skip the initial display reset");
@@ -165,12 +170,86 @@ static const struct drm_mode_config_funcs
rockchip_ebc_mode_config_funcs = {
.atomic_commit = drm_atomic_helper_commit,
};
+/**
+ * struct rockchip_ebc_ctx - context for performing display refreshes
+ *
+ * @kref: Reference count, maintained as part of the CRTC's atomic state
+ * @prev: Display contents (Y4) before this refresh
+ * @next: Display contents (Y4) after this refresh
+ * @final: Display contents (Y4) after all pending refreshes
+ * @phase: Buffers for selecting a phase from the EBC's LUT, 1 byte/pixel
+ * @gray4_pitch: Horizontal line length of a Y4 pixel buffer in bytes
+ * @gray4_size: Size of a Y4 pixel buffer in bytes
+ * @phase_pitch: Horizontal line length of a phase buffer in bytes
+ * @phase_size: Size of a phase buffer in bytes
+ */
+struct rockchip_ebc_ctx {
+ struct kref kref;
+ u8 *prev;
+ u8 *next;
+ u8 *final;
+ u8 *phase[2];
+ u32 gray4_pitch;
+ u32 gray4_size;
+ u32 phase_pitch;
+ u32 phase_size;
+};
+
+static void rockchip_ebc_ctx_free(struct rockchip_ebc_ctx *ctx)
+{
+ kfree(ctx->prev);
+ kfree(ctx->next);
+ kfree(ctx->final);
+ kfree(ctx->phase[0]);
+ kfree(ctx->phase[1]);
+ kfree(ctx);
+}
+
+static struct rockchip_ebc_ctx *rockchip_ebc_ctx_alloc(u32 width, u32 height)
+{
+ u32 gray4_size = width * height / 2;
+ u32 phase_size = width * height;
+ struct rockchip_ebc_ctx *ctx;
+
+ ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return NULL;
+
+ ctx->prev = kmalloc(gray4_size, GFP_KERNEL);
+ ctx->next = kmalloc(gray4_size, GFP_KERNEL);
+ ctx->final = kmalloc(gray4_size, GFP_KERNEL);
+ ctx->phase[0] = kmalloc(phase_size, GFP_KERNEL);
+ ctx->phase[1] = kmalloc(phase_size, GFP_KERNEL);
+ if (!ctx->prev || !ctx->next || !ctx->final ||
+ !ctx->phase[0] || !ctx->phase[1]) {
+ rockchip_ebc_ctx_free(ctx);
+ return NULL;
+ }
+
+ kref_init(>kref);
+ ctx->gray4_pitch = width / 2;
+ ctx->gray4_size = gray4_size;
+ ctx->phase_pitch = width;
+ ctx->phase_size = phase_size;
+
+ return ctx;
+}
+
+static void rockchip_ebc_ctx_release(struct kref *kref)
+{
+ struct rockchip_ebc_ctx *ctx =
+
[RFC PATCH 08/16] drm/rockchip: ebc: Add LUT loading
The EBC contains a 16 KiB SRAM which stores the current LUT. It needs to
be programmed any time the LUT changes or the hardware block is enabled.
Since both of these triggers can happen at the same time, use a flag to
avoid writing the LUT twice.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/Kconfig| 3 +-
drivers/gpu/drm/rockchip/rockchip_ebc.c | 76 +
2 files changed, 78 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig
index 9d3273a5fd97..efe4476e336d 100644
--- a/drivers/gpu/drm/rockchip/Kconfig
+++ b/drivers/gpu/drm/rockchip/Kconfig
@@ -94,7 +94,8 @@ endif
config DRM_ROCKCHIP_EBC
tristate "DRM Support for Rockchip EBC"
- depends on DRM
+ depends on DRM && IIO
+ select DRM_EPD_HELPER
select DRM_GEM_SHMEM_HELPER
select DRM_KMS_HELPER
help
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index 095d66e67c2f..ca3173b28d1c 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include
#include
@@ -122,6 +123,7 @@
#define EBC_WIN_MST2 0x0058
#define EBC_LUT_DATA 0x1000
+#define EBC_MAX_PHASES 256
#define EBC_NUM_LUT_REGS 0x1000
#define EBC_NUM_SUPPLIES 3
@@ -134,11 +136,15 @@ struct rockchip_ebc {
struct drm_crtc crtc;
struct drm_device drm;
struct drm_encoder encoder;
+ struct drm_epd_lut lut;
+ struct drm_epd_lut_file lut_file;
struct drm_planeplane;
+ struct iio_channel *temperature_channel;
struct regmap *regmap;
struct regulator_bulk_data supplies[EBC_NUM_SUPPLIES];
struct task_struct *refresh_thread;
u32 dsp_start;
+ boollut_changed;
boolreset_complete;
};
@@ -282,10 +288,59 @@ static void rockchip_ebc_refresh(struct rockchip_ebc *ebc,
bool global_refresh,
enum drm_epd_waveform waveform)
{
+ struct drm_device *drm = >drm;
+ struct device *dev = drm->dev;
+ int ret, temperature;
+
+ /* Resume asynchronously while preparing to refresh. */
+ ret = pm_runtime_get(dev);
+ if (ret < 0) {
+ drm_err(drm, "Failed to request resume: %d\n", ret);
+ return;
+ }
+
+ ret = iio_read_channel_processed(ebc->temperature_channel,
);
+ if (ret < 0) {
+ drm_err(drm, "Failed to get temperature: %d\n", ret);
+ } else {
+ /* Convert from millicelsius to celsius. */
+ temperature /= 1000;
+
+ ret = drm_epd_lut_set_temperature(>lut, temperature);
+ if (ret < 0)
+ drm_err(drm, "Failed to set LUT temperature: %d\n",
ret);
+ else if (ret)
+ ebc->lut_changed = true;
+ }
+
+ ret = drm_epd_lut_set_waveform(>lut, waveform);
+ if (ret < 0)
+ drm_err(drm, "Failed to set LUT waveform: %d\n", ret);
+ else if (ret)
+ ebc->lut_changed = true;
+
+ /* Wait for the resume to complete before writing any registers. */
+ ret = pm_runtime_resume(dev);
+ if (ret < 0) {
+ drm_err(drm, "Failed to resume: %d\n", ret);
+ pm_runtime_put(dev);
+ return;
+ }
+
+ /* This flag may have been set above, or by the runtime PM callback. */
+ if (ebc->lut_changed) {
+ ebc->lut_changed = false;
+ regmap_bulk_write(ebc->regmap, EBC_LUT_DATA,
+ ebc->lut.buf, EBC_NUM_LUT_REGS);
+ }
+
if (global_refresh)
rockchip_ebc_global_refresh(ebc, ctx);
else
rockchip_ebc_partial_refresh(ebc, ctx);
+
+ pm_runtime_mark_last_busy(dev);
+ pm_runtime_put_autosuspend(dev);
}
static int rockchip_ebc_refresh_thread(void *data)
@@ -708,6 +763,15 @@ static int rockchip_ebc_drm_init(struct rockchip_ebc *ebc)
struct drm_bridge *bridge;
int ret;
+ ret = drmm_epd_lut_file_init(drm, >lut_file, "rockchip/ebc.wbf");
+ if (ret)
+ return ret;
+
+ ret = drmm_epd_lut_init(>lut_file, >lut,
+ DRM_EPD_LUT_4BIT_PACKED, EBC_MAX_PHASES);
+ if (ret)
+ return ret;
+
ret = drmm_mode_config_init(drm);
if (ret)
return ret;
@@ -810,6 +874,13 @@ static int rockchip_ebc_runtime_resume(struct device *dev)
if (ret)
[RFC PATCH 03/16] drm/rockchip: Add EBC platform driver skeleton
The Rockchip E-Book Controller (EBC) is a timing controller (TCON) responsible for sending timing signals and pixel update waveforms to an electrophoretic display (EPD). The EBC has several modes of operation. In direct mode, it reads precomputed source driver polarity data from a series of buffers in RAM. In the other modes, it reads pixel luminance data from RAM, and uses a lookup table (LUT) to compute the source driver polarity for each phase within the waveform. This commit adds a platform driver skeleton for the EBC, containing the IRQ handler and runtime PM hooks. The EBC only needs to be powered up when the display is actively being refreshed. regcache is used to allow configuration changes (i.e. modeset) while the EBC is powered down. While two of the regulator consumers here actually power the display panel, not the EBC hardware, they are consumed here because again they are only needed during display refreshes. They do not match the normal panel prepare/enable lifecycle. Signed-off-by: Samuel Holland --- drivers/gpu/drm/Makefile| 2 +- drivers/gpu/drm/rockchip/Kconfig| 11 + drivers/gpu/drm/rockchip/Makefile | 2 + drivers/gpu/drm/rockchip/rockchip_ebc.c | 324 4 files changed, 338 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/rockchip/rockchip_ebc.c diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 49380ccfe9d6..e940f81a2acf 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -93,7 +93,7 @@ obj-$(CONFIG_DRM_VGEM)+= vgem/ obj-$(CONFIG_DRM_VKMS) += vkms/ obj-$(CONFIG_DRM_NOUVEAU) +=nouveau/ obj-$(CONFIG_DRM_EXYNOS) +=exynos/ -obj-$(CONFIG_DRM_ROCKCHIP) +=rockchip/ +obj-y +=rockchip/ obj-$(CONFIG_DRM_GMA500) += gma500/ obj-$(CONFIG_DRM_UDL) += udl/ obj-$(CONFIG_DRM_AST) += ast/ diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig index fa5cfda4e90e..9d3273a5fd97 100644 --- a/drivers/gpu/drm/rockchip/Kconfig +++ b/drivers/gpu/drm/rockchip/Kconfig @@ -91,3 +91,14 @@ config ROCKCHIP_RK3066_HDMI for the RK3066 HDMI driver. If you want to enable HDMI on RK3066 based SoC, you should select this option. endif + +config DRM_ROCKCHIP_EBC + tristate "DRM Support for Rockchip EBC" + depends on DRM + select DRM_GEM_SHMEM_HELPER + select DRM_KMS_HELPER + help + This selects DRM/KMS support for the Rockchip E-Book Controller (EBC). + Choose this option if you have a Rockchip SoC and an electrophoretic + display. This hardware and driver is separate from the normal Rockchip + display hardware and DRM driver. diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile index 1a56f696558c..e3accc526438 100644 --- a/drivers/gpu/drm/rockchip/Makefile +++ b/drivers/gpu/drm/rockchip/Makefile @@ -16,3 +16,5 @@ rockchipdrm-$(CONFIG_ROCKCHIP_RGB) += rockchip_rgb.o rockchipdrm-$(CONFIG_ROCKCHIP_RK3066_HDMI) += rk3066_hdmi.o obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o + +obj-$(CONFIG_DRM_ROCKCHIP_EBC) += rockchip_ebc.o diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c b/drivers/gpu/drm/rockchip/rockchip_ebc.c new file mode 100644 index ..5ed66c6cd2f0 --- /dev/null +++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c @@ -0,0 +1,324 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021-2022 Samuel Holland + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#define EBC_DSP_START 0x +#define EBC_DSP_START_DSP_OUT_LOW BIT(31) +#define EBC_DSP_START_DSP_SDCE_WIDTH(x)((x) << 16) +#define EBC_DSP_START_DSP_EINK_MODEBIT(13) +#define EBC_DSP_START_SW_BURST_CTRLBIT(12) +#define EBC_DSP_START_DSP_FRM_TOTAL(x) ((x) << 2) +#define EBC_DSP_START_DSP_RST BIT(1) +#define EBC_DSP_START_DSP_FRM_STARTBIT(0) +#define EBC_EPD_CTRL 0x0004 +#define EBC_EPD_CTRL_EINK_MODE_SWAPBIT(31) +#define EBC_EPD_CTRL_DSP_GD_END(x) ((x) << 16) +#define EBC_EPD_CTRL_DSP_GD_ST(x) ((x) << 8) +#define EBC_EPD_CTRL_DSP_THREE_WIN_MODEBIT(7) +#define EBC_EPD_CTRL_DSP_SDDW_MODE BIT(6) +#define EBC_EPD_CTRL_EPD_AUO BIT(5) +#define EBC_EPD_CTRL_EPD_PWR(x)((x) << 2) +#define EBC_EPD_CTRL_EPD_GDRL BIT(1) +#define EBC_EPD_CTRL_EPD_SDSHR BIT(0) +#define EBC_DSP_CTRL 0x0008 +#define EBC_DSP_CTRL_DSP_SWAP_MODE(x) ((x) << 30) +#define EBC_DSP_CTRL_DSP_DIFF_MODE BIT(29) +#define EBC_DSP_CTRL_DSP_LUT_MODE BIT(28) +#define EBC_DSP_CTRL_DSP_VCOM_MODE BIT(27) +#define EBC_DSP_CTRL_DSP_GDOE_POL BIT(26) +#define EBC_DSP_CTRL_DSP_GDSP_POL BIT(25) +#define
[RFC PATCH 04/16] drm/rockchip: ebc: Add DRM driver skeleton
The Rockchip E-Book Controller (EBC) has a relatively simple and self-
contained display pipeline. The pipeline consists of a single CRTC,
encoder, and bridge, with the bridge normally attached to a panel.
Initially, there is also a single plane. Since all blitting is done in
software, the driver could eventually support any number of planes. For
example, it could expose one plane for each supported waveform.
However, EPD controller hardware has some unique properties which
complicate using drm_simple_display_pipe:
- EPDs operate on relative pixel values, not absolute pixel values.
This requires the driver to maintain multiple shadow buffers for the
"previous" and "next" framebuffer contents.
- It also means that disabling the CRTC (i.e. clearing the screen)
requires access to these shadow buffers, as it requires knowing the
previous contents of the framebuffer. And of course it requires a
buffer for the blank image.
- Atomically managing these shadow buffers needs reference counting in
.atomic_check. However, drm_simple_display_pipe_funcs::check is only
called while the plane is visible, complicating this.
- Furthermore, because all plane blitting/blending must be done in
software, the number and location of these planes is arbitrary.
drm_simple_display_pipe enforces an unnecessary limitation that a
single plane covers the entire CRTC.
For these reasons, drm_simple_display_pipe is not used.
This commit adds the structure for this pipeline. The atomic helper
callbacks are left empty. They will be filled in incrementally by the
next several commits.
Both the CRTC and the pipe need extra state information, so this commit
adds the state hook boilerplate. Additionally, the plane takes advantage
of the shadow plane helpers.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/rockchip/rockchip_ebc.c | 359 +++-
1 file changed, 356 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_ebc.c
b/drivers/gpu/drm/rockchip/rockchip_ebc.c
index 5ed66c6cd2f0..f75fd23adda2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_ebc.c
+++ b/drivers/gpu/drm/rockchip/rockchip_ebc.c
@@ -12,6 +12,17 @@
#include
#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
#define EBC_DSP_START 0x
#define EBC_DSP_START_DSP_OUT_LOW BIT(31)
#define EBC_DSP_START_DSP_SDCE_WIDTH(x)((x) << 16)
@@ -118,10 +129,332 @@ struct rockchip_ebc {
struct clk *dclk;
struct clk *hclk;
struct completion display_end;
+ struct drm_crtc crtc;
+ struct drm_device drm;
+ struct drm_encoder encoder;
+ struct drm_planeplane;
struct regmap *regmap;
struct regulator_bulk_data supplies[EBC_NUM_SUPPLIES];
};
+DEFINE_DRM_GEM_FOPS(rockchip_ebc_fops);
+
+static const struct drm_driver rockchip_ebc_drm_driver = {
+ .lastclose = drm_fb_helper_lastclose,
+ DRM_GEM_SHMEM_DRIVER_OPS,
+ .major = 0,
+ .minor = 3,
+ .name = "rockchip-ebc",
+ .desc = "Rockchip E-Book Controller",
+ .date = "20220303",
+ .driver_features= DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+ .fops = _ebc_fops,
+};
+
+static const struct drm_mode_config_funcs rockchip_ebc_mode_config_funcs = {
+ .fb_create = drm_gem_fb_create_with_dirty,
+ .atomic_check = drm_atomic_helper_check,
+ .atomic_commit = drm_atomic_helper_commit,
+};
+
+/*
+ * CRTC
+ */
+
+struct ebc_crtc_state {
+ struct drm_crtc_state base;
+};
+
+static inline struct ebc_crtc_state *
+to_ebc_crtc_state(struct drm_crtc_state *crtc_state)
+{
+ return container_of(crtc_state, struct ebc_crtc_state, base);
+}
+
+static inline struct rockchip_ebc *crtc_to_ebc(struct drm_crtc *crtc)
+{
+ return container_of(crtc, struct rockchip_ebc, crtc);
+}
+
+static void rockchip_ebc_crtc_mode_set_nofb(struct drm_crtc *crtc)
+{
+}
+
+static int rockchip_ebc_crtc_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+ return 0;
+}
+
+static void rockchip_ebc_crtc_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+}
+
+static void rockchip_ebc_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_state *state)
+{
+}
+
+static void rockchip_ebc_crtc_atomic_disable(struct drm_crtc *crtc,
+struct drm_atomic_state *state)
+{
+}
+
+static const struct drm_crtc_helper_funcs
[RFC PATCH 01/16] drm: Add a helper library for EPD drivers
Currently, this library is focused on LUTs and LUT files, specifically
the PVI wbf-format files shipped with E-Ink displays. It provides
helpers to load and validate LUT files, and extract LUTs from them.
Since EPD controllers need the LUTs in various formats, this library
allows drivers to declare their desired format. It will then convert
LUTs to that format before returning them.
Signed-off-by: Samuel Holland
---
drivers/gpu/drm/Kconfig | 6 +
drivers/gpu/drm/Makefile | 2 +
drivers/gpu/drm/drm_epd_helper.c | 663 +++
include/drm/drm_epd_helper.h | 104 +
4 files changed, 775 insertions(+)
create mode 100644 drivers/gpu/drm/drm_epd_helper.c
create mode 100644 include/drm/drm_epd_helper.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index f1422bee3dcc..ad96cf605444 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -198,6 +198,12 @@ config DRM_DP_CEC
Note: not all adapters support this feature, and even for those
that do support this they often do not hook up the CEC pin.
+config DRM_EPD_HELPER
+ tristate
+ depends on DRM
+ help
+ Choose this if you need the EPD (LUT, etc.) helper functions
+
config DRM_TTM
tristate
depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index c2ef5f9fce54..49380ccfe9d6 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -33,6 +33,8 @@ drm-$(CONFIG_DRM_PRIVACY_SCREEN) += drm_privacy_screen.o
drm_privacy_screen_x86.
obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o
+obj-$(CONFIG_DRM_EPD_HELPER) += drm_epd_helper.o
+
drm_cma_helper-y := drm_gem_cma_helper.o
drm_cma_helper-$(CONFIG_DRM_KMS_HELPER) += drm_fb_cma_helper.o
obj-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_cma_helper.o
diff --git a/drivers/gpu/drm/drm_epd_helper.c b/drivers/gpu/drm/drm_epd_helper.c
new file mode 100644
index ..433a6728ef3e
--- /dev/null
+++ b/drivers/gpu/drm/drm_epd_helper.c
@@ -0,0 +1,663 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Electrophoretic Display Helper Library
+ *
+ * Copyright (C) 2022 Samuel Holland
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDERS, AUTHORS AND/OR ITS SUPPLIERS BE LIABLE FOR ANY CLAIM,
+ * DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+ * USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+/**
+ * DOC: overview
+ *
+ * This library provides functions for working with the lookup tables (LUTs)
+ * used by electrophoretic displays (EPDs). It fills in a LUT buffer based on
+ * the selected waveform, the panel temperature, and the buffer format needed
+ * by the driver.
+ */
+
+struct pvi_wbf_offset {
+ u8 b[3];
+};
+
+struct pvi_wbf_pointer {
+ struct pvi_wbf_offset offset;
+ u8 checksum;
+};
+
+struct pvi_wbf_file_header {
+ __le32 checksum; // 0x00
+ __le32 file_size; // 0x04
+ __le32 serial; // 0x08
+ u8 run_type; // 0x0c
+ u8 fpl_platform; // 0x0d
+ __le16 fpl_lot;// 0x0e
+ u8 mode_version; // 0x10
+ u8 wf_version; // 0x11
+ u8 wf_subversion; // 0x12
+ u8 wf_type;// 0x13
+ u8 panel_size; // 0x14
+ u8 amepd_part_number; // 0x15
+ u8 wf_rev; // 0x16
+ u8 frame_rate_bcd; // 0x17
+ u8 frame_rate_hex; // 0x18
+ u8 vcom_offset;// 0x19
+ u8
[RFC PATCH 02/16] dt-bindings: display: rockchip: Add EBC binding
The Rockchip E-Book Controller (EBC) is a controller for Electrophoretic
Displays (EPDs). It is self-contained; it does not interact directly
with the VOP or the RGA.
While two of the regulator consumers here actually power the display
panel, not the EBC hardware, they are consumed here because they are
only needed during display refreshes. They do not match the normal
panel prepare/enable lifecycle.
Signed-off-by: Samuel Holland
---
.../display/rockchip/rockchip,rk3568-ebc.yaml | 106 ++
1 file changed, 106 insertions(+)
create mode 100644
Documentation/devicetree/bindings/display/rockchip/rockchip,rk3568-ebc.yaml
diff --git
a/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3568-ebc.yaml
b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3568-ebc.yaml
new file mode 100644
index ..957ca874ab02
--- /dev/null
+++
b/Documentation/devicetree/bindings/display/rockchip/rockchip,rk3568-ebc.yaml
@@ -0,0 +1,106 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/rockchip/rockchip,rk3568-ebc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip SoC E-Book Controller (EBC)
+
+description:
+ Rockchip EBC is a controller for Electrophoretic Displays (EPDs).
+
+maintainers:
+ - Samuel Holland
+
+properties:
+ compatible:
+enum:
+ - rockchip,rk3568-ebc
+
+ reg:
+maxItems: 1
+
+ interrupts:
+maxItems: 1
+
+ clocks:
+items:
+ - description: AHB register clock
+ - description: Pixel clock
+
+ clock-names:
+items:
+ - const: hclk
+ - const: dclk
+
+ resets:
+items:
+ - description: hclk domain reset
+ - description: dclk domain reset
+
+ reset-names:
+items:
+ - const: hclk
+ - const: dclk
+
+ io-channels:
+maxItems: 1
+description: I/O channel for panel temperature measurement
+
+ panel-supply:
+description: Regulator supplying the panel's logic voltage
+
+ power-domains:
+maxItems: 1
+
+ vcom-supply:
+description: Regulator supplying the panel's compensation voltage
+
+ vdrive-supply:
+description: Regulator supplying the panel's gate and source drivers
+
+ port:
+$ref: /schemas/graph.yaml#/properties/port
+description: OF graph port for the attached display panel
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - clock-names
+ - resets
+ - reset-names
+ - power-domains
+ - panel-supply
+ - vcom-supply
+ - vdrive-supply
+
+additionalProperties: false
+
+examples:
+ - |
+#include
+#include
+#include
+
+ebc: ebc@fdec {
+ compatible = "rockchip,rk3568-ebc";
+ reg = <0x0 0xfdec 0x0 0x5000>;
+ interrupts = ;
+ clocks = < HCLK_EBC>, < DCLK_EBC>;
+ clock-names = "hclk", "dclk";
+ resets = < SRST_H_EBC>, < SRST_D_EBC>;
+ reset-names = "hclk", "dclk";
+ power-domains = < RK3568_PD_RGA>;
+
+ panel-supply = <>;
+ vcom-supply = <>;
+ vdrive-supply = <>;
+
+ port {
+ebc_out_panel: endpoint {
+ remote-endpoint = <_in_ebc>;
+};
+ };
+};
--
2.35.1
[RFC PATCH 00/16] drm/rockchip: Rockchip EBC ("E-Book Controller") display driver
This series adds a DRM driver for the electrophoretic display controller
found in a few different Rockchip SoCs, specifically the RK3566/RK3568
variant[0] used by the PineNote tablet[1].
This is my first real involvement with the DRM subsystem, so please let
me know where I am misunderstanding things.
This is now the second SoC-integrated EPD controller with a DRM driver
submission -- the first one being the i.MX6 EPDC[2]. I want to thank
Andreas for sending that series, and for his advice while writing this
driver.
One goal I have with sending this series is to discuss how to support
EPDs more generally within the DRM subsystem, so the interfaces with
panels and PMICs and waveform LUTs can be controller-independent.
My understanding is that the i.MX6 EPDC series is at least partly based
on the downstream vendor driver. This driver is a clean-sheet design for
hardware with different (read: fewer) capabilities, so we took some
different design paths, but we ran into many of the same sharp edges.
Here are some of the areas I would like input on:
Panel Lifecycle
===
Panels use prepare/unprepare callbacks for their power supply. EPDs
should only be powered up when the display contents are changed. Should
the controller call both drm_panel_(un)prepare during each atomic update
when the framebuffer is dirty?
Similarly, panel enable/disable callbacks are tied to backlight state.
For an EPD, it makes sense to have the backlight enabled while the panel
is powered down (because the contents are static). Is it acceptable to
call drm_panel_{en,dis}able while the panel is not prepared?
With panel_bridge, the "normal" callback ordering is enforced, and tied
to the atomic state, so neither of these is possible.
As a result, neither the backlight nor the voltage regulators are tied
to the panel. The panel's regulators are consumed by the EBC itself.
Panel Timing Parameters
===
EPDs have more timing parameters than LCDs, and there are several
different ways of labeling these parameters. See for example the timing
diagrams on pp. 2237-2239 of the RK3568 TRM[0], the descriptions in the
ED103TC2 panel datasheet[3], and the submitted EPDC bindings[2].
Both the EPDC and EBC vendor drivers put all of the timing parameters in
the controller's OF node. There is no panel device/node.
I was able to squeeze everything needed for my specific case into a
struct drm_display_mode (see patches 5 and 14), but I don't know if this
is an acceptable use of those fields, or if will work with other
controllers. Is adding more fields to drm_display_mode an option?
See also the discussion of "dumb" LCD TCONs below.
Panel Connector Type / Media Bus Format
===
The EBC supports either an 8-bit or 16-bit wide data bus, where each
pair of data lines represents the source driver polarity (positive,
negative, or neutral) for a pixel.
The only effect of the data bus width is the number of pixels that are
transferred per clock cycle. It has no impact on the number of possible
grayscale levels.
How does that translate to DRM_MODE_CONNECTOR_* or MEDIA_BUS_FMT_*?
Panel Reflection
The ED103TC2 panel scans from right to left. Currently, there is no API
or OF property to represent this. I can add something similar to
drm_panel_orientation.
Should this be exposed to userspace? It is acceptable for the kernel
driver to flip the image when blitting from the framebuffer?
CRTC "active" and "enabled" states
==
What do these states mean in the context of an EPD? Currently, the
driver ignores "active" and clears the screen to solid white when the
CRTC is disabled.
The vendor drivers can switch to a user-provided image when the CRTC is
disabled. Is this something that can/should be supported upstream? If
so, how? Would userspace provide the image to the kernel, or just tell
the kernel not to clear the screen?
VBLANK Events and Asynchronous Commits
==
When should the VBLANK event complete? When the pixels have been blitted
to the kernel's shadow buffer? When the first frame of the waveform is
sent to the panel? When the last frame is sent to the panel?
Currently, the driver is taking the first option, letting
drm_atomic_helper_fake_vblank() send the VBLANK event without waiting on
the refresh thread. This is the only way I was able to get good
performance with existing userspace.
Waveform Loading
Waveform files are calibrated for each batch of panels. So while a
single waveform file may be "good enough" for all panels of a certain
model, the correctly-calibrated file will have better image quality.
I don't know of a good way to choose the calibrated file. Even the
board's compatible string may not be specific enough, if the board is
manufactured with multiple batches of panels.
Maybe the filename should just be the panel compatible, and the user is
responsible for putting the
Re: [PATCH 1/4] drm/i915/doc: Convert drm_i915_query_topology_info comment to kerneldoc
Looks good to me, series is:
Reviewed-by: Francisco Jerez
Matt Roper writes:
> This structure has a great comment describing the fields, but it's not
> currently in kerneldoc form and does not show up in the generated
> documentation. Let's fix that and also clarify the description of what
> "subslice" refers to on gen12 platforms and beyond and that "slice" is
> no longer meaningful on Xe_HP and beyond.
>
> Signed-off-by: Matt Roper
> ---
> include/uapi/drm/i915_drm.h | 110 +---
> 1 file changed, 78 insertions(+), 32 deletions(-)
>
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index 9ab021c4d632..73e1c6180ddb 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -2775,66 +2775,112 @@ struct drm_i915_query {
> __u64 items_ptr;
> };
>
> -/*
> - * Data written by the kernel with query DRM_I915_QUERY_TOPOLOGY_INFO :
> - *
> - * data: contains the 3 pieces of information :
> - *
> - * - the slice mask with one bit per slice telling whether a slice is
> - * available. The availability of slice X can be queried with the following
> - * formula :
> - *
> - * (data[X / 8] >> (X % 8)) & 1
> - *
> - * - the subslice mask for each slice with one bit per subslice telling
> - * whether a subslice is available. Gen12 has dual-subslices, which are
> - * similar to two gen11 subslices. For gen12, this array represents dual-
> - * subslices. The availability of subslice Y in slice X can be queried
> - * with the following formula :
> - *
> - * (data[subslice_offset +
> - * X * subslice_stride +
> - * Y / 8] >> (Y % 8)) & 1
> - *
> - * - the EU mask for each subslice in each slice with one bit per EU telling
> - * whether an EU is available. The availability of EU Z in subslice Y in
> - * slice X can be queried with the following formula :
> +/**
> + * struct drm_i915_query_topology_info
> *
> - * (data[eu_offset +
> - * (X * max_subslices + Y) * eu_stride +
> - * Z / 8] >> (Z % 8)) & 1
> + * Describes slice/subslice/EU information queried by
> + * %DRM_I915_QUERY_TOPOLOGY_INFO
> */
> struct drm_i915_query_topology_info {
> - /*
> + /**
> + * @flags:
> + *
>* Unused for now. Must be cleared to zero.
>*/
> __u16 flags;
>
> + /**
> + * @max_slices:
> + *
> + * The number of bits used to express the slice mask.
> + */
> __u16 max_slices;
> +
> + /**
> + * @max_subslices:
> + *
> + * The number of bits used to express the subslice mask.
> + */
> __u16 max_subslices;
> +
> + /**
> + * @max_eus_per_subslice:
> + *
> + * The number of bits in the EU mask that correspond to a single
> + * subslice's EUs.
> + */
> __u16 max_eus_per_subslice;
>
> - /*
> + /**
> + * @subslice_offset:
> + *
>* Offset in data[] at which the subslice masks are stored.
>*/
> __u16 subslice_offset;
>
> - /*
> + /**
> + * @subslice_stride:
> + *
>* Stride at which each of the subslice masks for each slice are
>* stored.
>*/
> __u16 subslice_stride;
>
> - /*
> + /**
> + * @eu_offset:
> + *
>* Offset in data[] at which the EU masks are stored.
>*/
> __u16 eu_offset;
>
> - /*
> + /**
> + * @eu_stride:
> + *
>* Stride at which each of the EU masks for each subslice are stored.
>*/
> __u16 eu_stride;
>
> + /**
> + * @data:
> + *
> + * Contains 3 pieces of information :
> + *
> + * - The slice mask with one bit per slice telling whether a slice is
> + * available. The availability of slice X can be queried with the
> + * following formula :
> + *
> + * .. code:: c
> + *
> + * (data[X / 8] >> (X % 8)) & 1
> + *
> + * Starting with Xe_HP platforms, Intel hardware no longer has
> + * traditional slices so i915 will always report a single slice
> + * (hardcoded slicemask = 0x1) which contains all of the platform's
> + * subslices. I.e., the mask here does not reflect any of the newer
> + * hardware concepts such as "gslices" or "cslices" since userspace
> + * is capable of inferring those from the subslice mask.
> + *
> + * - The subslice mask for each slice with one bit per subslice telling
> + * whether a subslice is available. Starting with Gen12 we use the
> + * term "subslice" to refer to what the hardware documentation
> + * describes as a "dual-subslices." The availability of subslice Y
> + * in slice X can be queried with the following formula :
> + *
> + * .. code:: c
> + *
> + * (data[subslice_offset + X * subslice_stride + Y / 8] >> (Y %
> 8))
Re: [PATCH v4,2/3] dt-bindings: display: mediatek: dsi: Add compatible for MediaTek MT8186
On Sat, 09 Apr 2022 17:11:53 +0800, xinlei@mediatek.com wrote: > From: Xinlei Lee > > Add dt-binding documentation of dsi for MediaTek MT8186 SoC. > > Signed-off-by: Xinlei Lee > Reviewed-by: AngeloGioacchino Del Regno > > Reviewed-by: Rex-BC Chen > --- > .../devicetree/bindings/display/mediatek/mediatek,dsi.yaml | 1 + > 1 file changed, 1 insertion(+) > Acked-by: Rob Herring
Re: [PATCH v4,1/3] dt-bindings: display: mediatek: dsi: Convert dsi_dtbinding to .yaml
On Sat, Apr 09, 2022 at 05:11:52PM +0800, xinlei@mediatek.com wrote:
> From: Xinlei Lee
>
> Convert mediatek,dsi.txt to mediatek,dsi.yaml format
>
> Signed-off-by: Xinlei Lee
> ---
> .../display/mediatek/mediatek,dsi.txt | 62 -
> .../display/mediatek/mediatek,dsi.yaml| 118 ++
> 2 files changed, 118 insertions(+), 62 deletions(-)
> delete mode 100644
> Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.txt
> create mode 100644
> Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml
> diff --git
> a/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml
> b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml
> new file mode 100644
> index ..431bb981394f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dsi.yaml
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,dsi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek DSI Controller Device Tree Bindings
> +
> +maintainers:
> + - CK Hu
> + - Jitao Shi
> + - Xinlei Lee
> +
> +description: |
> + The MediaTek DSI function block is a sink of the display subsystem and can
> + drive up to 4-lane MIPI DSI output. Two DSIs can be synchronized for dual-
> + channel output.
allOf:
- $ref: /schemas/display/dsi-controller.yaml#
> +
> +properties:
> + compatible:
> +enum:
> + - mediatek,mt2701-dsi
> + - mediatek,mt7623-dsi
> + - mediatek,mt8167-dsi
> + - mediatek,mt8173-dsi
> + - mediatek,mt8183-dsi
> +
> + reg:
> +maxItems: 1
> +
> + interrupts:
> +maxItems: 1
> +
> + power-domains:
> +maxItems: 1
> +
> + clocks:
> +items:
> + - description: Engine Clock
> + - description: Digital Clock
> + - description: HS Clock
> +
> + clock-names:
> +items:
> + - const: engine
> + - const: digital
> + - const: hs
> +
> + resets:
> +maxItems: 1
> +
> + phys:
> +maxItems: 1
> +
> + phy-names:
> +items:
> + - const: dphy
> +
> + port:
> +$ref: /schemas/graph.yaml#/properties/port
> +description:
> + Output port node. This port should be connected to the input
> + port of an attached DSI panel or DSI-to-eDP encoder chip.
> +
> +
> + "#address-cells":
> +const: 2
> +
> + "#size-cells":
> +const: 2
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - power-domains
> + - clocks
> + - clock-names
> + - phys
> + - phy-names
> + - port
> +
> +additionalProperties: false
with the above,
unevaluatedProperties: false
> +
> +examples:
> + - |
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +soc {
> +#address-cells = <2>;
> +#size-cells = <2>;
> +
> +dsi0: dsi@14014000 {
> +compatible = "mediatek,mt8183-dsi";
> +reg = <0 0x14014000 0 0x1000>;
> +interrupts = ;
> +power-domains = < MT8183_POWER_DOMAIN_DISP>;
> +clocks = < CLK_MM_DSI0_MM>,
> +< CLK_MM_DSI0_IF>,
> +<_tx0>;
> +clock-names = "engine", "digital", "hs";
> +resets = < MT8183_MMSYS_SW0_RST_B_DISP_DSI0>;
> +phys = <_tx0>;
> +phy-names = "dphy";
> +port {
> +dsi0_out: endpoint {
> +remote-endpoint = <_in>;
> +};
> +};
> +};
> +};
> +
> +...
> --
> 2.18.0
>
>
Re: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
On 4/13/22 8:04 PM, Jason Gunthorpe wrote: > On Wed, Apr 13, 2022 at 07:17:52PM +, Wang, Zhi A wrote: >> On 4/13/22 5:37 PM, Jason Gunthorpe wrote: >>> On Wed, Apr 13, 2022 at 06:29:46PM +0200, Christoph Hellwig wrote: On Wed, Apr 13, 2022 at 01:18:14PM -0300, Jason Gunthorpe wrote: > Yeah, I was thinking about that too, but on the other hand I think it > is completely wrong that gvt requires kvm at all. A vfio_device is not > supposed to be tightly linked to KVM - the only exception possibly > being s390.. So i915/gvt uses it for: - poking into the KVM GFN translations - using the KVM page track notifier No idea how these could be solved in a more generic way. >>> >>> TBH I'm not sure how any of this works fully correctly.. >>> >>> I see this code getting something it calls a GFN and then passing >>> them to vfio - which makes no sense. Either a value is a GFN - the >>> physical memory address of the VM, or it is an IOVA. VFIO only takes >>> in IOVA and kvm only takes in GFN. So these are probably IOVAs really.. >>> >> Can you let me know the place? So that I can take a look. > > Well, for instance: > > static int gvt_pin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn, > unsigned long size, struct page **page) > > There is no way that is a GFN, it is an IOVA. > I see. The name is vague. There is an promised 1:1 mapping between guest GFN and host IOVA when a PCI device is passed to a VM, I guess mdev is just leveraging it as they are sharing the same code path in QEMU. It's in a function called vfio_listener_region_add() in the source code of QEMU. Are you planning to change the architecture? It would be nice to know your plan. >>> It seems the purpose is to shadow a page table, and it is capturing >>> user space CPU writes to this page table memory I guess? >>> >> Yes.The shadow page will be built according to the guest GPU page table. >> When a guest workload is executed in the GPU, the root pointer of the >> shadow page table in the shadow GPU context is used. If the host enables >> the IOMMU, the pages used by the shadow page table needs to be mapped as >> IOVA, and the PFNs in the shadow entries are IOVAs. > > So if the page table in the guest has IOVA addreses then why can you > use them as GFNs? > That's another problem. We don't support a guess enabling the guest IOMMU (aka virtual IOMMU). The guest/virtual IOMMU is implemented in QEMU, so does the translation between guest IOVA and GFN. For a mdev model implemented in the kernel, there isn't any mechanism so far to reach there. People were discussing it before. But none agreement was achieved. Is it possible to implement it in the kernel? Would like to discuss more about it if there is any good idea. > Or is it that only the page table levels themselves are GFNs and the > actual DMA's are IOVA? The unclear mixing of GFN as IOVA in the code > makes it inscrutible. > No. Even the HW is capable of controlling the level of translation, but it's not used like this in the existing driver. It's definitely an architecture open. > Jason >
[PATCH v2] drm/msm/dp: stop event kernel thread when DP unbind
Current DP driver implementation, event thread is kept running
after DP display is unbind. This patch fix this problem by disabling
DP irq and stop event thread to exit gracefully at dp_display_unbind().
Changes in v2:
-- start event thread at dp_display_bind()
Fixes: e91e3065a806 ("drm/msm/dp: Add DP compliance tests on Snapdragon
Chipsets")
Signed-off-by: Kuogee Hsieh
Reported-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_display.c | 40 +++--
1 file changed, 30 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 01453db..943e4f1 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -113,6 +113,7 @@ struct dp_display_private {
u32 hpd_state;
u32 event_pndx;
u32 event_gndx;
+ struct task_struct *ev_tsk;
struct dp_event event_list[DP_EVENT_Q_MAX];
spinlock_t event_lock;
@@ -230,6 +231,31 @@ void dp_display_signal_audio_complete(struct msm_dp
*dp_display)
complete_all(>audio_comp);
}
+static int hpd_event_thread(void *data);
+
+static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
+{
+ init_waitqueue_head(_priv->event_q);
+ spin_lock_init(_priv->event_lock);
+
+ dp_priv->ev_tsk = kthread_run(hpd_event_thread, dp_priv,
"dp_hpd_handler");
+
+ if (IS_ERR(dp_priv->ev_tsk))
+ DRM_ERROR("failed to create DP event thread\n");
+}
+
+static void dp_hpd_event_stop(struct dp_display_private *dp_priv)
+{
+ if (IS_ERR(dp_priv->ev_tsk))
+ return;
+
+ kthread_stop(dp_priv->ev_tsk);
+
+ /* reset event q to empty */
+ dp_priv->event_gndx = 0;
+ dp_priv->event_pndx = 0;
+}
+
static int dp_display_bind(struct device *dev, struct device *master,
void *data)
{
@@ -269,6 +295,7 @@ static int dp_display_bind(struct device *dev, struct
device *master,
if (rc)
DRM_ERROR("Audio registration Dp failed\n");
+ dp_hpd_event_setup(dp); /* start event thread */
end:
return rc;
}
@@ -280,6 +307,8 @@ static void dp_display_unbind(struct device *dev, struct
device *master,
struct drm_device *drm = dev_get_drvdata(master);
struct msm_drm_private *priv = drm->dev_private;
+ disable_irq(dp->irq);
+ dp_hpd_event_stop(dp); /* stop event thread */
dp_power_client_deinit(dp->power);
dp_aux_unregister(dp->aux);
priv->dp[dp->id] = NULL;
@@ -1054,7 +1083,7 @@ static int hpd_event_thread(void *data)
dp_priv = (struct dp_display_private *)data;
- while (1) {
+ while (!kthread_should_stop()) {
if (timeout_mode) {
wait_event_timeout(dp_priv->event_q,
(dp_priv->event_pndx == dp_priv->event_gndx),
@@ -1132,13 +1161,6 @@ static int hpd_event_thread(void *data)
return 0;
}
-static void dp_hpd_event_setup(struct dp_display_private *dp_priv)
-{
- init_waitqueue_head(_priv->event_q);
- spin_lock_init(_priv->event_lock);
-
- kthread_run(hpd_event_thread, dp_priv, "dp_hpd_handler");
-}
static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
{
@@ -1441,8 +1463,6 @@ void msm_dp_irq_postinstall(struct msm_dp *dp_display)
dp = container_of(dp_display, struct dp_display_private, dp_display);
- dp_hpd_event_setup(dp);
-
dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
On Wed, Apr 13, 2022 at 07:17:52PM +, Wang, Zhi A wrote: > On 4/13/22 5:37 PM, Jason Gunthorpe wrote: > > On Wed, Apr 13, 2022 at 06:29:46PM +0200, Christoph Hellwig wrote: > >> On Wed, Apr 13, 2022 at 01:18:14PM -0300, Jason Gunthorpe wrote: > >>> Yeah, I was thinking about that too, but on the other hand I think it > >>> is completely wrong that gvt requires kvm at all. A vfio_device is not > >>> supposed to be tightly linked to KVM - the only exception possibly > >>> being s390.. > >> > >> So i915/gvt uses it for: > >> > >> - poking into the KVM GFN translations > >> - using the KVM page track notifier > >> > >> No idea how these could be solved in a more generic way. > > > > TBH I'm not sure how any of this works fully correctly.. > > > > I see this code getting something it calls a GFN and then passing > > them to vfio - which makes no sense. Either a value is a GFN - the > > physical memory address of the VM, or it is an IOVA. VFIO only takes > > in IOVA and kvm only takes in GFN. So these are probably IOVAs really.. > > > Can you let me know the place? So that I can take a look. Well, for instance: static int gvt_pin_guest_page(struct intel_vgpu *vgpu, unsigned long gfn, unsigned long size, struct page **page) There is no way that is a GFN, it is an IOVA. > > It seems the purpose is to shadow a page table, and it is capturing > > user space CPU writes to this page table memory I guess? > > > Yes.The shadow page will be built according to the guest GPU page table. > When a guest workload is executed in the GPU, the root pointer of the > shadow page table in the shadow GPU context is used. If the host enables > the IOMMU, the pages used by the shadow page table needs to be mapped as > IOVA, and the PFNs in the shadow entries are IOVAs. So if the page table in the guest has IOVA addreses then why can you use them as GFNs? Or is it that only the page table levels themselves are GFNs and the actual DMA's are IOVA? The unclear mixing of GFN as IOVA in the code makes it inscrutible. Jason
Re: AMDGPU: regression on 5.17.1
On Wed, 13 Apr 2022 14:14:42 -0400 Alex Deucher wrote: > On Wed, Apr 13, 2022 at 1:33 PM Michele Ballabio > wrote: > > > > On Mon, 11 Apr 2022 14:34:37 -0400 > > Alex Deucher wrote: > > > > > On Sat, Apr 9, 2022 at 12:28 PM Michele Ballabio > > > wrote: > > > > > > > > On Tue, 5 Apr 2022 10:23:16 -0400 > > > > Alex Deucher wrote: > > > > > > > > > On Mon, Apr 4, 2022 at 3:39 PM Michele Ballabio > > > > > wrote: > > > > > > > > > > > > On Mon, 4 Apr 2022 13:03:41 -0400 > > > > > > Alex Deucher wrote: > > > > > > > > > > > > > On Sun, Apr 3, 2022 at 10:19 AM Michele Ballabio > > > > > > > wrote: > > > > > > > > > > > > > > > > Hi, > > > > > > > > I've hit a regression on 5.17.1 (haven't tested > > > > > > > > 5.17.0, but 5.16-stable didn't have this problem). > > > > > > > > > > > > > > > > The machine is a Ryzen 5 1600 with AMD graphics (RX > > > > > > > > 560). > > > > > > > > > > > > > > > > The regression I hit seems to trigger when the machine > > > > > > > > is left idle at boot (I don't boot straight to X, I > > > > > > > > boot to a tty, login and then start X). The machine > > > > > > > > after a while blanks the screen. Usually, the screen > > > > > > > > unblanks as the keyboard is hit or the mouse moves, but > > > > > > > > with kernel 5.17.1 the screen does not wake up. The > > > > > > > > machine seems to run mostly fine: I can login from ssh, > > > > > > > > but I cannot reboot or halt it: a sysrq sequence is > > > > > > > > needed for that. Note that if the screen goes blank > > > > > > > > under X, it wakes up fine. > > > > > > > > > > > > > > > > Below a dmesg and two traces from syslog (they're quite > > > > > > > > similar). > > > > > > > > > > > > > > Can you bisect? Does setting amdgpu.runpm=0 help? > > > > > > > > > > > > I can try to bisect, should I narrow the search to > > > > > > drivers/gpu/drm/ ? > > > > > > > > > > I would just do a full bisect if possible in case the change > > > > > happens to be outside of drm. > > > > > > > > > > > > > > > > > Setting amdgpu.runpm=0 works, the display now unblanks > > > > > > without problems. > > > > > > > > > > > > > Hi, > > > > I bisected this, and the first bad commit is > > > > [087451f372bf76d971184caa258807b7c35aac8f] drm/amdgpu: use > > > > generic fb helpers instead of setting up AMD own's. > > > > > > > > Let me know if you need some more testing. > > > > > > Thanks. Do the attached patches fix the issue? > > > > > > Thanks, > > > > > > Alex > > > > Sorry, no. I applied them both on top of 5.17.1. > > Thanks. Please try the attached patch. > > Thanks, > > Alex I applied the v2 patch on top of 5.17.1 and it works as expected. Tested-by: Michele Ballabio Thanks, Michele Ballabio
Re: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
On 4/13/22 5:37 PM, Jason Gunthorpe wrote: > On Wed, Apr 13, 2022 at 06:29:46PM +0200, Christoph Hellwig wrote: >> On Wed, Apr 13, 2022 at 01:18:14PM -0300, Jason Gunthorpe wrote: >>> Yeah, I was thinking about that too, but on the other hand I think it >>> is completely wrong that gvt requires kvm at all. A vfio_device is not >>> supposed to be tightly linked to KVM - the only exception possibly >>> being s390.. >> >> So i915/gvt uses it for: >> >> - poking into the KVM GFN translations >> - using the KVM page track notifier >> >> No idea how these could be solved in a more generic way. > > TBH I'm not sure how any of this works fully correctly.. > > I see this code getting something it calls a GFN and then passing > them to vfio - which makes no sense. Either a value is a GFN - the > physical memory address of the VM, or it is an IOVA. VFIO only takes > in IOVA and kvm only takes in GFN. So these are probably IOVAs really.. > Can you let me know the place? So that I can take a look. > But then, I see this code taking GFNs (which are probably IOVAs?) and > passing them to the kvm page track notifier? That can't be right, VFIO > needs to translate the IOVA to a GFN, not assume 1:1... > GFNs are from the guest page table. It takes the GFN from a entry belongs to a guest page table and request the kvm_page_track to track it, so that the shadow page table can be updated accordingly. > It seems the purpose is to shadow a page table, and it is capturing > user space CPU writes to this page table memory I guess? > Yes.The shadow page will be built according to the guest GPU page table. When a guest workload is executed in the GPU, the root pointer of the shadow page table in the shadow GPU context is used. If the host enables the IOMMU, the pages used by the shadow page table needs to be mapped as IOVA, and the PFNs in the shadow entries are IOVAs. > GFN's seems to come from gen8_gtt_get_pfn which seems to be parsing > some guest page table? > Yes. It's to extract the PFNs from a page table entry. > Jason >
Re: [PATCH 1/2] of: Create platform devices for OF framebuffers
eOn Wed, Apr 13, 2022 at 1:46 PM Rob Herring wrote:
>
> On Wed, Apr 13, 2022 at 12:58 PM Thomas Zimmermann
> wrote:
> >
> > Hi
> >
> > Am 13.04.22 um 14:51 schrieb Rob Herring:
> > > On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann
> > > wrote:
> > >>
> > >> Create a platform device for each OF-declared framebuffer and have
> > >> offb bind to these devices. Allows for real hot-unplugging and other
> > >> drivers besides offb.
> > >>
> > >> Originally, offb created framebuffer devices while initializing its
> > >> module by parsing the OF device tree. No actual Linux device was set
> > >> up. This tied OF framebuffers to offb and makes writing other drivers
> > >> for the OF framebuffers complicated. The absence of a Linux device
> > >> prevented real hot-unplugging. Adding a distinct platform device for
> > >> each OF framebuffer solves both problems. Specifically, a DRM drivers
> > >> can now provide graphics output with modern userspace.
> > >>
> > >> Some of the offb init code is now located in the OF initialization.
> > >> There's now also an implementation of
> > >> of_platform_default_populate_init(),
> > >> which was missing before. The OF side creates different devices for
> > >> either OF display nodes or bootx displays as they require different
> > >> handling by the driver. The offb drivers picks up each type of device
> > >> and runs the appropriate fbdev initialization.
> > >>
> > >> Tested with OF display nodes on qemu's ppc64le target.
> > >>
> > >> Signed-off-by: Thomas Zimmermann
> > >> ---
> > >> drivers/of/platform.c | 73 ++--
> > >> drivers/video/fbdev/offb.c | 98 +-
> > >> 2 files changed, 134 insertions(+), 37 deletions(-)
> > >>
> > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > >> index a16b74f32aa9..4c63b9a73587 100644
> > >> --- a/drivers/of/platform.c
> > >> +++ b/drivers/of/platform.c
> > >> @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
> > >> }
> > >> EXPORT_SYMBOL(of_platform_bus_probe);
> > >>
> > >> +static int __init of_platform_populate_framebuffers(void)
> > >> +{
> > >> + struct device_node *boot_display = NULL;
> > >> + struct device_node *node;
> > >> + struct platform_device *dev;
> > >> + int ret;
> > >> +
> > >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> > >> + of_platform_device_create(node, NULL, NULL);
> > >> + of_node_put(node);
> > >> +
> > >
> > > The rest is PPC only, so bail out here if !PPC.
> > >
> > >> + /* Check if we have a MacOS display without a node spec */
> > >> + if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
> > >> + /*
> > >> +* The old code tried to work out which node was the
> > >> MacOS
> > >> +* display based on the address. I'm dropping that since
> > >> the
> > >> +* lack of a node spec only happens with old BootX
> > >> versions
> > >> +* (users can update) and with this code, they'll still
> > >> get
> > >> +* a display (just not the palette hacks).
> > >> +*/
> > >> + dev = platform_device_alloc("bootx-noscreen", 0);
> > >> + if (WARN_ON(!dev))
> > >> + return -ENOMEM;
> > >> + ret = platform_device_add(dev);
> > >> + if (WARN_ON(ret)) {
> > >> + platform_device_put(dev);
> > >> + return ret;
> > >> + }
> > >> + }
> > >> +
> > >> + /*
> > >> +* For OF framebuffers, first create the device for the boot
> > >> display,
> > >> +* then for the other framebuffers. Only fail for the boot
> > >> display;
> > >> +* ignore errors for the rest.
> > >> +*/
> > >> + for_each_node_by_type(node, "display") {
> > >> + if (!of_get_property(node, "linux,opened", NULL) ||
> > >> + !of_get_property(node, "linux,boot-display", NULL))
> > >> + continue;
> > >> + dev = of_platform_device_create(node, "of-display",
> > >> NULL);
> > >> + if (WARN_ON(!dev))
> > >> + return -ENOMEM;
> > >> + boot_display = node;
> > >> + break;
> > >> + }
> > >> + for_each_node_by_type(node, "display") {
> > >> + if (!of_get_property(node, "linux,opened", NULL) || node
> > >> == boot_display)
> > >> + continue;
> > >> + of_platform_device_create(node, "of-display", NULL);
> > >> + }
> > >> +
> > >> + return 0;
> > >> +}
> > >> +
> > >> /**
> > >>* of_platform_populate() - Populate platform_devices from device tree
> > >> data
> > >>* @root: parent of the first level to probe or NULL for the root of
> > >> the tree
> > >> @@ -541,9 +595,7 @@
Re: [PATCH 1/2] of: Create platform devices for OF framebuffers
On Wed, Apr 13, 2022 at 12:58 PM Thomas Zimmermann wrote:
>
> Hi
>
> Am 13.04.22 um 14:51 schrieb Rob Herring:
> > On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann
> > wrote:
> >>
> >> Create a platform device for each OF-declared framebuffer and have
> >> offb bind to these devices. Allows for real hot-unplugging and other
> >> drivers besides offb.
> >>
> >> Originally, offb created framebuffer devices while initializing its
> >> module by parsing the OF device tree. No actual Linux device was set
> >> up. This tied OF framebuffers to offb and makes writing other drivers
> >> for the OF framebuffers complicated. The absence of a Linux device
> >> prevented real hot-unplugging. Adding a distinct platform device for
> >> each OF framebuffer solves both problems. Specifically, a DRM drivers
> >> can now provide graphics output with modern userspace.
> >>
> >> Some of the offb init code is now located in the OF initialization.
> >> There's now also an implementation of of_platform_default_populate_init(),
> >> which was missing before. The OF side creates different devices for
> >> either OF display nodes or bootx displays as they require different
> >> handling by the driver. The offb drivers picks up each type of device
> >> and runs the appropriate fbdev initialization.
> >>
> >> Tested with OF display nodes on qemu's ppc64le target.
> >>
> >> Signed-off-by: Thomas Zimmermann
> >> ---
> >> drivers/of/platform.c | 73 ++--
> >> drivers/video/fbdev/offb.c | 98 +-
> >> 2 files changed, 134 insertions(+), 37 deletions(-)
> >>
> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >> index a16b74f32aa9..4c63b9a73587 100644
> >> --- a/drivers/of/platform.c
> >> +++ b/drivers/of/platform.c
> >> @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
> >> }
> >> EXPORT_SYMBOL(of_platform_bus_probe);
> >>
> >> +static int __init of_platform_populate_framebuffers(void)
> >> +{
> >> + struct device_node *boot_display = NULL;
> >> + struct device_node *node;
> >> + struct platform_device *dev;
> >> + int ret;
> >> +
> >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> >> + of_platform_device_create(node, NULL, NULL);
> >> + of_node_put(node);
> >> +
> >
> > The rest is PPC only, so bail out here if !PPC.
> >
> >> + /* Check if we have a MacOS display without a node spec */
> >> + if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
> >> + /*
> >> +* The old code tried to work out which node was the MacOS
> >> +* display based on the address. I'm dropping that since
> >> the
> >> +* lack of a node spec only happens with old BootX versions
> >> +* (users can update) and with this code, they'll still get
> >> +* a display (just not the palette hacks).
> >> +*/
> >> + dev = platform_device_alloc("bootx-noscreen", 0);
> >> + if (WARN_ON(!dev))
> >> + return -ENOMEM;
> >> + ret = platform_device_add(dev);
> >> + if (WARN_ON(ret)) {
> >> + platform_device_put(dev);
> >> + return ret;
> >> + }
> >> + }
> >> +
> >> + /*
> >> +* For OF framebuffers, first create the device for the boot
> >> display,
> >> +* then for the other framebuffers. Only fail for the boot display;
> >> +* ignore errors for the rest.
> >> +*/
> >> + for_each_node_by_type(node, "display") {
> >> + if (!of_get_property(node, "linux,opened", NULL) ||
> >> + !of_get_property(node, "linux,boot-display", NULL))
> >> + continue;
> >> + dev = of_platform_device_create(node, "of-display", NULL);
> >> + if (WARN_ON(!dev))
> >> + return -ENOMEM;
> >> + boot_display = node;
> >> + break;
> >> + }
> >> + for_each_node_by_type(node, "display") {
> >> + if (!of_get_property(node, "linux,opened", NULL) || node
> >> == boot_display)
> >> + continue;
> >> + of_platform_device_create(node, "of-display", NULL);
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> /**
> >>* of_platform_populate() - Populate platform_devices from device tree
> >> data
> >>* @root: parent of the first level to probe or NULL for the root of the
> >> tree
> >> @@ -541,9 +595,7 @@ static int __init
> >> of_platform_default_populate_init(void)
> >> of_node_put(node);
> >> }
> >>
> >> - node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> >> - of_platform_device_create(node, NULL, NULL);
> >> - of_node_put(node);
> >> +
Re: [PATCH v8, 00/15] media: mtk-vcodec: support for M8192 decoder
Le mercredi 13 avril 2022 à 09:57 +0200, AngeloGioacchino Del Regno a écrit :
> Il 13/04/22 09:03, allen-kh.cheng ha scritto:
> > Hi Nicolas,
> >
> > On Tue, 2022-04-12 at 10:48 -0400, Nicolas Dufresne wrote:
> > > Le lundi 11 avril 2022 à 11:41 +0800, yunfei.d...@mediatek.com a
> > > écrit :
> > > > Hi Nicolas,
> > > >
> > > > On Thu, 2022-03-31 at 16:48 -0400, Nicolas Dufresne wrote:
> > > > > Hi Yunfei,
> > > > >
> > > > > thanks for the update, I should be testing this really soon.
> > > > >
> > > > > Le jeudi 31 mars 2022 à 10:47 +0800, Yunfei Dong a écrit :
> > > > > > This series adds support for mt8192 h264/vp8/vp9 decoder
> > > > > > drivers.
> > > > > > Firstly, refactor
> > > > > > power/clock/interrupt interfaces for mt8192 is lat and core
> > > > > > architecture.
> > > > >
> > > > > Similarly to MT8173 and MT8183, a shared* firmware is needed for
> > > > > this
> > > > > CODEC to
> > > > > work (scp.img). I looked into linux-firmware[1] it has not been
> > > > > added
> > > > > for mt8192
> > > > > yet. As your patches are getting close to be ready, it would be
> > > > > important to
> > > > > look into this so the patchset does not get blocked due to that.
> > > > >
> > > > > best regards,
> > > > > Nicolas
> > > > >
> > > > > [1]
> > > > >
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/mediatek__;!!CTRNKA9wMg0ARbw!zy4N6JDroSXtumXXa7MuxAgYAPAink8uyW-978vpWct8S3vOjBqXirFE8uTEHopHCovbSl0FNP9LPgWCEBrZfMIcvQ$
> > > > >
> > > > > * Shared at least between MDP3 and MTK VCODEC from my knowledge
> > > > >
> > > >
> > > > Thanks for your remind.
> > > >
> > > > I have already sent mt8192 scp.img to github.
> > > >
> > > >
> > https://urldefense.com/v3/__https://github.com/yunfeidongmediatek/linux_fw_scp_8192/commit/3ac2fc85bc7dfcebdb92b5b5808b0268cdfb772d__;!!CTRNKA9wMg0ARbw!zy4N6JDroSXtumXXa7MuxAgYAPAink8uyW-978vpWct8S3vOjBqXirFE8uTEHopHCovbSl0FNP9LPgWCEBpf9F_nWA$
> > > >
> > > >
> > > > Waiting for to be merged.
> > >
> > > On boards I have, the firmware is loaded from /lib/firmware/scp.img,
> > > but with
> > > this submission it will be in lib/firmware/mediatek/mt8192/scp.img .
> > > I haven't
> > > found anything around:
> > >
> > > drivers/remoteproc/mtk_scp.c:812: char *fw_name = "scp.img";
> > >
> > > That would use the platform path. This seems like a problem to me,
> > > the
> > > upstreaming of the firmware isn't being aligned with the were the
> > > firmware is
> > > picked by the upstream driver. Correct me if I got this wrong, but
> > > I'd really
> > > like to clarify this.
> > >
> > > Nicolas
> > >
> >
> > I am not sure why it's accepted the fw path of scp is
> > /lib/firmware/scp.img in mt8173/8183 but we upload scp.ing in
> > /lib/firmware/mediatek/mt8173(mt8183)/scp.img to
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/mediatek
> >
> > Currently, the scp driver will load firmware in /lib/firmware/scp.img.
> > that means there is only one firmware for a specific platform.
> > I think we can send a PATCH to make firmware name of scp being more
> > flexible.
> >
> > Maybe get firmware name from dts. e.g.,
> > {
> > status = "okay";
> > firmware-name = "mediatek/mt81xx/scp.img";
> > };
> >
> > Do you think it feasible?
> > If you have any concerns, please let us know.
> >
> > Thanks,
> > Allen
> >
>
> Hello Allen,
>
> what you proposed is exactly what has been done for other platforms because of
> both per-device firmware differences (different signatures) and per-SoC
> (different
> firmware entirely), found on TI K3, iMX DSP, Qualcomm MSS/DSP remoteproc and
> others.
>
> Of course this is an accepted way to resolve this situation: please go on!
Looks good to me! (don't forget to keep a fallback to /lib/firmware/scp.img to
maintain backward compatibility).
>
> Cheers,
> Angelo
>
Re: AMDGPU: regression on 5.17.1
On Wed, Apr 13, 2022 at 1:33 PM Michele Ballabio wrote: > > On Mon, 11 Apr 2022 14:34:37 -0400 > Alex Deucher wrote: > > > On Sat, Apr 9, 2022 at 12:28 PM Michele Ballabio > > wrote: > > > > > > On Tue, 5 Apr 2022 10:23:16 -0400 > > > Alex Deucher wrote: > > > > > > > On Mon, Apr 4, 2022 at 3:39 PM Michele Ballabio > > > > wrote: > > > > > > > > > > On Mon, 4 Apr 2022 13:03:41 -0400 > > > > > Alex Deucher wrote: > > > > > > > > > > > On Sun, Apr 3, 2022 at 10:19 AM Michele Ballabio > > > > > > wrote: > > > > > > > > > > > > > > Hi, > > > > > > > I've hit a regression on 5.17.1 (haven't tested > > > > > > > 5.17.0, but 5.16-stable didn't have this problem). > > > > > > > > > > > > > > The machine is a Ryzen 5 1600 with AMD graphics (RX 560). > > > > > > > > > > > > > > The regression I hit seems to trigger when the machine is > > > > > > > left idle at boot (I don't boot straight to X, I boot to a > > > > > > > tty, login and then start X). The machine after a while > > > > > > > blanks the screen. Usually, the screen unblanks as the > > > > > > > keyboard is hit or the mouse moves, but with kernel 5.17.1 > > > > > > > the screen does not wake up. The machine seems to run > > > > > > > mostly fine: I can login from ssh, but I cannot reboot or > > > > > > > halt it: a sysrq sequence is needed for that. Note that if > > > > > > > the screen goes blank under X, it wakes up fine. > > > > > > > > > > > > > > Below a dmesg and two traces from syslog (they're quite > > > > > > > similar). > > > > > > > > > > > > Can you bisect? Does setting amdgpu.runpm=0 help? > > > > > > > > > > I can try to bisect, should I narrow the search to > > > > > drivers/gpu/drm/ ? > > > > > > > > I would just do a full bisect if possible in case the change > > > > happens to be outside of drm. > > > > > > > > > > > > > > Setting amdgpu.runpm=0 works, the display now unblanks without > > > > > problems. > > > > > > > > > > Hi, > > > I bisected this, and the first bad commit is > > > [087451f372bf76d971184caa258807b7c35aac8f] drm/amdgpu: use generic > > > fb helpers instead of setting up AMD own's. > > > > > > Let me know if you need some more testing. > > > > Thanks. Do the attached patches fix the issue? > > > > Thanks, > > > > Alex > > Sorry, no. I applied them both on top of 5.17.1. Thanks. Please try the attached patch. Thanks, Alex > I'm pasting the output of dmesg and decode_stacktrace.sh: > > dmesg: > --- > > [0.00] Linux version 5.17.1+ (m...@darkstar.example.org) (gcc (GCC) > 11.2.0, GNU ld version 2.38-slack151) #221 SMP PREEMPT Wed Apr 13 18:51:36 > CEST 2022 > [0.00] Command line: BOOT_IMAGE=bzImage ro root=806 debug > [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point > registers' > [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' > [0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers' > [0.00] x86/fpu: xstate_offset[2]: 576, xstate_sizes[2]: 256 > [0.00] x86/fpu: Enabled xstate features 0x7, context size is 832 > bytes, using 'compacted' format. > [0.00] signal: max sigframe size: 1776 > [0.00] BIOS-provided physical RAM map: > [0.00] BIOS-e820: [mem 0x-0x0009d3ff] usable > [0.00] BIOS-e820: [mem 0x0009d400-0x0009] reserved > [0.00] BIOS-e820: [mem 0x000e-0x000f] reserved > [0.00] BIOS-e820: [mem 0x0010-0x03ff] usable > [0.00] BIOS-e820: [mem 0x0400-0x04009fff] ACPI NVS > [0.00] BIOS-e820: [mem 0x0400a000-0x09bf] usable > [0.00] BIOS-e820: [mem 0x09c0-0x09ff] reserved > [0.00] BIOS-e820: [mem 0x0a00-0x0aff] usable > [0.00] BIOS-e820: [mem 0x0b00-0x0b01] reserved > [0.00] BIOS-e820: [mem 0x0b02-0xd4434fff] usable > [0.00] BIOS-e820: [mem 0xd4435000-0xd444] ACPI > data > [0.00] BIOS-e820: [mem 0xd445-0xda830fff] usable > [0.00] BIOS-e820: [mem 0xda831000-0xda970fff] reserved > [0.00] BIOS-e820: [mem 0xda971000-0xda97afff] ACPI > data > [0.00] BIOS-e820: [mem 0xda97b000-0xdaa82fff] usable > [0.00] BIOS-e820: [mem 0xdaa83000-0xdae3efff] ACPI NVS > [0.00] BIOS-e820: [mem 0xdae3f000-0xdb93efff] reserved > [0.00] BIOS-e820: [mem 0xdb93f000-0xddff] usable > [0.00] BIOS-e820: [mem 0xde00-0xdfff] reserved > [0.00] BIOS-e820: [mem 0xf800-0xfbff] reserved > [0.00] BIOS-e820: [mem 0xfd80-0xfdff] reserved > [0.00] BIOS-e820: [mem 0xfea0-0xfea0] reserved > [
Re: [PATCH 2/2] fbdev: Remove hot-unplug workaround for framebuffers without device
Hi
Am 13.04.22 um 18:05 schrieb Daniel Vetter:
On Wed, Apr 13, 2022 at 12:50:50PM +0200, Javier Martinez Canillas wrote:
On 4/13/22 11:24, Thomas Zimmermann wrote:
A workaround makes fbdev hot-unplugging work for framebuffers without
device. The only user for this feature was offb. As each OF framebuffer
now has an associated platform device, the workaround is no longer
needed. Remove it. Effectively reverts commit 0f525289ff0d ("fbdev: Fix
unregistering of framebuffers without device").
Signed-off-by: Thomas Zimmermann
---
drivers/video/fbdev/core/fbmem.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index bc6ed750e915..bdd00d381bbc 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct
apertures_struct *a,
* If it's not a platform device, at least print a
warning. A
* fix would add code to remove the device from the
system.
*/
- if (!device) {
- /* TODO: Represent each OF framebuffer as its
own
-* device in the device hierarchy. For now, offb
-* doesn't have such a device, so unregister the
-* framebuffer as before without warning.
-*/
- do_unregister_framebuffer(registered_fb[i]);
Maybe we could still keep this for a couple of releases but with a big
warning that's not supported in case there are out-of-tree drivers out
there that still do this ?
Or at least a warning if the do_unregister_framebuffer() call is removed.
Yeah dying while holding console_lock isn't fun, and not having a WARN_ON
+ bail-out code pretty much forces bug reporters to do a bisect here to
give us something more than "machine dies at boot with no messages".
I'd just outright keep the WARN_ON here for 1-2 years even to really make
sure we got all the bug reports, since often these older machines only
update onto LTS releases.
If that's what the consent is, I'll go with it.
I'm just not sure if we talk about the same problem. offb didn't have a
platform device, so we recently added this workaround with 'if
(!device)'. All the other fbdev drivers have a platform device; and
anything else that could fail is out-of-tree. We don't really care about
those AFAIK.
With offb converted, we could practically remove all of the checks here
and call platform_device_unregister() unconditionally.
Best regards
Thomas
And it needs to be a WARN_ON + bail out since BUG_ON is as bad as just
oopsing.
-Daniel
Regardless of what you chose to do, the patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 1/2] of: Create platform devices for OF framebuffers
On 4/13/22 19:58, Thomas Zimmermann wrote: > Hi [snip] >>> >>> /* Populate everything else. */ >>> of_platform_default_populate(NULL, NULL, NULL); >> >> I'm pretty sure it's just this call that's the problem for PPC though >> none of the above existed when adding this caused a regression. Can we >> remove the ifdef and just make this call conditional on >> !IS_ENABLED(CONFIG_PPC). > > Together with the changes in of_platform_populate_framebuffers(), the > code is more or less an "if-else" depending on PPC. I'll drop > of_platform_populate_framebuffers() from the patch and make a separate > implementation of of_platform_default_populate_init for PPC. Seems like > the easiest solution to me. > That sounds reasonable to me as well. Feel free to retain my R-B tag when posting v2. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 1/2] of: Create platform devices for OF framebuffers
Hi
Am 13.04.22 um 14:51 schrieb Rob Herring:
On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann wrote:
Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.
Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.
Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.
Tested with OF display nodes on qemu's ppc64le target.
Signed-off-by: Thomas Zimmermann
---
drivers/of/platform.c | 73 ++--
drivers/video/fbdev/offb.c | 98 +-
2 files changed, 134 insertions(+), 37 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a16b74f32aa9..4c63b9a73587 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
}
EXPORT_SYMBOL(of_platform_bus_probe);
+static int __init of_platform_populate_framebuffers(void)
+{
+ struct device_node *boot_display = NULL;
+ struct device_node *node;
+ struct platform_device *dev;
+ int ret;
+
+ node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+ of_platform_device_create(node, NULL, NULL);
+ of_node_put(node);
+
The rest is PPC only, so bail out here if !PPC.
+ /* Check if we have a MacOS display without a node spec */
+ if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
+ /*
+* The old code tried to work out which node was the MacOS
+* display based on the address. I'm dropping that since the
+* lack of a node spec only happens with old BootX versions
+* (users can update) and with this code, they'll still get
+* a display (just not the palette hacks).
+*/
+ dev = platform_device_alloc("bootx-noscreen", 0);
+ if (WARN_ON(!dev))
+ return -ENOMEM;
+ ret = platform_device_add(dev);
+ if (WARN_ON(ret)) {
+ platform_device_put(dev);
+ return ret;
+ }
+ }
+
+ /*
+* For OF framebuffers, first create the device for the boot display,
+* then for the other framebuffers. Only fail for the boot display;
+* ignore errors for the rest.
+*/
+ for_each_node_by_type(node, "display") {
+ if (!of_get_property(node, "linux,opened", NULL) ||
+ !of_get_property(node, "linux,boot-display", NULL))
+ continue;
+ dev = of_platform_device_create(node, "of-display", NULL);
+ if (WARN_ON(!dev))
+ return -ENOMEM;
+ boot_display = node;
+ break;
+ }
+ for_each_node_by_type(node, "display") {
+ if (!of_get_property(node, "linux,opened", NULL) || node ==
boot_display)
+ continue;
+ of_platform_device_create(node, "of-display", NULL);
+ }
+
+ return 0;
+}
+
/**
* of_platform_populate() - Populate platform_devices from device tree data
* @root: parent of the first level to probe or NULL for the root of the tree
@@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void)
of_node_put(node);
}
- node = of_get_compatible_child(of_chosen, "simple-framebuffer");
- of_platform_device_create(node, NULL, NULL);
- of_node_put(node);
+ of_platform_populate_framebuffers();
/* Populate everything else. */
of_platform_default_populate(NULL, NULL, NULL);
I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).
Together with the changes in of_platform_populate_framebuffers(), the
code is more or less an "if-else" depending on PPC. I'll drop
of_platform_populate_framebuffers() from the patch and make a separate
implementation of of_platform_default_populate_init for PPC.
Re: [PATCH] dt-bindings: display: panel-timing: Define a single type for properties
Hi Rob, On Wed, Apr 13, 2022 at 09:00:15AM -0500, Rob Herring wrote: > It's not good practice to define multiple types for the same property, so > factor out the type reference making the properties always an uint32-array > with a length of 1 or 3 items. > > Signed-off-by: Rob Herring Looks good, Reviewed-by: Sam Ravnborg Let me know if we shall take it in drm-misc. Sam
Re: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
On Wed, Apr 13, 2022 at 06:29:46PM +0200, Christoph Hellwig wrote: > On Wed, Apr 13, 2022 at 01:18:14PM -0300, Jason Gunthorpe wrote: > > Yeah, I was thinking about that too, but on the other hand I think it > > is completely wrong that gvt requires kvm at all. A vfio_device is not > > supposed to be tightly linked to KVM - the only exception possibly > > being s390.. > > So i915/gvt uses it for: > > - poking into the KVM GFN translations > - using the KVM page track notifier > > No idea how these could be solved in a more generic way. TBH I'm not sure how any of this works fully correctly.. I see this code getting something it calls a GFN and then passing them to vfio - which makes no sense. Either a value is a GFN - the physical memory address of the VM, or it is an IOVA. VFIO only takes in IOVA and kvm only takes in GFN. So these are probably IOVAs really.. But then, I see this code taking GFNs (which are probably IOVAs?) and passing them to the kvm page track notifier? That can't be right, VFIO needs to translate the IOVA to a GFN, not assume 1:1... It seems the purpose is to shadow a page table, and it is capturing user space CPU writes to this page table memory I guess? GFN's seems to come from gen8_gtt_get_pfn which seems to be parsing some guest page table? Jason
Re: AMDGPU: regression on 5.17.1
On Mon, 11 Apr 2022 14:34:37 -0400 Alex Deucher wrote: > On Sat, Apr 9, 2022 at 12:28 PM Michele Ballabio > wrote: > > > > On Tue, 5 Apr 2022 10:23:16 -0400 > > Alex Deucher wrote: > > > > > On Mon, Apr 4, 2022 at 3:39 PM Michele Ballabio > > > wrote: > > > > > > > > On Mon, 4 Apr 2022 13:03:41 -0400 > > > > Alex Deucher wrote: > > > > > > > > > On Sun, Apr 3, 2022 at 10:19 AM Michele Ballabio > > > > > wrote: > > > > > > > > > > > > Hi, > > > > > > I've hit a regression on 5.17.1 (haven't tested > > > > > > 5.17.0, but 5.16-stable didn't have this problem). > > > > > > > > > > > > The machine is a Ryzen 5 1600 with AMD graphics (RX 560). > > > > > > > > > > > > The regression I hit seems to trigger when the machine is > > > > > > left idle at boot (I don't boot straight to X, I boot to a > > > > > > tty, login and then start X). The machine after a while > > > > > > blanks the screen. Usually, the screen unblanks as the > > > > > > keyboard is hit or the mouse moves, but with kernel 5.17.1 > > > > > > the screen does not wake up. The machine seems to run > > > > > > mostly fine: I can login from ssh, but I cannot reboot or > > > > > > halt it: a sysrq sequence is needed for that. Note that if > > > > > > the screen goes blank under X, it wakes up fine. > > > > > > > > > > > > Below a dmesg and two traces from syslog (they're quite > > > > > > similar). > > > > > > > > > > Can you bisect? Does setting amdgpu.runpm=0 help? > > > > > > > > I can try to bisect, should I narrow the search to > > > > drivers/gpu/drm/ ? > > > > > > I would just do a full bisect if possible in case the change > > > happens to be outside of drm. > > > > > > > > > > > Setting amdgpu.runpm=0 works, the display now unblanks without > > > > problems. > > > > > > > Hi, > > I bisected this, and the first bad commit is > > [087451f372bf76d971184caa258807b7c35aac8f] drm/amdgpu: use generic > > fb helpers instead of setting up AMD own's. > > > > Let me know if you need some more testing. > > Thanks. Do the attached patches fix the issue? > > Thanks, > > Alex Sorry, no. I applied them both on top of 5.17.1. I'm pasting the output of dmesg and decode_stacktrace.sh: dmesg: --- [0.00] Linux version 5.17.1+ (m...@darkstar.example.org) (gcc (GCC) 11.2.0, GNU ld version 2.38-slack151) #221 SMP PREEMPT Wed Apr 13 18:51:36 CEST 2022 [0.00] Command line: BOOT_IMAGE=bzImage ro root=806 debug [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers' [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' [0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers' [0.00] x86/fpu: xstate_offset[2]: 576, xstate_sizes[2]: 256 [0.00] x86/fpu: Enabled xstate features 0x7, context size is 832 bytes, using 'compacted' format. [0.00] signal: max sigframe size: 1776 [0.00] BIOS-provided physical RAM map: [0.00] BIOS-e820: [mem 0x-0x0009d3ff] usable [0.00] BIOS-e820: [mem 0x0009d400-0x0009] reserved [0.00] BIOS-e820: [mem 0x000e-0x000f] reserved [0.00] BIOS-e820: [mem 0x0010-0x03ff] usable [0.00] BIOS-e820: [mem 0x0400-0x04009fff] ACPI NVS [0.00] BIOS-e820: [mem 0x0400a000-0x09bf] usable [0.00] BIOS-e820: [mem 0x09c0-0x09ff] reserved [0.00] BIOS-e820: [mem 0x0a00-0x0aff] usable [0.00] BIOS-e820: [mem 0x0b00-0x0b01] reserved [0.00] BIOS-e820: [mem 0x0b02-0xd4434fff] usable [0.00] BIOS-e820: [mem 0xd4435000-0xd444] ACPI data [0.00] BIOS-e820: [mem 0xd445-0xda830fff] usable [0.00] BIOS-e820: [mem 0xda831000-0xda970fff] reserved [0.00] BIOS-e820: [mem 0xda971000-0xda97afff] ACPI data [0.00] BIOS-e820: [mem 0xda97b000-0xdaa82fff] usable [0.00] BIOS-e820: [mem 0xdaa83000-0xdae3efff] ACPI NVS [0.00] BIOS-e820: [mem 0xdae3f000-0xdb93efff] reserved [0.00] BIOS-e820: [mem 0xdb93f000-0xddff] usable [0.00] BIOS-e820: [mem 0xde00-0xdfff] reserved [0.00] BIOS-e820: [mem 0xf800-0xfbff] reserved [0.00] BIOS-e820: [mem 0xfd80-0xfdff] reserved [0.00] BIOS-e820: [mem 0xfea0-0xfea0] reserved [0.00] BIOS-e820: [mem 0xfeb8-0xfec01fff] reserved [0.00] BIOS-e820: [mem 0xfec1-0xfec10fff] reserved [0.00] BIOS-e820: [mem 0xfec3-0xfec30fff] reserved [0.00] BIOS-e820: [mem 0xfed0-0xfed00fff]
[pull] drm/msm: drm-msm-fixes-2022-04-13 for v5.18
Hi Dave & Daniel, A few fixes for v5.18. The following changes since commit 05afd57f4d34602a652fdaf58e0a2756b3c20fd4: drm/msm/gpu: Fix crash on devices without devfreq support (v2) (2022-03-08 13:55:23 -0800) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git drm-msm-fixes-2022-04-13 for you to fetch changes up to 390d645877ffd6dcb55f162d618045b2779217b3: drm/msm/gpu: Avoid -Wunused-function with !CONFIG_PM_SLEEP (2022-04-11 18:35:31 -0700) Dmitry Baryshkov (1): dt-bindings: display/msm: another fix for the dpu-qcm2290 example Kuogee Hsieh (1): drm/msm/dp: add fail safe mode outside of event_mutex context Marijn Suijten (1): drm/msm/dpu: Use indexed array initializer to prevent mismatches Nathan Chancellor (1): drm/msm/gpu: Avoid -Wunused-function with !CONFIG_PM_SLEEP Rob Clark (5): drm/msm/gpu: Rename runtime suspend/resume functions drm/msm/gpu: Park scheduler threads for system suspend drm/msm/gpu: Remove mutex from wait_event condition drm/msm: Add missing put_task_struct() in debugfs path drm/msm: Fix range size vs end confusion Robin Murphy (1): drm/msm: Stop using iommu_present() Stephen Boyd (1): drm/msm/dsi: Use connector directly in msm_dsi_manager_connector_init() Xiaoke Wang (2): drm/msm/disp: check the return value of kzalloc() drm/msm/mdp5: check the return of kzalloc() .../bindings/display/msm/dpu-qcm2290.yaml | 4 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/adreno_device.c | 80 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_interrupts.c | 34 - drivers/gpu/drm/msm/disp/mdp5/mdp5_plane.c | 3 + drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 + drivers/gpu/drm/msm/dp/dp_display.c| 6 ++ drivers/gpu/drm/msm/dp/dp_panel.c | 20 +++--- drivers/gpu/drm/msm/dp/dp_panel.h | 1 + drivers/gpu/drm/msm/dsi/dsi_manager.c | 2 +- drivers/gpu/drm/msm/msm_drv.c | 2 +- drivers/gpu/drm/msm/msm_gem.c | 1 + 12 files changed, 109 insertions(+), 48 deletions(-)
Re: commit 15512021eb3975a8c2366e3883337e252bb0eee5 causes with spots in console screeens.
On Mon, 11 Apr 2022, François Valenduc wrote: > Commit 15512021eb3975a8c2366e3883337e252bb0eee5 > (15512021eb3975a8c2366e3883337e252bb0eee5) causes a lof of white spots > to appears on the right upper corner of all console screens see the > attached photo). git-bisect shows that this is the offending commit and > if I revert it, the problem goes away. The problem still occurs with > kernel 5.18-rc2 and to all stable trees where it was applied. > Can somebody explains what happens ? > > The video card is the following: VGA compatible controller: Intel > Corporation WhiskeyLake-U GT2 [UHD Graphics 620] (rev 02) (prog-if 00 > [VGA controller]) > > Please tell me if you need more info. Replied to your other message: https://lore.kernel.org/r/87v8vcgb63@intel.com BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm/i915: Remove intel_gvt_init_host declaration
On Wed, 13 Apr 2022, Cong Liu wrote:
> this function has been deleted since commit 9bdb073464d6 ("drm/i915/gvt:
> Change KVMGT as self load module"), remove the deprecated function
> declaration.
I don't want to push this right now avoid conflicts in other pending
work. Cc'd Zhi & Zhenyu to pick this up.
BR,
Jani.
>
> Signed-off-by: Cong Liu
> ---
> drivers/gpu/drm/i915/intel_gvt.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_gvt.h
> b/drivers/gpu/drm/i915/intel_gvt.h
> index d7d3fb6186fd..daaf0957ebbc 100644
> --- a/drivers/gpu/drm/i915/intel_gvt.h
> +++ b/drivers/gpu/drm/i915/intel_gvt.h
> @@ -31,7 +31,6 @@ int intel_gvt_init(struct drm_i915_private *dev_priv);
> void intel_gvt_driver_remove(struct drm_i915_private *dev_priv);
> int intel_gvt_init_device(struct drm_i915_private *dev_priv);
> void intel_gvt_clean_device(struct drm_i915_private *dev_priv);
> -int intel_gvt_init_host(void);
> void intel_gvt_sanitize_options(struct drm_i915_private *dev_priv);
> void intel_gvt_resume(struct drm_i915_private *dev_priv);
> #else
--
Jani Nikula, Intel Open Source Graphics Center
Re: commit 15512021eb3975a8c2366e3883337e252bb0eee5 causes white spots in console screens
On Wed, 13 Apr 2022, François Valenduc wrote:
> Commit 15512021eb3975a8c2366e3883337e252bb0eee5
> (15512021eb3975a8c2366e3883337e252bb0eee5) causes a lof of white spots
> to appears on the right upper corner of all console screens (see
> https://drive.google.com/file/d/13GabEvOIKSAj5yox6ybAZEDu3Ixncw5Q/view).
> git-bisect shows that this is the offending commit and if I revert it,
> the problem goes away. The problem still occurs with kernel 5.18-rc2 and
> to all stable trees where it was applied.
> Can somebody explains what happens ?
>
> The video card is the following: VGA compatible controller: Intel
> Corporation WhiskeyLake-U GT2 [UHD Graphics 620] (rev 02) (prog-if 00
> [VGA controller])
>
> Please tell me if you need more info.
That's commit 15512021eb39 ("drm/i915: Workaround broken BIOS DBUF
configuration on TGL/RKL"), adding Cc's.
Please file a report at fdo gitlab [1] and attach dmesg etc. there.
Thanks,
Jani.
[1] https://gitlab.freedesktop.org/drm/intel/wikis/How-to-file-i915-bugs
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH v4 4/5] drm/solomon: Move device info from ssd130x-i2c to the core driver
On 4/13/22 19:02, Andy Shevchenko wrote:
> On Wed, Apr 13, 2022 at 06:23:57PM +0200, Javier Martinez Canillas wrote:
>> These are declared in the ssd130x-i2c transport driver but the information
>> is not I2C specific, and could be used by other SSD130x transport drivers.
>>
>> Move them to the ssd130x core driver and just set the OF device entries to
>> an ID that could be used to lookup the correct device info from an array.
>>
>> While being there, also move the SSD130X_DATA and SSD130X_COMMAND control
>> bytes. Since even though they are used by the I2C interface, they could
>> also be useful for other transport protocols such as SPI.
>
> Thanks!
>
> ...
>
>> @@ -139,6 +106,8 @@ static struct i2c_driver ssd130x_i2c_driver = {
>> };
>> module_i2c_driver(ssd130x_i2c_driver);
>>
>> +
>
> Seems stray change. Dunno if maintainers can fix this when applying.
>
Ups, indeed. I can fix it when applying.
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH v4 4/5] drm/solomon: Move device info from ssd130x-i2c to the core driver
On Wed, Apr 13, 2022 at 06:23:57PM +0200, Javier Martinez Canillas wrote:
> These are declared in the ssd130x-i2c transport driver but the information
> is not I2C specific, and could be used by other SSD130x transport drivers.
>
> Move them to the ssd130x core driver and just set the OF device entries to
> an ID that could be used to lookup the correct device info from an array.
>
> While being there, also move the SSD130X_DATA and SSD130X_COMMAND control
> bytes. Since even though they are used by the I2C interface, they could
> also be useful for other transport protocols such as SPI.
Thanks!
...
> @@ -139,6 +106,8 @@ static struct i2c_driver ssd130x_i2c_driver = {
> };
> module_i2c_driver(ssd130x_i2c_driver);
>
> +
Seems stray change. Dunno if maintainers can fix this when applying.
> MODULE_DESCRIPTION(DRIVER_DESC);
> MODULE_AUTHOR("Javier Martinez Canillas ");
> MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(DRM_SSD130X);
--
With Best Regards,
Andy Shevchenko
[PATCH v4 0/5] drm/solomon: Add SSD130x OLED displays SPI support
Hello, This series adds a ssd130x-spi driver that provides a 4-wire SPI transport support for SSD130x OLED controllers that can be accessed over a SPI bus. The driver is quite similar to existing ssd130x-i2c driver that is used by I2C controllers, but there is a difference in the protocol used by SSD130x depending on the transport used. The details are in patch #4 description. Patch #1 just makes the current ssd130x-i2c compatible strings in the DT binding to be deprecated, and add new ones that don't have an "fb-i2c". Patch #2 extends the DT binding with the properties needed to support SPI. Patch #3 adds the new compatible strings to the OF device ID table in the ssd130x-i2c DRM driver and deprecate the old ones. Patch #4 moves the device info for the different SSD130x variants from the ssd130x-i2c transport driver to the ssd130x core driver. Finally patch #5 adds the ssd130x-spi DRM driver for the OLED controllers that come with a 4-wire SPI interface, instead of an I2C interface. This is a v4 that addresses the issues pointed out in v3. Best regards, Javier Changes in v4: - Add a description for the dc-gpios property for SPI (Geert Uytterhoeven) - Export ssd13x_variants array using EXPORT_SYMBOL_NS_GPL() (Andy Shevchenko) - Use MODULE_IMPORT_NS(DRM_SSD130X) in the ssd130x-i2c driver (Andy Shevchenko) - Use MODULE_IMPORT_NS(DRM_SSD130X) in the ssd130x-spi driver (Andy Shevchenko) Changes in v3: - Drop the "sinowealth,sh1106-i2c", wasn't in a released version (Chen-Yu Tsai) - Continue enforcing required properties for deprecated strings (Maxime Ripard) - Add a comment to the properties required for SPI (Geert Uytterhoeven) - Drop the "sinowealth,sh1106-i2c", wasn't in a released version (Chen-Yu Tsai) - s/it/they in the commit description (Geert Uytterhoeven) - Drop unnecessary blank line (Geert Uytterhoeven) - Export variants array and use [ID] in device table (Andy Shevchenko) - Drop ssd130x_spi_get_dc() helper and open code it (Geert Uytterhoeven) - Export variants array and use [ID] in device table (Andy Shevchenko) - Add Geert Uytterhoeven's Reviewed-by tag to patches. Changes in v2: - Drop the -i2c suffixes from the compatible strings too (Geert Uytterhoeven) - Don't add compatible strings with an "-spi" suffix (Geert Uytterhoeven) - Use the compatible strings that don't have "fb-i2c" (Geert Uytterhoeven). - Drop ssd13x_variant_to_info() and just use the array index (Neil Armstrong). - Add the same compatible strings than I2C (Geert Uytterhoeven) - Add Mark Brown's Acked-by tag to all patches. Javier Martinez Canillas (5): dt-bindings: display: ssd1307fb: Deprecate "-i2c" compatible strings dt-bindings: display: ssd1307fb: Extend schema for SPI controllers drm/solomon: Add ssd130x new compatible strings and deprecate old ones. drm/solomon: Move device info from ssd130x-i2c to the core driver drm/solomon: Add SSD130x OLED displays SPI support .../bindings/display/solomon,ssd1307fb.yaml | 86 +++-- drivers/gpu/drm/solomon/Kconfig | 9 + drivers/gpu/drm/solomon/Makefile | 1 + drivers/gpu/drm/solomon/ssd130x-i2c.c | 64 +++ drivers/gpu/drm/solomon/ssd130x-spi.c | 178 ++ drivers/gpu/drm/solomon/ssd130x.c | 35 +++- drivers/gpu/drm/solomon/ssd130x.h | 14 ++ 7 files changed, 330 insertions(+), 57 deletions(-) create mode 100644 drivers/gpu/drm/solomon/ssd130x-spi.c -- 2.35.1
[PATCH v4 4/5] drm/solomon: Move device info from ssd130x-i2c to the core driver
These are declared in the ssd130x-i2c transport driver but the information
is not I2C specific, and could be used by other SSD130x transport drivers.
Move them to the ssd130x core driver and just set the OF device entries to
an ID that could be used to lookup the correct device info from an array.
While being there, also move the SSD130X_DATA and SSD130X_COMMAND control
bytes. Since even though they are used by the I2C interface, they could
also be useful for other transport protocols such as SPI.
Suggested-by: Chen-Yu Tsai
Signed-off-by: Javier Martinez Canillas
Acked-by: Mark Brown
Reviewed-by: Geert Uytterhoeven
---
Changes in v4:
- Export ssd13x_variants array using EXPORT_SYMBOL_NS_GPL() (Andy Shevchenko)
- Use MODULE_IMPORT_NS(DRM_SSD130X) in the ssd130x-i2c driver (Andy Shevchenko)
Changes in v3:
- s/it/they in the commit description (Geert Uytterhoeven)
- Drop unnecessary blank line (Geert Uytterhoeven)
- Export variants array and use [ID] in device table (Andy Shevchenko)
Changes in v2:
- Drop ssd13x_variant_to_info() and just use the array index (Neil Armstrong).
drivers/gpu/drm/solomon/ssd130x-i2c.c | 53 ++-
drivers/gpu/drm/solomon/ssd130x.c | 35 --
drivers/gpu/drm/solomon/ssd130x.h | 14 +++
3 files changed, 57 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c
b/drivers/gpu/drm/solomon/ssd130x-i2c.c
index 45867ef2bc8b..b0dd218da13c 100644
--- a/drivers/gpu/drm/solomon/ssd130x-i2c.c
+++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c
@@ -53,76 +53,43 @@ static void ssd130x_i2c_shutdown(struct i2c_client *client)
ssd130x_shutdown(ssd130x);
}
-static struct ssd130x_deviceinfo ssd130x_sh1106_deviceinfo = {
- .default_vcomh = 0x40,
- .default_dclk_div = 1,
- .default_dclk_frq = 5,
- .page_mode_only = 1,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1305_deviceinfo = {
- .default_vcomh = 0x34,
- .default_dclk_div = 1,
- .default_dclk_frq = 7,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1306_deviceinfo = {
- .default_vcomh = 0x20,
- .default_dclk_div = 1,
- .default_dclk_frq = 8,
- .need_chargepump = 1,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1307_deviceinfo = {
- .default_vcomh = 0x20,
- .default_dclk_div = 2,
- .default_dclk_frq = 12,
- .need_pwm = 1,
-};
-
-static struct ssd130x_deviceinfo ssd130x_ssd1309_deviceinfo = {
- .default_vcomh = 0x34,
- .default_dclk_div = 1,
- .default_dclk_frq = 10,
-};
-
static const struct of_device_id ssd130x_of_match[] = {
{
.compatible = "sinowealth,sh1106",
- .data = _sh1106_deviceinfo,
+ .data = _variants[SH1106_ID],
},
{
.compatible = "solomon,ssd1305",
- .data = _ssd1305_deviceinfo,
+ .data = _variants[SSD1305_ID],
},
{
.compatible = "solomon,ssd1306",
- .data = _ssd1306_deviceinfo,
+ .data = _variants[SSD1306_ID],
},
{
.compatible = "solomon,ssd1307",
- .data = _ssd1307_deviceinfo,
+ .data = _variants[SSD1307_ID],
},
{
.compatible = "solomon,ssd1309",
- .data = _ssd1309_deviceinfo,
+ .data = _variants[SSD1309_ID],
},
/* Deprecated but kept for backward compatibility */
{
.compatible = "solomon,ssd1305fb-i2c",
- .data = _ssd1305_deviceinfo,
+ .data = _variants[SSD1305_ID],
},
{
.compatible = "solomon,ssd1306fb-i2c",
- .data = _ssd1306_deviceinfo,
+ .data = _variants[SSD1306_ID],
},
{
.compatible = "solomon,ssd1307fb-i2c",
- .data = _ssd1307_deviceinfo,
+ .data = _variants[SSD1307_ID],
},
{
.compatible = "solomon,ssd1309fb-i2c",
- .data = _ssd1309_deviceinfo,
+ .data = _variants[SSD1309_ID],
},
{ /* sentinel */ }
};
@@ -139,6 +106,8 @@ static struct i2c_driver ssd130x_i2c_driver = {
};
module_i2c_driver(ssd130x_i2c_driver);
+
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_AUTHOR("Javier Martinez Canillas ");
MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(DRM_SSD130X);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c
b/drivers/gpu/drm/solomon/ssd130x.c
index a7e784518c69..ba2de93d00f0 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -39,9 +39,6 @@
#define DRIVER_MAJOR 1
#define DRIVER_MINOR 0
-#define SSD130X_DATA 0x40
-#define SSD130X_COMMAND0x80
-
#define SSD130X_PAGE_COL_START_LOW 0x00
#define SSD130X_PAGE_COL_START_HIGH0x10
#define
[PATCH v4 5/5] drm/solomon: Add SSD130x OLED displays SPI support
The ssd130x driver only provides the core support for these devices but it
does not have any bus transport logic. Add a driver to interface over SPI.
There is a difference in the communication protocol when using 4-wire SPI
instead of I2C. For the latter, a control byte that contains a D/C# field
has to be sent. This field tells the controller whether the data has to be
written to the command register or to the graphics display data memory.
But for 4-wire SPI that control byte is not used, instead a real D/C# line
must be pulled HIGH for commands data and LOW for graphics display data.
For this reason the standard SPI regmap can't be used and a custom .write
bus handler is needed.
Signed-off-by: Javier Martinez Canillas
Acked-by: Mark Brown
---
Changes in v4:
- Use MODULE_IMPORT_NS(DRM_SSD130X) in the ssd130x-spi driver (Andy Shevchenko)
Changes in v3:
- Drop ssd130x_spi_get_dc() helper and open code it (Geert Uytterhoeven)
- Export variants array and use [ID] in device table (Andy Shevchenko)
Changes in v2:
- Add the same compatible strings than I2C (Geert Uytterhoeven)
drivers/gpu/drm/solomon/Kconfig | 9 ++
drivers/gpu/drm/solomon/Makefile | 1 +
drivers/gpu/drm/solomon/ssd130x-spi.c | 178 ++
3 files changed, 188 insertions(+)
create mode 100644 drivers/gpu/drm/solomon/ssd130x-spi.c
diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig
index 8c0a0c788385..e170716d976b 100644
--- a/drivers/gpu/drm/solomon/Kconfig
+++ b/drivers/gpu/drm/solomon/Kconfig
@@ -20,3 +20,12 @@ config DRM_SSD130X_I2C
I2C bus.
If M is selected the module will be called ssd130x-i2c.
+
+config DRM_SSD130X_SPI
+ tristate "DRM support for Solomon SSD130X OLED displays (SPI bus)"
+ depends on DRM_SSD130X && SPI
+ select REGMAP
+ help
+ Say Y here if the SSD130x OLED display is connected via SPI bus.
+
+ If M is selected the module will be called ssd130x-spi.
diff --git a/drivers/gpu/drm/solomon/Makefile b/drivers/gpu/drm/solomon/Makefile
index 4bfc5acb0447..b5fc792257d7 100644
--- a/drivers/gpu/drm/solomon/Makefile
+++ b/drivers/gpu/drm/solomon/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_DRM_SSD130X) += ssd130x.o
obj-$(CONFIG_DRM_SSD130X_I2C) += ssd130x-i2c.o
+obj-$(CONFIG_DRM_SSD130X_SPI) += ssd130x-spi.o
diff --git a/drivers/gpu/drm/solomon/ssd130x-spi.c
b/drivers/gpu/drm/solomon/ssd130x-spi.c
new file mode 100644
index ..c94bbaa731da
--- /dev/null
+++ b/drivers/gpu/drm/solomon/ssd130x-spi.c
@@ -0,0 +1,178 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * DRM driver for Solomon SSD130X OLED displays (SPI bus)
+ *
+ * Copyright 2022 Red Hat Inc.
+ * Authors: Javier Martinez Canillas
+ */
+#include
+#include
+
+#include "ssd130x.h"
+
+#define DRIVER_NAME"ssd130x-spi"
+#define DRIVER_DESC"DRM driver for Solomon SSD130X OLED displays (SPI)"
+
+struct ssd130x_spi_transport {
+ struct spi_device *spi;
+ struct gpio_desc *dc;
+};
+
+static const struct regmap_config ssd130x_spi_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+/*
+ * The regmap bus .write handler, it is just a wrapper around spi_write()
+ * but toggling the Data/Command control pin (D/C#). Since for 4-wire SPI
+ * a D/C# pin is used, in contrast with I2C where a control byte is sent,
+ * prior to every data byte, that contains a bit with the D/C# value.
+ *
+ * These control bytes are considered registers by the ssd130x core driver
+ * and can be used by the ssd130x SPI driver to determine if the data sent
+ * is for a command register or for the Graphic Display Data RAM (GDDRAM).
+ */
+static int ssd130x_spi_write(void *context, const void *data, size_t count)
+{
+ struct ssd130x_spi_transport *t = context;
+ struct spi_device *spi = t->spi;
+ const u8 *reg = data;
+
+ if (*reg == SSD130X_COMMAND)
+ gpiod_set_value_cansleep(t->dc, 0);
+
+ if (*reg == SSD130X_DATA)
+ gpiod_set_value_cansleep(t->dc, 1);
+
+ /* Remove the control byte since is not used by the 4-wire SPI */
+ return spi_write(spi, ((u8 *)data) + 1, count - 1);
+}
+
+/* The ssd130x driver does not read registers but regmap expects a .read */
+static int ssd130x_spi_read(void *context, const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ return -EOPNOTSUPP;
+}
+
+/*
+ * A custom bus is needed due the special write that toggles a D/C# pin,
+ * another option could be to just have a .reg_write() callback but that
+ * will prevent to do data writes in bulk.
+ *
+ * Once the regmap API is extended to support defining a bulk write handler
+ * in the struct regmap_config, this can be simplified and the bus dropped.
+ */
+static struct regmap_bus regmap_ssd130x_spi_bus = {
+ .write = ssd130x_spi_write,
+ .read = ssd130x_spi_read,
+};
+
+static int ssd130x_spi_probe(struct spi_device *spi)
+{
[PATCH v4 2/5] dt-bindings: display: ssd1307fb: Extend schema for SPI controllers
The Solomon SSD130x OLED displays can either have an I2C or SPI interface,
add to the schema the properties and examples for OLED devices under SPI.
Signed-off-by: Javier Martinez Canillas
Acked-by: Mark Brown
Reviewed-by: Geert Uytterhoeven
---
Changes in v4:
- Add a description for the dc-gpios property for SPI (Geert Uytterhoeven)
Changes in v3:
- Add a comment to the properties required for SPI (Geert Uytterhoeven)
Changes in v2:
- Don't add compatible strings with an "-spi" suffix (Geert Uytterhoeven)
.../bindings/display/solomon,ssd1307fb.yaml | 42 ++-
1 file changed, 40 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
index 7653b6c3fcb6..3fbd87c2c120 100644
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
@@ -38,9 +38,20 @@ properties:
reset-gpios:
maxItems: 1
+ # Only required for SPI
+ dc-gpios:
+description:
+ GPIO connected to the controller's D/C# (Data/Command) pin,
+ that is needed for 4-wire SPI to tell the controller if the
+ data sent is for a command register or the display data RAM
+maxItems: 1
+
vbat-supply:
description: The supply for VBAT
+ # Only required for SPI
+ spi-max-frequency: true
+
solomon,height:
$ref: /schemas/types.yaml#/definitions/uint32
default: 16
@@ -220,14 +231,14 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
-ssd1307: oled@3c {
+ssd1307_i2c: oled@3c {
compatible = "solomon,ssd1307";
reg = <0x3c>;
pwms = < 4 3000>;
reset-gpios = < 7>;
};
-ssd1306: oled@3d {
+ssd1306_i2c: oled@3d {
compatible = "solomon,ssd1306";
reg = <0x3c>;
pwms = < 4 3000>;
@@ -238,3 +249,30 @@ examples:
solomon,lookup-table = /bits/ 8 <0x3f 0x3f 0x3f 0x3f>;
};
};
+ - |
+spi {
+#address-cells = <1>;
+#size-cells = <0>;
+
+ssd1307_spi: oled@0 {
+compatible = "solomon,ssd1307";
+reg = <0x0>;
+pwms = < 4 3000>;
+reset-gpios = < 7>;
+dc-gpios = < 8>;
+spi-max-frequency = <1000>;
+};
+
+ssd1306_spi: oled@1 {
+compatible = "solomon,ssd1306";
+reg = <0x1>;
+pwms = < 4 3000>;
+reset-gpios = < 7>;
+dc-gpios = < 8>;
+spi-max-frequency = <1000>;
+solomon,com-lrremap;
+solomon,com-invdir;
+solomon,com-offset = <32>;
+solomon,lookup-table = /bits/ 8 <0x3f 0x3f 0x3f 0x3f>;
+};
+};
--
2.35.1
[PATCH v4 3/5] drm/solomon: Add ssd130x new compatible strings and deprecate old ones.
The current compatible strings for SSD130x I2C controllers contain an "fb"
and "-i2c" suffixes. These have been deprecated and more correct ones were
added, that don't encode a subsystem or bus used to interface the devices.
Signed-off-by: Javier Martinez Canillas
Acked-by: Mark Brown
Reviewed-by: Geert Uytterhoeven
---
(no changes since v3)
Changes in v3:
- Drop the "sinowealth,sh1106-i2c", wasn't in a released version (Chen-Yu Tsai)
Changes in v2:
- Use the compatible strings that don't have "fb-i2c" (Geert Uytterhoeven).
drivers/gpu/drm/solomon/ssd130x-i2c.c | 19 ++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/solomon/ssd130x-i2c.c
b/drivers/gpu/drm/solomon/ssd130x-i2c.c
index d099b241dd3f..45867ef2bc8b 100644
--- a/drivers/gpu/drm/solomon/ssd130x-i2c.c
+++ b/drivers/gpu/drm/solomon/ssd130x-i2c.c
@@ -88,9 +88,26 @@ static struct ssd130x_deviceinfo ssd130x_ssd1309_deviceinfo
= {
static const struct of_device_id ssd130x_of_match[] = {
{
- .compatible = "sinowealth,sh1106-i2c",
+ .compatible = "sinowealth,sh1106",
.data = _sh1106_deviceinfo,
},
+ {
+ .compatible = "solomon,ssd1305",
+ .data = _ssd1305_deviceinfo,
+ },
+ {
+ .compatible = "solomon,ssd1306",
+ .data = _ssd1306_deviceinfo,
+ },
+ {
+ .compatible = "solomon,ssd1307",
+ .data = _ssd1307_deviceinfo,
+ },
+ {
+ .compatible = "solomon,ssd1309",
+ .data = _ssd1309_deviceinfo,
+ },
+ /* Deprecated but kept for backward compatibility */
{
.compatible = "solomon,ssd1305fb-i2c",
.data = _ssd1305_deviceinfo,
--
2.35.1
[PATCH v4 1/5] dt-bindings: display: ssd1307fb: Deprecate "-i2c" compatible strings
The current compatible strings for SSD130x I2C controllers contain both an
"fb" and "-i2c" suffixes. It seems to indicate that are for a fbdev driver
and also that are for devices that can be accessed over an I2C bus.
But a DT is supposed to describe the hardware and not Linux implementation
details. So let's deprecate those compatible strings and add new ones that
only contain the vendor and device name, without any of these suffixes.
These will just describe the device and can be matched by both I2C and SPI
DRM drivers. The required properties should still be enforced for old ones.
While being there, just drop the "sinowealth,sh1106-i2c" compatible string
since that was never present in a released Linux version.
Signed-off-by: Javier Martinez Canillas
Acked-by: Mark Brown
Reviewed-by: Geert Uytterhoeven
---
(no changes since v3)
Changes in v3:
- Drop the "sinowealth,sh1106-i2c", wasn't in a released version (Chen-Yu Tsai)
- Continue enforcing required properties for deprecated strings (Maxime Ripard)
Changes in v2:
- Drop the -i2c suffixes from the compatible strings too (Geert Uytterhoeven)
.../bindings/display/solomon,ssd1307fb.yaml | 44 +--
1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
index ade61d502edd..7653b6c3fcb6 100644
--- a/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
+++ b/Documentation/devicetree/bindings/display/solomon,ssd1307fb.yaml
@@ -12,12 +12,22 @@ maintainers:
properties:
compatible:
-enum:
- - sinowealth,sh1106-i2c
- - solomon,ssd1305fb-i2c
- - solomon,ssd1306fb-i2c
- - solomon,ssd1307fb-i2c
- - solomon,ssd1309fb-i2c
+oneOf:
+ # Deprecated compatible strings
+ - items:
+ - enum:
+ - solomon,ssd1305fb-i2c
+ - solomon,ssd1306fb-i2c
+ - solomon,ssd1307fb-i2c
+ - solomon,ssd1309fb-i2c
+deprecated: true
+ - items:
+ - enum:
+ - sinowealth,sh1106
+ - solomon,ssd1305
+ - solomon,ssd1306
+ - solomon,ssd1307
+ - solomon,ssd1309
reg:
maxItems: 1
@@ -136,7 +146,7 @@ allOf:
properties:
compatible:
contains:
-const: sinowealth,sh1106-i2c
+const: sinowealth,sh1106
then:
properties:
solomon,dclk-div:
@@ -148,7 +158,9 @@ allOf:
properties:
compatible:
contains:
-const: solomon,ssd1305fb-i2c
+enum:
+ - solomon,ssd1305-i2c
+ - solomon,ssd1305
then:
properties:
solomon,dclk-div:
@@ -160,7 +172,9 @@ allOf:
properties:
compatible:
contains:
-const: solomon,ssd1306fb-i2c
+enum:
+ - solomon,ssd1306-i2c
+ - solomon,ssd1306
then:
properties:
solomon,dclk-div:
@@ -172,7 +186,9 @@ allOf:
properties:
compatible:
contains:
-const: solomon,ssd1307fb-i2c
+enum:
+ - solomon,ssd1307-i2c
+ - solomon,ssd1307
then:
properties:
solomon,dclk-div:
@@ -186,7 +202,9 @@ allOf:
properties:
compatible:
contains:
-const: solomon,ssd1309fb-i2c
+enum:
+ - solomon,ssd1309-i2c
+ - solomon,ssd1309
then:
properties:
solomon,dclk-div:
@@ -203,14 +221,14 @@ examples:
#size-cells = <0>;
ssd1307: oled@3c {
-compatible = "solomon,ssd1307fb-i2c";
+compatible = "solomon,ssd1307";
reg = <0x3c>;
pwms = < 4 3000>;
reset-gpios = < 7>;
};
ssd1306: oled@3d {
-compatible = "solomon,ssd1306fb-i2c";
+compatible = "solomon,ssd1306";
reg = <0x3c>;
pwms = < 4 3000>;
reset-gpios = < 7>;
--
2.35.1
[RFC] drm/kms: Stop registering multiple /sys/class/backlight devs for a single display
Hi All, As discussed in the "[RFC] drm/kms: control display brightness through drm_connector properties" thread, step 1 of the plan is to stop registering multiple /sys/class/backlight devs for a single display. On x86 there can be multiple firmware + direct-hw-access methods for controlling the backlight and in some cases the kernel registers multiple backlight-devices for a single internal laptop LCD panel, 2 common scenarios where this happens are: 1) i915 and nouveau unconditionally register their "native" backlight dev even on devices where /sys/class/backlight/acpi_video0 must be used to control the backlight, relying on userspace to prefer the "firmware" acpi_video0 device over "native" devices. 2) amdgpu and nouveau rely on the acpi_video driver initializing before them, which currently causes /sys/class/backlight/acpi_video0 to usually show up and then they register their own native backlight driver after which the drivers/acpi/video_detect.c code unregisters the acpi_video0 device. This means that userspace briefly sees 2 devices and the disappearing of acpi_video0 after a brief time confuses the systemd backlight level save/restore code, see e.g.: https://bbs.archlinux.org/viewtopic.php?id=269920 Fixing kms driver unconditionally register their "native" backlight dev === The plan for fixing 1) is to add a "bool native_backlight_available" parameter to acpi_video_get_backlight_type(), which will be set to true when called by e.g. the i915 code to check if it should register its native backlight-device. This way acpi_video_get_backlight_type() will know that a native backlight-device is (will be) available even though it has not been registered yet and then it can return acpi_backlight_native if that is the best option. And then the i915 code will only actually register its native backlight when acpi_backlight_native gets returned, thus hiding it in scenario 1. Fixing acpi_video0 getting registered for a brief time == ATM the acpi_video code will delay parsing the ACPI video extensions when an i915 opregion is present and will immediately parse these + register an acpi_video backlight device on laptops without Intel integrated graphics. On laptops with i915 gfx the i915 driver calls acpi_video_register() near the end of its probe() function when things are ready for the acpi_video code to run, avoiding scenario 2. Where as on systems without i915 gfx acpi_video's module_init() immediately calls acpi_video_register() and then later the ACPI video_detect code calls acpi_video_unregister_backlight() to hide the acpi_video# backlight-device on systems where the native backlight-device should be used. The plan to fix this is to add an acpi_video_register_backlight() and to make acpi_video_register() do all the usual ACPI video extension probing, but have it skip the actual registering of the backlight devices and have drivers explicitly call acpi_video_register() after they have setup their own native backlight-device. This way acpi_video_get_backlight_type() already will know that a native backlight-device is available (and preferred) when acpi_video_register_backlight() runs and the registering of the acpi_video# device will then be skipped, removing it briefly showing up and disappearing again. One problem with this approach is that this relies on the GPU driver to call acpi_video_register_backlight() when it is done. One could argue that this is actually a feature, we have had issues with some desktops where acpi_video falsely registers its backlight (even though there is no internal LCD panel), but this will likely cause issues on some systems (1). So the plan is to queue a delayed work with an 8 second (1) delay from acpi_video_register() and have that register the backlight-device if not done already. Other issues The above only takes native vs acpi_video backlight issues into account, there are also a couple of other scenarios which may lead to multiple backlight-devices getting registered: 1) Apple laptops using the apple_gmux driver 2) Nvidia laptops using the (new) nvidia-wmi-ec-backlight driver 3) drivers/platform/x86 drivers calling acpi_video_set_dmi_backlight_type() to override the normal acpi_video_get_backlight_type() heuristics after the native/acpi_video drivers have already loaded The plan for 1) + 2) is to extend the acpi_backlight_type enum with acpi_backlight_gmux and acpi_backlight_nvidia_wmi_ec values and to add detection of these 2 to acpi_video_get_backlight_type(). The plan for 3) is to move the DMI quirks from drivers/platform/x86 drivers which call acpi_video_set_dmi_backlight_type() in to the existing DMI quirk table in drivers/acpi/video_detect.c, so that they will be available during the first/every call of acpi_video_get_backlight_type() and then remove
Re: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
On Wed, Apr 13, 2022 at 06:06:01PM +0200, Christoph Hellwig wrote: > On Wed, Apr 13, 2022 at 08:39:52AM -0300, Jason Gunthorpe wrote: > > I already looked into that for a while, it is a real mess too because > > of how the notifiers are used by the current drivers, eg gvt assumes > > the notifier is called during its open_device callback to setup its > > kvm. > > gvt very much expects kvm to be set before open and thus get the > cachup notifier, yes. And given that this is how qemu uses > the ioctl I think we can actually simplify this further and require > vfio_group_set_kvm to be called before open for s390/ap as well and > do away with this whole mess. Yeah, I was thinking about that too, but on the other hand I think it is completely wrong that gvt requires kvm at all. A vfio_device is not supposed to be tightly linked to KVM - the only exception possibly being s390.. Jason
[PATCH] drm/radeon: Add build directory to include path
From: Michel Dänzer Fixes compile errors with out-of-tree builds, e.g. ../drivers/gpu/drm/radeon/r420.c:38:10: fatal error: r420_reg_safe.h: No such file or directory 38 | #include "r420_reg_safe.h" | ^ Signed-off-by: Michel Dänzer --- drivers/gpu/drm/radeon/Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/radeon/Makefile b/drivers/gpu/drm/radeon/Makefile index 11c97edde54d..37caf5236048 100644 --- a/drivers/gpu/drm/radeon/Makefile +++ b/drivers/gpu/drm/radeon/Makefile @@ -3,6 +3,8 @@ # Makefile for the drm device driver. This driver provides support for the # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. +ccflags-y += -I$(src) + hostprogs := mkregtable targets := rn50_reg_safe.h r100_reg_safe.h r200_reg_safe.h rv515_reg_safe.h r300_reg_safe.h r420_reg_safe.h rs600_reg_safe.h r600_reg_safe.h evergreen_reg_safe.h cayman_reg_safe.h -- 2.35.1
[PATCH] drm/bochs: Explicitly include linux/module.h
From: Michel Dänzer Instead of relying on it getting pulled in indirectly. Signed-off-by: Michel Dänzer --- drivers/gpu/drm/tiny/bochs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c index ed971c8bb446..4f8bf86633df 100644 --- a/drivers/gpu/drm/tiny/bochs.c +++ b/drivers/gpu/drm/tiny/bochs.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-or-later +#include #include #include -- 2.35.1
Re: [PATCH 2/2] fbdev: Remove hot-unplug workaround for framebuffers without device
On Wed, Apr 13, 2022 at 12:50:50PM +0200, Javier Martinez Canillas wrote:
> On 4/13/22 11:24, Thomas Zimmermann wrote:
> > A workaround makes fbdev hot-unplugging work for framebuffers without
> > device. The only user for this feature was offb. As each OF framebuffer
> > now has an associated platform device, the workaround is no longer
> > needed. Remove it. Effectively reverts commit 0f525289ff0d ("fbdev: Fix
> > unregistering of framebuffers without device").
> >
> > Signed-off-by: Thomas Zimmermann
> > ---
> > drivers/video/fbdev/core/fbmem.c | 9 +
> > 1 file changed, 1 insertion(+), 8 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/core/fbmem.c
> > b/drivers/video/fbdev/core/fbmem.c
> > index bc6ed750e915..bdd00d381bbc 100644
> > --- a/drivers/video/fbdev/core/fbmem.c
> > +++ b/drivers/video/fbdev/core/fbmem.c
> > @@ -1579,14 +1579,7 @@ static void
> > do_remove_conflicting_framebuffers(struct apertures_struct *a,
> > * If it's not a platform device, at least print a
> > warning. A
> > * fix would add code to remove the device from the
> > system.
> > */
> > - if (!device) {
> > - /* TODO: Represent each OF framebuffer as its
> > own
> > -* device in the device hierarchy. For now, offb
> > -* doesn't have such a device, so unregister the
> > -* framebuffer as before without warning.
> > -*/
> > - do_unregister_framebuffer(registered_fb[i]);
>
> Maybe we could still keep this for a couple of releases but with a big
> warning that's not supported in case there are out-of-tree drivers out
> there that still do this ?
>
> Or at least a warning if the do_unregister_framebuffer() call is removed.
Yeah dying while holding console_lock isn't fun, and not having a WARN_ON
+ bail-out code pretty much forces bug reporters to do a bisect here to
give us something more than "machine dies at boot with no messages".
I'd just outright keep the WARN_ON here for 1-2 years even to really make
sure we got all the bug reports, since often these older machines only
update onto LTS releases.
And it needs to be a WARN_ON + bail out since BUG_ON is as bad as just
oopsing.
-Daniel
>
> Regardless of what you chose to do, the patch looks good to me.
>
> Reviewed-by: Javier Martinez Canillas
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Linux Engineering
> Red Hat
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Re: refactor the i915 GVT support and move to the modern mdev API v3
On Wed, 13 Apr 2022, Christoph Hellwig wrote: > On Wed, Apr 13, 2022 at 01:47:05PM +, Wang, Zhi A wrote: >> > the GVT code in the i915 is a bit of a mess right now due to strange >> > abstractions and lots of indirect calls. This series refactors various >> > bits to clean that up. The main user visible change is that almost all >> > of the GVT code moves out of the main i915 driver and into the kvmgt >> > module. >> >> Hi Christoph: >> >> Do you want me to merge the GVT-g patches in this series? Or you want them >> to get merged from your side? > > The two option here are drm tree via gvt and i915 trees or the vfio > tree, neither of which really is my tree. > > We already have a fair bit of vfio changes at the tail end of the series, > and Jason has some more that should sit on top of it, and I have some > more that I haven't sent yet. > > So if we could get the MMIO table and Makefile cleanups into a topic > branch that we could pull into the vfio tree and merge it through that > that would seem easiest to me, assuming that is ok with the i915, drm > and vfio maintainers. AFAICS the changes are mostly to gvt/, and at least I'm fine with the minor changes to i915 (in this series and in my two patches) being merged via whichever tree you all see fit. Acked-by: Jani Nikula Joonas, Tvrtko, Rodrigo, chime in now if you have any issues with that. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
[PULL] drm-intel-next
Hi Dave & Daniel - The first drm/i915 pull for v5.19. BR, Jani. drm-intel-next-2022-04-13-1: drm/i915 feature pull for v5.19: Features and functionality: - Add support for new Tile 4 format on DG2 (Stan) - Add support for new CCS clear color compression on DG2 (Mika, Juha-Pekka) - Add support for new render and media compression formats on DG2 (Matt) - Support multiple eDP and LVDS native mode refresh rates (Ville) - Support static DRRS (Ville) - ATS-M platform info (Matt) - RPL-S PCI IDs (Tejas) - Extend DP HDR support to HSW+ (Uma) - Bump ADL-P DMC version to v2.16 (Madhumitha) - Let users disable PSR2 while enabling PSR1 (José) Refactoring and cleanups: - Massive DRRS and panel fixed mode refactoring and cleanups (Ville) - Power well refactoring and cleanup (Imre) - Clean up and refactor crtc readout and compute config (Ville) - Use kernel string helpers (Lucas) - Refactor gmbus pin lookups and allocation (Jani) - PCH display cleanups (Ville) - DPLL and DPLL manager refactoring (Ville) - Include and header refactoring (Jani, Tvrtko) - DMC abstractions (Jani) - Non-x86 build refactoring (Casey) - VBT parsing refactoring (Ville) - Bigjoiner refactoring (Ville) - Optimize plane, pfit, scaler, etc. programming using unlocked writes (Ville) - Split several register writes in commit to noarm+arm pairs (Ville) - Clean up SAGV handling (Ville) - Clean up bandwidth and ddb allocation (Ville) - FBC cleanups (Ville) Fixes: - Fix native HDMI and DP HDMI DFP clock limits on deep color/4:2:0 (Ville) - Fix DMC firmware platform check (Lucas) - Fix cursor coordinates on bigjoiner secondary (Ville) - Fix MSO vs. bigjoiner timing confusion (Ville) - Fix ADL-P eDP voltage swing (José) - Fix VRR capability property update (Manasi) - Log DG2 SNPS PHY calibration errors (Matt, Lucas) - Fix PCODE request status checks (Stan) - Fix uncore unclaimed access warnings (Lucas) - Fix VBT new max TMDS clock parsing (Shawn) - Fix ADL-P non-existent underrun recovery (Swathi Dhanavanthri) - Fix ADL-N stepping info (Tejas) - Fix DPT mapping flags to contiguous (Stan) - Fix DG2 max display bandwidth (Vinod) - Fix DP low voltage SKU checks (Ankit) - Fix RPL-S VT-d translation enable via quirk (Tejas) - Fixes to PSR2 (José) - Fix PIPE_MBUS_DBOX_CTL programming (José) - Fix LTTPR capability read/check on DP 1.2 (Imre) - Fix ADL-P register corruption after DDI clock enabling (Imre) - Fix ADL-P MBUS DBOX BW and B credits (Caz) Merges: - Backmerge drm-next (Rodrigo, Jani) The following changes since commit 3123109284176b1532874591f7c81f3837bbdc17: Linux 5.18-rc1 (2022-04-03 14:08:21 -0700) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-intel tags/drm-intel-next-2022-04-13-1 for you to fetch changes up to b39d2c6202426b560641e5800c5523851b5db586: drm/i915/fbc: Call intel_fbc_activate() directly from frontbuffer flush (2022-04-13 17:20:49 +0300) drm/i915 feature pull for v5.19: Features and functionality: - Add support for new Tile 4 format on DG2 (Stan) - Add support for new CCS clear color compression on DG2 (Mika, Juha-Pekka) - Add support for new render and media compression formats on DG2 (Matt) - Support multiple eDP and LVDS native mode refresh rates (Ville) - Support static DRRS (Ville) - ATS-M platform info (Matt) - RPL-S PCI IDs (Tejas) - Extend DP HDR support to HSW+ (Uma) - Bump ADL-P DMC version to v2.16 (Madhumitha) - Let users disable PSR2 while enabling PSR1 (José) Refactoring and cleanups: - Massive DRRS and panel fixed mode refactoring and cleanups (Ville) - Power well refactoring and cleanup (Imre) - Clean up and refactor crtc readout and compute config (Ville) - Use kernel string helpers (Lucas) - Refactor gmbus pin lookups and allocation (Jani) - PCH display cleanups (Ville) - DPLL and DPLL manager refactoring (Ville) - Include and header refactoring (Jani, Tvrtko) - DMC abstractions (Jani) - Non-x86 build refactoring (Casey) - VBT parsing refactoring (Ville) - Bigjoiner refactoring (Ville) - Optimize plane, pfit, scaler, etc. programming using unlocked writes (Ville) - Split several register writes in commit to noarm+arm pairs (Ville) - Clean up SAGV handling (Ville) - Clean up bandwidth and ddb allocation (Ville) - FBC cleanups (Ville) Fixes: - Fix native HDMI and DP HDMI DFP clock limits on deep color/4:2:0 (Ville) - Fix DMC firmware platform check (Lucas) - Fix cursor coordinates on bigjoiner secondary (Ville) - Fix MSO vs. bigjoiner timing confusion (Ville) - Fix ADL-P eDP voltage swing (José) - Fix VRR capability property update (Manasi) - Log DG2 SNPS PHY calibration errors (Matt, Lucas) - Fix PCODE request status checks (Stan) - Fix uncore unclaimed access warnings (Lucas) - Fix VBT new max TMDS clock parsing (Shawn) - Fix ADL-P non-existent underrun recovery (Swathi Dhanavanthri) - Fix ADL-N stepping info (Tejas) - Fix DPT mapping flags to contiguous (Stan) - Fix DG2 max display bandwidth
Re: [PATCHv4] drm/amdgpu: disable ASPM on Intel Alder Lake based systems
Hi Richard,
On Tue, Apr 12, 2022 at 04:50:00PM -0500, Richard Gong wrote:
> Active State Power Management (ASPM) feature is enabled since kernel 5.14.
> There are some AMD GFX cards (such as WX3200 and RX640) that won't work
> with ASPM-enabled Intel Alder Lake based systems. Using these GFX cards as
> video/display output, Intel Alder Lake based systems will hang during
> suspend/resume.
>
> The issue was initially reported on one system (Dell Precision 3660 with
> BIOS version 0.14.81), but was later confirmed to affect at least 4 Alder
> Lake based systems.
>
> Add extra check to disable ASPM on Intel Alder Lake based systems.
>
> Fixes: 0064b0ce85bb ("drm/amd/pm: enable ASPM by default")
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1885
> Reported-by: kernel test robot
> Signed-off-by: Richard Gong
> ---
> v4: s/CONFIG_X86_64/CONFIG_X86
> enhanced check logic
> v3: s/intel_core_asom_chk/aspm_support_quirk_check
> correct build error with W=1 option
> v2: correct commit description
> move the check from chip family to problematic platform
> ---
> drivers/gpu/drm/amd/amdgpu/vi.c | 17 -
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c b/drivers/gpu/drm/amd/amdgpu/vi.c
> index 039b90cdc3bc..b33e0a9bee65 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> @@ -81,6 +81,10 @@
> #include "mxgpu_vi.h"
> #include "amdgpu_dm.h"
>
> +#if IS_ENABLED(CONFIG_X86)
> +#include
> +#endif
> +
> #define ixPCIE_LC_L1_PM_SUBSTATE 0x100100C6
> #define PCIE_LC_L1_PM_SUBSTATE__LC_L1_SUBSTATES_OVERRIDE_EN_MASK
> 0x0001L
> #define PCIE_LC_L1_PM_SUBSTATE__LC_PCI_PM_L1_2_OVERRIDE_MASK 0x0002L
> @@ -1134,13 +1138,24 @@ static void vi_enable_aspm(struct amdgpu_device *adev)
> WREG32_PCIE(ixPCIE_LC_CNTL, data);
> }
>
> +static bool aspm_support_quirk_check(void)
> +{
> + if (IS_ENABLED(CONFIG_X86)) {
> + struct cpuinfo_x86 *c = _data(0);
> +
> + return !(c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
> + }
I have not seen this reported by a bot, sorry if it is a duplicate. This
breaks non-x86 builds (arm64 allmodconfig for example):
drivers/gpu/drm/amd/amdgpu/vi.c:1144:28: error: implicit declaration of
function 'cpu_data' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
struct cpuinfo_x86 *c = _data(0);
^
drivers/gpu/drm/amd/amdgpu/vi.c:1144:27: error: cannot take the address of an
rvalue of type 'int'
struct cpuinfo_x86 *c = _data(0);
^~~~
drivers/gpu/drm/amd/amdgpu/vi.c:1146:13: error: incomplete definition of type
'struct cpuinfo_x86'
return !(c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
~^
drivers/gpu/drm/amd/amdgpu/vi.c:1144:10: note: forward declaration of 'struct
cpuinfo_x86'
struct cpuinfo_x86 *c = _data(0);
^
drivers/gpu/drm/amd/amdgpu/vi.c:1146:28: error: incomplete definition of type
'struct cpuinfo_x86'
return !(c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
~^
drivers/gpu/drm/amd/amdgpu/vi.c:1144:10: note: forward declaration of 'struct
cpuinfo_x86'
struct cpuinfo_x86 *c = _data(0);
^
drivers/gpu/drm/amd/amdgpu/vi.c:1146:43: error: use of undeclared identifier
'INTEL_FAM6_ALDERLAKE'
return !(c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
^
5 errors generated.
'struct cpuinfo_x86' is only defined for CONFIG_X86 so this section
needs to guarded with the preprocessor, which is how it was done in v2.
Please go back to that.
Cheers,
Nathan
Re: [PATCH] fbcon: Fix delayed takeover locking
On Wed, Apr 13, 2022 at 11:25:02AM +0200, Daniel Vetter wrote:
> On Wed, Apr 13, 2022 at 10:21:28AM +0200, Daniel Vetter wrote:
> > I messed up the delayed takover path in the locking conversion in
> > 6e7da3af008b ("fbcon: Move console_lock for register/unlink/unregister").
> >
> > Fix it by re-extracting the lockless function and using it in the
> > delayed takeover path, where we need to hold the lock already to
> > iterate over the list of already registered fb. Well the current code
> > still is broken in there (since the list is protected by a
> > registration_lock, which we can't take here because it nests the other
> > way round with console_lock), but in the future this will be a list
> > protected by console_lock when this is all sorted out.
> >
> > Reported-by: Nathan Chancellor
> > Cc: Nathan Chancellor
>
> Nathan, if you can supply a tested-by today still I could push it before I
> disappear into easter w/e. I'm pretty sure this is it, but better safe
> than sorry.
> -Daniel
Yup, sorry for the delay, timezones and such :( This patch resolves the
problem I was seeing, thank you for the quick response and fix!
Tested-by: Nathan Chancellor
> > Fixes: 6e7da3af008b ("fbcon: Move console_lock for
> > register/unlink/unregister")
> > Cc: Sam Ravnborg
> > Cc: Thomas Zimmermann
> > Cc: Du Cheng
> > Cc: Claudio Suarez
> > Cc: Greg Kroah-Hartman
> > Cc: Tetsuo Handa
> > Cc: Matthew Wilcox
> > Cc: Zheyu Ma
> > Cc: Guenter Roeck
> > Cc: Helge Deller
> > Cc: Geert Uytterhoeven
> > Cc: Javier Martinez Canillas
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/video/fbdev/core/fbcon.c | 28 ++--
> > 1 file changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/video/fbdev/core/fbcon.c
> > b/drivers/video/fbdev/core/fbcon.c
> > index 6a7d470beec7..b4e43b39d9a8 100644
> > --- a/drivers/video/fbdev/core/fbcon.c
> > +++ b/drivers/video/fbdev/core/fbcon.c
> > @@ -2772,7 +2772,6 @@ static void fbcon_unbind(void)
> > static inline void fbcon_unbind(void) {}
> > #endif /* CONFIG_VT_HW_CONSOLE_BINDING */
> >
> > -/* called with console_lock held */
> > void fbcon_fb_unbind(struct fb_info *info)
> > {
> > int i, new_idx = -1;
> > @@ -2822,7 +2821,6 @@ void fbcon_fb_unbind(struct fb_info *info)
> > console_unlock();
> > }
> >
> > -/* called with console_lock held */
> > void fbcon_fb_unregistered(struct fb_info *info)
> > {
> > int i, idx;
> > @@ -2928,14 +2926,11 @@ MODULE_PARM_DESC(lockless_register_fb,
> > "Lockless framebuffer registration for debugging [default=off]");
> >
> > /* called with console_lock held */
> > -int fbcon_fb_registered(struct fb_info *info)
> > +static int do_fb_registered(struct fb_info *info)
> > {
> > int ret = 0, i, idx;
> >
> > - if (!lockless_register_fb)
> > - console_lock();
> > - else
> > - atomic_inc(_console_lock_warning);
> > + WARN_CONSOLE_UNLOCKED();
> >
> > fbcon_registered_fb[info->node] = info;
> > fbcon_num_registered_fb++;
> > @@ -2945,7 +2940,7 @@ int fbcon_fb_registered(struct fb_info *info)
> >
> > if (deferred_takeover) {
> > pr_info("fbcon: Deferring console take-over\n");
> > - goto out;
> > + return 0;
> > }
> >
> > if (info_idx == -1) {
> > @@ -2965,7 +2960,20 @@ int fbcon_fb_registered(struct fb_info *info)
> > }
> > }
> >
> > -out:
> > + return ret;
> > +}
> > +
> > +int fbcon_fb_registered(struct fb_info *info)
> > +{
> > + int ret;
> > +
> > + if (!lockless_register_fb)
> > + console_lock();
> > + else
> > + atomic_inc(_console_lock_warning);
> > +
> > + ret = do_fb_registered(info);
> > +
> > if (!lockless_register_fb)
> > console_unlock();
> > else
> > @@ -3280,7 +3288,7 @@ static void fbcon_register_existing_fbs(struct
> > work_struct *work)
> > logo_shown = FBCON_LOGO_DONTSHOW;
> >
> > fbcon_for_each_registered_fb(i)
> > - fbcon_fb_registered(fbcon_registered_fb[i]);
> > + do_fb_registered(fbcon_registered_fb[i]);
> >
> > console_unlock();
> > }
> > --
> > 2.34.1
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Re: [PATCH 05/34] drm/i915/gvt: cleanup the Makefile
On Wed, Apr 13, 2022 at 02:26:23PM +, Wang, Zhi A wrote: > On 4/13/22 1:43 PM, Jason Gunthorpe wrote: > > On Wed, Apr 13, 2022 at 01:39:35PM +, Wang, Zhi A wrote: > > > >> It seems Jani's makefile clean patch has already included this one, I can > >> just simply drop this one so that Christoph won't need to re-send > >> everything. > >> > >> For the branch to move on, I am merging the patches and will re-generate > >> the > >> gvt-staging branch, which combines the newest drm-tip vfio-upstream and > >> other > >> gvt branches. > >> > >> If you are in a rush of re-basing the patches of non-GVT-g stuff, you can > >> use > >> gvt-staging branch until my pull request landed in drm-intel-next. > >> > >> Also our QA will test gvt-staging-branch before the pull request. I suppose > >> it will take one or two days. > > > > When you are wrangling the branches it would be great if Christoph's > > series and it's minimal dependencies could be on a single branch that > > could reasonably be pulled to the VFIO tree too, thanks > > > > Jason > > > > Hi Jason: > > I am thinking about the process of merging process. Here are the dependence: > > 1) My patches depend on one patch in drm-intel/drm-intel-next. So it has to > go through drm. > My patches of GVT-g will go through drm-intel-next -> drm -> upstream. > > 2) Christoph's patches depends on my patches, but part of them are for VFIO. > > a. If they are fully going through VFIO repo, they might have to wait my > patches to get landed first. > > b. If only the GVT-g parts goes through GVT repo, and rest of them goes > through VFIO, the rest part still needs to wait. > > What would be a better process? You should organize everything onto one simple branch based on a rc to make this all work. Make your #1 patch as a single patch PR based on rc to drm-intel so it gets to the right tree Make your MMIO series as PR on the branch above that first PR and merge to the gvt tree Make Christoph's series as a PR on the branch above the second PR's MMIO series and merge to the gvt tree Merge the gvt toward DRM in the normal way - ie the main merge path for this should be through DRM. Then ask Alex to merge the 3rd PR as well. I don't see any intel-next stuff in linux-next yet so hopefully it is early enough to get #1 OK. Jason
Re: [PATCH 05/34] drm/i915/gvt: cleanup the Makefile
On 4/13/22 1:43 PM, Jason Gunthorpe wrote: > On Wed, Apr 13, 2022 at 01:39:35PM +, Wang, Zhi A wrote: > >> It seems Jani's makefile clean patch has already included this one, I can >> just simply drop this one so that Christoph won't need to re-send everything. >> >> For the branch to move on, I am merging the patches and will re-generate the >> gvt-staging branch, which combines the newest drm-tip vfio-upstream and other >> gvt branches. >> >> If you are in a rush of re-basing the patches of non-GVT-g stuff, you can use >> gvt-staging branch until my pull request landed in drm-intel-next. >> >> Also our QA will test gvt-staging-branch before the pull request. I suppose >> it will take one or two days. > > When you are wrangling the branches it would be great if Christoph's > series and it's minimal dependencies could be on a single branch that > could reasonably be pulled to the VFIO tree too, thanks > > Jason > Hi Jason: I am thinking about the process of merging process. Here are the dependence: 1) My patches depend on one patch in drm-intel/drm-intel-next. So it has to go through drm. My patches of GVT-g will go through drm-intel-next -> drm -> upstream. 2) Christoph's patches depends on my patches, but part of them are for VFIO. a. If they are fully going through VFIO repo, they might have to wait my patches to get landed first. b. If only the GVT-g parts goes through GVT repo, and rest of them goes through VFIO, the rest part still needs to wait. What would be a better process? Thanks, Zhi.
Re: [PATCH 3/4] dt-bindings: drm/bridge: anx7625: Change bus-type to 7 (DPI)
On Sat, 9 Apr 2022 at 06:47, Xin Ji wrote:
>
> On Mon, Apr 04, 2022 at 12:52:14PM -0500, Rob Herring wrote:
> > On Mon, Mar 28, 2022 at 08:09:54PM +0800, Xin Ji wrote:
> > > Change bus-type define for DPI.
> > >
> > > Fixes: a43661e7e819 ("dt-bindings:drm/bridge:anx7625:add vendor define")
> > >
> > > Signed-off-by: Xin Ji
> > > ---
> > > .../devicetree/bindings/display/bridge/analogix,anx7625.yaml | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > index 0d38d6fe3983..4590186c4a0b 100644
> > > ---
> > > a/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > +++
> > > b/Documentation/devicetree/bindings/display/bridge/analogix,anx7625.yaml
> > > @@ -106,7 +106,7 @@ properties:
> > >remote-endpoint: true
> > >
> > >bus-type:
> > > -enum: [1, 5]
> > > +enum: [7]
> >
> > Changing is an ABI break, but didn't we revert adding this?
> Hi Rob, sorry, what do you mean about ABI break? Do I need remove this
> patch in this serial? Or do I need revert patch
> https://patchwork.freedesktop.org/patch/462331/, I don't know how to do
> it.
>
I contributed to the confusion about this, let's see if we can clear it up.
An issue was found related to which enum values were used to represent
what late in the last release cycle. As a result a revert[1] patch for
everything touching bus-type in anx7625 was merged.
This patch, does not apply to drm-misc-next due to the revert, and if
Xin Ji rebases his work on the drm-misc-next there should be no
ABI-change as this patch when fixed up will introduce bus-type to the
nax7625 ABI.
Xin: Does this make sense to you?
[1]
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=979452fbc43028675b5a5da156f91928b739dea8
Re: [PATCH 9/9] vfio: Remove calls to vfio_group_add_container_user()
On Wed, Apr 13, 2022 at 08:11:05AM +0200, Christoph Hellwig wrote:
> On Tue, Apr 12, 2022 at 12:53:36PM -0300, Jason Gunthorpe wrote:
> > + if (WARN_ON(!READ_ONCE(vdev->open_count)))
> > + return -EINVAL;
>
> I think all the WARN_ON()s in this patch need to be WARN_ON_ONCE,
> otherwise there will be too many backtraces to be useful if a driver
> ever gets the API wrong.
Sure, I added a wrapper to make that have less overhead and merged it
with the other 'driver is calling this correctly' checks:
@@ -1330,6 +1330,12 @@ static int vfio_group_add_container_user(struct
vfio_group *group)
static const struct file_operations vfio_device_fops;
+/* true if the vfio_device has open_device() called but not close_device() */
+static bool vfio_assert_device_open(struct vfio_device *device)
+{
+ return !WARN_ON_ONCE(!READ_ONCE(device->open_count));
+}
+
static int vfio_group_get_device_fd(struct vfio_group *group, char *buf)
{
struct vfio_device *device;
@@ -1544,6 +1550,7 @@ static int vfio_device_fops_release(struct inode *inode,
struct file *filep)
struct vfio_device *device = filep->private_data;
mutex_lock(>dev_set->lock);
+ vfio_assert_device_open(device);
if (!--device->open_count && device->ops->close_device)
device->ops->close_device(device);
mutex_unlock(>dev_set->lock);
@@ -2112,7 +2119,7 @@ int vfio_pin_pages(struct vfio_device *vdev, unsigned
long *user_pfn, int npage,
struct vfio_iommu_driver *driver;
int ret;
- if (!user_pfn || !phys_pfn || !npage)
+ if (!user_pfn || !phys_pfn || !npage || !vfio_assert_device_open(vdev))
return -EINVAL;
if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
@@ -2121,9 +2128,6 @@ int vfio_pin_pages(struct vfio_device *vdev, unsigned
long *user_pfn, int npage,
if (group->dev_counter > 1)
return -EINVAL;
- if (WARN_ON(!READ_ONCE(vdev->open_count)))
- return -EINVAL;
-
container = group->container;
driver = container->iommu_driver;
if (likely(driver && driver->ops->pin_pages))
@@ -2153,15 +2157,12 @@ int vfio_unpin_pages(struct vfio_device *vdev, unsigned
long *user_pfn,
struct vfio_iommu_driver *driver;
int ret;
- if (!user_pfn || !npage)
+ if (!user_pfn || !npage || !vfio_assert_device_open(vdev))
return -EINVAL;
if (npage > VFIO_PIN_PAGES_MAX_ENTRIES)
return -E2BIG;
- if (WARN_ON(!READ_ONCE(vdev->open_count)))
- return -EINVAL;
-
container = vdev->group->container;
driver = container->iommu_driver;
if (likely(driver && driver->ops->unpin_pages))
@@ -2198,10 +2199,7 @@ int vfio_dma_rw(struct vfio_device *vdev, dma_addr_t
user_iova,
struct vfio_iommu_driver *driver;
int ret = 0;
- if (!data || len <= 0)
- return -EINVAL;
-
- if (WARN_ON(!READ_ONCE(vdev->open_count)))
+ if (!data || len <= 0 || !vfio_assert_device_open(vdev))
return -EINVAL;
container = vdev->group->container;
@@ -2294,10 +2292,7 @@ int vfio_register_notifier(struct vfio_device *dev, enum
vfio_notify_type type,
struct vfio_group *group = dev->group;
int ret;
- if (!nb || !events || (*events == 0))
- return -EINVAL;
-
- if (WARN_ON(!READ_ONCE(dev->open_count)))
+ if (!nb || !events || (*events == 0) || !vfio_assert_device_open(dev))
return -EINVAL;
switch (type) {
@@ -2321,10 +2316,7 @@ int vfio_unregister_notifier(struct vfio_device *dev,
struct vfio_group *group = dev->group;
int ret;
- if (!nb)
- return -EINVAL;
-
- if (WARN_ON(!READ_ONCE(dev->open_count)))
+ if (!nb || !vfio_assert_device_open(dev))
return -EINVAL;
switch (type) {
Thanks,
Jason
[PATCH] dt-bindings: display: panel-timing: Define a single type for properties
It's not good practice to define multiple types for the same property, so factor out the type reference making the properties always an uint32-array with a length of 1 or 3 items. Signed-off-by: Rob Herring --- .../bindings/display/panel/panel-timing.yaml | 42 --- 1 file changed, 18 insertions(+), 24 deletions(-) diff --git a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml index 9bf592dc3033..7749de95ee40 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml @@ -71,78 +71,72 @@ properties: hfront-porch: description: Horizontal front porch panel timing +$ref: /schemas/types.yaml#/definitions/uint32-array oneOf: - - $ref: /schemas/types.yaml#/definitions/uint32 -maxItems: 1 + - maxItems: 1 items: description: typical number of pixels - - $ref: /schemas/types.yaml#/definitions/uint32-array -minItems: 3 + - minItems: 3 maxItems: 3 items: description: min, typ, max number of pixels hback-porch: description: Horizontal back porch timing +$ref: /schemas/types.yaml#/definitions/uint32-array oneOf: - - $ref: /schemas/types.yaml#/definitions/uint32 -maxItems: 1 + - maxItems: 1 items: description: typical number of pixels - - $ref: /schemas/types.yaml#/definitions/uint32-array -minItems: 3 + - minItems: 3 maxItems: 3 items: description: min, typ, max number of pixels hsync-len: description: Horizontal sync length panel timing +$ref: /schemas/types.yaml#/definitions/uint32-array oneOf: - - $ref: /schemas/types.yaml#/definitions/uint32 -maxItems: 1 + - maxItems: 1 items: description: typical number of pixels - - $ref: /schemas/types.yaml#/definitions/uint32-array -minItems: 3 + - minItems: 3 maxItems: 3 items: description: min, typ, max number of pixels vfront-porch: description: Vertical front porch panel timing +$ref: /schemas/types.yaml#/definitions/uint32-array oneOf: - - $ref: /schemas/types.yaml#/definitions/uint32 -maxItems: 1 + - maxItems: 1 items: description: typical number of lines - - $ref: /schemas/types.yaml#/definitions/uint32-array -minItems: 3 + - minItems: 3 maxItems: 3 items: description: min, typ, max number of lines vback-porch: description: Vertical back porch panel timing +$ref: /schemas/types.yaml#/definitions/uint32-array oneOf: - - $ref: /schemas/types.yaml#/definitions/uint32 -maxItems: 1 + - maxItems: 1 items: description: typical number of lines - - $ref: /schemas/types.yaml#/definitions/uint32-array -minItems: 3 + - minItems: 3 maxItems: 3 items: description: min, typ, max number of lines vsync-len: description: Vertical sync length panel timing +$ref: /schemas/types.yaml#/definitions/uint32-array oneOf: - - $ref: /schemas/types.yaml#/definitions/uint32 -maxItems: 1 + - maxItems: 1 items: description: typical number of lines - - $ref: /schemas/types.yaml#/definitions/uint32-array -minItems: 3 + - minItems: 3 maxItems: 3 items: description: min, typ, max number of lines -- 2.32.0
Re: refactor the i915 GVT support and move to the modern mdev API v3
On 4/11/22 2:13 PM, Christoph Hellwig wrote: > Hi all, > > the GVT code in the i915 is a bit of a mess right now due to strange > abstractions and lots of indirect calls. This series refactors various > bits to clean that up. The main user visible change is that almost all > of the GVT code moves out of the main i915 driver and into the kvmgt > module. Hi Christoph: Do you want me to merge the GVT-g patches in this series? Or you want them to get merged from your side? Thanks, Zhi. > > Tested on my Thinkpad with a Kaby Lake CPU and integrated graphics. > > Git tree: > > git://git.infradead.org/users/hch/misc.git i915-gvt > > Gitweb: > > http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/i915-gvt > > Changes since v2: > - rebased on top of Linx 5.18-rc + >"Refactor GVT-g MMIO tracking table and handlers" > - don't fold the gvt Makefile into the main Makefile > - add the mdev patches to remove the legacy interface that is now >unused to the end of the series > > Changes since v1: > - rebased on Linux 5.15 > - allow the kvmgvt module to be loaded at any time and thus solve >the deadlock when both i915 amd kvmgvt are modular > - include the conversion to the modern mdev API > > Note that I do expect to rebased this again against 5.16-rc1 once > released, but I'd like to get this out for review ASAP. > > Diffstat: > b/drivers/gpu/drm/i915/Kconfig | 33 > b/drivers/gpu/drm/i915/Makefile | 31 > b/drivers/gpu/drm/i915/gvt/cfg_space.c | 89 -- > b/drivers/gpu/drm/i915/gvt/cmd_parser.c |4 > b/drivers/gpu/drm/i915/gvt/dmabuf.c | 36 - > b/drivers/gpu/drm/i915/gvt/execlist.c | 12 > b/drivers/gpu/drm/i915/gvt/gtt.c| 55 + > b/drivers/gpu/drm/i915/gvt/gvt.h| 125 ++- > b/drivers/gpu/drm/i915/gvt/interrupt.c | 38 + > b/drivers/gpu/drm/i915/gvt/kvmgt.c | 1099 > +++- > b/drivers/gpu/drm/i915/gvt/mmio.c |4 > b/drivers/gpu/drm/i915/gvt/opregion.c | 148 > b/drivers/gpu/drm/i915/gvt/page_track.c |8 > b/drivers/gpu/drm/i915/gvt/scheduler.c | 37 - > b/drivers/gpu/drm/i915/gvt/trace.h |2 > b/drivers/gpu/drm/i915/gvt/vgpu.c | 22 > b/drivers/gpu/drm/i915/i915_drv.c |7 > b/drivers/gpu/drm/i915/i915_drv.h |1 > b/drivers/gpu/drm/i915/i915_trace.h |1 > b/drivers/gpu/drm/i915/intel_gvt.c | 162 +++- > b/drivers/gpu/drm/i915/intel_gvt.h | 17 > drivers/gpu/drm/i915/gvt/Makefile |9 > drivers/gpu/drm/i915/gvt/gvt.c | 340 - > drivers/gpu/drm/i915/gvt/hypercall.h| 82 -- > drivers/gpu/drm/i915/gvt/mpt.h | 400 --- > 25 files changed, 929 insertions(+), 1833 deletions(-) >
Re: [PATCH 05/34] drm/i915/gvt: cleanup the Makefile
On Wed, Apr 13, 2022 at 01:39:35PM +, Wang, Zhi A wrote: > It seems Jani's makefile clean patch has already included this one, I can > just simply drop this one so that Christoph won't need to re-send everything. > > For the branch to move on, I am merging the patches and will re-generate the > gvt-staging branch, which combines the newest drm-tip vfio-upstream and other > gvt branches. > > If you are in a rush of re-basing the patches of non-GVT-g stuff, you can use > gvt-staging branch until my pull request landed in drm-intel-next. > > Also our QA will test gvt-staging-branch before the pull request. I suppose > it will take one or two days. When you are wrangling the branches it would be great if Christoph's series and it's minimal dependencies could be on a single branch that could reasonably be pulled to the VFIO tree too, thanks Jason
Re: [PATCH 4/9] drm/i915/gvt: Change from vfio_group_(un)pin_pages to vfio_(un)pin_pages
On Wed, Apr 13, 2022 at 08:01:10AM +0200, Christoph Hellwig wrote: > On Tue, Apr 12, 2022 at 12:53:31PM -0300, Jason Gunthorpe wrote: > > Use the existing vfio_device versions of vfio_(un)pin_pages(). There is no > > reason to use a group interface here, kvmgt has easy access to a > > vfio_device. > > Once this is moved after the vfio_dma_rw, this is the last user of > the vfio_group, and I think it owuld make sense to merge it with the > patch to remove the vfio_group instead of leaving that around once > unused. Done Thanks, Jason
Re: [PATCH 05/34] drm/i915/gvt: cleanup the Makefile
On 4/13/22 12:33 PM, Jani Nikula wrote: > On Mon, 11 Apr 2022, Christoph Hellwig wrote: >> On Mon, Apr 11, 2022 at 07:11:03PM +0300, Jani Nikula wrote: Up to you but I usually sort these lists >>> >>> Yeah, please do. Otherwise matches what I sent, so ack. >> >> Let's just merge your 2 patch series ASAP and I'll rebase on top of >> that. > > I rebased them on top of current gvt-next and resent [1]. Zhenyu, Zhi, > please pick them up if you approve. > >> What branch in drm-intel should I use as the base for the next version >> btw? Or does gvt go through yet another tree? > > It's yet another tree... Basically gvt is developed on top of gvt-next > (see MAINTAINERS) and Zhenyu and Zhi send pull requests for > drm-intel-next. > Hi Jani and Christoph: It seems Jani's makefile clean patch has already included this one, I can just simply drop this one so that Christoph won't need to re-send everything. For the branch to move on, I am merging the patches and will re-generate the gvt-staging branch, which combines the newest drm-tip vfio-upstream and other gvt branches. If you are in a rush of re-basing the patches of non-GVT-g stuff, you can use gvt-staging branch until my pull request landed in drm-intel-next. Also our QA will test gvt-staging-branch before the pull request. I suppose it will take one or two days. Again, thanks so much for making this happen. Thanks, Zhi. > > BR, > Jani. > > > [1] https://lore.kernel.org/all/cover.1649852517.git.jani.nik...@intel.com >
Re: [PATCH 5/9] vfio: Pass in a struct vfio_device * to vfio_dma_rw()
On Wed, Apr 13, 2022 at 08:00:08AM +0200, Christoph Hellwig wrote: > This looks good execept the extern nitpick: > > Reviewed-by: Christoph Hellwig > > However I'd move this before the previous patch. More of the explanation > there. Yes, that looks good, done Thanks, Jason
RE: [PATCHv4] drm/amdgpu: disable ASPM on Intel Alder Lake based systems
[Public]
> On Wed, Apr 13, 2022 at 3:43 AM Paul Menzel
> wrote:
> >
> > Dear Richard,
> >
> >
> > Thank you for sending out v4.
> >
> > Am 12.04.22 um 23:50 schrieb Richard Gong:
> > > Active State Power Management (ASPM) feature is enabled since kernel
> 5.14.
> > > There are some AMD GFX cards (such as WX3200 and RX640) that won't
> work
> > > with ASPM-enabled Intel Alder Lake based systems. Using these GFX
> cards as
> > > video/display output, Intel Alder Lake based systems will hang during
> > > suspend/resume.
> >
> > I am still not clear, what "hang during suspend/resume" means. I guess
> > suspending works fine? During resume (S3 or S0ix?), where does it hang?
> > The system is functional, but there are only display problems?
I believe Intel would need to identify the state of the SOC to determine where
the PCIE problem actually occurs; on the way down or up.
As he said in the commit message it results in a hang.
> >
> > > The issue was initially reported on one system (Dell Precision 3660 with
> > > BIOS version 0.14.81), but was later confirmed to affect at least 4 Alder
> > > Lake based systems.
> > >
> > > Add extra check to disable ASPM on Intel Alder Lake based systems.
> > >
> > > Fixes: 0064b0ce85bb ("drm/amd/pm: enable ASPM by default")
> > > Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitla
> b.freedesktop.org%2Fdrm%2Famd%2F-
> %2Fissues%2F1885data=04%7C01%7Cmario.limonciello%40amd.com%
> 7Cfe4b6b553c3b47c1288f08da1d4da9c8%7C3dd8961fe4884e608e11a82d994e
> 183d%7C0%7C0%7C637854516675116782%7CUnknown%7CTWFpbGZsb3d8ey
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7C3000sdata=gvFmP1HQP%2FyzLfT0gYYCupAQBIG%2FtiDYelQNqTLAx
> ck%3Dreserved=0
> > > Reported-by: kernel test robot
> >
> > This tag is a little confusing. Maybe clarify that it was for an issue
> > in a previous patch iteration?
> >
> > > Signed-off-by: Richard Gong
> > > ---
> > > v4: s/CONFIG_X86_64/CONFIG_X86
> > > enhanced check logic
> > > v3: s/intel_core_asom_chk/aspm_support_quirk_check
> > > correct build error with W=1 option
> > > v2: correct commit description
> > > move the check from chip family to problematic platform
> > > ---
> > > drivers/gpu/drm/amd/amdgpu/vi.c | 17 -
> > > 1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> b/drivers/gpu/drm/amd/amdgpu/vi.c
> > > index 039b90cdc3bc..b33e0a9bee65 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> > > @@ -81,6 +81,10 @@
> > > #include "mxgpu_vi.h"
> > > #include "amdgpu_dm.h"
> > >
> > > +#if IS_ENABLED(CONFIG_X86)
> > > +#include
> > > +#endif
> > > +
> > > #define ixPCIE_LC_L1_PM_SUBSTATE0x100100C6
> > > #define
> PCIE_LC_L1_PM_SUBSTATE__LC_L1_SUBSTATES_OVERRIDE_EN_MASK
> 0x0001L
> > > #define
> PCIE_LC_L1_PM_SUBSTATE__LC_PCI_PM_L1_2_OVERRIDE_MASK
> 0x0002L
> > > @@ -1134,13 +1138,24 @@ static void vi_enable_aspm(struct
> amdgpu_device *adev)
> > > WREG32_PCIE(ixPCIE_LC_CNTL, data);
> > > }
> > >
> > > +static bool aspm_support_quirk_check(void)
> > > +{
> > > + if (IS_ENABLED(CONFIG_X86)) {
> > > + struct cpuinfo_x86 *c = _data(0);
> > > +
> > > + return !(c->x86 == 6 && c->x86_model ==
> INTEL_FAM6_ALDERLAKE);
Don't you need to check x86_vendor? Although extremely unlikely if you don't
check x86_vendor nothing to stop another X86 manufacturer from having a
design that has same model # as INTEL_FAM6_ALDERLAKE.
> > > + }
> > > +
> > > + return true;
> > > +}
> > > +
> > > static void vi_program_aspm(struct amdgpu_device *adev)
> > > {
> > > u32 data, data1, orig;
> > > bool bL1SS = false;
> > > bool bClkReqSupport = true;
> > >
> > > - if (!amdgpu_device_should_use_aspm(adev))
> > > + if (!amdgpu_device_should_use_aspm(adev) ||
> !aspm_support_quirk_check())
> > > return;
> >
> > Can users still forcefully enable ASPM with the parameter `amdgpu.aspm`?
amdgpu.aspm is module wide not just for one card. That is it covers all AMD
GPU cards
in the system. If it's set to 1 or pcie_aspm_enabled returns true it will
enable for other
GPUs besides these.
There is the possibility to move this quirk check within "
amdgpu_device_should_use_aspm"
and only match this combination when set to amdgpu.aspm is set to "-1" (the
default), something
like this:
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1335,6 +1335,8 @@ bool amdgpu_device_should_use_aspm(struct amdgpu_device
*adev)
default:
return false;
}
+ if (amdgpu_device_is_quirked_for_aspm(adev))
+ return false;
return pcie_aspm_enabled(adev->pdev);
}
> >
> > >
> > > if (adev->flags & AMD_IS_APU ||
> >
> > If I remember correctly, there were also newer cards, where ASPM worked
>
Re: [PATCHv4] drm/amdgpu: disable ASPM on Intel Alder Lake based systems
On Wed, Apr 13, 2022 at 3:43 AM Paul Menzel wrote:
>
> Dear Richard,
>
>
> Thank you for sending out v4.
>
> Am 12.04.22 um 23:50 schrieb Richard Gong:
> > Active State Power Management (ASPM) feature is enabled since kernel 5.14.
> > There are some AMD GFX cards (such as WX3200 and RX640) that won't work
> > with ASPM-enabled Intel Alder Lake based systems. Using these GFX cards as
> > video/display output, Intel Alder Lake based systems will hang during
> > suspend/resume.
>
> I am still not clear, what “hang during suspend/resume” means. I guess
> suspending works fine? During resume (S3 or S0ix?), where does it hang?
> The system is functional, but there are only display problems?
>
> > The issue was initially reported on one system (Dell Precision 3660 with
> > BIOS version 0.14.81), but was later confirmed to affect at least 4 Alder
> > Lake based systems.
> >
> > Add extra check to disable ASPM on Intel Alder Lake based systems.
> >
> > Fixes: 0064b0ce85bb ("drm/amd/pm: enable ASPM by default")
> > Link: https://gitlab.freedesktop.org/drm/amd/-/issues/1885
> > Reported-by: kernel test robot
>
> This tag is a little confusing. Maybe clarify that it was for an issue
> in a previous patch iteration?
>
> > Signed-off-by: Richard Gong
> > ---
> > v4: s/CONFIG_X86_64/CONFIG_X86
> > enhanced check logic
> > v3: s/intel_core_asom_chk/aspm_support_quirk_check
> > correct build error with W=1 option
> > v2: correct commit description
> > move the check from chip family to problematic platform
> > ---
> > drivers/gpu/drm/amd/amdgpu/vi.c | 17 -
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/vi.c
> > b/drivers/gpu/drm/amd/amdgpu/vi.c
> > index 039b90cdc3bc..b33e0a9bee65 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/vi.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/vi.c
> > @@ -81,6 +81,10 @@
> > #include "mxgpu_vi.h"
> > #include "amdgpu_dm.h"
> >
> > +#if IS_ENABLED(CONFIG_X86)
> > +#include
> > +#endif
> > +
> > #define ixPCIE_LC_L1_PM_SUBSTATE0x100100C6
> > #define PCIE_LC_L1_PM_SUBSTATE__LC_L1_SUBSTATES_OVERRIDE_EN_MASK
> > 0x0001L
> > #define PCIE_LC_L1_PM_SUBSTATE__LC_PCI_PM_L1_2_OVERRIDE_MASK
> > 0x0002L
> > @@ -1134,13 +1138,24 @@ static void vi_enable_aspm(struct amdgpu_device
> > *adev)
> > WREG32_PCIE(ixPCIE_LC_CNTL, data);
> > }
> >
> > +static bool aspm_support_quirk_check(void)
> > +{
> > + if (IS_ENABLED(CONFIG_X86)) {
> > + struct cpuinfo_x86 *c = _data(0);
> > +
> > + return !(c->x86 == 6 && c->x86_model == INTEL_FAM6_ALDERLAKE);
> > + }
> > +
> > + return true;
> > +}
> > +
> > static void vi_program_aspm(struct amdgpu_device *adev)
> > {
> > u32 data, data1, orig;
> > bool bL1SS = false;
> > bool bClkReqSupport = true;
> >
> > - if (!amdgpu_device_should_use_aspm(adev))
> > + if (!amdgpu_device_should_use_aspm(adev) ||
> > !aspm_support_quirk_check())
> > return;
>
> Can users still forcefully enable ASPM with the parameter `amdgpu.aspm`?
>
> >
> > if (adev->flags & AMD_IS_APU ||
>
> If I remember correctly, there were also newer cards, where ASPM worked
> with Intel Alder Lake, right? Can only the problematic generations for
> WX3200 and RX640 be excluded from ASPM?
This patch only disables it for the generation that was problematic.
Alex
>
>
> Kind regards,
>
> Paul
Re: [PATCH 1/2] of: Create platform devices for OF framebuffers
On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann wrote:
>
> Create a platform device for each OF-declared framebuffer and have
> offb bind to these devices. Allows for real hot-unplugging and other
> drivers besides offb.
>
> Originally, offb created framebuffer devices while initializing its
> module by parsing the OF device tree. No actual Linux device was set
> up. This tied OF framebuffers to offb and makes writing other drivers
> for the OF framebuffers complicated. The absence of a Linux device
> prevented real hot-unplugging. Adding a distinct platform device for
> each OF framebuffer solves both problems. Specifically, a DRM drivers
> can now provide graphics output with modern userspace.
>
> Some of the offb init code is now located in the OF initialization.
> There's now also an implementation of of_platform_default_populate_init(),
> which was missing before. The OF side creates different devices for
> either OF display nodes or bootx displays as they require different
> handling by the driver. The offb drivers picks up each type of device
> and runs the appropriate fbdev initialization.
>
> Tested with OF display nodes on qemu's ppc64le target.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/of/platform.c | 73 ++--
> drivers/video/fbdev/offb.c | 98 +-
> 2 files changed, 134 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index a16b74f32aa9..4c63b9a73587 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root,
> }
> EXPORT_SYMBOL(of_platform_bus_probe);
>
> +static int __init of_platform_populate_framebuffers(void)
> +{
> + struct device_node *boot_display = NULL;
> + struct device_node *node;
> + struct platform_device *dev;
> + int ret;
> +
> + node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> + of_platform_device_create(node, NULL, NULL);
> + of_node_put(node);
> +
The rest is PPC only, so bail out here if !PPC.
> + /* Check if we have a MacOS display without a node spec */
> + if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
> + /*
> +* The old code tried to work out which node was the MacOS
> +* display based on the address. I'm dropping that since the
> +* lack of a node spec only happens with old BootX versions
> +* (users can update) and with this code, they'll still get
> +* a display (just not the palette hacks).
> +*/
> + dev = platform_device_alloc("bootx-noscreen", 0);
> + if (WARN_ON(!dev))
> + return -ENOMEM;
> + ret = platform_device_add(dev);
> + if (WARN_ON(ret)) {
> + platform_device_put(dev);
> + return ret;
> + }
> + }
> +
> + /*
> +* For OF framebuffers, first create the device for the boot display,
> +* then for the other framebuffers. Only fail for the boot display;
> +* ignore errors for the rest.
> +*/
> + for_each_node_by_type(node, "display") {
> + if (!of_get_property(node, "linux,opened", NULL) ||
> + !of_get_property(node, "linux,boot-display", NULL))
> + continue;
> + dev = of_platform_device_create(node, "of-display", NULL);
> + if (WARN_ON(!dev))
> + return -ENOMEM;
> + boot_display = node;
> + break;
> + }
> + for_each_node_by_type(node, "display") {
> + if (!of_get_property(node, "linux,opened", NULL) || node ==
> boot_display)
> + continue;
> + of_platform_device_create(node, "of-display", NULL);
> + }
> +
> + return 0;
> +}
> +
> /**
> * of_platform_populate() - Populate platform_devices from device tree data
> * @root: parent of the first level to probe or NULL for the root of the tree
> @@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void)
> of_node_put(node);
> }
>
> - node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> - of_platform_device_create(node, NULL, NULL);
> - of_node_put(node);
> + of_platform_populate_framebuffers();
>
> /* Populate everything else. */
> of_platform_default_populate(NULL, NULL, NULL);
I'm pretty sure it's just this call that's the problem for PPC though
none of the above existed when adding this caused a regression. Can we
remove the ifdef and just make this call conditional on
!IS_ENABLED(CONFIG_PPC).
> @@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void)
> return 0;
> }
>
Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On Wed, 2022-04-13 at 16:19 +0530, Vinod Koul wrote: > On 13-04-22, 18:04, Liu Ying wrote: > > Hi Vinod, > > > > On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote: > > > On 02-04-22, 13:24, Liu Ying wrote: > > > > This patch allows LVDS PHYs to be configured through > > > > the generic functions and through a custom structure > > > > added to the generic union. > > > > > > > > The parameters added here are based on common LVDS PHY > > > > implementation practices. The set of parameters > > > > should cover all potential users. > > > > > > > > Cc: Kishon Vijay Abraham I > > > > Cc: Vinod Koul > > > > Cc: NXP Linux Team > > > > Signed-off-by: Liu Ying > > > > --- [...] > > > > + */ > > > > + > > > > +#ifndef __PHY_LVDS_H_ > > > > +#define __PHY_LVDS_H_ > > > > + > > > > +/** > > > > + * struct phy_configure_opts_lvds - LVDS configuration set > > > > + * @bits_per_lane_and_dclk_cycle: Number of bits per data > > > > lane > > > > and > > > > + * differential clock > > > > cycle. > > > > > > What does it mean by bits per data lane and differential clock > > > cycle? > > > > Please check > > Documentation/devicetree/bindings/display/panel/lvds.yaml. > > lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the > > number of slots. But, I don't find the word 'slot' in my lvds > > relevant > > specs which mentioned in lvds.yaml, so 'slots' is probably not a > > generic name(lvds.yaml is for display panel). So, I use > > 'bits_per_lane_and_dclk_cycle' as the name tells what it means. > > variable name is fine, explanation for bit per lane and differential > clock cycle didnt help, maybe add better explanation of what this > variable means I may add an example diagram as below... > > > > > > > > > > + * @differential_clk_rate: Clock rate, in Hertz, > > > > of the > > > > LVDS > > > > + * differential clock. > > > > + * @lanes: Number of active, > > > > consecutive, > > > > + * data lanes, starting > > > > from lane > > > > 0, > > > > + * used for the > > > > transmissions. > > > > + * @is_slave: Boolean, true if the > > > > phy is a slave > > > > + * which works together > > > > with a > > > > master > > > > + * phy to support dual > > > > link > > > > transmission, > > > > + * otherwise a regular phy > > > > or a > > > > master phy. -8< + * This is an example with 4 lanes and 7 bits per data lane and differential + * clock cycle: + * + * :: + * |<- one differential clock cycle ->| + * _ + * Clock \___/ + * __ __ __ __ __ __ __ + * Lane0 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< + * Lane1 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< + * Lane2 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< + * Lane3 ><_bit0_><_bit1_><_bit2_><_bit3_><_bit4_><_bit5_><_bit 6_>< -8< What do you think? Regards, Liu Ying
Re: [PATCH 05/34] drm/i915/gvt: cleanup the Makefile
On Mon, 11 Apr 2022, Christoph Hellwig wrote: > On Mon, Apr 11, 2022 at 07:11:03PM +0300, Jani Nikula wrote: >> > Up to you but I usually sort these lists >> >> Yeah, please do. Otherwise matches what I sent, so ack. > > Let's just merge your 2 patch series ASAP and I'll rebase on top of > that. I rebased them on top of current gvt-next and resent [1]. Zhenyu, Zhi, please pick them up if you approve. > What branch in drm-intel should I use as the base for the next version > btw? Or does gvt go through yet another tree? It's yet another tree... Basically gvt is developed on top of gvt-next (see MAINTAINERS) and Zhenyu and Zhi send pull requests for drm-intel-next. BR, Jani. [1] https://lore.kernel.org/all/cover.1649852517.git.jani.nik...@intel.com -- Jani Nikula, Intel Open Source Graphics Center
Re: [bug report] drm/ttm: Add a generic TTM memcpy move for page-based iomem
Hello Dan Carpenter.
Thanks for the report.
On 4/13/22 13:11, Dan Carpenter wrote:
Hello Thomas Hellström,
The patch 3bf3710e3718: "drm/ttm: Add a generic TTM memcpy move for
page-based iomem" from Jun 2, 2021, leads to the following Smatch
static checker warning:
./include/drm/ttm/ttm_bo_driver.h:259 ttm_bo_move_sync_cleanup()
error: NULL dereference inside function 'ttm_bo_move_accel_cleanup()'
./include/drm/ttm/ttm_bo_driver.h
256 static inline void ttm_bo_move_sync_cleanup(struct ttm_buffer_object
*bo,
257 struct ttm_resource
*new_mem)
258 {
--> 259 int ret = ttm_bo_move_accel_cleanup(bo, NULL, true, false,
new_mem);
Passing a NULL for "fence" will crash. The first place where it will
crash is in dma_resv_add_fence() where it does:
Indeed, and this has been discussed thoroughly on dri-devel lately. The
bug was introduced in a recent patch that made NULL pointers here crash.
Not the patch indicated.
Thanks,
Thomas
WARN_ON(dma_fence_is_container(fence));
260
261 WARN_ON(ret);
262 }
regards,
dan carpenter
Re: [PATCH 3/9] vfio/mdev: Pass in a struct vfio_device * to vfio_pin/unpin_pages()
On Wed, Apr 13, 2022 at 07:57:17AM +0200, Christoph Hellwig wrote: > > - extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, > > + extern int vfio_pin_pages(struct vfio_device *vdev, unsigned long > > *user_pfn, > > int npage, int prot, unsigned long *phys_pfn); > > > > - extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, > > + extern int vfio_unpin_pages(struct vfio_device *vdev, unsigned long > > *user_pfn, > > Please drop the externs when you touch this (also for the actual > header). Alex has been asking to keep them in the H files for consistency Removing from the docs should be fine though Thanks, Jason
Re: [PATCH 1/9] vfio: Make vfio_(un)register_notifier accept a vfio_device
On Wed, Apr 13, 2022 at 07:55:24AM +0200, Christoph Hellwig wrote: > On Tue, Apr 12, 2022 at 12:53:28PM -0300, Jason Gunthorpe wrote: > > All callers have a struct vfio_device trivially available, pass it in > > directly and avoid calling the expensive vfio_group_get_from_dev(). > > Instead of bothering the drivers with the notifiers at all, the two > notifier_blocks should move into struct vfio_device, and the > vfio_ops should just grow two new dma_unmap and set_kvm methods. > > This will isolate the drives from the whole notifiers mess and it's > boilerplate code. I already looked into that for a while, it is a real mess too because of how the notifiers are used by the current drivers, eg gvt assumes the notifier is called during its open_device callback to setup its kvm. The unmap is less convoluted and I might still try to do that.. For this series I prefer to leave it alone Jason
Re: [PATCH] drm/atomic-helpers: remove legacy_cursor_update hacks
On Wed, 13 Apr 2022 at 01:36, Abhinav Kumar wrote:
>
> Hi Daniel
>
> On 4/8/2022 9:04 PM, Abhinav Kumar wrote:
> >
> >
> > On 4/7/2022 4:12 PM, Rob Clark wrote:
> >> On Thu, Apr 7, 2022 at 3:59 PM Abhinav Kumar
> >> wrote:
> >>>
> >>> Hi Rob and Daniel
> >>>
> >>> On 4/7/2022 3:51 PM, Rob Clark wrote:
> On Wed, Apr 6, 2022 at 6:27 PM Jessica Zhang
> wrote:
> >
> >
> >
> > On 3/31/2022 8:20 AM, Daniel Vetter wrote:
> >> The stuff never really worked, and leads to lots of fun because it
> >> out-of-order frees atomic states. Which upsets KASAN, among other
> >> things.
> >>
> >> For async updates we now have a more solid solution with the
> >> ->atomic_async_check and ->atomic_async_commit hooks. Support for
> >> that
> >> for msm and vc4 landed. nouveau and i915 have their own commit
> >> routines, doing something similar.
> >>
> >> For everyone else it's probably better to remove the use-after-free
> >> bug, and encourage folks to use the async support instead. The
> >> affected drivers which register a legacy cursor plane and don't
> >> either
> >> use the new async stuff or their own commit routine are: amdgpu,
> >> atmel, mediatek, qxl, rockchip, sti, sun4i, tegra, virtio, and
> >> vmwgfx.
> >>
> >> Inspired by an amdgpu bug report.
> >>
> >> v2: Drop RFC, I think with amdgpu converted over to use
> >> atomic_async_check/commit done in
> >>
> >> commit 674e78acae0dfb4beb56132e41cbae5b60f7d662
> >> Author: Nicholas Kazlauskas
> >> Date: Wed Dec 5 14:59:07 2018 -0500
> >>
> >>drm/amd/display: Add fast path for cursor plane updates
> >>
> >> we don't have any driver anymore where we have userspace expecting
> >> solid legacy cursor support _and_ they are using the atomic
> >> helpers in
> >> their fully glory. So we can retire this.
> >>
> >> v3: Paper over msm and i915 regression. The complete_all is the only
> >> thing missing afaict.
> >>
> >> v4: Fixup i915 fixup ...
> >>
> >> References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
> >> References:
> >> https://lore.kernel.org/all/20220221134155.125447-9-max...@cerno.tech/
> >>
> >> References: https://bugzilla.kernel.org/show_bug.cgi?id=199425
> >> Cc: Maxime Ripard
> >> Tested-by: Maxime Ripard
> >> Cc: mikita.lip...@amd.com
> >> Cc: Michel Dänzer
> >> Cc: harry.wentl...@amd.com
> >> Cc: Rob Clark
> >
> > Hey Rob,
> >
> > I saw your tested-by and reviewed-by tags on Patchwork. Just curious,
> > what device did you test on?
>
> I was testing on strongbad.. v5.18-rc1 + patches (notably, revert
> 80253168dbfd ("drm: of: Lookup if child node has panel or bridge")
>
> I think the display setup shouldn't be significantly different than
> limozeen (ie. it's an eDP panel). But I didn't do much start/stop
> ui.. I was mostly looking to make sure cursor movements weren't
> causing fps drops ;-)
>
> BR,
> -R
> >>>
> >>> start ui/ stop ui is a basic operation for us to use IGT on msm-next.
> >>> So we cannot let that break.
> >>>
> >>> I think we need to check whats causing this splat.
> >>>
> >>> Can we hold back this change till then?
> >>
> >> Can you reproduce on v5.18-rc1 (plus 80253168dbfd)? I'm running a
> >> loop of stop ui / start ui and hasn't triggered a splat yet.
> >>
> >> Otherwise maybe you can addr2line to figure out where it crashed?
> >>
> >> BR,
> >> -R
> >
> > So this is not a crash. Its a warning splat coming from
> >
> > https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c#L785
> >
> >
> > Looks like the complete_commit() which should signal the event has not
> > happened before the next cursor commit.
> >
> > Somehow this change is affecting the flow to miss the event signaling
> > that the event is done.
> >
> > We tried a couple of approaches but couldnt still fix the warning.
> >
> > Will continue to check further next week.
> >
> >>
> >>> Thanks
> >>>
> >>> Abhinav
>
> After checking this more this week, I think the current patch needs to
> be changed a bit.
>
> So, here you are removing the complete_all part and leaving that to the
> individual drivers, which is fine.
>
> But, you are also removing the continue part which I think seems
> incorrect and causing these warnings for MSM driver.
>
> @@ -2135,12 +2128,6 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state,
> continue;
> }
>
> - /* Legacy cursor updates are fully unsynced. */
> - if (state->legacy_cursor_update) {
> - complete_all(>flip_done);
> - continue;
> - }
> -
>
> Thats because MSM driver thinks that if the previous crtc_state->event
> was not consumed, then
[bug report] drm/ttm: Add a generic TTM memcpy move for page-based iomem
Hello Thomas Hellström,
The patch 3bf3710e3718: "drm/ttm: Add a generic TTM memcpy move for
page-based iomem" from Jun 2, 2021, leads to the following Smatch
static checker warning:
./include/drm/ttm/ttm_bo_driver.h:259 ttm_bo_move_sync_cleanup()
error: NULL dereference inside function 'ttm_bo_move_accel_cleanup()'
./include/drm/ttm/ttm_bo_driver.h
256 static inline void ttm_bo_move_sync_cleanup(struct ttm_buffer_object
*bo,
257 struct ttm_resource
*new_mem)
258 {
--> 259 int ret = ttm_bo_move_accel_cleanup(bo, NULL, true, false,
new_mem);
Passing a NULL for "fence" will crash. The first place where it will
crash is in dma_resv_add_fence() where it does:
WARN_ON(dma_fence_is_container(fence));
260
261 WARN_ON(ret);
262 }
regards,
dan carpenter
Re: [PATCH v3 4/5] drm/solomon: Move device info from ssd130x-i2c to the core driver
Hello Andy, On 4/13/22 12:46, Andy Shevchenko wrote: > On Tue, Apr 12, 2022 at 06:27:28PM +0200, Javier Martinez Canillas wrote: >> These are declared in the ssd130x-i2c transport driver but the information >> is not I2C specific, and could be used by other SSD130x transport drivers. >> >> Move them to the ssd130x core driver and just set the OF device entries to >> an ID that could be used to lookup the correct device info from an array. >> >> While being there, also move the SSD130X_DATA and SSD130X_COMMAND control >> bytes. Since even though they are used by the I2C interface, they could >> also be useful for other transport protocols such as SPI. > > ... > >> +EXPORT_SYMBOL_GPL(ssd130x_variants); > > What I meant is to use EXPORT_SYMBOL_NS_GPL() here. It might require a > separate > patch to move other exports to that namespace first. > Oh, I wasn't aware of the namespace aware variant of these. Thanks for pointing it out! I'll change and use that one instead for v4. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 2/2] fbdev: Remove hot-unplug workaround for framebuffers without device
On 4/13/22 11:24, Thomas Zimmermann wrote:
> A workaround makes fbdev hot-unplugging work for framebuffers without
> device. The only user for this feature was offb. As each OF framebuffer
> now has an associated platform device, the workaround is no longer
> needed. Remove it. Effectively reverts commit 0f525289ff0d ("fbdev: Fix
> unregistering of framebuffers without device").
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/video/fbdev/core/fbmem.c | 9 +
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fbmem.c
> b/drivers/video/fbdev/core/fbmem.c
> index bc6ed750e915..bdd00d381bbc 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -1579,14 +1579,7 @@ static void do_remove_conflicting_framebuffers(struct
> apertures_struct *a,
>* If it's not a platform device, at least print a
> warning. A
>* fix would add code to remove the device from the
> system.
>*/
> - if (!device) {
> - /* TODO: Represent each OF framebuffer as its
> own
> - * device in the device hierarchy. For now, offb
> - * doesn't have such a device, so unregister the
> - * framebuffer as before without warning.
> - */
> - do_unregister_framebuffer(registered_fb[i]);
Maybe we could still keep this for a couple of releases but with a big
warning that's not supported in case there are out-of-tree drivers out
there that still do this ?
Or at least a warning if the do_unregister_framebuffer() call is removed.
Regardless of what you chose to do, the patch looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
Re: [PATCH 1/2] of: Create platform devices for OF framebuffers
Hi
Am 13.04.22 um 12:45 schrieb Javier Martinez Canillas:
Hello Thomas,
Thanks for working on this.
On 4/13/22 11:24, Thomas Zimmermann wrote:
Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.
Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
prevented real hot-unplugging. Adding a distinct platform device for
each OF framebuffer solves both problems. Specifically, a DRM drivers
can now provide graphics output with modern userspace.
Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or bootx displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.
Tested with OF display nodes on qemu's ppc64le target.
Signed-off-by: Thomas Zimmermann
---
[snip]
+ for_each_node_by_type(node, "display") {
+ if (!of_get_property(node, "linux,opened", NULL) ||
+ !of_get_property(node, "linux,boot-display", NULL))
+ continue;
+ dev = of_platform_device_create(node, "of-display", NULL);
+ if (WARN_ON(!dev))
+ return -ENOMEM;
+ boot_display = node;
+ break;
+ }
+ for_each_node_by_type(node, "display") {
+ if (!of_get_property(node, "linux,opened", NULL) || node ==
boot_display)
+ continue;
+ of_platform_device_create(node, "of-display", NULL);
Shouldn't check for the return value here too ?
Failing is probably not useful, as it's not the main FB used for
booting. Printing an error message wouldn't hurt, I guess. I'll also
update the new driver registration in offb with an error message.
Other than this small nit, it looks good to me.
Reviewed-by: Javier Martinez Canillas
Thanks.
Best regards
Thomas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v3 4/5] drm/solomon: Move device info from ssd130x-i2c to the core driver
On Tue, Apr 12, 2022 at 06:27:28PM +0200, Javier Martinez Canillas wrote: > These are declared in the ssd130x-i2c transport driver but the information > is not I2C specific, and could be used by other SSD130x transport drivers. > > Move them to the ssd130x core driver and just set the OF device entries to > an ID that could be used to lookup the correct device info from an array. > > While being there, also move the SSD130X_DATA and SSD130X_COMMAND control > bytes. Since even though they are used by the I2C interface, they could > also be useful for other transport protocols such as SPI. ... > +EXPORT_SYMBOL_GPL(ssd130x_variants); What I meant is to use EXPORT_SYMBOL_NS_GPL() here. It might require a separate patch to move other exports to that namespace first. -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 resend 2/5] phy: Add LVDS configuration options
On 13-04-22, 18:04, Liu Ying wrote:
> Hi Vinod,
>
> On Wed, 2022-04-13 at 11:41 +0530, Vinod Koul wrote:
> > On 02-04-22, 13:24, Liu Ying wrote:
> > > This patch allows LVDS PHYs to be configured through
> > > the generic functions and through a custom structure
> > > added to the generic union.
> > >
> > > The parameters added here are based on common LVDS PHY
> > > implementation practices. The set of parameters
> > > should cover all potential users.
> > >
> > > Cc: Kishon Vijay Abraham I
> > > Cc: Vinod Koul
> > > Cc: NXP Linux Team
> > > Signed-off-by: Liu Ying
> > > ---
> > > v5->v6:
> > > * Rebase upon v5.17-rc1.
> > >
> > > v4->v5:
> > > * Align kernel-doc style to include/linux/phy/phy.h. (Vinod)
> > > * Trivial tweaks.
> > > * Drop Robert's R-b tag.
> > >
> > > v3->v4:
> > > * Add Robert's R-b tag.
> > >
> > > v2->v3:
> > > * No change.
> > >
> > > v1->v2:
> > > * No change.
> > >
> > > include/linux/phy/phy-lvds.h | 32
> > > include/linux/phy/phy.h | 4
> > > 2 files changed, 36 insertions(+)
> > > create mode 100644 include/linux/phy/phy-lvds.h
> > >
> > > diff --git a/include/linux/phy/phy-lvds.h b/include/linux/phy/phy-
> > > lvds.h
> > > new file mode 100644
> > > index ..7a2f4747f624
> > > --- /dev/null
> > > +++ b/include/linux/phy/phy-lvds.h
> > > @@ -0,0 +1,32 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright 2020 NXP
> >
> > 2022 now
>
> I may change it to 'Copyright 2020,2022 NXP'.
>
> >
> > > + */
> > > +
> > > +#ifndef __PHY_LVDS_H_
> > > +#define __PHY_LVDS_H_
> > > +
> > > +/**
> > > + * struct phy_configure_opts_lvds - LVDS configuration set
> > > + * @bits_per_lane_and_dclk_cycle:Number of bits per data lane
> > > and
> > > + * differential clock cycle.
> >
> > What does it mean by bits per data lane and differential clock cycle?
>
> Please check Documentation/devicetree/bindings/display/panel/lvds.yaml.
> lvds.yaml metions slot. 'bits_per_lane_and_dclk_cycle' means the
> number of slots. But, I don't find the word 'slot' in my lvds relevant
> specs which mentioned in lvds.yaml, so 'slots' is probably not a
> generic name(lvds.yaml is for display panel). So, I use
> 'bits_per_lane_and_dclk_cycle' as the name tells what it means.
variable name is fine, explanation for bit per lane and differential
clock cycle didnt help, maybe add better explanation of what this
variable means
>
> >
> > > + * @differential_clk_rate: Clock rate, in Hertz, of the
> > > LVDS
> > > + * differential clock.
> > > + * @lanes: Number of active, consecutive,
> > > + * data lanes, starting from lane
> > > 0,
> > > + * used for the transmissions.
> > > + * @is_slave:Boolean, true if the
> > > phy is a slave
> > > + * which works together with a
> > > master
> > > + * phy to support dual link
> > > transmission,
> > > + * otherwise a regular phy or a
> > > master phy.
> > > + *
> > > + * This structure is used to represent the configuration state of
> > > a LVDS phy.
> > > + */
> > > +struct phy_configure_opts_lvds {
> > > + unsigned intbits_per_lane_and_dclk_cycle;
> > > + unsigned long differential_clk_rate;
> > > + unsigned intlanes;
> > > + boolis_slave;
> > > +};
> >
> > Where is the user of this new configuration? Can you post that patch
> > for
> > reference as well please
>
> Patch 5/5 uses it. Also, I posted two drm bridge drivers[1][2] which
> use it.
That is phy driver not the user
>
> [1]
> https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-13-git-send-email-victor@nxp.com/
> [2]
> https://patchwork.kernel.org/project/dri-devel/patch/1617172405-12962-14-git-send-email-victor@nxp.com/
this is helpful, thanks
--
~Vinod
Re: [PATCH 1/2] of: Create platform devices for OF framebuffers
Hello Thomas,
Thanks for working on this.
On 4/13/22 11:24, Thomas Zimmermann wrote:
> Create a platform device for each OF-declared framebuffer and have
> offb bind to these devices. Allows for real hot-unplugging and other
> drivers besides offb.
>
> Originally, offb created framebuffer devices while initializing its
> module by parsing the OF device tree. No actual Linux device was set
> up. This tied OF framebuffers to offb and makes writing other drivers
> for the OF framebuffers complicated. The absence of a Linux device
> prevented real hot-unplugging. Adding a distinct platform device for
> each OF framebuffer solves both problems. Specifically, a DRM drivers
> can now provide graphics output with modern userspace.
>
> Some of the offb init code is now located in the OF initialization.
> There's now also an implementation of of_platform_default_populate_init(),
> which was missing before. The OF side creates different devices for
> either OF display nodes or bootx displays as they require different
> handling by the driver. The offb drivers picks up each type of device
> and runs the appropriate fbdev initialization.
>
> Tested with OF display nodes on qemu's ppc64le target.
>
> Signed-off-by: Thomas Zimmermann
> ---
[snip]
> + for_each_node_by_type(node, "display") {
> + if (!of_get_property(node, "linux,opened", NULL) ||
> + !of_get_property(node, "linux,boot-display", NULL))
> + continue;
> + dev = of_platform_device_create(node, "of-display", NULL);
> + if (WARN_ON(!dev))
> + return -ENOMEM;
> + boot_display = node;
> + break;
> + }
> + for_each_node_by_type(node, "display") {
> + if (!of_get_property(node, "linux,opened", NULL) || node ==
> boot_display)
> + continue;
> + of_platform_device_create(node, "of-display", NULL);
Shouldn't check for the return value here too ?
Other than this small nit, it looks good to me.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Linux Engineering
Red Hat
