Re: [PATCH] drm/ttm: Use init_on_free to early release TTM BOs
Am 05.07.23 um 21:57 schrieb Rajneesh Bhardwaj:
Early release TTM BOs when the kernel default setting is init_on_free to
wipe out and reinitialize system memory chunks.
"Delay release the TTM BO when the kernels init_on_free setting is used."
This could potentially
optimize performance when an application does a lot of malloc/free style
allocations with unified system memory.
"This offloads the overhead of clearing the system memory to the work
item and potentially a different CPU. And is very beneficial when the
application does a lot of malloc/free style allocations of system memory."
With those changes to the commit message the patch is Reviewed-by:
Christian König .
I'm going to push that to drm-misc-next when you send it out once more.
Thanks,
Christian.
Signed-off-by: Rajneesh Bhardwaj
---
drivers/gpu/drm/ttm/ttm_bo.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 326a3d13a829..bd2e7e4f497a 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -347,6 +347,7 @@ static void ttm_bo_release(struct kref *kref)
if (!dma_resv_test_signaled(bo->base.resv,
DMA_RESV_USAGE_BOOKKEEP) ||
+ (want_init_on_free() && (bo->ttm != NULL)) ||
!dma_resv_trylock(bo->base.resv)) {
/* The BO is not idle, resurrect it for delayed destroy
*/
ttm_bo_flush_all_fences(bo);
Re: [PATCH] dma-buf: fix an error pointer vs NULL bug
Am 06.07.23 um 07:52 schrieb Dan Carpenter:
The __dma_fence_unwrap_merge() function is supposed to return NULL on
error. But the dma_fence_allocate_private_stub() returns error pointers
so check for that and covert the error pointers to NULL returns.
Otherwise, the callers do not expect error pointers and it leads to an
Oops.
Oh, good catch.
But I think we should probably change dma_fence_allocate_private_stub()
instead, that this function returns an ERR_PTR doesn't seem to make to
much sense.
Christian.
Fixes: f781f661e8c9 ("dma-buf: keep the signaling time of merged fences v3")
Signed-off-by: Dan Carpenter
---
drivers/dma-buf/dma-fence-unwrap.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-unwrap.c
b/drivers/dma-buf/dma-fence-unwrap.c
index c625bb2b5d56..d183eda0db89 100644
--- a/drivers/dma-buf/dma-fence-unwrap.c
+++ b/drivers/dma-buf/dma-fence-unwrap.c
@@ -94,8 +94,12 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int
num_fences,
* If we couldn't find a pending fence just return a private signaled
* fence with the timestamp of the last signaled one.
*/
- if (count == 0)
- return dma_fence_allocate_private_stub(timestamp);
+ if (count == 0) {
+ tmp = dma_fence_allocate_private_stub(timestamp);
+ if (IS_ERR(tmp))
+ return NULL;
+ return tmp;
+ }
array = kmalloc_array(count, sizeof(*array), GFP_KERNEL);
if (!array)
@@ -176,6 +180,8 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int
num_fences,
return_tmp:
kfree(array);
+ if (IS_ERR(tmp))
+ return NULL;
return tmp;
}
EXPORT_SYMBOL_GPL(__dma_fence_unwrap_merge);
[PATCH] dma-buf: fix an error pointer vs NULL bug
The __dma_fence_unwrap_merge() function is supposed to return NULL on
error. But the dma_fence_allocate_private_stub() returns error pointers
so check for that and covert the error pointers to NULL returns.
Otherwise, the callers do not expect error pointers and it leads to an
Oops.
Fixes: f781f661e8c9 ("dma-buf: keep the signaling time of merged fences v3")
Signed-off-by: Dan Carpenter
---
drivers/dma-buf/dma-fence-unwrap.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-unwrap.c
b/drivers/dma-buf/dma-fence-unwrap.c
index c625bb2b5d56..d183eda0db89 100644
--- a/drivers/dma-buf/dma-fence-unwrap.c
+++ b/drivers/dma-buf/dma-fence-unwrap.c
@@ -94,8 +94,12 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int
num_fences,
* If we couldn't find a pending fence just return a private signaled
* fence with the timestamp of the last signaled one.
*/
- if (count == 0)
- return dma_fence_allocate_private_stub(timestamp);
+ if (count == 0) {
+ tmp = dma_fence_allocate_private_stub(timestamp);
+ if (IS_ERR(tmp))
+ return NULL;
+ return tmp;
+ }
array = kmalloc_array(count, sizeof(*array), GFP_KERNEL);
if (!array)
@@ -176,6 +180,8 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int
num_fences,
return_tmp:
kfree(array);
+ if (IS_ERR(tmp))
+ return NULL;
return tmp;
}
EXPORT_SYMBOL_GPL(__dma_fence_unwrap_merge);
--
2.39.2
Re: [PATCH v2, 2/2] drm/mediatek: dp: Add the audio control to mtk_dp_data struct
[PATCH] drm/vkms Add hotplug support via configfs to VKMS.
This change adds the ability to read or write a "1" or a "0" to the newly added "connected" attribute of a connector in the vkms entry in configfs. A write will trigger a call to drm_kms_helper_hotplug_event, causing a hotplug uevent. With this we can write virtualized multidisplay tests that involve hotplugging displays (eg recompositing windows when a monitor is turned off). --- This is a first attempt and I am sure I could use some feedback. I have this working locally and I'm continuing to develop the test framework around this prototype, but I'm ready to switch gears back to addressing your feedback! This is also only my second patch ever to the kernel, so if my patch sending process is a little strange or I'm missing something feedback is appreciated. I also am basing these off of jshargo's not yet submitted configFS changes so I added an in-reply-to to that series. Not sure if that is alright. Signed-off-by: Brandon Pollack --- Documentation/gpu/vkms.rst| 2 +- drivers/gpu/drm/vkms/vkms_configfs.c | 96 ++- drivers/gpu/drm/vkms/vkms_drv.h | 11 +++ drivers/gpu/drm/vkms/vkms_output.c| 47 - drivers/gpu/drm/vkms/vkms_writeback.c | 2 +- 5 files changed, 138 insertions(+), 20 deletions(-) diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst index 2c342ef0fb7b..1eaae9f48693 100644 --- a/Documentation/gpu/vkms.rst +++ b/Documentation/gpu/vkms.rst @@ -144,7 +144,7 @@ Runtime Configuration We want to vkms instances without having to reload the module. Such configuration can be added as extensions to vkms's ConfigFS support. Use-cases: -- Hotplug/hotremove connectors on the fly (to be able to test DP MST handling +- Hotremove connectors on the fly (to be able to test DP MST handling of compositors). - Change output configuration: Plug/unplug screens, change EDID, allow changing diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c index f5eed6d23dcd..84cdb8d02ee7 100644 --- a/drivers/gpu/drm/vkms/vkms_configfs.c +++ b/drivers/gpu/drm/vkms/vkms_configfs.c @@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0+ +#include "drm/drm_probe_helper.h" #include #include #include @@ -30,7 +31,7 @@ * * mkdir /config/vkms/test * - * With your device created you'll find an new directory ready to be + * With your device created you'll find an new directory ready to be * configured:: * * /config @@ -39,6 +40,7 @@ * | `-- enabled * `-- test * |-- connectors + *`-- connected * |-- crtcs * |-- encoders * |-- planes @@ -48,25 +50,25 @@ * directories will let you configure a new object of that type. Adding new * objects will automatically create a set of default files and folders you can * use to configure that object. - * + * * For instance, we can set up a two-output device like so:: - * + * * DRM_PLANE_TYPE_PRIMARY=1 * DRM_PLANE_TYPE_CURSOR=2 * DRM_PLANE_TYPE_OVERLAY=0 - * + * * mkdir /config/vkms/test/planes/primary * echo $DRM_PLANE_TYPE_PRIMARY > /config/vkms/test/planes/primary/type - * + * * mkdir /config/vkms/test/planes/other_primary * echo $DRM_PLANE_TYPE_PRIMARY > /config/vkms/test/planes/other_primary/type - * + * * mkdir /config/vkms/test/planes/cursor * echo $DRM_PLANE_TYPE_CURSOR > /config/vkms/test/planes/cursor/type - * + * * mkdir /config/vkms/test/planes/overlay * echo $DRM_PLANE_TYPE_OVERLAY > /config/vkms/test/planes/overlay/type - * + * * mkdir /config/vkms/test/crtcs/crtc * mkdir /config/vkms/test/crtcs/crtc_other * mkdir /config/vkms/test/encoders/encoder @@ -75,25 +77,33 @@ * You can see that specific attributes, such as ``...//type``, can be * configured by writing into them. Associating objects together can be done via * symlinks:: - * + * * ln -s /config/vkms/test/encoders/encoder /config/vkms/test/connectors/connector/possible_encoders - * + * * ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/encoders/encoder/possible_crtcs/ * ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/primary/possible_crtcs/ * ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/cursor/possible_crtcs/ * ln -s /config/vkms/test/crtcs/crtc /config/vkms/test/planes/overlay/possible_crtcs/ - * + * * ln -s /config/vkms/test/crtcs/crtc_other /config/vkms/test/planes/overlay/possible_crtcs/ * ln -s /config/vkms/test/crtcs/crtc_other /config/vkms/test/planes/other_primary/possible_crtcs/ - * + * * Finally, to enable your configured device, just write 1 to the ``enabled`` * file:: - * + * * echo 1 > /config/vkms/test/enabled * + * By default no display is "connected" so to connect a connector you'll also + * have to write 1 to a connectors "connected" attribute:: + * + * echo 1 > /config/vkms/test/connectors/connector/connected + * + * One an v
Re: [32/39] drm: renesas: shmobile: Shutdown the display on remove
Hi,
On 2023/7/5 18:29, Geert Uytterhoeven wrote:
Hi Sui,
On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng wrote:
On 2023/6/22 17:21, Geert Uytterhoeven wrote:
When the device is unbound from the driver, the display may be active.
Make sure it gets shut down.
would you mind to give a short description why this is necessary.
That's a good comment.
It turned out that this is not really necessary here, but to avoid a regression
with "[PATCH 34/39] drm: renesas: shmobile: Atomic conversion part 1", where
it is needed to call drm_atomic_helper_shutdown().
As the comments for drm_atomic_helper_shutdown() says it is the
atomic version of drm_helper_force_disable_all(), I figured I had to
introduce a call to the latter first, before doing the atomic conversion.
Does that make sense?
I'm just noticed that I'm actually ask a duplicated question.
I didn't notice Laurent's remark about this patch.
I'm actually agree with Laurent that this function should be turned
into drm_atomic_helper_shutdown() finally.
Yes, I do noticed that you replace this function with in [PATCH 34/39],
Originally, I thought this patch could possibly merged with the [PATCH
34/39].
then, the net result of the merged two patch is equivalent to
adding drm_atomic_helper_shutdown() function only in the
shmob_drm_remove() function.
I also realized that it is necessary that disable the CRTC when the
driver going to leave.
Otherwise there are some plane resource still being referenced.
OK, I'm satisfy about you answer (if no other reviewers complains).
Thanks for you answer. :-)
Signed-off-by: Geert Uytterhoeven
Reviewed-by: Laurent Pinchart
--- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
+++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
@@ -16,6 +16,7 @@
#include
#include
+#include
#include
#include
#include
@@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device *pdev)
struct drm_device *ddev = &sdev->ddev;
drm_dev_unregister(ddev);
+ drm_helper_force_disable_all(ddev);
Is it that the DRM core recommend us to use
drm_atomic_helper_disable_all() ?
Well, drm_atomic_helper_shutdown() is a convenience wrapper
around drm_atomic_helper_disable_all()... But we can't call any
atomic helpers yet, before the conversion to atomic modesetting.
drm_kms_helper_poll_fini(ddev);
return 0;
}
Gr{oetje,eeting}s,
Geert
Re: [PATCH v2 1/3] drm/mediatek: Use devm_platform_ioremap_resource()
Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
On 7/5/23 21:58, Quan, Evan wrote: [AMD Official Use Only - General] Hi Andrew, I discussed with Mario about your proposal/concerns here. We believe some changes below might address your concerns. - place/move the wbrf_supported_producer check inside acpi_amd_wbrf_add_exclusion and acpi_amd_wbrf_add_exclusion - place the wbrf_supported_consumer check inside acpi_amd_wbrf_retrieve_exclusions So that the wbrf_supported_producer and wbrf_supported_consumer can be dropped. We made some prototypes and even performed some tests which showed technically it is absolutely practicable. However, we found several issues with that. - The overhead caused by the extra _producer/_consumer check on every calling of wbrf_add/remove/retrieve_ecxclusion. Especially when you consider there might be multiple producers and consumers in the system at the same time. And some of them might do in-use band/frequency switching frequently. One more piece of overhead that is in this same theme that Evan didn't mention is the case of a system "without" AMD's ACPI WBRF but the kernel was configured with it enabled. Think like a distro kernel. Moving it into add/remove exclusion would mean that every single time frequency changed by a producer the _DSM would attempt to be evaluated and fail. To avoid that extra call overhead after the first time would mean needing to keep a variable somewhere, and at that point what did you save? - Some extra costs caused by the "know it only at the last minute". For example, to support WBRF, amdgpu driver needs some preparations: install the notification hander, setup the delay workqueue(to handle possible events flooding) and even notify firmware engine to be ready. However, only on the 1st notification receiving, it is realized(reported by wbrf_supported_consumer check) the WBRF feature is actually not supported. All those extra costs can be actually avoided if we can know the WBRF is not supported at first. This could happen to other consumers and producers too. After a careful consideration, we think the changes do not benefit us much. It does not deserve us to spend extra efforts. Thus we would like to stick with original implementations. That is to have wbrf_supported_producer and wbrf_supported_consumer interfaces exposed. Then other drivers/subsystems can do necessary wbrf support check in advance and coordinate their actions accordingly. Please let us know your thoughts. BR, Evan -Original Message- From: Andrew Lunn Sent: Tuesday, July 4, 2023 9:07 PM To: Quan, Evan Cc: raf...@kernel.org; l...@kernel.org; Deucher, Alexander ; Koenig, Christian ; Pan, Xinhui ; airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net; da...@davemloft.net; eduma...@google.com; k...@kernel.org; pab...@redhat.com; Limonciello, Mario ; mdaen...@redhat.com; maarten.lankho...@linux.intel.com; tzimmerm...@suse.de; hdego...@redhat.com; jingyuwang_...@163.com; Lazar, Lijo ; jim.cro...@gmail.com; bellosili...@gmail.com; andrealm...@igalia.com; t...@redhat.com; j...@jsg.id.au; a...@arndb.de; linux-ker...@vger.kernel.org; linux- a...@vger.kernel.org; amd-...@lists.freedesktop.org; dri- de...@lists.freedesktop.org; linux-wirel...@vger.kernel.org; net...@vger.kernel.org Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations What is the purpose of this stage? Why would it not be supported for this device? This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) does not support the wbrf adding/removing for some device, it should speak that out so that the device can be aware of that. How much overhead is this adding? How deep do you need to go to find the BIOS does not support it? And how often is this called? Where do we want to add complexity? In the generic API? Or maybe a little deeper in the ACPI specific code? Andrew
RE: [PATCH V5 1/9] drivers core: Add support for Wifi band RF mitigations
[AMD Official Use Only - General] Hi Andrew, I discussed with Mario about your proposal/concerns here. We believe some changes below might address your concerns. - place/move the wbrf_supported_producer check inside acpi_amd_wbrf_add_exclusion and acpi_amd_wbrf_add_exclusion - place the wbrf_supported_consumer check inside acpi_amd_wbrf_retrieve_exclusions So that the wbrf_supported_producer and wbrf_supported_consumer can be dropped. We made some prototypes and even performed some tests which showed technically it is absolutely practicable. However, we found several issues with that. - The overhead caused by the extra _producer/_consumer check on every calling of wbrf_add/remove/retrieve_ecxclusion. Especially when you consider there might be multiple producers and consumers in the system at the same time. And some of them might do in-use band/frequency switching frequently. - Some extra costs caused by the "know it only at the last minute". For example, to support WBRF, amdgpu driver needs some preparations: install the notification hander, setup the delay workqueue(to handle possible events flooding) and even notify firmware engine to be ready. However, only on the 1st notification receiving, it is realized(reported by wbrf_supported_consumer check) the WBRF feature is actually not supported. All those extra costs can be actually avoided if we can know the WBRF is not supported at first. This could happen to other consumers and producers too. After a careful consideration, we think the changes do not benefit us much. It does not deserve us to spend extra efforts. Thus we would like to stick with original implementations. That is to have wbrf_supported_producer and wbrf_supported_consumer interfaces exposed. Then other drivers/subsystems can do necessary wbrf support check in advance and coordinate their actions accordingly. Please let us know your thoughts. BR, Evan > -Original Message- > From: Andrew Lunn > Sent: Tuesday, July 4, 2023 9:07 PM > To: Quan, Evan > Cc: raf...@kernel.org; l...@kernel.org; Deucher, Alexander > ; Koenig, Christian > ; Pan, Xinhui ; > airl...@gmail.com; dan...@ffwll.ch; johan...@sipsolutions.net; > da...@davemloft.net; eduma...@google.com; k...@kernel.org; > pab...@redhat.com; Limonciello, Mario ; > mdaen...@redhat.com; maarten.lankho...@linux.intel.com; > tzimmerm...@suse.de; hdego...@redhat.com; jingyuwang_...@163.com; > Lazar, Lijo ; jim.cro...@gmail.com; > bellosili...@gmail.com; andrealm...@igalia.com; t...@redhat.com; > j...@jsg.id.au; a...@arndb.de; linux-ker...@vger.kernel.org; linux- > a...@vger.kernel.org; amd-...@lists.freedesktop.org; dri- > de...@lists.freedesktop.org; linux-wirel...@vger.kernel.org; > net...@vger.kernel.org > Subject: Re: [PATCH V5 1/9] drivers core: Add support for Wifi band RF > mitigations > > > > What is the purpose of this stage? Why would it not be supported for > > > this device? > > This is needed for wbrf support via ACPI mechanism. If BIOS(AML code) > > does not support the wbrf adding/removing for some device, it should > speak that out so that the device can be aware of that. > > How much overhead is this adding? How deep do you need to go to find the > BIOS does not support it? And how often is this called? > > Where do we want to add complexity? In the generic API? Or maybe a little > deeper in the ACPI specific code? > >Andrew
[PATCH v2, 1/2] dt-bindings: display: mediatek: dp: Add compatible for MediaTek MT8188
Add dt-binding documentation of dp-tx for MediaTek MT8188 SoC. Signed-off-by: Shuijing Li Signed-off-by: Jitao Shi --- Changes in v2: add a mediatek,mt8188-edp-tx compatible per suggestion from the previous thread: https://lore.kernel.org/lkml/c4a4a900-c80d-b110-f10e-7fa2dae8b...@collabora.com/ --- .../devicetree/bindings/display/mediatek/mediatek,dp.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml index ff781f2174a0..2aef1eb32e11 100644 --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dp.yaml @@ -21,6 +21,8 @@ description: | properties: compatible: enum: + - mediatek,mt8188-dp-tx + - mediatek,mt8188-edp-tx - mediatek,mt8195-dp-tx - mediatek,mt8195-edp-tx -- 2.40.1
[PATCH v2,0/2] Add compatible to increase MT8188 audio control
Add dt-binding documentation of dp-tx for MediaTek MT8188 SoC. Mainly add the following two flag: 1.The audio packet arrangement function is to only arrange audio packets into the Hblanking area. In order to align with the HW default setting of g1200, this function needs to be turned off. 2.Due to the difference of HW, different dividers need to be set. Base on the branch of linus/master v6.4. Shuijing Li (2): dt-bindings: display: mediatek: dp: Add compatible for MediaTek MT8188 drm/mediatek: dp: Add the audio control to mtk_dp_data struct .../display/mediatek/mediatek,dp.yaml | 2 + drivers/gpu/drm/mediatek/mtk_dp.c | 47 ++- drivers/gpu/drm/mediatek/mtk_dp_reg.h | 6 +++ 3 files changed, 54 insertions(+), 1 deletion(-) -- 2.40.1
[PATCH v2, 2/2] drm/mediatek: dp: Add the audio control to mtk_dp_data struct
Mainly add the following two flag:
1.The audio packet arrangement function is to only arrange audio
packets into the Hblanking area. In order to align with the HW
default setting of g1200, this function needs to be turned off.
2.Due to the difference of HW, different dividers need to be set.
Signed-off-by: Shuijing Li
Signed-off-by: Jitao Shi
---
Changes in v2:
- change the variables' name to be more descriptive
- add a comment that describes the function of mtk_dp_audio_sample_arrange
- reduce indentation by doing the inverse check
- add a definition of some bits
- add support for mediatek, mt8188-edp-tx
per suggestion from the previous thread:
https://lore.kernel.org/lkml/ac0fcec9-a2fe-06cc-c727-189ef7bab...@collabora.com/
---
drivers/gpu/drm/mediatek/mtk_dp.c | 47 ++-
drivers/gpu/drm/mediatek/mtk_dp_reg.h | 6
2 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c
b/drivers/gpu/drm/mediatek/mtk_dp.c
index 64eee77452c0..8e1a13ab2ba2 100644
--- a/drivers/gpu/drm/mediatek/mtk_dp.c
+++ b/drivers/gpu/drm/mediatek/mtk_dp.c
@@ -139,6 +139,8 @@ struct mtk_dp_data {
unsigned int smc_cmd;
const struct mtk_dp_efuse_fmt *efuse_fmt;
bool audio_supported;
+ bool audio_pkt_in_hblank_area;
+ u16 audio_m_div2_bit;
};
static const struct mtk_dp_efuse_fmt mt8195_edp_efuse_fmt[MTK_DP_CAL_MAX] = {
@@ -647,7 +649,7 @@ static void mtk_dp_audio_sdp_asp_set_channels(struct mtk_dp
*mtk_dp,
static void mtk_dp_audio_set_divider(struct mtk_dp *mtk_dp)
{
mtk_dp_update_bits(mtk_dp, MTK_DP_ENC0_P0_30BC,
- AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
+ mtk_dp->data->audio_m_div2_bit,
AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_MASK);
}
@@ -1362,6 +1364,18 @@ static void
mtk_dp_sdp_set_down_cnt_init_in_hblank(struct mtk_dp *mtk_dp)
SDP_DOWN_CNT_INIT_IN_HBLANK_DP_ENC1_P0_MASK);
}
+static void mtk_dp_audio_sample_arrange(struct mtk_dp *mtk_dp)
+{
+ /* arrange audio packets into the Hblanking and Vblanking area */
+ if (!mtk_dp->data->audio_pkt_in_hblank_area)
+ return;
+
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3374, 0,
+ SDP_ASP_INSERT_IN_HBLANK_DP_ENC1_P0_MASK);
+ mtk_dp_update_bits(mtk_dp, MTK_DP_ENC1_P0_3374, 0,
+ SDP_DOWN_ASP_CNT_INIT_DP_ENC1_P0_MASK);
+}
+
static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp)
{
u32 sram_read_start = min_t(u32, MTK_DP_TBC_BUF_READ_START_ADDR,
@@ -1371,6 +1385,7 @@ static void mtk_dp_setup_tu(struct mtk_dp *mtk_dp)
MTK_DP_PIX_PER_ADDR);
mtk_dp_set_sram_read_start(mtk_dp, sram_read_start);
mtk_dp_setup_encoder(mtk_dp);
+ mtk_dp_audio_sample_arrange(mtk_dp);
mtk_dp_sdp_set_down_cnt_init_in_hblank(mtk_dp);
mtk_dp_sdp_set_down_cnt_init(mtk_dp, sram_read_start);
}
@@ -2616,11 +2631,31 @@ static int mtk_dp_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(mtk_dp_pm_ops, mtk_dp_suspend, mtk_dp_resume);
+static const struct mtk_dp_data mt8188_edp_data = {
+ .bridge_type = DRM_MODE_CONNECTOR_eDP,
+ .smc_cmd = MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE,
+ .efuse_fmt = mt8195_edp_efuse_fmt,
+ .audio_supported = false,
+ .audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
+};
+
+static const struct mtk_dp_data mt8188_dp_data = {
+ .bridge_type = DRM_MODE_CONNECTOR_DisplayPort,
+ .smc_cmd = MTK_DP_SIP_ATF_VIDEO_UNMUTE,
+ .efuse_fmt = mt8195_dp_efuse_fmt,
+ .audio_supported = true,
+ .audio_pkt_in_hblank_area = true,
+ .audio_m_div2_bit = MT8188_AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
+};
+
static const struct mtk_dp_data mt8195_edp_data = {
.bridge_type = DRM_MODE_CONNECTOR_eDP,
.smc_cmd = MTK_DP_SIP_ATF_EDP_VIDEO_UNMUTE,
.efuse_fmt = mt8195_edp_efuse_fmt,
.audio_supported = false,
+ .audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct mtk_dp_data mt8195_dp_data = {
@@ -2628,9 +2663,19 @@ static const struct mtk_dp_data mt8195_dp_data = {
.smc_cmd = MTK_DP_SIP_ATF_VIDEO_UNMUTE,
.efuse_fmt = mt8195_dp_efuse_fmt,
.audio_supported = true,
+ .audio_pkt_in_hblank_area = false,
+ .audio_m_div2_bit = AUDIO_M_CODE_MULT_DIV_SEL_DP_ENC0_P0_DIV_2,
};
static const struct of_device_id mtk_dp_of_match[] = {
+ {
+ .compatible = "mediatek,mt8188-edp-tx",
+ .data = &mt8188_edp_data,
+ },
+ {
+ .compatible = "mediatek,mt8188-dp-tx",
+ .data = &mt8188_dp_data,
+ },
{
.compatible = "mediatek,mt8195-edp-tx",
.data = &mt8195_edp_dat
Re: [PATCH v2 00/24] use vmalloc_array and vcalloc
Julia,
> The functions vmalloc_array and vcalloc were introduced in
>
> commit a8749a35c399 ("mm: vmalloc: introduce array allocation functions")
>
> but are not used much yet. This series introduces uses of
> these functions, to protect against multiplication overflows.
Applied #7 and #24 to 6.5/scsi-staging, thanks!
--
Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2 10/14] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf code
Hi Dmitry,
kernel test robot noticed the following build warnings:
[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.4 next-20230705]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/drm-msm-dpu-drop-enum-dpu_core_perf_data_bus_id/20230704-230618
base: git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:
https://lore.kernel.org/r/20230704150354.159882-11-dmitry.baryshkov%40linaro.org
patch subject: [PATCH v2 10/14] drm/msm/dpu: use dpu_perf_cfg in DPU core_perf
code
config: arm-defconfig
(https://download.01.org/0day-ci/archive/20230706/202307060717.jqn298i0-...@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project.git
f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce:
(https://download.01.org/0day-ci/archive/20230706/202307060717.jqn298i0-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202307060717.jqn298i0-...@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c:251:25: warning: variable
>> 'kms' is uninitialized when used here [-Wuninitialized]
if (atomic_dec_return(&kms->bandwidth_ref) > 0)
^~~
drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c:242:21: note: initialize the
variable 'kms' to silence this warning
struct dpu_kms *kms;
^
= NULL
1 warning generated.
vim +/kms +251 drivers/gpu/drm/msm/disp/dpu1/dpu_core_perf.c
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 230
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 231 /**
2785fd47959003 Lee Jones 2020-11-23 232 *
dpu_core_perf_crtc_release_bw() - request zero bandwidth
2785fd47959003 Lee Jones 2020-11-23 233 * @crtc: pointer to a crtc
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 234 *
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 235 * Function checks a state
variable for the crtc, if all pending commit
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 236 * requests are done, meaning
no more bandwidth is needed, release
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 237 * bandwidth request.
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 238 */
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 239 void
dpu_core_perf_crtc_release_bw(struct drm_crtc *crtc)
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 240 {
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 241struct dpu_crtc
*dpu_crtc;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 242struct dpu_kms *kms;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 243
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 244if (!crtc) {
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 245
DPU_ERROR("invalid crtc\n");
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 246return;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 247}
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 248
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 249dpu_crtc =
to_dpu_crtc(crtc);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 250
241b507c166fef Rob Clark 2019-08-20 @251if
(atomic_dec_return(&kms->bandwidth_ref) > 0)
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 252return;
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 253
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 254/* Release the
bandwidth */
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 255if
(kms->perf.enable_bw_release) {
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 256
trace_dpu_cmd_release_bw(crtc->base.id);
5b702d787b47e1 Stephen Boyd 2021-04-30 257
DRM_DEBUG_ATOMIC("Release BW crtc=%d\n", crtc->base.id);
cb88482e2570f6 Jayant Shekhar2019-06-18 258
dpu_crtc->cur_perf.bw_ctl = 0;
cb88482e2570f6 Jayant Shekhar2019-06-18 259
_dpu_core_perf_crtc_update_bus(kms, crtc);
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 260}
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 261 }
25fdd5933e4c0f Jeykumar Sankaran 2018-06-27 262
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH v3] drm/i915/pxp: Optimize GET_PARAM:PXP_STATUS
On 6/29/2023 6:44 PM, Alan Previn wrote:
After recent discussions with Mesa folks, it was requested
that we optimize i915's GET_PARAM for the PXP_STATUS without
changing the UAPI spec.
Add these additional optimizations:
- If any PXP initializatoin flow failed, then ensure that
we catch it so that we can change the returned PXP_STATUS
from "2" (i.e. 'PXP is supported but not yet ready')
to "-ENODEV". This typically should not happen and if it
does, we have a platform configuration issue.
- If a PXP arbitration session creation event failed
due to incorrect firmware version or blocking SOC fusing
or blocking BIOS configuration (platform reasons that won't
change if we retry), then reflect that blockage by also
returning -ENODEV in the GET_PARAM:PXP_STATUS.
- GET_PARAM:PXP_STATUS should not wait at all if PXP is
supported but non-i915 dependencies (component-driver /
firmware) we are still pending to complete the init flows.
In this case, just return "2" immediately (i.e. 'PXP is
supported but not yet ready').
Difference from prio revs:
v2: - Use a #define for the default readiness timeout (Vivaik).
- Improve comments around the failing of proxy-init.
v1: - Change the commit msg style to be imperative. (Jani)
- Rename timeout to timeout_ms. (Jani)
- Fix is_fw_err_platform_config to use higher order
param (pxp) first. (Jani)
Signed-off-by: Alan Previn
---
drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c | 10 +-
drivers/gpu/drm/i915/i915_getparam.c | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp.c | 40 ++
drivers/gpu/drm/i915/pxp/intel_pxp.h | 2 +-
drivers/gpu/drm/i915/pxp/intel_pxp_gsccs.c | 7 ++--
drivers/gpu/drm/i915/pxp/intel_pxp_tee.c | 7 ++--
drivers/gpu/drm/i915/pxp/intel_pxp_types.h | 9 +
7 files changed, 61 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
index 034b53a71541..21c2b7cce335 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_gsc_uc.c
@@ -62,8 +62,16 @@ static void gsc_work(struct work_struct *work)
}
ret = intel_gsc_proxy_request_handler(gsc);
- if (ret)
+ if (ret) {
+ if (actions & GSC_ACTION_FW_LOAD) {
+ /*
+* A failure right after firmware load means
the proxy-init
+* step has failed so mark GSC as not usable
after this
+*/
+ intel_uc_fw_change_status(&gsc->fw,
INTEL_UC_FIRMWARE_LOAD_FAIL);
+ }
goto out_put;
+ }
/* mark the GSC FW init as done the first time we run this */
if (actions & GSC_ACTION_FW_LOAD) {
diff --git a/drivers/gpu/drm/i915/i915_getparam.c
b/drivers/gpu/drm/i915/i915_getparam.c
index 890f2b382bee..5c3fec63cb4c 100644
--- a/drivers/gpu/drm/i915/i915_getparam.c
+++ b/drivers/gpu/drm/i915/i915_getparam.c
@@ -109,7 +109,7 @@ int i915_getparam_ioctl(struct drm_device *dev, void *data,
return value;
break;
case I915_PARAM_PXP_STATUS:
- value = intel_pxp_get_readiness_status(i915->pxp);
+ value = intel_pxp_get_readiness_status(i915->pxp, 0);
if (value < 0)
return value;
break;
diff --git a/drivers/gpu/drm/i915/pxp/intel_pxp.c
b/drivers/gpu/drm/i915/pxp/intel_pxp.c
index bb2e15329f34..e3b47525dc60 100644
--- a/drivers/gpu/drm/i915/pxp/intel_pxp.c
+++ b/drivers/gpu/drm/i915/pxp/intel_pxp.c
@@ -359,22 +359,46 @@ void intel_pxp_end(struct intel_pxp *pxp)
intel_runtime_pm_put(&i915->runtime_pm, wakeref);
}
+static bool pxp_required_fw_failed(struct intel_pxp *pxp)
+{
+ if (__intel_uc_fw_status(&pxp->ctrl_gt->uc.huc.fw) ==
INTEL_UC_FIRMWARE_LOAD_FAIL)
+ return true;
+ if (HAS_ENGINE(pxp->ctrl_gt, GSC0) &&
+ __intel_uc_fw_status(&pxp->ctrl_gt->uc.gsc.fw) ==
INTEL_UC_FIRMWARE_LOAD_FAIL)
+ return true;
+
+ return false;
+}
+
+static bool pxp_fw_dependencies_completed(struct intel_pxp *pxp)
+{
+ if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
+ return intel_pxp_gsccs_is_ready_for_sessions(pxp);
+
+ return pxp_component_bound(pxp);
+}
+
/*
* this helper is used by both intel_pxp_start and by
* the GET_PARAM IOCTL that user space calls. Thus, the
* return values here should match the UAPI spec.
*/
-int intel_pxp_get_readiness_status(struct intel_pxp *pxp)
+int intel_pxp_get_readiness_status(struct intel_pxp *pxp, int timeout_ms)
{
if (!intel_pxp_is_enabled(pxp))
return -ENODEV;
- if (HAS_ENGINE(pxp->ctrl_gt, GSC0))
Re: [PATCH v2 4/5] drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks
On 7/5/2023 12:30 PM, Ryan McCann wrote: For a device core dump, the registers of sub-blocks are printed under a title formatted as . For example, the csc sub-block for an SSPP main block "sspp_0" would be printed "sspp_0_sspp_csc0". The title is clearly redundant due to the duplicate "sspp" and "0" that exist in both the mainBlkName and sblkName. To eliminate this redundancy, remove the secondary "sspp" and "0" that exist in the sub-block name by elimanting the "sspp_" prefix and the concatenation of "num" that results in the redundant "0" suffix. Remove num parameter altogether from relevant macros as a consequence of it no longer being used. Signed-off-by: Ryan McCann --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 50 +- 1 file changed, 25 insertions(+), 25 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH v2 3/5] drm/msm/dpu: Define names for unnamed sblks
On 7/5/2023 12:30 PM, Ryan McCann wrote: Some sub-blocks in the hw catalog have not been given a name, so when the registers from that block are dumped, there is no name to reference. Define names for relevant sub-blocks to fix this. Signed-off-by: Ryan McCann --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH v2 2/5] drm/msm/dpu: Drop unused num argument from relevant macros
On 7/5/2023 12:30 PM, Ryan McCann wrote: Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub-block macros. Update calls to relevant macros to reflect change. Signed-off-by: Ryan McCann --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) Reviewed-by: Abhinav Kumar
Re: [PATCH v2 1/5] drm/msm: Update dev core dump to not print backwards
On 7/5/2023 12:30 PM, Ryan McCann wrote:
Device core dump add block method adds hardware blocks to dumping queue
with stack behavior which causes the hardware blocks to be printed in
reverse order. Change the addition to dumping queue data structure
from "list_add" to "list_add_tail" for FIFO queue behavior.
Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Abhinav Kumar
[Bug 217636] Commit edcfed8671 disables previously supported video resolutions (on AMD Rembrandt)
https://bugzilla.kernel.org/show_bug.cgi?id=217636 Alex Deucher (alexdeuc...@gmail.com) changed: What|Removed |Added CC||alexdeuc...@gmail.com --- Comment #1 from Alex Deucher (alexdeuc...@gmail.com) --- The revert of that patch is already on it's way to Linus. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: [PATCH v2 5/5] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks
On 05/07/2023 23:39, Ryan McCann wrote:
On 7/5/2023 1:22 PM, Dmitry Baryshkov wrote:
On 05/07/2023 22:30, Ryan McCann wrote:
Currently, the device core dump mechanism does not dump registers of
sub-blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Edit
dpu_kms_mdp_snapshot function to account for sub-blocks.
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 106
1 file changed, 82 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa8499de1b9f..c83f5d79e5c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct
msm_disp_state *disp_state, struct msm_k
int i;
struct dpu_kms *dpu_kms;
const struct dpu_mdss_cfg *cat;
+ void __iomem *mmio;
+ u32 base;
dpu_kms = to_dpu_kms(kms);
cat = dpu_kms->catalog;
+ mmio = dpu_kms->mmio;
pm_runtime_get_sync(&dpu_kms->pdev->dev);
/* dump CTL sub-blocks HW regs info */
for (i = 0; i < cat->ctl_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
- dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
mmio + cat->ctl[i].base,
+ "%s", cat->ctl[i].name);
This is not relevant to sub-blocks. If you wish to refactor the main
block printing, please split it to a separate commit.
Ok. I will split this commit into changes pertaining to sub-blocks and
changes to how the name of main blocks are printed. I would like to
print main block names as they appear in the catalog.
Yes, please.
Also, please note that `msm_disp_snapshot_add_block(, "%s",
block->name)` is redundant. We do not expect formatting characters in
block names. So, "%s" can be dropped.
Here, "%s" is used in order to print the the name of the main block from
the catalog. As mentioned above I can implement this in another commit.
Dropping the extra "%s" will get the same result.
/* dump DSPP sub-blocks HW regs info */
- for (i = 0; i < cat->dspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
- dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
+ for (i = 0; i < cat->dspp_count; i++) {
+ base = cat->dspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
mmio + base, "%s",
+ cat->dspp[i].name);
+
+ if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->dspp[i].sblk->pcc.len,
+ mmio + base + cat->dspp[i].sblk->pcc.base,
+ "%s_%s", cat->dspp[i].name,
+ cat->dspp[i].sblk->pcc.name);
+ }
+
/* dump INTF sub-blocks HW regs info */
for (i = 0; i < cat->intf_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
- dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
mmio + cat->intf[i].base,
+ "%s", cat->intf[i].name);
/* dump PP sub-blocks HW regs info */
- for (i = 0; i < cat->pingpong_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
- dpu_kms->mmio + cat->pingpong[i].base,
"pingpong_%d", i);
+ for (i = 0; i < cat->pingpong_count; i++) {
+ base = cat->pingpong[i].base;
+ msm_disp_snapshot_add_block(disp_state,
cat->pingpong[i].len, mmio + base, "%s",
+ cat->pingpong[i].name);
+
+ /* TE2 block has length of 0, so will not print it */
+
+ if (cat->pingpong[i].sblk &&
cat->pingpong[i].sblk->dither.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->pingpong[i].sblk->dither.len,
+ mmio + base +
cat->pingpong[i].sblk->dither.base,
+ "%s_%s", cat->pingpong[i].name,
+ cat->pingpong[i].sblk->dither.name);
+ }
/* dump SSPP sub-blocks HW regs info */
- for (i = 0; i < cat->sspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
- dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
+ for (i = 0; i < cat->sspp_count; i++) {
+ base = cat->sspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
mmio + cat->sspp[i].base,
+ "%s", cat->sspp[i].name);
+
+ if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->sspp[i].sblk->scaler_blk.len,
+ mmio + base +
cat->sspp[i].sblk->scaler_blk.base,
+ "%s_%s", cat->sspp[i].name,
+
Re: [PATCH v2 5/5] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks
On 7/5/2023 1:22 PM, Dmitry Baryshkov wrote:
On 05/07/2023 22:30, Ryan McCann wrote:
Currently, the device core dump mechanism does not dump registers of
sub-blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Edit
dpu_kms_mdp_snapshot function to account for sub-blocks.
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 106
1 file changed, 82 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa8499de1b9f..c83f5d79e5c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct
msm_disp_state *disp_state, struct msm_k
int i;
struct dpu_kms *dpu_kms;
const struct dpu_mdss_cfg *cat;
+ void __iomem *mmio;
+ u32 base;
dpu_kms = to_dpu_kms(kms);
cat = dpu_kms->catalog;
+ mmio = dpu_kms->mmio;
pm_runtime_get_sync(&dpu_kms->pdev->dev);
/* dump CTL sub-blocks HW regs info */
for (i = 0; i < cat->ctl_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
- dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, mmio
+ cat->ctl[i].base,
+ "%s", cat->ctl[i].name);
This is not relevant to sub-blocks. If you wish to refactor the main
block printing, please split it to a separate commit.
Ok. I will split this commit into changes pertaining to sub-blocks and
changes to how the name of main blocks are printed. I would like to
print main block names as they appear in the catalog.
Also, please note that `msm_disp_snapshot_add_block(, "%s",
block->name)` is redundant. We do not expect formatting characters in
block names. So, "%s" can be dropped.
Here, "%s" is used in order to print the the name of the main block from
the catalog. As mentioned above I can implement this in another commit.
/* dump DSPP sub-blocks HW regs info */
- for (i = 0; i < cat->dspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
- dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
+ for (i = 0; i < cat->dspp_count; i++) {
+ base = cat->dspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
mmio + base, "%s",
+ cat->dspp[i].name);
+
+ if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->dspp[i].sblk->pcc.len,
+ mmio + base + cat->dspp[i].sblk->pcc.base,
+ "%s_%s", cat->dspp[i].name,
+ cat->dspp[i].sblk->pcc.name);
+ }
+
/* dump INTF sub-blocks HW regs info */
for (i = 0; i < cat->intf_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
- dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
mmio + cat->intf[i].base,
+ "%s", cat->intf[i].name);
/* dump PP sub-blocks HW regs info */
- for (i = 0; i < cat->pingpong_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
- dpu_kms->mmio + cat->pingpong[i].base, "pingpong_%d",
i);
+ for (i = 0; i < cat->pingpong_count; i++) {
+ base = cat->pingpong[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
mmio + base, "%s",
+ cat->pingpong[i].name);
+
+ /* TE2 block has length of 0, so will not print it */
+
+ if (cat->pingpong[i].sblk &&
cat->pingpong[i].sblk->dither.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->pingpong[i].sblk->dither.len,
+ mmio + base +
cat->pingpong[i].sblk->dither.base,
+ "%s_%s", cat->pingpong[i].name,
+ cat->pingpong[i].sblk->dither.name);
+ }
/* dump SSPP sub-blocks HW regs info */
- for (i = 0; i < cat->sspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
- dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
+ for (i = 0; i < cat->sspp_count; i++) {
+ base = cat->sspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
mmio + cat->sspp[i].base,
+ "%s", cat->sspp[i].name);
+
+ if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->sspp[i].sblk->scaler_blk.len,
+ mmio + base +
cat->sspp[i].sblk->scaler_blk.base,
+ "%s_%s", cat->sspp[i].name,
+ cat->sspp[i].sblk->scaler_blk.name);
+
+ if (cat->sspp[i].sblk && cat->sspp[i].sblk
Re: [PATCH v2 3/5] drm/msm/dpu: Define names for unnamed sblks
On 05/07/2023 22:30, Ryan McCann wrote: Some sub-blocks in the hw catalog have not been given a name, so when the registers from that block are dumped, there is no name to reference. Define names for relevant sub-blocks to fix this. Signed-off-by: Ryan McCann --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) I'm not happy with this approach, but let's see how it goes. Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
[Bug 217636] New: Commit edcfed8671 disables previously supported video resolutions (on AMD Rembrandt)
https://bugzilla.kernel.org/show_bug.cgi?id=217636
Bug ID: 217636
Summary: Commit edcfed8671 disables previously supported video
resolutions (on AMD Rembrandt)
Product: Drivers
Version: 2.5
Hardware: All
OS: Linux
Status: NEW
Severity: normal
Priority: P3
Component: Video(DRI - non Intel)
Assignee: drivers_video-...@kernel-bugs.osdl.org
Reporter: saverio.p...@gmail.com
Regression: No
I have a laptop with an AMD Rembrandt APU (6800U).
The display has a native resolution of 2880x1800, however, via
`~/.config/monitors.xml`, I set it 1920x1200.
After the upgrade to the kernel v6.3.9, the resolution 1920x1200 is not allowed
anymore, and on login, I see a brief popup with an error.
After bisection, i've isolated the culprit to commit `edcfed8671`
("drm/amd/display: edp do not add non-edid timings").
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
Re: [PATCH v2 5/5] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks
On 05/07/2023 22:30, Ryan McCann wrote:
Currently, the device core dump mechanism does not dump registers of
sub-blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Edit
dpu_kms_mdp_snapshot function to account for sub-blocks.
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 106
1 file changed, 82 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa8499de1b9f..c83f5d79e5c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state
*disp_state, struct msm_k
int i;
struct dpu_kms *dpu_kms;
const struct dpu_mdss_cfg *cat;
+ void __iomem *mmio;
+ u32 base;
dpu_kms = to_dpu_kms(kms);
cat = dpu_kms->catalog;
+ mmio = dpu_kms->mmio;
pm_runtime_get_sync(&dpu_kms->pdev->dev);
/* dump CTL sub-blocks HW regs info */
for (i = 0; i < cat->ctl_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
- dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, mmio +
cat->ctl[i].base,
+ "%s", cat->ctl[i].name);
This is not relevant to sub-blocks. If you wish to refactor the main
block printing, please split it to a separate commit.
Also, please note that `msm_disp_snapshot_add_block(, "%s",
block->name)` is redundant. We do not expect formatting characters in
block names. So, "%s" can be dropped.
/* dump DSPP sub-blocks HW regs info */
- for (i = 0; i < cat->dspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
- dpu_kms->mmio + cat->dspp[i].base, "dspp_%d",
i);
+ for (i = 0; i < cat->dspp_count; i++) {
+ base = cat->dspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, mmio + base,
"%s",
+ cat->dspp[i].name);
+
+ if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->dspp[i].sblk->pcc.len,
+ mmio + base +
cat->dspp[i].sblk->pcc.base,
+ "%s_%s", cat->dspp[i].name,
+
cat->dspp[i].sblk->pcc.name);
+ }
+
/* dump INTF sub-blocks HW regs info */
for (i = 0; i < cat->intf_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
- dpu_kms->mmio + cat->intf[i].base, "intf_%d",
i);
+ msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, mmio +
cat->intf[i].base,
+ "%s", cat->intf[i].name);
/* dump PP sub-blocks HW regs info */
- for (i = 0; i < cat->pingpong_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
- dpu_kms->mmio + cat->pingpong[i].base,
"pingpong_%d", i);
+ for (i = 0; i < cat->pingpong_count; i++) {
+ base = cat->pingpong[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, mmio +
base, "%s",
+ cat->pingpong[i].name);
+
+ /* TE2 block has length of 0, so will not print it */
+
+ if (cat->pingpong[i].sblk && cat->pingpong[i].sblk->dither.len
> 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->pingpong[i].sblk->dither.len,
+ mmio + base +
cat->pingpong[i].sblk->dither.base,
+ "%s_%s",
cat->pingpong[i].name,
+
cat->pingpong[i].sblk->dither.name);
+ }
/* dump SSPP sub-blocks HW regs info */
- for (i = 0; i < cat->sspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
- dpu_kms->mmio + cat->sspp[i].base, "sspp_%d",
i);
+ for (i = 0; i < cat->sspp_count; i++) {
+ base = cat->sspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, mmio +
cat->sspp[i].base,
+ "%s", cat->sspp[i].name);
+
+ if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->sspp[i].sblk->scaler_blk.len,
+ mmio + base +
cat->sspp[i].sblk->scaler_blk.base,
+
Re: [PATCH v2 4/5] drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks
On 05/07/2023 22:30, Ryan McCann wrote: For a device core dump, the registers of sub-blocks are printed under a title formatted as . For example, the csc sub-block for an SSPP main block "sspp_0" would be printed "sspp_0_sspp_csc0". The title is clearly redundant due to the duplicate "sspp" and "0" that exist in both the mainBlkName and sblkName. To eliminate this redundancy, remove the secondary "sspp" and "0" that exist in the sub-block name by elimanting the "sspp_" prefix and the concatenation of "num" that results in the redundant "0" suffix. Remove num parameter altogether from relevant macros as a consequence of it no longer being used. Signed-off-by: Ryan McCann --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 50 +- 1 file changed, 25 insertions(+), 25 deletions(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [PATCH v2 2/5] drm/msm/dpu: Drop unused num argument from relevant macros
On 05/07/2023 22:30, Ryan McCann wrote: Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub-block macros. Update calls to relevant macros to reflect change. Signed-off-by: Ryan McCann --- drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++-- 1 file changed, 10 insertions(+), 10 deletions(-) Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On 05/07/2023 19:53, Maxime Ripard wrote: On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: On Wed, 5 Jul 2023 at 17:24, Maxime Ripard wrote: On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: Either way, I'm not really sure it's a good idea to multiply the capabilities flags of the DSI host, and we should just stick to the spec. If the spec says that we have to support DSC while video is output, then that's what the panels should expect. Except some panels supports DSC & non-DSC, Video and Command mode, and all that is runtime configurable. How do you handle that ? In this case, most of the constraints are going to be on the encoder still so it should be the one driving it. The panel will only care about which mode has been selected, but it shouldn't be the one driving it, and thus we still don't really need to expose the host capabilities. This is an interesting perspective. This means that we can and actually have to extend the drm_display_mode with the DSI data and compression information. I wouldn't extend drm_display_mode, but extending one of the state structures definitely. We already have some extra variables in drm_connector_state for HDMI, I don't think it would be a big deal to add a few for MIPI-DSI. We also floated the idea for a while to create bus-specific states, with helpers to match. Maybe it would be a good occasion to start doing it? For example, the panel that supports all four types for the 1080p should export several modes: 1920x1080-command 1920x1080-command-DSC 1920x1080-video 1920x1080-video-DSC where video/command and DSC are some kinds of flags and/or information in the drm_display_mode? Ideally DSC also has several sub-flags, which denote what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, etc). So we have two things to do, right? We need to expose what the panel can take (ie, EDID for HDMI), and then we need to tell it what we picked (infoframes). We already express the former in mipi_dsi_device, so we could extend the flags stored there. And then, we need to tie what the DSI host chose to a given atomic state so the panel knows what was picked and how it should set everything up. This is definitely something we need. Marijn has been stuck with the panels that support different models ([1]). Would you prefer a separate API for this kind of information or abusing atomic_enable() is fine from your point of view? My vote would be for going with existing operations, with the slight fear of ending up with another DSI-specific hack (like pre_enable_prev_first). I don't think we can get away without getting access to the atomic_state from the panel at least. Choosing one setup over another is likely going to depend on the mode, and that's only available in the state. We don't have to go the whole way though and create the sub-classes of drm_connector_state, but I think we should at least provide it to the panel. What do you think of creating a new set of atomic_* callbacks for panels, call that new set of functions from msm and start from there? We are (somewhat) bound by the panel_bridge, but yeah, it seems possible. -- With best wishes Dmitry
[PATCH] drm/ttm: Use init_on_free to early release TTM BOs
Early release TTM BOs when the kernel default setting is init_on_free to
wipe out and reinitialize system memory chunks. This could potentially
optimize performance when an application does a lot of malloc/free style
allocations with unified system memory.
Signed-off-by: Rajneesh Bhardwaj
---
drivers/gpu/drm/ttm/ttm_bo.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 326a3d13a829..bd2e7e4f497a 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -347,6 +347,7 @@ static void ttm_bo_release(struct kref *kref)
if (!dma_resv_test_signaled(bo->base.resv,
DMA_RESV_USAGE_BOOKKEEP) ||
+ (want_init_on_free() && (bo->ttm != NULL)) ||
!dma_resv_trylock(bo->base.resv)) {
/* The BO is not idle, resurrect it for delayed destroy
*/
ttm_bo_flush_all_fences(bo);
--
2.17.1
[PATCH v2 5/5] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks
Currently, the device core dump mechanism does not dump registers of
sub-blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Edit
dpu_kms_mdp_snapshot function to account for sub-blocks.
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 106
1 file changed, 82 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa8499de1b9f..c83f5d79e5c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state
*disp_state, struct msm_k
int i;
struct dpu_kms *dpu_kms;
const struct dpu_mdss_cfg *cat;
+ void __iomem *mmio;
+ u32 base;
dpu_kms = to_dpu_kms(kms);
cat = dpu_kms->catalog;
+ mmio = dpu_kms->mmio;
pm_runtime_get_sync(&dpu_kms->pdev->dev);
/* dump CTL sub-blocks HW regs info */
for (i = 0; i < cat->ctl_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
- dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, mmio +
cat->ctl[i].base,
+ "%s", cat->ctl[i].name);
/* dump DSPP sub-blocks HW regs info */
- for (i = 0; i < cat->dspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
- dpu_kms->mmio + cat->dspp[i].base, "dspp_%d",
i);
+ for (i = 0; i < cat->dspp_count; i++) {
+ base = cat->dspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, mmio
+ base, "%s",
+ cat->dspp[i].name);
+
+ if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->dspp[i].sblk->pcc.len,
+ mmio + base +
cat->dspp[i].sblk->pcc.base,
+ "%s_%s", cat->dspp[i].name,
+
cat->dspp[i].sblk->pcc.name);
+ }
+
/* dump INTF sub-blocks HW regs info */
for (i = 0; i < cat->intf_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
- dpu_kms->mmio + cat->intf[i].base, "intf_%d",
i);
+ msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, mmio
+ cat->intf[i].base,
+ "%s", cat->intf[i].name);
/* dump PP sub-blocks HW regs info */
- for (i = 0; i < cat->pingpong_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
- dpu_kms->mmio + cat->pingpong[i].base,
"pingpong_%d", i);
+ for (i = 0; i < cat->pingpong_count; i++) {
+ base = cat->pingpong[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
mmio + base, "%s",
+ cat->pingpong[i].name);
+
+ /* TE2 block has length of 0, so will not print it */
+
+ if (cat->pingpong[i].sblk && cat->pingpong[i].sblk->dither.len
> 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->pingpong[i].sblk->dither.len,
+ mmio + base +
cat->pingpong[i].sblk->dither.base,
+ "%s_%s",
cat->pingpong[i].name,
+
cat->pingpong[i].sblk->dither.name);
+ }
/* dump SSPP sub-blocks HW regs info */
- for (i = 0; i < cat->sspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
- dpu_kms->mmio + cat->sspp[i].base, "sspp_%d",
i);
+ for (i = 0; i < cat->sspp_count; i++) {
+ base = cat->sspp[i].base;
+ msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, mmio
+ cat->sspp[i].base,
+ "%s", cat->sspp[i].name);
+
+ if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->sspp[i].sblk->scaler_blk.len,
+ mmio + base +
cat->sspp[i].sblk->scaler_blk.base,
+ "%s_%s", cat->sspp[i].name,
+
cat->sspp[i].sblk->scaler_blk.name);
+
+ if (cat->sspp[i].sblk && cat->sspp[i].sblk->csc_blk.len > 0)
+ msm_disp_snapshot_add_block(disp_state,
cat->sspp[i].sblk->csc_blk.len,
+
[PATCH v2 0/5] Add support to print sub-block registers in dpu hw catalog
The purpose of this patch series is to add support to print the registers of sub-blocks in the DPU hardware catalog and fix the order in which all hardware blocks are dumped for a device core dump. This involves: 1. Changing data structure from stack to queue to fix the printing order of the device core dump. 2. Removing redundant suffix of sub-block names. 3. Removing redundant prefix of sub-block names. 4. Eliminating unused variable from relevant macros. 5. Defining names for sub-blocks that have not yet been defined. 6. Implementing wrapper function that prints the registers of sub-blocks when there is a need. Sample Output of the sspp_0 block and its sub-blocks for devcore dump: ==sspp_0== ...registers ... sspp_0_scaler ... ... sspp_0_csc ... ... next_block ... --- Changes in v2: - Changed spelling "sub block" to "sub-block" or "sblk". - Capitalized DPU. - Eliminated multiplexer/wrapper function. Inlined instead. - Changed if statements from feature checks to length checks. - Squashed prefix and suffix patch into one. - Link to v1: https://lore.kernel.org/r/20230622-devcoredump_patch-v1-0-3b2cdcc6a...@quicinc.com --- Ryan McCann (5): drm/msm: Update dev core dump to not print backwards drm/msm/dpu: Drop unused num argument from relevant macros drm/msm/dpu: Define names for unnamed sblks drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks drm/msm/dpu: Update dev core dump to dump registers of sub-blocks drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c| 90 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 106 +- drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 +- 3 files changed, 128 insertions(+), 70 deletions(-) --- base-commit: a0364260213c96f6817f7e85cdce293cb743460f change-id: 20230622-devcoredump_patch-df7e8f6fd632 Best regards, -- Ryan McCann
[PATCH v2 2/5] drm/msm/dpu: Drop unused num argument from relevant macros
Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub-block
macros. Update calls to relevant macros to reflect change.
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 0de507d4d7b7..9f9d5ac3992f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -288,7 +288,7 @@ static const uint32_t wb2_formats[] = {
.rotation_cfg = rot_cfg, \
}
-#define _DMA_SBLK(num, sdma_pri) \
+#define _DMA_SBLK(sdma_pri) \
{ \
.maxdwnscale = SSPP_UNITY_SCALE, \
.maxupscale = SSPP_UNITY_SCALE, \
@@ -323,10 +323,10 @@ static const struct dpu_sspp_sub_blks sdm845_vig_sblk_2 =
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_3 =
_VIG_SBLK("3", 8, DPU_SSPP_SCALER_QSEED3);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK("8", 1);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK("9", 2);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_2 = _DMA_SBLK("10", 3);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK("11", 4);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK(1);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK(2);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_2 = _DMA_SBLK(3);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK(4);
#define SSPP_BLK(_name, _id, _base, _len, _features, \
_sblk, _xinid, _type, _clkctrl) \
@@ -366,10 +366,10 @@ static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 =
_VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 =
_VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4);
-static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK("12", 5);
-static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK("13", 6);
+static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK(5);
+static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK(6);
-#define _VIG_SBLK_NOSCALE(num, sdma_pri) \
+#define _VIG_SBLK_NOSCALE(sdma_pri) \
{ \
.maxdwnscale = SSPP_UNITY_SCALE, \
.maxupscale = SSPP_UNITY_SCALE, \
@@ -380,8 +380,8 @@ static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 =
_DMA_SBLK("13", 6);
.virt_num_formats = ARRAY_SIZE(plane_formats), \
}
-static const struct dpu_sspp_sub_blks qcm2290_vig_sblk_0 =
_VIG_SBLK_NOSCALE("0", 2);
-static const struct dpu_sspp_sub_blks qcm2290_dma_sblk_0 = _DMA_SBLK("8", 1);
+static const struct dpu_sspp_sub_blks qcm2290_vig_sblk_0 =
_VIG_SBLK_NOSCALE(2);
+static const struct dpu_sspp_sub_blks qcm2290_dma_sblk_0 = _DMA_SBLK(1);
/*
* MIXER sub blocks config
--
2.25.1
[PATCH v2 4/5] drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks
For a device core dump, the registers of sub-blocks are printed under a
title formatted as . For example, the csc sub-block
for an SSPP main block "sspp_0" would be printed "sspp_0_sspp_csc0". The
title is clearly redundant due to the duplicate "sspp" and "0" that exist
in both the mainBlkName and sblkName. To eliminate this redundancy, remove
the secondary "sspp" and "0" that exist in the sub-block name by
elimanting the "sspp_" prefix and the concatenation of "num" that results
in the redundant "0" suffix. Remove num parameter altogether from relevant
macros as a consequence of it no longer being used.
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 50 +-
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 79e495dbc11d..836efa074a35 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -252,15 +252,15 @@ static const uint32_t wb2_formats[] = {
*/
/* SSPP common configuration */
-#define _VIG_SBLK(num, sdma_pri, qseed_ver) \
+#define _VIG_SBLK(sdma_pri, qseed_ver) \
{ \
.maxdwnscale = MAX_DOWNSCALE_RATIO, \
.maxupscale = MAX_UPSCALE_RATIO, \
.smart_dma_priority = sdma_pri, \
- .scaler_blk = {.name = STRCAT("sspp_scaler", num), \
+ .scaler_blk = {.name = "scaler", \
.id = qseed_ver, \
.base = 0xa00, .len = 0xa0,}, \
- .csc_blk = {.name = STRCAT("sspp_csc", num), \
+ .csc_blk = {.name = "csc", \
.id = DPU_SSPP_CSC_10BIT, \
.base = 0x1a00, .len = 0x100,}, \
.format_list = plane_formats_yuv, \
@@ -270,15 +270,15 @@ static const uint32_t wb2_formats[] = {
.rotation_cfg = NULL, \
}
-#define _VIG_SBLK_ROT(num, sdma_pri, qseed_ver, rot_cfg) \
+#define _VIG_SBLK_ROT(sdma_pri, qseed_ver, rot_cfg) \
{ \
.maxdwnscale = MAX_DOWNSCALE_RATIO, \
.maxupscale = MAX_UPSCALE_RATIO, \
.smart_dma_priority = sdma_pri, \
- .scaler_blk = {.name = STRCAT("sspp_scaler", num), \
+ .scaler_blk = {.name = "scaler", \
.id = qseed_ver, \
.base = 0xa00, .len = 0xa0,}, \
- .csc_blk = {.name = STRCAT("sspp_csc", num), \
+ .csc_blk = {.name = "csc", \
.id = DPU_SSPP_CSC_10BIT, \
.base = 0x1a00, .len = 0x100,}, \
.format_list = plane_formats_yuv, \
@@ -300,13 +300,13 @@ static const uint32_t wb2_formats[] = {
}
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
- _VIG_SBLK("0", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
- _VIG_SBLK("1", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
- _VIG_SBLK("2", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
- _VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
.rot_maxheight = 1088,
@@ -315,13 +315,13 @@ static const struct dpu_rotation_cfg
dpu_rot_sc7280_cfg_v2 = {
};
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_0 =
- _VIG_SBLK("0", 5, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(5, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_1 =
- _VIG_SBLK("1", 6, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(6, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_2 =
- _VIG_SBLK("2", 7, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(7, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_3 =
- _VIG_SBLK("3", 8, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(8, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK(1);
static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK(2);
@@ -341,31 +341,31 @@ static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 =
_DMA_SBLK(4);
}
static const struct dpu_sspp_sub_blks sc7180_vig_sblk_0 =
- _VIG_SBLK("0", 4, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(4, DPU_SSPP_SCALER_QSEED4);
static const
[PATCH v2 3/5] drm/msm/dpu: Define names for unnamed sblks
Some sub-blocks in the hw catalog have not been given a name, so when the
registers from that block are dumped, there is no name to reference.
Define names for relevant sub-blocks to fix this.
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 9f9d5ac3992f..79e495dbc11d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -444,12 +444,12 @@ static const struct dpu_lm_sub_blks qcm2290_lm_sblk = {
* DSPP sub blocks config
*/
static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
- .pcc = {.id = DPU_DSPP_PCC, .base = 0x1700,
+ .pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
.len = 0x90, .version = 0x10007},
};
static const struct dpu_dspp_sub_blks sm8150_dspp_sblk = {
- .pcc = {.id = DPU_DSPP_PCC, .base = 0x1700,
+ .pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
.len = 0x90, .version = 0x4},
};
@@ -465,19 +465,19 @@ static const struct dpu_dspp_sub_blks sm8150_dspp_sblk = {
* PINGPONG sub blocks config
*/
static const struct dpu_pingpong_sub_blks sdm845_pp_sblk_te = {
- .te2 = {.id = DPU_PINGPONG_TE2, .base = 0x2000, .len = 0x0,
+ .te2 = {.name = "te2", .id = DPU_PINGPONG_TE2, .base = 0x2000, .len =
0x0,
.version = 0x1},
- .dither = {.id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+ .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
.len = 0x20, .version = 0x1},
};
static const struct dpu_pingpong_sub_blks sdm845_pp_sblk = {
- .dither = {.id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+ .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
.len = 0x20, .version = 0x1},
};
static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
- .dither = {.id = DPU_PINGPONG_DITHER, .base = 0xe0,
+ .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0xe0,
.len = 0x20, .version = 0x2},
};
@@ -517,13 +517,13 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk
= {
* DSC sub blocks config
*/
static const struct dpu_dsc_sub_blks dsc_sblk_0 = {
- .enc = {.base = 0x100, .len = 0x100},
- .ctl = {.base = 0xF00, .len = 0x10},
+ .enc = {.name = "enc", .base = 0x100, .len = 0x100},
+ .ctl = {.name = "ctl", .base = 0xF00, .len = 0x10},
};
static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
- .enc = {.base = 0x200, .len = 0x100},
- .ctl = {.base = 0xF80, .len = 0x10},
+ .enc = {.name = "enc", .base = 0x200, .len = 0x100},
+ .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
};
#define DSC_BLK(_name, _id, _base, _features) \
--
2.25.1
[PATCH v2 1/5] drm/msm: Update dev core dump to not print backwards
Device core dump add block method adds hardware blocks to dumping queue
with stack behavior which causes the hardware blocks to be printed in
reverse order. Change the addition to dumping queue data structure
from "list_add" to "list_add_tail" for FIFO queue behavior.
Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Ryan McCann
---
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index acfe1b31e079..add72bbc28b1 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -192,5 +192,5 @@ void msm_disp_snapshot_add_block(struct msm_disp_state
*disp_state, u32 len,
new_blk->base_addr = base_addr;
msm_disp_state_dump_regs(&new_blk->state, new_blk->size, base_addr);
- list_add(&new_blk->node, &disp_state->blocks);
+ list_add_tail(&new_blk->node, &disp_state->blocks);
}
--
2.25.1
Re: [PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence
On 7/5/23 18:54, Gurchetan Singh wrote:
> On Wed, Jun 28, 2023 at 8:58 AM Gurchetan Singh
> wrote:
>>
>> We don't want to create a fence for every command submission. It's
>> only necessary when userspace provides a waitable token for submission.
>> This could be:
>>
>> 1) bo_handles, to be used with VIRTGPU_WAIT
>> 2) out_fence_fd, to be used with dma_fence apis
>> 3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
>>+ DRM event API
>> 4) syncobjs in the future
>>
>> The use case for just submitting a command to the host, and expected
>> no response. For example, gfxstream has GFXSTREAM_CONTEXT_PING that
>> just wakes up the host side worker threads. There's also
>> CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.
>>
>> This prevents the need to signal the automatically created
>> virtio_gpu_fence.
>>
>> Signed-off-by: Gurchetan Singh
>> Reviewed-by:
>> ---
>> v2: Fix indent (Dmitry)
>>
>> drivers/gpu/drm/virtio/virtgpu_submit.c | 10 +++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c
>> b/drivers/gpu/drm/virtio/virtgpu_submit.c
>> index cf3c04b16a7a..8c7e15c31164 100644
>> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
>> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
>> @@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct
>> virtio_gpu_submit *submit,
>>
>> memset(submit, 0, sizeof(*submit));
>>
>> - out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
>> - if (!out_fence)
>> - return -ENOMEM;
>> + if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
>> + ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
>> + (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||
>> + exbuf->num_bo_handles)
>> + out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx,
>> ring_idx);
>> + else
>> + out_fence = NULL;
>>
>> err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
>> if (err) {
>> --
>
> Ping for additional reviews or merge.
I tested this patch with virgl,venus and nctx. No problems spotted.
Going to apply it tomorrow if there won't be additional comments from
anyone.
Tested-by: Dmitry Osipenko
--
Best regards,
Dmitry
Re: [PATCH] backlight: led_bl: fix initial power state
Hi Daniel, On Wed, Jul 05, 2023 at 03:07:31PM +0100, Daniel Thompson wrote: > On Tue, Jul 04, 2023 at 07:07:31PM +0200, Sam Ravnborg wrote: > > Hi Daniel, > > > > > > @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device > > > > *pdev) > > > > props.type = BACKLIGHT_RAW; > > > > props.max_brightness = priv->max_brightness; > > > > props.brightness = priv->default_brightness; > > > > - props.power = (priv->default_brightness > 0) ? > > > > FB_BLANK_POWERDOWN : > > > > - FB_BLANK_UNBLANK; > > > > + props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK > > > > : > > > > + FB_BLANK_POWERDOWN; > > > > > > The logic was wrong before but I think will still be wrong after the > > > change too (e.g. the bogus logic is probably avoiding backlight flicker > > > in some use cases). > > > > > > The logic here needs to be similar to what pwm_bl.c implements in > > > pwm_backlight_initial_power_state(). Whilst it might be better > > > to implement this in led_bl_get_leds() let me show what I mean > > > in code that fits in the current line: > > > > > > /* > > >* Activate the backlight if the LEDs are already lit *or* > > >* there is no phandle link (meaning the backlight power > > >* state cannot be synced with the display state). > > >*/ > > > props.power = (active_at_boot || !dev->node->phandle) ? > > > FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN; > > > > > The following code does the same using helpers: > > > > if (active_at_boot || !dev->node->phandle)) > > backlight_enable(bd); > > else > > backlight_disable(bd); > > > > The code needs to execute after backlight_device_register() so maybe not > > so great an idea?!? > > It would introduce a need for a bunch of new locks since userspace > could get in between device creation and the call to the helpers. I thought we were safe while in the probe function, but I have been fooled by the driver model before. > > Additionally, it is even correct for a driver to forcefully set > fb_blank to powerdown during the probe? All current drivers set > fb_blank to unblank during the probe. fb_blank is more or less unused. I thought that Lee applied the patch set to eliminate most users, but I see that this is not the case. I need to resend one day. Some (at least one) drivers update .power after registering the device, so if this is racy then these drivers could use some care. Anyway, looking at how many drivers access backlight_properties direct is is futile to try to avoid it. So the approach you suggest seems the best way to do it. Sam
Re: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
Hi Paul,
On Mon, Jul 03, 2023 at 11:47:14PM +0200, Paul Cercueil wrote:
> Register a backlight device to be able to switch between all the gamma
> levels.
>
> Signed-off-by: Paul Cercueil
> ---
> drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> index 7fd9444b42c5..b4f87d6244cb 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> @@ -8,6 +8,7 @@
> * Andrzej Hajda
> */
>
> +#include
> #include
> #include
> #include
> @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx)
> return 0;
> }
>
> +static int ld9040_bl_update_status(struct backlight_device *dev)
> +{
> + struct ld9040 *ctx = dev_get_drvdata(&dev->dev);
There is also the helper bl_get_data() - that do the same.
Sam
Re: [PATCH 1/1] drm: panel-simple: add missing bus flags for Tianma tm070jvhg[30/33]
Hi Alexander,
On Wed, Jul 05, 2023 at 03:22:39PM +0200, Alexander Stein wrote:
> Hi,
>
> another gentle ping
>
> Best regards,
> Alexander
>
> Am Mittwoch, 25. Januar 2023, 15:52:15 CEST schrieb Alexander Stein:
> > From: Markus Niebel
> >
> > The DE signal is active high on this display, fill in the missing
> > bus_flags. This aligns panel_desc with its display_timing.
> >
> > Fixes: 9a2654c0f62a ("drm/panel: Add and fill drm_panel type field")
> > Fixes: b3bfcdf8a3b6 ("drm/panel: simple: add Tianma TM070JVHG33")
> > Signed-off-by: Markus Niebel
> > Signed-off-by: Alexander Stein
Reviewed-by: Sam Ravnborg
I hope someone else will pick it up.
Sam
Re: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
Hi Paul,
On Wed, Jul 05, 2023 at 04:38:05PM +0200, Paul Cercueil wrote:
> Hi Neil,
>
> Le mercredi 05 juillet 2023 à 15:57 +0200, Neil Armstrong a écrit :
> > On 03/07/2023 23:47, Paul Cercueil wrote:
> > > Register a backlight device to be able to switch between all the
> > > gamma
> > > levels.
> > >
> > > Signed-off-by: Paul Cercueil
> > > ---
> > > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40
> > >
> > > 1 file changed, 40 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > > b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > > index 7fd9444b42c5..b4f87d6244cb 100644
> > > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > > @@ -8,6 +8,7 @@
> > > * Andrzej Hajda
> > > */
> > >
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx)
> > > return 0;
> > > }
> > >
> > > +static int ld9040_bl_update_status(struct backlight_device *dev)
> > > +{
> > > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev);
> > > +
> > > + ctx->brightness = dev->props.brightness;
Use backlight_get_brightness(dev);
> > > + ld9040_brightness_set(ctx);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int ld9040_bl_get_intensity(struct backlight_device *dev)
> > > +{
> > > + if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
> > > + dev->props.power == FB_BLANK_UNBLANK)
> > > + return dev->props.brightness;
> > > +
> > > + return 0;
> > > +}
> >
> > You can totally drop the _get_brightness.
>
> The current behaviour is to return 0 when the framebuffer is blanked. A
> few drivers do that so I thought it was the norm; and the backlight
> core doesn't do that by default (and just uses dev->props.brightness).
>
> It is not clear to me if that's the preferred behaviour. The
> "backlight_get_brightness" function in backlight.h seems to suggest
> that the current behaviour is correct, unless it is not supposed to be
> used in the backlight_ops.get_brightness() callback. Then in that case
> some other drivers get it wrong too.
Several drivers get it wrong.
You are supposed to provide get_brightness only when you read back a
value from the HW, which is not the case here so just drop it is the
right choice.
Sam
Re: [PATCH v2] drm/arm/komeda: Remove component framework and add a simple encoder
Hi Faiz,
On Tue, Jul 04, 2023 at 10:04:54PM +0530, Faiz Abbas wrote:
> The Komeda driver always expects the remote connector node to initialize
> an encoder. It uses the component aggregator framework which consists
> of component->bind() calls used to initialize the remote encoder and attach
> it to the crtc. This makes it incompatible with connector drivers which
> implement drm_bridge APIs.
>
> Remove all component framework calls from the komeda driver and declare and
> attach an encoder inside komeda_crtc_add().
>
> The remote connector driver has to implement the DRM bridge APIs which
> can be used to glue the encoder to the remote connector. Since we
> usually pair this with a component encoder that also implements a
> drm_bridge, dropping support is not expected to affect users of this
> driver.
Thanks for updating the commit description, I think it shows the intent better.
When I'm trying to apply your patch to drm-misc next (or any branch that
matters)
it fails because ...
>
> Signed-off-by: Faiz Abbas
> ---
> .../gpu/drm/arm/display/komeda/komeda_crtc.c | 23 +++-
> .../gpu/drm/arm/display/komeda/komeda_drv.c | 57 ++-
> .../gpu/drm/arm/display/komeda/komeda_kms.c | 10 +---
> .../gpu/drm/arm/display/komeda/komeda_kms.h | 3 +
> 4 files changed, 32 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> index 4cc07d6bb9d82..e5a8a80b173f4 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c
> @@ -13,6 +13,8 @@
> #include
... this line is different in my tree. It looks like your tree is missing
commit e3b63718827880 ("drm/arm/komeda: Remove unnecessary include
statements for drm_crtc_helper.h"), which has been applied in early January.
Best regards,
Liviu
> #include
> #include
> +#include
> +#include
>
> #include "komeda_dev.h"
> #include "komeda_kms.h"
> @@ -613,9 +615,11 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
> struct komeda_crtc *kcrtc)
> {
> struct drm_crtc *crtc = &kcrtc->base;
> + struct drm_device *base = &kms->base;
> + struct drm_bridge *bridge;
> int err;
>
> - err = drm_crtc_init_with_planes(&kms->base, crtc,
> + err = drm_crtc_init_with_planes(base, crtc,
> get_crtc_primary(kms, kcrtc), NULL,
> &komeda_crtc_funcs, NULL);
> if (err)
> @@ -625,6 +629,23 @@ static int komeda_crtc_add(struct komeda_kms_dev *kms,
>
> crtc->port = kcrtc->master->of_output_port;
>
> +
> + /* Construct an encoder for each pipeline and attach it to the remote
> + * bridge
> + */
> + kcrtc->encoder.possible_crtcs = drm_crtc_mask(crtc);
> + err = drm_simple_encoder_init(base, &kcrtc->encoder,
> + DRM_MODE_ENCODER_TMDS);
> + if (err)
> + return err;
> +
> + bridge = devm_drm_of_get_bridge(base->dev, kcrtc->master->of_node,
> + KOMEDA_OF_PORT_OUTPUT, 0);
> + if (IS_ERR(bridge))
> + return PTR_ERR(bridge);
> +
> + err = drm_bridge_attach(&kcrtc->encoder, bridge, NULL, 0);
> +
> drm_crtc_enable_color_mgmt(crtc, 0, true, KOMEDA_COLOR_LUT_SIZE);
>
> return err;
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> index 9fce4239d4ad4..98e7ca1ad8fe7 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
> @@ -7,7 +7,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -27,13 +26,11 @@ struct komeda_dev *dev_to_mdev(struct device *dev)
> return mdrv ? mdrv->mdev : NULL;
> }
>
> -static void komeda_unbind(struct device *dev)
> +static int komeda_platform_remove(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> struct komeda_drv *mdrv = dev_get_drvdata(dev);
>
> - if (!mdrv)
> - return;
> -
> komeda_kms_detach(mdrv->kms);
>
> if (pm_runtime_enabled(dev))
> @@ -45,10 +42,13 @@ static void komeda_unbind(struct device *dev)
>
> dev_set_drvdata(dev, NULL);
> devm_kfree(dev, mdrv);
> +
> + return 0;
> }
>
> -static int komeda_bind(struct device *dev)
> +static int komeda_platform_probe(struct platform_device *pdev)
> {
> + struct device *dev = &pdev->dev;
> struct komeda_drv *mdrv;
> int err;
>
> @@ -88,52 +88,7 @@ static int komeda_bind(struct device *dev)
> free_mdrv:
> devm_kfree(dev, mdrv);
> return err;
> -}
> -
> -static const struct component_master_ops komeda_master_ops = {
> - .bind = komeda_bind,
> - .unbind = komeda_unbind,
> -};
>
> -static void komeda_add_slave(struct device *master,
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote: > On Wed, 5 Jul 2023 at 17:24, Maxime Ripard wrote: > > > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > > > capabilities flags of the DSI host, and we should just stick to the > > > > > > spec. If the spec says that we have to support DSC while video is > > > > > > output, then that's what the panels should expect. > > > > > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > > > all that is runtime configurable. How do you handle that ? > > > > > > > > In this case, most of the constraints are going to be on the encoder > > > > still so it should be the one driving it. The panel will only care about > > > > which mode has been selected, but it shouldn't be the one driving it, > > > > and thus we still don't really need to expose the host capabilities. > > > > > > This is an interesting perspective. This means that we can and actually > > > have > > > to extend the drm_display_mode with the DSI data and compression > > > information. > > > > I wouldn't extend drm_display_mode, but extending one of the state > > structures definitely. > > > > We already have some extra variables in drm_connector_state for HDMI, > > I don't think it would be a big deal to add a few for MIPI-DSI. > > > > We also floated the idea for a while to create bus-specific states, with > > helpers to match. Maybe it would be a good occasion to start doing it? > > > > > For example, the panel that supports all four types for the 1080p should > > > export several modes: > > > > > > 1920x1080-command > > > 1920x1080-command-DSC > > > 1920x1080-video > > > 1920x1080-video-DSC > > > > > > where video/command and DSC are some kinds of flags and/or information in > > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > > > etc). > > > > So we have two things to do, right? We need to expose what the panel can > > take (ie, EDID for HDMI), and then we need to tell it what we picked > > (infoframes). > > > > We already express the former in mipi_dsi_device, so we could extend the > > flags stored there. > > > > And then, we need to tie what the DSI host chose to a given atomic state > > so the panel knows what was picked and how it should set everything up. > > This is definitely something we need. Marijn has been stuck with the > panels that support different models ([1]). > > Would you prefer a separate API for this kind of information or > abusing atomic_enable() is fine from your point of view? > > My vote would be for going with existing operations, with the slight > fear of ending up with another DSI-specific hack (like > pre_enable_prev_first). I don't think we can get away without getting access to the atomic_state from the panel at least. Choosing one setup over another is likely going to depend on the mode, and that's only available in the state. We don't have to go the whole way though and create the sub-classes of drm_connector_state, but I think we should at least provide it to the panel. What do you think of creating a new set of atomic_* callbacks for panels, call that new set of functions from msm and start from there? Maxime signature.asc Description: PGP signature
Re: [PATCH libdrm v2 04/10] util: Add missing big-endian RGB16 frame buffer formats
Hi Ville,
On Mon, Jul 11, 2022 at 2:34 PM Geert Uytterhoeven wrote:
> 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) },
> >
> > 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.
Nowadays we have drmGetFormatName(), which returns an allocated string
with the name for a format code.
Using that helper in string_to_fourcc() would mean looping over the
table, and for each entry, calling drmGetFormatName(), comparing the
name, and freeing the name again.
Would that be acceptable?
Thanks!
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 v4 2/8] drm/atomic: Add support for mouse hotspots
On 7/4/23 01:08, Pekka Paalanen wrote: On Mon, 3 Jul 2023 14:06:56 -0700 Michael Banack wrote: Hi, I can speak to the virtual mouse/console half of this from the VMware-side. I believe Zack's preparing a new set of comments here that can speak to most of your concerns, but I'll answer some of the other questions directly. On 6/29/23 01:03, Pekka Paalanen wrote: Is it really required that the hotspot coordinates fall inside the cursor plane? Will the atomic commit be rejected otherwise? Most console systems require the hotspot to get within the cursor image, but in theory it's semantically meaningful to have it extend outside the image. VMware's clients in particular will clamp the hotspot to the dimension of the cursor image if we receive one that's out of bounds. So I would assume the right thing to do here would be to allow it and let the clients figure out how to best handle it. Hi, if it is normal that clients clamp the hotspot to inside the cursor image, then I would come to the opposite conclusion: KMS UAPI needs to require the hotspot to be within the cursor image. Otherwise the results would be unpredictable, if clients still continue to clamp it anyway. I would assume that clients in use today are not prepared to handle hotspot outside the cursor image. It is also not a big deal to require that. I think it would be very rare to not have hotspot inside the cursor image, and even if it happened, the only consequence would be that the guest display server falls back to rendered cursor instead of a cursor plane. That may happen any time anyway, if an application sets e.g. a huge cursor that exceeds cursor plane size limits. Hypervisors are normally more privileged than the kernel, so any hypervisor/remoting client here really should be dealing with this case rather than trusting the kernel to handle it for them. From that perspective, we would normally try to preserve the application/guest semantics as much as possible, and then that gives us the ability to deal with this on the hypervisor side if it turns out that there's a critical case with the hotspot outside the image that we need to handle. But that said, while I've seen real Guests do this in the past, I don't recall seeing this from any modern operating systems, so I don't think it's a big deal for us either way. What I'm after with the question is that the requirement must be spelled out clearly if it is there, or not even hinted at if it's not there. Agreed. The question of which input device corresponds to which cursor plane might be good to answer too. I presume the VM runner is configured to expose exactly one of each, so there can be only one association? As far as I know, all of the VM consoles are written as though they taking the place of what would the the physical monitors and input devices on a native machine. So they assume that there is one user, sitting in front of one console, and all monitors/input devices are being used by that user. Ok, but having a single user does not mean that there cannot be multiple independent pointers, especially on Wayland. The same with keyboards. True, and if the userspace is doing anything complicated here, the hypervisor has to be responsible for ensuring that whatever it's doing works with that, or else this system won't work. I don't know that the kernel is in a good position to police that. Any more complicated multi-user/multi-cursor setup would have to be coordinated through a lot of layers (ie from the VM's userspace/kernel and then through hypervisor/client-consoles), and as far as I know nobody has tried to plumb that all the way through. Even physical multi-user/multi-console configurations like that are rare. Right. So if there a VM viewer client running on a Wayland system, that viewer client may be presented with an arbitrary number of independent pointer/keyboard/touchscreen input devices. Then it is up to the client to pick one at a time to pass through to the VM. That's fine. I just think it would be good to document, that VM/viewer systems expect to expose just a single pointer to the guest, hence it is obvious which input device in the guest is associated with all the cursor planes in the guest. I don't have a problem adding something that suggests what we think the hypervisors are doing, but I would be a little cautious trying to prescribe what the hypervisors should be doing here. I certainly can't speak for all of them, but we at least do a lot of odd tricks to keep this coordinated that violate what would normally be abstraction layers in a physical system such as having the mouse and the display adapter collude. Ultimately it's the hypervisor that is responsible for doing the synchronization correctly, and the kernel really isn't involved there besides ferrying the right information down. Btw. what do you do if a guest display server simultaneously uses multiple cursor planes, assuming there are multiple
Re: [PATCH libdrm] amdgpu: Use %ll to format 64-bit integers
On Wed, Jul 5, 2023 at 5:17 PM Geert Uytterhoeven
wrote:
>
> On 32-bit:
>
> ../tests/amdgpu/amdgpu_stress.c: In function ‘alloc_bo’:
> ../tests/amdgpu/amdgpu_stress.c:178:49: warning: format ‘%lx’ expects
> argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t’ {aka
> ‘long long unsigned int’} [-Wformat=]
> fprintf(stdout, "Allocated BO number %u at 0x%lx, domain 0x%x, size
> %lu\n",
>~~^
>%llx
>num_buffers++, addr, domain, size);
>
> ../tests/amdgpu/amdgpu_stress.c:178:72: warning: format ‘%lu’ expects
> argument of type ‘long unsigned int’, but argument 6 has type ‘uint64_t’ {aka
> ‘long long unsigned int’} [-Wformat=]
> fprintf(stdout, "Allocated BO number %u at 0x%lx, domain 0x%x, size
> %lu\n",
> ~~^
> %llu
>num_buffers++, addr, domain, size);
>
> ../tests/amdgpu/amdgpu_stress.c: In function ‘submit_ib’:
> ../tests/amdgpu/amdgpu_stress.c:276:54: warning: format ‘%lx’ expects
> argument of type ‘long unsigned int’, but argument 5 has type ‘uint64_t’ {aka
> ‘long long unsigned int’} [-Wformat=]
> fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
> bytes took %lu usec\n",
> ~~^
> %llx
>count, from, virtual[from], to, virtual[to], copied, delta / 1000);
> ~
> ../tests/amdgpu/amdgpu_stress.c:276:65: warning: format ‘%lx’ expects
> argument of type ‘long unsigned int’, but argument 7 has type ‘uint64_t’ {aka
> ‘long long unsigned int’} [-Wformat=]
> fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
> bytes took %lu usec\n",
>~~^
>%llx
>count, from, virtual[from], to, virtual[to], copied, delta / 1000);
>~~~
> ../tests/amdgpu/amdgpu_stress.c:276:70: warning: format ‘%lu’ expects
> argument of type ‘long unsigned int’, but argument 8 has type ‘uint64_t’ {aka
> ‘long long unsigned int’} [-Wformat=]
> fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
> bytes took %lu usec\n",
> ~~^
> %llu
>count, from, virtual[from], to, virtual[to], copied, delta / 1000);
> ~~
> ../tests/amdgpu/amdgpu_stress.c:276:85: warning: format ‘%lu’ expects
> argument of type ‘long unsigned int’, but argument 9 has type ‘uint64_t’ {aka
> ‘long long unsigned int’} [-Wformat=]
> fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
> bytes took %lu usec\n",
>
> ~~^
>
> %llu
>count, from, virtual[from], to, virtual[to], copied, delta / 1000);
>
> ../tests/amdgpu/amdgpu_stress.c: In function ‘parse_size’:
> ../tests/amdgpu/amdgpu_stress.c:296:24: warning: format ‘%li’ expects
> argument of type ‘long int *’, but argument 3 has type ‘uint64_t *’ {aka
> ‘long long unsigned int *’} [-Wformat=]
> if (sscanf(optarg, "%li%1[kmgKMG]", &size, ext) < 1) {
> ~~^ ~
> %lli
> ../tests/amdgpu/amdgpu_stress.c: In function ‘main’:
> ../tests/amdgpu/amdgpu_stress.c:378:45: warning: format ‘%lu’ expects
> argument of type ‘long unsigned int’, but argument 3 has type ‘uint64_t’ {aka
> ‘long long unsigned int’} [-Wformat=]
> fprintf(stderr, "Buffer size to small %lu\n", size);
>~~^
>%llu
>
> Fix this by using the proper "%ll" format specifier prefix.
>
> Fixes: d77ccdf3ba6f5a39 ("amdgpu: add amdgpu_stress utility v2")
> Signed-off-by: Geert Uytterhoeven
Scrap it, now it fails on 64-bit :-(
> --- a/tests/amdgpu/amdgpu_stress.c
> +++ b/tests/amdgpu/amdgpu_stress.c
> @@ -175,7 +175,7 @@ int alloc_bo(uint32_t domain, uint64_t size)
>
> resources[num_buffers] = bo;
> virtual[num_buffers] = addr;
> - fprintf(stdout, "Allocated BO number %u at 0x%lx, domain 0x%x, size
> %lu\n",
> + fprintf(stdout, "Allocated BO number %u at 0x%llx,
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On 05/07/2023 16:24, Maxime Ripard wrote: On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: Either way, I'm not really sure it's a good idea to multiply the capabilities flags of the DSI host, and we should just stick to the spec. If the spec says that we have to support DSC while video is output, then that's what the panels should expect. Except some panels supports DSC & non-DSC, Video and Command mode, and all that is runtime configurable. How do you handle that ? In this case, most of the constraints are going to be on the encoder still so it should be the one driving it. The panel will only care about which mode has been selected, but it shouldn't be the one driving it, and thus we still don't really need to expose the host capabilities. This is an interesting perspective. This means that we can and actually have to extend the drm_display_mode with the DSI data and compression information. I wouldn't extend drm_display_mode, but extending one of the state structures definitely. We already have some extra variables in drm_connector_state for HDMI, I don't think it would be a big deal to add a few for MIPI-DSI. We also floated the idea for a while to create bus-specific states, with helpers to match. Maybe it would be a good occasion to start doing it? For example, the panel that supports all four types for the 1080p should export several modes: 1920x1080-command 1920x1080-command-DSC 1920x1080-video 1920x1080-video-DSC where video/command and DSC are some kinds of flags and/or information in the drm_display_mode? Ideally DSC also has several sub-flags, which denote what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, etc). So we have two things to do, right? We need to expose what the panel can take (ie, EDID for HDMI), and then we need to tell it what we picked (infoframes). We already express the former in mipi_dsi_device, so we could extend the flags stored there. And then, we need to tie what the DSI host chose to a given atomic state so the panel knows what was picked and how it should set everything up. Yep this looks like a good plan Neil Another option would be to get this handled via the bus format negotiation, but that sounds like worse idea to me. Yeah, I'm not really fond of the format negociation stuff either. Maxime
Re: [PATCH v2] drm/virtio: conditionally allocate virtio_gpu_fence
On Wed, Jun 28, 2023 at 8:58 AM Gurchetan Singh
wrote:
>
> We don't want to create a fence for every command submission. It's
> only necessary when userspace provides a waitable token for submission.
> This could be:
>
> 1) bo_handles, to be used with VIRTGPU_WAIT
> 2) out_fence_fd, to be used with dma_fence apis
> 3) a ring_idx provided with VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK
>+ DRM event API
> 4) syncobjs in the future
>
> The use case for just submitting a command to the host, and expected
> no response. For example, gfxstream has GFXSTREAM_CONTEXT_PING that
> just wakes up the host side worker threads. There's also
> CROSS_DOMAIN_CMD_SEND which just sends data to the Wayland server.
>
> This prevents the need to signal the automatically created
> virtio_gpu_fence.
>
> Signed-off-by: Gurchetan Singh
> Reviewed-by:
> ---
> v2: Fix indent (Dmitry)
>
> drivers/gpu/drm/virtio/virtgpu_submit.c | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_submit.c
> b/drivers/gpu/drm/virtio/virtgpu_submit.c
> index cf3c04b16a7a..8c7e15c31164 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_submit.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_submit.c
> @@ -168,9 +168,13 @@ static int virtio_gpu_init_submit(struct
> virtio_gpu_submit *submit,
>
> memset(submit, 0, sizeof(*submit));
>
> - out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);
> - if (!out_fence)
> - return -ENOMEM;
> + if ((exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_OUT) ||
> + ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX) &&
> + (vfpriv->ring_idx_mask & BIT_ULL(ring_idx))) ||
> + exbuf->num_bo_handles)
> + out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx,
> ring_idx);
> + else
> + out_fence = NULL;
>
> err = virtio_gpu_fence_event_create(dev, file, out_fence, ring_idx);
> if (err) {
> --
Ping for additional reviews or merge.
> 2.41.0.162.gfafddb0af9-goog
>
Re: [PATCH v5 1/1] drm/doc: Document DRM device reset expectations
On Wed, Jul 5, 2023 at 3:32 AM Michel Dänzer wrote: > > On 7/5/23 08:30, Marek Olšák wrote: > > On Tue, Jul 4, 2023, 03:55 Michel Dänzer wrote: > > On 7/4/23 04:34, Marek Olšák wrote: > > > On Mon, Jul 3, 2023, 03:12 Michel Dänzer > > wrote: > > > On 6/30/23 22:32, Marek Olšák wrote: > > > > On Fri, Jun 30, 2023 at 11:11 AM Michel Dänzer > > wrote: > > > >> On 6/30/23 16:59, Alex Deucher wrote: > > > >>> On Fri, Jun 30, 2023 at 10:49 AM Sebastian Wick > > > >>> mailto:sebastian.w...@redhat.com> > > wrote: > > > On Tue, Jun 27, 2023 at 3:23 PM André Almeida > > wrote: > > > > > > > > +Robustness > > > > +-- > > > > + > > > > +The only way to try to keep an application working after a > > reset is if it > > > > +complies with the robustness aspects of the graphical API > > that it is using. > > > > + > > > > +Graphical APIs provide ways to applications to deal with > > device resets. However, > > > > +there is no guarantee that the app will use such features > > correctly, and the > > > > +UMD can implement policies to close the app if it is a > > repeating offender, > > > > +likely in a broken loop. This is done to ensure that it > > does not keep blocking > > > > +the user interface from being correctly displayed. This > > should be done even if > > > > +the app is correct but happens to trigger some bug in the > > hardware/driver. > > > > > > I still don't think it's good to let the kernel arbitrarily > > kill > > > processes that it thinks are not well-behaved based on some > > heuristics > > > and policy. > > > > > > Can't this be outsourced to user space? Expose the > > information about > > > processes causing a device and let e.g. systemd deal with > > coming up > > > with a policy and with killing stuff. > > > >>> > > > >>> I don't think it's the kernel doing the killing, it would be > > the UMD. > > > >>> E.g., if the app is guilty and doesn't support robustness the > > UMD can > > > >>> just call exit(). > > > >> > > > >> It would be safer to just ignore API calls[0], similarly to > > what is done until the application destroys the context with robustness. > > Calling exit() likely results in losing any unsaved work, whereas at least > > some applications might otherwise allow saving the work by other means. > > > > > > > > That's a terrible idea. Ignoring API calls would be identical > > to a freeze. You might as well disable GPU recovery because the result > > would be the same. > > > > > > No GPU recovery would affect everything using the GPU, whereas > > this affects only non-robust applications. > > > > > > which is currently the majority. > > > > Not sure where you're going with this. Applications need to use > > robustness to be able to recover from a GPU hang, and the GPU needs to be > > reset for that. So disabling GPU reset is not the same as what we're > > discussing here. > > > > > > > > - non-robust contexts: call exit(1) immediately, which is the > > best way to recover > > > > > > That's not the UMD's call to make. > > > > > > That's absolutely the UMD's call to make because that's mandated by > > the hw and API design > > > > Can you point us to a spec which mandates that the process must be > > killed in this case? > > > > > > > and only driver devs know this, which this thread is a proof of. The > > default behavior is to skip all command submission if a non-robust context > > is lost, which looks like a freeze. That's required to prevent infinite > > hangs from the same context and can be caused by the side effects of the > > GPU reset itself, not by the cause of the previous hang. The only way out > > of that is killing the process. > > > > The UMD killing the process is not the only way out of that, and doing > > so is overreach on its part. The UMD is but one out of many components in a > > process, not the main one or a special one. It doesn't get to decide when > > the process must die, certainly not under circumstances where it must be > > able to continue while ignoring API calls (that's required for robustness). > > > > > > You're mixing things up. Robust apps don't any special action from a UMD. > > Only non-robust apps need to be killed for proper recovery with the only > > other alternative being not updating the window/screen, > > I'm saying they don't "need" to be killed, since the UMD must be able to keep > going while ignoring API calls (or it couldn't support robustness). It's a > choice, one which is not for the UMD to make. > > > > Also it's already used and required
Re: [PATCH] dt-bindings: cleanup DTS example whitespaces
On Sun, 02 Jul 2023 20:23:08 +0200, Krzysztof Kozlowski wrote: > The DTS code coding style expects spaces around '=' sign. > > Signed-off-by: Krzysztof Kozlowski > > --- > > Rob, > > Maybe this could go via your tree? Rebased on your for-next: > v6.4-rc2-45-gf0ac35049606 > --- > .../bindings/arm/arm,coresight-cti.yaml| 18 +- > .../bindings/arm/keystone/ti,sci.yaml | 8 > .../devicetree/bindings/display/msm/gmu.yaml | 2 +- > .../display/panel/samsung,s6e8aa0.yaml | 2 +- > .../display/rockchip/rockchip-vop.yaml | 4 ++-- > .../bindings/iio/adc/ti,adc108s102.yaml| 2 +- > .../bindings/media/renesas,rzg2l-cru.yaml | 4 ++-- > .../devicetree/bindings/media/renesas,vin.yaml | 4 ++-- > .../devicetree/bindings/mtd/mtd-physmap.yaml | 2 +- > .../bindings/net/mediatek-dwmac.yaml | 2 +- > .../bindings/perf/amlogic,g12-ddr-pmu.yaml | 4 ++-- > .../bindings/phy/mediatek,dsi-phy.yaml | 2 +- > .../remoteproc/amlogic,meson-mx-ao-arc.yaml| 2 +- > .../devicetree/bindings/usb/mediatek,mtu3.yaml | 2 +- > .../devicetree/bindings/usb/ti,am62-usb.yaml | 2 +- > 15 files changed, 30 insertions(+), 30 deletions(-) > Applied, thanks!
[PATCH libdrm 2/3] util: Add pattern support for DRM_FORMAT_NV{24, 42}
Add support for drawing the SMPTE and tiles patterns in buffers using semi-planar YUV formats with non-subsampled chroma planes. Signed-off-by: Geert Uytterhoeven --- tests/util/pattern.c | 4 1 file changed, 4 insertions(+) diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 158c0b160a2ee870..f45a26ccec38f202 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -698,6 +698,8 @@ static void fill_smpte(const struct util_format_info *info, void *planes[3], case DRM_FORMAT_NV21: case DRM_FORMAT_NV16: case DRM_FORMAT_NV61: + case DRM_FORMAT_NV24: + case DRM_FORMAT_NV42: u = info->yuv.order & YUV_YCbCr ? planes[1] : planes[1] + 1; v = info->yuv.order & YUV_YCrCb ? planes[1] : planes[1] + 1; return fill_smpte_yuv_planar(&info->yuv, planes[0], u, v, @@ -1023,6 +1025,8 @@ static void fill_tiles(const struct util_format_info *info, void *planes[3], case DRM_FORMAT_NV21: case DRM_FORMAT_NV16: case DRM_FORMAT_NV61: + case DRM_FORMAT_NV24: + case DRM_FORMAT_NV42: u = info->yuv.order & YUV_YCbCr ? planes[1] : planes[1] + 1; v = info->yuv.order & YUV_YCrCb ? planes[1] : planes[1] + 1; return fill_tiles_yuv_planar(info, planes[0], u, v, -- 2.34.1
[PATCH libdrm 3/3] modetest: Add support for DRM_FORMAT_NV{24,42}
Add support for creating buffers using semi-planar YUV formats with non-subsampled chroma planes. Signed-off-by: Geert Uytterhoeven --- tests/modetest/buffers.c | 20 1 file changed, 20 insertions(+) diff --git a/tests/modetest/buffers.c b/tests/modetest/buffers.c index 0b55aeddfef9a854..0605b12552bb8eec 100644 --- a/tests/modetest/buffers.c +++ b/tests/modetest/buffers.c @@ -129,6 +129,8 @@ bo_create(int fd, unsigned int format, case DRM_FORMAT_NV21: case DRM_FORMAT_NV16: case DRM_FORMAT_NV61: + case DRM_FORMAT_NV24: + case DRM_FORMAT_NV42: case DRM_FORMAT_YUV420: case DRM_FORMAT_YVU420: bpp = 8; @@ -208,6 +210,11 @@ bo_create(int fd, unsigned int format, virtual_height = height * 2; break; + case DRM_FORMAT_NV24: + case DRM_FORMAT_NV42: + virtual_height = height * 3; + break; + default: virtual_height = height; break; @@ -255,6 +262,19 @@ bo_create(int fd, unsigned int format, planes[1] = virtual + offsets[1]; break; + case DRM_FORMAT_NV24: + case DRM_FORMAT_NV42: + offsets[0] = 0; + handles[0] = bo->handle; + pitches[0] = bo->pitch; + pitches[1] = pitches[0] * 2; + offsets[1] = pitches[0] * height; + handles[1] = bo->handle; + + planes[0] = virtual; + planes[1] = virtual + offsets[1]; + break; + case DRM_FORMAT_YUV420: case DRM_FORMAT_YVU420: offsets[0] = 0; -- 2.34.1
[PATCH libdrm 0/3] Add support for DRM_FORMAT_NV{24,42}
Hi all,
This patch series adds support for semi-planar YUV formats with
non-subsampled chroma planes.
It has been tested with the shmob_drm driver on the R-Mobile A1-based
Atmark Techno Armadillo-800-EVA development board.
Thanks for your comments!
Geert Uytterhoeven (3):
util: Add NV24 and NV42 frame buffer formats
util: Add pattern support for DRM_FORMAT_NV{24,42}
modetest: Add support for DRM_FORMAT_NV{24,42}
tests/modetest/buffers.c | 20
tests/util/format.c | 2 ++
tests/util/pattern.c | 4
3 files changed, 26 insertions(+)
--
2.34.1
[PATCH libdrm 1/3] util: Add NV24 and NV42 frame buffer formats
Add the missing entries for semi-planar YUV formats with
non-subsampled chroma planes.
Signed-off-by: Geert Uytterhoeven
---
tests/util/format.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/util/format.c b/tests/util/format.c
index 1ca1b82ce947b2f4..f825027227ddba24 100644
--- a/tests/util/format.c
+++ b/tests/util/format.c
@@ -51,6 +51,8 @@ static const struct util_format_info format_info[] = {
{ DRM_FORMAT_NV21, "NV21", MAKE_YUV_INFO(YUV_YCrCb, 2, 2, 2) },
{ DRM_FORMAT_NV16, "NV16", MAKE_YUV_INFO(YUV_YCbCr, 2, 1, 2) },
{ DRM_FORMAT_NV61, "NV61", MAKE_YUV_INFO(YUV_YCrCb, 2, 1, 2) },
+ { DRM_FORMAT_NV24, "NV24", MAKE_YUV_INFO(YUV_YCbCr, 1, 1, 2) },
+ { DRM_FORMAT_NV42, "NV42", MAKE_YUV_INFO(YUV_YCrCb, 1, 1, 2) },
/* YUV planar */
{ DRM_FORMAT_YUV420, "YU12", MAKE_YUV_INFO(YUV_YCbCr, 2, 2, 1) },
{ DRM_FORMAT_YVU420, "YV12", MAKE_YUV_INFO(YUV_YCrCb, 2, 2, 1) },
--
2.34.1
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On Wed, 5 Jul 2023 at 17:24, Maxime Ripard wrote: > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > > capabilities flags of the DSI host, and we should just stick to the > > > > > spec. If the spec says that we have to support DSC while video is > > > > > output, then that's what the panels should expect. > > > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > > all that is runtime configurable. How do you handle that ? > > > > > > In this case, most of the constraints are going to be on the encoder > > > still so it should be the one driving it. The panel will only care about > > > which mode has been selected, but it shouldn't be the one driving it, > > > and thus we still don't really need to expose the host capabilities. > > > > This is an interesting perspective. This means that we can and actually have > > to extend the drm_display_mode with the DSI data and compression > > information. > > I wouldn't extend drm_display_mode, but extending one of the state > structures definitely. > > We already have some extra variables in drm_connector_state for HDMI, > I don't think it would be a big deal to add a few for MIPI-DSI. > > We also floated the idea for a while to create bus-specific states, with > helpers to match. Maybe it would be a good occasion to start doing it? > > > For example, the panel that supports all four types for the 1080p should > > export several modes: > > > > 1920x1080-command > > 1920x1080-command-DSC > > 1920x1080-video > > 1920x1080-video-DSC > > > > where video/command and DSC are some kinds of flags and/or information in > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > > etc). > > So we have two things to do, right? We need to expose what the panel can > take (ie, EDID for HDMI), and then we need to tell it what we picked > (infoframes). > > We already express the former in mipi_dsi_device, so we could extend the > flags stored there. > > And then, we need to tie what the DSI host chose to a given atomic state > so the panel knows what was picked and how it should set everything up. This is definitely something we need. Marijn has been stuck with the panels that support different models ([1]). Would you prefer a separate API for this kind of information or abusing atomic_enable() is fine from your point of view? My vote would be for going with existing operations, with the slight fear of ending up with another DSI-specific hack (like pre_enable_prev_first). > > > Another option would be to get this handled via the bus format negotiation, > > but that sounds like worse idea to me. > > Yeah, I'm not really fond of the format negociation stuff either. [1] https://lore.kernel.org/linux-arm-msm/20230521-drm-panels-sony-v1-8-541c341d6...@somainline.org/ -- With best wishes Dmitry
[PATCH libdrm] amdgpu: Use %ll to format 64-bit integers
On 32-bit:
../tests/amdgpu/amdgpu_stress.c: In function ‘alloc_bo’:
../tests/amdgpu/amdgpu_stress.c:178:49: warning: format ‘%lx’ expects
argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t’ {aka
‘long long unsigned int’} [-Wformat=]
fprintf(stdout, "Allocated BO number %u at 0x%lx, domain 0x%x, size
%lu\n",
~~^
%llx
num_buffers++, addr, domain, size);
../tests/amdgpu/amdgpu_stress.c:178:72: warning: format ‘%lu’ expects
argument of type ‘long unsigned int’, but argument 6 has type ‘uint64_t’ {aka
‘long long unsigned int’} [-Wformat=]
fprintf(stdout, "Allocated BO number %u at 0x%lx, domain 0x%x, size
%lu\n",
~~^
%llu
num_buffers++, addr, domain, size);
../tests/amdgpu/amdgpu_stress.c: In function ‘submit_ib’:
../tests/amdgpu/amdgpu_stress.c:276:54: warning: format ‘%lx’ expects
argument of type ‘long unsigned int’, but argument 5 has type ‘uint64_t’ {aka
‘long long unsigned int’} [-Wformat=]
fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
bytes took %lu usec\n",
~~^
%llx
count, from, virtual[from], to, virtual[to], copied, delta / 1000);
~
../tests/amdgpu/amdgpu_stress.c:276:65: warning: format ‘%lx’ expects
argument of type ‘long unsigned int’, but argument 7 has type ‘uint64_t’ {aka
‘long long unsigned int’} [-Wformat=]
fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
bytes took %lu usec\n",
~~^
%llx
count, from, virtual[from], to, virtual[to], copied, delta / 1000);
~~~
../tests/amdgpu/amdgpu_stress.c:276:70: warning: format ‘%lu’ expects
argument of type ‘long unsigned int’, but argument 8 has type ‘uint64_t’ {aka
‘long long unsigned int’} [-Wformat=]
fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
bytes took %lu usec\n",
~~^
%llu
count, from, virtual[from], to, virtual[to], copied, delta / 1000);
~~
../tests/amdgpu/amdgpu_stress.c:276:85: warning: format ‘%lu’ expects
argument of type ‘long unsigned int’, but argument 9 has type ‘uint64_t’ {aka
‘long long unsigned int’} [-Wformat=]
fprintf(stdout, "Submitted %u IBs to copy from %u(%lx) to %u(%lx) %lu
bytes took %lu usec\n",
~~^
%llu
count, from, virtual[from], to, virtual[to], copied, delta / 1000);
../tests/amdgpu/amdgpu_stress.c: In function ‘parse_size’:
../tests/amdgpu/amdgpu_stress.c:296:24: warning: format ‘%li’ expects
argument of type ‘long int *’, but argument 3 has type ‘uint64_t *’ {aka ‘long
long unsigned int *’} [-Wformat=]
if (sscanf(optarg, "%li%1[kmgKMG]", &size, ext) < 1) {
~~^ ~
%lli
../tests/amdgpu/amdgpu_stress.c: In function ‘main’:
../tests/amdgpu/amdgpu_stress.c:378:45: warning: format ‘%lu’ expects
argument of type ‘long unsigned int’, but argument 3 has type ‘uint64_t’ {aka
‘long long unsigned int’} [-Wformat=]
fprintf(stderr, "Buffer size to small %lu\n", size);
~~^
%llu
Fix this by using the proper "%ll" format specifier prefix.
Fixes: d77ccdf3ba6f5a39 ("amdgpu: add amdgpu_stress utility v2")
Signed-off-by: Geert Uytterhoeven
---
tests/amdgpu/amdgpu_stress.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/amdgpu/amdgpu_stress.c b/tests/amdgpu/amdgpu_stress.c
index 5c5c88c5be985eb6..7182f9005703f1a4 100644
--- a/tests/amdgpu/amdgpu_stress.c
+++ b/tests/amdgpu/amdgpu_stress.c
@@ -175,7 +175,7 @@ int alloc_bo(uint32_t domain, uint64_t size)
resources[num_buffers] = bo;
virtual[num_buffers] = addr;
- fprintf(stdout, "Allocated BO number %u at 0x%lx, domain 0x%x, size
%lu\n",
+ fprintf(stdout, "Allocated BO number %u at 0x%llx, domain 0x%x, size
%llu\n",
num_buf
[PATCH libdrm] amdgpu: Fix pointer/integer mismatch warning
On 32-bit:
../amdgpu/amdgpu_bo.c: In function ‘amdgpu_find_bo_by_cpu_mapping’:
../amdgpu/amdgpu_bo.c:554:13: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
cpu < (void*)((uintptr_t)bo->cpu_ptr + bo->alloc_size))
^
Indeed, as amdgpu_bo_info.alloc_size is "uint64_t", the sum is
always 64-bit, while "void *" can be 32-bit or 64-bit.
Fix this by casting bo->alloc_size to "size_t", which is either
32-bit or 64-bit, just like "void *".
Fixes: c6493f360e7529c2 ("amdgpu: Eliminate void* arithmetic in
amdgpu_find_bo_by_cpu_mapping")
Signed-off-by: Geert Uytterhoeven
---
amdgpu/amdgpu_bo.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/amdgpu/amdgpu_bo.c b/amdgpu/amdgpu_bo.c
index f4e0435254f6aa9f..672f000d64801012 100644
--- a/amdgpu/amdgpu_bo.c
+++ b/amdgpu/amdgpu_bo.c
@@ -551,7 +551,7 @@ drm_public int
amdgpu_find_bo_by_cpu_mapping(amdgpu_device_handle dev,
if (!bo || !bo->cpu_ptr || size > bo->alloc_size)
continue;
if (cpu >= bo->cpu_ptr &&
- cpu < (void*)((uintptr_t)bo->cpu_ptr + bo->alloc_size))
+ cpu < (void*)((uintptr_t)bo->cpu_ptr +
(size_t)bo->alloc_size))
break;
}
--
2.34.1
Re: [PATCH] backlight: led_bl: fix initial power state
On Wed, Jul 05, 2023 at 03:36:46PM +0100, Måns Rullgård wrote:
> Daniel Thompson writes:
>
> > On Wed, Jul 05, 2023 at 03:24:14PM +0100, Mans Rullgard wrote:
> >> The condition for the initial power state based on the default
> >> brightness value is reversed. Fix it.
> >>
> >> Furthermore, use the actual state of the LEDs rather than the default
> >> brightness specified in the devicetree as the latter should not cause
> >> the backlight to be automatically turned on.
> >>
> >> If the backlight device is not linked to any display, set the initial
> >> power to on unconditionally.
> >>
> >> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> >> Signed-off-by: Mans Rullgard
> >> ---
> >> Changes in v3:
> >> - Add comment
> >
> > This mismatches the subject line ;-) but I can live with that if Lee
> > and Jingoo can!
>
> Does it not fix it? If you think the subject is misleading, feel free
> to change it.
The bit that goes into version control is fine!
However without '[PATCH v3]' on the subject line for the initial patch
there is a risk this thread will get overlooked and not queued[1].
Daniel.
[1] Just to be clear, Lee J. typically hoovers up the backlight patches
and sends the PR. I only queue backlight patches myself as holiday
cover...
Re: [PATCH 1/2] drm/msm/adreno: Fix warn splat for devices without revn
On Tue, Jul 4, 2023 at 10:20 AM Dmitry Baryshkov
wrote:
>
> On Tue, 4 Jul 2023 at 19:36, Rob Clark wrote:
> >
> > From: Rob Clark
> >
> > Recently, a WARN_ON() was introduced to ensure that revn is filled before
> > adreno_is_aXYZ is called. This however doesn't work very well when revn is
> > 0 by design (such as for A635).
> >
> > Cc: Konrad Dybcio
> > Fixes: cc943f43ece7 ("drm/msm/adreno: warn if chip revn is verified before
> > being set")
> > Signed-off-by: Rob Clark
> > ---
> > drivers/gpu/drm/msm/adreno/adreno_gpu.h | 9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > index 65379e4824d9..ef1bcb6b624e 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> > @@ -149,7 +149,8 @@ bool adreno_cmp_rev(struct adreno_rev rev1, struct
> > adreno_rev rev2);
> >
> > static inline bool adreno_is_revn(const struct adreno_gpu *gpu, uint32_t
> > revn)
> > {
> > - WARN_ON_ONCE(!gpu->revn);
> > + /* revn can be zero, but if not is set at same time as info */
> > + WARN_ON_ONCE(!gpu->info);
> >
> > return gpu->revn == revn;
> > }
> > @@ -161,14 +162,16 @@ static inline bool adreno_has_gmu_wrapper(const
> > struct adreno_gpu *gpu)
> >
> > static inline bool adreno_is_a2xx(const struct adreno_gpu *gpu)
> > {
> > - WARN_ON_ONCE(!gpu->revn);
> > + /* revn can be zero, but if not is set at same time as info */
> > + WARN_ON_ONCE(!gpu->info);
> >
> > return (gpu->revn < 300);
>
> This is then incorrect for a635 / a690 if they have revn at 0.
Fortunately not any more broken that it has ever been. But as long as
sc7280/sc8280 have GPU OPP tables, you'd never hit this. I'm working
on a better solution for next merge window.
BR,
-R
> > }
> >
> > static inline bool adreno_is_a20x(const struct adreno_gpu *gpu)
> > {
> > - WARN_ON_ONCE(!gpu->revn);
> > + /* revn can be zero, but if not is set at same time as info */
> > + WARN_ON_ONCE(!gpu->info);
> >
> > return (gpu->revn < 210);
>
> And this too.
>
> > }
> > --
> > 2.41.0
> >
>
>
> --
> With best wishes
> Dmitry
Re: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
Hi Neil,
Le mercredi 05 juillet 2023 à 15:57 +0200, Neil Armstrong a écrit :
> On 03/07/2023 23:47, Paul Cercueil wrote:
> > Register a backlight device to be able to switch between all the
> > gamma
> > levels.
> >
> > Signed-off-by: Paul Cercueil
> > ---
> > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40
> >
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > index 7fd9444b42c5..b4f87d6244cb 100644
> > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > @@ -8,6 +8,7 @@
> > * Andrzej Hajda
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx)
> > return 0;
> > }
> >
> > +static int ld9040_bl_update_status(struct backlight_device *dev)
> > +{
> > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev);
> > +
> > + ctx->brightness = dev->props.brightness;
> > + ld9040_brightness_set(ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static int ld9040_bl_get_intensity(struct backlight_device *dev)
> > +{
> > + if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
> > + dev->props.power == FB_BLANK_UNBLANK)
> > + return dev->props.brightness;
> > +
> > + return 0;
> > +}
>
> You can totally drop the _get_brightness.
The current behaviour is to return 0 when the framebuffer is blanked. A
few drivers do that so I thought it was the norm; and the backlight
core doesn't do that by default (and just uses dev->props.brightness).
It is not clear to me if that's the preferred behaviour. The
"backlight_get_brightness" function in backlight.h seems to suggest
that the current behaviour is correct, unless it is not supposed to be
used in the backlight_ops.get_brightness() callback. Then in that case
some other drivers get it wrong too.
Cheers,
-Paul
> Neil
>
> > +
> > +static const struct backlight_ops ld9040_bl_ops = {
> > + .get_brightness = ld9040_bl_get_intensity,
> > + .update_status = ld9040_bl_update_status,
> > +};
> > +
> > +static const struct backlight_properties ld9040_bl_props = {
> > + .type = BACKLIGHT_RAW,
> > + .scale = BACKLIGHT_SCALE_NON_LINEAR,
> > + .max_brightness = ARRAY_SIZE(ld9040_gammas) - 1,
> > + .brightness = ARRAY_SIZE(ld9040_gammas) - 1,
> > +};
> > +
> > static int ld9040_probe(struct spi_device *spi)
> > {
> > + struct backlight_device *bldev;
> > struct device *dev = &spi->dev;
> > struct ld9040 *ctx;
> > int ret;
> > @@ -354,6 +387,13 @@ static int ld9040_probe(struct spi_device
> > *spi)
> > drm_panel_init(&ctx->panel, dev, &ld9040_drm_funcs,
> > DRM_MODE_CONNECTOR_DPI);
> >
> > +
> > + bldev = devm_backlight_device_register(dev, dev_name(dev),
> > dev,
> > + ctx, &ld9040_bl_ops,
> > + &ld9040_bl_props);
> > + if (IS_ERR(bldev))
> > + return PTR_ERR(bldev);
> > +
> > drm_panel_add(&ctx->panel);
> >
> > return 0;
>
Re: [PATCH] backlight: led_bl: fix initial power state
On Wed, Jul 05, 2023 at 03:24:14PM +0100, Mans Rullgard wrote:
> The condition for the initial power state based on the default
> brightness value is reversed. Fix it.
>
> Furthermore, use the actual state of the LEDs rather than the default
> brightness specified in the devicetree as the latter should not cause
> the backlight to be automatically turned on.
>
> If the backlight device is not linked to any display, set the initial
> power to on unconditionally.
>
> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> Signed-off-by: Mans Rullgard
> ---
> Changes in v3:
> - Add comment
This mismatches the subject line ;-) but I can live with that if Lee
and Jingoo can!
Reviewed-by: Daniel Thompson
Daniel.
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote: > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the > > > > capabilities flags of the DSI host, and we should just stick to the > > > > spec. If the spec says that we have to support DSC while video is > > > > output, then that's what the panels should expect. > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and > > > all that is runtime configurable. How do you handle that ? > > > > In this case, most of the constraints are going to be on the encoder > > still so it should be the one driving it. The panel will only care about > > which mode has been selected, but it shouldn't be the one driving it, > > and thus we still don't really need to expose the host capabilities. > > This is an interesting perspective. This means that we can and actually have > to extend the drm_display_mode with the DSI data and compression > information. I wouldn't extend drm_display_mode, but extending one of the state structures definitely. We already have some extra variables in drm_connector_state for HDMI, I don't think it would be a big deal to add a few for MIPI-DSI. We also floated the idea for a while to create bus-specific states, with helpers to match. Maybe it would be a good occasion to start doing it? > For example, the panel that supports all four types for the 1080p should > export several modes: > > 1920x1080-command > 1920x1080-command-DSC > 1920x1080-video > 1920x1080-video-DSC > > where video/command and DSC are some kinds of flags and/or information in > the drm_display_mode? Ideally DSC also has several sub-flags, which denote > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv, > etc). So we have two things to do, right? We need to expose what the panel can take (ie, EDID for HDMI), and then we need to tell it what we picked (infoframes). We already express the former in mipi_dsi_device, so we could extend the flags stored there. And then, we need to tie what the DSI host chose to a given atomic state so the panel knows what was picked and how it should set everything up. > Another option would be to get this handled via the bus format negotiation, > but that sounds like worse idea to me. Yeah, I'm not really fond of the format negociation stuff either. Maxime signature.asc Description: PGP signature
Re: [PATCH v2] backlight: led_bl: fix initial power state
On Tue, Jul 04, 2023 at 05:19:52PM +0100, Mans Rullgard wrote:
> The condition for the initial power state based on the default
> brightness value is reversed. Fix it.
>
> Furthermore, use the actual state of the LEDs rather than the default
> brightness specified in the devicetree as the latter should not cause
> the backlight to be automatically turned on.
>
> If the backlight device is not linked to any display, set the initial
> power to on unconditionally.
>
> Fixes: ae232e45acf9 ("backlight: add led-backlight driver")
> Signed-off-by: Mans Rullgard
> ---
> Changes in v2:
> - Use the reported LED state to set initial power state
> - Always power on if no phandle in DT
> ---
> drivers/video/backlight/led_bl.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/led_bl.c
> b/drivers/video/backlight/led_bl.c
> index 3259292fda76..bbf1673b1fb0 100644
> --- a/drivers/video/backlight/led_bl.c
> +++ b/drivers/video/backlight/led_bl.c
> @@ -176,6 +176,7 @@ static int led_bl_probe(struct platform_device *pdev)
> {
> struct backlight_properties props;
> struct led_bl_data *priv;
> + int init_brightness;
> int ret, i;
>
> priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> @@ -190,6 +191,8 @@ static int led_bl_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + init_brightness = priv->default_brightness;
> +
> ret = led_bl_parse_levels(&pdev->dev, priv);
> if (ret < 0) {
> dev_err(&pdev->dev, "Failed to parse DT data\n");
> @@ -200,8 +203,8 @@ static int led_bl_probe(struct platform_device *pdev)
> props.type = BACKLIGHT_RAW;
> props.max_brightness = priv->max_brightness;
> props.brightness = priv->default_brightness;
> - props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> - FB_BLANK_UNBLANK;
> + props.power = (init_brightness || !pdev->dev.of_node->phandle) ?
> + FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN;
I was rather expecting to see a comment in the code here... it's super
hard to figure out the purpose of the phandle check otherwise.
Daniel.
Re: [PATCH] backlight: led_bl: fix initial power state
On Tue, Jul 04, 2023 at 07:07:31PM +0200, Sam Ravnborg wrote: > Hi Daniel, > > > > @@ -200,8 +200,8 @@ static int led_bl_probe(struct platform_device *pdev) > > > props.type = BACKLIGHT_RAW; > > > props.max_brightness = priv->max_brightness; > > > props.brightness = priv->default_brightness; > > > - props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN : > > > - FB_BLANK_UNBLANK; > > > + props.power = (priv->default_brightness > 0) ? FB_BLANK_UNBLANK : > > > + FB_BLANK_POWERDOWN; > > > > The logic was wrong before but I think will still be wrong after the > > change too (e.g. the bogus logic is probably avoiding backlight flicker > > in some use cases). > > > > The logic here needs to be similar to what pwm_bl.c implements in > > pwm_backlight_initial_power_state(). Whilst it might be better > > to implement this in led_bl_get_leds() let me show what I mean > > in code that fits in the current line: > > > > /* > > * Activate the backlight if the LEDs are already lit *or* > > * there is no phandle link (meaning the backlight power > > * state cannot be synced with the display state). > > */ > > props.power = (active_at_boot || !dev->node->phandle) ? > > FB_BLANK_UNBLANK : FB_BLANK_POWERDOWN; > > > The following code does the same using helpers: > > if (active_at_boot || !dev->node->phandle)) > backlight_enable(bd); > else > backlight_disable(bd); > > The code needs to execute after backlight_device_register() so maybe not > so great an idea?!? It would introduce a need for a bunch of new locks since userspace could get in between device creation and the call to the helpers. Additionally, it is even correct for a driver to forcefully set fb_blank to powerdown during the probe? All current drivers set fb_blank to unblank during the probe. Daniel.
Re: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
On 03/07/2023 23:47, Paul Cercueil wrote:
Register a backlight device to be able to switch between all the gamma
levels.
Signed-off-by: Paul Cercueil
---
drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40
1 file changed, 40 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
index 7fd9444b42c5..b4f87d6244cb 100644
--- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
+++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
@@ -8,6 +8,7 @@
* Andrzej Hajda
*/
+#include
#include
#include
#include
@@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx)
return 0;
}
+static int ld9040_bl_update_status(struct backlight_device *dev)
+{
+ struct ld9040 *ctx = dev_get_drvdata(&dev->dev);
+
+ ctx->brightness = dev->props.brightness;
+ ld9040_brightness_set(ctx);
+
+ return 0;
+}
+
+static int ld9040_bl_get_intensity(struct backlight_device *dev)
+{
+ if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
+ dev->props.power == FB_BLANK_UNBLANK)
+ return dev->props.brightness;
+
+ return 0;
+}
You can totally drop the _get_brightness.
Neil
+
+static const struct backlight_ops ld9040_bl_ops = {
+ .get_brightness = ld9040_bl_get_intensity,
+ .update_status = ld9040_bl_update_status,
+};
+
+static const struct backlight_properties ld9040_bl_props = {
+ .type = BACKLIGHT_RAW,
+ .scale = BACKLIGHT_SCALE_NON_LINEAR,
+ .max_brightness = ARRAY_SIZE(ld9040_gammas) - 1,
+ .brightness = ARRAY_SIZE(ld9040_gammas) - 1,
+};
+
static int ld9040_probe(struct spi_device *spi)
{
+ struct backlight_device *bldev;
struct device *dev = &spi->dev;
struct ld9040 *ctx;
int ret;
@@ -354,6 +387,13 @@ static int ld9040_probe(struct spi_device *spi)
drm_panel_init(&ctx->panel, dev, &ld9040_drm_funcs,
DRM_MODE_CONNECTOR_DPI);
+
+ bldev = devm_backlight_device_register(dev, dev_name(dev), dev,
+ ctx, &ld9040_bl_ops,
+ &ld9040_bl_props);
+ if (IS_ERR(bldev))
+ return PTR_ERR(bldev);
+
drm_panel_add(&ctx->panel);
return 0;
Re: [PATCH 1/3] drm/panel: ld9040: Use better magic values
Hi
On 03/07/2023 23:47, Paul Cercueil wrote:
I have no idea what the prior magic values mean, and I have no idea
what my replacement (extracted from [1]) magic values mean.
What I do know, is that these new values result in a much better
picture, where the blacks are really black (as you would expect on an
AMOLED display) instead of grey-ish.
[1]
https://github.com/dorimanx/Dorimanx-SG2-I9100-Kernel/blob/master-jelly-bean/arch/arm/mach-exynos/u1-panel.h
Signed-off-by: Paul Cercueil
---
drivers/gpu/drm/panel/panel-samsung-ld9040.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
index 01eb211f32f7..7fd9444b42c5 100644
--- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
+++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
@@ -180,17 +180,18 @@ static void ld9040_init(struct ld9040 *ctx)
{
ld9040_dcs_write_seq_static(ctx, MCS_USER_SETTING, 0x5a, 0x5a);
ld9040_dcs_write_seq_static(ctx, MCS_PANEL_CONDITION,
- 0x05, 0x65, 0x96, 0x71, 0x7d, 0x19, 0x3b, 0x0d,
- 0x19, 0x7e, 0x0d, 0xe2, 0x00, 0x00, 0x7e, 0x7d,
- 0x07, 0x07, 0x20, 0x20, 0x20, 0x02, 0x02);
+ 0x05, 0x5e, 0x96, 0x6b, 0x7d, 0x0d, 0x3f, 0x00,
+ 0x00, 0x32, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+ 0x07, 0x05, 0x1f, 0x1f, 0x1f, 0x00, 0x00);
ld9040_dcs_write_seq_static(ctx, MCS_DISPCTL,
- 0x02, 0x08, 0x08, 0x10, 0x10);
+ 0x02, 0x06, 0x0a, 0x10, 0x10);
ld9040_dcs_write_seq_static(ctx, MCS_MANPWR, 0x04);
ld9040_dcs_write_seq_static(ctx, MCS_POWER_CTRL,
0x0a, 0x87, 0x25, 0x6a, 0x44, 0x02, 0x88);
- ld9040_dcs_write_seq_static(ctx, MCS_ELVSS_ON, 0x0d, 0x00, 0x16);
+ ld9040_dcs_write_seq_static(ctx, MCS_ELVSS_ON, 0x0f, 0x00, 0x16);
ld9040_dcs_write_seq_static(ctx, MCS_GTCON, 0x09, 0x00, 0x00);
ld9040_brightness_set(ctx);
+
You can drop this spurious new line for v2
ld9040_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
ld9040_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
}
And add
Reviewed-by: Neil Armstrong
Neil
[Bug 217621] AMDGPU - Internal 4K display used with 1920x1080 leads to screen flickering and distortion, regression from commit edcfed8671ee57bb599184f2e12a1b3e11b32306
https://bugzilla.kernel.org/show_bug.cgi?id=217621 Alex Deucher (alexdeuc...@gmail.com) changed: What|Removed |Added CC||alexdeuc...@gmail.com --- Comment #5 from Alex Deucher (alexdeuc...@gmail.com) --- The revert of that patch is already on it's way to Linus. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On 05/07/2023 16:29, Maxime Ripard wrote:
On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote:
On 05/07/2023 14:04, Maxime Ripard wrote:
Hi,
On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
On Tue, 30 May 2023 at 10:24, Neil Armstrong
wrote:
Hi Marijn, Dmitry, Caleb, Jessica,
On 29/05/2023 23:11, Marijn Suijten wrote:
On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
+ if (ctx->dsi->dsc) {
dsi->dsc is always set, thus this condition can be dropped.
I want to leave room for possibly running the panel without DSC (at a
lower resolution/refresh rate, or at higher power consumption if there
is enough BW) by not assigning the pointer, if we get access to panel
documentation: probably one of the magic commands sent in this driver
controls it but we don't know which.
I'd like to investigate if DSC should perhaps only be enabled if we
run non certain platforms/socs ?
I mean, we don't know if the controller supports DSC and those
particular
DSC parameters so we should probably start adding something like :
static drm_dsc_config dsc_params_qcom = {}
static const struct of_device_id panel_of_dsc_params[] = {
{ .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
{ .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
{ .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
{ .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
};
I think this would damage the reusability of the drivers. The panel
driver does not actually care if the SoC is SM8350, sunxi-something or
RCar.
Instead it cares about host capabilities.
I think instead we should extend mipi_dsi_host:
#define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
#define MIPI_DSI_HOST_MODE_CMD BIT(1)
#define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
// FIXME: do we need to provide additional caps here ?
#define MIPI_DSI_DSC_1_1 BIT(0)
#define MIPI_DSI_DSC_1_2 BIT(1)
#define MIPI_DSI_DSC_NATIVE_422 BIT(2)
#define MIPI_DSI_DSC_NATIVE_420 BIT(3)
#define MIPI_DSI_DSC_FRAC_BPP BIT(4)
// etc.
struct mipi_dsi_host {
// new fields only
unsigned long mode_flags;
unsigned long dsc_flags;
};
Then the panel driver can adapt itself to the host capabilities and
(possibly) select one of the internally supported DSC profiles.
I completely agree about extending mipi_dsi_host, other SoCs could reuse
that and
support for DSC panels would become a lot cleaner.
Sounds good. I will wait for one or two more days (to get the possible
feedback on fields/flags/etc) and post an RFC patch to dri-devel.
I just came across that discussion, and couldn't find those patches, did
you ever send them?
No, I got sidetracked by other issues.
Either way, I'm not really sure it's a good idea to multiply the
capabilities flags of the DSI host, and we should just stick to the
spec. If the spec says that we have to support DSC while video is
output, then that's what the panels should expect.
Except some panels supports DSC & non-DSC, Video and Command mode, and
all that is runtime configurable. How do you handle that ?
In this case, most of the constraints are going to be on the encoder
still so it should be the one driving it. The panel will only care about
which mode has been selected, but it shouldn't be the one driving it,
and thus we still don't really need to expose the host capabilities.
This is an interesting perspective. This means that we can and actually
have to extend the drm_display_mode with the DSI data and compression
information.
For example, the panel that supports all four types for the 1080p should
export several modes:
1920x1080-command
1920x1080-command-DSC
1920x1080-video
1920x1080-video-DSC
where video/command and DSC are some kinds of flags and/or information
in the drm_display_mode? Ideally DSC also has several sub-flags, which
denote what kind of configuration is supported by the DSC sink (e.g.
bpp, yuv, etc).
Another option would be to get this handled via the bus format
negotiation, but that sounds like worse idea to me.
This is very much like HDMI: the encoder knows what the monitor is
capable of, will take a decision based on its capabilities and the
monitor's and will then let the monitor know. But the monitor never
knows what the encoder is truly capable of, nor will it enforce
something.
Maxime
--
With best wishes
Dmitry
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote:
> On 05/07/2023 14:04, Maxime Ripard wrote:
> > Hi,
> >
> > On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
> > > On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
> > > > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
> > > > > On Tue, 30 May 2023 at 10:24, Neil Armstrong
> > > > > wrote:
> > > > > >
> > > > > > Hi Marijn, Dmitry, Caleb, Jessica,
> > > > > >
> > > > > > On 29/05/2023 23:11, Marijn Suijten wrote:
> > > > > > > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> > > > > > >
> > > > > > > > > + if (ctx->dsi->dsc) {
> > > > > > > >
> > > > > > > > dsi->dsc is always set, thus this condition can be dropped.
> > > > > > >
> > > > > > > I want to leave room for possibly running the panel without DSC
> > > > > > > (at a
> > > > > > > lower resolution/refresh rate, or at higher power consumption if
> > > > > > > there
> > > > > > > is enough BW) by not assigning the pointer, if we get access to
> > > > > > > panel
> > > > > > > documentation: probably one of the magic commands sent in this
> > > > > > > driver
> > > > > > > controls it but we don't know which.
> > > > > >
> > > > > > I'd like to investigate if DSC should perhaps only be enabled if we
> > > > > > run non certain platforms/socs ?
> > > > > >
> > > > > > I mean, we don't know if the controller supports DSC and those
> > > > > > particular
> > > > > > DSC parameters so we should probably start adding something like :
> > > > > >
> > > > > > static drm_dsc_config dsc_params_qcom = {}
> > > > > >
> > > > > > static const struct of_device_id panel_of_dsc_params[] = {
> > > > > > { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom
> > > > > > },
> > > > > > { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom
> > > > > > },
> > > > > > { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom
> > > > > > },
> > > > > > { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom
> > > > > > },
> > > > > > };
> > > > >
> > > > > I think this would damage the reusability of the drivers. The panel
> > > > > driver does not actually care if the SoC is SM8350, sunxi-something or
> > > > > RCar.
> > > > > Instead it cares about host capabilities.
> > > > >
> > > > > I think instead we should extend mipi_dsi_host:
> > > > >
> > > > > #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
> > > > > #define MIPI_DSI_HOST_MODE_CMD BIT(1)
> > > > > #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
> > > > > // FIXME: do we need to provide additional caps here ?
> > > > >
> > > > > #define MIPI_DSI_DSC_1_1 BIT(0)
> > > > > #define MIPI_DSI_DSC_1_2 BIT(1)
> > > > > #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
> > > > > #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
> > > > > #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
> > > > > // etc.
> > > > >
> > > > > struct mipi_dsi_host {
> > > > > // new fields only
> > > > > unsigned long mode_flags;
> > > > > unsigned long dsc_flags;
> > > > > };
> > > > >
> > > > > Then the panel driver can adapt itself to the host capabilities and
> > > > > (possibly) select one of the internally supported DSC profiles.
> > > > >
> > > >
> > > > I completely agree about extending mipi_dsi_host, other SoCs could reuse
> > > > that and
> > > > support for DSC panels would become a lot cleaner.
> > >
> > > Sounds good. I will wait for one or two more days (to get the possible
> > > feedback on fields/flags/etc) and post an RFC patch to dri-devel.
> >
> > I just came across that discussion, and couldn't find those patches, did
> > you ever send them?
> >
> > Either way, I'm not really sure it's a good idea to multiply the
> > capabilities flags of the DSI host, and we should just stick to the
> > spec. If the spec says that we have to support DSC while video is
> > output, then that's what the panels should expect.
>
> Except some panels supports DSC & non-DSC, Video and Command mode, and
> all that is runtime configurable. How do you handle that ?
In this case, most of the constraints are going to be on the encoder
still so it should be the one driving it. The panel will only care about
which mode has been selected, but it shouldn't be the one driving it,
and thus we still don't really need to expose the host capabilities.
This is very much like HDMI: the encoder knows what the monitor is
capable of, will take a decision based on its capabilities and the
monitor's and will then let the monitor know. But the monitor never
knows what the encoder is truly capable of, nor will it enforce
something.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH 1/1] drm: panel-simple: add missing bus flags for Tianma tm070jvhg[30/33]
Hi,
another gentle ping
Best regards,
Alexander
Am Mittwoch, 25. Januar 2023, 15:52:15 CEST schrieb Alexander Stein:
> From: Markus Niebel
>
> The DE signal is active high on this display, fill in the missing
> bus_flags. This aligns panel_desc with its display_timing.
>
> Fixes: 9a2654c0f62a ("drm/panel: Add and fill drm_panel type field")
> Fixes: b3bfcdf8a3b6 ("drm/panel: simple: add Tianma TM070JVHG33")
> Signed-off-by: Markus Niebel
> Signed-off-by: Alexander Stein
> ---
> drivers/gpu/drm/panel/panel-simple.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c index 065f378bba9d..fbccaf1cb6f2
> 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -3598,6 +3598,7 @@ static const struct panel_desc tianma_tm070jdhg30 = {
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> .connector_type = DRM_MODE_CONNECTOR_LVDS,
> + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> };
>
> static const struct panel_desc tianma_tm070jvhg33 = {
> @@ -3610,6 +3611,7 @@ static const struct panel_desc tianma_tm070jvhg33 = {
> },
> .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG,
> .connector_type = DRM_MODE_CONNECTOR_LVDS,
> + .bus_flags = DRM_BUS_FLAG_DE_HIGH,
> };
>
> static const struct display_timing tianma_tm070rvhg71_timing = {
--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/
Re: Re: [PATCH v3 07/17] drm/imagination: Add GPU ID parsing and firmware loading
On Mon, 2023-06-26 at 10:38 -0500, Adam Ford wrote: > On Mon, Jun 26, 2023 at 8:22 AM Frank Binns wrote: > > Hi Adam, > > > > On Sat, 2023-06-17 at 07:48 -0500, Adam Ford wrote: > > > On Tue, Jun 13, 2023 at 10:20 AM Sarah Walker > > > wrote: > > > > Read the GPU ID register at probe time and select the correct > > > > features/quirks/enhancements. Use the GPU ID to form the firmware > > > > file name and load the firmware. > > > > > > I have a Rogue 6250 variant, but the BVNC is returning a slightly > > > different revision than the firmware that's currently available, and > > > the older firmware for the vendor driver doesn't work with this new > > > code. > > > > > > Linux responds with Unsupported BVNC: 4.45.2.58. From what I can > > > tell, the closest available firmware is 4.40.2.51. > > > > > > Will there be more firmware variants in the future or will there be > > > some options to build the firmware somehow? > > > > We don't plan to support the SoC vendor provided firmware binaries as this > > will > > mean having to deal with many different versions of the firmware and its > > interface. Specifically, the interface can change in backwards incompatible > > ways > > between releases, it changes based on the hardware feature set & bugs and > > it's > > split across userspace & the kernel. This makes these SoC provided firmware > > binaries very difficult to support in this new driver. > > Thanks for the response. > > That makes sense. I would hope that various SoC vendors would jump on > the bandwagon to work with your group to get their hardware supported. > > As an alternative, we'll be releasing firmware binaries as we add support > > for > > more Rogue GPUs. We'll also release binaries upon request, in case others > > in the > > community want to work on support in the meantime - we're just getting > > things > > set up for this at the moment. > > The Mesa side of things appears to be missing some documentation, and > the power VR stuff still appears listed as experimental. Is there > some documentation somewhere that would explain to someone how to go > about porting the Rogue 6250 to a different hardware variant of the > 6250? I don't really know the difference between BVNC of 4.45.2.58 > and 4.40.2.51, but I can't imagine they are drastically different. One thing I forgot to mention is that, alongside the firmware binaries, we'll also provide the corresponding device info, e.g. for Mesa: https://gitlab.freedesktop.org/mesa/mesa/-/blob/e714b35301a33145399f8939ca864ffd14b49de9/src/imagination/common/pvr_device_info.c#L32-125 We don't have any specific porting documentation, but we did just send out a Mesa MR adding some initial (basic) documentation: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23992 In terms of differences between the two GX6250 variants, it doesn't seem that there's anything feature-wise that will require any driver changes that are specific to the 4.45.2.58 variant (the different firmware should in theory be sufficient). There are still some driver changes required, however. Assuming the SoC you're interested in is already well supported upstream and the clocks, power controllers, etc needed by the GPU are also already supported then the following changes will be required at a minimum: 1. A GPU node will need adding to the device tree source file for your specific board 2. The compatible string for the GPU node will need adding to the list of supported devices in the kernel driver (grep for "dt_match" in the driver code) 3. The device info we provide alongside the firmware binary will need adding to the kernel driver and Mesa 4. The compatible string for the GPU and display controller device tree nodes will need adding to the Vulkan driver (grep for "pvr_drm_configs" in the Mesa code to see existing examples) Hopefully that covers everything, but no doubt I missed something! With respect to the experimental status of the driver, I think there are three things that need to happen before we can drop this tag. Firstly, the kernel driver needs to be merged to the kernel. Secondly, we need to pass Khronos conformance on at least one of the devices we support (our current focus is on the AXE-1-16M). Finally, we need to upstream all our Mesa changes. This is something that we've been chipping away at, but we do have a big backlog in our public branch [1]. I expect it's going to be quite some time until all of this work is complete. While so much code is sitting in downstream branches I think it's going to be somewhat painful for people to meaningfully contribute to the driver itself. Effort is probably best spent on getting the other drivers, which the GPU driver depends on, upstream for the platform(s) you're interested in. Just to say that I'm by no means trying to put you off from contributing, but simply trying to warn you that until the driver is out of its experimental state, a lot of things are going to be in fl
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
On 05/07/2023 14:04, Maxime Ripard wrote:
Hi,
On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
On Tue, 30 May 2023 at 10:24, Neil Armstrong
wrote:
Hi Marijn, Dmitry, Caleb, Jessica,
On 29/05/2023 23:11, Marijn Suijten wrote:
On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
+ if (ctx->dsi->dsc) {
dsi->dsc is always set, thus this condition can be dropped.
I want to leave room for possibly running the panel without DSC (at a
lower resolution/refresh rate, or at higher power consumption if there
is enough BW) by not assigning the pointer, if we get access to panel
documentation: probably one of the magic commands sent in this driver
controls it but we don't know which.
I'd like to investigate if DSC should perhaps only be enabled if we
run non certain platforms/socs ?
I mean, we don't know if the controller supports DSC and those
particular
DSC parameters so we should probably start adding something like :
static drm_dsc_config dsc_params_qcom = {}
static const struct of_device_id panel_of_dsc_params[] = {
{ .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
{ .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
{ .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
{ .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
};
I think this would damage the reusability of the drivers. The panel
driver does not actually care if the SoC is SM8350, sunxi-something or
RCar.
Instead it cares about host capabilities.
I think instead we should extend mipi_dsi_host:
#define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
#define MIPI_DSI_HOST_MODE_CMD BIT(1)
#define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
// FIXME: do we need to provide additional caps here ?
#define MIPI_DSI_DSC_1_1 BIT(0)
#define MIPI_DSI_DSC_1_2 BIT(1)
#define MIPI_DSI_DSC_NATIVE_422 BIT(2)
#define MIPI_DSI_DSC_NATIVE_420 BIT(3)
#define MIPI_DSI_DSC_FRAC_BPP BIT(4)
// etc.
struct mipi_dsi_host {
// new fields only
unsigned long mode_flags;
unsigned long dsc_flags;
};
Then the panel driver can adapt itself to the host capabilities and
(possibly) select one of the internally supported DSC profiles.
I completely agree about extending mipi_dsi_host, other SoCs could reuse
that and
support for DSC panels would become a lot cleaner.
Sounds good. I will wait for one or two more days (to get the possible
feedback on fields/flags/etc) and post an RFC patch to dri-devel.
I just came across that discussion, and couldn't find those patches, did
you ever send them?
Either way, I'm not really sure it's a good idea to multiply the
capabilities flags of the DSI host, and we should just stick to the
spec. If the spec says that we have to support DSC while video is
output, then that's what the panels should expect.
Except some panels supports DSC & non-DSC, Video and Command mode, and
all that is runtime configurable. How do you handle that ?
If a host isn't able to provide that, it's a bug and we should fix the
controller driver instead of creating a workaround in the core for
broken drivers.
Another concern I have is that, those broken drivers are usually the
undocumented ones that already have trouble supporting the most trivial
setup. Creating more combinations both at the controller and panel level
will just make it harder for those drivers.
Maxime
Re: Missing AMDGPU drm-fixes-6.4 merges
On Wed, 05 Jul 2023 14:17:20 +0200, Alex Deucher wrote: > > On Wed, Jul 5, 2023 at 2:26 AM Takashi Iwai wrote: > > > > Hi Dave, Alex, > > > > it seems that the last PR for AMDGPU 6.4 fixes wasn't taken by Linus > > due to the missing signed tag: > > > > https://lore.kernel.org/lkml/CAHk-=wiOCgiwzVPOwORHPML9eBphnbtM2DhRcv+v=-tnrrg...@mail.gmail.com/ > > > > And more importantly, this series isn't seen on linux-next, either; so > > the whole fixes are missing in the upstream. Could you check it? > > I included all of the necessary changes in last week's 6.5 fixes PR. > Once that lands, I'll make sure the necessary changes make it to > stable. Great, thanks for the update! Another bug report (for Leap 15.5) showed that the series fixed some nasty suspend/resume issue, so let's hope that all those get merged soon :) Takashi > > Alex > > > > > FWIW, I stumbled on this due to a recent regression report on openSUSE > > Tumbleweed: > > https://bugzilla.suse.com/show_bug.cgi?id=1212848 > > > > > > thanks, > > > > Takashi >
Re: [PATCH] drm/amdgpu: avoid integer overflow warning in amdgpu_device_resize_fb_bar()
Am 04.07.23 um 17:24 schrieb Arnd Bergmann: On Tue, Jul 4, 2023, at 16:51, Christian König wrote: Am 04.07.23 um 16:31 schrieb Arnd Bergmann: On Tue, Jul 4, 2023, at 14:33, Christian König wrote: Modern AMD GPUs have 16GiB of local memory (VRAM), making those accessible to a CPU which can only handle 32bit addresses by resizing the BAR is impossible to begin with. But going a step further even without resizing it is pretty hard to get that hardware working on such an architecture. I'd still like to understand this part better, as we have a lot of arm64 chips with somewhat flawed PCIe implementations, often with a tiny 64-bit memory space, but otherwise probably capable of using a GPU. Yeah, those are unfortunately very well known to us :( What exactly do you expect to happen here? a) Use only part of the VRAM but otherwise work as expected b) Access all of the VRAM, but at a performance cost for bank switching? We have tons of x86 systems where we can't resize the BAR (because of lack of BIOS setup of the root PCIe windows). So bank switching is still perfectly supported. Ok, good. After investigating (which sometimes even includes involving engineers from ARM) we usually find that those boards doesn't even remotely comply to the PCIe specification, both regarding power as well as functional things like DMA coherency. Makes sense, the power usage is clearly going to make this impossible on a lot of boards. I would have expected noncoherent DMA to be a solvable problem, since that generally works with all drivers that use the dma-mapping interfaces correctly, but I understand that drivers/gpu/* often does its own thing here, which may make that harder. Yeah, I've heard that before. The problem is simply that the dma-mapping interface can't handle those cases. User space APIs like Vulkan and some OpenGL extensions make a coherent memory model between GPU and CPU mandatory. In other words you have things like ring buffers between code running on the GPU and code running on the CPU and the kernel is not even involved in that communication. This is all based on the PCIe specification which makes it quite clear that things like snooping caches is mandatory for a compliant root complex. There has been success to some degree by making everything uncached, but then the performance just sucks so badly that you can practically forget it as well. Regards, Christian. Arnd
Re: Missing AMDGPU drm-fixes-6.4 merges
On Wed, Jul 5, 2023 at 2:26 AM Takashi Iwai wrote: > > Hi Dave, Alex, > > it seems that the last PR for AMDGPU 6.4 fixes wasn't taken by Linus > due to the missing signed tag: > > https://lore.kernel.org/lkml/CAHk-=wiOCgiwzVPOwORHPML9eBphnbtM2DhRcv+v=-tnrrg...@mail.gmail.com/ > > And more importantly, this series isn't seen on linux-next, either; so > the whole fixes are missing in the upstream. Could you check it? I included all of the necessary changes in last week's 6.5 fixes PR. Once that lands, I'll make sure the necessary changes make it to stable. Alex > > FWIW, I stumbled on this due to a recent regression report on openSUSE > Tumbleweed: > https://bugzilla.suse.com/show_bug.cgi?id=1212848 > > > thanks, > > Takashi
Re: [PATCH] drm/i915: Remove some dead "code"
On Wed, 05 Jul 2023, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin
>
> Commit 2caffbf11762 ("drm/i915: Revoke mmaps and prevent access to fence
> registers across reset") removed the temporary implementation of a reset
> under stop machine but forgot to remove this one commented out define.
>
> Signed-off-by: Tvrtko Ursulin
Reviewed-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/gt/intel_reset.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
> b/drivers/gpu/drm/i915/gt/intel_reset.c
> index 6916eba3bd33..cdbc08dad7ae 100644
> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
> @@ -35,9 +35,6 @@
>
> #define RESET_MAX_RETRIES 3
>
> -/* XXX How to handle concurrent GGTT updates using tiling registers? */
> -#define RESET_UNDER_STOP_MACHINE 0
> -
> static void client_mark_guilty(struct i915_gem_context *ctx, bool banned)
> {
> struct drm_i915_file_private *file_priv = ctx->file_priv;
--
Jani Nikula, Intel Open Source Graphics Center
Re: RFC: DSI host capabilities (was: [PATCH RFC 03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3)
Hi,
On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
> > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
> > > On Tue, 30 May 2023 at 10:24, Neil Armstrong
> > > wrote:
> > > >
> > > > Hi Marijn, Dmitry, Caleb, Jessica,
> > > >
> > > > On 29/05/2023 23:11, Marijn Suijten wrote:
> > > > > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> > > > >
> > > > > > > + if (ctx->dsi->dsc) {
> > > > > >
> > > > > > dsi->dsc is always set, thus this condition can be dropped.
> > > > >
> > > > > I want to leave room for possibly running the panel without DSC (at a
> > > > > lower resolution/refresh rate, or at higher power consumption if there
> > > > > is enough BW) by not assigning the pointer, if we get access to panel
> > > > > documentation: probably one of the magic commands sent in this driver
> > > > > controls it but we don't know which.
> > > >
> > > > I'd like to investigate if DSC should perhaps only be enabled if we
> > > > run non certain platforms/socs ?
> > > >
> > > > I mean, we don't know if the controller supports DSC and those
> > > > particular
> > > > DSC parameters so we should probably start adding something like :
> > > >
> > > > static drm_dsc_config dsc_params_qcom = {}
> > > >
> > > > static const struct of_device_id panel_of_dsc_params[] = {
> > > > { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
> > > > { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
> > > > { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
> > > > { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
> > > > };
> > >
> > > I think this would damage the reusability of the drivers. The panel
> > > driver does not actually care if the SoC is SM8350, sunxi-something or
> > > RCar.
> > > Instead it cares about host capabilities.
> > >
> > > I think instead we should extend mipi_dsi_host:
> > >
> > > #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
> > > #define MIPI_DSI_HOST_MODE_CMD BIT(1)
> > > #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
> > > // FIXME: do we need to provide additional caps here ?
> > >
> > > #define MIPI_DSI_DSC_1_1 BIT(0)
> > > #define MIPI_DSI_DSC_1_2 BIT(1)
> > > #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
> > > #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
> > > #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
> > > // etc.
> > >
> > > struct mipi_dsi_host {
> > > // new fields only
> > > unsigned long mode_flags;
> > > unsigned long dsc_flags;
> > > };
> > >
> > > Then the panel driver can adapt itself to the host capabilities and
> > > (possibly) select one of the internally supported DSC profiles.
> > >
> >
> > I completely agree about extending mipi_dsi_host, other SoCs could reuse
> > that and
> > support for DSC panels would become a lot cleaner.
>
> Sounds good. I will wait for one or two more days (to get the possible
> feedback on fields/flags/etc) and post an RFC patch to dri-devel.
I just came across that discussion, and couldn't find those patches, did
you ever send them?
Either way, I'm not really sure it's a good idea to multiply the
capabilities flags of the DSI host, and we should just stick to the
spec. If the spec says that we have to support DSC while video is
output, then that's what the panels should expect.
If a host isn't able to provide that, it's a bug and we should fix the
controller driver instead of creating a workaround in the core for
broken drivers.
Another concern I have is that, those broken drivers are usually the
undocumented ones that already have trouble supporting the most trivial
setup. Creating more combinations both at the controller and panel level
will just make it harder for those drivers.
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2] drm/ttm: fix one use-after-free
On 07/05/ , Matthew Auld wrote:
> On Wed, 5 Jul 2023 at 11:08, Lang Yu wrote:
> >
> > bo->kref is increased once(kref_init()) in ttm_bo_release,
> > but decreased twice(ttm_bo_put()) respectively in
> > ttm_bo_delayed_delete and ttm_bo_cleanup_refs,
> > which is unbalanced.
> >
> > Just clean up bo resource in one place for a delayed deleted bo.
> >
> > Fixes: 9bff18d13473 ("drm/ttm: use per BO cleanup workers")
> >
> > [ 67.399887] refcount_t: underflow; use-after-free.
> > [ 67.399901] WARNING: CPU: 0 PID: 3172 at lib/refcount.c:28
> > refcount_warn_saturate+0xc2/0x110
> > [ 67.400124] RIP: 0010:refcount_warn_saturate+0xc2/0x110
> > [ 67.400173] Call Trace:
> > [ 67.400176]
> > [ 67.400181] ttm_mem_evict_first+0x4fe/0x5b0 [ttm]
> > [ 67.400216] ttm_bo_mem_space+0x1e3/0x240 [ttm]
> > [ 67.400239] ttm_bo_validate+0xc7/0x190 [ttm]
> > [ 67.400253] ? ww_mutex_trylock+0x1b1/0x390
> > [ 67.400266] ttm_bo_init_reserved+0x183/0x1c0 [ttm]
> > [ 67.400280] ? __rwlock_init+0x3d/0x70
> > [ 67.400292] amdgpu_bo_create+0x1cd/0x4f0 [amdgpu]
> > [ 67.400607] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
> > [ 67.400980] amdgpu_bo_create_user+0x38/0x70 [amdgpu]
> > [ 67.401291] amdgpu_gem_object_create+0x77/0xb0 [amdgpu]
> > [ 67.401641] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
> > [ 67.401958] amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu+0x228/0xa30 [amdgpu]
> > [ 67.402433] kfd_ioctl_alloc_memory_of_gpu+0x14e/0x390 [amdgpu]
> > [ 67.402824] ? lock_release+0x13f/0x290
> > [ 67.402838] kfd_ioctl+0x1e0/0x640 [amdgpu]
> > [ 67.403205] ? __pfx_kfd_ioctl_alloc_memory_of_gpu+0x10/0x10 [amdgpu]
> > [ 67.403579] ? tomoyo_file_ioctl+0x19/0x20
> > [ 67.403590] __x64_sys_ioctl+0x95/0xd0
> > [ 67.403601] do_syscall_64+0x3b/0x90
> > [ 67.403609] entry_SYSCALL_64_after_hwframe+0x72/0xdc
> >
> > Signed-off-by: Lang Yu
> > ---
> > drivers/gpu/drm/ttm/ttm_bo.c | 89
> > 1 file changed, 10 insertions(+), 79 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 326a3d13a829..1e073dfb1332 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -224,82 +224,6 @@ static void ttm_bo_flush_all_fences(struct
> > ttm_buffer_object *bo)
> > dma_resv_iter_end(&cursor);
> > }
> >
> > -/**
> > - * ttm_bo_cleanup_refs
> > - * If bo idle, remove from lru lists, and unref.
> > - * If not idle, block if possible.
> > - *
> > - * Must be called with lru_lock and reservation held, this function
> > - * will drop the lru lock and optionally the reservation lock before
> > returning.
> > - *
> > - * @bo:The buffer object to clean-up
> > - * @interruptible: Any sleeps should occur interruptibly.
> > - * @no_wait_gpu: Never wait for gpu. Return -EBUSY instead.
> > - * @unlock_resv: Unlock the reservation lock as well.
> > - */
> > -
> > -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> > - bool interruptible, bool no_wait_gpu,
> > - bool unlock_resv)
> > -{
> > - struct dma_resv *resv = &bo->base._resv;
> > - int ret;
> > -
> > - if (dma_resv_test_signaled(resv, DMA_RESV_USAGE_BOOKKEEP))
> > - ret = 0;
> > - else
> > - ret = -EBUSY;
> > -
> > - if (ret && !no_wait_gpu) {
> > - long lret;
> > -
> > - if (unlock_resv)
> > - dma_resv_unlock(bo->base.resv);
> > - spin_unlock(&bo->bdev->lru_lock);
> > -
> > - lret = dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP,
> > -interruptible,
> > -30 * HZ);
> > -
> > - if (lret < 0)
> > - return lret;
> > - else if (lret == 0)
> > - return -EBUSY;
> > -
> > - spin_lock(&bo->bdev->lru_lock);
> > - if (unlock_resv && !dma_resv_trylock(bo->base.resv)) {
> > - /*
> > -* We raced, and lost, someone else holds the
> > reservation now,
> > -* and is probably busy in
> > ttm_bo_cleanup_memtype_use.
> > -*
> > -* Even if it's not the case, because we finished
> > waiting any
> > -* delayed destruction would succeed, so just
> > return success
> > -* here.
> > -*/
> > - spin_unlock(&bo->bdev->lru_lock);
> > - return 0;
> > - }
> > - ret = 0;
> > - }
> > -
> > - if (ret) {
> > - if (unlock_resv)
> > - dma_resv_unlock(bo->base.resv);
> > - spin_unlock(&bo->bde
Re: [PATCH 1/2] accel/ivpu: Fix VPU register access in irq disable
Applied to drm-misc-fixes
Thanks
Stanislaw
On Mon, Jul 03, 2023 at 10:07:24AM +0200, Stanislaw Gruszka wrote:
> From: Karol Wachowski
>
> Incorrect REGB_WR32() macro was used to access VPUIP register.
> Use correct REGV_WR32().
>
> Fixes: 35b137630f08 ("accel/ivpu: Introduce a new DRM driver for Intel VPU")
> Cc: sta...@vger.kernel.org # 6.3.x
> Signed-off-by: Karol Wachowski
> Signed-off-by: Stanislaw Gruszka
> ---
> drivers/accel/ivpu/ivpu_hw_mtl.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/accel/ivpu/ivpu_hw_mtl.c
> b/drivers/accel/ivpu/ivpu_hw_mtl.c
> index 3ff60fbbc8d9..d3ba633daaa0 100644
> --- a/drivers/accel/ivpu/ivpu_hw_mtl.c
> +++ b/drivers/accel/ivpu/ivpu_hw_mtl.c
> @@ -874,7 +874,7 @@ static void ivpu_hw_mtl_irq_disable(struct ivpu_device
> *vdev)
> REGB_WR32(MTL_BUTTRESS_GLOBAL_INT_MASK, 0x1);
> REGB_WR32(MTL_BUTTRESS_LOCAL_INT_MASK, BUTTRESS_IRQ_DISABLE_MASK);
> REGV_WR64(MTL_VPU_HOST_SS_ICB_ENABLE_0, 0x0ull);
> - REGB_WR32(MTL_VPU_HOST_SS_FW_SOC_IRQ_EN, 0x0);
> + REGV_WR32(MTL_VPU_HOST_SS_FW_SOC_IRQ_EN, 0x0);
> }
>
> static void ivpu_hw_mtl_irq_wdt_nce_handler(struct ivpu_device *vdev)
> --
> 2.25.1
>
Re: [PATCH v2] drm/ttm: fix one use-after-free
On Wed, 5 Jul 2023 at 11:08, Lang Yu wrote:
>
> bo->kref is increased once(kref_init()) in ttm_bo_release,
> but decreased twice(ttm_bo_put()) respectively in
> ttm_bo_delayed_delete and ttm_bo_cleanup_refs,
> which is unbalanced.
>
> Just clean up bo resource in one place for a delayed deleted bo.
>
> Fixes: 9bff18d13473 ("drm/ttm: use per BO cleanup workers")
>
> [ 67.399887] refcount_t: underflow; use-after-free.
> [ 67.399901] WARNING: CPU: 0 PID: 3172 at lib/refcount.c:28
> refcount_warn_saturate+0xc2/0x110
> [ 67.400124] RIP: 0010:refcount_warn_saturate+0xc2/0x110
> [ 67.400173] Call Trace:
> [ 67.400176]
> [ 67.400181] ttm_mem_evict_first+0x4fe/0x5b0 [ttm]
> [ 67.400216] ttm_bo_mem_space+0x1e3/0x240 [ttm]
> [ 67.400239] ttm_bo_validate+0xc7/0x190 [ttm]
> [ 67.400253] ? ww_mutex_trylock+0x1b1/0x390
> [ 67.400266] ttm_bo_init_reserved+0x183/0x1c0 [ttm]
> [ 67.400280] ? __rwlock_init+0x3d/0x70
> [ 67.400292] amdgpu_bo_create+0x1cd/0x4f0 [amdgpu]
> [ 67.400607] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
> [ 67.400980] amdgpu_bo_create_user+0x38/0x70 [amdgpu]
> [ 67.401291] amdgpu_gem_object_create+0x77/0xb0 [amdgpu]
> [ 67.401641] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
> [ 67.401958] amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu+0x228/0xa30 [amdgpu]
> [ 67.402433] kfd_ioctl_alloc_memory_of_gpu+0x14e/0x390 [amdgpu]
> [ 67.402824] ? lock_release+0x13f/0x290
> [ 67.402838] kfd_ioctl+0x1e0/0x640 [amdgpu]
> [ 67.403205] ? __pfx_kfd_ioctl_alloc_memory_of_gpu+0x10/0x10 [amdgpu]
> [ 67.403579] ? tomoyo_file_ioctl+0x19/0x20
> [ 67.403590] __x64_sys_ioctl+0x95/0xd0
> [ 67.403601] do_syscall_64+0x3b/0x90
> [ 67.403609] entry_SYSCALL_64_after_hwframe+0x72/0xdc
>
> Signed-off-by: Lang Yu
> ---
> drivers/gpu/drm/ttm/ttm_bo.c | 89
> 1 file changed, 10 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 326a3d13a829..1e073dfb1332 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -224,82 +224,6 @@ static void ttm_bo_flush_all_fences(struct
> ttm_buffer_object *bo)
> dma_resv_iter_end(&cursor);
> }
>
> -/**
> - * ttm_bo_cleanup_refs
> - * If bo idle, remove from lru lists, and unref.
> - * If not idle, block if possible.
> - *
> - * Must be called with lru_lock and reservation held, this function
> - * will drop the lru lock and optionally the reservation lock before
> returning.
> - *
> - * @bo:The buffer object to clean-up
> - * @interruptible: Any sleeps should occur interruptibly.
> - * @no_wait_gpu: Never wait for gpu. Return -EBUSY instead.
> - * @unlock_resv: Unlock the reservation lock as well.
> - */
> -
> -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> - bool interruptible, bool no_wait_gpu,
> - bool unlock_resv)
> -{
> - struct dma_resv *resv = &bo->base._resv;
> - int ret;
> -
> - if (dma_resv_test_signaled(resv, DMA_RESV_USAGE_BOOKKEEP))
> - ret = 0;
> - else
> - ret = -EBUSY;
> -
> - if (ret && !no_wait_gpu) {
> - long lret;
> -
> - if (unlock_resv)
> - dma_resv_unlock(bo->base.resv);
> - spin_unlock(&bo->bdev->lru_lock);
> -
> - lret = dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP,
> -interruptible,
> -30 * HZ);
> -
> - if (lret < 0)
> - return lret;
> - else if (lret == 0)
> - return -EBUSY;
> -
> - spin_lock(&bo->bdev->lru_lock);
> - if (unlock_resv && !dma_resv_trylock(bo->base.resv)) {
> - /*
> -* We raced, and lost, someone else holds the
> reservation now,
> -* and is probably busy in ttm_bo_cleanup_memtype_use.
> -*
> -* Even if it's not the case, because we finished
> waiting any
> -* delayed destruction would succeed, so just return
> success
> -* here.
> -*/
> - spin_unlock(&bo->bdev->lru_lock);
> - return 0;
> - }
> - ret = 0;
> - }
> -
> - if (ret) {
> - if (unlock_resv)
> - dma_resv_unlock(bo->base.resv);
> - spin_unlock(&bo->bdev->lru_lock);
> - return ret;
> - }
> -
> - spin_unlock(&bo->bdev->lru_lock);
> - ttm_bo_cleanup_memtype_use(bo);
> -
> - if (unlock_resv)
> - dma_resv_unlock(bo->base.resv);
> -
> - ttm_bo_put(bo);
The put() here
Re: [32/39] drm: renesas: shmobile: Shutdown the display on remove
Hi Sui,
On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng wrote:
> On 2023/6/22 17:21, Geert Uytterhoeven wrote:
> > When the device is unbound from the driver, the display may be active.
> > Make sure it gets shut down.
>
> would you mind to give a short description why this is necessary.
That's a good comment.
It turned out that this is not really necessary here, but to avoid a regression
with "[PATCH 34/39] drm: renesas: shmobile: Atomic conversion part 1", where
it is needed to call drm_atomic_helper_shutdown().
As the comments for drm_atomic_helper_shutdown() says it is the
atomic version of drm_helper_force_disable_all(), I figured I had to
introduce a call to the latter first, before doing the atomic conversion.
Does that make sense?
> > Signed-off-by: Geert Uytterhoeven
> > Reviewed-by: Laurent Pinchart
> > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> > @@ -16,6 +16,7 @@
> > #include
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device
> > *pdev)
> > struct drm_device *ddev = &sdev->ddev;
> >
> > drm_dev_unregister(ddev);
> > + drm_helper_force_disable_all(ddev);
>
> Is it that the DRM core recommend us to use
> drm_atomic_helper_disable_all() ?
Well, drm_atomic_helper_shutdown() is a convenience wrapper
around drm_atomic_helper_disable_all()... But we can't call any
atomic helpers yet, before the conversion to atomic modesetting.
>
> > drm_kms_helper_poll_fini(ddev);
> > return 0;
> > }
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] drm/ttm: fix one use-after-free
bo->kref is increased once(kref_init()) in ttm_bo_release,
but decreased twice(ttm_bo_put()) respectively in
ttm_bo_delayed_delete and ttm_bo_cleanup_refs,
which is unbalanced.
Just clean up bo resource in one place for a delayed deleted bo.
Fixes: 9bff18d13473 ("drm/ttm: use per BO cleanup workers")
[ 67.399887] refcount_t: underflow; use-after-free.
[ 67.399901] WARNING: CPU: 0 PID: 3172 at lib/refcount.c:28
refcount_warn_saturate+0xc2/0x110
[ 67.400124] RIP: 0010:refcount_warn_saturate+0xc2/0x110
[ 67.400173] Call Trace:
[ 67.400176]
[ 67.400181] ttm_mem_evict_first+0x4fe/0x5b0 [ttm]
[ 67.400216] ttm_bo_mem_space+0x1e3/0x240 [ttm]
[ 67.400239] ttm_bo_validate+0xc7/0x190 [ttm]
[ 67.400253] ? ww_mutex_trylock+0x1b1/0x390
[ 67.400266] ttm_bo_init_reserved+0x183/0x1c0 [ttm]
[ 67.400280] ? __rwlock_init+0x3d/0x70
[ 67.400292] amdgpu_bo_create+0x1cd/0x4f0 [amdgpu]
[ 67.400607] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
[ 67.400980] amdgpu_bo_create_user+0x38/0x70 [amdgpu]
[ 67.401291] amdgpu_gem_object_create+0x77/0xb0 [amdgpu]
[ 67.401641] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
[ 67.401958] amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu+0x228/0xa30 [amdgpu]
[ 67.402433] kfd_ioctl_alloc_memory_of_gpu+0x14e/0x390 [amdgpu]
[ 67.402824] ? lock_release+0x13f/0x290
[ 67.402838] kfd_ioctl+0x1e0/0x640 [amdgpu]
[ 67.403205] ? __pfx_kfd_ioctl_alloc_memory_of_gpu+0x10/0x10 [amdgpu]
[ 67.403579] ? tomoyo_file_ioctl+0x19/0x20
[ 67.403590] __x64_sys_ioctl+0x95/0xd0
[ 67.403601] do_syscall_64+0x3b/0x90
[ 67.403609] entry_SYSCALL_64_after_hwframe+0x72/0xdc
Signed-off-by: Lang Yu
---
drivers/gpu/drm/ttm/ttm_bo.c | 89
1 file changed, 10 insertions(+), 79 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index 326a3d13a829..1e073dfb1332 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -224,82 +224,6 @@ static void ttm_bo_flush_all_fences(struct
ttm_buffer_object *bo)
dma_resv_iter_end(&cursor);
}
-/**
- * ttm_bo_cleanup_refs
- * If bo idle, remove from lru lists, and unref.
- * If not idle, block if possible.
- *
- * Must be called with lru_lock and reservation held, this function
- * will drop the lru lock and optionally the reservation lock before returning.
- *
- * @bo:The buffer object to clean-up
- * @interruptible: Any sleeps should occur interruptibly.
- * @no_wait_gpu: Never wait for gpu. Return -EBUSY instead.
- * @unlock_resv: Unlock the reservation lock as well.
- */
-
-static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
- bool interruptible, bool no_wait_gpu,
- bool unlock_resv)
-{
- struct dma_resv *resv = &bo->base._resv;
- int ret;
-
- if (dma_resv_test_signaled(resv, DMA_RESV_USAGE_BOOKKEEP))
- ret = 0;
- else
- ret = -EBUSY;
-
- if (ret && !no_wait_gpu) {
- long lret;
-
- if (unlock_resv)
- dma_resv_unlock(bo->base.resv);
- spin_unlock(&bo->bdev->lru_lock);
-
- lret = dma_resv_wait_timeout(resv, DMA_RESV_USAGE_BOOKKEEP,
-interruptible,
-30 * HZ);
-
- if (lret < 0)
- return lret;
- else if (lret == 0)
- return -EBUSY;
-
- spin_lock(&bo->bdev->lru_lock);
- if (unlock_resv && !dma_resv_trylock(bo->base.resv)) {
- /*
-* We raced, and lost, someone else holds the
reservation now,
-* and is probably busy in ttm_bo_cleanup_memtype_use.
-*
-* Even if it's not the case, because we finished
waiting any
-* delayed destruction would succeed, so just return
success
-* here.
-*/
- spin_unlock(&bo->bdev->lru_lock);
- return 0;
- }
- ret = 0;
- }
-
- if (ret) {
- if (unlock_resv)
- dma_resv_unlock(bo->base.resv);
- spin_unlock(&bo->bdev->lru_lock);
- return ret;
- }
-
- spin_unlock(&bo->bdev->lru_lock);
- ttm_bo_cleanup_memtype_use(bo);
-
- if (unlock_resv)
- dma_resv_unlock(bo->base.resv);
-
- ttm_bo_put(bo);
-
- return 0;
-}
-
/*
* Block for the dma_resv object to become idle, lock the buffer and clean up
* the resource and tt object.
@@ -622,8 +546,10 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
}
if (bo->deleted) {
- ret = ttm_bo_cleanup_refs(bo, ctx->interrupt
Re: [PATCH v4 1/6] drm: allow DRM_MODE_PAGE_FLIP_ASYNC for atomic commits
On 7/4/23 19:06, Sebastian Wick wrote:
> On Sat, Jul 1, 2023 at 4:09 AM André Almeida wrote:
>>
>> @@ -949,6 +949,15 @@ struct hdr_output_metadata {
>> * Request that the page-flip is performed as soon as possible, ie. with no
>> * delay due to waiting for vblank. This may cause tearing to be visible on
>> * the screen.
>> + *
>> + * When used with atomic uAPI, the driver will return an error if the
>> hardware
>> + * doesn't support performing an asynchronous page-flip for this update.
>> + * User-space should handle this, e.g. by falling back to a regular
>> page-flip.
>> + *
>> + * Note, some hardware might need to perform one last synchronous page-flip
>> + * before being able to switch to asynchronous page-flips. As an exception,
>> + * the driver will return success even though that first page-flip is not
>> + * asynchronous.
>
> What would happen if one commits another async KMS update before the
> first page flip? Does one receive EAGAIN, does it amend the previous
> commit?
Should be the former. DRM_MODE_PAGE_FLIP_ASYNC just means the flip may complete
outside of vertical blank, it doesn't change anything else.
--
Earthling Michel Dänzer| https://redhat.com
Libre software enthusiast | Mesa and Xwayland developer
[PATCH] drm/i915: Remove some dead "code"
From: Tvrtko Ursulin
Commit 2caffbf11762 ("drm/i915: Revoke mmaps and prevent access to fence
registers across reset") removed the temporary implementation of a reset
under stop machine but forgot to remove this one commented out define.
Signed-off-by: Tvrtko Ursulin
---
drivers/gpu/drm/i915/gt/intel_reset.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
b/drivers/gpu/drm/i915/gt/intel_reset.c
index 6916eba3bd33..cdbc08dad7ae 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -35,9 +35,6 @@
#define RESET_MAX_RETRIES 3
-/* XXX How to handle concurrent GGTT updates using tiling registers? */
-#define RESET_UNDER_STOP_MACHINE 0
-
static void client_mark_guilty(struct i915_gem_context *ctx, bool banned)
{
struct drm_i915_file_private *file_priv = ctx->file_priv;
--
2.39.2
Re: [PATCH 06/10] drm/exynos: Set fbdev flags
Thomas Zimmermann writes: > Hi > > Am 05.07.23 um 10:49 schrieb Javier Martinez Canillas: [...] >> >> The #define FBINFO_FLAG_DEFAULT FBINFO_DEFAULT seems to be there since >> the >> original v2.6.12-rc2 git import in commit 1da177e4c3f4, so is hard to know >> why was introduced. FBINFO_DEFAULT is more used, I will just stick to that: > > Thanks for commenting on this issue. I didn't notice that. > > I think I'll just remove these _DEFAULT assignments from the patchset. > > And I think we should nuke them entirely everywhere. The _DEFAULT values > are just 0 after commit 376b3ff54c9a1. So there's no value in assigning > them at all. > Agreed. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
[PATCH v2 2/3] drm/mediatek: Use dev_err_probe() in probe functions
Convert all instances of dev_err() -> return to dev_err_probe() and
where it makes sense to, change instances of `return ret` at the end
of probe functions to `return 0`, as errors are returned earlier.
Signed-off-by: AngeloGioacchino Del Regno
Reviewed-by: Alexandre Mergnat
Reviewed-by: CK Hu
---
drivers/gpu/drm/mediatek/mtk_cec.c| 26 +
drivers/gpu/drm/mediatek/mtk_disp_aal.c | 16 --
drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 16 --
drivers/gpu/drm/mediatek/mtk_disp_color.c | 17 +--
drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 16 --
drivers/gpu/drm/mediatek/mtk_disp_merge.c | 25 +++-
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 23 ++-
.../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 6 ++--
drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 29 +++
drivers/gpu/drm/mediatek/mtk_dsi.c| 18 +---
drivers/gpu/drm/mediatek/mtk_ethdr.c | 18 +---
drivers/gpu/drm/mediatek/mtk_hdmi.c | 14 +++--
drivers/gpu/drm/mediatek/mtk_hdmi_ddc.c | 12 +++-
drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 18 +---
14 files changed, 96 insertions(+), 158 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_cec.c
b/drivers/gpu/drm/mediatek/mtk_cec.c
index c3b89a5c138a..56b3917801d7 100644
--- a/drivers/gpu/drm/mediatek/mtk_cec.c
+++ b/drivers/gpu/drm/mediatek/mtk_cec.c
@@ -196,18 +196,12 @@ static int mtk_cec_probe(struct platform_device *pdev)
spin_lock_init(&cec->lock);
cec->regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(cec->regs)) {
- ret = PTR_ERR(cec->regs);
- dev_err(dev, "Failed to ioremap cec: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(cec->regs))
+ return dev_err_probe(dev, PTR_ERR(cec->regs), "Failed to
ioremap cec\n");
cec->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(cec->clk)) {
- ret = PTR_ERR(cec->clk);
- dev_err(dev, "Failed to get cec clock: %d\n", ret);
- return ret;
- }
+ if (IS_ERR(cec->clk))
+ return dev_err_probe(dev, PTR_ERR(cec->clk), "Failed to get cec
clock\n");
cec->irq = platform_get_irq(pdev, 0);
if (cec->irq < 0)
@@ -217,16 +211,12 @@ static int mtk_cec_probe(struct platform_device *pdev)
mtk_cec_htplg_isr_thread,
IRQF_SHARED | IRQF_TRIGGER_LOW |
IRQF_ONESHOT, "hdmi hpd", dev);
- if (ret) {
- dev_err(dev, "Failed to register cec irq: %d\n", ret);
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to register cec irq\n");
ret = clk_prepare_enable(cec->clk);
- if (ret) {
- dev_err(dev, "Failed to enable cec clock: %d\n", ret);
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to enable cec clock\n");
mtk_cec_htplg_irq_init(cec);
mtk_cec_htplg_irq_enable(cec);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index bd1d67a5baff..c60aa244de67 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -111,16 +111,12 @@ static int mtk_disp_aal_probe(struct platform_device
*pdev)
return -ENOMEM;
priv->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(priv->clk)) {
- dev_err(dev, "failed to get aal clk\n");
- return PTR_ERR(priv->clk);
- }
+ if (IS_ERR(priv->clk))
+ return dev_err_probe(dev, PTR_ERR(priv->clk), "failed to get
aal clk\n");
priv->regs = devm_platform_ioremap_resource(pdev, 0);
- if (IS_ERR(priv->regs)) {
- dev_err(dev, "failed to ioremap aal\n");
- return PTR_ERR(priv->regs);
- }
+ if (IS_ERR(priv->regs))
+ return dev_err_probe(dev, PTR_ERR(priv->regs), "failed to
ioremap aal\n");
#if IS_REACHABLE(CONFIG_MTK_CMDQ)
ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
@@ -133,9 +129,9 @@ static int mtk_disp_aal_probe(struct platform_device *pdev)
ret = component_add(dev, &mtk_disp_aal_component_ops);
if (ret)
- dev_err(dev, "Failed to add component: %d\n", ret);
+ return dev_err_probe(dev, ret, "Failed to add component\n");
- return ret;
+ return 0;
}
static int mtk_disp_aal_remove(struct platform_device *pdev)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
index 5cee84cce0be..77bc8ae7c536 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
@@ -166,16 +166,12 @@ stat
[PATCH v2 1/3] drm/mediatek: Use devm_platform_ioremap_resource()
Instead of open coding calls to platform_get_resource() followed by
devm_ioremap_resource(), perform a single call to the helper
devm_platform_ioremap_resource().
Also, in order to drop the now useless struct resource pointer in
all of the probe functions, it was also necessary to remove a
dev_dbg() in mtk_hdmi_ddc.c that was printing the iospace start/end.
This commit brings no functional changes.
Signed-off-by: AngeloGioacchino Del Regno
---
drivers/gpu/drm/mediatek/mtk_cec.c| 3 +--
drivers/gpu/drm/mediatek/mtk_disp_aal.c | 4 +---
drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 4 +---
drivers/gpu/drm/mediatek/mtk_disp_color.c | 4 +---
drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 4 +---
drivers/gpu/drm/mediatek/mtk_disp_merge.c | 4 +---
drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 4 +---
drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 4 +---
drivers/gpu/drm/mediatek/mtk_dpi.c| 3 +--
drivers/gpu/drm/mediatek/mtk_dsi.c| 4 +---
drivers/gpu/drm/mediatek/mtk_hdmi.c | 4 +---
drivers/gpu/drm/mediatek/mtk_hdmi_ddc.c | 6 +-
drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 4 +---
13 files changed, 13 insertions(+), 39 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_cec.c
b/drivers/gpu/drm/mediatek/mtk_cec.c
index b640bc0559e7..c3b89a5c138a 100644
--- a/drivers/gpu/drm/mediatek/mtk_cec.c
+++ b/drivers/gpu/drm/mediatek/mtk_cec.c
@@ -195,8 +195,7 @@ static int mtk_cec_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, cec);
spin_lock_init(&cec->lock);
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- cec->regs = devm_ioremap_resource(dev, res);
+ cec->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(cec->regs)) {
ret = PTR_ERR(cec->regs);
dev_err(dev, "Failed to ioremap cec: %d\n", ret);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
index 8ddf7a97e583..bd1d67a5baff 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_aal.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_aal.c
@@ -104,7 +104,6 @@ static int mtk_disp_aal_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct mtk_disp_aal *priv;
- struct resource *res;
int ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -117,8 +116,7 @@ static int mtk_disp_aal_probe(struct platform_device *pdev)
return PTR_ERR(priv->clk);
}
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- priv->regs = devm_ioremap_resource(dev, res);
+ priv->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(priv->regs)) {
dev_err(dev, "failed to ioremap aal\n");
return PTR_ERR(priv->regs);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
index 1773379b2439..5cee84cce0be 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ccorr.c
@@ -159,7 +159,6 @@ static int mtk_disp_ccorr_probe(struct platform_device
*pdev)
{
struct device *dev = &pdev->dev;
struct mtk_disp_ccorr *priv;
- struct resource *res;
int ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -172,8 +171,7 @@ static int mtk_disp_ccorr_probe(struct platform_device
*pdev)
return PTR_ERR(priv->clk);
}
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- priv->regs = devm_ioremap_resource(dev, res);
+ priv->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(priv->regs)) {
dev_err(dev, "failed to ioremap ccorr\n");
return PTR_ERR(priv->regs);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_color.c
b/drivers/gpu/drm/mediatek/mtk_disp_color.c
index cac9206079e7..e3816730ab51 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_color.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_color.c
@@ -97,7 +97,6 @@ static int mtk_disp_color_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct mtk_disp_color *priv;
- struct resource *res;
int ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -110,8 +109,7 @@ static int mtk_disp_color_probe(struct platform_device
*pdev)
return PTR_ERR(priv->clk);
}
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- priv->regs = devm_ioremap_resource(dev, res);
+ priv->regs = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(priv->regs)) {
dev_err(dev, "failed to ioremap color\n");
return PTR_ERR(priv->regs);
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
index bd530e603264..6ab67e6392c7 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_gamma.c
@@ -263,7 +263,6 @@ static int mt
[PATCH v2 3/3] drm/mediatek: Use devm variant for pm_runtime_enable() when possible
Simplify the error path of return functions and drop the call to
pm_runtime_disable() in remove functions by switching to
devm_pm_runtime_enable() where possible.
Signed-off-by: AngeloGioacchino Del Regno
Reviewed-by: Alexandre Mergnat
Reviewed-by: CK Hu
---
drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 9 -
drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 11 ---
drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 10 +-
3 files changed, 13 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
index 1993b688befa..14e8ad6c78c3 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
@@ -519,13 +519,13 @@ static int mtk_disp_ovl_adaptor_probe(struct
platform_device *pdev)
component_master_add_with_match(dev, &mtk_disp_ovl_adaptor_master_ops,
match);
- pm_runtime_enable(dev);
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
ret = component_add(dev, &mtk_disp_ovl_adaptor_comp_ops);
- if (ret) {
- pm_runtime_disable(dev);
+ if (ret)
return dev_err_probe(dev, ret, "Failed to add component\n");
- }
return 0;
}
@@ -533,7 +533,6 @@ static int mtk_disp_ovl_adaptor_probe(struct
platform_device *pdev)
static int mtk_disp_ovl_adaptor_remove(struct platform_device *pdev)
{
component_master_del(&pdev->dev, &mtk_disp_ovl_adaptor_master_ops);
- pm_runtime_disable(&pdev->dev);
return 0;
}
diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
index cfbc037a0f6d..0469076cf715 100644
--- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
@@ -360,13 +360,13 @@ static int mtk_disp_rdma_probe(struct platform_device
*pdev)
platform_set_drvdata(pdev, priv);
- pm_runtime_enable(dev);
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
ret = component_add(dev, &mtk_disp_rdma_component_ops);
- if (ret) {
- pm_runtime_disable(dev);
+ if (ret)
return dev_err_probe(dev, ret, "Failed to add component\n");
- }
return 0;
}
@@ -374,9 +374,6 @@ static int mtk_disp_rdma_probe(struct platform_device *pdev)
static int mtk_disp_rdma_remove(struct platform_device *pdev)
{
component_del(&pdev->dev, &mtk_disp_rdma_component_ops);
-
- pm_runtime_disable(&pdev->dev);
-
return 0;
}
diff --git a/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c
b/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c
index ae05d9660592..a5d811c37207 100644
--- a/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c
+++ b/drivers/gpu/drm/mediatek/mtk_mdp_rdma.c
@@ -299,20 +299,20 @@ static int mtk_mdp_rdma_probe(struct platform_device
*pdev)
#endif
platform_set_drvdata(pdev, priv);
- pm_runtime_enable(dev);
+ ret = devm_pm_runtime_enable(dev);
+ if (ret)
+ return ret;
ret = component_add(dev, &mtk_mdp_rdma_component_ops);
- if (ret) {
- pm_runtime_disable(dev);
+ if (ret)
return dev_err_probe(dev, ret, "Failed to add component\n");
- }
+
return 0;
}
static int mtk_mdp_rdma_remove(struct platform_device *pdev)
{
component_del(&pdev->dev, &mtk_mdp_rdma_component_ops);
- pm_runtime_disable(&pdev->dev);
return 0;
}
--
2.40.1
[PATCH v2 0/3] drm/mediatek: General cleanups
This series performs some cleanups in drm/mediatek; specifically, changes it to use devm_platform_ioremap_resource(), dev_err_probe() and devm_pm_runtime_enable, hence harmonizing log formats and removing some unneeded lines of code. Changes in v2: - Switched from devm_platform_get_and_ioremap_resource() to dropping struct resource pointer with using devm_platform_ioremap_resource() AngeloGioacchino Del Regno (3): drm/mediatek: Use devm_platform_ioremap_resource() drm/mediatek: Use dev_err_probe() in probe functions drm/mediatek: Use devm variant for pm_runtime_enable() when possible drivers/gpu/drm/mediatek/mtk_cec.c| 29 drivers/gpu/drm/mediatek/mtk_disp_aal.c | 22 -- drivers/gpu/drm/mediatek/mtk_disp_ccorr.c | 20 +++-- drivers/gpu/drm/mediatek/mtk_disp_color.c | 23 -- drivers/gpu/drm/mediatek/mtk_disp_gamma.c | 20 +++-- drivers/gpu/drm/mediatek/mtk_disp_merge.c | 29 +--- drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 27 +--- .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 13 +++--- drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 44 +++ drivers/gpu/drm/mediatek/mtk_dpi.c| 3 +- drivers/gpu/drm/mediatek/mtk_dsi.c| 22 -- drivers/gpu/drm/mediatek/mtk_ethdr.c | 18 drivers/gpu/drm/mediatek/mtk_hdmi.c | 18 +++- drivers/gpu/drm/mediatek/mtk_hdmi_ddc.c | 18 +++- drivers/gpu/drm/mediatek/mtk_mdp_rdma.c | 30 + 15 files changed, 122 insertions(+), 214 deletions(-) -- 2.40.1
Re: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
Hi Inki,
Le mardi 04 juillet 2023 à 08:49 +0900, 대인기/Tizen Platform Lab(SR)/삼성전자
a écrit :
> Hi,
>
> > -Original Message-
> > From: dri-devel On Behalf
> > Of
> > Paul Cercueil
> > Sent: Tuesday, July 4, 2023 6:47 AM
> > To: Krzysztof Kozlowski ; Rob
> > Herring
> > ; Conor Dooley ; Alim
> > Akhtar
> > ; Neil Armstrong
> > ; Sam
> > Ravnborg
> > Cc: devicet...@vger.kernel.org; linux-samsung-...@vger.kernel.org;
> > linux-
> > ker...@vger.kernel.org; dri-devel@lists.freedesktop.org; Paul
> > Cercueil
> > ; linux-arm-ker...@lists.infradead.org
> > Subject: [PATCH 2/3] drm/panel: ld9040: Register a backlight device
> >
> > Register a backlight device to be able to switch between all the
> > gamma
> > levels.
> >
> > Signed-off-by: Paul Cercueil
> > ---
> > drivers/gpu/drm/panel/panel-samsung-ld9040.c | 40
> >
> > 1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > index 7fd9444b42c5..b4f87d6244cb 100644
> > --- a/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > +++ b/drivers/gpu/drm/panel/panel-samsung-ld9040.c
> > @@ -8,6 +8,7 @@
> > * Andrzej Hajda
> > */
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -311,8 +312,40 @@ static int ld9040_parse_dt(struct ld9040 *ctx)
> > return 0;
> > }
> >
> > +static int ld9040_bl_update_status(struct backlight_device *dev)
> > +{
> > + struct ld9040 *ctx = dev_get_drvdata(&dev->dev);
> > +
> > + ctx->brightness = dev->props.brightness;
> > + ld9040_brightness_set(ctx);
> > +
> > + return 0;
> > +}
> > +
> > +static int ld9040_bl_get_intensity(struct backlight_device *dev)
> > +{
> > + if (dev->props.fb_blank == FB_BLANK_UNBLANK &&
>
> fb_blank member is deprecated according to the description of
> backlight.h
> file so you could drop above condition I think.
Thanks. I'll send a V2.
Cheers,
-Paul
Re: [PATCH 04/10] drm/tegra: Set fbdev flags
Thomas Zimmermann writes: [...] >>> + info->flags |= FBINFO_VIRTFB; >> >> I see that all fbdev drivers just do: info->flags = FBINFO_FLAG_DEFAULT | >> FBINFO_VIRTFB >> >> Guess you are doing in two assignments to be consistent with drm_fbdev_dma.c >> ? >> I was just curious about the rationale for setting the flags in two steps. > > The _DEFAULT flag is really just a zero. And the other flags describe > different aspects of the framebuffer. I think it makes sense to set the > flags together with the respective state. For example, _VIRTFB is set > next to ->screen_buffer, because they belong together. > Yes, that makes sense. > _VIRTFB is currently only used in defio code at > > https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/fb_defio.c#L232 > > I think the fbdev I/O helpers should also test this flag after all > drivers have been annotated correctly. For example, fb_io_read() would > WARN_ONCE if the _VIRTFB flag has been set; and fb_sys_read() would warn > if it hasn't been set. For the read helpers, it also makes sense to > WARN_ONCE if the _READS_FAST flag has not been set. > Agreed. Maybe you could add those warn (or at least info or debug?) even if not all drivers have been annotated correctly. That way people can be aware that is missing and fix if there are remaining drivers. > Best regards > Thomas > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 06/10] drm/exynos: Set fbdev flags
Hi Am 05.07.23 um 10:49 schrieb Javier Martinez Canillas: Thomas Zimmermann writes: Set fbdev default flags FBNFO_DEFAULT and mark the framebuffer with FBINFO_DEFAULT, or did you meand FBINFO_FLAG_DEFAULT (the flag your patch is actually using) ? I just noticed that are the same... and in patch 04/10 you used the former for the tegra driver, but here you are using the latter. Is on purpose or just a mistake ? FBINFO_VIRTFB. The framebuffer range is in DMA-able memory and should be accessed with the CPU's regular memory ops. Signed-off-by: Thomas Zimmermann Cc: Inki Dae Cc: Seung-Woo Kim Cc: Kyungmin Park Cc: Krzysztof Kozlowski Cc: Alim Akhtar --- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c index 7ca3424b59ce..28dc398d6e10 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c @@ -72,6 +72,7 @@ static int exynos_drm_fbdev_update(struct drm_fb_helper *helper, return PTR_ERR(fbi); } + fbi->flags = FBINFO_FLAG_DEFAULT; The #define FBINFO_FLAG_DEFAULT FBINFO_DEFAULT seems to be there since the original v2.6.12-rc2 git import in commit 1da177e4c3f4, so is hard to know why was introduced. FBINFO_DEFAULT is more used, I will just stick to that: Thanks for commenting on this issue. I didn't notice that. I think I'll just remove these _DEFAULT assignments from the patchset. And I think we should nuke them entirely everywhere. The _DEFAULT values are just 0 after commit 376b3ff54c9a1. So there's no value in assigning them at all. Best regards Thomas $ git grep FBINFO_DEFAULT | wc -l 92 $ git grep FBINFO_FLAG_DEFAULT | wc -l 38 Reviewed-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) OpenPGP_signature Description: OpenPGP digital signature
[PATCH] drm/i915: Do not disable preemption for resets
From: Tvrtko Ursulin
Commit ade8a0f59844 ("drm/i915: Make all GPU resets atomic") added a
preempt disable section over the hardware reset callback to prepare the
driver for being able to reset from atomic contexts.
In retrospect I can see that the work item at a time was about removing
the struct mutex from the reset path. Code base also briefly entertained
the idea of doing the reset under stop_machine in order to serialize
userspace mmap and temporary glitch in the fence registers (see
eb8d0f5af4ec ("drm/i915: Remove GPU reset dependence on struct_mutex"),
but that never materialized and was soon removed in 2caffbf11762
("drm/i915: Revoke mmaps and prevent access to fence registers across
reset") and replaced with a SRCU based solution.
As such, as far as I can see, today we still have a requirement that
resets must not sleep (invoked from submission tasklets), but no need to
support invoking them from a truly atomic context.
Given that the preemption section is problematic on RT kernels, since the
uncore lock becomes a sleeping lock and so is invalid in such section,
lets try and remove it. Potential downside is that our short waits on GPU
to complete the reset may get extended if CPU scheduling interferes, but
in practice that probably isn't a deal breaker.
In terms of mechanics, since the preemption disabled block is being
removed we just need to replace a few of the wait_for_atomic macros into
busy looping versions which will work (and not complain) when called from
non-atomic sections.
Signed-off-by: Tvrtko Ursulin
Cc: Chris Wilson
Cc: Paul Gortmaker
Cc: Sebastian Andrzej Siewior
---
drivers/gpu/drm/i915/gt/intel_reset.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
b/drivers/gpu/drm/i915/gt/intel_reset.c
index e2152f75ba2e..6916eba3bd33 100644
--- a/drivers/gpu/drm/i915/gt/intel_reset.c
+++ b/drivers/gpu/drm/i915/gt/intel_reset.c
@@ -167,13 +167,13 @@ static int i915_do_reset(struct intel_gt *gt,
/* Assert reset for at least 20 usec, and wait for acknowledgement. */
pci_write_config_byte(pdev, I915_GDRST, GRDOM_RESET_ENABLE);
udelay(50);
- err = wait_for_atomic(i915_in_reset(pdev), 50);
+ err = _wait_for_atomic(i915_in_reset(pdev), 50, 0);
/* Clear the reset request. */
pci_write_config_byte(pdev, I915_GDRST, 0);
udelay(50);
if (!err)
- err = wait_for_atomic(!i915_in_reset(pdev), 50);
+ err = _wait_for_atomic(!i915_in_reset(pdev), 50, 0);
return err;
}
@@ -193,7 +193,7 @@ static int g33_do_reset(struct intel_gt *gt,
struct pci_dev *pdev = to_pci_dev(gt->i915->drm.dev);
pci_write_config_byte(pdev, I915_GDRST, GRDOM_RESET_ENABLE);
- return wait_for_atomic(g4x_reset_complete(pdev), 50);
+ return _wait_for_atomic(g4x_reset_complete(pdev), 50, 0);
}
static int g4x_do_reset(struct intel_gt *gt,
@@ -210,7 +210,7 @@ static int g4x_do_reset(struct intel_gt *gt,
pci_write_config_byte(pdev, I915_GDRST,
GRDOM_MEDIA | GRDOM_RESET_ENABLE);
- ret = wait_for_atomic(g4x_reset_complete(pdev), 50);
+ ret = _wait_for_atomic(g4x_reset_complete(pdev), 50, 0);
if (ret) {
GT_TRACE(gt, "Wait for media reset failed\n");
goto out;
@@ -218,7 +218,7 @@ static int g4x_do_reset(struct intel_gt *gt,
pci_write_config_byte(pdev, I915_GDRST,
GRDOM_RENDER | GRDOM_RESET_ENABLE);
- ret = wait_for_atomic(g4x_reset_complete(pdev), 50);
+ ret = _wait_for_atomic(g4x_reset_complete(pdev), 50, 0);
if (ret) {
GT_TRACE(gt, "Wait for render reset failed\n");
goto out;
@@ -788,9 +788,7 @@ int __intel_gt_reset(struct intel_gt *gt,
intel_engine_mask_t engine_mask)
reset_mask = wa_14015076503_start(gt, engine_mask, !retry);
GT_TRACE(gt, "engine_mask=%x\n", reset_mask);
- preempt_disable();
ret = reset(gt, reset_mask, retry);
- preempt_enable();
wa_14015076503_end(gt, reset_mask);
}
--
2.39.2
Re: [PATCH 04/10] drm/tegra: Set fbdev flags
Hi Javier
Am 05.07.23 um 10:34 schrieb Javier Martinez Canillas:
Thomas Zimmermann writes:
Set fbdev default flags FBNFO_DEFAULT and mark the framebuffer with
FBINFO_VIRTFB. The framebuffer range is in DMA-able memory and should
be accessed with the CPU's regular memory ops.
Signed-off-by: Thomas Zimmermann
Cc: Thierry Reding
Cc: Mikko Perttunen
---
drivers/gpu/drm/tegra/fbdev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/tegra/fbdev.c b/drivers/gpu/drm/tegra/fbdev.c
index 82577b7c88da..8074430c52f1 100644
--- a/drivers/gpu/drm/tegra/fbdev.c
+++ b/drivers/gpu/drm/tegra/fbdev.c
@@ -103,6 +103,8 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper,
return PTR_ERR(info);
}
+ info->flags = FBINFO_DEFAULT;
+
fb = tegra_fb_alloc(drm, &cmd, &bo, 1);
if (IS_ERR(fb)) {
err = PTR_ERR(fb);
@@ -132,6 +134,7 @@ static int tegra_fbdev_probe(struct drm_fb_helper *helper,
}
}
+ info->flags |= FBINFO_VIRTFB;
I see that all fbdev drivers just do: info->flags = FBINFO_FLAG_DEFAULT |
FBINFO_VIRTFB
Guess you are doing in two assignments to be consistent with drm_fbdev_dma.c ?
I was just curious about the rationale for setting the flags in two steps.
The _DEFAULT flag is really just a zero. And the other flags describe
different aspects of the framebuffer. I think it makes sense to set the
flags together with the respective state. For example, _VIRTFB is set
next to ->screen_buffer, because they belong together.
_VIRTFB is currently only used in defio code at
https://elixir.bootlin.com/linux/latest/source/drivers/video/fbdev/core/fb_defio.c#L232
I think the fbdev I/O helpers should also test this flag after all
drivers have been annotated correctly. For example, fb_io_read() would
WARN_ONCE if the _VIRTFB flag has been set; and fb_sys_read() would warn
if it hasn't been set. For the read helpers, it also makes sense to
WARN_ONCE if the _READS_FAST flag has not been set.
Best regards
Thomas
Reviewed-by: Javier Martinez Canillas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 01/10] fbdev: Add fb_ops init macros for framebuffers in DMA-able memory
Thomas Zimmermann writes: [...] >> >> The comment for I/O memory helpers says: >> >> /* >> * Initializes struct fb_ops for framebuffers in I/O memory. >> */ >> >> I think that would be good to have consistency between these two, > > Sure, I had the same thought. I think I'll rather change the existing > comments a bit. > Yes, that works for me too. Thanks! > Best regards > Thomas > > -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 01/10] fbdev: Add fb_ops init macros for framebuffers in DMA-able memory
Hi Javier Am 05.07.23 um 10:23 schrieb Javier Martinez Canillas: Thomas Zimmermann writes: Hello Thomas, Add initializer macros for struct fb_ops for framebuffers in DMA-able memory areas. Also add a corresponding Kconfig token. As of now, this is equivalent to system framebuffers and mostly useful for labeling drivers correctly. A later patch may add a generic DMA-specific mmap operation. Linux offers a number of dma_mmap_*() helpers for different use cases. Signed-off-by: Thomas Zimmermann Cc: Helge Deller --- drivers/video/fbdev/Kconfig | 8 include/linux/fb.h | 13 + 2 files changed, 21 insertions(+) diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index cecf15418632..f14229757311 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -168,6 +168,14 @@ config FB_DEFERRED_IO bool depends on FB +config FB_DMA_HELPERS + bool + depends on FB + select FB_SYS_COPYAREA + select FB_SYS_FILLRECT + select FB_SYS_FOPS + select FB_SYS_IMAGEBLIT + config FB_IO_HELPERS bool depends on FB diff --git a/include/linux/fb.h b/include/linux/fb.h index 1d5c13f34b09..1191a78c5289 100644 --- a/include/linux/fb.h +++ b/include/linux/fb.h @@ -594,6 +594,19 @@ extern ssize_t fb_sys_write(struct fb_info *info, const char __user *buf, __FB_DEFAULT_SYS_OPS_DRAW, \ __FB_DEFAULT_SYS_OPS_MMAP +/* + * Helpers for framebuffers in DMA-able memory + */ + The comment for I/O memory helpers says: /* * Initializes struct fb_ops for framebuffers in I/O memory. */ I think that would be good to have consistency between these two, Sure, I had the same thought. I think I'll rather change the existing comments a bit. Best regards Thomas so something like: /* * Initializes struct fb_ops for framebuffers in DMA-able memory. */ +#define __FB_DEFAULT_DMA_OPS_RDWR \ + .fb_read= fb_sys_read, \ + .fb_write = fb_sys_write + +#define __FB_DEFAULT_DMA_OPS_DRAW \ + .fb_fillrect= sys_fillrect, \ + .fb_copyarea= sys_copyarea, \ + .fb_imageblit = sys_imageblit + Reviewed-by: Javier Martinez Canillas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg) OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 10/10] fbdev: Remove FB_DEFAULT_SYS_OPS
Thomas Zimmermann writes: > Remove the initializer macro FB_DEFAULT_SYS_OPS and its helper macro > __FB_DEFAULT_SYS_OPS_MMAP. There are no users. > > Signed-off-by: Thomas Zimmermann > Cc: Helge Deller (maintainer:FRAMEBUFFER LAYER) > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 09/10] drm/omapdrm: Set fbdev flags
Thomas Zimmermann writes: > Set fbdev default flags FBNFO_DEFAULT and mark the framebuffer with FBINFO_DEFAULT. I noticed that the same typo is in patch 04/10 as well. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm/ttm: fix one use-after-free
I was just to complain that this is certainly incorrect.
But it's strange that ttm_mem_evict_first causes the warning in the
first place since it should never try to evict a BO which is about to be
destroyed.
Regards,
Christian.
Am 05.07.23 um 10:31 schrieb Lang Yu:
Please ignore this patch, it will cause another issue.
Will send a new one.
Regards,
Lang
On 07/05/ , Lang Yu wrote:
[ 67.399887] refcount_t: underflow; use-after-free.
[ 67.399901] WARNING: CPU: 0 PID: 3172 at lib/refcount.c:28
refcount_warn_saturate+0xc2/0x110
[ 67.400124] RIP: 0010:refcount_warn_saturate+0xc2/0x110
[ 67.400173] Call Trace:
[ 67.400176]
[ 67.400181] ttm_mem_evict_first+0x4fe/0x5b0 [ttm]
[ 67.400216] ttm_bo_mem_space+0x1e3/0x240 [ttm]
[ 67.400239] ttm_bo_validate+0xc7/0x190 [ttm]
[ 67.400253] ? ww_mutex_trylock+0x1b1/0x390
[ 67.400266] ttm_bo_init_reserved+0x183/0x1c0 [ttm]
[ 67.400280] ? __rwlock_init+0x3d/0x70
[ 67.400292] amdgpu_bo_create+0x1cd/0x4f0 [amdgpu]
[ 67.400607] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
[ 67.400980] amdgpu_bo_create_user+0x38/0x70 [amdgpu]
[ 67.401291] amdgpu_gem_object_create+0x77/0xb0 [amdgpu]
[ 67.401641] ? __pfx_amdgpu_bo_user_destroy+0x10/0x10 [amdgpu]
[ 67.401958] amdgpu_amdkfd_gpuvm_alloc_memory_of_gpu+0x228/0xa30 [amdgpu]
[ 67.402433] kfd_ioctl_alloc_memory_of_gpu+0x14e/0x390 [amdgpu]
[ 67.402824] ? lock_release+0x13f/0x290
[ 67.402838] kfd_ioctl+0x1e0/0x640 [amdgpu]
[ 67.403205] ? __pfx_kfd_ioctl_alloc_memory_of_gpu+0x10/0x10 [amdgpu]
[ 67.403579] ? tomoyo_file_ioctl+0x19/0x20
[ 67.403590] __x64_sys_ioctl+0x95/0xd0
[ 67.403601] do_syscall_64+0x3b/0x90
[ 67.403609] entry_SYSCALL_64_after_hwframe+0x72/0xdc
Fixes: 9bff18d13473 ("drm/ttm: use per BO cleanup workers")
Signed-off-by: Lang Yu
---
drivers/gpu/drm/ttm/ttm_bo.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index bd5dae4d1624..e047b191001c 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -308,6 +308,9 @@ static void ttm_bo_delayed_delete(struct work_struct *work)
bo = container_of(work, typeof(*bo), delayed_delete);
+ if (!ttm_bo_get_unless_zero(bo))
+ return;
+
dma_resv_wait_timeout(bo->base.resv, DMA_RESV_USAGE_BOOKKEEP, false,
MAX_SCHEDULE_TIMEOUT);
dma_resv_lock(bo->base.resv, NULL);
--
2.25.1
Re: [PATCH 08/10] drm/omapdrm: Use GEM mmap for fbdev emulation
Thomas Zimmermann writes: > The fbdev emulation currently uses fbdev's default mmap code, which > has been written for I/O memory. Provide an mmap that uses GEM's mmap > infrastructure. > > Utilize fine-grained fbdev macros to initialize struct fb_ops. The > macros set the read/write and the draw callbacks for DMA memory. Set > the fb_mmap callback to omapdrm's new mmap helper. Also select the > correct Kconfig token for fbdev's DMA helpers. Note that the DMA > helpers are the same as for system memory. > > Signed-off-by: Thomas Zimmermann > Cc: Tomi Valkeinen > --- Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH 07/10] drm/omapdrm: Set VM flags in GEM-object mmap function
Thomas Zimmermann writes:
> Use the mmap callback in struct drm_gem_object_funcs to set the
> VM flags. Replace a number of mmap helpers in omapdrm with their
> GEM helper counterparts. Generate DRM's file-operations instance
> with GEM's DEFINE_DRM_GEM_FOPS.
>
> Signed-off-by: Thomas Zimmermann
> Cc: Tomi Valkeinen
> ---
> +static int omap_gem_object_mmap(struct drm_gem_object *obj, struct
> vm_area_struct *vma)
> {
> struct omap_gem_object *omap_obj = to_omap_bo(obj);
>
> - vm_flags_mod(vma, VM_MIXEDMAP, VM_PFNMAP);
> + vm_flags_set(vma, VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP | VM_IO |
> VM_MIXEDMAP);
>
> if (omap_obj->flags & OMAP_BO_WC) {
> vma->vm_page_prot =
> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> @@ -563,12 +548,14 @@ int omap_gem_mmap_obj(struct drm_gem_object *obj,
>* address_space (so unmap_mapping_range does what we want,
>* in particular in the case of mmap'd dmabufs)
>*/
> - vma->vm_pgoff = 0;
> + vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
> vma_set_file(vma, obj->filp);
>
> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> }
>
> + vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
> +
> return 0;
> }
>
I think this rework deserves a more elaborated commit message.
Reviewed-by: Javier Martinez Canillas
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
