Re: [PATCH v1 0/11] drm/via: Make via a single file driver
Hi Sam, this looks like a good solution to me. I'd give you a detailed review, but dri-devel decided to only send me the color letter. I had to use patchwork get the patches. For the series Acked-by: Thomas Zimmermann with suggestions below. Am 10.07.22 um 10:54 schrieb Sam Ravnborg: We have an upcoming openchrome driver for VIA where some of the files conflicts with the existing via driver. It is not acceptable to just delete the existing DRI1 based driver as there are most likely users out there, so a different approach was required. It was disccussed what to do and the least invasive solution was to keep the DRI1 driver in the current directory as a single file. Thomas Zimmermann already posted a patch to do the same but it attemped to have a single driver for the DRI1 and the upcoming new driver. After the openchrome patches land, there will be an option in Kconfig to build the old driver instead of of the new one? This patchset embeds the files one by one to make the diffs remotely readable and end up with an independent DRI1 driver. The driver was built tested for each step. The driver is in the end renamed to via_dri1. Some feedback on this would be good as I do not know what impact it will have on users. I don't know how Mesa decides about which userspace driver to load, but It seems related to the name of the kernel driver. Renaming the module might interfere somehow. But if the old and new driver are mutually exclusive at build time, it should not be a problem to use the same name for both modules. The very last patch synchronize the via_3d_reg header file with the one used in the openchrome driver. This was added to verify that the new header would not break the DRI1 driver. Some of the macros introduce line wraps. I don't know if you did that intentionally, but it shouldn't be necessary. We have a 100-character limit per line. Maybe you can also add an extra patch that adds SPDX license tags at the top of the files. Best regard Thomas Sam Sam Ravnborg (11): drm/via: Rename via_drv to via_dri1 drm/via: Embed via_dma in via_dri1 drm/via: Embed via_map in via_dri1 drm/via: Embed via_mm in via_dri1 drm/via: Embed via_video in via_dri1 drm/via: Embed via_irq in via_dri1 drm/via: Embed via_dmablit in via_dri1 drm/via: Embed via_verifier in via_dri1 drm/via: Embed via_drv.h in via_dri1 drm/via: Rename the via driver to via_dri1 drm/via: Update to the latest via_3d_reg file drivers/gpu/drm/via/Makefile |4 +- drivers/gpu/drm/via/via_3d_reg.h | 395 +++- drivers/gpu/drm/via/via_dma.c | 744 drivers/gpu/drm/via/via_dmablit.c | 807 drivers/gpu/drm/via/via_dmablit.h | 140 -- drivers/gpu/drm/via/via_dri1.c | 3630 drivers/gpu/drm/via/via_drv.c | 124 -- drivers/gpu/drm/via/via_drv.h | 229 --- drivers/gpu/drm/via/via_irq.c | 388 drivers/gpu/drm/via/via_map.c | 132 -- drivers/gpu/drm/via/via_mm.c | 241 --- drivers/gpu/drm/via/via_verifier.c | 1110 --- drivers/gpu/drm/via/via_verifier.h | 62 - drivers/gpu/drm/via/via_video.c| 94 - 14 files changed, 3935 insertions(+), 4165 deletions(-) -- 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: drm-tip amdgpu not compiling, possible fix attached
Hi Am 10.07.22 um 22:01 schrieb Hans de Goede: Hi All, I've been rebasing my backlight refactor on top of drm-tip to submit a new version upstream and I noticed that drm-tip does not compile. This is caused by a mismerge of: commit 925b6e59138cefa47275c67891c65d48d3266d57 (drm-misc/for-linux-next-fixes, drm-misc/drm-misc-fixes, drm-misc-fixes)Selvam Date: Fri Jul 8 02:30:47 2022 -0700 Revert "drm/amdgpu: add drm buddy support to amdgpu" This reverts commit c9cad937c0c58618fe5b0310fd539a854dc1ae95. This is part of a revert of the following commits: commit 708d19d9f362 ("drm/amdgpu: move internal vram_mgr function into the C file") commit 5e3f1e7729ec ("drm/amdgpu: fix start calculation in amdgpu_vram_mgr_new") commit c9cad937c0c5 ("drm/amdgpu: add drm buddy support to amdgpu") [WHY] Few users reported garbaged graphics as soon as x starts, reverting until this can be resolved. Signed-off-by: Arunpravin Paneer Selvam From drm-misc/for-linux-next-fixes / drm-misc-fixes The attached patch on top of drm-tip fixes the mismerge. Note compile-tested only! Thank you for the workaround. I ran into the same problem yesterday after synchronizing with drm-tip. amdgpu doesn't build on drm-tip. Best regards Thomas If someone with more rerere experience then me can use this to fix things that would be great. Regards, Hans -- 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 02/14] drm/mgag200: Move DAC-register setup into model-specific code
Hi Sam Am 08.07.22 um 17:54 schrieb Sam Ravnborg: Hi Thomas, On Fri, Jul 08, 2022 at 11:39:17AM +0200, Thomas Zimmermann wrote: Provide an init function for each model's DAC registers. Remove the shared helper. The code for initializing the DAC registers consisted of a large table of default value, plus many exceptions for the various G200 models. Providing a per-model implementation makes if more readable. The suggested implementation introduces a lot of duplications which is not nice and the increase in readability is low as it continues to use a lot of direct value rather than referring to register index values. In some places it replaces nice register index values with hardcoded constants. I typed up something that I see as easier to read - but it is also much more verbose. I noticed that for example index 0x1c has the value 0xBF - but the value is never used. With the scheme below we would only assign values to registers in use. This would be obvious in the scheme below. I suggest to keep the current implementation as it is easier to read what happens with the registers, or go for a more verbose version where there is no hard coded index to registers like for example outlined below. The current code is not maintainable or even readable. That for loop with the branches isn't funny anymore. It needs to be untangled into per-model code in some way. Sam struct dac_data { u8 data; bool use; /* If }; All the registers that has the same values for all models: static void mgag200_default_regs(struct dac_data *dac) { memset(dac, 0, MGA200_REGS);// New constat equal 0x50 dac[MGA1064_VREF_CTL].use = true; dac[MGA1064_VREF_CTL].data = 0; dac[MGA1064_MUL_CTL].use = true; dac[MGA1064_MUL_CTL].data = 0; dac[MGA1064_PIX_CLK_CTL].use = true; dac[MGA1064_PIX_CLK_CTL] = 0xC9; And so on. Much more verbose than a table, but also more readable. A generic write function: void mgag200_write_dac(struct dac_data *dac) { int i; for (i = 0; i < MGA200_REGS) { if (dac[i].use]) WREG_DAC(i, dac[i]); } } Then for each model something like void mgag200_g200eh_init_registers(struct mga_device *mdev) { struct dac_data dac[MGA200_REGS]; mgag200_default_regs(&dac]; dac[MGA1064_MISC_CTL].use = true; dac[MGA1064_MISC_CTL].data = MGA1064_MISC_CTL_VGA8 | /* G200EH specific: MGA1064_MISC_CTL */ MGA1064_MISC_CTL_DAC_RAM_CS; /* No PIX_PLL setup */ -- The logic should be reversed I hink, so the ones that need PLL setup set to true. But the idea is obvious so I did not redo it dac[MGA1064_PIX_PLLA_M].use = false; dac[MGA1064_PIX_PLLA_N].use = false; dac[MGA1064_PIX_PLLA_P].use = false; dac[MGA1064_PIX_PLLB_M].use = false; dac[MGA1064_PIX_PLLB_N].use = false; dac[MGA1064_PIX_PLLB_P].use = false; dac[MGA1064_PIX_PLLC_M].use = false; dac[MGA1064_PIX_PLLC_N].use = false; dac[MGA1064_PIX_PLLC_P].use = false; mgag200_write_dac(&dac); } IMHO that's not going to work well. Algorithms are much more error prone than data structures. Moving program expressions from a data structure into an algorithm is rarely useful. (Exceptions exist, of course.) I think, the dupliction of DAC values can be solved by an intializer macro. MGAG200_DEFAULT_DAC_VALUES(...) = \ ... and the macro parameter are those values that change among the different models. There aren't too many. It won't solve the for-loop for now, but it's a start. In principle, these init_register() functions shouldn't exist at all. The initialization of these registers should be done in the CRTC's and plane's reset callbacks. [1] Most would then again be possible for share a good part if the register init among models. For the PLL, a per-model PLL-reset callback can be used. All that is, however, enough change for another complete patchset. Best regards Thomas [1] https://elixir.bootlin.com/linux/latest/source/include/drm/drm_crtc.h#L431 At some point, some of the initialization should probably move into the modesetting code. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/mgag200/mgag200_drv.h | 3 + drivers/gpu/drm/mgag200/mgag200_g200.c| 37 +++ drivers/gpu/drm/mgag200/mgag200_g200eh.c | 36 ++ drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 2 + drivers/gpu/drm/mgag200/mgag200_g200er.c | 34 ++ drivers/gpu/drm/mgag200/mgag200_g200ev.c | 40 drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 2 + drivers/gpu/drm/mgag200/mgag200_g200se.c | 45 + drivers/gpu/drm/mgag200/mgag200_g200wb.c | 34 ++ drivers/gpu/drm/mgag200/mgag200_mod
Re: [RFC PATCH 1/3] drm/bridge: ti-sn65dsi86: switch to atomic ops
On Sun, 10 Jul 2022 at 22:04, Sam Ravnborg wrote: > > Hi Dmitry, > > On Sun, Jul 10, 2022 at 09:45:34PM +0300, Dmitry Baryshkov wrote: > > Make ti-sn65dsi86 use atomic_enable / atomic_disable / atomic_pre_enable > > / atomic_post_disable rather than their non-atomic versions. > > > > Signed-off-by: Dmitry Baryshkov > > a more or less identical patch was applied to drm-misc-next > the other day. > See d8b599bf625d1d818fdbb322a272fd2a5ea32e38. Ugh, thanks for pointing this out. > > Sam > > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index 8cad662de9bb..01171547f638 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -752,7 +752,8 @@ ti_sn_bridge_mode_valid(struct drm_bridge *bridge, > > return MODE_OK; > > } > > > > -static void ti_sn_bridge_disable(struct drm_bridge *bridge) > > +static void ti_sn_bridge_atomic_disable(struct drm_bridge *bridge, > > + struct drm_bridge_state > > *old_bridge_state) > > { > > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > > > @@ -1011,7 +1012,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 > > *pdata, int dp_rate_idx, > > return ret; > > } > > > > -static void ti_sn_bridge_enable(struct drm_bridge *bridge) > > +static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, > > +struct drm_bridge_state > > *old_bridge_state) > > { > > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > const char *last_err_str = "No supported DP rate"; > > @@ -1080,7 +1082,8 @@ static void ti_sn_bridge_enable(struct drm_bridge > > *bridge) > > VSTREAM_ENABLE); > > } > > > > -static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) > > +static void ti_sn_bridge_atomic_pre_enable(struct drm_bridge *bridge, > > +struct drm_bridge_state > > *old_bridge_state) > > { > > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > > > @@ -1093,7 +1096,8 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge > > *bridge) > > usleep_range(100, 110); > > } > > > > -static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) > > +static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge, > > + struct drm_bridge_state > > *old_bridge_state) > > { > > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > > > @@ -1114,10 +1118,10 @@ static const struct drm_bridge_funcs > > ti_sn_bridge_funcs = { > > .attach = ti_sn_bridge_attach, > > .detach = ti_sn_bridge_detach, > > .mode_valid = ti_sn_bridge_mode_valid, > > - .pre_enable = ti_sn_bridge_pre_enable, > > - .enable = ti_sn_bridge_enable, > > - .disable = ti_sn_bridge_disable, > > - .post_disable = ti_sn_bridge_post_disable, > > + .atomic_pre_enable = ti_sn_bridge_atomic_pre_enable, > > + .atomic_enable = ti_sn_bridge_atomic_enable, > > + .atomic_disable = ti_sn_bridge_atomic_disable, > > + .atomic_post_disable = ti_sn_bridge_atomic_post_disable, > > }; > > > > static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata, > > -- > > 2.35.1 -- With best wishes Dmitry
Re: [PATCH 00/14] drm/mgag200: Move model-specific code into separate functions
Hi Sam, thanks for reviewing. Am 08.07.22 um 19:22 schrieb Sam Ravnborg: Hi Thomas, On Fri, Jul 08, 2022 at 11:39:15AM +0200, Thomas Zimmermann wrote: Mgag200 still mixes model-specific code and generic code in the same functions. Separate it into distinct helpers. As part of this effort, convert the driver from simple-KMS helpers to regular atomic helpers. The latter are way more flexible and can be adapted easily for each hardware model. Tested on Matrox G200 and G200EH hardware. Thomas Zimmermann (14): drm/mgag200: Split mgag200_modeset_init() drm/mgag200: Move DAC-register setup into model-specific code dmr/mgag200: Move ER/EW3 register initializatino to per-model code drm/mgag200: Acquire I/O-register lock in atomic_commit_tail function drm/mgag200: Store primary plane's color format in CRTC state drm/mgag200: Reorganize before dropping simple-KMS helpers drm/mgag200: Replace simple-KMS with regular atomic helpers drm/mgag200: Set SCROFF in primary-plane code drm/mgag200: Add per-device callbacks drm/mgag200: Provide per-device callbacks for BMC synchronization drm/mgag200: Provide per-device callbacks for PIXPLLC drm/mgag200: Move mode-config to model-specific code drm/mgag200: Move CRTC atomic_enable to model-specfic code drm/mgag200: Remove type field from struct mga_device I have browsed the patches and they looked good. I was about to complain about several things, like bmc init, but then later this is all nicely cleaned up. Especially the pll setup stuff did not get much scrunity, as it like most of the patch looked like code movements. Indeed. The patch moves code and adapts the functions' interfaces to the new callbacks. But the implementation remains the same. All patches except "Move DAC-register setup into model-specific code" are: Acked-by: Sam Ravnborg 14 files changed, 2804 insertions(+), 1586 deletions(-) This is not a diffstat that makes one go - yyipeee. But the improvements makes it worthwhile. Yeah. I accept a grow in driver size in exchange for the more cleanly structured code base. In my reply to the DAC review, I outline ways to re-share some of the duplicated code. Best regards Thomas Sam -- 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: [RFC PATCH 2/3] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
On Sun, 10 Jul 2022 at 22:11, Sam Ravnborg wrote: > > Hi Dmitry, > > On Sun, Jul 10, 2022 at 09:45:35PM +0300, Dmitry Baryshkov wrote: > > Rather than reading the pdata->connector directly, fetch the connector > > using drm_atomic_state. This allows us to make pdata->connector optional > > (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR). > > > > Signed-off-by: Dmitry Baryshkov > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 20 ++-- > > 1 file changed, 14 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > index 01171547f638..df08207d6223 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > > @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct > > ti_sn65dsi86 *pdata) > > regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); > > } > > > > -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) > > +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector) > > { > > - if (pdata->connector->display_info.bpc <= 6) > > + if (connector->display_info.bpc <= 6) > > return 18; > > else > > return 24; > > @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { > > 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 > > }; > > > > -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) > > +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, > > unsigned int bpp) > > { > > unsigned int bit_rate_khz, dp_rate_mhz; > > unsigned int i; > > @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct > > ti_sn65dsi86 *pdata) > > &pdata->bridge.encoder->crtc->state->adjusted_mode; > > > > /* Calculate minimum bit rate based on our pixel clock. */ > > - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata); > > + bit_rate_khz = mode->clock * bpp; > > > > /* Calculate minimum DP data rate, taking 80% as per DP spec */ > > dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM, > > @@ -1016,12 +1016,19 @@ static void ti_sn_bridge_atomic_enable(struct > > drm_bridge *bridge, > > struct drm_bridge_state > > *old_bridge_state) > > { > > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > > + struct drm_connector *connector; > > const char *last_err_str = "No supported DP rate"; > > unsigned int valid_rates; > > int dp_rate_idx; > > unsigned int val; > > int ret = -EINVAL; > > int max_dp_lanes; > > + unsigned int bpp; > > + > > + connector = > > drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state, > > + bridge->encoder); > > + if (!connector) > > + return; > It would be prudent with a dev_err() logging here as we do not expect to > fail. > I looked into something similar, but with a less elegant solution, and > could not convince myself that the display driver would create the > connector before ti_sn_bridge_atomic_enable() was called. If I understand your concern, the connectors (as does the rest of CRTC/encoder/etc objects) are not dynamic, so they must be created before being able to use the DRM device or any part of thereof is being actually used (enable/disable/modeset). > > This is another reason why a dev_err would be nice - so tester could see > if this fails or not. Will fix this in v2. > > With the dev_err added: > Reviewed-by: Sam Ravnborg > > > > > max_dp_lanes = ti_sn_get_max_lanes(pdata); > > pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes); > > @@ -1047,8 +1054,9 @@ static void ti_sn_bridge_atomic_enable(struct > > drm_bridge *bridge, > > drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, > > DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); > > > > + bpp = ti_sn_bridge_get_bpp(connector); > > /* Set the DP output format (18 bpp or 24 bpp) */ > > - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0; > > + val = (bpp == 18) ? BPP_18_RGB : 0; > > regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, > > val); > > > > /* DP lane config */ > > @@ -1059,7 +1067,7 @@ static void ti_sn_bridge_atomic_enable(struct > > drm_bridge *bridge, > > valid_rates = ti_sn_bridge_read_valid_rates(pdata); > > > > /* Train until we run out of rates */ > > - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata); > > + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp); > >dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); > >dp_rate_idx++) { > > if (!(valid_rates & BIT(dp_rate_idx))) > > -- > > 2.35.1 -- With best wishes Dmitry
[RFC PATCH v2 0/2] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
An RFC (or rather RFT, Request-for-Testing) series adding support for DRM_BRIDGE_ATTACH_NO_CONNECTOR. Note, it was compile-tested only. This bridge is the last one used on the Qualcomm platforms (in upstream-supported devices) and thus it is the only bridge that prevents us from removing support for bridge-created connectors from MSM DSI code. Changes since RFC v1: - Dropped first patch (conversion to atomic), corresponding patch has been already applied upstream - Added DRM_DEV_ERR_RATELIMITED to notifiy users/developers that corresponding connector was not found. Dmitry Baryshkov (2): drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR drivers/gpu/drm/bridge/ti-sn65dsi86.c | 40 --- 1 file changed, 24 insertions(+), 16 deletions(-) -- 2.35.1
[RFC PATCH v2 1/2] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
Rather than reading the pdata->connector directly, fetch the connector using drm_atomic_state. This allows us to make pdata->connector optional (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR). Reviewed-by: Sam Ravnborg Reviewed-by: Laurent Pinchart Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index d6dd4d99a229..b1b6ed3a8acc 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); } -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector) { - if (pdata->connector->display_info.bpc <= 6) + if (connector->display_info.bpc <= 6) return 18; else return 24; @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 }; -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp) { unsigned int bit_rate_khz, dp_rate_mhz; unsigned int i; @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) &pdata->bridge.encoder->crtc->state->adjusted_mode; /* Calculate minimum bit rate based on our pixel clock. */ - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata); + bit_rate_khz = mode->clock * bpp; /* Calculate minimum DP data rate, taking 80% as per DP spec */ dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM, @@ -1016,12 +1016,21 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + struct drm_connector *connector; const char *last_err_str = "No supported DP rate"; unsigned int valid_rates; int dp_rate_idx; unsigned int val; int ret = -EINVAL; int max_dp_lanes; + unsigned int bpp; + + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state, +bridge->encoder); + if (!connector) { + DRM_DEV_ERROR_RATELIMITED(pdata->dev, "Could not get the connector\n"); + return; + } max_dp_lanes = ti_sn_get_max_lanes(pdata); pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes); @@ -1047,8 +1056,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); + bpp = ti_sn_bridge_get_bpp(connector); /* Set the DP output format (18 bpp or 24 bpp) */ - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0; + val = bpp == 18 ? BPP_18_RGB : 0; regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val); /* DP lane config */ @@ -1059,7 +1069,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, valid_rates = ti_sn_bridge_read_valid_rates(pdata); /* Train until we run out of rates */ - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata); + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp); dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); dp_rate_idx++) { if (!(valid_rates & BIT(dp_rate_idx))) -- 2.35.1
[RFC PATCH v2 2/2] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
Now as the driver does not depend on pdata->connector, add support for attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR. Reviewed-by: Sam Ravnborg Reviewed-by: Laurent Pinchart Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 18 -- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index b1b6ed3a8acc..314a814da6cc 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -698,11 +698,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); int ret; - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - pdata->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&pdata->aux); if (ret < 0) { @@ -710,15 +705,18 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return ret; } - /* We never want the next bridge to *also* create a connector: */ - flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR; - - /* Attach the next bridge */ + /* +* Attach the next bridge. +* We never want the next bridge to *also* create a connector. +*/ ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge, - &pdata->bridge, flags); + &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (ret < 0) goto err_initted_aux; + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) + return 0; + pdata->connector = drm_bridge_connector_init(pdata->bridge.dev, pdata->bridge.encoder); if (IS_ERR(pdata->connector)) { -- 2.35.1
[PATCH] drm/panel-edp: add AUO B133UAN02.1 panel entry
Add an eDP panel entry for AUO B133UAN02.1. Due to lack of documentation, use the delay_200_500_e50 timings like some other AUO entries for now. Signed-off-by: Johan Hovold --- drivers/gpu/drm/panel/panel-edp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c index fe3897b86665..30f69cd8f9ee 100644 --- a/drivers/gpu/drm/panel/panel-edp.c +++ b/drivers/gpu/drm/panel/panel-edp.c @@ -1845,6 +1845,7 @@ static const struct panel_delay delay_100_500_e200 = { * Sort first by vendor, then by product ID. */ static const struct edp_panel_entry edp_panels[] = { + EDP_PANEL_ENTRY('A', 'U', 'O', 0x1e9b, &delay_200_500_e50, "B133UAN02.1"), EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01"), EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1"), EDP_PANEL_ENTRY('A', 'U', 'O', 0x8594, &delay_200_500_e50, "B133UAN01.0"), -- 2.35.1
Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code
Hi Javier, I'll try to give the motivation of this patch below. I known it's rather hypothetical as the VGA driver is probably not used much. Am 08.07.22 um 15:09 schrieb Javier Martinez Canillas: Hello Thomas, On 7/7/22 17:39, Thomas Zimmermann wrote: Move the device-creation from vga16fb to sysfb code. Move the few extra videomode checks into vga16fb's probe function. The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except for some MIPS systems. It's not clear if the vga16fb driver actually works in practice. Signed-off-by: Thomas Zimmermann --- drivers/firmware/sysfb.c | 4 +++ drivers/video/fbdev/vga16fb.c | 59 +-- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index 1f276f108cc9..3fd3563d962b 100644 --- a/drivers/firmware/sysfb.c +++ b/drivers/firmware/sysfb.c @@ -94,6 +94,10 @@ static __init int sysfb_init(void) name = "efi-framebuffer"; else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB) name = "vesa-framebuffer"; + else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC) + name = "vga-framebuffer"; + else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC) + I wonder if we really need to do this distinction or could just have a single platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the same fbdev driver is bound against both platform devices. With the current driver, we don't strictly need to distinguish. But the sysfb code is the one we care about. So I wanted it to be clear and nicely looking. All the mode tests, etc depend on the driver (which no one cares about). There's also a difference in hardware features between EGA and VGA. Most notably, VGA has much better color support. [...] static int vga16fb_probe(struct platform_device *dev) { + struct screen_info *si; struct fb_info *info; struct vga16fb_par *par; int i; int ret = 0; + si = dev_get_platdata(&dev->dev); + if (!si) + return -ENODEV; + + ret = check_mode_supported(si); + if (ret) + return ret; + What do you see as the advantage of moving the check to the driver's probe? Because after this change the platform driver will be registered but for no reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC. The driver only supports very specific modes, which may not be what screen_info detected. Note that VGAC/EGAC can also refer to text-mode buffers. The current vgacon could be turned into a platform driver that adopts such a text buffer and integrates it with aperture helpers. [...] +static const struct platform_device_id vga16fb_driver_id_table[] = { + {"ega-framebuffer", 0}, + {"vga-framebuffer", 0}, + { } +}; + The fact that the two entries don't have a platform data is an indication for The name is the indication. I know that vga16 doesn't treat them differently. me that we could just consolidate in a single "vga16-framebuffer" or smt. I know that this won't be consistent with efi, vesa, etc but I don't think is that important and also quite likely we will get rid of this driver and the platform device registration soon. Since as you said, it's unclear that is even used. There's mips code in the arch/ directory that appears to setup screen_info in the correct way. I can't say whether that's still useful to anyone. On x86, I could set a VGA mode on the kernel command line, but screen_info's isVGA only contained '1'. It might be possible to fix this easily by setting the right values in vga_probe(). [1] I simply don't have the time to provide a patch and deal with the potential fallout of such a change. static struct platform_driver vga16fb_driver = { .probe = vga16fb_probe, .remove = vga16fb_remove, .driver = { - .name = "vga16fb", + .name = "vga-framebuffer", }, Maybe "vga16-framebuffer" instead? Since for example VESA is also VGA AFAIK. VESA is something else than VGA. Setting a VESA mode (done via INT 10h IIRC) and then fiddling with VGA state will likely produce broken output on the screen. Best regards Thomas [1] https://elixir.bootlin.com/linux/v5.18.10/source/arch/x86/boot/video-vga.c#L231 -- 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 03/11] fbdev/vga16fb: Auto-generate module init/exit code
Hi Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas: On 7/7/22 17:39, Thomas Zimmermann wrote: Move vgag16fb's option parsing into the driver's probe function and generate the rest of the module's init/exit functions from macros. Keep the options code, although there are no options defined. Ah, I see now why you wanted to move the check to the probe function. If is to allow this cleanup then discard that comment from previous patch and I'm OK with the move. Maybe you could comment in patch 02/11 commit message that the check is moved to the probe handler to allow this cleanup as a follow-up patch ? Sure. I mostly wanted to use module_platform_driver(). The options handling is in the way. Signed-off-by: Thomas Zimmermann --- drivers/video/fbdev/vga16fb.c | 35 ++- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c index f7c1bb018843..e7767ed50c5b 100644 --- a/drivers/video/fbdev/vga16fb.c +++ b/drivers/video/fbdev/vga16fb.c @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) static int vga16fb_probe(struct platform_device *dev) { +#ifndef MODULE + char *option = NULL; +#endif struct screen_info *si; struct fb_info *info; struct vga16fb_par *par; int i; int ret = 0; +#ifndef MODULE + if (fb_get_options("vga16fb", &option)) + return -ENODEV; + vga16fb_setup(option); +#endif + I would just drop these ifdefery and have the option unconditionally. It seems that's what most fbdev drivers do AFAICT. Or can we kill it entirely? There are no actual options. Best regards Thomas Reviewed-by: Javier Martinez Canillas -- 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: linux-next: manual merge of the drm tree with the drm-misc-fixes tree
Hi Stephen, Am 11.07.22 um 04:47 schrieb Stephen Rothwell: Hi all, Today's linux-next merge of the drm tree got a conflict in: drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c between commit: 925b6e59138c ("Revert "drm/amdgpu: add drm buddy support to amdgpu"") from the drm-misc-fixes tree and commit: 5e3f1e7729ec ("drm/amdgpu: fix start calculation in amdgpu_vram_mgr_new") from the drm tree. This is a mess :-( I have just reverted the above revert before mergin the drm tree for today, please fix it up. Sorry for the noise, the patch "5e3f1e7729ec ("drm/amdgpu: fix start calculation in amdgpu_vram_mgr_new")" and another one is going to be reverted from the drm tree as well. It's just that -fixes patches where faster than -next patches. Regards, Christian.
[syzbot] BUG: unable to handle kernel paging request in bitfill_aligned (3)
Hello, syzbot found the following issue on: HEAD commit:e35e5b6f695d Merge tag 'xsa-5.19-tag' of git://git.kernel... git tree: upstream console output: https://syzkaller.appspot.com/x/log.txt?x=17f49bbc08 kernel config: https://syzkaller.appspot.com/x/.config?x=f3bf7765b1ebd721 dashboard link: https://syzkaller.appspot.com/bug?extid=a168dbeaaa7778273c1b compiler: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63, GNU ld (GNU Binutils for Debian) 2.35.2 Unfortunately, I don't have any reproducer for this issue yet. IMPORTANT: if you fix the issue, please add the following tag to the commit: Reported-by: syzbot+a168dbeaaa7778273...@syzkaller.appspotmail.com BUG: unable to handle page fault for address: c90004331000 #PF: supervisor write access in kernel mode #PF: error_code(0x0002) - not-present page PGD 11c00067 P4D 11c00067 PUD 11dc5067 PMD 1cffd067 PTE 0 Oops: 0002 [#1] PREEMPT SMP KASAN CPU: 0 PID: 11483 Comm: syz-executor.4 Not tainted 5.19.0-rc5-syzkaller-00056-ge35e5b6f695d #0 Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 RIP: 0010:memset64 arch/x86/include/asm/string_64.h:49 [inline] RIP: 0010:memset_l include/linux/string.h:128 [inline] RIP: 0010:bitfill_aligned+0x1ad/0x270 drivers/video/fbdev/core/sysfillrect.c:53 Code: 08 49 31 ef eb 66 e8 32 9c 05 fd 45 89 e6 4c 8b 64 24 10 44 89 f0 31 d2 f7 f3 89 c3 48 8b 6c 24 08 48 89 e8 4c 89 e7 48 89 d9 48 ab 31 ff 44 89 ee e8 26 a0 05 fd 4d 85 ed 74 5f 4d 8d 24 dc RSP: 0018:c9000ae3f7e8 EFLAGS: 00010246 RAX: RBX: 1800 RCX: 1200 RDX: RSI: 0bca RDI: c90004331000 RBP: R08: 8481e07e R09: 0040 R10: 0002 R11: 88803938d880 R12: c9000432e000 R13: R14: 0006 R15: FS: 7f8c16811700() GS:8880b9a0() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: c90004331000 CR3: 6dd66000 CR4: 003506f0 DR0: DR1: DR2: DR3: DR6: fffe0ff0 DR7: 0400 Call Trace: sys_fillrect+0x5ce/0x7f0 drivers/video/fbdev/core/sysfillrect.c:281 drm_fb_helper_sys_fillrect drivers/gpu/drm/drm_fb_helper.c:795 [inline] drm_fbdev_fb_fillrect+0x163/0x300 drivers/gpu/drm/drm_fb_helper.c:2310 bit_clear_margins+0x3f1/0x6e0 drivers/video/fbdev/core/bitblit.c:232 fbcon_clear_margins drivers/video/fbdev/core/fbcon.c:1304 [inline] fbcon_do_set_font+0xd7c/0x1330 drivers/video/fbdev/core/fbcon.c:2434 fbcon_set_font+0xa9c/0xd80 drivers/video/fbdev/core/fbcon.c:2517 con_font_set drivers/tty/vt/vt.c:4666 [inline] con_font_op+0xbe8/0x1070 drivers/tty/vt/vt.c:4710 vt_k_ioctl drivers/tty/vt/vt_ioctl.c:474 [inline] vt_ioctl+0x172e/0x1d00 drivers/tty/vt/vt_ioctl.c:752 tty_ioctl+0x874/0xc60 drivers/tty/tty_io.c:2778 vfs_ioctl fs/ioctl.c:51 [inline] __do_sys_ioctl fs/ioctl.c:870 [inline] __se_sys_ioctl+0xfb/0x170 fs/ioctl.c:856 do_syscall_x64 arch/x86/entry/common.c:50 [inline] do_syscall_64+0x2b/0x70 arch/x86/entry/common.c:80 entry_SYSCALL_64_after_hwframe+0x46/0xb0 RIP: 0033:0x7f8c15689109 Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48 RSP: 002b:7f8c16811168 EFLAGS: 0246 ORIG_RAX: 0010 RAX: ffda RBX: 7f8c1579bf60 RCX: 7f8c15689109 RDX: 2040 RSI: 4b72 RDI: 0003 RBP: 7f8c156e305d R08: R09: R10: R11: 0246 R12: R13: 7ffdfe77e39f R14: 7f8c16811300 R15: 00022000 Modules linked in: CR2: c90004331000 ---[ end trace ]--- RIP: 0010:memset64 arch/x86/include/asm/string_64.h:49 [inline] RIP: 0010:memset_l include/linux/string.h:128 [inline] RIP: 0010:bitfill_aligned+0x1ad/0x270 drivers/video/fbdev/core/sysfillrect.c:53 Code: 08 49 31 ef eb 66 e8 32 9c 05 fd 45 89 e6 4c 8b 64 24 10 44 89 f0 31 d2 f7 f3 89 c3 48 8b 6c 24 08 48 89 e8 4c 89 e7 48 89 d9 48 ab 31 ff 44 89 ee e8 26 a0 05 fd 4d 85 ed 74 5f 4d 8d 24 dc RSP: 0018:c9000ae3f7e8 EFLAGS: 00010246 RAX: RBX: 1800 RCX: 1200 RDX: RSI: 0bca RDI: c90004331000 RBP: R08: 8481e07e R09: 0040 R10: 0002 R11: 88803938d880 R12: c9000432e000 R13: R14: 0006 R15: FS: 7f8c16811700() GS:8880b9a0() knlGS: CS: 0010 DS: ES: CR0: 80050033 CR2: c90004331000 CR3: 6dd66000 CR4: 003506f0 DR0: DR1: DR2: 000
Re: [PATCH v2 02/11] arm64: dts: qcom: sc7180: rename DPU device node
On 10.07.2022 11:00, Dmitry Baryshkov wrote: > Rename DPU device node to display-controller@ae01000 to follow the > DPU schema. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Konrad Dybcio Konrad > arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi > b/arch/arm64/boot/dts/qcom/sc7180.dtsi > index 72994f599825..e63b4515453f 100644 > --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi > @@ -2911,7 +2911,7 @@ mdss: mdss@ae0 { > > status = "disabled"; > > - mdp: mdp@ae01000 { > + mdp: display-controller@ae01000 { > compatible = "qcom,sc7180-dpu"; > reg = <0 0x0ae01000 0 0x8f000>, > <0 0x0aeb 0 0x2008>;
Re: [PATCH v2 01/11] arm64: dts: qcom: sdm845: rename DPU device node
On 10.07.2022 11:00, Dmitry Baryshkov wrote: > Rename DPU device node to display-controller@ae01000 to follow the > DPU schema. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Konrad Dybcio Konrad > arch/arm64/boot/dts/qcom/sdm845.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi > b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index 32ad5972a642..7c66f490e822 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -4265,7 +4265,7 @@ mdss: mdss@ae0 { > #size-cells = <2>; > ranges; > > - mdss_mdp: mdp@ae01000 { > + mdss_mdp: display-controller@ae01000 { > compatible = "qcom,sdm845-dpu"; > reg = <0 0x0ae01000 0 0x8f000>, > <0 0x0aeb 0 0x2008>;
Re: [PATCH v2 03/11] arm64: dts: qcom: sm8250: rename DPU device node
On 10.07.2022 11:00, Dmitry Baryshkov wrote: > Rename DPU device node to display-controller@ae01000 to follow the > DPU schema. > > Signed-off-by: Dmitry Baryshkov > --- Reviewed-by: Konrad Dybcio Konrad > arch/arm64/boot/dts/qcom/sm8250.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi > b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 43c2d04b226f..48c60df59080 100644 > --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi > @@ -3444,7 +3444,7 @@ mdss: mdss@ae0 { > #size-cells = <2>; > ranges; > > - mdss_mdp: mdp@ae01000 { > + mdss_mdp: display-controller@ae01000 { > compatible = "qcom,sm8250-dpu"; > reg = <0 0x0ae01000 0 0x8f000>, > <0 0x0aeb 0 0x2008>;
Re: [PATCH 1/9] dt-bindings: msm/dp: drop extra p1 region
On 08/07/2022 22:47, Kuogee Hsieh wrote: On 7/8/2022 12:38 PM, Abhinav Kumar wrote: + kuogee On 7/8/2022 12:27 PM, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-07-07 20:46:43) On 08/07/2022 04:28, Stephen Boyd wrote: Quoting Dmitry Baryshkov (2022-07-07 14:31:56) The p1 region was probably added by mistake, none of the DTS files provides one (and the driver source code also doesn't use one). Drop it now. Yes, looks like the driver doesn't use it. Fixes: 687825c402f1 ("dt-bindings: msm/dp: Change reg definition") Signed-off-by: Dmitry Baryshkov --- Documentation/devicetree/bindings/display/msm/dp-controller.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml index 94bc6e1b6451..d6bbe58ef9e8 100644 --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml @@ -29,7 +29,6 @@ properties: - description: aux register block - description: link register block - description: p0 register block - - description: p1 register block The p1 registers exist on sc7180. They start where the example starts, at 0xae91400. Do they exist on e.g. sc7280? In other words, should we add the region to the DTS? For now I'm going to mark it as optional. Yes I see the same address for P1 on sc7280. Maybe it's a typo? Abhinav, can you confirm? P1 block does exist on sc7280 and yes its address is same as the address mentioned in sc7180. So its not a typo. Yes, we are not programming this today but I would prefer to keep this as optional. I did sync up with Kuogee on this change this morning, we will check a few things internally on the P1 block's usage as to which use-cases we need to program it for and update here. P1 block is for dp MST application. This allow two dp streams can be mux into same DP phy. We should keep it since we may support MST later. Thanks for the confirmation. Next question, does it exist on eDP controllers? The idea behind having this register space listed in the yaml is thats how the software documents have the blocks listed so dropping P1 block just because its unused seemed wrong to me. Optional seems more appropriate. Thanks Abhinav -- With best wishes Dmitry
Re: [PATCH v3 06/10] drm/fb-helper: Add support for DRM_FORMAT_C[124]
Hi Am 08.07.22 um 20:20 schrieb Geert Uytterhoeven: Add support for color-indexed frame buffer formats with two, four, and sixteen colors to the DRM framebuffer helper functions: 1. Add support for 1, 2, and 4 bits per pixel to the damage helper, 2. For color-indexed modes, the length of the color bitfields must be set to the color depth, else the logo code may pick a logo with too many colors. Drop the incorrect DAC width comment, which originates from the i915 driver. 3. Accept C[124] modes when validating or filling in struct fb_var_screeninfo, and use the correct number of bits per pixel. 4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed modes. Signed-off-by: Geert Uytterhoeven Reviewed-by: Javier Martinez Canillas --- If "[PATCH] drm/fb-helper: Remove helpers to change frame buffer config"[1] is accepted, the changes to drm_fb_helper_check_var() can just be removed. v3: - Fix FB_VISUAL_TRUECOLOR, - Add Reviewed-by, v2: - Use drm_format_info_bpp() helper instead of deprecated .depth field or format-dependent calculations, - Use new .is_color_indexed field instead of checking against a list of formats. [1] Link: https://lore.kernel.org/r/20220629105658.1373770-1-ge...@linux-m68k.org --- drivers/gpu/drm/drm_fb_helper.c | 101 1 file changed, 75 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 1705e8d345aba50a..5098efb374fe64ed 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -376,12 +376,31 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper, struct iosys_map *dst) { struct drm_framebuffer *fb = fb_helper->fb; - unsigned int cpp = fb->format->cpp[0]; - size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp; - void *src = fb_helper->fbdev->screen_buffer + offset; - size_t len = (clip->x2 - clip->x1) * cpp; + size_t offset = clip->y1 * fb->pitches[0]; + size_t len = clip->x2 - clip->x1; drm_rect_width() please. unsigned int y; + void *src; + switch (drm_format_info_bpp(fb->format, 0)) { + case 1: + offset += clip->x1 / 8; + len = DIV_ROUND_UP(len + clip->x1 % 8, 8); + break; + case 2: + offset += clip->x1 / 4; + len = DIV_ROUND_UP(len + clip->x1 % 4, 4); + break; + case 4: + offset += clip->x1 / 2; + len = DIV_ROUND_UP(len + clip->x1 % 2, 2); + break; + default: + offset += clip->x1 * fb->format->cpp[0]; + len *= fb->format->cpp[0]; + break; + } + + src = fb_helper->fbdev->screen_buffer + offset; iosys_map_incr(dst, offset); /* go to first pixel within clip rect */ for (y = clip->y1; y < clip->y2; y++) { @@ -1273,19 +1292,23 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1, } static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var, -u8 depth) +const struct drm_format_info *format) { - switch (depth) { - case 8: + u8 depth = format->depth; + + if (format->is_color_indexed) { var->red.offset = 0; var->green.offset = 0; var->blue.offset = 0; - var->red.length = 8; /* 8bit DAC */ - var->green.length = 8; - var->blue.length = 8; + var->red.length = depth; + var->green.length = depth; + var->blue.length = depth; var->transp.offset = 0; var->transp.length = 0; - break; + return; + } + + switch (depth) { case 15: var->red.offset = 10; var->green.offset = 5; @@ -1340,7 +1363,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, { struct drm_fb_helper *fb_helper = info->par; struct drm_framebuffer *fb = fb_helper->fb; + const struct drm_format_info *format = fb->format; struct drm_device *dev = fb_helper->dev; + unsigned int bpp; if (in_dbg_master()) return -EINVAL; @@ -1350,22 +1375,33 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var, var->pixclock = 0; } - if ((drm_format_info_block_width(fb->format, 0) > 1) || - (drm_format_info_block_height(fb->format, 0) > 1)) - return -EINVAL; + switch (format->format) { + case DRM_FORMAT_C1: + case DRM_FORMAT_C2: + case DRM_FORMAT_C4: + /* supported format with sub-byte pixels */ + break; + + default: +
[PATCH] drm/mipi-dsi: Fix device_node's refcount bugs
In mipi_dsi_device_register_full(), we should call of_node_get() when a new reference is created in to dsi->dev.of_node(), and call of_node_put() in fail path or when it is not used anymore in *_unregister(). The reasons to commit this patch as folllow: (1) There are totally 16 locations to call (devm_)mipi_dsi_device_register_full() in current version of kernel source code (5.19rc2), and 13 of them pass 'NULL' to the function. (2) Three other call sites have different manners as follow: <1> ./drm/drm_mipi_dsi.c/of_mipi_dsi_device_add() calls of_node_get() before pass the reference into above register func. <2> ./drm/panel/panel-raspberrypi-touchscreen.c/rpi_touchscreen_probe() calls of_graph_get_remote_port() to get the referenece with refcount incremented before pass it into above register func. <3> ./drm/panel/panel-novatek-nt35950.c/nt35950_probe() directly use the reference without any refcounting to call above register func. (3) So it is better to move refcounting into register func and also add the corresponding of_node_put() into in fail path and *_unregister func. Fixes: c63ae8a9686b ("drm/dsi: Use mipi_dsi_device_register_full() for DSI device creation") Signed-off-by: Liang He --- I have noticed that the of_node_get() is moved into of_mipi_dsi_device_add() which then call above register func in commit-c63ae8a9686b. However, as there are different ways to directly call the register, I think it is better to move the refcounting back into it. Please check it carefully. drivers/gpu/drm/drm_mipi_dsi.c| 6 -- drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c | 1 + 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c index c40bde96cfdf..856ecb3bcecb 100644 --- a/drivers/gpu/drm/drm_mipi_dsi.c +++ b/drivers/gpu/drm/drm_mipi_dsi.c @@ -173,7 +173,7 @@ of_mipi_dsi_device_add(struct mipi_dsi_host *host, struct device_node *node) } info.channel = reg; - info.node = of_node_get(node); + info.node = node; return mipi_dsi_device_register_full(host, &info); } @@ -221,12 +221,13 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host, return dsi; } - dsi->dev.of_node = info->node; + dsi->dev.of_node = of_node_get(info->node); dsi->channel = info->channel; strlcpy(dsi->name, info->type, sizeof(dsi->name)); ret = mipi_dsi_device_add(dsi); if (ret) { + of_node_put(dsi->dev.of_node); drm_err(host, "failed to add DSI device %d\n", ret); kfree(dsi); return ERR_PTR(ret); @@ -242,6 +243,7 @@ EXPORT_SYMBOL(mipi_dsi_device_register_full); */ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi) { + of_node_put(dsi->dev.of_node); device_unregister(&dsi->dev); } EXPORT_SYMBOL(mipi_dsi_device_unregister); diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c index 145047e19394..9622f903d4c2 100644 --- a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c +++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c @@ -424,6 +424,7 @@ static int rpi_touchscreen_probe(struct i2c_client *i2c, of_node_put(endpoint); ts->dsi = mipi_dsi_device_register_full(host, &info); + of_node_put(info.node); if (IS_ERR(ts->dsi)) { dev_err(dev, "DSI device registration failed: %ld\n", PTR_ERR(ts->dsi)); -- 2.25.1
Re: [PATCH v3 00/10] drm: Add support for low-color frame buffer formats
Hi Am 09.07.22 um 15:38 schrieb Sam Ravnborg: Hi Geert, On Fri, Jul 08, 2022 at 08:20:45PM +0200, Geert Uytterhoeven wrote: Hi all, A long outstanding issue with the DRM subsystem has been the lack of support for low-color displays, as used typically on older desktop systems, and on small embedded displays. For the patchset Acked-by: Thomas Zimemrmann IT is super to have this addressed - thanks! This patch series adds support for color-indexed frame buffer formats with 2, 4, and 16 colors. It has been tested on ARAnyM using a work-in-progress Atari DRM driver supporting 2, 4, 16, 256, and 65536 colors, with text console operation, fbtest, and modetest. Overview: - Patch 1 introduces a helper, to be used by later patches in the series, - Patch 2 introduces a flag to indicate color-indexed formats, - Patches 3 and 4 correct calculations of bits per pixel for sub-byte pixel formats, - Patches 5 and 6 introduce the new C[124] formats, - Patch 7 fixes an untested code path, - Patch 8 documents the use of "red" for light-on-dark displays, - Patches 9 and 10 add more fourcc codes for light-on-dark and dark-on-light frame buffer formats, which may be useful for e.g. the ssd130x and repaper drivers. Applied all patches to drm-misc (drm-misc-next), including the last two RFC patches as we then have the formats ready when a user pops up. I know it's v3 already, but give people at least a workday for reviewing before merging patches of this size and impact. Friday-evening patches are not supposed to be merged on Saturday afternoons. Best regards Thomas Sam -- 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
[PATCH v3] drm/i915/ttm: fix sg_table construction
If we encounter some monster sized local-memory page that exceeds the maximum sg length (UINT32_MAX), ensure that don't end up with some misaligned address in the entry that follows, leading to fireworks later. Also ensure we have some coverage of this in the selftests. v2(Chris): - Use round_down consistently to avoid udiv errors v3(Nirmoy): - Also update the max_segment in the selftest Fixes: f701b16d4cc5 ("drm/i915/ttm: add i915_sg_from_buddy_resource") Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6379 Signed-off-by: Matthew Auld Cc: Thomas Hellström Cc: Nirmoy Das --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 11 -- drivers/gpu/drm/i915/i915_scatterlist.c | 19 + drivers/gpu/drm/i915/i915_scatterlist.h | 6 -- drivers/gpu/drm/i915/intel_region_ttm.c | 10 ++--- drivers/gpu/drm/i915/intel_region_ttm.h | 3 ++- .../drm/i915/selftests/intel_memory_region.c | 21 +-- drivers/gpu/drm/i915/selftests/mock_region.c | 3 ++- 7 files changed, 58 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 7e1f8b83077f..c5c8aa1f8558 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -602,10 +602,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj, struct ttm_resource *res) { struct ttm_buffer_object *bo = i915_gem_to_ttm(obj); + u64 page_alignment; if (!i915_ttm_gtt_binds_lmem(res)) return i915_ttm_tt_get_st(bo->ttm); + page_alignment = bo->page_alignment << PAGE_SHIFT; + if (!page_alignment) + page_alignment = obj->mm.region->min_page_size; + /* * If CPU mapping differs, we need to add the ttm_tt pages to * the resulting st. Might make sense for GGTT. @@ -616,7 +621,8 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj, struct i915_refct_sgt *rsgt; rsgt = intel_region_ttm_resource_to_rsgt(obj->mm.region, -res); +res, + page_alignment); if (IS_ERR(rsgt)) return rsgt; @@ -625,7 +631,8 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj, return i915_refct_sgt_get(obj->ttm.cached_io_rsgt); } - return intel_region_ttm_resource_to_rsgt(obj->mm.region, res); + return intel_region_ttm_resource_to_rsgt(obj->mm.region, res, +page_alignment); } static int i915_ttm_truncate(struct drm_i915_gem_object *obj) diff --git a/drivers/gpu/drm/i915/i915_scatterlist.c b/drivers/gpu/drm/i915/i915_scatterlist.c index 159571b9bd24..f63b50b71e10 100644 --- a/drivers/gpu/drm/i915/i915_scatterlist.c +++ b/drivers/gpu/drm/i915/i915_scatterlist.c @@ -68,6 +68,7 @@ void i915_refct_sgt_init(struct i915_refct_sgt *rsgt, size_t size) * drm_mm_node * @node: The drm_mm_node. * @region_start: An offset to add to the dma addresses of the sg list. + * @page_alignment: Required page alignment for each sg entry. Power of two. * * Create a struct sg_table, initializing it from a struct drm_mm_node, * taking a maximum segment length into account, splitting into segments @@ -77,15 +78,18 @@ void i915_refct_sgt_init(struct i915_refct_sgt *rsgt, size_t size) * error code cast to an error pointer on failure. */ struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct drm_mm_node *node, - u64 region_start) + u64 region_start, + u64 page_alignment) { - const u64 max_segment = SZ_1G; /* Do we have a limit on this? */ + const u64 max_segment = round_down(UINT_MAX, page_alignment); u64 segment_pages = max_segment >> PAGE_SHIFT; u64 block_size, offset, prev_end; struct i915_refct_sgt *rsgt; struct sg_table *st; struct scatterlist *sg; + GEM_BUG_ON(!max_segment); + rsgt = kmalloc(sizeof(*rsgt), GFP_KERNEL); if (!rsgt) return ERR_PTR(-ENOMEM); @@ -112,6 +116,8 @@ struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct drm_mm_node *node, sg = __sg_next(sg); sg_dma_address(sg) = region_start + offset; + GEM_BUG_ON(!IS_ALIGNED(sg_dma_address(sg), + page_alignment)); sg_dma_len(sg) = 0; sg->length = 0; st->nents++; @@ -138,6 +144,7 @@ struct i915_refct_sgt *i915_rsgt_from_mm_nod
Re: [PATCH v1 0/11] drm/via: Make via a single file driver
Hi Thomas, On Mon, Jul 11, 2022 at 09:01:50AM +0200, Thomas Zimmermann wrote: > Hi Sam, > > this looks like a good solution to me. I'd give you a detailed review, but > dri-devel decided to only send me the color letter. I had to use patchwork > get the patches. > > For the series > > Acked-by: Thomas Zimmermann Thanks! > > with suggestions below. > > Am 10.07.22 um 10:54 schrieb Sam Ravnborg: > > We have an upcoming openchrome driver for VIA where some > > of the files conflicts with the existing via driver. > > > > It is not acceptable to just delete the existing DRI1 > > based driver as there are most likely users out there, > > so a different approach was required. > > > > It was disccussed what to do and the least invasive > > solution was to keep the DRI1 driver in the current > > directory as a single file. > > > > Thomas Zimmermann already posted a patch to do the > > same but it attemped to have a single driver > > for the DRI1 and the upcoming new driver. > > After the openchrome patches land, there will be an option in Kconfig to > build the old driver instead of of the new one? > > > > > This patchset embeds the files one by one to make the > > diffs remotely readable and end up with an independent > > DRI1 driver. > > > > The driver was built tested for each step. > > > > The driver is in the end renamed to via_dri1. > > Some feedback on this would be good as I do not know > > what impact it will have on users. > > I don't know how Mesa decides about which userspace driver to load, but It > seems related to the name of the kernel driver. Renaming the module might > interfere somehow. But if the old and new driver are mutually exclusive at > build time, it should not be a problem to use the same name for both > modules. Another option could be to keep the "via.ko" name and come up with a new name for the openchrome variant (via_drm?). I think we need to allow both drivers to be built as a user may want to try out the old and the new driver without to much hassle. In the next iteration I will drop the rename of the driver - it is easy to do later as it is a simple one-liner. > > > > The very last patch synchronize the via_3d_reg header > > file with the one used in the openchrome driver. > > This was added to verify that the new header would not > > break the DRI1 driver. > > Some of the macros introduce line wraps. I don't know if you did that > intentionally, but it shouldn't be necessary. We have a 100-character limit > per line. The via_3d_reg file was copied verbatim from the openchrome repo. I will fix up relevant checkpatch warnings in a follow-up patch so it is obvious what is changed from the original source. > Maybe you can also add an extra patch that adds SPDX license tags at the top > of the files. Yep, will do. Thanks for the feedback! Sam
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
Hi Geert Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: The mode parsing code recognizes named modes only if they are explicitly listed in the internal whitelist, which is currently limited to "NTSC" and "PAL". Provide a mechanism for drivers to override this list to support custom mode names. Ideally, this list should just come from the driver's actual list of modes, but connector->probed_modes is not yet populated at the time of parsing. I've looked for code that uses these names, couldn't find any. How is this being used in practice? For example, if I say "PAL" on the command line, is there DRM code that fills in the PAL mode parameters? And another question I have is whether this whitelist belongs into the driver at all. Standard modes exist independent from drivers or hardware. Shouldn't there simply be a global list of all possible mode names? Drivers would filter out the unsupported modes anyway. Best regards Thomas Signed-off-by: Geert Uytterhoeven --- drivers/gpu/drm/drm_modes.c | 15 +++ include/drm/drm_connector.h | 10 ++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 9ce275fbda566b7c..7a00eb6df502e991 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1748,25 +1748,31 @@ static int drm_mode_parse_cmdline_options(const char *str, static const char * const drm_named_modes_whitelist[] = { "NTSC", "PAL", + NULL }; static int drm_mode_parse_cmdline_named_mode(const char *name, unsigned int length, bool refresh, +const struct drm_connector *connector, struct drm_cmdline_mode *mode) { + const char * const *named_modes_whitelist; unsigned int i; int ret; - for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) { - ret = str_has_prefix(name, drm_named_modes_whitelist[i]); + named_modes_whitelist = connector->named_modes_whitelist ? : + drm_named_modes_whitelist; + + for (i = 0; named_modes_whitelist[i]; i++) { + ret = str_has_prefix(name, named_modes_whitelist[i]); if (!ret) continue; if (refresh) return -EINVAL; /* named + refresh is invalid */ - strcpy(mode->name, drm_named_modes_whitelist[i]); + strcpy(mode->name, named_modes_whitelist[i]); mode->specified = true; return 0; } @@ -1850,7 +1856,8 @@ bool drm_mode_parse_command_line_for_connector(const char *mode_option, /* First check for a named mode */ if (mode_end) { ret = drm_mode_parse_cmdline_named_mode(name, mode_end, - refresh_ptr, mode); + refresh_ptr, connector, + mode); if (ret) return false; } diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index 3ac4bf87f2571c4c..6361f8a596c01107 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -1659,6 +1659,16 @@ struct drm_connector { /** @hdr_sink_metadata: HDR Metadata Information read from sink */ struct hdr_sink_metadata hdr_sink_metadata; + + /** +* @named_modes_whitelist: +* +* Optional NULL-terminated array of names to be considered valid mode +* names. This lets the command line option parser distinguish between +* mode names and freestanding extras and/or options. +* If not set, a set of defaults will be used. +*/ + const char * const *named_modes_whitelist; }; #define obj_to_connector(x) container_of(x, struct drm_connector, base) -- 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 0/5] drm/modes: Command line mode selection fixes and improvements
Hi Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: Hi all, This patch series contains fixes and improvements for specifying video modes on the kernel command line. This has been tested on ARAnyM using a work-in-progress Atari DRM driver (more info and related patches can be found in [1]). Thanks for your comments! Patches 1 to 3 look reasonable to me. For those: Acked-by: Thomas Zimmermann Please see my questions on patch 4. Best regards Thomas [1] "[PATCH v3 00/10] drm: Add support for low-color frame buffer formats" https://lore.kernel.org/r/cover.1657294931.git.ge...@linux-m68k.org Geert Uytterhoeven (5): drm/modes: parse_cmdline: Handle empty mode name part drm/modes: Extract drm_mode_parse_cmdline_named_mode() drm/modes: parse_cmdline: Make mode->*specified handling more uniform drm/modes: Add support for driver-specific named modes drm/modes: parse_cmdline: Add support for named modes containing dashes drivers/gpu/drm/drm_modes.c | 57 ++--- include/drm/drm_connector.h | 10 +++ 2 files changed, 50 insertions(+), 17 deletions(-) -- 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 v1 0/11] drm/via: Make via a single file driver
Hi Am 11.07.22 um 11:01 schrieb Sam Ravnborg: Hi Thomas, On Mon, Jul 11, 2022 at 09:01:50AM +0200, Thomas Zimmermann wrote: Hi Sam, this looks like a good solution to me. I'd give you a detailed review, but dri-devel decided to only send me the color letter. I had to use patchwork get the patches. For the series Acked-by: Thomas Zimmermann Thanks! with suggestions below. Am 10.07.22 um 10:54 schrieb Sam Ravnborg: We have an upcoming openchrome driver for VIA where some of the files conflicts with the existing via driver. It is not acceptable to just delete the existing DRI1 based driver as there are most likely users out there, so a different approach was required. It was disccussed what to do and the least invasive solution was to keep the DRI1 driver in the current directory as a single file. Thomas Zimmermann already posted a patch to do the same but it attemped to have a single driver for the DRI1 and the upcoming new driver. After the openchrome patches land, there will be an option in Kconfig to build the old driver instead of of the new one? This patchset embeds the files one by one to make the diffs remotely readable and end up with an independent DRI1 driver. The driver was built tested for each step. The driver is in the end renamed to via_dri1. Some feedback on this would be good as I do not know what impact it will have on users. I don't know how Mesa decides about which userspace driver to load, but It seems related to the name of the kernel driver. Renaming the module might interfere somehow. But if the old and new driver are mutually exclusive at build time, it should not be a problem to use the same name for both modules. Another option could be to keep the "via.ko" name and come up with a new name for the openchrome variant (via_drm?). viakms.ko then. I think we need to allow both drivers to be built as a user may want to try out the old and the new driver without to much hassle. I wish we'd make it really hard to use the old driver. :) Best regards Thomas In the next iteration I will drop the rename of the driver - it is easy to do later as it is a simple one-liner. The very last patch synchronize the via_3d_reg header file with the one used in the openchrome driver. This was added to verify that the new header would not break the DRI1 driver. Some of the macros introduce line wraps. I don't know if you did that intentionally, but it shouldn't be necessary. We have a 100-character limit per line. The via_3d_reg file was copied verbatim from the openchrome repo. I will fix up relevant checkpatch warnings in a follow-up patch so it is obvious what is changed from the original source. Maybe you can also add an extra patch that adds SPDX license tags at the top of the files. Yep, will do. Thanks for the feedback! Sam -- 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: [RFC PATCH v2 1/2] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
Hi Dmitry, On Mon, Jul 11, 2022 at 10:37:32AM +0300, Dmitry Baryshkov wrote: > Rather than reading the pdata->connector directly, fetch the connector > using drm_atomic_state. This allows us to make pdata->connector optional > (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR). > > Reviewed-by: Sam Ravnborg > Reviewed-by: Laurent Pinchart > Signed-off-by: Dmitry Baryshkov > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 -- > 1 file changed, 16 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index d6dd4d99a229..b1b6ed3a8acc 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 > *pdata) > regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); > } > > -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) > +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector) > { > - if (pdata->connector->display_info.bpc <= 6) > + if (connector->display_info.bpc <= 6) > return 18; > else > return 24; > @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { > 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 > }; > > -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) > +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, > unsigned int bpp) > { > unsigned int bit_rate_khz, dp_rate_mhz; > unsigned int i; > @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct > ti_sn65dsi86 *pdata) > &pdata->bridge.encoder->crtc->state->adjusted_mode; > > /* Calculate minimum bit rate based on our pixel clock. */ > - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata); > + bit_rate_khz = mode->clock * bpp; > > /* Calculate minimum DP data rate, taking 80% as per DP spec */ > dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM, > @@ -1016,12 +1016,21 @@ static void ti_sn_bridge_atomic_enable(struct > drm_bridge *bridge, > struct drm_bridge_state > *old_bridge_state) > { > struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); > + struct drm_connector *connector; > const char *last_err_str = "No supported DP rate"; > unsigned int valid_rates; > int dp_rate_idx; > unsigned int val; > int ret = -EINVAL; > int max_dp_lanes; > + unsigned int bpp; > + > + connector = > drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state, > + bridge->encoder); > + if (!connector) { > + DRM_DEV_ERROR_RATELIMITED(pdata->dev, "Could not get the > connector\n"); >From the documentation of DRM_DEV_ERROR_RATELIMITED: * NOTE: this is deprecated in favor of drm_err_ratelimited() or * dev_err_ratelimited(). Can you fix this, so we do not introduce deprecated functions/macros. Sam > + return; > + } > > max_dp_lanes = ti_sn_get_max_lanes(pdata); > pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes); > @@ -1047,8 +1056,9 @@ static void ti_sn_bridge_atomic_enable(struct > drm_bridge *bridge, > drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, > DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); > > + bpp = ti_sn_bridge_get_bpp(connector); > /* Set the DP output format (18 bpp or 24 bpp) */ > - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0; > + val = bpp == 18 ? BPP_18_RGB : 0; > regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val); > > /* DP lane config */ > @@ -1059,7 +1069,7 @@ static void ti_sn_bridge_atomic_enable(struct > drm_bridge *bridge, > valid_rates = ti_sn_bridge_read_valid_rates(pdata); > > /* Train until we run out of rates */ > - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata); > + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp); >dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); >dp_rate_idx++) { > if (!(valid_rates & BIT(dp_rate_idx))) > -- > 2.35.1
Re: [PATCH v3 00/10] drm: Add support for low-color frame buffer formats
Hi Thomas, On Mon, Jul 11, 2022 at 10:50:00AM +0200, Thomas Zimmermann wrote: > Hi > > Am 09.07.22 um 15:38 schrieb Sam Ravnborg: > > Hi Geert, > > > > On Fri, Jul 08, 2022 at 08:20:45PM +0200, Geert Uytterhoeven wrote: > > > Hi all, > > > > > > A long outstanding issue with the DRM subsystem has been the lack of > > > support for low-color displays, as used typically on older desktop > > > systems, and on small embedded displays. > > For the patchset > > Acked-by: Thomas Zimemrmann > > > > > IT is super to have this addressed - thanks! > > > > > > > > This patch series adds support for color-indexed frame buffer formats > > > with 2, 4, and 16 colors. It has been tested on ARAnyM using a > > > work-in-progress Atari DRM driver supporting 2, 4, 16, 256, and 65536 > > > colors, with text console operation, fbtest, and modetest. > > > > > > Overview: > > >- Patch 1 introduces a helper, to be used by later patches in the > > > series, > > >- Patch 2 introduces a flag to indicate color-indexed formats, > > >- Patches 3 and 4 correct calculations of bits per pixel for sub-byte > > > pixel formats, > > >- Patches 5 and 6 introduce the new C[124] formats, > > >- Patch 7 fixes an untested code path, > > >- Patch 8 documents the use of "red" for light-on-dark displays, > > >- Patches 9 and 10 add more fourcc codes for light-on-dark and > > > dark-on-light frame buffer formats, which may be useful for e.g. the > > > ssd130x and repaper drivers. > > > > Applied all patches to drm-misc (drm-misc-next), including the last two > > RFC patches as we then have the formats ready when a user pops up. > > I know it's v3 already, but give people at least a workday for reviewing > before merging patches of this size and impact. Friday-evening patches are > not supposed to be merged on Saturday afternoons. Sorry for being too enthusiastic on this one. Will wait a bit more in the future for these kind of patches. Sam
Re: [RFC PATCH v2 1/2] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
On 11/07/2022 12:10, Sam Ravnborg wrote: Hi Dmitry, On Mon, Jul 11, 2022 at 10:37:32AM +0300, Dmitry Baryshkov wrote: Rather than reading the pdata->connector directly, fetch the connector using drm_atomic_state. This allows us to make pdata->connector optional (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR). Reviewed-by: Sam Ravnborg Reviewed-by: Laurent Pinchart Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index d6dd4d99a229..b1b6ed3a8acc 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); } -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector) { - if (pdata->connector->display_info.bpc <= 6) + if (connector->display_info.bpc <= 6) return 18; else return 24; @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 }; -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp) { unsigned int bit_rate_khz, dp_rate_mhz; unsigned int i; @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) &pdata->bridge.encoder->crtc->state->adjusted_mode; /* Calculate minimum bit rate based on our pixel clock. */ - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata); + bit_rate_khz = mode->clock * bpp; /* Calculate minimum DP data rate, taking 80% as per DP spec */ dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM, @@ -1016,12 +1016,21 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + struct drm_connector *connector; const char *last_err_str = "No supported DP rate"; unsigned int valid_rates; int dp_rate_idx; unsigned int val; int ret = -EINVAL; int max_dp_lanes; + unsigned int bpp; + + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state, +bridge->encoder); + if (!connector) { + DRM_DEV_ERROR_RATELIMITED(pdata->dev, "Could not get the connector\n"); From the documentation of DRM_DEV_ERROR_RATELIMITED: * NOTE: this is deprecated in favor of drm_err_ratelimited() or * dev_err_ratelimited(). Can you fix this, so we do not introduce deprecated functions/macros. Ack. I hesitated between the DRM_DEV and dev_ functions, but decided to use the family that was used by the rest of the driver. Will change this call to the dev_err in v3. Sam + return; + } max_dp_lanes = ti_sn_get_max_lanes(pdata); pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes); @@ -1047,8 +1056,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); + bpp = ti_sn_bridge_get_bpp(connector); /* Set the DP output format (18 bpp or 24 bpp) */ - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0; + val = bpp == 18 ? BPP_18_RGB : 0; regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val); /* DP lane config */ @@ -1059,7 +1069,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, valid_rates = ti_sn_bridge_read_valid_rates(pdata); /* Train until we run out of rates */ - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata); + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp); dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); dp_rate_idx++) { if (!(valid_rates & BIT(dp_rate_idx))) -- 2.35.1 -- With best wishes Dmitry
[RFC PATCH v3 0/2] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
An RFC (or rather RFT, Request-for-Testing) series adding support for DRM_BRIDGE_ATTACH_NO_CONNECTOR. Note, it was compile-tested only. This bridge is the last one used on the Qualcomm platforms (in upstream-supported devices) and thus it is the only bridge that prevents us from removing support for bridge-created connectors from MSM DSI code. Changes since RFC v2: - Changed DRM_DEV_ERR_RATELIMITED to dev_err_ratelimited() Changes since RFC v1: - Dropped first patch (conversion to atomic), corresponding patch has been already applied upstream - Added DRM_DEV_ERR_RATELIMITED to notifiy users/developers that corresponding connector was not found. Dmitry Baryshkov (2): drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR drivers/gpu/drm/bridge/ti-sn65dsi86.c | 40 --- 1 file changed, 24 insertions(+), 16 deletions(-) -- 2.35.1
[RFC PATCH v3 2/2] drm/bridge: ti-sn65dsi86: support DRM_BRIDGE_ATTACH_NO_CONNECTOR
Now as the driver does not depend on pdata->connector, add support for attaching the bridge with DRM_BRIDGE_ATTACH_NO_CONNECTOR. Reviewed-by: Sam Ravnborg Reviewed-by: Laurent Pinchart Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 18 -- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index b362a7bf4d97..369bf72717f6 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -698,11 +698,6 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); int ret; - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - pdata->aux.drm_dev = bridge->dev; ret = drm_dp_aux_register(&pdata->aux); if (ret < 0) { @@ -710,15 +705,18 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, return ret; } - /* We never want the next bridge to *also* create a connector: */ - flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR; - - /* Attach the next bridge */ + /* +* Attach the next bridge. +* We never want the next bridge to *also* create a connector. +*/ ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge, - &pdata->bridge, flags); + &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (ret < 0) goto err_initted_aux; + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) + return 0; + pdata->connector = drm_bridge_connector_init(pdata->bridge.dev, pdata->bridge.encoder); if (IS_ERR(pdata->connector)) { -- 2.35.1
[RFC PATCH v3 1/2] drm/bridge: ti-sn65dsi86: fetch bpc using drm_atomic_state
Rather than reading the pdata->connector directly, fetch the connector using drm_atomic_state. This allows us to make pdata->connector optional (and thus supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR). Reviewed-by: Sam Ravnborg Reviewed-by: Laurent Pinchart Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 -- 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index d6dd4d99a229..b362a7bf4d97 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -779,9 +779,9 @@ static void ti_sn_bridge_set_dsi_rate(struct ti_sn65dsi86 *pdata) regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val); } -static unsigned int ti_sn_bridge_get_bpp(struct ti_sn65dsi86 *pdata) +static unsigned int ti_sn_bridge_get_bpp(struct drm_connector *connector) { - if (pdata->connector->display_info.bpc <= 6) + if (connector->display_info.bpc <= 6) return 18; else return 24; @@ -796,7 +796,7 @@ static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 0, 1620, 2160, 2430, 2700, 3240, 4320, 5400 }; -static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) +static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata, unsigned int bpp) { unsigned int bit_rate_khz, dp_rate_mhz; unsigned int i; @@ -804,7 +804,7 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata) &pdata->bridge.encoder->crtc->state->adjusted_mode; /* Calculate minimum bit rate based on our pixel clock. */ - bit_rate_khz = mode->clock * ti_sn_bridge_get_bpp(pdata); + bit_rate_khz = mode->clock * bpp; /* Calculate minimum DP data rate, taking 80% as per DP spec */ dp_rate_mhz = DIV_ROUND_UP(bit_rate_khz * DP_CLK_FUDGE_NUM, @@ -1016,12 +1016,21 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); + struct drm_connector *connector; const char *last_err_str = "No supported DP rate"; unsigned int valid_rates; int dp_rate_idx; unsigned int val; int ret = -EINVAL; int max_dp_lanes; + unsigned int bpp; + + connector = drm_atomic_get_new_connector_for_encoder(old_bridge_state->base.state, +bridge->encoder); + if (!connector) { + dev_err_ratelimited(pdata->dev, "Could not get the connector\n"); + return; + } max_dp_lanes = ti_sn_get_max_lanes(pdata); pdata->dp_lanes = min(pdata->dp_lanes, max_dp_lanes); @@ -1047,8 +1056,9 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, drm_dp_dpcd_writeb(&pdata->aux, DP_EDP_CONFIGURATION_SET, DP_ALTERNATE_SCRAMBLER_RESET_ENABLE); + bpp = ti_sn_bridge_get_bpp(connector); /* Set the DP output format (18 bpp or 24 bpp) */ - val = (ti_sn_bridge_get_bpp(pdata) == 18) ? BPP_18_RGB : 0; + val = bpp == 18 ? BPP_18_RGB : 0; regmap_update_bits(pdata->regmap, SN_DATA_FORMAT_REG, BPP_18_RGB, val); /* DP lane config */ @@ -1059,7 +1069,7 @@ static void ti_sn_bridge_atomic_enable(struct drm_bridge *bridge, valid_rates = ti_sn_bridge_read_valid_rates(pdata); /* Train until we run out of rates */ - for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata); + for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata, bpp); dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); dp_rate_idx++) { if (!(valid_rates & BIT(dp_rate_idx))) -- 2.35.1
Re: [PATCH] drm/mgag200: Don't read-back PCI option register before writing
On 08/07/2022 09:21, Thomas Zimmermann wrote: Remove the read operation from mgag200_init_pci_options(). It was incorrectly added while refactoring the code. Reading the PCI option register clears the register's new value and subsequently leads to re-writing the old value. Looks good to me. Reviewed-by: Jocelyn Falempe Signed-off-by: Thomas Zimmermann Fixes: ce19021fd99a ("drm/mgag200: Move PCI-option setup into model-specific code") Cc: Thomas Zimmermann Cc: Jocelyn Falempe Cc: Dave Airlie Cc: dri-devel@lists.freedesktop.org --- drivers/gpu/drm/mgag200/mgag200_drv.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 73e8e4e9e54b..251a1bb648cc 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -28,12 +28,6 @@ int mgag200_init_pci_options(struct pci_dev *pdev, u32 option, u32 option2) struct device *dev = &pdev->dev; int err; - err = pci_read_config_dword(pdev, PCI_MGA_OPTION, &option); - if (err != PCIBIOS_SUCCESSFUL) { - dev_err(dev, "pci_read_config_dword(PCI_MGA_OPTION) failed: %d\n", err); - return pcibios_err_to_errno(err); - } - err = pci_write_config_dword(pdev, PCI_MGA_OPTION, option); if (err != PCIBIOS_SUCCESSFUL) { dev_err(dev, "pci_write_config_dword(PCI_MGA_OPTION) failed: %d\n", err);
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
Hi Thomas, On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: > > The mode parsing code recognizes named modes only if they are explicitly > > listed in the internal whitelist, which is currently limited to "NTSC" > > and "PAL". > > > > Provide a mechanism for drivers to override this list to support custom > > mode names. > > > > Ideally, this list should just come from the driver's actual list of > > modes, but connector->probed_modes is not yet populated at the time of > > parsing. > > I've looked for code that uses these names, couldn't find any. How is this > being used in practice? For example, if I say "PAL" on the command line, is > there DRM code that fills in the PAL mode parameters? We have some code to deal with this in sun4i: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292 It's a bit off topic, but for TV standards, I'm still not sure what the best course of action is. There's several interactions that make this a bit troublesome: * Some TV standards differ by their mode (ie, PAL vs NSTC), but some other differ by parameters that are not part of drm_display_mode (NTSC vs NSTC-J where the only difference is the black and blanking signal levels for example). * The mode names allow to provide a fairly convenient way to add that extra information, but the userspace is free to create its own mode and might omit the mode name entirely. So in the code above, if the name has been preserved we match by name, but we fall back to matching by mode if it hasn't been, which in this case means that we have no way to differentiate between NTSC, NTSC-J, PAL-M in this case. We have some patches downstream for the RaspberryPi that has the TV standard as a property. There's a few extra logic required for the userspace (like setting the PAL property, with the NTSC mode) so I'm not sure it's preferable. Or we could do something like a property to try that standard, and another that reports the one we actually chose. > And another question I have is whether this whitelist belongs into the > driver at all. Standard modes exist independent from drivers or hardware. > Shouldn't there simply be a global list of all possible mode names? Drivers > would filter out the unsupported modes anyway. We should totally do something like that, yeah Maxime signature.asc Description: PGP signature
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
Hi Thomas, On Mon, Jul 11, 2022 at 11:03 AM Thomas Zimmermann wrote: > Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: > > The mode parsing code recognizes named modes only if they are explicitly > > listed in the internal whitelist, which is currently limited to "NTSC" > > and "PAL". > > > > Provide a mechanism for drivers to override this list to support custom > > mode names. > > > > Ideally, this list should just come from the driver's actual list of > > modes, but connector->probed_modes is not yet populated at the time of > > parsing. > > I've looked for code that uses these names, couldn't find any. How is > this being used in practice? For example, if I say "PAL" on the command > line, is there DRM code that fills in the PAL mode parameters? I guess Maxime knows, as he added the whitelist? Reading the description of commit 3764137906a5acec ("drm/modes: Introduce a whitelist for the named modes"), it looks like this is more about preventing the parser from taking any string as a random mode, than about adding support for "PAL" or "NTSC"? Note that drivers/gpu/drm/i915/display/intel_tv.c defines an array of tv_modes[], including "PAL", so perhaps these end up as named modes? > And another question I have is whether this whitelist belongs into the > driver at all. Standard modes exist independent from drivers or > hardware. Shouldn't there simply be a global list of all possible mode > names? Drivers would filter out the unsupported modes anyway. For standard modes, I agree. And these are usually specified by resolution and refresh rate (e.g. "640x480@60", instead of "480p"). But legacy hardware may have very limited support for programmable pixel clocks (e.g. Amiga is limited to pixel clocks of 7, 14, or 28 MHz), so the standard modes are a bad match, or may not work at all. Hence drivers may need to provide their own modes, but it seems wrong to me to make these non-standard modes global, and possibly pollute the experience for everyone. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
[PATCH v2 0/4] drm/msm/dsi: stop using drm_panel directly
This series superseeds two existing patch series ([1] and [2]) and merges them together in a single patchset to demonstrate necessity for the first three patches. The drm/msm/dsi driver has two separate code paths, one utilizing a chain of drm_bridges and another one for directly interfacing the drm_panel. We would have dropped the second path completely but for the DSI DSC pps data. To properly support DSI DSC the DSI sink driver (panel) has to pass DSC pps data to the source (DSI host). The commit 0f40ba48de3b ("drm/msm/dsi: Pass DSC params to drm_panel") added a pointer to the DSC data to the struct drm_panel. However this is not ideal. First, this keeps DSC-supporting DSI sink bridges out of the picture (like ANX7625 which support DSC decoding on the MIPI DSI inputs). Second, this does not play well with the panel_bridge. Drivers depending solely on the bridge chains will still have to lookup panel and fetch data from it. Last, but not least, the DSC data is not relevant for the wide variety of panels including DPI and LVDS panels. To solve all these problems, move struct drm_dsc_config pointer from struct drm_panel to struct mipi_host_device. This way MIPI DSI host driver receives DSC data during attach callback without additional lookups. The last commit drops the drm_panel code from msm/dsi driver, providing code simplification. Changes since v1 (of both patchsets): - Minor cleanups in the msm/dsi driver, dropping more now-unused code. [1] https://patchwork.freedesktop.org/series/103411/ [2] https://patchwork.freedesktop.org/series/105995/ Dmitry Baryshkov (4): drm/mipi-dsi: pass DSC data through the struct mipi_dsi_device drm/msm/dsi: fetch DSC pps payload from struct mipi_dsi_device drm/panel: drop DSC pps pointer drm/msm/dsi: switch to DRM_PANEL_BRIDGE drivers/gpu/drm/msm/dsi/dsi.c | 38 +--- drivers/gpu/drm/msm/dsi/dsi.h | 14 +- drivers/gpu/drm/msm/dsi/dsi_host.c| 50 ++--- drivers/gpu/drm/msm/dsi/dsi_manager.c | 264 ++ include/drm/drm_mipi_dsi.h| 2 + include/drm/drm_panel.h | 7 - 6 files changed, 39 insertions(+), 336 deletions(-) -- 2.35.1
[PATCH v2 1/4] drm/mipi-dsi: pass DSC data through the struct mipi_dsi_device
The commit 0f40ba48de3b ("drm/msm/dsi: Pass DSC params to drm_panel") added a pointer to the DSC data to the struct drm_panel. However DSC support is not limited to the DSI panels. MIPI DSI bridges can also consume DSC command streams. Thus add struct drm_dsc_config pointer to the struct mipi_dsi_device. Signed-off-by: Dmitry Baryshkov --- include/drm/drm_mipi_dsi.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h index 91a164bdd8f3..bb20bc27ce87 100644 --- a/include/drm/drm_mipi_dsi.h +++ b/include/drm/drm_mipi_dsi.h @@ -179,6 +179,7 @@ struct mipi_dsi_device_info { * @lp_rate: maximum lane frequency for low power mode in hertz, this should * be set to the real limits of the hardware, zero is only accepted for * legacy drivers + * @dsc: panel/bridge DSC pps payload to be sent */ struct mipi_dsi_device { struct mipi_dsi_host *host; @@ -191,6 +192,7 @@ struct mipi_dsi_device { unsigned long mode_flags; unsigned long hs_rate; unsigned long lp_rate; + struct drm_dsc_config *dsc; }; #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:" -- 2.35.1
[PATCH v2 2/4] drm/msm/dsi: fetch DSC pps payload from struct mipi_dsi_device
Now that struct mipi_dsi_device provides DSC data, fetch it from the mentioned struct rather than from the struct drm_panel itself. This would allow supporting MIPI DSI bridges handling DSC on their input side. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi_host.c | 25 +++-- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index a34078497af1..fb5ab6c718c8 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -1686,6 +1686,17 @@ static int dsi_host_attach(struct mipi_dsi_host *host, msm_host->lanes = dsi->lanes; msm_host->format = dsi->format; msm_host->mode_flags = dsi->mode_flags; + if (dsi->dsc) { + struct msm_display_dsc_config *dsc = msm_host->dsc; + + if (!dsc) { + dsc = devm_kzalloc(&msm_host->pdev->dev, sizeof(*dsc), GFP_KERNEL); + if (!dsc) + return -ENOMEM; + dsc->drm = dsi->dsc; + msm_host->dsc = dsc; + } + } /* Some gpios defined in panel DT need to be controlled by host */ ret = dsi_host_init_panel_gpios(msm_host, &dsi->dev); @@ -2159,23 +2170,9 @@ int msm_dsi_host_modeset_init(struct mipi_dsi_host *host, { struct msm_dsi_host *msm_host = to_msm_dsi_host(host); const struct msm_dsi_cfg_handler *cfg_hnd = msm_host->cfg_hnd; - struct drm_panel *panel; int ret; msm_host->dev = dev; - panel = msm_dsi_host_get_panel(&msm_host->base); - - if (!IS_ERR(panel) && panel->dsc) { - struct msm_display_dsc_config *dsc = msm_host->dsc; - - if (!dsc) { - dsc = devm_kzalloc(&msm_host->pdev->dev, sizeof(*dsc), GFP_KERNEL); - if (!dsc) - return -ENOMEM; - dsc->drm = panel->dsc; - msm_host->dsc = dsc; - } - } ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K); if (ret) { -- 2.35.1
[PATCH v2 4/4] drm/msm/dsi: switch to DRM_PANEL_BRIDGE
Currently the DSI driver has two separate paths: one if the next device in a chain is a bridge and another one if the panel is connected directly to the DSI host. Simplify the code path by using panel-bridge driver (already selected in Kconfig) and dropping support for handling the panel directly. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/dsi/dsi.c | 38 +--- drivers/gpu/drm/msm/dsi/dsi.h | 14 +- drivers/gpu/drm/msm/dsi/dsi_host.c| 25 --- drivers/gpu/drm/msm/dsi/dsi_manager.c | 264 ++ 4 files changed, 26 insertions(+), 315 deletions(-) diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 1625328fa430..3c53e28092db 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -6,14 +6,6 @@ #include "dsi.h" #include "dsi_cfg.h" -struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi) -{ - if (!msm_dsi || !msm_dsi_device_connected(msm_dsi)) - return NULL; - - return msm_dsi->encoder; -} - bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi) { unsigned long host_flags = msm_dsi_host_get_mode_flags(msm_dsi->host); @@ -220,7 +212,7 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, struct drm_encoder *encoder) { struct msm_drm_private *priv; - struct drm_bridge *ext_bridge; + struct drm_connector *connector; int ret; if (WARN_ON(!encoder) || WARN_ON(!msm_dsi) || WARN_ON(!dev)) @@ -254,26 +246,12 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, goto fail; } - /* -* check if the dsi encoder output is connected to a panel or an -* external bridge. We create a connector only if we're connected to a -* drm_panel device. When we're connected to an external bridge, we -* assume that the drm_bridge driver will create the connector itself. -*/ - ext_bridge = msm_dsi_host_get_bridge(msm_dsi->host); - - if (ext_bridge) - msm_dsi->connector = - msm_dsi_manager_ext_bridge_init(msm_dsi->id); - else - msm_dsi->connector = - msm_dsi_manager_connector_init(msm_dsi->id); - - if (IS_ERR(msm_dsi->connector)) { - ret = PTR_ERR(msm_dsi->connector); + connector = msm_dsi_manager_ext_bridge_init(msm_dsi->id); + + if (IS_ERR(connector)) { + ret = PTR_ERR(connector); DRM_DEV_ERROR(dev->dev, "failed to create dsi connector: %d\n", ret); - msm_dsi->connector = NULL; goto fail; } @@ -287,12 +265,6 @@ int msm_dsi_modeset_init(struct msm_dsi *msm_dsi, struct drm_device *dev, msm_dsi->bridge = NULL; } - /* don't destroy connector if we didn't make it */ - if (msm_dsi->connector && !msm_dsi->external_bridge) - msm_dsi->connector->funcs->destroy(msm_dsi->connector); - - msm_dsi->connector = NULL; - return ret; } diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 580a1e6358bf..41630b8f5358 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -12,7 +12,6 @@ #include #include #include -#include #include "msm_drv.h" #include "disp/msm_disp_snapshot.h" @@ -49,8 +48,6 @@ struct msm_dsi { struct drm_device *dev; struct platform_device *pdev; - /* connector managed by us when we're connected to a drm_panel */ - struct drm_connector *connector; /* internal dsi bridge attached to MDP interface */ struct drm_bridge *bridge; @@ -58,10 +55,8 @@ struct msm_dsi { struct msm_dsi_phy *phy; /* -* panel/external_bridge connected to dsi bridge output, only one of the -* two can be valid at a time +* external_bridge connected to dsi bridge output */ - struct drm_panel *panel; struct drm_bridge *external_bridge; struct device *phy_dev; @@ -76,7 +71,6 @@ struct msm_dsi { /* dsi manager */ struct drm_bridge *msm_dsi_manager_bridge_init(u8 id); void msm_dsi_manager_bridge_destroy(struct drm_bridge *bridge); -struct drm_connector *msm_dsi_manager_connector_init(u8 id); struct drm_connector *msm_dsi_manager_ext_bridge_init(u8 id); int msm_dsi_manager_cmd_xfer(int id, const struct mipi_dsi_msg *msg); bool msm_dsi_manager_cmd_xfer_trigger(int id, u32 dma_base, u32 len); @@ -87,11 +81,9 @@ void msm_dsi_manager_tpg_enable(void); /* msm dsi */ static inline bool msm_dsi_device_connected(struct msm_dsi *msm_dsi) { - return msm_dsi->panel || msm_dsi->external_bridge; + return msm_dsi->external_bridge; } -struct drm_encoder *msm_dsi_get_encoder(struct msm_dsi *msm_dsi); - /* dsi host */ struct msm_dsi_host; int msm_dsi_host_xfer_prepare
[PATCH v2 3/4] drm/panel: drop DSC pps pointer
Complete the move of DSC data pointer from struct drm_panel to struct mipi_dsi_device. Signed-off-by: Dmitry Baryshkov --- include/drm/drm_panel.h | 7 --- 1 file changed, 7 deletions(-) diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 3a271128c078..994bfcdd84c5 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -188,13 +188,6 @@ struct drm_panel { * Panel entry in registry. */ struct list_head list; - - /** -* @dsc: -* -* Panel DSC pps payload to be sent -*/ - struct drm_dsc_config *dsc; }; void drm_panel_init(struct drm_panel *panel, struct device *dev, -- 2.35.1
Re: [PATCH 3/5] dma-buf: return only unsignaled fences in dma_fence_unwrap_for_each v3
Hi Christian, I'm sorry for digging this one out so late. On 06.05.2022 16:10, Christian König wrote: dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well. v2: fix missing walk over the array v3: massively simplify the patch and actually update the description. Signed-off-by: Christian König --- include/linux/dma-fence-unwrap.h | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned. + * + * Note that signalled fences are opportunistically filtered out, which + * means the iteration is potentially over no fence at all. */ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ -fence = dma_fence_unwrap_next(cursor)) +fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled - sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all - sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling) Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help. All the best, Karolina - [1] - reproducible locally, but can be also seen in the CI: https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=igt@sw_sync
[PATCH v5 05/10] drm/msm/dp: use the eDP bridge ops to validate eDP modes
The eDP and DP interfaces shared the bridge operations and the eDP specific changes were implemented under is_edp check. To add psr support for eDP, we started using a new set of eDP bridge ops. We are moving the eDP specific code in the dp_bridge_mode_valid function to a new eDP function, edp_bridge_mode_valid under the eDP bridge ops. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/dp/dp_display.c | 8 drivers/gpu/drm/msm/dp/dp_drm.c | 34 +- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 64a6254..2b3ec6b 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -986,14 +986,6 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge, return -EINVAL; } - /* -* The eDP controller currently does not have a reliable way of -* enabling panel power to read sink capabilities. So, we rely -* on the panel driver to populate only supported modes for now. -*/ - if (dp->is_edp) - return MODE_OK; - if (mode->clock > DP_MAX_PIXEL_CLK_KHZ) return MODE_BAD; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 8e2cb35..eb08ee8 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -183,12 +183,44 @@ static void edp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge, dp_bridge_atomic_post_disable(drm_bridge, old_bridge_state); } +/** + * edp_bridge_mode_valid - callback to determine if specified mode is valid + * @bridge: Pointer to drm bridge structure + * @info: display info + * @mode: Pointer to drm mode structure + * Returns: Validity status for specified mode + */ +static enum drm_mode_status edp_bridge_mode_valid(struct drm_bridge *bridge, + const struct drm_display_info *info, + const struct drm_display_mode *mode) +{ + struct msm_dp *dp; + int mode_pclk_khz = mode->clock; + + dp = to_dp_bridge(bridge)->dp_display; + + if (!dp || !mode_pclk_khz || !dp->connector) { + DRM_ERROR("invalid params\n"); + return -EINVAL; + } + + if (mode->clock > DP_MAX_PIXEL_CLK_KHZ) + return MODE_CLOCK_HIGH; + + /* +* The eDP controller currently does not have a reliable way of +* enabling panel power to read sink capabilities. So, we rely +* on the panel driver to populate only supported modes for now. +*/ + return MODE_OK; +} + static const struct drm_bridge_funcs edp_bridge_ops = { .atomic_enable = edp_bridge_atomic_enable, .atomic_disable = edp_bridge_atomic_disable, .atomic_post_disable = edp_bridge_atomic_post_disable, .mode_set = dp_bridge_mode_set, - .mode_valid = dp_bridge_mode_valid, + .mode_valid = edp_bridge_mode_valid, .atomic_reset = drm_atomic_helper_bridge_reset, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, -- 2.7.4
[PATCH v5 04/10] drm/msm/dp: Add basic PSR support for eDP
Add support for basic panel self refresh (PSR) feature for eDP. Add a new interface to set PSR state in the sink from DPU. Program the eDP controller to issue PSR enter and exit SDP to the sink. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/dp/dp_catalog.c | 81 + drivers/gpu/drm/msm/dp/dp_catalog.h | 4 ++ drivers/gpu/drm/msm/dp/dp_ctrl.c| 73 +++ drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 + drivers/gpu/drm/msm/dp/dp_display.c | 14 drivers/gpu/drm/msm/dp/dp_display.h | 2 + drivers/gpu/drm/msm/dp/dp_drm.c | 137 +++- drivers/gpu/drm/msm/dp/dp_drm.h | 9 ++- drivers/gpu/drm/msm/dp/dp_link.c| 36 ++ drivers/gpu/drm/msm/dp/dp_panel.c | 22 ++ drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++ drivers/gpu/drm/msm/dp/dp_reg.h | 27 +++ 12 files changed, 410 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 7257515..b9021ed 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -47,6 +47,14 @@ #define DP_INTERRUPT_STATUS2_MASK \ (DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT) +#define DP_INTERRUPT_STATUS4 \ + (PSR_UPDATE_INT | PSR_CAPTURE_INT | PSR_EXIT_INT | \ + PSR_UPDATE_ERROR_INT | PSR_WAKE_ERROR_INT) + +#define DP_INTERRUPT_MASK4 \ + (PSR_UPDATE_MASK | PSR_CAPTURE_MASK | PSR_EXIT_MASK | \ + PSR_UPDATE_ERROR_MASK | PSR_WAKE_ERROR_MASK) + struct dp_catalog_private { struct device *dev; struct drm_device *drm_dev; @@ -359,6 +367,24 @@ void dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog) ln_mapping); } +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog, + bool enable) +{ + u32 val; + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + + val = dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + val &= ~DP_MAINLINK_CTRL_ENABLE; + + if (enable) + val |= DP_MAINLINK_CTRL_ENABLE; + else + val &= ~DP_MAINLINK_CTRL_ENABLE; + + dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val); +} + void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool enable) { @@ -610,6 +636,47 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); } +static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog) +{ + /* trigger sdp */ + dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); + dp_write_link(catalog, MMSS_DP_SDP_CFG3, !UPDATE_SDP); +} + +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 config; + + /* enable PSR1 function */ + config = dp_read_link(catalog, REG_PSR_CONFIG); + config |= PSR1_SUPPORTED; + dp_write_link(catalog, REG_PSR_CONFIG, config); + + dp_write_ahb(catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); + dp_catalog_enable_sdp(catalog); +} + +void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 cmd; + + cmd = dp_read_link(catalog, REG_PSR_CMD); + + cmd &= ~(PSR_ENTER | PSR_EXIT); + + if (enter) + cmd |= PSR_ENTER; + else + cmd |= PSR_EXIT; + + dp_catalog_enable_sdp(catalog); + dp_write_link(catalog, REG_PSR_CMD, cmd); +} + u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, @@ -645,6 +712,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) return isr & (mask | ~DP_DP_HPD_INT_MASK); } +int dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 intr, intr_ack; + + intr = dp_read_ahb(catalog, REG_DP_INTR_STATUS4); + intr_ack = (intr & DP_INTERRUPT_STATUS4) + << DP_INTERRUPT_STATUS_ACK_SHIFT; + dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack); + + return intr; +} + int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 1f7
[PATCH v5 08/10] drm/msm/disp/dpu1: use atomic enable/disable callbacks for encoder functions
Use atomic variants for encoder callback functions such that certain states like self-refresh can be accessed as part of enable/disable sequence. Reviewed-by: Dmitry Baryshkov Signed-off-by: Kalyan Thota Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 8fb3e15..cd7d568 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1130,7 +1130,8 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc) mutex_unlock(&dpu_enc->enc_lock); } -static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) +static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc, + struct drm_atomic_state *state) { struct dpu_encoder_virt *dpu_enc = NULL; int ret = 0; @@ -1166,7 +1167,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc) mutex_unlock(&dpu_enc->enc_lock); } -static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc) +static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, + struct drm_atomic_state *state) { struct dpu_encoder_virt *dpu_enc = NULL; int i = 0; @@ -2331,8 +2333,8 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t) static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = { .atomic_mode_set = dpu_encoder_virt_atomic_mode_set, - .disable = dpu_encoder_virt_disable, - .enable = dpu_encoder_virt_enable, + .atomic_disable = dpu_encoder_virt_atomic_disable, + .atomic_enable = dpu_encoder_virt_atomic_enable, .atomic_check = dpu_encoder_virt_atomic_check, }; -- 2.7.4
[PATCH v5 01/10] drm/msm/disp/dpu1: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
Update crtc retrieval from dpu_enc to dpu_enc connector state, since new links get set as part of the dpu enc virt mode set. The dpu_enc->crtc cache is no more needed, hence cleaning it as part of this change. Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 4 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 29 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 3 files changed, 14 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index b56f777..f91e3d1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -972,7 +972,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, */ if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO) release_bandwidth = true; - dpu_encoder_assign_crtc(encoder, NULL); } /* wait for frame_event_done completion */ @@ -1042,9 +1041,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); dpu_crtc->enabled = true; - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) - dpu_encoder_assign_crtc(encoder, crtc); - /* Enable/restore vblank irq handling */ drm_crtc_vblank_on(crtc); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 52516eb..8fb3e15 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1245,6 +1245,7 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, struct dpu_encoder_phys *phy_enc) { struct dpu_encoder_virt *dpu_enc = NULL; + struct drm_crtc *crtc; unsigned long lock_flags; if (!drm_enc || !phy_enc) @@ -1253,9 +1254,14 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, DPU_ATRACE_BEGIN("encoder_vblank_callback"); dpu_enc = to_dpu_encoder_virt(drm_enc); + if (!dpu_enc->connector || !dpu_enc->connector->state) + return; + + crtc = dpu_enc->connector->state->crtc; + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc) - dpu_crtc_vblank_callback(dpu_enc->crtc); + if (crtc) + dpu_crtc_vblank_callback(crtc); spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); atomic_inc(&phy_enc->vsync_cnt); @@ -1280,29 +1286,22 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, DPU_ATRACE_END("encoder_underrun_callback"); } -void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc) -{ - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); - unsigned long lock_flags; - - spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - /* crtc should always be cleared before re-assigning */ - WARN_ON(crtc && dpu_enc->crtc); - dpu_enc->crtc = crtc; - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); -} - void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc, bool enable) { struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); + struct drm_crtc *new_crtc; unsigned long lock_flags; int i; trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); + if (!dpu_enc->connector || !dpu_enc->connector->state) + return; + + new_crtc = dpu_enc->connector->state->crtc; spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc != crtc) { + if (!new_crtc || crtc != crtc) { spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); return; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 781d41c..edba815 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -39,14 +39,6 @@ struct msm_display_info { }; /** - * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to - * @encoder: encoder pointer - * @crtc: crtc pointer - */ -void dpu_encoder_assign_crtc(struct drm_encoder *encoder, -struct drm_crtc *crtc); - -/** * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or off if * the encoder is assigned to the given crtc * @encoder: encoder pointer -- 2.7.4
[PATCH v5 03/10] drm/msm/dp: use atomic callbacks for DP bridge ops
Use atomic variants for DP bridge callback functions so that the atomic state can be accessed in the interface drivers. The atomic state will help the driver find out if the display is in self refresh state. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/dp/dp_display.c | 9 ++--- drivers/gpu/drm/msm/dp/dp_drm.c | 18 +++--- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index bce7793..5bd6677 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1652,7 +1652,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, return 0; } -void dp_bridge_enable(struct drm_bridge *drm_bridge) +void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge, +struct drm_bridge_state *old_bridge_state) { struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge); struct msm_dp *dp = dp_bridge->dp_display; @@ -1716,7 +1717,8 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) mutex_unlock(&dp_display->event_mutex); } -void dp_bridge_disable(struct drm_bridge *drm_bridge) +void dp_bridge_atomic_disable(struct drm_bridge *drm_bridge, + struct drm_bridge_state *old_bridge_state) { struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge); struct msm_dp *dp = dp_bridge->dp_display; @@ -1727,7 +1729,8 @@ void dp_bridge_disable(struct drm_bridge *drm_bridge) dp_ctrl_push_idle(dp_display->ctrl); } -void dp_bridge_post_disable(struct drm_bridge *drm_bridge) +void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge, + struct drm_bridge_state *old_bridge_state) { struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge); struct msm_dp *dp = dp_bridge->dp_display; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 62d58b9..9d0fc74 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -61,13 +61,17 @@ static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector * } static const struct drm_bridge_funcs dp_bridge_ops = { - .enable = dp_bridge_enable, - .disable = dp_bridge_disable, - .post_disable = dp_bridge_post_disable, - .mode_set = dp_bridge_mode_set, - .mode_valid = dp_bridge_mode_valid, - .get_modes= dp_bridge_get_modes, - .detect = dp_bridge_detect, + .atomic_enable = dp_bridge_atomic_enable, + .atomic_disable = dp_bridge_atomic_disable, + .atomic_post_disable = dp_bridge_atomic_post_disable, + .mode_set = dp_bridge_mode_set, + .mode_valid = dp_bridge_mode_valid, + .get_modes = dp_bridge_get_modes, + .detect = dp_bridge_detect, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_check = edp_bridge_atomic_check, }; struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, -- 2.7.4
[PATCH v5 04/10] drm/msm/dp: add basic PSR support for eDP
Add support for basic panel self refresh (PSR) feature for eDP. Add a new interface to set PSR state in the sink from DPU. Program the eDP controller to issue PSR enter and exit SDP to the sink. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/dp/dp_catalog.c | 81 + drivers/gpu/drm/msm/dp/dp_catalog.h | 4 ++ drivers/gpu/drm/msm/dp/dp_ctrl.c| 78 - drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 + drivers/gpu/drm/msm/dp/dp_display.c | 14 drivers/gpu/drm/msm/dp/dp_display.h | 2 + drivers/gpu/drm/msm/dp/dp_drm.c | 136 +++- drivers/gpu/drm/msm/dp/dp_drm.h | 9 ++- drivers/gpu/drm/msm/dp/dp_link.c| 36 ++ drivers/gpu/drm/msm/dp/dp_panel.c | 22 ++ drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++ drivers/gpu/drm/msm/dp/dp_reg.h | 27 +++ 12 files changed, 411 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 7257515..b9021ed 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -47,6 +47,14 @@ #define DP_INTERRUPT_STATUS2_MASK \ (DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT) +#define DP_INTERRUPT_STATUS4 \ + (PSR_UPDATE_INT | PSR_CAPTURE_INT | PSR_EXIT_INT | \ + PSR_UPDATE_ERROR_INT | PSR_WAKE_ERROR_INT) + +#define DP_INTERRUPT_MASK4 \ + (PSR_UPDATE_MASK | PSR_CAPTURE_MASK | PSR_EXIT_MASK | \ + PSR_UPDATE_ERROR_MASK | PSR_WAKE_ERROR_MASK) + struct dp_catalog_private { struct device *dev; struct drm_device *drm_dev; @@ -359,6 +367,24 @@ void dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog) ln_mapping); } +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog, + bool enable) +{ + u32 val; + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + + val = dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + val &= ~DP_MAINLINK_CTRL_ENABLE; + + if (enable) + val |= DP_MAINLINK_CTRL_ENABLE; + else + val &= ~DP_MAINLINK_CTRL_ENABLE; + + dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val); +} + void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool enable) { @@ -610,6 +636,47 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); } +static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog) +{ + /* trigger sdp */ + dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); + dp_write_link(catalog, MMSS_DP_SDP_CFG3, !UPDATE_SDP); +} + +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 config; + + /* enable PSR1 function */ + config = dp_read_link(catalog, REG_PSR_CONFIG); + config |= PSR1_SUPPORTED; + dp_write_link(catalog, REG_PSR_CONFIG, config); + + dp_write_ahb(catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); + dp_catalog_enable_sdp(catalog); +} + +void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 cmd; + + cmd = dp_read_link(catalog, REG_PSR_CMD); + + cmd &= ~(PSR_ENTER | PSR_EXIT); + + if (enter) + cmd |= PSR_ENTER; + else + cmd |= PSR_EXIT; + + dp_catalog_enable_sdp(catalog); + dp_write_link(catalog, REG_PSR_CMD, cmd); +} + u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, @@ -645,6 +712,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) return isr & (mask | ~DP_DP_HPD_INT_MASK); } +int dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 intr, intr_ack; + + intr = dp_read_ahb(catalog, REG_DP_INTR_STATUS4); + intr_ack = (intr & DP_INTERRUPT_STATUS4) + << DP_INTERRUPT_STATUS_ACK_SHIFT; + dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack); + + return intr; +} + int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h index 1
[PATCH v5 09/10] drm/msm/disp/dpu1: add PSR support for eDP interface in dpu driver
Enable PSR on eDP interface using drm self-refresh librabry. This patch uses a trigger from self-refresh library to enter/exit into PSR, when there are no updates from framework. Signed-off-by: Kalyan Thota Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 13 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 16 +++- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index f91e3d1..eb3915a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -18,6 +18,7 @@ #include #include #include +#include #include "dpu_kms.h" #include "dpu_hw_lm.h" @@ -961,6 +962,9 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, DRM_DEBUG_KMS("crtc%d\n", crtc->base.id); + if (old_crtc_state->self_refresh_active) + return; + /* Disable/save vblank irq handling */ drm_crtc_vblank_off(crtc); @@ -1521,7 +1525,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, { struct drm_crtc *crtc = NULL; struct dpu_crtc *dpu_crtc = NULL; - int i; + int i, ret; dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL); if (!dpu_crtc) @@ -1558,6 +1562,13 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane, /* initialize event handling */ spin_lock_init(&dpu_crtc->event_lock); + ret = drm_self_refresh_helper_init(crtc); + if (ret) { + DPU_ERROR("Failed to initialize %s with self-refresh helpers %d\n", + crtc->name, ret); + return ERR_PTR(ret); + } + DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name); return crtc; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index cd7d568..726acc9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -12,6 +12,7 @@ #include #include +#include #include #include #include @@ -1171,11 +1172,24 @@ static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc, struct drm_atomic_state *state) { struct dpu_encoder_virt *dpu_enc = NULL; + struct drm_crtc *crtc; + struct drm_crtc_state *old_state = NULL; int i = 0; dpu_enc = to_dpu_encoder_virt(drm_enc); DPU_DEBUG_ENC(dpu_enc, "\n"); + crtc = drm_atomic_get_old_crtc_for_encoder(state, drm_enc); + if (crtc) + old_state = drm_atomic_get_old_crtc_state(state, crtc); + + /* +* The encoder is already disabled if self refresh mode was set earlier, +* in the old_state for the corresponding crtc. +*/ + if (old_state && old_state->self_refresh_active) + return; + mutex_lock(&dpu_enc->enc_lock); dpu_enc->enabled = false; @@ -1303,7 +1317,7 @@ void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, new_crtc = dpu_enc->connector->state->crtc; spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (!new_crtc || crtc != crtc) { + if (!new_crtc || new_crtc != crtc) { spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); return; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index bce4764..cc0a674 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -507,7 +507,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms, return; } - if (!crtc->state->active) { + if (!drm_atomic_crtc_effectively_active(crtc->state)) { DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id); return; } -- 2.7.4
[PATCH v5 06/10] drm/bridge: use atomic enable/disable callbacks for panel bridge
Use atomic variants for panel bridge callback functions such that certain states like self-refresh can be accessed as part of enable/disable sequence. Reviewed-by: Dmitry Baryshkov Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/bridge/panel.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ee563e..eeb9546 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -108,28 +108,32 @@ static void panel_bridge_detach(struct drm_bridge *bridge) drm_connector_cleanup(connector); } -static void panel_bridge_pre_enable(struct drm_bridge *bridge) +static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_panel_prepare(panel_bridge->panel); } -static void panel_bridge_enable(struct drm_bridge *bridge) +static void panel_bridge_atomic_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_panel_enable(panel_bridge->panel); } -static void panel_bridge_disable(struct drm_bridge *bridge) +static void panel_bridge_atomic_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_panel_disable(panel_bridge->panel); } -static void panel_bridge_post_disable(struct drm_bridge *bridge) +static void panel_bridge_atomic_post_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); @@ -158,10 +162,10 @@ static void panel_bridge_debugfs_init(struct drm_bridge *bridge, static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { .attach = panel_bridge_attach, .detach = panel_bridge_detach, - .pre_enable = panel_bridge_pre_enable, - .enable = panel_bridge_enable, - .disable = panel_bridge_disable, - .post_disable = panel_bridge_post_disable, + .atomic_pre_enable = panel_bridge_atomic_pre_enable, + .atomic_enable = panel_bridge_atomic_enable, + .atomic_disable = panel_bridge_atomic_disable, + .atomic_post_disable = panel_bridge_atomic_post_disable, .get_modes = panel_bridge_get_modes, .atomic_reset = drm_atomic_helper_bridge_reset, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, -- 2.7.4
[PATCH v5 02/10] drm: add helper functions to retrieve old and new crtc
Add new helper functions, drm_atomic_get_old_crtc_for_encoder and drm_atomic_get_new_crtc_for_encoder to retrieve the corresponding crtc for the encoder. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/drm_atomic.c | 60 include/drm/drm_atomic.h | 7 ++ 2 files changed, 67 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 58c0283..87fcb55 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -983,6 +983,66 @@ drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state, EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder); /** + * drm_atomic_get_old_crtc_for_encoder - Get old crtc for an encoder + * @state: Atomic state + * @encoder: The encoder to fetch the crtc state for + * + * This function finds and returns the crtc that was connected to @encoder + * as specified by the @state. + * + * Returns: The old crtc connected to @encoder, or NULL if the encoder is + * not connected. + */ +struct drm_crtc * +drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state, + struct drm_encoder *encoder) +{ + struct drm_connector *connector; + struct drm_connector_state *conn_state; + + connector = drm_atomic_get_old_connector_for_encoder(state, encoder); + if (!connector) + return NULL; + + conn_state = drm_atomic_get_old_connector_state(state, connector); + if (!conn_state) + return NULL; + + return conn_state->crtc; +} +EXPORT_SYMBOL(drm_atomic_get_old_crtc_for_encoder); + +/** + * drm_atomic_get_new_crtc_for_encoder - Get new crtc for an encoder + * @state: Atomic state + * @encoder: The encoder to fetch the crtc state for + * + * This function finds and returns the crtc that will be connected to @encoder + * as specified by the @state. + * + * Returns: The new crtc connected to @encoder, or NULL if the encoder is + * not connected. + */ +struct drm_crtc * +drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_state *state, + struct drm_encoder *encoder) +{ + struct drm_connector *connector; + struct drm_connector_state *conn_state; + + connector = drm_atomic_get_new_connector_for_encoder(state, encoder); + if (!connector) + return NULL; + + conn_state = drm_atomic_get_new_connector_state(state, connector); + if (!conn_state) + return NULL; + + return conn_state->crtc; +} +EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_encoder); + +/** * drm_atomic_get_connector_state - get connector state * @state: global atomic state object * @connector: connector to get state object for diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 025..7001f12 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -528,6 +528,13 @@ struct drm_connector * drm_atomic_get_new_connector_for_encoder(struct drm_atomic_state *state, struct drm_encoder *encoder); +struct drm_crtc * +drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state, +struct drm_encoder *encoder); +struct drm_crtc * +drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_state *state, +struct drm_encoder *encoder); + /** * drm_atomic_get_existing_crtc_state - get CRTC state, if it exists * @state: global atomic state object -- 2.7.4
[PATCH v5 07/10] drm/bridge: add psr support for panel bridge callbacks
This change will handle the psr entry exit cases in the panel bridge atomic callback functions. For example, the panel power should not turn off if the panel is entering psr. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/bridge/panel.c | 48 ++ 1 file changed, 48 insertions(+) diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index eeb9546..9770b8c 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -112,6 +112,18 @@ static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + struct drm_atomic_state *atomic_state = old_bridge_state->base.state; + struct drm_encoder *encoder = bridge->encoder; + struct drm_crtc *crtc; + struct drm_crtc_state *old_crtc_state; + + crtc = drm_atomic_get_new_crtc_for_encoder(atomic_state, encoder); + if (!crtc) + return; + + old_crtc_state = drm_atomic_get_old_crtc_state(atomic_state, crtc); + if (old_crtc_state && old_crtc_state->self_refresh_active) + return; drm_panel_prepare(panel_bridge->panel); } @@ -120,6 +132,18 @@ static void panel_bridge_atomic_enable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + struct drm_atomic_state *atomic_state = old_bridge_state->base.state; + struct drm_encoder *encoder = bridge->encoder; + struct drm_crtc *crtc; + struct drm_crtc_state *old_crtc_state; + + crtc = drm_atomic_get_new_crtc_for_encoder(atomic_state, encoder); + if (!crtc) + return; + + old_crtc_state = drm_atomic_get_old_crtc_state(atomic_state, crtc); + if (old_crtc_state && old_crtc_state->self_refresh_active) + return; drm_panel_enable(panel_bridge->panel); } @@ -128,6 +152,18 @@ static void panel_bridge_atomic_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + struct drm_atomic_state *atomic_state = old_bridge_state->base.state; + struct drm_encoder *encoder = bridge->encoder; + struct drm_crtc *crtc; + struct drm_crtc_state *new_crtc_state; + + crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state, encoder); + if (!crtc) + return; + + new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc); + if (new_crtc_state && new_crtc_state->self_refresh_active) + return; drm_panel_disable(panel_bridge->panel); } @@ -136,6 +172,18 @@ static void panel_bridge_atomic_post_disable(struct drm_bridge *bridge, struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); + struct drm_atomic_state *atomic_state = old_bridge_state->base.state; + struct drm_encoder *encoder = bridge->encoder; + struct drm_crtc *crtc; + struct drm_crtc_state *new_crtc_state; + + crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state, encoder); + if (!crtc) + return; + + new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc); + if (new_crtc_state && new_crtc_state->self_refresh_active) + return; drm_panel_unprepare(panel_bridge->panel); } -- 2.7.4
[PATCH v5 10/10] drm/msm/disp/dpu1: check for crtc enable rather than crtc active to release shared resources
According to KMS documentation, The driver must not release any shared resources if active is set to false but enable still true. Fixes: ccc862b957c6 ("drm/msm/dpu: Fix reservation failures in modeset") Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 726acc9..10dd4ba 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -593,7 +593,7 @@ static int dpu_encoder_virt_atomic_check( if (drm_atomic_crtc_needs_modeset(crtc_state)) { dpu_rm_release(global_state, drm_enc); - if (!crtc_state->active_changed || crtc_state->active) + if (!crtc_state->active_changed || crtc_state->enable) ret = dpu_rm_reserve(&dpu_kms->rm, global_state, drm_enc, crtc_state, topology); } -- 2.7.4
[PATCH v5 00/10] Add PSR support for eDP
Changes in v2: - Use dp bridge to set psr entry/exit instead of dpu_enocder. - Don't modify whitespaces. - Set self refresh aware from atomic_check. - Set self refresh aware only if psr is supported. - Provide a stub for msm_dp_display_set_psr. - Move dp functions to bridge code. Changes in v3: - Change callback names to reflect atomic interfaces. - Move bridge callback change to separate patch as suggested by Dmitry. - Remove psr function declaration from msm_drv.h. - Set self_refresh_aware flag only if psr is supported. - Modify the variable names to simpler form. - Define bit fields for PSR settings. - Add comments explaining the steps to enter/exit psr. - Change DRM_INFO to drm_dbg_db. Changes in v4: - Move the get crtc functions to drm_atomic. - Add atomic functions for DP bridge too. - Add ternary operator to choose eDP or DP ops. - Return true/false instead of 1/0. - mode_valid missing in the eDP bridge ops. - Move the functions to get crtc into drm_atomic.c. - Fix compilation issues. - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc. - Check for crtc state enable while reserving resources. Changes in v5: - Move the mode_valid changes into a different patch. - Complete psr_op_comp only when isr is set. - Move the DP atomic callback changes to a different patch. - Get crtc from drm connector state crtc. - Move to separate patch for check for crtc state enable while reserving resources. Signed-off-by: Sankeerth Billakanti Signed-off-by: Kalyan Thota Signed-off-by: Vinod Polimera Vinod Polimera (10): drm/msm/disp/dpu1: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc drm: add helper functions to retrieve old and new crtc drm/msm/dp: use atomic callbacks for DP bridge ops drm/msm/dp: Add basic PSR support for eDP drm/msm/dp: use the eDP bridge ops to validate eDP modes drm/bridge: use atomic enable/disable callbacks for panel bridge drm/bridge: add psr support for panel bridge callbacks drm/msm/disp/dpu1: use atomic enable/disable callbacks for encoder functions drm/msm/disp/dpu1: add PSR support for eDP interface in dpu driver drm/msm/disp/dpu1: check for crtc enable rather than crtc active to release shared resources drivers/gpu/drm/bridge/panel.c | 68 -- drivers/gpu/drm/drm_atomic.c| 60 + drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 17 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- drivers/gpu/drm/msm/dp/dp_catalog.c | 81 drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_ctrl.c| 73 +++ drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 + drivers/gpu/drm/msm/dp/dp_display.c | 31 +++-- drivers/gpu/drm/msm/dp/dp_display.h | 2 + drivers/gpu/drm/msm/dp/dp_drm.c | 187 ++-- drivers/gpu/drm/msm/dp/dp_drm.h | 9 +- drivers/gpu/drm/msm/dp/dp_link.c| 36 ++ drivers/gpu/drm/msm/dp/dp_panel.c | 22 drivers/gpu/drm/msm/dp/dp_panel.h | 6 + drivers/gpu/drm/msm/dp/dp_reg.h | 27 include/drm/drm_atomic.h| 7 ++ 19 files changed, 634 insertions(+), 64 deletions(-) -- 2.7.4
Re: [PATCH v3] drm/i915/ttm: fix sg_table construction
On 7/11/2022 10:58 AM, Matthew Auld wrote: If we encounter some monster sized local-memory page that exceeds the maximum sg length (UINT32_MAX), ensure that don't end up with some misaligned address in the entry that follows, leading to fireworks later. Also ensure we have some coverage of this in the selftests. v2(Chris): - Use round_down consistently to avoid udiv errors v3(Nirmoy): - Also update the max_segment in the selftest Fixes: f701b16d4cc5 ("drm/i915/ttm: add i915_sg_from_buddy_resource") Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/6379 Signed-off-by: Matthew Auld Reviewed-by: Nirmoy Das Cc: Thomas Hellström Cc: Nirmoy Das --- drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 11 -- drivers/gpu/drm/i915/i915_scatterlist.c | 19 + drivers/gpu/drm/i915/i915_scatterlist.h | 6 -- drivers/gpu/drm/i915/intel_region_ttm.c | 10 ++--- drivers/gpu/drm/i915/intel_region_ttm.h | 3 ++- .../drm/i915/selftests/intel_memory_region.c | 21 +-- drivers/gpu/drm/i915/selftests/mock_region.c | 3 ++- 7 files changed, 58 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c index 7e1f8b83077f..c5c8aa1f8558 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c @@ -602,10 +602,15 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj, struct ttm_resource *res) { struct ttm_buffer_object *bo = i915_gem_to_ttm(obj); + u64 page_alignment; if (!i915_ttm_gtt_binds_lmem(res)) return i915_ttm_tt_get_st(bo->ttm); + page_alignment = bo->page_alignment << PAGE_SHIFT; + if (!page_alignment) + page_alignment = obj->mm.region->min_page_size; + /* * If CPU mapping differs, we need to add the ttm_tt pages to * the resulting st. Might make sense for GGTT. @@ -616,7 +621,8 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj, struct i915_refct_sgt *rsgt; rsgt = intel_region_ttm_resource_to_rsgt(obj->mm.region, -res); +res, + page_alignment); if (IS_ERR(rsgt)) return rsgt; @@ -625,7 +631,8 @@ i915_ttm_resource_get_st(struct drm_i915_gem_object *obj, return i915_refct_sgt_get(obj->ttm.cached_io_rsgt); } - return intel_region_ttm_resource_to_rsgt(obj->mm.region, res); + return intel_region_ttm_resource_to_rsgt(obj->mm.region, res, +page_alignment); } static int i915_ttm_truncate(struct drm_i915_gem_object *obj) diff --git a/drivers/gpu/drm/i915/i915_scatterlist.c b/drivers/gpu/drm/i915/i915_scatterlist.c index 159571b9bd24..f63b50b71e10 100644 --- a/drivers/gpu/drm/i915/i915_scatterlist.c +++ b/drivers/gpu/drm/i915/i915_scatterlist.c @@ -68,6 +68,7 @@ void i915_refct_sgt_init(struct i915_refct_sgt *rsgt, size_t size) * drm_mm_node * @node: The drm_mm_node. * @region_start: An offset to add to the dma addresses of the sg list. + * @page_alignment: Required page alignment for each sg entry. Power of two. * * Create a struct sg_table, initializing it from a struct drm_mm_node, * taking a maximum segment length into account, splitting into segments @@ -77,15 +78,18 @@ void i915_refct_sgt_init(struct i915_refct_sgt *rsgt, size_t size) * error code cast to an error pointer on failure. */ struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct drm_mm_node *node, - u64 region_start) + u64 region_start, + u64 page_alignment) { - const u64 max_segment = SZ_1G; /* Do we have a limit on this? */ + const u64 max_segment = round_down(UINT_MAX, page_alignment); u64 segment_pages = max_segment >> PAGE_SHIFT; u64 block_size, offset, prev_end; struct i915_refct_sgt *rsgt; struct sg_table *st; struct scatterlist *sg; + GEM_BUG_ON(!max_segment); + rsgt = kmalloc(sizeof(*rsgt), GFP_KERNEL); if (!rsgt) return ERR_PTR(-ENOMEM); @@ -112,6 +116,8 @@ struct i915_refct_sgt *i915_rsgt_from_mm_node(const struct drm_mm_node *node, sg = __sg_next(sg); sg_dma_address(sg) = region_start + offset; + GEM_BUG_ON(!IS_ALIGNED(sg_dma_address(sg), + page_alignment)); sg_dma_len(sg) = 0; sg->length = 0; st->nents++; @@ -138,6
Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code
Hello Thomas, On 7/11/22 09:58, Thomas Zimmermann wrote: > Hi Javier, > > I'll try to give the motivation of this patch below. I known it's rather > hypothetical as the VGA driver is probably not used much. > > Am 08.07.22 um 15:09 schrieb Javier Martinez Canillas: >> Hello Thomas, >> >> On 7/7/22 17:39, Thomas Zimmermann wrote: >>> Move the device-creation from vga16fb to sysfb code. Move the few >>> extra videomode checks into vga16fb's probe function. >>> >>> The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC >>> or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except >>> for some MIPS systems. It's not clear if the vga16fb driver actually >>> works in practice. >>> >>> Signed-off-by: Thomas Zimmermann >>> --- >>> drivers/firmware/sysfb.c | 4 +++ >>> drivers/video/fbdev/vga16fb.c | 59 +-- >>> 2 files changed, 32 insertions(+), 31 deletions(-) >>> >>> diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c >>> index 1f276f108cc9..3fd3563d962b 100644 >>> --- a/drivers/firmware/sysfb.c >>> +++ b/drivers/firmware/sysfb.c >>> @@ -94,6 +94,10 @@ static __init int sysfb_init(void) >>> name = "efi-framebuffer"; >>> else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB) >>> name = "vesa-framebuffer"; >>> + else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC) >>> + name = "vga-framebuffer"; >>> + else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC) >>> + >> >> I wonder if we really need to do this distinction or could just have a single >> platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the >> same fbdev driver is bound against both platform devices. > > With the current driver, we don't strictly need to distinguish. But the > sysfb code is the one we care about. So I wanted it to be clear and > nicely looking. All the mode tests, etc depend on the driver (which no > one cares about). > Right. That is a good point. We don't want to leak a driver implementation detail in the sysfb code. And in theory there could be for example a DRM driver for EGA and one for VGA. > There's also a difference in hardware features between EGA and VGA. Most > notably, VGA has much better color support. > Yes, I know the differences. My point was that the orig_video_isVGA was used to make the distinction and that the same driver supported both, but as you said that may not be the case and separate drivers could be used. >> >> [...] >> >>> static int vga16fb_probe(struct platform_device *dev) >>> { >>> + struct screen_info *si; >>> struct fb_info *info; >>> struct vga16fb_par *par; >>> int i; >>> int ret = 0; >>> >>> + si = dev_get_platdata(&dev->dev); >>> + if (!si) >>> + return -ENODEV; >>> + >>> + ret = check_mode_supported(si); >>> + if (ret) >>> + return ret; >>> + >> >> What do you see as the advantage of moving the check to the driver's probe? >> >> Because after this change the platform driver will be registered but for no >> reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC. > > The driver only supports very specific modes, which may not be what > screen_info detected. Note that VGAC/EGAC can also refer to text-mode > buffers. The current vgacon could be turned into a platform driver that > adopts such a text buffer and integrates it with aperture helpers. > Yes, and also there's also the monochrome variant of VGA and EGA (VGAM/EGAM). Should we also make that distinction or for example "vga-framebuffer" should handle both color and monochrome variants if at some point vgacon is turned into a fbdev or DRM driver ? >> >> [...] >> >>> +static const struct platform_device_id vga16fb_driver_id_table[] = { >>> + {"ega-framebuffer", 0}, >>> + {"vga-framebuffer", 0}, >>> + { } >>> +}; >>> + >> >> The fact that the two entries don't have a platform data is an indication for > > The name is the indication. I know that vga16 doesn't treat them > differently. > Yes, my point was that the platform device name used to bind is an internal Linux interface that could be changed later if needed. But I understand your point and since the platform device names are exposed to user-space, makes more sense for them to reflect what devices are bound even when the existing driver doesn't treat them differently. >> me that we could just consolidate in a single "vga16-framebuffer" or smt. I >> know that this won't be consistent with efi, vesa, etc but I don't think is >> that important and also quite likely we will get rid of this driver and the >> platform device registration soon. Since as you said, it's unclear that is >> even used. > > There's mips code in the arch/ directory that appears to setup > screen_info in the correct way. I can't say whether that's still useful > to anyone. On x86, I could set a VGA mode on the kernel command line, > but screen_info's isVGA only contained '1'. It might be
Re: [PATCH 03/11] fbdev/vga16fb: Auto-generate module init/exit code
On 7/11/22 10:01, Thomas Zimmermann wrote: > Hi > > Am 08.07.22 um 15:16 schrieb Javier Martinez Canillas: >> On 7/7/22 17:39, Thomas Zimmermann wrote: >>> Move vgag16fb's option parsing into the driver's probe function and >>> generate the rest of the module's init/exit functions from macros. >>> Keep the options code, although there are no options defined. >>> >> >> Ah, I see now why you wanted to move the check to the probe function. If >> is to allow this cleanup then discard that comment from previous patch >> and I'm OK with the move. >> >> Maybe you could comment in patch 02/11 commit message that the check is >> moved to the probe handler to allow this cleanup as a follow-up patch ? > > Sure. > > I mostly wanted to use module_platform_driver(). The options handling is > in the way. > Yes, I got it when looked at this patch. >> >>> Signed-off-by: Thomas Zimmermann >>> --- >>> drivers/video/fbdev/vga16fb.c | 35 ++- >>> 1 file changed, 10 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/video/fbdev/vga16fb.c b/drivers/video/fbdev/vga16fb.c >>> index f7c1bb018843..e7767ed50c5b 100644 >>> --- a/drivers/video/fbdev/vga16fb.c >>> +++ b/drivers/video/fbdev/vga16fb.c >>> @@ -1321,12 +1321,21 @@ static int __init vga16fb_setup(char *options) >>> >>> static int vga16fb_probe(struct platform_device *dev) >>> { >>> +#ifndef MODULE >>> + char *option = NULL; >>> +#endif >>> struct screen_info *si; >>> struct fb_info *info; >>> struct vga16fb_par *par; >>> int i; >>> int ret = 0; >>> >>> +#ifndef MODULE >>> + if (fb_get_options("vga16fb", &option)) >>> + return -ENODEV; >>> + vga16fb_setup(option); >>> +#endif >>> + >> >> I would just drop these ifdefery and have the option unconditionally. >> It seems that's what most fbdev drivers do AFAICT. > > Or can we kill it entirely? There are no actual options. > That sounds good to me as well. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
[PATCH v3 0/2] Add R16 Vista E board from RenewWorldOutreach
This patch series adds the R16-Vista-E board from RenewWorldOutreach based on allwinner R16(A33). Patch 1/2 adds the dt-bindings for the board. Patch 2/2 adds the board with the following below features: General features: - 1GB RAM - microSD slot - Realtek Wifi - 1 x USB 2.0 - HDMI IN - HDMI OUT - Audio out - MIPI DSI - TI DLPC3433 It has also connectors to connect an external mini keypad. v2 patches can be found here: https://lore.kernel.org/all/20220615093900.344726-1-su...@amarulasolutions.com/ Suniel Mahesh (2): dt-bindings: arm: sunxi: Add binding for RenewWorldOutReach R16-Vista-E board ARM: dts: sun8i: Add R16 Vista E board from RenewWorldOutreach .../devicetree/bindings/arm/sunxi.yaml| 6 + .../devicetree/bindings/vendor-prefixes.yaml | 2 + arch/arm/boot/dts/Makefile| 1 + arch/arm/boot/dts/sun8i-r16-renew-vista-e.dts | 362 ++ 4 files changed, 371 insertions(+) create mode 100644 arch/arm/boot/dts/sun8i-r16-renew-vista-e.dts -- 2.25.1
Re: [PATCH 3/5] dma-buf: return only unsignaled fences in dma_fence_unwrap_for_each v3
Hi Karolina, Am 11.07.22 um 11:44 schrieb Karolina Drobnik: Hi Christian, I'm sorry for digging this one out so late. On 06.05.2022 16:10, Christian König wrote: dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well. v2: fix missing walk over the array v3: massively simplify the patch and actually update the description. Signed-off-by: Christian König --- include/linux/dma-fence-unwrap.h | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned. + * + * Note that signalled fences are opportunistically filtered out, which + * means the iteration is potentially over no fence at all. */ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled - sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all - sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling) Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help. Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits. Do you have a description how to easy reproduce this? E.g. how to run just those specific igts? Thanks, Christian. All the best, Karolina - [1] - reproducible locally, but can be also seen in the CI: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fintel-gfx-ci.01.org%2Ftree%2Fdrm-tip%2Findex.html%3Ftestfilter%3Digt%40sw_sync&data=05%7C01%7Cchristian.koenig%40amd.com%7C2af59c808f664f0cf04908da6321e708%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637931294507736831%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=NTI04OpP7kMsCu%2BDWsJ0%2FRIVJGJbxy36tJBImD2MQDU%3D&reserved=0
[PATCH v3 1/2] dt-bindings: arm: sunxi: Add binding for RenewWorldOutReach R16-Vista-E board
Add a binding for the RenewWorldOutReach R16-Vista-E board based on allwinner R16. Signed-off-by: Suniel Mahesh Signed-off-by: Jagan Teki Signed-off-by: Christopher Vollo Acked-by: Rob Herring --- Changes for v3: - As suggested by Samuel Holland: - vendor prefix documented - primary author of the commit should be the first signer Changes for v2: - Add missing compatible string - insert missing signatures of contributors --- Documentation/devicetree/bindings/arm/sunxi.yaml | 6 ++ Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 2 files changed, 8 insertions(+) diff --git a/Documentation/devicetree/bindings/arm/sunxi.yaml b/Documentation/devicetree/bindings/arm/sunxi.yaml index 95278a6a9a8e..52b8c9aba6f3 100644 --- a/Documentation/devicetree/bindings/arm/sunxi.yaml +++ b/Documentation/devicetree/bindings/arm/sunxi.yaml @@ -787,6 +787,12 @@ properties: - const: allwinner,r7-tv-dongle - const: allwinner,sun5i-a10s + - description: RenewWorldOutreach R16-Vista-E +items: + - const: renewworldoutreach,r16-vista-e + - const: allwinner,sun8i-r16 + - const: allwinner,sun8i-a33 + - description: RerVision H3-DVK items: - const: rervision,h3-dvk diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index 6bb20b4554d7..f5cd0acb1036 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -1030,6 +1030,8 @@ patternProperties: description: reMarkable AS "^renesas,.*": description: Renesas Electronics Corporation + "^renewworldoutreach,.*": +description: RENEW WORLD OUTREACH "^rex,.*": description: iMX6 Rex Project "^rervision,.*": -- 2.25.1
[PATCH v3 2/2] ARM: dts: sun8i: Add R16 Vista E board from RenewWorldOutreach
The R16-Vista-E board from RenewWorldOutreach based on allwinner R16(A33). General features: - 1GB RAM - microSD slot - Realtek Wifi - 1 x USB 2.0 - HDMI IN - HDMI OUT - Audio out - MIPI DSI - TI DLPC3433 It has also connectors to connect an external mini keypad. Signed-off-by: Suniel Mahesh Signed-off-by: Jagan Teki Signed-off-by: Christopher Vollo --- Changes for v3: - As suggested by Samuel Holland: - changed binding to gpio-fan - changed widgets to DACL and DACR to describe audio routing - fixed indentation - primary author of the commit should be the first signer Changes for v2: - Add missing compatible string - insert missing signatures of contributors --- arch/arm/boot/dts/Makefile| 1 + arch/arm/boot/dts/sun8i-r16-renew-vista-e.dts | 362 ++ 2 files changed, 363 insertions(+) create mode 100644 arch/arm/boot/dts/sun8i-r16-renew-vista-e.dts diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile index 184899808ee7..b5966c0742e1 100644 --- a/arch/arm/boot/dts/Makefile +++ b/arch/arm/boot/dts/Makefile @@ -1353,6 +1353,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \ sun8i-r16-nintendo-nes-classic.dtb \ sun8i-r16-nintendo-super-nes-classic.dtb \ sun8i-r16-parrot.dtb \ + sun8i-r16-renew-vista-e.dtb \ sun8i-r40-bananapi-m2-ultra.dtb \ sun8i-r40-oka40i-c.dtb \ sun8i-s3-elimo-initium.dtb \ diff --git a/arch/arm/boot/dts/sun8i-r16-renew-vista-e.dts b/arch/arm/boot/dts/sun8i-r16-renew-vista-e.dts new file mode 100644 index ..ff72914eb110 --- /dev/null +++ b/arch/arm/boot/dts/sun8i-r16-renew-vista-e.dts @@ -0,0 +1,362 @@ +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) +/* + * Copyright (C) 2022 RenewWorldOutreach + * Copyright (C) 2022 Amarula Solutions(India) + */ + +/dts-v1/; +#include "sun8i-a33.dtsi" + +#include +#include +#include + +/ { + model = "RenewWorldOutreach R16-Vista-E"; + compatible = "renewworldoutreach,r16-vista-e", "allwinner,sun8i-r16", "allwinner,sun8i-a33"; + + aliases { + serial0 = &uart0; + }; + + chosen { + stdout-path = "serial0:115200n8"; + }; + + gpio-keys-polled { + compatible = "gpio-keys-polled"; + poll-interval = <100>; + + ok { + label = "ok"; + linux,code = ; + gpios = <&pio 4 0 GPIO_ACTIVE_LOW>; + }; + + left { + label = "left"; + linux,code = ; + gpios = <&pio 4 1 GPIO_ACTIVE_LOW>; + }; + + right { + label = "right"; + linux,code = ; + gpios = <&pio 4 2 GPIO_ACTIVE_LOW>; + }; + + up { + label = "up"; + linux,code = ; + gpios = <&pio 4 3 GPIO_ACTIVE_LOW>; + }; + + down { + label = "down"; + linux,code = ; + gpios = <&pio 4 4 GPIO_ACTIVE_LOW>; + }; + + back { + label = "back"; + linux,code = ; + gpios = <&pio 4 5 GPIO_ACTIVE_LOW>; + }; + + power { + label = "power"; + linux,code = ; + gpios = <&pio 4 6 GPIO_ACTIVE_LOW>; + }; + + vol-down { + label = "vol-down"; + linux,code = ; + gpios = <&pio 7 3 GPIO_ACTIVE_LOW>; + }; + + vol-up { + label = "vol-up"; + linux,code = ; + gpios = <&pio 7 9 GPIO_ACTIVE_LOW>; + }; + }; + + leds { + compatible = "gpio-leds"; + + battery-led0 { + label = "renew-e:battery-led0"; + gpios = <&r_pio 0 2 GPIO_ACTIVE_HIGH>; + }; + + battery-led1 { + label = "renew-e:battery-led1"; + gpios = <&r_pio 0 3 GPIO_ACTIVE_HIGH>; + }; + + battery-led2 { + label = "renew-e:battery-led2"; + gpios = <&r_pio 0 4 GPIO_ACTIVE_HIGH>; + }; + + battery-led3 { + label = "renew-e:battery-led3"; + gpios = <&r_pio 0 5 GPIO_ACTIVE_HIGH>; + }; + + battery-led4 { + label = "renew-e:battery-led4"; + gpios = <&r_pio 0 6 GPIO_ACTIVE_HIGH>; + }; + + volume-led0 { + label = "renew-e:volume-le
[PATCH 1/2] drm: mxsfb: DRM_MXSFB should depend on ARCH_MXS || ARCH_MXC
Freescale/NXP i.MX LCDIF and eLCDIF LCD controllers are only present on Freescale/NXP i.MX SoCs. Hence add a dependency on ARCH_MXS || ARCH_MXC, to prevent asking the user about this driver when configuring a kernel without Freescale/NXP i.MX support. Fixes: 45d59d704080cc0c ("drm: Add new driver for MXSFB controller") Signed-off-by: Geert Uytterhoeven --- drivers/gpu/drm/mxsfb/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig index 873551b4552f5023..02cf19fcef315724 100644 --- a/drivers/gpu/drm/mxsfb/Kconfig +++ b/drivers/gpu/drm/mxsfb/Kconfig @@ -8,6 +8,7 @@ config DRM_MXSFB tristate "i.MX (e)LCDIF LCD controller" depends on DRM && OF depends on COMMON_CLK + depends on ARCH_MXS || ARCH_MXC || COMPILE_TEST select DRM_MXS select DRM_KMS_HELPER select DRM_GEM_CMA_HELPER -- 2.25.1
[PATCH 2/2] drm: mxsfb: DRM_IMX_LCDIF should depend on ARCH_MXC
The Freescale/NXP i.MX LCDIFv3 LCD controller is only present on Freescale/NXP i.MX SoCs. Hence add a dependency on ARCH_MXC, to prevent asking the user about this driver when configuring a kernel without Freescale/NXP i.MX support. Fixes: 9db35bb349a0ef32 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") Signed-off-by: Geert Uytterhoeven --- drivers/gpu/drm/mxsfb/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig index 02cf19fcef315724..40cc9b8fb749f3e4 100644 --- a/drivers/gpu/drm/mxsfb/Kconfig +++ b/drivers/gpu/drm/mxsfb/Kconfig @@ -25,6 +25,7 @@ config DRM_IMX_LCDIF tristate "i.MX LCDIFv3 LCD controller" depends on DRM && OF depends on COMMON_CLK + depends on ARCH_MXC || COMPILE_TEST select DRM_MXS select DRM_KMS_HELPER select DRM_GEM_CMA_HELPER -- 2.25.1
[PATCH 0/2] drm/msm: remove struct msm_display_dsc_config
The struct msm_display_dsc_config contains just a single member, a pointer to struct drm_dsc_config. Use the later one directly instead of wrapping it in the additional struct. Dmitry Baryshkov (2): drm/msm/dpu: use drm_dsc_config instead of msm_display_dsc_config drm/msm/dsi: use drm_dsc_config instead of msm_display_dsc_config drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 ++-- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 74 - drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 4 +- drivers/gpu/drm/msm/dsi/dsi.c | 2 +- drivers/gpu/drm/msm/dsi/dsi.h | 2 +- drivers/gpu/drm/msm/dsi/dsi_host.c | 157 +--- drivers/gpu/drm/msm/msm_drv.h | 9 +- 8 files changed, 131 insertions(+), 144 deletions(-) -- 2.35.1
[PATCH 1/2] drm/msm/dpu: use drm_dsc_config instead of msm_display_dsc_config
There is no need to use the struct msm_display_dsc_config wrapper inside the dpu driver, use the struct drm_dsc_config directly to pass pps data. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 25 +++ drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 74 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.h | 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- 5 files changed, 54 insertions(+), 53 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index c682d4e02d1b..07b22b7df2e9 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -162,7 +162,7 @@ enum dpu_enc_rc_states { * @vsync_event_work: worker to handle vsync event for autorefresh * @topology: topology of the display * @idle_timeout: idle timeout duration in milliseconds - * @dsc: msm_display_dsc_config pointer, for DSC-enabled encoders + * @dsc: drm_dsc_config pointer, for DSC-enabled encoders */ struct dpu_encoder_virt { struct drm_encoder base; @@ -208,7 +208,7 @@ struct dpu_encoder_virt { bool wide_bus_en; /* DSC configuration */ - struct msm_display_dsc_config *dsc; + struct drm_dsc_config *dsc; }; #define to_dpu_encoder_virt(x) container_of(x, struct dpu_encoder_virt, base) @@ -1791,12 +1791,12 @@ static void dpu_encoder_vsync_event_work_handler(struct kthread_work *work) } static u32 -dpu_encoder_dsc_initial_line_calc(struct msm_display_dsc_config *dsc, +dpu_encoder_dsc_initial_line_calc(struct drm_dsc_config *dsc, u32 enc_ip_width) { int ssm_delay, total_pixels, soft_slice_per_enc; - soft_slice_per_enc = enc_ip_width / dsc->drm->slice_width; + soft_slice_per_enc = enc_ip_width / dsc->slice_width; /* * minimum number of initial line pixels is a sum of: @@ -1808,16 +1808,16 @@ dpu_encoder_dsc_initial_line_calc(struct msm_display_dsc_config *dsc, * 5. 6 additional pixels as the output of the rate buffer is *48 bits wide */ - ssm_delay = ((dsc->drm->bits_per_component < 10) ? 84 : 92); - total_pixels = ssm_delay * 3 + dsc->drm->initial_xmit_delay + 47; + ssm_delay = ((dsc->bits_per_component < 10) ? 84 : 92); + total_pixels = ssm_delay * 3 + dsc->initial_xmit_delay + 47; if (soft_slice_per_enc > 1) total_pixels += (ssm_delay * 3); - return DIV_ROUND_UP(total_pixels, dsc->drm->slice_width); + return DIV_ROUND_UP(total_pixels, dsc->slice_width); } static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, struct dpu_hw_pingpong *hw_pp, -struct msm_display_dsc_config *dsc, +struct drm_dsc_config *dsc, u32 common_mode, u32 initial_lines) { @@ -1835,7 +1835,7 @@ static void dpu_encoder_dsc_pipe_cfg(struct dpu_hw_dsc *hw_dsc, } static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, -struct msm_display_dsc_config *dsc) +struct drm_dsc_config *dsc) { /* coding only for 2LM, 2enc, 1 dsc config */ struct dpu_encoder_phys *enc_master = dpu_enc->cur_master; @@ -1858,14 +1858,15 @@ static void dpu_encoder_prep_dsc(struct dpu_encoder_virt *dpu_enc, } } - pic_width = dsc->drm->pic_width; + dsc_common_mode = 0; + pic_width = dsc->pic_width; dsc_common_mode = DSC_MODE_MULTIPLEX | DSC_MODE_SPLIT_PANEL; if (enc_master->intf_mode == INTF_MODE_VIDEO) dsc_common_mode |= DSC_MODE_VIDEO; - this_frame_slices = pic_width / dsc->drm->slice_width; - intf_ip_w = this_frame_slices * dsc->drm->slice_width; + this_frame_slices = pic_width / dsc->slice_width; + intf_ip_w = this_frame_slices * dsc->slice_width; /* * dsc merge case: when using 2 encoders for the same stream, diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index d4d1ecd416e3..9e7236ef34e6 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -36,7 +36,7 @@ struct msm_display_info { uint32_t h_tile_instance[MAX_H_TILES_PER_DISPLAY]; bool is_cmd_mode; bool is_te_using_watchdog_timer; - struct msm_display_dsc_config *dsc; + struct drm_dsc_config *dsc; }; /** diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c index 184a1b27b13d..20a033cd323d 100644 --- a/drivers/gpu/drm/msm/disp
[PATCH 2/2] drm/msm/dsi: use drm_dsc_config instead of msm_display_dsc_config
There is no need to use the struct msm_display_dsc_config wrapper inside the dsi driver, use the struct drm_dsc_config directly to pass pps data. Signed-off-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- drivers/gpu/drm/msm/dsi/dsi.c | 2 +- drivers/gpu/drm/msm/dsi/dsi.h | 2 +- drivers/gpu/drm/msm/dsi/dsi_host.c | 157 +++- drivers/gpu/drm/msm/msm_drv.h | 9 +- 5 files changed, 79 insertions(+), 93 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 8016d0a3aade..75ed2b36e1b3 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -585,7 +585,7 @@ static int _dpu_kms_initialize_dsi(struct drm_device *dev, info.h_tile_instance[info.num_of_h_tiles++] = i; info.is_cmd_mode = msm_dsi_is_cmd_mode(priv->dsi[i]); - info.dsc = msm_dsi_get_dsc_config(priv->dsi[i])->drm; + info.dsc = msm_dsi_get_dsc_config(priv->dsi[i]); if (msm_dsi_is_bonded_dsi(priv->dsi[i]) && priv->dsi[other]) { rc = msm_dsi_modeset_init(priv->dsi[other], dev, encoder); diff --git a/drivers/gpu/drm/msm/dsi/dsi.c b/drivers/gpu/drm/msm/dsi/dsi.c index 1625328fa430..8f1ed31b048a 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.c +++ b/drivers/gpu/drm/msm/dsi/dsi.c @@ -21,7 +21,7 @@ bool msm_dsi_is_cmd_mode(struct msm_dsi *msm_dsi) return !(host_flags & MIPI_DSI_MODE_VIDEO); } -struct msm_display_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi) +struct drm_dsc_config *msm_dsi_get_dsc_config(struct msm_dsi *msm_dsi) { return msm_dsi_host_get_dsc_config(msm_dsi->host); } diff --git a/drivers/gpu/drm/msm/dsi/dsi.h b/drivers/gpu/drm/msm/dsi/dsi.h index 580a1e6358bf..df46cdda1b43 100644 --- a/drivers/gpu/drm/msm/dsi/dsi.h +++ b/drivers/gpu/drm/msm/dsi/dsi.h @@ -154,7 +154,7 @@ int dsi_calc_clk_rate_v2(struct msm_dsi_host *msm_host, bool is_bonded_dsi); int dsi_calc_clk_rate_6g(struct msm_dsi_host *msm_host, bool is_bonded_dsi); void msm_dsi_host_snapshot(struct msm_disp_state *disp_state, struct mipi_dsi_host *host); void msm_dsi_host_test_pattern_en(struct mipi_dsi_host *host); -struct msm_display_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host); +struct drm_dsc_config *msm_dsi_host_get_dsc_config(struct mipi_dsi_host *host); /* dsi phy */ struct msm_dsi_phy; diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c index a34078497af1..15e108be1901 100644 --- a/drivers/gpu/drm/msm/dsi/dsi_host.c +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c @@ -33,7 +33,7 @@ #define DSI_RESET_TOGGLE_DELAY_MS 20 -static int dsi_populate_dsc_params(struct msm_display_dsc_config *dsc); +static int dsi_populate_dsc_params(struct drm_dsc_config *dsc); static int dsi_get_version(const void __iomem *base, u32 *major, u32 *minor) { @@ -161,7 +161,7 @@ struct msm_dsi_host { struct regmap *sfpb; struct drm_display_mode *mode; - struct msm_display_dsc_config *dsc; + struct drm_dsc_config *dsc; /* connected device info */ struct device_node *device_node; @@ -916,7 +916,7 @@ static void dsi_ctrl_config(struct msm_dsi_host *msm_host, bool enable, static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mode, u32 hdisplay) { - struct msm_display_dsc_config *dsc = msm_host->dsc; + struct drm_dsc_config *dsc = msm_host->dsc; u32 reg, intf_width, reg_ctrl, reg_ctrl2; u32 slice_per_intf, total_bytes_per_intf; u32 pkt_per_line; @@ -927,24 +927,24 @@ static void dsi_update_dsc_timing(struct msm_dsi_host *msm_host, bool is_cmd_mod * compress mode registers */ intf_width = hdisplay; - slice_per_intf = DIV_ROUND_UP(intf_width, dsc->drm->slice_width); + slice_per_intf = DIV_ROUND_UP(intf_width, dsc->slice_width); /* If slice_per_pkt is greater than slice_per_intf * then default to 1. This can happen during partial * update. */ - if (slice_per_intf > dsc->drm->slice_count) - dsc->drm->slice_count = 1; + if (slice_per_intf > dsc->slice_count) + dsc->slice_count = 1; - slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->drm->slice_width); - bytes_in_slice = DIV_ROUND_UP(dsc->drm->slice_width * dsc->drm->bits_per_pixel, 8); + slice_per_intf = DIV_ROUND_UP(hdisplay, dsc->slice_width); + bytes_in_slice = DIV_ROUND_UP(dsc->slice_width * dsc->bits_per_pixel, 8); - dsc->drm->slice_chunk_size = bytes_in_slice; + dsc->slice_chunk_size = bytes_in_slice; total_bytes_per_intf = bytes_in_slice * slice_per_intf; eol_byte_num = total_bytes_per_intf % 3; - pkt_per_line = slice_per_intf / dsc->drm->slice_count; + pkt_per_line = slice_per_in
Re: [PATCH v5 01/10] drm/msm/disp/dpu1: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc
On 11/07/2022 12:47, Vinod Polimera wrote: Update crtc retrieval from dpu_enc to dpu_enc connector state, since new links get set as part of the dpu enc virt mode set. The dpu_enc->crtc cache is no more needed, hence cleaning it as part of this change. Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 4 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 29 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 3 files changed, 14 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index b56f777..f91e3d1 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -972,7 +972,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc, */ if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO) release_bandwidth = true; - dpu_encoder_assign_crtc(encoder, NULL); } /* wait for frame_event_done completion */ @@ -1042,9 +1041,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc, trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc); dpu_crtc->enabled = true; - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask) - dpu_encoder_assign_crtc(encoder, crtc); - /* Enable/restore vblank irq handling */ drm_crtc_vblank_on(crtc); } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index 52516eb..8fb3e15 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1245,6 +1245,7 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, struct dpu_encoder_phys *phy_enc) { struct dpu_encoder_virt *dpu_enc = NULL; + struct drm_crtc *crtc; unsigned long lock_flags; if (!drm_enc || !phy_enc) @@ -1253,9 +1254,14 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, DPU_ATRACE_BEGIN("encoder_vblank_callback"); dpu_enc = to_dpu_encoder_virt(drm_enc); + if (!dpu_enc->connector || !dpu_enc->connector->state) + return; + + crtc = dpu_enc->connector->state->crtc; + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc) - dpu_crtc_vblank_callback(dpu_enc->crtc); + if (crtc) + dpu_crtc_vblank_callback(crtc); spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); atomic_inc(&phy_enc->vsync_cnt); @@ -1280,29 +1286,22 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, DPU_ATRACE_END("encoder_underrun_callback"); } -void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc) -{ - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); - unsigned long lock_flags; - - spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - /* crtc should always be cleared before re-assigning */ - WARN_ON(crtc && dpu_enc->crtc); - dpu_enc->crtc = crtc; - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); -} Please also remove the dpu_encoder_virt::crtc pointer. - void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc, bool enable) { struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc); + struct drm_crtc *new_crtc; unsigned long lock_flags; int i; trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); + if (!dpu_enc->connector || !dpu_enc->connector->state) + return; + + new_crtc = dpu_enc->connector->state->crtc; spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc != crtc) { + if (!new_crtc || crtc != crtc) { Second condition is always false. spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); return; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 781d41c..edba815 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -39,14 +39,6 @@ struct msm_display_info { }; /** - * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to - * @encoder: encoder pointer - * @crtc: crtc pointer - */ -void dpu_encoder_assign_crtc(struct drm_encoder *encoder, -struct drm_crtc *crtc); - -/** * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or off if *the encoder is assigned to the given crtc * @encoder: encoder pointer -- With best wishes Dmitry
Re: [PATCH v5 00/10] Add PSR support for eDP
On 11/07/2022 12:47, Vinod Polimera wrote: Changes in v2: - Use dp bridge to set psr entry/exit instead of dpu_enocder. - Don't modify whitespaces. - Set self refresh aware from atomic_check. - Set self refresh aware only if psr is supported. - Provide a stub for msm_dp_display_set_psr. - Move dp functions to bridge code. Changes in v3: - Change callback names to reflect atomic interfaces. - Move bridge callback change to separate patch as suggested by Dmitry. - Remove psr function declaration from msm_drv.h. - Set self_refresh_aware flag only if psr is supported. - Modify the variable names to simpler form. - Define bit fields for PSR settings. - Add comments explaining the steps to enter/exit psr. - Change DRM_INFO to drm_dbg_db. Changes in v4: - Move the get crtc functions to drm_atomic. - Add atomic functions for DP bridge too. - Add ternary operator to choose eDP or DP ops. - Return true/false instead of 1/0. - mode_valid missing in the eDP bridge ops. - Move the functions to get crtc into drm_atomic.c. - Fix compilation issues. - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc. - Check for crtc state enable while reserving resources. Changes in v5: - Move the mode_valid changes into a different patch. - Complete psr_op_comp only when isr is set. - Move the DP atomic callback changes to a different patch. - Get crtc from drm connector state crtc. - Move to separate patch for check for crtc state enable while reserving resources. Signed-off-by: Sankeerth Billakanti Signed-off-by: Kalyan Thota Signed-off-by: Vinod Polimera Vinod Polimera (10): drm/msm/disp/dpu1: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc Generic comment: please use 'drm/msm/dpu:' prefix instead. drm: add helper functions to retrieve old and new crtc drm/msm/dp: use atomic callbacks for DP bridge ops drm/msm/dp: Add basic PSR support for eDP drm/msm/dp: use the eDP bridge ops to validate eDP modes drm/bridge: use atomic enable/disable callbacks for panel bridge drm/bridge: add psr support for panel bridge callbacks drm/msm/disp/dpu1: use atomic enable/disable callbacks for encoder functions drm/msm/disp/dpu1: add PSR support for eDP interface in dpu driver drm/msm/disp/dpu1: check for crtc enable rather than crtc active to release shared resources drivers/gpu/drm/bridge/panel.c | 68 -- drivers/gpu/drm/drm_atomic.c| 60 + drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c| 17 ++- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 55 +--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +- drivers/gpu/drm/msm/dp/dp_catalog.c | 81 drivers/gpu/drm/msm/dp/dp_catalog.h | 4 + drivers/gpu/drm/msm/dp/dp_ctrl.c| 73 +++ drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 + drivers/gpu/drm/msm/dp/dp_display.c | 31 +++-- drivers/gpu/drm/msm/dp/dp_display.h | 2 + drivers/gpu/drm/msm/dp/dp_drm.c | 187 ++-- drivers/gpu/drm/msm/dp/dp_drm.h | 9 +- drivers/gpu/drm/msm/dp/dp_link.c| 36 ++ drivers/gpu/drm/msm/dp/dp_panel.c | 22 drivers/gpu/drm/msm/dp/dp_panel.h | 6 + drivers/gpu/drm/msm/dp/dp_reg.h | 27 include/drm/drm_atomic.h| 7 ++ 19 files changed, 634 insertions(+), 64 deletions(-) -- With best wishes Dmitry
Re: [PATCH v5 03/10] drm/msm/dp: use atomic callbacks for DP bridge ops
On 11/07/2022 12:47, Vinod Polimera wrote: Use atomic variants for DP bridge callback functions so that the atomic state can be accessed in the interface drivers. The atomic state will help the driver find out if the display is in self refresh state. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/dp/dp_display.c | 9 ++--- drivers/gpu/drm/msm/dp/dp_drm.c | 18 +++--- 2 files changed, 17 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index bce7793..5bd6677 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -1652,7 +1652,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev, return 0; } -void dp_bridge_enable(struct drm_bridge *drm_bridge) +void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge, +struct drm_bridge_state *old_bridge_state) { struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge); struct msm_dp *dp = dp_bridge->dp_display; @@ -1716,7 +1717,8 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) mutex_unlock(&dp_display->event_mutex); } -void dp_bridge_disable(struct drm_bridge *drm_bridge) +void dp_bridge_atomic_disable(struct drm_bridge *drm_bridge, + struct drm_bridge_state *old_bridge_state) { struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge); struct msm_dp *dp = dp_bridge->dp_display; @@ -1727,7 +1729,8 @@ void dp_bridge_disable(struct drm_bridge *drm_bridge) dp_ctrl_push_idle(dp_display->ctrl); } -void dp_bridge_post_disable(struct drm_bridge *drm_bridge) +void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge, + struct drm_bridge_state *old_bridge_state) { struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge); struct msm_dp *dp = dp_bridge->dp_display; diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c index 62d58b9..9d0fc74 100644 --- a/drivers/gpu/drm/msm/dp/dp_drm.c +++ b/drivers/gpu/drm/msm/dp/dp_drm.c @@ -61,13 +61,17 @@ static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector * } static const struct drm_bridge_funcs dp_bridge_ops = { - .enable = dp_bridge_enable, - .disable = dp_bridge_disable, - .post_disable = dp_bridge_post_disable, - .mode_set = dp_bridge_mode_set, - .mode_valid = dp_bridge_mode_valid, - .get_modes= dp_bridge_get_modes, - .detect = dp_bridge_detect, + .atomic_enable = dp_bridge_atomic_enable, + .atomic_disable = dp_bridge_atomic_disable, + .atomic_post_disable = dp_bridge_atomic_post_disable, + .mode_set = dp_bridge_mode_set, + .mode_valid = dp_bridge_mode_valid, + .get_modes = dp_bridge_get_modes, + .detect = dp_bridge_detect, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_check = edp_bridge_atomic_check, This function does not exist (yet). Please move this line to the corresponding patch. }; struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev, -- With best wishes Dmitry
Re: [PATCH 3/3] drm/komeda - Fix handling of pending crtc state commit to avoid lock-up
On Fri, Jul 08, 2022 at 07:03:37PM +0100, Carsten Haitzler wrote: > > > On 7/8/22 17:02, Liviu Dudau wrote: > > On Mon, Jun 06, 2022 at 12:47:14PM +0100, carsten.haitz...@foss.arm.com > > wrote: > > > From: Carsten Haitzler > > > > Hi Carsten, > > > > > > > > Sometimes there is an extra dcm crtc state in the pipeline whose > > > penting vblank event has not been handled yet. We would previously > > > throw this out and the vblank event never triggers leading to hard > > > lockups higher up the stack where an expected vlank event never comes > > > back (screen freezes). > > > > > > This fixes that by tracking a pending crtc state that needs handling > > > and handle it producing a vlank event next vblank if it had not > > > laready been handled before. This fixes the hangs and ensures our > > > display keeps updating seamlessly and is certainly a much better state > > > to be in than after some time ending up with a mysteriously frozen > > > screen and a lot of kernle messages complaining about this too. > > > > Sorry it took me so long to review and reply to this patch, but I had this > > nagging > > No worries. :) > > > feeling that the patch is not actually correct so I've tried to track the > > actual > > issue. It turns out that the problem is easy to reproduce in a different > > setup with > > Mali D71 and it comes from the fact that Komeda doesn't properly wait for > > the > > hardware to signal acceptance of config valid on setting new commits. I > > have created > > a new patch that I would be happy if you can test to try to fix the actual > > issue. > > This works (tested). Thank you very much for testing this! Can I add your Tested-by? > One errant "flip without commit": > > [9.402559] fbcon: Taking over console > [9.525373] [drm:komeda_print_events [komeda]] *ERROR* err detect: gcu: > MERR, pipes[0]: FLIP, pipes[1]: None > [9.525455] [drm] CRTC[0]: FLIP happened but no pending commit. > [9.542215] Console: switching to colour frame buffer device 240x67 > Is this with your 2/3 patch applied and coming out from the firmware having already initialised the hardware? If so, the handover probably doesn't quiescence the interrupts correctly so an interrupt is pending when the kernel driver is initialised. That's another area worth looking at for the handover purposes. > But nothing worrying. It does work, though doesn't compile due to: > > drivers/gpu/drm/arm/display/komeda/komeda_kms.c: In function > ‘komeda_kms_atomic_commit_hw_done’: > drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: error: ‘for’ loop > initial declarations are only allowed in C99 or C11 mode >77 | for (int i = 0; i < kms->n_crtcs; i++) { > | ^~~ > drivers/gpu/drm/arm/display/komeda/komeda_kms.c:77:9: note: use option > ‘-std=c9 > ’, ‘-std=gnu99’, ‘-std=c11’ or ‘-std=gnu11’ to compile your code > > but that was a trivial fixup. Interesting that I'm not seeing that, probably due to using GCC12? Anyway, I'll fix that and send a proper patch. > > Your commit handler does sit and wait. I guess I avoided that and had it > still deferred and handled next time the vblank interrupt fires. Yours is a > bit shorter and less complex so it wins. :) Yes, that's the essence of my issue with your patch. Delaying the handling of the event until the next vblank means older software that doesn't use the timestamping from the vblank event will get wrong framerates (playing video might be affected). Waiting here when we're also calling drm_atomic_helper_wait_for_flip_done() after drm_atomic_helper_commit_hw_done() feels wrong, but then the later is checking if we have consumed the event so we have to. Maybe the introduction of the drm_atomic_helper_fake_vblank() is needed in komeda as well like the generic commit_tail helper function does? I need to investigate more the next time I get some spare cycles on komeda. I will send a new email with the updated patch and your Tested-by if that's OK. Best regards, Liviu > > > --8<--- > > > > From 76f9e5fed216a815e9eb56152f85454521079f10 Mon Sep 17 00:00:00 2001 > > From: Liviu Dudau > > Date: Fri, 8 Jul 2022 16:39:21 +0100 > > Subject: [PATCH] drm/komeda: Fix handling of atomic commits in the > > atomic_commit_tail hook > > > > Komeda driver relies on the generic DRM atomic helper functions to handle > > commits. It only implements an atomic_commit_tail hook for the > > mode_config_helper_funcs and even that one is pretty close to the generic > > implementation with the exception of additional dma_fence signalling. > > > > What the generic helper framework doesn't do is waiting for the actual > > hardware to signal that the commit parameters have been written into the > > appropriate registers. As we signal CRTC events only on the irq handlers, > > we need to flush the configuration and wait for the hardware to respond. > > > > Add the Komed
Re: [PATCH v5 04/10] drm/msm/dp: add basic PSR support for eDP
On 11/07/2022 12:47, Vinod Polimera wrote: Add support for basic panel self refresh (PSR) feature for eDP. Add a new interface to set PSR state in the sink from DPU. Program the eDP controller to issue PSR enter and exit SDP to the sink. Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/msm/dp/dp_catalog.c | 81 + drivers/gpu/drm/msm/dp/dp_catalog.h | 4 ++ drivers/gpu/drm/msm/dp/dp_ctrl.c| 78 - drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 + drivers/gpu/drm/msm/dp/dp_display.c | 14 drivers/gpu/drm/msm/dp/dp_display.h | 2 + drivers/gpu/drm/msm/dp/dp_drm.c | 136 +++- drivers/gpu/drm/msm/dp/dp_drm.h | 9 ++- drivers/gpu/drm/msm/dp/dp_link.c| 36 ++ drivers/gpu/drm/msm/dp/dp_panel.c | 22 ++ drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++ drivers/gpu/drm/msm/dp/dp_reg.h | 27 +++ 12 files changed, 411 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c index 7257515..b9021ed 100644 --- a/drivers/gpu/drm/msm/dp/dp_catalog.c +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c @@ -47,6 +47,14 @@ #define DP_INTERRUPT_STATUS2_MASK \ (DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT) +#define DP_INTERRUPT_STATUS4 \ + (PSR_UPDATE_INT | PSR_CAPTURE_INT | PSR_EXIT_INT | \ + PSR_UPDATE_ERROR_INT | PSR_WAKE_ERROR_INT) + +#define DP_INTERRUPT_MASK4 \ + (PSR_UPDATE_MASK | PSR_CAPTURE_MASK | PSR_EXIT_MASK | \ + PSR_UPDATE_ERROR_MASK | PSR_WAKE_ERROR_MASK) + struct dp_catalog_private { struct device *dev; struct drm_device *drm_dev; @@ -359,6 +367,24 @@ void dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog) ln_mapping); } +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog, + bool enable) +{ + u32 val; + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + + val = dp_read_link(catalog, REG_DP_MAINLINK_CTRL); + val &= ~DP_MAINLINK_CTRL_ENABLE; + + if (enable) + val |= DP_MAINLINK_CTRL_ENABLE; + else + val &= ~DP_MAINLINK_CTRL_ENABLE; + + dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val); +} + void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool enable) { @@ -610,6 +636,47 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog) dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN); } +static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog) +{ + /* trigger sdp */ + dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); + dp_write_link(catalog, MMSS_DP_SDP_CFG3, !UPDATE_SDP); +} + +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 config; + + /* enable PSR1 function */ + config = dp_read_link(catalog, REG_PSR_CONFIG); + config |= PSR1_SUPPORTED; + dp_write_link(catalog, REG_PSR_CONFIG, config); + + dp_write_ahb(catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4); + dp_catalog_enable_sdp(catalog); +} + +void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 cmd; + + cmd = dp_read_link(catalog, REG_PSR_CMD); + + cmd &= ~(PSR_ENTER | PSR_EXIT); + + if (enter) + cmd |= PSR_ENTER; + else + cmd |= PSR_EXIT; + + dp_catalog_enable_sdp(catalog); + dp_write_link(catalog, REG_PSR_CMD, cmd); +} + u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, @@ -645,6 +712,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog) return isr & (mask | ~DP_DP_HPD_INT_MASK); } +int dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog *dp_catalog) +{ + struct dp_catalog_private *catalog = container_of(dp_catalog, + struct dp_catalog_private, dp_catalog); + u32 intr, intr_ack; + + intr = dp_read_ahb(catalog, REG_DP_INTR_STATUS4); + intr_ack = (intr & DP_INTERRUPT_STATUS4) + << DP_INTERRUPT_STATUS_ACK_SHIFT; + dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack); + + return intr; +} + int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) { struct dp_catalog_private *catalog = container_of(dp_catalog, diff --git a/drive
Re: [PATCH 2/2] drm: mxsfb: DRM_IMX_LCDIF should depend on ARCH_MXC
On 7/11/22 11:01, Geert Uytterhoeven wrote: The Freescale/NXP i.MX LCDIFv3 LCD controller is only present on Freescale/NXP i.MX SoCs. Hence add a dependency on ARCH_MXC, to prevent asking the user about this driver when configuring a kernel without Freescale/NXP i.MX support. Fixes: 9db35bb349a0ef32 ("drm: lcdif: Add support for i.MX8MP LCDIF variant") Signed-off-by: Geert Uytterhoeven Reviewed-by: Marek Vasut
Re: [PATCH 1/2] drm: mxsfb: DRM_MXSFB should depend on ARCH_MXS || ARCH_MXC
On 7/11/22 11:01, Geert Uytterhoeven wrote: Freescale/NXP i.MX LCDIF and eLCDIF LCD controllers are only present on Freescale/NXP i.MX SoCs. Hence add a dependency on ARCH_MXS || ARCH_MXC, to prevent asking the user about this driver when configuring a kernel without Freescale/NXP i.MX support. Fixes: 45d59d704080cc0c ("drm: Add new driver for MXSFB controller") Signed-off-by: Geert Uytterhoeven Is the Fixes tag really justified in this case ? Otherwise Reviewed-by: Marek Vasut
Re: [PATCH v11 20/24] arm64: dts: rockchip: enable vop2 and hdmi tx on rock-3a
On 2022-06-25 16:31, Piotr Oniszczuk wrote: Wiadomość napisana przez Peter Geis w dniu 25.06.2022, o godz. 16:00: The first issue you have is the TV isn't responding until the absolute end. I suspect this is because lack on idle gaps between cec commands sent from board to tv. Maybe TV sw. can't deal with consecutive commands without any idle between them? It is interesting that disconnecting TV - so CEC line is driven only by board - rock3a still don't have any idle gaps while rock3b (and radxa 4.19 bsp) has them (very similar between 5.18mailine and 4.19 bsp). How this is possible that change I/O from m0->m1 impacts _timings_ on free hanging CEC line? Check all the pinctrl settings beyond just the function mux - pulls, drive strength, output type, etc. - the defaults tend to be all over the place, and rarely what you want. Robin. This strikes me as a signal integrity issue. Do you have an oscilloscope (not a logic analyzer, you need voltages and ramp times) to compare the working vs non-working signals? Check both sides of the level shifter. Indeed - i will verify this with digital oscilloscope. Already ordered and must await week or 2 for delivery :-( My analog oscilloscope shows correct levels and slopes "seems" to be the same like in working (no memory so i can compare only visually on fuzzy screen) For me key is to understand why on rock3a there is no any idles between cec commands - even when nothing is connected to bard (so cec is only sending and nothing external impacts cec state machine) ___ Linux-rockchip mailing list linux-rockc...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code
Hi Javier Am 11.07.22 um 11:54 schrieb Javier Martinez Canillas: Hello Thomas, On 7/11/22 09:58, Thomas Zimmermann wrote: Hi Javier, I'll try to give the motivation of this patch below. I known it's rather hypothetical as the VGA driver is probably not used much. Am 08.07.22 um 15:09 schrieb Javier Martinez Canillas: Hello Thomas, On 7/7/22 17:39, Thomas Zimmermann wrote: Move the device-creation from vga16fb to sysfb code. Move the few extra videomode checks into vga16fb's probe function. The vga16fb driver requires a screen_info for type VIDEO_TYPE_VGAC or VIDEO_TYPE_EGAC. Such code is nowhere present in the kernel, except for some MIPS systems. It's not clear if the vga16fb driver actually works in practice. Signed-off-by: Thomas Zimmermann --- drivers/firmware/sysfb.c | 4 +++ drivers/video/fbdev/vga16fb.c | 59 +-- 2 files changed, 32 insertions(+), 31 deletions(-) diff --git a/drivers/firmware/sysfb.c b/drivers/firmware/sysfb.c index 1f276f108cc9..3fd3563d962b 100644 --- a/drivers/firmware/sysfb.c +++ b/drivers/firmware/sysfb.c @@ -94,6 +94,10 @@ static __init int sysfb_init(void) name = "efi-framebuffer"; else if (si->orig_video_isVGA == VIDEO_TYPE_VLFB) name = "vesa-framebuffer"; + else if (si->orig_video_isVGA == VIDEO_TYPE_VGAC) + name = "vga-framebuffer"; + else if (si->orig_video_isVGA == VIDEO_TYPE_EGAC) + I wonder if we really need to do this distinction or could just have a single platform device for both VIDEO_TYPE_VGAC and VIDEO_TYPE_EGAC. After all, the same fbdev driver is bound against both platform devices. With the current driver, we don't strictly need to distinguish. But the sysfb code is the one we care about. So I wanted it to be clear and nicely looking. All the mode tests, etc depend on the driver (which no one cares about). Right. That is a good point. We don't want to leak a driver implementation detail in the sysfb code. And in theory there could be for example a DRM driver for EGA and one for VGA. There's also a difference in hardware features between EGA and VGA. Most notably, VGA has much better color support. Yes, I know the differences. My point was that the orig_video_isVGA was used to make the distinction and that the same driver supported both, but as you said that may not be the case and separate drivers could be used. [...] static int vga16fb_probe(struct platform_device *dev) { + struct screen_info *si; struct fb_info *info; struct vga16fb_par *par; int i; int ret = 0; + si = dev_get_platdata(&dev->dev); + if (!si) + return -ENODEV; + + ret = check_mode_supported(si); + if (ret) + return ret; + What do you see as the advantage of moving the check to the driver's probe? Because after this change the platform driver will be registered but for no reason, since can't even probe if orig_video_isVGA is neither VGAC nor EGAC. The driver only supports very specific modes, which may not be what screen_info detected. Note that VGAC/EGAC can also refer to text-mode buffers. The current vgacon could be turned into a platform driver that adopts such a text buffer and integrates it with aperture helpers. Yes, and also there's also the monochrome variant of VGA and EGA (VGAM/EGAM). Should we also make that distinction or for example "vga-framebuffer" should handle both color and monochrome variants if at some point vgacon is turned into a fbdev or DRM driver ? [...] +static const struct platform_device_id vga16fb_driver_id_table[] = { + {"ega-framebuffer", 0}, + {"vga-framebuffer", 0}, + { } +}; + The fact that the two entries don't have a platform data is an indication for The name is the indication. I know that vga16 doesn't treat them differently. Yes, my point was that the platform device name used to bind is an internal Linux interface that could be changed later if needed. But I understand your point and since the platform device names are exposed to user-space, makes more sense for them to reflect what devices are bound even when the existing driver doesn't treat them differently. me that we could just consolidate in a single "vga16-framebuffer" or smt. I know that this won't be consistent with efi, vesa, etc but I don't think is that important and also quite likely we will get rid of this driver and the platform device registration soon. Since as you said, it's unclear that is even used. There's mips code in the arch/ directory that appears to setup screen_info in the correct way. I can't say whether that's still useful to anyone. On x86, I could set a VGA mode on the kernel command line, but screen_info's isVGA only contained '1'. It might be possible to fix this easily by setting the right values in vga_probe(). [1] I simply don't have the time to provide a patch and deal with the potential
Re: [PATCH 02/11] fbdev/vga16fb: Create EGA/VGA devices in sysfb code
On 7/11/22 12:42, Thomas Zimmermann wrote: > Hi Javier [...] >> That's why I think that "vga-framebuffer" as driver name would be misleading. >> Specially since it would also bind to the "ega-framebuffer" platform device. > > I messed up device and driver name, such that misunderstood your remark. > > As we use the id_table field for matching devices, the driver name > doesn't matter. [1] So let's keep the driver name as vga16fb. The change > above must have been left over from an earlier prototype patch, I guess. > Agreed. The driver name is used as the last resort to match a device only if there isn't any device ID table (ACPI, OF, platform, etc) but that's discouraged. Specially when the same driver supports different devices as it's the case for this driver. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 04/11] fbdev/core: Remove remove_conflicting_pci_framebuffers()
On 7/7/22 17:39, Thomas Zimmermann wrote: > Remove remove_conflicting_pci_framebuffers() and implement similar > functionality in aperture_remove_conflicting_pci_device(), which was > the only caller. Removes an otherwise unused interface and streamlines > the aperture helper. No functional changes. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 05/11] fbdev: Convert drivers to aperture helpers
On 7/7/22 17:39, Thomas Zimmermann wrote: > Convert fbdev drivers from fbdev's remove_conflicting_framebuffers() to > the framework-independent aperture_remove_conflicting_devices(). Calling > this function will also remove conflicting DRM drivers. > > Signed-off-by: Thomas Zimmermann > --- [...] > static int lynxfb_kick_out_firmware_fb(struct pci_dev *pdev) > { > - struct apertures_struct *ap; > + resource_size_t base = pci_resource_start(pdev, 0); > + resource_size_t size = pci_resource_len(pdev, 0); > bool primary = false; > > - ap = alloc_apertures(1); > - if (!ap) > - return -ENOMEM; > - > - ap->ranges[0].base = pci_resource_start(pdev, 0); > - ap->ranges[0].size = pci_resource_len(pdev, 0); > #ifdef CONFIG_X86 > primary = pdev->resource[PCI_ROM_RESOURCE].flags & > IORESOURCE_ROM_SHADOW; > #endif > - remove_conflicting_framebuffers(ap, "sm750_fb1", primary); > - kfree(ap); > - return 0; > + > + return aperture_remove_conflicting_devices(base, size, primary, > "sm750_fb1"); Do you know why this can't just use aperture_remove_conflicting_pci_devices() ? It seems that the driver is open coding part of the logic already in that helper. For example, figuring out if is a primary by checking the IORESOURCE_ROM_SHADOW flag in the PCI_ROM_RESOURCE. But also getting the base and size for PCI BAR 0, since the loop in that helper would already take care of that (it also starts at BAR 0). > } > > static int lynxfb_pci_probe(struct pci_dev *pdev, > diff --git a/drivers/video/fbdev/aty/radeon_base.c > b/drivers/video/fbdev/aty/radeon_base.c > index b311c07fe66d..e5e362b8c9da 100644 > --- a/drivers/video/fbdev/aty/radeon_base.c > +++ b/drivers/video/fbdev/aty/radeon_base.c > @@ -54,6 +54,7 @@ > > #include "radeonfb.h" > > +#include > #include > #include > #include > @@ -2239,20 +2240,10 @@ static const struct bin_attribute edid2_attr = { > > static int radeon_kick_out_firmware_fb(struct pci_dev *pdev) > { > - struct apertures_struct *ap; > + resource_size_t base = pci_resource_start(pdev, 0); > + resource_size_t size = pci_resource_len(pdev, 0); > > - ap = alloc_apertures(1); > - if (!ap) > - return -ENOMEM; > - > - ap->ranges[0].base = pci_resource_start(pdev, 0); > - ap->ranges[0].size = pci_resource_len(pdev, 0); > - > - remove_conflicting_framebuffers(ap, KBUILD_MODNAME, false); > - > - kfree(ap); > - > - return 0; > + return aperture_remove_conflicting_devices(base, size, KBUILD_MODNAME, > false); Same for this. -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
Hi Am 11.07.22 um 11:35 schrieb Geert Uytterhoeven: Hi Thomas, On Mon, Jul 11, 2022 at 11:03 AM Thomas Zimmermann wrote: Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: The mode parsing code recognizes named modes only if they are explicitly listed in the internal whitelist, which is currently limited to "NTSC" and "PAL". Provide a mechanism for drivers to override this list to support custom mode names. Ideally, this list should just come from the driver's actual list of modes, but connector->probed_modes is not yet populated at the time of parsing. I've looked for code that uses these names, couldn't find any. How is this being used in practice? For example, if I say "PAL" on the command line, is there DRM code that fills in the PAL mode parameters? I guess Maxime knows, as he added the whitelist? Yeah, I saw his reply already. Reading the description of commit 3764137906a5acec ("drm/modes: Introduce a whitelist for the named modes"), it looks like this is more about preventing the parser from taking any string as a random mode, than about adding support for "PAL" or "NTSC"? Note that drivers/gpu/drm/i915/display/intel_tv.c defines an array of tv_modes[], including "PAL", so perhaps these end up as named modes? And another question I have is whether this whitelist belongs into the driver at all. Standard modes exist independent from drivers or hardware. Shouldn't there simply be a global list of all possible mode names? Drivers would filter out the unsupported modes anyway. For standard modes, I agree. And these are usually specified by resolution and refresh rate (e.g. "640x480@60", instead of "480p"). But legacy hardware may have very limited support for programmable pixel clocks (e.g. Amiga is limited to pixel clocks of 7, 14, or 28 MHz), so the standard modes are a bad match, or may not work at all. Hence drivers may need to provide their own modes, but it seems wrong to me to make these non-standard modes global, and possibly pollute the experience for everyone. I don't really have a strong opinion, but having all modes in one global list is quite user-friendly. It's all there for everyone. Otherwise users would somehow have to know which hardware supports which modes. That's actually the job of each driver's mode_valid and atomic_check functions. Best regards Thomas Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- 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 v11 20/24] arm64: dts: rockchip: enable vop2 and hdmi tx on rock-3a
> Wiadomość napisana przez Robin Murphy w dniu > 11.07.2022, o godz. 12:41: > > On 2022-06-25 16:31, Piotr Oniszczuk wrote: >>> Wiadomość napisana przez Peter Geis w dniu >>> 25.06.2022, o godz. 16:00: >>> >>> >>> The first issue you have is the TV isn't responding until the absolute >>> end. >> I suspect this is because lack on idle gaps between cec commands sent from >> board to tv. >> Maybe TV sw. can't deal with consecutive commands without any idle between >> them? >> It is interesting that disconnecting TV - so CEC line is driven only by >> board - rock3a still don't have any idle gaps while rock3b (and radxa 4.19 >> bsp) has them (very similar between 5.18mailine and 4.19 bsp). >> How this is possible that change I/O from m0->m1 impacts _timings_ on free >> hanging CEC line? > > Check all the pinctrl settings beyond just the function mux - pulls, drive > strength, output type, etc. - the defaults tend to be all over the place, and > rarely what you want. > > Robin. Robin, I'm not sure do I looked in right place... but: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi?h=v5.18.10#n788 vs. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi?h=v5.18.10#n795 are looking ok? > >>> This strikes me as a signal integrity issue. Do you have an >>> oscilloscope (not a logic analyzer, you need voltages and ramp times) >>> to compare the working vs non-working signals? Check both sides of the >>> level shifter. >> Indeed - i will verify this with digital oscilloscope. >> Already ordered and must await week or 2 for delivery :-( >> My analog oscilloscope shows correct levels and slopes "seems" to be the >> same like in working (no memory so i can compare only visually on fuzzy >> screen) >> For me key is to understand why on rock3a there is no any idles between cec >> commands - even when nothing is connected to bard (so cec is only sending >> and nothing external impacts cec state machine) >> ___ >> Linux-rockchip mailing list >> linux-rockc...@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
Hi Maxime Am 11.07.22 um 11:35 schrieb Maxime Ripard: Hi Thomas, On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: The mode parsing code recognizes named modes only if they are explicitly listed in the internal whitelist, which is currently limited to "NTSC" and "PAL". Provide a mechanism for drivers to override this list to support custom mode names. Ideally, this list should just come from the driver's actual list of modes, but connector->probed_modes is not yet populated at the time of parsing. I've looked for code that uses these names, couldn't find any. How is this being used in practice? For example, if I say "PAL" on the command line, is there DRM code that fills in the PAL mode parameters? We have some code to deal with this in sun4i: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292 It's a bit off topic, but for TV standards, I'm still not sure what the best course of action is. There's several interactions that make this a bit troublesome: * Some TV standards differ by their mode (ie, PAL vs NSTC), but some other differ by parameters that are not part of drm_display_mode (NTSC vs NSTC-J where the only difference is the black and blanking signal levels for example). * The mode names allow to provide a fairly convenient way to add that extra information, but the userspace is free to create its own mode and might omit the mode name entirely. So in the code above, if the name has been preserved we match by name, but we fall back to matching by mode if it hasn't been, which in this case means that we have no way to differentiate between NTSC, NTSC-J, PAL-M in this case. We have some patches downstream for the RaspberryPi that has the TV standard as a property. There's a few extra logic required for the userspace (like setting the PAL property, with the NTSC mode) so I'm not sure it's preferable. Or we could do something like a property to try that standard, and another that reports the one we actually chose. And another question I have is whether this whitelist belongs into the driver at all. Standard modes exist independent from drivers or hardware. Shouldn't there simply be a global list of all possible mode names? Drivers would filter out the unsupported modes anyway. We should totally do something like that, yeah That sun code already looks like sometihng the DRM core/helpers should be doing. And if we want to support named modes well, there's a long list of modes in Wikipedia. https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg Best regards Thomas Maxime -- 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 06/11] fbdev: Remove conflicting devices on PCI bus
On 7/7/22 17:39, Thomas Zimmermann wrote: > Remove firmware devices on the PCI bus, by calling > aperture_remove_conflicting_pci_devices() in the probe function of > each related fbdev driver. iSo far, most of these drivers did not > remove conflicting VESA or EFI devices, or outride failed for > resource conflicts (i.e., matroxfb.) This must have been broken > for quite some time. > > Signed-off-by: Thomas Zimmermann > --- [...] > @@ -949,6 +950,10 @@ static int ark_pci_probe(struct pci_dev *dev, const > struct pci_device_id *id) > int rc; > u8 regval; > > + rc = aperture_remove_conflicting_pci_devices(dev, "arkfb"); > + if (rc < 0) > + return rc; > + I wonder if we could think of a trick to avoid open coding the same check in all drivers. Maybe a combination of using KBUILD_MODNAME for the name and a probe callback wrapper or something ? But probably not worth to invest more in the fbdev drivers and could be done as a follow-up anyways if someone feels like it. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 07/11] video/aperture: Disable and unregister sysfb devices via aperture helpers
On 7/7/22 17:39, Thomas Zimmermann wrote: > Call sysfb_disable() before removing conflicting devices in aperture > helpers. Fixes sysfb state if fbdev has been disabled. > > Signed-off-by: Thomas Zimmermann > Fixes: fb84efa28a48 ("drm/aperture: Run fbdev removal before internal > helpers") > Cc: Zack Rusin > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > Cc: Daniel Vetter > Cc: Daniel Vetter > Cc: Sam Ravnborg > Cc: Helge Deller > Cc: Alex Deucher > Cc: Zhen Lei > Cc: Changcheng Deng > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: dri-devel@lists.freedesktop.org > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 08/11] video: Provide constants for VGA I/O range
On 7/7/22 17:39, Thomas Zimmermann wrote: > Provide VGA_FB_ constants for the VGA framebuffer I/O range and convert > fbdev code. > I would probably mention in the commit message to that you are renaming s/VGA_FB_PHYS/VGA_FB_PHYS_BASE and s/VGA_FB_PHYS_LEN/VGA_FB_PHYS_SIZE. I do agree with the rename and think that makes it much more readable, but think that's worth to mention the rationale for that change too. > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 09/11] video/aperture: Remove conflicting VGA devices, if any
On 7/7/22 17:39, Thomas Zimmermann wrote: > On the primary graphics adapter, a driver might conflict with a VGA > driver that controls the VGA framebuffer I/O range. Remove the VGA > driver from the aperture helpers. Until now, this case has been > hendled by fbdev, but it should work even with fbdev disabled. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 10/11] fbdev: Acquire framebuffer apertures for firmware devices
On 7/7/22 17:39, Thomas Zimmermann wrote: > When registering a generic framebuffer, automatically acquire ownership > of the framebuffer's I/O range. The device will now be handled by the > aperture helpers. Fbdev-based conflict handling is no longer required. > > Signed-off-by: Thomas Zimmermann > --- > drivers/video/fbdev/core/fbmem.c | 33 > 1 file changed, 33 insertions(+) > > diff --git a/drivers/video/fbdev/core/fbmem.c > b/drivers/video/fbdev/core/fbmem.c > index 2237049327db..e556ad69f48f 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -13,6 +13,7 @@ > > #include > > +#include > #include > #include > #include > @@ -1739,6 +1740,32 @@ static void do_unregister_framebuffer(struct fb_info > *fb_info) > put_fb_info(fb_info); > } > > +static int fbm_aperture_acquire_for_platform_device(struct fb_info *fb_info) > +{ What's the meaning of 'm' here ? Misc, memory ? I would just call it 'fb_'. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH 11/11] fbdev: Remove conflict-handling code
On 7/7/22 17:39, Thomas Zimmermann wrote: > Remove the call to do_remove_conflicting_framebuffers() from the > framebuffer registration. Aperture helpers take care of removing > conflicting devices. With all ownership information stored in the > aperture datastrcutures, remove remove_conflicting_framebuffers() data structures. > entirely. > > This change also rectifies DRM generic-framebuffer registration, which > tried to unregister conflicting framebuffers, even though it's entirely > build on top of DRM. > > Signed-off-by: Thomas Zimmermann > --- Amazing patch! The whole series really. Thanks a lot for working on this. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Linux Engineering Red Hat
Re: [PATCH v11 20/24] arm64: dts: rockchip: enable vop2 and hdmi tx on rock-3a
On 2022-07-11 12:04, Piotr Oniszczuk wrote: Wiadomość napisana przez Robin Murphy w dniu 11.07.2022, o godz. 12:41: On 2022-06-25 16:31, Piotr Oniszczuk wrote: Wiadomość napisana przez Peter Geis w dniu 25.06.2022, o godz. 16:00: The first issue you have is the TV isn't responding until the absolute end. I suspect this is because lack on idle gaps between cec commands sent from board to tv. Maybe TV sw. can't deal with consecutive commands without any idle between them? It is interesting that disconnecting TV - so CEC line is driven only by board - rock3a still don't have any idle gaps while rock3b (and radxa 4.19 bsp) has them (very similar between 5.18mailine and 4.19 bsp). How this is possible that change I/O from m0->m1 impacts _timings_ on free hanging CEC line? Check all the pinctrl settings beyond just the function mux - pulls, drive strength, output type, etc. - the defaults tend to be all over the place, and rarely what you want. Robin. Robin, I'm not sure do I looked in right place... but: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi?h=v5.18.10#n788 vs. https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3568-pinctrl.dtsi?h=v5.18.10#n795 are looking ok? I meant more in terms of dumping out the actual hardware state to compare across both axes of cec_m0 vs. cec_m1 and mainline vs. BSP. However from a quick skim of the Rock3 schematic there doesn't appear to be an external pull-up, so the internal pull-up also being disabled is a clear suspect to start with. Robin.
Re: [PATCH v2 04/11] dt-bindings: display/msm: split qcom, mdss bindings
On 10/07/2022 11:00, Dmitry Baryshkov wrote: Thank you for your patch. There is something to discuss/improve. > +required: > + - compatible > + - reg > + - reg-names > + - interrupts > + - interrupt-controller > + - "#interrupt-cells" > + - power-domains > + - clocks > + - clock-names > + - "#address-cells" > + - "#size-cells" > + - ranges > + > +patternProperties: > + "^mdp@(0|[1-9a-f][0-9a-f]*)$": You used some unusual pattern. It's just "[0-9a-f]+" - the device schema's job is not to validate patterns in unit addresses. Another question - why do you allow "@0" alone? > +type: object > +# TODO: add reference once the mdp5 is converted > + > + "^dsi@(0|[1-9a-f][0-9a-f]*)$": > +$ref: dsi-controller-main.yaml# Best regards, Krzysztof
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > Hi Maxime > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > Hi Thomas, > > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: > > > > The mode parsing code recognizes named modes only if they are explicitly > > > > listed in the internal whitelist, which is currently limited to "NTSC" > > > > and "PAL". > > > > > > > > Provide a mechanism for drivers to override this list to support custom > > > > mode names. > > > > > > > > Ideally, this list should just come from the driver's actual list of > > > > modes, but connector->probed_modes is not yet populated at the time of > > > > parsing. > > > > > > I've looked for code that uses these names, couldn't find any. How is this > > > being used in practice? For example, if I say "PAL" on the command line, > > > is > > > there DRM code that fills in the PAL mode parameters? > > > > We have some code to deal with this in sun4i: > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292 > > > > It's a bit off topic, but for TV standards, I'm still not sure what the > > best course of action is. There's several interactions that make this a > > bit troublesome: > > > >* Some TV standards differ by their mode (ie, PAL vs NSTC), but some > > other differ by parameters that are not part of drm_display_mode > > (NTSC vs NSTC-J where the only difference is the black and blanking > > signal levels for example). > > > >* The mode names allow to provide a fairly convenient way to add that > > extra information, but the userspace is free to create its own mode > > and might omit the mode name entirely. > > > > So in the code above, if the name has been preserved we match by name, > > but we fall back to matching by mode if it hasn't been, which in this > > case means that we have no way to differentiate between NTSC, NTSC-J, > > PAL-M in this case. > > > > We have some patches downstream for the RaspberryPi that has the TV > > standard as a property. There's a few extra logic required for the > > userspace (like setting the PAL property, with the NTSC mode) so I'm not > > sure it's preferable. > > > > Or we could do something like a property to try that standard, and > > another that reports the one we actually chose. > > > > > And another question I have is whether this whitelist belongs into the > > > driver at all. Standard modes exist independent from drivers or hardware. > > > Shouldn't there simply be a global list of all possible mode names? > > > Drivers > > > would filter out the unsupported modes anyway. > > > > We should totally do something like that, yeah > > That sun code already looks like sometihng the DRM core/helpers should be > doing. And if we want to support named modes well, there's a long list of > modes in Wikipedia. > > https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg Yeah, and NTSC is missing :) Thinking about this some more, I'm not sure how we would do that. Like I said, we would need some extra parameters to drm_display_mode (like blanking levels) that the core would need to pass to the driver. If we go through the property route, I think the core could just look at the name, with the new mode and state, and the driver should deal with it. I'm not sure we can do more than that. Maxime signature.asc Description: PGP signature
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
Hi Maxime, On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard wrote: > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: > > > > > The mode parsing code recognizes named modes only if they are > > > > > explicitly > > > > > listed in the internal whitelist, which is currently limited to "NTSC" > > > > > and "PAL". > > > > > > > > > > Provide a mechanism for drivers to override this list to support > > > > > custom > > > > > mode names. > > > > > > > > > > Ideally, this list should just come from the driver's actual list of > > > > > modes, but connector->probed_modes is not yet populated at the time of > > > > > parsing. > > > > > > > > I've looked for code that uses these names, couldn't find any. How is > > > > this > > > > being used in practice? For example, if I say "PAL" on the command > > > > line, is > > > > there DRM code that fills in the PAL mode parameters? > > > > > > We have some code to deal with this in sun4i: > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292 > > > > > > It's a bit off topic, but for TV standards, I'm still not sure what the > > > best course of action is. There's several interactions that make this a > > > bit troublesome: > > > > > >* Some TV standards differ by their mode (ie, PAL vs NSTC), but some > > > other differ by parameters that are not part of drm_display_mode > > > (NTSC vs NSTC-J where the only difference is the black and blanking > > > signal levels for example). > > > > > >* The mode names allow to provide a fairly convenient way to add that > > > extra information, but the userspace is free to create its own mode > > > and might omit the mode name entirely. > > > > > > So in the code above, if the name has been preserved we match by name, > > > but we fall back to matching by mode if it hasn't been, which in this > > > case means that we have no way to differentiate between NTSC, NTSC-J, > > > PAL-M in this case. > > > > > > We have some patches downstream for the RaspberryPi that has the TV > > > standard as a property. There's a few extra logic required for the > > > userspace (like setting the PAL property, with the NTSC mode) so I'm not > > > sure it's preferable. > > > > > > Or we could do something like a property to try that standard, and > > > another that reports the one we actually chose. > > > > > > > And another question I have is whether this whitelist belongs into the > > > > driver at all. Standard modes exist independent from drivers or > > > > hardware. > > > > Shouldn't there simply be a global list of all possible mode names? > > > > Drivers > > > > would filter out the unsupported modes anyway. > > > > > > We should totally do something like that, yeah > > > > That sun code already looks like sometihng the DRM core/helpers should be > > doing. And if we want to support named modes well, there's a long list of > > modes in Wikipedia. > > > > https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg > > Yeah, and NTSC is missing :) And that diagram is about the "digital" variant of PAL. If you go the analog route, the only fixed parts are vfreq/hfreq, number of lines, and synchronization. Other parameters like overscan can vary. The actual dot clock can vary wildly: while there is an upper limit due to bandwidth limitations, you can come up with an almost infinite number of video modes that can be called PAL, which is one of the reasons why I don't want hardware-specific variants to end up in a global video mode database. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard wrote: > > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > > Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: > > > > > > The mode parsing code recognizes named modes only if they are > > > > > > explicitly > > > > > > listed in the internal whitelist, which is currently limited to > > > > > > "NTSC" > > > > > > and "PAL". > > > > > > > > > > > > Provide a mechanism for drivers to override this list to support > > > > > > custom > > > > > > mode names. > > > > > > > > > > > > Ideally, this list should just come from the driver's actual list of > > > > > > modes, but connector->probed_modes is not yet populated at the time > > > > > > of > > > > > > parsing. > > > > > > > > > > I've looked for code that uses these names, couldn't find any. How is > > > > > this > > > > > being used in practice? For example, if I say "PAL" on the command > > > > > line, is > > > > > there DRM code that fills in the PAL mode parameters? > > > > > > > > We have some code to deal with this in sun4i: > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292 > > > > > > > > It's a bit off topic, but for TV standards, I'm still not sure what the > > > > best course of action is. There's several interactions that make this a > > > > bit troublesome: > > > > > > > >* Some TV standards differ by their mode (ie, PAL vs NSTC), but some > > > > other differ by parameters that are not part of drm_display_mode > > > > (NTSC vs NSTC-J where the only difference is the black and blanking > > > > signal levels for example). > > > > > > > >* The mode names allow to provide a fairly convenient way to add that > > > > extra information, but the userspace is free to create its own mode > > > > and might omit the mode name entirely. > > > > > > > > So in the code above, if the name has been preserved we match by name, > > > > but we fall back to matching by mode if it hasn't been, which in this > > > > case means that we have no way to differentiate between NTSC, NTSC-J, > > > > PAL-M in this case. > > > > > > > > We have some patches downstream for the RaspberryPi that has the TV > > > > standard as a property. There's a few extra logic required for the > > > > userspace (like setting the PAL property, with the NTSC mode) so I'm not > > > > sure it's preferable. > > > > > > > > Or we could do something like a property to try that standard, and > > > > another that reports the one we actually chose. > > > > > > > > > And another question I have is whether this whitelist belongs into the > > > > > driver at all. Standard modes exist independent from drivers or > > > > > hardware. > > > > > Shouldn't there simply be a global list of all possible mode names? > > > > > Drivers > > > > > would filter out the unsupported modes anyway. > > > > > > > > We should totally do something like that, yeah > > > > > > That sun code already looks like sometihng the DRM core/helpers should be > > > doing. And if we want to support named modes well, there's a long list of > > > modes in Wikipedia. > > > > > > https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg > > > > Yeah, and NTSC is missing :) > > And that diagram is about the "digital" variant of PAL. > If you go the analog route, the only fixed parts are vfreq/hfreq, > number of lines, and synchronization. Other parameters like overscan > can vary. The actual dot clock can vary wildly: while there is an > upper limit due to bandwidth limitations, you can come up with an > almost infinite number of video modes that can be called PAL, which > is one of the reasons why I don't want hardware-specific variants to > end up in a global video mode database. Do you have an example of what that would look like? Maxime signature.asc Description: PGP signature
Re: [PATCH 4/5] drm/modes: Add support for driver-specific named modes
Hi Maxime, On Mon, Jul 11, 2022 at 2:02 PM Maxime Ripard wrote: > On Mon, Jul 11, 2022 at 01:59:28PM +0200, Geert Uytterhoeven wrote: > > On Mon, Jul 11, 2022 at 1:42 PM Maxime Ripard wrote: > > > On Mon, Jul 11, 2022 at 01:11:14PM +0200, Thomas Zimmermann wrote: > > > > Am 11.07.22 um 11:35 schrieb Maxime Ripard: > > > > > On Mon, Jul 11, 2022 at 11:03:38AM +0200, Thomas Zimmermann wrote: > > > > > > Am 08.07.22 um 20:21 schrieb Geert Uytterhoeven: > > > > > > > The mode parsing code recognizes named modes only if they are > > > > > > > explicitly > > > > > > > listed in the internal whitelist, which is currently limited to > > > > > > > "NTSC" > > > > > > > and "PAL". > > > > > > > > > > > > > > Provide a mechanism for drivers to override this list to support > > > > > > > custom > > > > > > > mode names. > > > > > > > > > > > > > > Ideally, this list should just come from the driver's actual list > > > > > > > of > > > > > > > modes, but connector->probed_modes is not yet populated at the > > > > > > > time of > > > > > > > parsing. > > > > > > > > > > > > I've looked for code that uses these names, couldn't find any. How > > > > > > is this > > > > > > being used in practice? For example, if I say "PAL" on the command > > > > > > line, is > > > > > > there DRM code that fills in the PAL mode parameters? > > > > > > > > > > We have some code to deal with this in sun4i: > > > > > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/sun4i/sun4i_tv.c#L292 > > > > > > > > > > It's a bit off topic, but for TV standards, I'm still not sure what > > > > > the > > > > > best course of action is. There's several interactions that make this > > > > > a > > > > > bit troublesome: > > > > > > > > > >* Some TV standards differ by their mode (ie, PAL vs NSTC), but > > > > > some > > > > > other differ by parameters that are not part of drm_display_mode > > > > > (NTSC vs NSTC-J where the only difference is the black and > > > > > blanking > > > > > signal levels for example). > > > > > > > > > >* The mode names allow to provide a fairly convenient way to add > > > > > that > > > > > extra information, but the userspace is free to create its own > > > > > mode > > > > > and might omit the mode name entirely. > > > > > > > > > > So in the code above, if the name has been preserved we match by name, > > > > > but we fall back to matching by mode if it hasn't been, which in this > > > > > case means that we have no way to differentiate between NTSC, NTSC-J, > > > > > PAL-M in this case. > > > > > > > > > > We have some patches downstream for the RaspberryPi that has the TV > > > > > standard as a property. There's a few extra logic required for the > > > > > userspace (like setting the PAL property, with the NTSC mode) so I'm > > > > > not > > > > > sure it's preferable. > > > > > > > > > > Or we could do something like a property to try that standard, and > > > > > another that reports the one we actually chose. > > > > > > > > > > > And another question I have is whether this whitelist belongs into > > > > > > the > > > > > > driver at all. Standard modes exist independent from drivers or > > > > > > hardware. > > > > > > Shouldn't there simply be a global list of all possible mode names? > > > > > > Drivers > > > > > > would filter out the unsupported modes anyway. > > > > > > > > > > We should totally do something like that, yeah > > > > > > > > That sun code already looks like sometihng the DRM core/helpers should > > > > be > > > > doing. And if we want to support named modes well, there's a long list > > > > of > > > > modes in Wikipedia. > > > > > > > > https://en.wikipedia.org/wiki/Video_Graphics_Array#/media/File:Vector_Video_Standards2.svg > > > > > > Yeah, and NTSC is missing :) > > > > And that diagram is about the "digital" variant of PAL. > > If you go the analog route, the only fixed parts are vfreq/hfreq, > > number of lines, and synchronization. Other parameters like overscan > > can vary. The actual dot clock can vary wildly: while there is an > > upper limit due to bandwidth limitations, you can come up with an > > almost infinite number of video modes that can be called PAL, which > > is one of the reasons why I don't want hardware-specific variants to > > end up in a global video mode database. > > Do you have an example of what that would look like? You mean a PAL mode that does not use 768x576? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/video/fbdev/amifb.c#n834 (TAG_HIRES is replaced by the actual dot clock at runtime, as it depends on the crystal present on the mainboard). Amifb also supports 320x256, by doubling the dot clock, but that mode is not part of the database. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talk
Re: [PATCH libdrm v2 04/10] util: Add missing big-endian RGB16 frame buffer formats
On Fri, Jul 08, 2022 at 08:21:43PM +0200, Geert Uytterhoeven wrote: > Signed-off-by: Geert Uytterhoeven > --- > Any better suggestion than appending "be"? > > v2: > - New. > --- > tests/util/format.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tests/util/format.c b/tests/util/format.c > index a5464de6fc1ac70f..42a652c9a402a654 100644 > --- a/tests/util/format.c > +++ b/tests/util/format.c > @@ -76,6 +76,9 @@ static const struct util_format_info format_info[] = { > { DRM_FORMAT_BGRX5551, "BX15", MAKE_RGB_INFO(5, 1, 5, 6, 5, 11, 0, 0) }, > { DRM_FORMAT_RGB565, "RG16", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > { DRM_FORMAT_BGR565, "BG16", MAKE_RGB_INFO(5, 0, 6, 5, 5, 11, 0, 0) }, > + /* Big-endian RGB16 */ > + { DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, "XR15be", > MAKE_RGB_INFO(5, 10, 5, 5, 5, 0, 0, 0) }, > + { DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, "RG16be", MAKE_RGB_INFO(5, > 11, 6, 5, 5, 0, 0, 0) }, How about just stripping the BE bit in util_format_info_find() so we don't have to duplicate the entries in the table? I guess util_format_fourcc() would end up being more a bit complicated since you'd have to massage the string. But I'm not sure why we even store the fourcc as a string in the table anyway. Could just add some kind of string_to_fourcc() thingy instead AFAICS. > /* RGB24 */ > { DRM_FORMAT_BGR888, "BG24", MAKE_RGB_INFO(8, 0, 8, 8, 8, 16, 0, 0) }, > { DRM_FORMAT_RGB888, "RG24", MAKE_RGB_INFO(8, 16, 8, 8, 8, 0, 0, 0) }, > -- > 2.25.1 -- Ville Syrjälä Intel
Re: [PATCH 3/5] dma-buf: return only unsignaled fences in dma_fence_unwrap_for_each v3
Hi Christian, On 11.07.2022 11:57, Christian König wrote: Hi Karolina, Am 11.07.22 um 11:44 schrieb Karolina Drobnik: Hi Christian, I'm sorry for digging this one out so late. On 06.05.2022 16:10, Christian König wrote: dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well. v2: fix missing walk over the array v3: massively simplify the patch and actually update the description. Signed-off-by: Christian König --- include/linux/dma-fence-unwrap.h | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned. + * + * Note that signalled fences are opportunistically filtered out, which + * means the iteration is potentially over no fence at all. */ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled - sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all - sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling) Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help. Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits. The reproduction with IGTs should be quite easy. You'll need to clone/download the IGT code and follow instructions for Building[1] the project (make sure you have meson and ninja installed): https://gitlab.freedesktop.org/drm/igt-gpu-tools Once you have it up and running, go to /build/tests, and run the subtests: ./sw_sync --run sync_merge ./sw_sync --run sync_merge_same ./sw_sync --run sync_multi_timeline_wait You can run all the subtests with ./sw_sync, but I think these are the most relevant to you. Many thanks, Karolina -- [1] - https://gitlab.freedesktop.org/drm/igt-gpu-tools#building Do you have a description how to easy reproduce this? E.g. how to run just those specific igts? Thanks, Christian. All the best, Karolina
Re: [PATCH 3/5] dma-buf: return only unsignaled fences in dma_fence_unwrap_for_each v3
Hi Karolina, Am 11.07.22 um 14:17 schrieb Karolina Drobnik: Hi Christian, On 11.07.2022 11:57, Christian König wrote: Hi Karolina, Am 11.07.22 um 11:44 schrieb Karolina Drobnik: Hi Christian, I'm sorry for digging this one out so late. On 06.05.2022 16:10, Christian König wrote: dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well. v2: fix missing walk over the array v3: massively simplify the patch and actually update the description. Signed-off-by: Christian König --- include/linux/dma-fence-unwrap.h | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned. + * + * Note that signalled fences are opportunistically filtered out, which + * means the iteration is potentially over no fence at all. */ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled - sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all - sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling) Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help. Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits. The reproduction with IGTs should be quite easy. You'll need to clone/download the IGT code and follow instructions for Building[1] the project (make sure you have meson and ninja installed): https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools&data=05%7C01%7Cchristian.koenig%40amd.com%7C9a9587aefd2d4ac2d86208da63375cb6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637931386683611766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=4WsMutcFJ2HwBqld%2BTv9N1Tx6cbFMwJJZ6kjm5rbfoI%3D&reserved=0 Once you have it up and running, go to /build/tests, and run the subtests: ./sw_sync --run sync_merge ./sw_sync --run sync_merge_same ./sw_sync --run sync_multi_timeline_wait You can run all the subtests with ./sw_sync, but I think these are the most relevant to you. Thanks, I've already managed to reproduce it. Not sure what's going on here, but could be that the test case was never correct in the first place. Need to double check. Thanks, Christian. Many thanks, Karolina -- [1] - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%23building&data=05%7C01%7Cchristian.koenig%40amd.com%7C9a9587aefd2d4ac2d86208da63375cb6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637931386683611766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FV0Ao6ra8EOyr4cOs4N7mCmpOEUUObTrgyOrd0tvEV8%3D&reserved=0 Do you have a description how to easy reproduce this? E.g. how to run just those specific igts? Thanks, Christian. All the best, Karolina
Re: [PATCH v3 0/4] drm: rename CMA helpers to DMA helpers
On 7/7/22 19:39, Danilo Krummrich wrote: This patch series renames all CMA helpers to DMA helpers - considering the hierarchy of APIs (mm/cma -> dma -> gem/fb dma helpers) calling them DMA helpers seems to be more applicable. Additionally, commit e57924d4ae80 ("drm/doc: Task to rename CMA helpers") requests to rename the CMA helpers and implies that people seem to be confused about the naming. The patches are compile-time tested building a x86_64 kernel with `make allyesconfig && make drivers/gpu/drm`. Changes in v2: - Fixed up comments for consistent memory/address classification (DMA-contiguous) - Added a patch to rename struct drm_gem_dma_object.{paddr => dma_addr} Changes in v3: - Use a ccoccinelle script for "drm/gem: rename struct drm_gem_dma_object.{paddr => dma_addr}" for fixing up missing drivers and compile-test on x86_64, arm and arm64. Danilo Krummrich (4): drm/fb: rename FB CMA helpers to FB DMA helpers drm/gem: rename GEM CMA helpers to GEM DMA helpers drm/gem: rename struct drm_gem_dma_object.{paddr => dma_addr} drm/todo: remove task to rename CMA helpers Since those patches are based on Linus' tree they (unsurprisingly) don't apply on drm-next cleanly. Please let me know if you want me to send a rebased version for drm-next. - Danilo Documentation/gpu/drm-kms-helpers.rst | 8 +- Documentation/gpu/drm-mm.rst | 16 +- Documentation/gpu/todo.rst| 13 - drivers/gpu/drm/Kconfig | 4 +- drivers/gpu/drm/Makefile | 6 +- drivers/gpu/drm/arm/Kconfig | 4 +- drivers/gpu/drm/arm/display/Kconfig | 2 +- .../arm/display/komeda/komeda_framebuffer.c | 12 +- .../gpu/drm/arm/display/komeda/komeda_kms.c | 10 +- drivers/gpu/drm/arm/hdlcd_crtc.c | 6 +- drivers/gpu/drm/arm/hdlcd_drv.c | 8 +- drivers/gpu/drm/arm/malidp_drv.c | 10 +- drivers/gpu/drm/arm/malidp_mw.c | 8 +- drivers/gpu/drm/arm/malidp_planes.c | 34 +- drivers/gpu/drm/armada/armada_gem.c | 6 +- drivers/gpu/drm/aspeed/Kconfig| 2 +- drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 10 +- drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 8 +- drivers/gpu/drm/atmel-hlcdc/Kconfig | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 6 +- .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 8 +- ...rm_fb_cma_helper.c => drm_fb_dma_helper.c} | 67 ++-- drivers/gpu/drm/drm_file.c| 2 +- drivers/gpu/drm/drm_format_helper.c | 4 +- ..._gem_cma_helper.c => drm_gem_dma_helper.c} | 314 +- drivers/gpu/drm/drm_mipi_dbi.c| 2 +- drivers/gpu/drm/fsl-dcu/Kconfig | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 8 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c | 2 +- drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 10 +- drivers/gpu/drm/hisilicon/kirin/Kconfig | 2 +- .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 +- .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 +- drivers/gpu/drm/imx/Kconfig | 2 +- drivers/gpu/drm/imx/dcss/Kconfig | 2 +- drivers/gpu/drm/imx/dcss/dcss-kms.c | 6 +- drivers/gpu/drm/imx/dcss/dcss-plane.c | 18 +- drivers/gpu/drm/imx/imx-drm-core.c| 10 +- drivers/gpu/drm/imx/imx-drm.h | 2 +- drivers/gpu/drm/imx/ipuv3-crtc.c | 4 +- drivers/gpu/drm/imx/ipuv3-plane.c | 28 +- drivers/gpu/drm/ingenic/Kconfig | 2 +- drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 14 +- drivers/gpu/drm/ingenic/ingenic-ipu.c | 12 +- drivers/gpu/drm/kmb/Kconfig | 2 +- drivers/gpu/drm/kmb/kmb_drv.c | 6 +- drivers/gpu/drm/kmb/kmb_plane.c | 10 +- drivers/gpu/drm/mcde/Kconfig | 2 +- drivers/gpu/drm/mcde/mcde_display.c | 8 +- drivers/gpu/drm/mcde/mcde_drv.c | 10 +- drivers/gpu/drm/mediatek/Kconfig | 2 +- drivers/gpu/drm/mediatek/mtk_drm_drv.c| 2 +- drivers/gpu/drm/mediatek/mtk_drm_gem.c| 4 +- drivers/gpu/drm/meson/Kconfig | 2 +- drivers/gpu/drm/meson/meson_drv.c | 10 +- drivers/gpu/drm/meson/meson_overlay.c | 18 +- drivers/gpu/drm/meson/meson_plane.c | 10 +- drivers/gpu/drm/msm/msm_drv.c | 2 +- drivers/gpu/drm/mxsfb/Kconfig | 2 +- drivers/gpu/drm/mxsfb/mxsfb_drv.c | 6 +- drivers/gpu/drm/mxsfb/mxsfb_kms.c | 34 +- drivers/gpu/drm/panel/Kconfig | 2 +- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 6 +- drivers/gpu/drm/pl111/Kconfig | 2 +- drivers/gpu/d
Re: [PATCH libdrm v2 04/10] util: Add missing big-endian RGB16 frame buffer formats
Hi Ville, On Mon, Jul 11, 2022 at 2:17 PM Ville Syrjälä wrote: > On Fri, Jul 08, 2022 at 08:21:43PM +0200, Geert Uytterhoeven wrote: > > Signed-off-by: Geert Uytterhoeven > > --- > > Any better suggestion than appending "be"? > > > > v2: > > - New. > > --- a/tests/util/format.c > > +++ b/tests/util/format.c > > @@ -76,6 +76,9 @@ static const struct util_format_info format_info[] = { > > { DRM_FORMAT_BGRX5551, "BX15", MAKE_RGB_INFO(5, 1, 5, 6, 5, 11, 0, 0) > > }, > > { DRM_FORMAT_RGB565, "RG16", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > > { DRM_FORMAT_BGR565, "BG16", MAKE_RGB_INFO(5, 0, 6, 5, 5, 11, 0, 0) }, > > + /* Big-endian RGB16 */ > > + { DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, "XR15be", > > MAKE_RGB_INFO(5, 10, 5, 5, 5, 0, 0, 0) }, > > + { DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, "RG16be", > > MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > > How about just stripping the BE bit in util_format_info_find() > so we don't have to duplicate the entries in the table? There is no need to support big-endian variants of all formats. E.g. big-endian [AX]RGB just map to little-endian BGR[AX]. XRGB1555 and RGB565 are probably the only RGB formats we care about. Or perhaps some of the *30 formats? > I guess util_format_fourcc() would end up being more a bit > complicated since you'd have to massage the string. True. > But I'm not sure why we even store the fourcc as a string in > the table anyway. Could just add some kind of string_to_fourcc() > thingy instead AFAICS. I guess that can be done. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Re: [PATCH v3 0/4] drm: rename CMA helpers to DMA helpers
Hi Danilo, On Mon, Jul 11, 2022 at 02:32:53PM +0200, Danilo Krummrich wrote: > On 7/7/22 19:39, Danilo Krummrich wrote: > > This patch series renames all CMA helpers to DMA helpers - considering the > > hierarchy of APIs (mm/cma -> dma -> gem/fb dma helpers) calling them DMA > > helpers seems to be more applicable. > > > > Additionally, commit e57924d4ae80 ("drm/doc: Task to rename CMA helpers") > > requests to rename the CMA helpers and implies that people seem to be > > confused > > about the naming. > > > > The patches are compile-time tested building a x86_64 kernel with > > `make allyesconfig && make drivers/gpu/drm`. > > > > Changes in v2: > >- Fixed up comments for consistent memory/address classification > > (DMA-contiguous) > >- Added a patch to rename struct drm_gem_dma_object.{paddr => dma_addr} > > > > Changes in v3: > >- Use a ccoccinelle script for > > "drm/gem: rename struct drm_gem_dma_object.{paddr => dma_addr}" for > > fixing > > up missing drivers and compile-test on x86_64, arm and arm64. > > > > Danilo Krummrich (4): > >drm/fb: rename FB CMA helpers to FB DMA helpers > >drm/gem: rename GEM CMA helpers to GEM DMA helpers > >drm/gem: rename struct drm_gem_dma_object.{paddr => dma_addr} > >drm/todo: remove task to rename CMA helpers > > Since those patches are based on Linus' tree they (unsurprisingly) don't > apply on drm-next cleanly. > > Please let me know if you want me to send a rebased version for drm-next. I would assume they will go through drm-misc, so rebasing on drm-misc-next would be best. > > > > Documentation/gpu/drm-kms-helpers.rst | 8 +- > > Documentation/gpu/drm-mm.rst | 16 +- > > Documentation/gpu/todo.rst| 13 - > > drivers/gpu/drm/Kconfig | 4 +- > > drivers/gpu/drm/Makefile | 6 +- > > drivers/gpu/drm/arm/Kconfig | 4 +- > > drivers/gpu/drm/arm/display/Kconfig | 2 +- > > .../arm/display/komeda/komeda_framebuffer.c | 12 +- > > .../gpu/drm/arm/display/komeda/komeda_kms.c | 10 +- > > drivers/gpu/drm/arm/hdlcd_crtc.c | 6 +- > > drivers/gpu/drm/arm/hdlcd_drv.c | 8 +- > > drivers/gpu/drm/arm/malidp_drv.c | 10 +- > > drivers/gpu/drm/arm/malidp_mw.c | 8 +- > > drivers/gpu/drm/arm/malidp_planes.c | 34 +- > > drivers/gpu/drm/armada/armada_gem.c | 6 +- > > drivers/gpu/drm/aspeed/Kconfig| 2 +- > > drivers/gpu/drm/aspeed/aspeed_gfx_crtc.c | 10 +- > > drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 8 +- > > drivers/gpu/drm/atmel-hlcdc/Kconfig | 2 +- > > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 6 +- > > .../gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 8 +- > > ...rm_fb_cma_helper.c => drm_fb_dma_helper.c} | 67 ++-- > > drivers/gpu/drm/drm_file.c| 2 +- > > drivers/gpu/drm/drm_format_helper.c | 4 +- > > ..._gem_cma_helper.c => drm_gem_dma_helper.c} | 314 +- > > drivers/gpu/drm/drm_mipi_dbi.c| 2 +- > > drivers/gpu/drm/fsl-dcu/Kconfig | 2 +- > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c | 8 +- > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_kms.c | 2 +- > > drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_plane.c | 10 +- > > drivers/gpu/drm/hisilicon/kirin/Kconfig | 2 +- > > .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 14 +- > > .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 4 +- > > drivers/gpu/drm/imx/Kconfig | 2 +- > > drivers/gpu/drm/imx/dcss/Kconfig | 2 +- > > drivers/gpu/drm/imx/dcss/dcss-kms.c | 6 +- > > drivers/gpu/drm/imx/dcss/dcss-plane.c | 18 +- > > drivers/gpu/drm/imx/imx-drm-core.c| 10 +- > > drivers/gpu/drm/imx/imx-drm.h | 2 +- > > drivers/gpu/drm/imx/ipuv3-crtc.c | 4 +- > > drivers/gpu/drm/imx/ipuv3-plane.c | 28 +- > > drivers/gpu/drm/ingenic/Kconfig | 2 +- > > drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 14 +- > > drivers/gpu/drm/ingenic/ingenic-ipu.c | 12 +- > > drivers/gpu/drm/kmb/Kconfig | 2 +- > > drivers/gpu/drm/kmb/kmb_drv.c | 6 +- > > drivers/gpu/drm/kmb/kmb_plane.c | 10 +- > > drivers/gpu/drm/mcde/Kconfig | 2 +- > > drivers/gpu/drm/mcde/mcde_display.c | 8 +- > > drivers/gpu/drm/mcde/mcde_drv.c | 10 +- > > drivers/gpu/drm/mediatek/Kconfig | 2 +- > > drivers/gpu/drm/mediatek/mtk_drm_drv.c| 2 +- > > drivers/gpu/drm/mediatek/mtk_drm_gem.c| 4 +- > > drivers/gpu/drm/meson/Kconfig | 2 +- > > drivers/gpu/drm/meson/meson_drv.c | 10 +- > > drivers/gpu/drm
Re: [PATCH 3/5] dma-buf: return only unsignaled fences in dma_fence_unwrap_for_each v3
Hi Christian, On 11.07.2022 14:25, Christian König wrote: Hi Karolina, Am 11.07.22 um 14:17 schrieb Karolina Drobnik: Hi Christian, On 11.07.2022 11:57, Christian König wrote: Hi Karolina, Am 11.07.22 um 11:44 schrieb Karolina Drobnik: Hi Christian, I'm sorry for digging this one out so late. On 06.05.2022 16:10, Christian König wrote: dma_fence_chain containers cleanup signaled fences automatically, so filter those out from arrays as well. v2: fix missing walk over the array v3: massively simplify the patch and actually update the description. Signed-off-by: Christian König --- include/linux/dma-fence-unwrap.h | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-fence-unwrap.h b/include/linux/dma-fence-unwrap.h index e7c219da4ed7..a4d342fef8e0 100644 --- a/include/linux/dma-fence-unwrap.h +++ b/include/linux/dma-fence-unwrap.h @@ -43,9 +43,13 @@ struct dma_fence *dma_fence_unwrap_next(struct dma_fence_unwrap *cursor); * Unwrap dma_fence_chain and dma_fence_array containers and deep dive into all * potential fences in them. If @head is just a normal fence only that one is * returned. + * + * Note that signalled fences are opportunistically filtered out, which + * means the iteration is potentially over no fence at all. */ #define dma_fence_unwrap_for_each(fence, cursor, head) \ for (fence = dma_fence_unwrap_first(head, cursor); fence; \ - fence = dma_fence_unwrap_next(cursor)) + fence = dma_fence_unwrap_next(cursor)) \ + if (!dma_fence_is_signaled(fence)) #endif It looks like this particular patch affects merging Sync Fences, which is reflected by failing IGT test (igt@sw_sync)[1]. The failing subtests are: - sync_merge - merging different fences on the same timeline, neither single nor merged fences are signaled - sync_merge_same - merging the fence with itself on the same timeline, the fence didn't signal at all - sync_multi_timeline_wait - merging different fences on different timelines; the subtest checks if counting fences of various states works. Currently, it can only see 2 active fences, 0 signaling (should be 2 active, 1 signaling) Reverting this commit on the top of drm-tip fixes the issue, but I'm not sure if it wouldn't impact other places in the code. Please let me know if I can be of any help. Thanks for letting me know. Not sure what's going on here, but I can take a look today if time permits. The reproduction with IGTs should be quite easy. You'll need to clone/download the IGT code and follow instructions for Building[1] the project (make sure you have meson and ninja installed): https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools&data=05%7C01%7Cchristian.koenig%40amd.com%7C9a9587aefd2d4ac2d86208da63375cb6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637931386683611766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=4WsMutcFJ2HwBqld%2BTv9N1Tx6cbFMwJJZ6kjm5rbfoI%3D&reserved=0 Once you have it up and running, go to /build/tests, and run the subtests: ./sw_sync --run sync_merge ./sw_sync --run sync_merge_same ./sw_sync --run sync_multi_timeline_wait You can run all the subtests with ./sw_sync, but I think these are the most relevant to you. Thanks, I've already managed to reproduce it. Not sure what's going on here, but could be that the test case was never correct in the first place. Need to double check. That's also a possibility, but I couldn't verify it before writing to you, as it's not my area of expertise. Thanks for taking a look at this. All the best, Karolina Thanks, Christian. Many thanks, Karolina -- [1] - https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fdrm%2Figt-gpu-tools%23building&data=05%7C01%7Cchristian.koenig%40amd.com%7C9a9587aefd2d4ac2d86208da63375cb6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637931386683611766%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FV0Ao6ra8EOyr4cOs4N7mCmpOEUUObTrgyOrd0tvEV8%3D&reserved=0 Do you have a description how to easy reproduce this? E.g. how to run just those specific igts? Thanks, Christian. All the best, Karolina
[PATCH v6 06/10] drm/bridge: use atomic enable/disable callbacks for panel bridge
Use atomic variants for panel bridge callback functions such that certain states like self-refresh can be accessed as part of enable/disable sequence. Reviewed-by: Dmitry Baryshkov Signed-off-by: Sankeerth Billakanti Signed-off-by: Vinod Polimera --- drivers/gpu/drm/bridge/panel.c | 20 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 0ee563e..eeb9546 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -108,28 +108,32 @@ static void panel_bridge_detach(struct drm_bridge *bridge) drm_connector_cleanup(connector); } -static void panel_bridge_pre_enable(struct drm_bridge *bridge) +static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_panel_prepare(panel_bridge->panel); } -static void panel_bridge_enable(struct drm_bridge *bridge) +static void panel_bridge_atomic_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_panel_enable(panel_bridge->panel); } -static void panel_bridge_disable(struct drm_bridge *bridge) +static void panel_bridge_atomic_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_panel_disable(panel_bridge->panel); } -static void panel_bridge_post_disable(struct drm_bridge *bridge) +static void panel_bridge_atomic_post_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); @@ -158,10 +162,10 @@ static void panel_bridge_debugfs_init(struct drm_bridge *bridge, static const struct drm_bridge_funcs panel_bridge_bridge_funcs = { .attach = panel_bridge_attach, .detach = panel_bridge_detach, - .pre_enable = panel_bridge_pre_enable, - .enable = panel_bridge_enable, - .disable = panel_bridge_disable, - .post_disable = panel_bridge_post_disable, + .atomic_pre_enable = panel_bridge_atomic_pre_enable, + .atomic_enable = panel_bridge_atomic_enable, + .atomic_disable = panel_bridge_atomic_disable, + .atomic_post_disable = panel_bridge_atomic_post_disable, .get_modes = panel_bridge_get_modes, .atomic_reset = drm_atomic_helper_bridge_reset, .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, -- 2.7.4