Re: drivers/gpu/drm/bridge/fsl-ldb.c:101: possible loss of information.
Hello there Laurent, >We could, but I don't think it will make any difference in practice as >the maximum pixel clock frequency supported by the SoC is 80MHz (per >LVDS channel). That would result in a 560MHz frequency returned by this >function, well below the 31 bits limit. Thanks for your explanation. I have a couple of suggestions for possible improvements: 1. Your explanatory text in a comment nearby. This helps all readers of the code. 2. Might the frequency go up to 300 MHz anytime soon ? The code will stop working then. In this case, I would suggest to put in a run time sanity check to make sure no 31 bit overflow. Just a couple of ideas for the code. Regards David Binderman
Re: [PATCH] fbdev: tgafb: Fix potential divide by zero
On Wed, 08 Mar 2023, Helge Deller wrote:
> On 3/7/23 14:08, harperchen wrote:
>> fb_set_var would by called when user invokes ioctl with cmd
>> FBIOPUT_VSCREENINFO. User-provided data would finally reach
>> tgafb_check_var. In case var->pixclock is assigned to zero,
>> divide by zero would occur when checking whether reciprocal
>> of var->pixclock is too high.
>>
>> Similar crashes have happened in other fbdev drivers. There
>> is no check and modification on var->pixclock along the call
>> chain to tgafb_check_var. We believe it could also be triggered
>> in driver tgafb from user site.
>>
>> Signed-off-by: harperchen
>
> Could you provide a real name?
> Otherwise applied to fbdev git tree.
See commit d4563201f33a ("Documentation: simplify and clarify DCO
contribution example language").
BR,
Jani.
>
> Thanks!
> Helge
>
>> ---
>> drivers/video/fbdev/tgafb.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
>> index 14d37c49633c..b44004880f0d 100644
>> --- a/drivers/video/fbdev/tgafb.c
>> +++ b/drivers/video/fbdev/tgafb.c
>> @@ -173,6 +173,9 @@ tgafb_check_var(struct fb_var_screeninfo *var, struct
>> fb_info *info)
>> {
>> struct tga_par *par = (struct tga_par *)info->par;
>>
>> +if (!var->pixclock)
>> +return -EINVAL;
>> +
>> if (par->tga_type == TGA_TYPE_8PLANE) {
>> if (var->bits_per_pixel != 8)
>> return -EINVAL;
>
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH 00/11] tree-wide: remove support for Renesas R-Car H3 ES1
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski : On Tue, 7 Mar 2023 17:30:28 +0100 you wrote: > Because H3 ES1 becomes an increasing maintenance burden and was only available > to a development group, we decided to remove upstream support for it. Here are > the patches to remove driver changes. Review tags have been gathered before > during an internal discussion. Only change since the internal version is a > plain rebase to v6.3-rc1. A branch with all removals is here: > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git > renesas/h3es1-removal > > [...] Here is the summary with links: - [07/11] ravb: remove R-Car H3 ES1.* handling https://git.kernel.org/netdev/net-next/c/6bf0ad7f2917 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Re: [PATCH v2 3/7] drm/ttm: Use the BIT macro for the TTM_TT_FLAGs
Hi, Christian,
Thanks for reviewing these.
Ack to merge reviewed patches through drm-misc-next?
Thanks,
Thomas
On 3/8/23 09:49, Christian König wrote:
Am 07.03.23 um 15:46 schrieb Thomas Hellström:
New code is recommended to use the BIT macro instead of the explicit
shifts. Change the older defines so that we can keep the style
consistent
with upcoming changes.
v2:
- Also change the value of the _PRIV_POPULATED bit (Christian König)
Signed-off-by: Thomas Hellström
Reviewed-by: Christian König
---
include/drm/ttm/ttm_tt.h | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
index b7d3f3843f1e..977ca195a536 100644
--- a/include/drm/ttm/ttm_tt.h
+++ b/include/drm/ttm/ttm_tt.h
@@ -83,12 +83,12 @@ struct ttm_tt {
* set by TTM after ttm_tt_populate() has successfully
returned, and is
* then unset when TTM calls ttm_tt_unpopulate().
*/
-#define TTM_TT_FLAG_SWAPPED (1 << 0)
-#define TTM_TT_FLAG_ZERO_ALLOC (1 << 1)
-#define TTM_TT_FLAG_EXTERNAL (1 << 2)
-#define TTM_TT_FLAG_EXTERNAL_MAPPABLE (1 << 3)
+#define TTM_TT_FLAG_SWAPPED BIT(0)
+#define TTM_TT_FLAG_ZERO_ALLOC BIT(1)
+#define TTM_TT_FLAG_EXTERNAL BIT(2)
+#define TTM_TT_FLAG_EXTERNAL_MAPPABLE BIT(3)
-#define TTM_TT_FLAG_PRIV_POPULATED (1U << 31)
+#define TTM_TT_FLAG_PRIV_POPULATED BIT(4)
uint32_t page_flags;
/** @num_pages: Number of pages in the page array. */
uint32_t num_pages;
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 09/03/2023 05.14, Christian König wrote: >> I think you mean wake_up_interruptible(). That would be >> drm_sched_job_done(), on the fence callback when a job completes, which >> as I keep saying is the same logic used for >> hw_rq_count/hw_submission_limit tracking. > > As the documentation to wait_event says: > > * wake_up() has to be called after changing any variable that could > * change the result of the wait condition. > > So what you essentially try to do here is to skip that and say > drm_sched_job_done() would call that anyway, but when you read any > variable to determine that state then as far as I can see nothing is > guarantying that order. The driver needs to guarantee that any changes to that state precede a job completion fence signal of course, that's the entire idea of the API. It's supposed to represent a check for per-scheduler (or more specific, but not more global) resources that are released on job completion. Of course if you misuse the API you could cause a problem, but what I'm trying to say is that the API as designed and when used as intended does work properly. Put another way: job completions always need to cause the sched main loop to run an iteration anyway (otherwise we wouldn't make forward progress), and job completions are exactly the signal that the can_run_job() condition may have changed. > The only other possibility how you could use the callback correctly > would be to call drm_fence_is_signaled() to query the state of your hw > submission from the same fence which is then signaled. But then the > question is once more why you don't give that fence directly to the > scheduler? But the driver is supposed to guarantee that the ordering is always 1. resources freed, 2. fence signaled. So you don't need to check for the fence, you can just check for the resource state. If the callback returns false then by definition the fence wasn't yet signaled at some point during its execution (because the resources weren't yet freed), and since it would be in the wait_event_interruptible() check path, by definition the fence signaling at any point during or after the check would cause the thread to wake up again and re-check. Thread 1 Thread 2 1. wait_event_interruptible() arms wq 1. Free resources 2. can_run_job() checks resources 2. Signal fence 3. wait_event_interruptible() sleeps on wq3. Fence wakes up wq 4. loop There is no possible interleaving of those sequences that leads to a lost event and the thread not waking up: - If T2.3 happens before T1.1, that means T2.1 happened earlier and T1.2 must return true. - If T2.3 happens after T1.1 but before T1.3, the wq code will ensure the wq does not sleep (or immediately wakes up) at T1.3 since it was signaled during the condition check, after the wq was armed. At the next check loop, T1.2 will then return true, since T2.1 already happened before T2.3. - If T2.3 happens during T1.3, the wq wakes up normally and does another check, and at that point T1.2 returns true. QED. ~~ Lina
[PATCH v3 6/7] drm/i915/vdsc: Check slice design requirement
Add function to check if slice design requirements are being
met as defined in Bspec: 49259 in the section
Slice Design Requirement
--v7
-remove full bspec link [Jani]
-rename intel_dsc_check_slice_design_req to
intel_dsc_slice_dimensions_valid [Jani]
--v8
-fix condition to check if slice width and height are
of two
-fix minimum pixel in slice condition
--v10
-condition should be < rather then >= [Uma]
Cc: Uma Shankar
Signed-off-by: Suraj Kandpal
Reviewed-by: Uma Shankar
---
drivers/gpu/drm/i915/display/intel_vdsc.c | 32 +++
1 file changed, 32 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 0388efb49b92..8e787c13d26d 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -448,6 +448,29 @@ calculate_rc_params(struct rc_parameters *rc,
}
}
+static int intel_dsc_slice_dimensions_valid(struct intel_crtc_state
*pipe_config,
+ struct drm_dsc_config *vdsc_cfg)
+{
+ if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_RGB ||
+ pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
+ if (vdsc_cfg->slice_height > 4095)
+ return -EINVAL;
+ if (vdsc_cfg->slice_height * vdsc_cfg->slice_width < 15000)
+ return -EINVAL;
+ } else if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420) {
+ if (vdsc_cfg->slice_width % 2)
+ return -EINVAL;
+ if (vdsc_cfg->slice_height % 2)
+ return -EINVAL;
+ if (vdsc_cfg->slice_height > 4094)
+ return -EINVAL;
+ if (vdsc_cfg->slice_height * vdsc_cfg->slice_width < 3)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
int intel_dsc_compute_params(struct intel_crtc_state *pipe_config)
{
struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
@@ -456,11 +479,20 @@ int intel_dsc_compute_params(struct intel_crtc_state
*pipe_config)
u16 compressed_bpp = pipe_config->dsc.compressed_bpp;
const struct rc_parameters *rc_params;
struct rc_parameters *rc = NULL;
+ int err;
u8 i = 0;
vdsc_cfg->pic_width = pipe_config->hw.adjusted_mode.crtc_hdisplay;
vdsc_cfg->slice_width = DIV_ROUND_UP(vdsc_cfg->pic_width,
pipe_config->dsc.slice_count);
+
+ err = intel_dsc_slice_dimensions_valid(pipe_config, vdsc_cfg);
+
+ if (err) {
+ drm_dbg_kms(_priv->drm, "Slice dimension requirements not
met\n");
+ return err;
+ }
+
/*
* According to DSC 1.2 specs if colorspace is YCbCr then convert_rgb
is 0
* else 1
--
2.25.1
[PATCH v3 7/7] drm/i915/dsc: Add debugfs entry to validate DSC output formats
From: Swati Sharma
DSC_Output_Format_Sink_Support entry is added to i915_dsc_fec_support_show
to depict if sink supports DSC output formats (RGB/YCbCr420/YCbCr444).
Also, new debugfs entry is created to enforce output format. This is
required because of our driver policy. For ex. if a mode is supported
in both RGB and YCbCr420 output formats by the sink, our policy is to
try RGB first and fall back to YCbCr420, if mode cannot be shown
using RGB. So, to test other output formats like YCbCr420 or YCbCr444,
we need a debugfs entry (force_dsc_output_format) to force this
output format.
v2: -Func name changed to intel_output_format_name() (Jani N)
-Return forced o/p format from intel_dp_output_format() (Jani N)
v3: -output_format_str[] to remain static (Jani N)
Signed-off-by: Swati Sharma
Reviewed-by: Uma Shankar
---
.../drm/i915/display/intel_crtc_state_dump.c | 4 +-
.../drm/i915/display/intel_crtc_state_dump.h | 2 +
.../drm/i915/display/intel_display_debugfs.c | 78 +++
.../drm/i915/display/intel_display_types.h| 1 +
drivers/gpu/drm/i915/display/intel_dp.c | 4 +
5 files changed, 87 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
index 766633566fd6..54c8adc0702e 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
@@ -123,7 +123,7 @@ static const char * const output_format_str[] = {
[INTEL_OUTPUT_FORMAT_YCBCR444] = "YCBCR4:4:4",
};
-static const char *output_formats(enum intel_output_format format)
+const char *intel_output_format_name(enum intel_output_format format)
{
if (format >= ARRAY_SIZE(output_format_str))
return "invalid";
@@ -181,7 +181,7 @@ void intel_crtc_state_dump(const struct intel_crtc_state
*pipe_config,
"active: %s, output_types: %s (0x%x), output format: %s\n",
str_yes_no(pipe_config->hw.active),
buf, pipe_config->output_types,
- output_formats(pipe_config->output_format));
+ intel_output_format_name(pipe_config->output_format));
drm_dbg_kms(>drm,
"cpu_transcoder: %s, pipe bpp: %i, dithering: %i\n",
diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.h
b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.h
index 9399c35b7e5e..780f3f1190d7 100644
--- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.h
+++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.h
@@ -8,9 +8,11 @@
struct intel_crtc_state;
struct intel_atomic_state;
+enum intel_output_format;
void intel_crtc_state_dump(const struct intel_crtc_state *crtc_state,
struct intel_atomic_state *state,
const char *context);
+const char *intel_output_format_name(enum intel_output_format format);
#endif /* __INTEL_CRTC_STATE_H__ */
diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
index 1e654ddd0815..fc2905574e5b 100644
--- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
+++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
@@ -12,6 +12,7 @@
#include "i915_irq.h"
#include "i915_reg.h"
#include "intel_de.h"
+#include "intel_crtc_state_dump.h"
#include "intel_display_debugfs.h"
#include "intel_display_power.h"
#include "intel_display_power_well.h"
@@ -1535,6 +1536,13 @@ static int i915_dsc_fec_support_show(struct seq_file *m,
void *data)
str_yes_no(crtc_state->dsc.compression_enable));
seq_printf(m, "DSC_Sink_Support: %s\n",
str_yes_no(drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)));
+ seq_printf(m, "DSC_Output_Format_Sink_Support: RGB: %s
YCBCR420: %s YCBCR444: %s\n",
+
str_yes_no(drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd,
+
DP_DSC_RGB)),
+
str_yes_no(drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd,
+
DP_DSC_YCbCr420_Native)),
+
str_yes_no(drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd,
+
DP_DSC_YCbCr444)));
seq_printf(m, "Force_DSC_Enable: %s\n",
str_yes_no(intel_dp->force_dsc_en));
if (!intel_dp_is_edp(intel_dp))
@@ -1660,6 +1668,73 @@ static const struct file_operations i915_dsc_bpc_fops = {
.write = i915_dsc_bpc_write
};
+static int i915_dsc_output_format_show(struct seq_file *m, void *data)
+{
+ struct drm_connector *connector = m->private;
+ struct drm_device *dev = connector->dev;
+ struct drm_crtc *crtc;
+ struct
[PATCH v3 4/7] drm/i915/dsc: Enable YCbCr420 for VDSC
Implementation of VDSC for YCbCr420.
Add QP tables for 8,10,12 BPC from rc_tables.h in intel_qp_tables.c
(Derived from C-Model, which is given along with DSC1.2a Spec from Vesa)
intel_lookup_range_min/max_qp functons need to take into account the
output format. Based on that appropriate qp table need to be chosen.
Other rc_parameters need to be set where currently values for 444 format
is hardcoded in calculate_rc_parameters( ).
vdsc_cfg struct needs to be filled with output format information, where
these are hardcoded for 444 format.
Bspec: 49259
Signed-off-by: Suraj Kandpal
Reviewed-by: Vandita Kulkarni
Reviewed-by: Uma Shankar
---
.../gpu/drm/i915/display/intel_qp_tables.c| 187 --
.../gpu/drm/i915/display/intel_qp_tables.h| 4 +-
drivers/gpu/drm/i915/display/intel_vdsc.c | 4 +-
3 files changed, 180 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_qp_tables.c
b/drivers/gpu/drm/i915/display/intel_qp_tables.c
index 6f8e4ec5c0fb..6e86c0971d24 100644
--- a/drivers/gpu/drm/i915/display/intel_qp_tables.c
+++ b/drivers/gpu/drm/i915/display/intel_qp_tables.c
@@ -17,6 +17,15 @@
/* from BPP 6 to 36 in steps of 0.5 */
#define RC_RANGE_QP444_12BPC_MAX_NUM_BPP 61
+/* from BPP 6 to 24 in steps of 0.5 */
+#define RC_RANGE_QP420_8BPC_MAX_NUM_BPP17
+
+/* from BPP 6 to 30 in steps of 0.5 */
+#define RC_RANGE_QP420_10BPC_MAX_NUM_BPP 23
+
+/* from BPP 6 to 36 in steps of 0.5 */
+#define RC_RANGE_QP420_12BPC_MAX_NUM_BPP 29
+
/*
* These qp tables are as per the C model
* and it has the rows pointing to bpps which increment
@@ -283,26 +292,182 @@ static const u8
rc_range_maxqp444_12bpc[DSC_NUM_BUF_RANGES][RC_RANGE_QP444_12BPC
11, 11, 10, 10, 10, 10, 10, 9, 9, 8, 8, 8, 8, 8, 7, 7, 6, 6, 6, 6, 5,
5, 4 }
};
-#define PARAM_TABLE(_minmax, _bpc, _row, _col) do { \
- if (bpc == (_bpc)) \
- return rc_range_##_minmax##qp444_##_bpc##bpc[_row][_col]; \
+static const u8
rc_range_minqp420_8bpc[DSC_NUM_BUF_RANGES][RC_RANGE_QP420_8BPC_MAX_NUM_BPP] = {
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 3, 3, 3, 3, 3, 2, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0 },
+ { 3, 3, 3, 3, 3, 2, 2, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0 },
+ { 3, 3, 3, 3, 3, 3, 2, 2, 1, 1, 1, 1, 1, 1, 1, 0, 0 },
+ { 3, 3, 3, 3, 3, 3, 2, 2, 2, 2, 2, 2, 2, 1, 1, 1, 0 },
+ { 3, 3, 3, 3, 3, 3, 2, 2, 2, 2, 2, 2, 2, 2, 1, 1, 0 },
+ { 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 3, 2, 2, 1, 1 },
+ { 5, 5, 5, 5, 5, 4, 4, 4, 4, 4, 3, 3, 3, 3, 2, 1, 1 },
+ { 5, 5, 5, 5, 5, 5, 5, 4, 4, 4, 4, 4, 4, 3, 2, 2, 1 },
+ { 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 4, 4, 3, 3, 2, 1 },
+ { 9, 8, 8, 7, 7, 7, 7, 7, 7, 6, 5, 5, 4, 3, 3, 3, 2 },
+ { 13, 12, 12, 11, 10, 10, 9, 8, 8, 7, 6, 6, 5, 5, 4, 4, 3 }
+};
+
+static const u8
rc_range_maxqp420_8bpc[DSC_NUM_BUF_RANGES][RC_RANGE_QP420_8BPC_MAX_NUM_BPP] = {
+ { 4, 4, 3, 3, 2, 2, 1, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 4, 4, 4, 4, 4, 3, 2, 2, 1, 1, 1, 1, 0, 0, 0, 0, 0 },
+ { 5, 5, 5, 5, 5, 4, 3, 2, 1, 1, 1, 1, 1, 1, 0, 0, 0 },
+ { 6, 6, 6, 6, 6, 5, 4, 3, 2, 2, 2, 1, 1, 1, 1, 0, 0 },
+ { 7, 7, 7, 7, 7, 5, 4, 3, 2, 2, 2, 2, 2, 1, 1, 1, 0 },
+ { 7, 7, 7, 7, 7, 6, 5, 4, 3, 3, 3, 2, 2, 2, 1, 1, 0 },
+ { 7, 7, 7, 7, 7, 6, 5, 4, 3, 3, 3, 3, 2, 2, 2, 1, 1 },
+ { 8, 8, 8, 8, 8, 7, 6, 5, 4, 4, 4, 3, 3, 2, 2, 2, 1 },
+ { 9, 9, 9, 8, 8, 7, 6, 6, 5, 5, 4, 4, 3, 3, 2, 2, 1 },
+ { 10, 10, 9, 9, 9, 8, 7, 6, 5, 5, 5, 4, 4, 3, 3, 2, 2 },
+ { 10, 10, 10, 9, 9, 8, 8, 7, 6, 6, 5, 5, 4, 4, 3, 2, 2 },
+ { 11, 11, 10, 10, 9, 9, 8, 7, 7, 6, 6, 5, 5, 4, 3, 3, 2 },
+ { 11, 11, 11, 10, 9, 9, 9, 8, 7, 7, 6, 5, 5, 4, 4, 3, 2 },
+ { 13, 12, 12, 11, 10, 10, 9, 8, 8, 7, 6, 6, 5, 4, 4, 4, 3 },
+ { 14, 13, 13, 12, 11, 11, 10, 9, 9, 8, 7, 7, 6, 6, 5, 5, 4 }
+};
+
+static const u8
rc_range_minqp420_10bpc[DSC_NUM_BUF_RANGES][RC_RANGE_QP420_10BPC_MAX_NUM_BPP] =
{
+ { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 4, 4, 4, 3, 2, 2, 2, 2, 2, 2, 2, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 4, 4, 4, 3, 3, 3, 3, 3, 3, 2, 2, 2, 2, 2, 1, 0, 0, 0, 0, 0, 0, 0, 0 },
+ { 5, 5, 5, 4, 4, 4, 4, 4, 4, 3, 3, 2, 2, 2, 2, 1, 0, 0, 0, 0, 0, 0, 0 },
+ { 7, 7, 7, 6, 6, 5, 5, 4, 4, 3, 3, 3, 3, 2, 2, 2, 1, 1, 1, 0, 0, 0, 0 },
+ { 7, 7, 7, 7, 7, 6, 5, 5, 5, 5, 5, 4, 3, 3, 2, 2, 1, 1, 1, 1, 1, 0, 0 },
+ { 7, 7, 7, 7, 7, 6, 6, 5, 5, 5, 5, 4, 4, 4, 3, 2, 2, 2, 2, 1, 1, 1, 0 },
+ { 7, 7, 7, 7, 7, 7, 6, 6, 6, 6, 6, 5, 4, 4, 4, 3, 2, 2, 2, 1, 1, 1, 0 },
+ { 7, 7, 7, 7, 7, 7, 7, 7, 6, 6, 6, 6, 5, 5, 4, 4, 3, 3, 2, 2, 2, 1, 1 },
+ { 7, 7, 7, 7, 7, 7, 7, 7, 7,
[PATCH v3 5/7] drm/i915/dsc: Fill in native_420 field
Now that we have laid the groundwork for YUV420 Enablement
we fill up native_420 field in vdsc_cfg and add appropriate
checks wherever required.
---v2
-adding native_422 field as 0 [Vandita]
-filling in second_line_bpg_offset, second_line_offset_adj
and nsl_bpg_offset in vds_cfg when native_420 is true
---v3
-adding display version check to solve igt issue
--v7
-remove is_pipe_dsc check as its always true for D14 [Jani]
--v10
-keep sink capability check [Jani]
-move from !(x == y || w == z) to x !=y && w != z [Jani]
--v11
-avoid native_420 computation if not gen14 [Uma]
--v12
-fix state mismatch issue of compressed_bpp
Cc: Uma Shankar
Cc: Jani Nikula
Signed-off-by: Suraj Kandpal
Reviewed-by: Uma Shankar
---
drivers/gpu/drm/i915/display/icl_dsi.c| 2 -
drivers/gpu/drm/i915/display/intel_dp.c | 16 +++-
drivers/gpu/drm/i915/display/intel_vdsc.c | 98 ---
3 files changed, 100 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
b/drivers/gpu/drm/i915/display/icl_dsi.c
index 50dcaa895854..dde0269a2778 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -1552,8 +1552,6 @@ static int gen11_dsi_dsc_compute_config(struct
intel_encoder *encoder,
if (crtc_state->dsc.slice_count > 1)
crtc_state->dsc.dsc_split = true;
- vdsc_cfg->convert_rgb = true;
-
/* FIXME: initialize from VBT */
vdsc_cfg->rc_model_size = DSC_RC_MODEL_SIZE_CONST;
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index c725b40e2718..b40bb5fd9abb 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1467,9 +1467,10 @@ static int intel_dp_dsc_compute_params(struct
intel_encoder *encoder,
vdsc_cfg->dsc_version_minor =
min(intel_dp_source_dsc_version_minor(intel_dp),
intel_dp_sink_dsc_version_minor(intel_dp));
-
- vdsc_cfg->convert_rgb = intel_dp->dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP
- DP_DSC_SUPPORT] &
- DP_DSC_RGB;
+ if (vdsc_cfg->convert_rgb)
+ vdsc_cfg->convert_rgb =
+ intel_dp->dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP -
DP_DSC_SUPPORT] &
+ DP_DSC_RGB;
line_buf_depth = drm_dp_dsc_sink_line_buf_depth(intel_dp->dsc_dpcd);
if (!line_buf_depth) {
@@ -1587,6 +1588,15 @@ int intel_dp_dsc_compute_config(struct intel_dp
*intel_dp,
pipe_config->bigjoiner_pipes,
pipe_bpp,
timeslots);
+ /*
+* According to DSC 1.2a Section 4.1.1 Table 4.1 the
maximum
+* supported PPS value can be 63.9375 and with the
further
+* mention that bpp should be programmed double the
target bpp
+* restricting our target bpp to be 31.9375 at max
+*/
+ if (pipe_config->output_format ==
INTEL_OUTPUT_FORMAT_YCBCR420)
+ dsc_max_output_bpp = min_t(u16,
dsc_max_output_bpp, 31 << 4);
+
if (!dsc_max_output_bpp) {
drm_dbg_kms(_priv->drm,
"Compressed BPP not supported\n");
diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c
b/drivers/gpu/drm/i915/display/intel_vdsc.c
index 6fa70f3c074b..0388efb49b92 100644
--- a/drivers/gpu/drm/i915/display/intel_vdsc.c
+++ b/drivers/gpu/drm/i915/display/intel_vdsc.c
@@ -461,14 +461,50 @@ int intel_dsc_compute_params(struct intel_crtc_state
*pipe_config)
vdsc_cfg->pic_width = pipe_config->hw.adjusted_mode.crtc_hdisplay;
vdsc_cfg->slice_width = DIV_ROUND_UP(vdsc_cfg->pic_width,
pipe_config->dsc.slice_count);
-
- /* Gen 11 does not support YCbCr */
+ /*
+* According to DSC 1.2 specs if colorspace is YCbCr then convert_rgb
is 0
+* else 1
+*/
+ vdsc_cfg->convert_rgb = pipe_config->output_format !=
INTEL_OUTPUT_FORMAT_YCBCR420 &&
+ pipe_config->output_format !=
INTEL_OUTPUT_FORMAT_YCBCR444;
+
+ if (DISPLAY_VER(dev_priv) >= 14 &&
+ pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
+ vdsc_cfg->native_420 = true;
+ /* We do not support YcBCr422 as of now */
+ vdsc_cfg->native_422 = false;
vdsc_cfg->simple_422 = false;
/* Gen 11 does not support VBR */
vdsc_cfg->vbr_enable = false;
/* Gen 11 only supports integral values of bpp */
vdsc_cfg->bits_per_pixel = compressed_bpp << 4;
+
+ /*
+* According to DSC 1.2 specs in Section 4.1 if native_420 is set:
[PATCH v3 3/7] drm/i915/dsc: Adding the new registers for DSC
Adding new DSC register which are introducted MTL onwards Signed-off-by: Suraj Kandpal Reviewed-by: Vandita Kulkarni Reviewed-by: Uma Shankar --- .../gpu/drm/i915/display/intel_vdsc_regs.h| 28 +++ 1 file changed, 28 insertions(+) diff --git a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h index 4fd883463752..b71f00b5c761 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc_regs.h +++ b/drivers/gpu/drm/i915/display/intel_vdsc_regs.h @@ -46,6 +46,32 @@ _ICL_PIPE_DSS_CTL2_PB, \ _ICL_PIPE_DSS_CTL2_PC) +/* MTL Display Stream Compression registers */ +#define _MTL_DSC0_PICTURE_PARAMETER_SET_17_PB 0x782B4 +#define _MTL_DSC1_PICTURE_PARAMETER_SET_17_PB 0x783B4 +#define _MTL_DSC0_PICTURE_PARAMETER_SET_17_PC 0x784B4 +#define _MTL_DSC1_PICTURE_PARAMETER_SET_17_PC 0x785B4 +#define MTL_DSC0_PICTURE_PARAMETER_SET_17(pipe)_MMIO_PIPE((pipe) - PIPE_B, \ + _MTL_DSC0_PICTURE_PARAMETER_SET_17_PB, \ + _MTL_DSC0_PICTURE_PARAMETER_SET_17_PC) +#define MTL_DSC1_PICTURE_PARAMETER_SET_17(pipe)_MMIO_PIPE((pipe) - PIPE_B, \ + _MTL_DSC1_PICTURE_PARAMETER_SET_17_PB, \ + _MTL_DSC1_PICTURE_PARAMETER_SET_17_PC) +#define DSC_SL_BPG_OFFSET(offset) ((offset) << 27) + +#define _MTL_DSC0_PICTURE_PARAMETER_SET_18_PB 0x782B8 +#define _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB 0x783B8 +#define _MTL_DSC0_PICTURE_PARAMETER_SET_18_PC 0x784B8 +#define _MTL_DSC1_PICTURE_PARAMETER_SET_18_PC 0x785B8 +#define MTL_DSC0_PICTURE_PARAMETER_SET_18(pipe)_MMIO_PIPE((pipe) - PIPE_B, \ + _MTL_DSC0_PICTURE_PARAMETER_SET_18_PB, \ + _MTL_DSC0_PICTURE_PARAMETER_SET_18_PC) +#define MTL_DSC1_PICTURE_PARAMETER_SET_18(pipe)_MMIO_PIPE((pipe) - PIPE_B, \ + _MTL_DSC1_PICTURE_PARAMETER_SET_18_PB, \ + _MTL_DSC1_PICTURE_PARAMETER_SET_18_PC) +#define DSC_NSL_BPG_OFFSET(offset) ((offset) << 16) +#define DSC_SL_OFFSET_ADJ(offset) ((offset) << 0) + /* Icelake Display Stream Compression Registers */ #define DSCA_PICTURE_PARAMETER_SET_0 _MMIO(0x6B200) #define DSCC_PICTURE_PARAMETER_SET_0 _MMIO(0x6BA00) @@ -59,6 +85,8 @@ #define ICL_DSC1_PICTURE_PARAMETER_SET_0(pipe) _MMIO_PIPE((pipe) - PIPE_B, \ _ICL_DSC1_PICTURE_PARAMETER_SET_0_PB, \ _ICL_DSC1_PICTURE_PARAMETER_SET_0_PC) +#define DSC_NATIVE_422_ENABLE BIT(23) +#define DSC_NATIVE_420_ENABLE BIT(22) #define DSC_ALT_ICH_SEL (1 << 20) #define DSC_VBR_ENABLE(1 << 19) #define DSC_422_ENABLE(1 << 18) -- 2.25.1
[PATCH v3 2/7] drm/i915/dp: Check if DSC supports the given output_format
From: Ankit Nautiyal
Go with DSC only if the given output_format is supported.
v2: Use drm helper to get DSC format support for sink.
v3: remove drm_dp_dsc_compute_bpp.
Cc: Uma Shankar
Signed-off-by: Ankit Nautiyal
Reviewed-by: Uma Shankar
---
drivers/gpu/drm/i915/display/intel_dp.c | 28 +
1 file changed, 28 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index aee93b0d810e..c725b40e2718 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1492,6 +1492,31 @@ static int intel_dp_dsc_compute_params(struct
intel_encoder *encoder,
return drm_dsc_compute_rc_parameters(vdsc_cfg);
}
+static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
+enum intel_output_format output_format)
+{
+ u8 sink_dsc_format;
+
+ switch (output_format) {
+ case INTEL_OUTPUT_FORMAT_RGB:
+ sink_dsc_format = DP_DSC_RGB;
+ break;
+ case INTEL_OUTPUT_FORMAT_YCBCR444:
+ sink_dsc_format = DP_DSC_YCbCr444;
+ break;
+ case INTEL_OUTPUT_FORMAT_YCBCR420:
+ if (min(intel_dp_source_dsc_version_minor(intel_dp),
+ intel_dp_sink_dsc_version_minor(intel_dp)) < 2)
+ return false;
+ sink_dsc_format = DP_DSC_YCbCr420_Native;
+ break;
+ default:
+ return false;
+ }
+
+ return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd,
sink_dsc_format);
+}
+
int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
struct intel_crtc_state *pipe_config,
struct drm_connector_state *conn_state,
@@ -1512,6 +1537,9 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
if (!intel_dp_supports_dsc(intel_dp, pipe_config))
return -EINVAL;
+ if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
+ return -EINVAL;
+
if (compute_pipe_bpp)
pipe_bpp = intel_dp_dsc_compute_bpp(intel_dp,
conn_state->max_requested_bpc);
else
--
2.25.1
[PATCH v3 1/7] drm/dp_helper: Add helper to check DSC support with given o/p format
From: Ankit Nautiyal
Add helper to check if the DP sink supports DSC with the given
o/p format.
v2: Add documentation for the helper. (Uma Shankar)
v3: /** instead of /* (Uma Shankar)
Signed-off-by: Ankit Nautiyal
Reviewed-by: Uma Shankar
---
include/drm/display/drm_dp_helper.h | 13 +
1 file changed, 13 insertions(+)
diff --git a/include/drm/display/drm_dp_helper.h
b/include/drm/display/drm_dp_helper.h
index ab55453f2d2c..533d3ee7fe05 100644
--- a/include/drm/display/drm_dp_helper.h
+++ b/include/drm/display/drm_dp_helper.h
@@ -194,6 +194,19 @@ drm_dp_dsc_sink_max_slice_width(const u8
dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
DP_DSC_SLICE_WIDTH_MULTIPLIER;
}
+/**
+ * drm_dp_dsc_sink_supports_format() - check if sink supports DSC with given
output format
+ * @dsc_dpcd : DSC-capability DPCDs of the sink
+ * @output_format: output_format which is to be checked
+ *
+ * Returns true if the sink supports DSC with the given output_format, false
otherwise.
+ */
+static inline bool
+drm_dp_dsc_sink_supports_format(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE],
u8 output_format)
+{
+ return dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - DP_DSC_SUPPORT] &
output_format;
+}
+
/* Forward Error Correction Support on DP 1.4 */
static inline bool
drm_dp_sink_supports_fec(const u8 fec_capable)
--
2.25.1
[PATCH v3 0/7] Enable YCbCr420 format for VDSC
This patch series aims to enable the YCbCr420 format for DSC. Changes are mostly compute params related for hdmi,dp and dsi along with the addition of new rc_tables for native_420 and corresponding changes to macros used to fetch them. There have been discussions prior to this series in which some patches have gotten rb and can be found in the below link https://patchwork.freedesktop.org/series/113729 Ankit Nautiyal (2): drm/dp_helper: Add helper to check DSC support with given o/p format drm/i915/dp: Check if DSC supports the given output_format Suraj Kandpal (4): drm/i915/dsc: Adding the new registers for DSC drm/i915/dsc: Enable YCbCr420 for VDSC drm/i915/dsc: Fill in native_420 field drm/i915/vdsc: Check slice design requirement Swati Sharma (1): drm/i915/dsc: Add debugfs entry to validate DSC output formats drivers/gpu/drm/i915/display/icl_dsi.c| 2 - .../drm/i915/display/intel_crtc_state_dump.c | 4 +- .../drm/i915/display/intel_crtc_state_dump.h | 2 + .../drm/i915/display/intel_display_debugfs.c | 78 .../drm/i915/display/intel_display_types.h| 1 + drivers/gpu/drm/i915/display/intel_dp.c | 48 - .../gpu/drm/i915/display/intel_qp_tables.c| 187 -- .../gpu/drm/i915/display/intel_qp_tables.h| 4 +- drivers/gpu/drm/i915/display/intel_vdsc.c | 132 +++-- .../gpu/drm/i915/display/intel_vdsc_regs.h| 28 +++ include/drm/display/drm_dp_helper.h | 13 ++ 11 files changed, 467 insertions(+), 32 deletions(-) -- 2.25.1
[Bug 216625] [regression] GPU lockup on Radeon R7 Kaveri
https://bugzilla.kernel.org/show_bug.cgi?id=216625 --- Comment #10 from Pierre Ossman (pierre-bugzi...@ossman.eu) --- I finally got that old version of mesa to build. Unfortunately, the hangs still happen even with that. :/ > Mar 09 07:18:30 kernel: radeon :00:01.0: ring 3 stalled for more than > 10028msec > Mar 09 07:18:30 kernel: radeon :00:01.0: GPU lockup (current fence id > 0xfa91 last fence id 0xfabc on ring 3) > Mar 09 07:18:31 kernel: radeon :00:01.0: ring 5 stalled for more than > 10077msec > Mar 09 07:18:31 kernel: radeon :00:01.0: GPU lockup (current fence id > 0x18fb last fence id 0x18fe on ring 5) > Mar 09 07:18:31 kernel: radeon :00:01.0: ring 0 stalled for more than > 10202msec > ... What can we do next to pinpoint this? It seems to fail rather reliably after a suspend/resume. Is there some test suite I can run to provoke things? -- 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 RFC 01/18] rust: drm: ioctl: Add DRM ioctl abstraction
On Thu, 9 Mar 2023 at 15:32, Asahi Lina wrote:
>
> On 08/03/2023 00.32, Maíra Canal wrote:
> > On 3/7/23 11:25, Asahi Lina wrote:
> >> DRM drivers need to be able to declare which driver-specific ioctls they
> >> support. This abstraction adds the required types and a helper macro to
> >> generate the ioctl definition inside the DRM driver.
> >>
> >> Note that this macro is not usable until further bits of the
> >> abstraction are in place (but it will not fail to compile on its own, if
> >> not called).
> >>
> >> Signed-off-by: Asahi Lina
> >> ---
> >> drivers/gpu/drm/Kconfig | 7 ++
> >> rust/bindings/bindings_helper.h | 2 +
> >> rust/kernel/drm/ioctl.rs| 147
> >>
> >> rust/kernel/drm/mod.rs | 5 ++
> >> rust/kernel/lib.rs | 2 +
> >> 5 files changed, 163 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> >> index dc0f94f02a82..dab8f0f9aa96 100644
> >> --- a/drivers/gpu/drm/Kconfig
> >> +++ b/drivers/gpu/drm/Kconfig
> >> @@ -27,6 +27,13 @@ menuconfig DRM
> >>details. You should also select and configure AGP
> >>(/dev/agpgart) support if it is available for your platform.
> >>
> >
> > [...]
> >
> >> +
> >> +/// Declare the DRM ioctls for a driver.
> >> +///
> >> +/// Each entry in the list should have the form:
> >> +///
> >> +/// `(ioctl_number, argument_type, flags, user_callback),`
> >> +///
> >> +/// `argument_type` is the type name within the `bindings` crate.
> >> +/// `user_callback` should have the following prototype:
> >> +///
> >> +/// ```
> >> +/// fn foo(device: ::drm::device::Device,
> >> +///data: bindings::argument_type,
> >> +///file: ::drm::file::File,
> >> +/// )
> >> +/// ```
> >> +/// where `Self` is the drm::drv::Driver implementation these ioctls are
> >> being declared within.
> >> +///
> >> +/// # Examples
> >> +///
> >> +/// ```
> >> +/// kernel::declare_drm_ioctls! {
> >> +/// (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW,
> >> my_get_param_handler),
> >> +/// }
> >> +/// ```
> >> +///
> >> +#[macro_export]
> >> +macro_rules! declare_drm_ioctls {
> >> +( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? )
> >> => {
> >> +const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] =
> >> {
> >> +const _:() = {
> >> +let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
> >> +// Assert that all the IOCTLs are in the right order and
> >> there are no gaps,
> >> +// and that the sizeof of the specified type is correct.
> >
> > I believe that not necessarily the IOCTLs need to be in the right order and
> > with no gaps. For example, armada_drm.h has a gap in between 0x00 and
> > 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and
> > virtgpu, start their IOCTLs with 0x01.
>
> Yeah, we talked about this a bit... do you have any ideas about how to
> design this? I think it should be possible with a const function
> initializing an array entry by entry, we just need a two-pass macro
> (once to determine the max ioctl number, then again to actually output
> the implementation).
>
> I'm not sure why drivers would have gaps in the ioctl numbers though...
> my idea was that new drivers shouldn't need that as far as I can tell
> (you can't remove APIs after the fact due to UAPI stability guarantees,
> so as long as you don't have gaps to begin with...). But I guess if
> we're reimplementing existing drivers in Rust we'll need this... though
> maybe it makes sense to just say it's not supported and require
> reimplementations that have holes to just explicitly add dummy ioctls
> that return EINVAL? We could even provide such a dummy generic ioctl
> handler on the abstraction side, so drivers just have to add it to the
> list, or make the macro take a special token that is used for
> placeholder ioctls that don't exist (which then creates the NULL
> function pointer that the drm core interprets as invalid)...
I can think of two reason for gaps having appeared:
a) developers wanted to group new uapis at a nice base number.
This is never essential it's just makes things easier to read, and
allows slotting other ioctls into the gaps later.
b) parallel feature development ends up conflicting then one thread never lands.
I've got two-three devs each adding a uAPI, we assign them 0x10, 0x11,
0x12 while they work, then 0x11 never lands because it was a bad idea.
However I think you should be fine enforcing a non-sparse space here
unless we want to handle replacing current drivers, as long as it's
hard to screw up so you know early.
Dave.
Re: [PATCH] fbdev: tgafb: Fix potential divide by zero
Dear Helge,
Thank you for the kind words. My real name is Wei Chen.
Please apply this patch to fbdev git tree if it is correct.
Best,
Wei
On Thu, 9 Mar 2023 at 06:05, Helge Deller wrote:
>
> On 3/7/23 14:08, harperchen wrote:
> > fb_set_var would by called when user invokes ioctl with cmd
> > FBIOPUT_VSCREENINFO. User-provided data would finally reach
> > tgafb_check_var. In case var->pixclock is assigned to zero,
> > divide by zero would occur when checking whether reciprocal
> > of var->pixclock is too high.
> >
> > Similar crashes have happened in other fbdev drivers. There
> > is no check and modification on var->pixclock along the call
> > chain to tgafb_check_var. We believe it could also be triggered
> > in driver tgafb from user site.
> >
> > Signed-off-by: harperchen
>
> Could you provide a real name?
> Otherwise applied to fbdev git tree.
>
> Thanks!
> Helge
>
> > ---
> > drivers/video/fbdev/tgafb.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
> > index 14d37c49633c..b44004880f0d 100644
> > --- a/drivers/video/fbdev/tgafb.c
> > +++ b/drivers/video/fbdev/tgafb.c
> > @@ -173,6 +173,9 @@ tgafb_check_var(struct fb_var_screeninfo *var, struct
> > fb_info *info)
> > {
> > struct tga_par *par = (struct tga_par *)info->par;
> >
> > + if (!var->pixclock)
> > + return -EINVAL;
> > +
> > if (par->tga_type == TGA_TYPE_8PLANE) {
> > if (var->bits_per_pixel != 8)
> > return -EINVAL;
>
Re: [PATCH RFC 02/18] rust: drm: Add Device and Driver abstractions
On 08/03/2023 03.19, Björn Roy Baron wrote:
> --- Original Message ---
> On Tuesday, March 7th, 2023 at 15:25, Asahi Lina wrote:
>
>> Add the initial abstractions for DRM drivers and devices. These go
>> together in one commit since they are fairly tightly coupled types.
>>
>> A few things have been stubbed out, to be implemented as further bits of
>> the DRM subsystem are introduced.
>>
>> Signed-off-by: Asahi Lina l...@asahilina.net
>>
>> ---
[...]
>> +/// Information data for a DRM Driver.
>> +pub struct DriverInfo {
>> +/// Driver major version.
>> +pub major: i32,
>> +/// Driver minor version.
>> +pub minor: i32,
>> +/// Driver patchlevel version.
>> +pub patchlevel: i32,
>> +/// Driver name.
>> +pub name: &'static CStr,
>> +/// Driver description.
>> +pub desc: &'static CStr,
>> +/// Driver date.
>> +pub date: &'static CStr,
>> +}
>> +
>
> Could you please add an Invariants section to the doc comments indicating
> what requirements these function pointers must satisfy?
I can try (as much as I can divine from the C side anyway...). I guess
you want interface docs for each callback, so like what it must do and
what invariants each one must uphold?
Note that this is a kernel crate-only struct (the fields are not public)
so users can't create their own AllocOps variants anyway (plus AllocImpl
is sealed, on top of that), but I guess it makes sense to document for
internal kernel crate purposes. At some point it might make sense to
allow drivers to override these with proper Rust callbacks (and then the
wrappers need to ensure safety), but right now that's not implemented.
>> +/// Internal memory management operation set, normally created by memory
>> managers (e.g. GEM).
>> +///
>> +/// See `kernel::drm::gem` and `kernel::drm::gem::shmem`.
>> +pub struct AllocOps {
>> +pub(crate) gem_create_object: Option<
>> +unsafe extern "C" fn(
>> +dev: *mut bindings::drm_device,
>> +size: usize,
>> +) -> *mut bindings::drm_gem_object,
>> +>,
>> +pub(crate) prime_handle_to_fd: Option<
>> +unsafe extern "C" fn(
>> +dev: *mut bindings::drm_device,
>> +file_priv: *mut bindings::drm_file,
>> +handle: u32,
>> +flags: u32,
>> +prime_fd: *mut core::ffi::c_int,
>> +) -> core::ffi::c_int,
>> +>,
>> +pub(crate) prime_fd_to_handle: Option<
>> +unsafe extern "C" fn(
>> +dev: *mut bindings::drm_device,
>> +file_priv: *mut bindings::drm_file,
>> +prime_fd: core::ffi::c_int,
>> +handle: *mut u32,
>> +) -> core::ffi::c_int,
>> +>,
>> +pub(crate) gem_prime_import: Option<
>> +unsafe extern "C" fn(
>> +dev: *mut bindings::drm_device,
>> +dma_buf: *mut bindings::dma_buf,
>> +) -> *mut bindings::drm_gem_object,
>> +>,
>> +pub(crate) gem_prime_import_sg_table: Option<
>> +unsafe extern "C" fn(
>> +dev: *mut bindings::drm_device,
>> +attach: *mut bindings::dma_buf_attachment,
>> +sgt: *mut bindings::sg_table,
>> +) -> *mut bindings::drm_gem_object,
>> +>,
>> +pub(crate) gem_prime_mmap: Option<
>> +unsafe extern "C" fn(
>> +obj: *mut bindings::drm_gem_object,
>> +vma: *mut bindings::vm_area_struct,
>> +) -> core::ffi::c_int,
>> +>,
>> +pub(crate) dumb_create: Option<
>> +unsafe extern "C" fn(
>> +file_priv: *mut bindings::drm_file,
>> +dev: *mut bindings::drm_device,
>> +args: *mut bindings::drm_mode_create_dumb,
>> +) -> core::ffi::c_int,
>> +>,
>> +pub(crate) dumb_map_offset: Option<
>> +unsafe extern "C" fn(
>> +file_priv: *mut bindings::drm_file,
>> +dev: *mut bindings::drm_device,
>> +handle: u32,
>> +offset: *mut u64,
>> +) -> core::ffi::c_int,
>> +>,
>> +pub(crate) dumb_destroy: Option<
>> +unsafe extern "C" fn(
>> +file_priv: *mut bindings::drm_file,
>> +dev: *mut bindings::drm_device,
>> +handle: u32,
>> +) -> core::ffi::c_int,
>> +>,
>> +}
>> +
~~ Lina
Re: [PATCH RFC 01/18] rust: drm: ioctl: Add DRM ioctl abstraction
On 08/03/2023 02.34, Björn Roy Baron wrote: >> +// SAFETY: This is just the ioctl argument, >> which hopefully has the right type >> +// (we've done our best checking the size). > > In the rust tree there is the ReadableFromBytes [1] trait which indicates > that it is safe to read arbitrary bytes into the type. Maybe you could add it > as bound on the argument type when it lands in rust-next? This way you can't > end up with for example a struct containing a bool with the byte value 2, > which is UB. There's actually a much bigger story here, because that trait isn't really very useful without a way to auto-derive it. I need the same kind of guarantee for all the GPU firmware structs... There's one using only declarative macros [1] and one using proc macros [2]. And then, since ioctl arguments are declared in C UAPI header files, we need a way to be able to derive those traits for them... which I guess means bindgen changes? For now though, I don't think this is something we need to worry about too much for this particular use case because the macro forces all struct types to be part of `bindings`, and any driver UAPI should already follow these constraints if it is well-formed (and UAPIs are going to already attract a lot of scrutiny anyway). Technically you could try taking a random kernel struct containing a `bool` in an ioctl list, but that would stand out as nonsense just as much as trying to unsafe impl ReadableFromBytes for it so... it's kind of an academic problem ^^ Actually, I think we talked of moving UAPI types to a separate crate (so drivers can get access to those types and only those types, not the main bindings crate). Then maybe we could just say that if the macro forces the type to be from that crate, it's inherently safe since all UAPIs should already be castable to/from bytes if properly designed. Aside: I'm not sure the ReadableFromBytes/WritableToBytes distinction is very useful. I know it exists (padding bytes, uninit fields, and technically bool should be WritableToBytes but not ReadableFromBytes), but I can't think of a good use case for it... I think I'd rather start with a single trait and just always enforce the union of the rules, because pretty much any time you're casting to/from bytes you want well-defined "bag of bytes" struct layouts anyway. ioctls can be R/W/RW so having separate traits depending on ioctl type complicates the code... [1] https://github.com/QubesOS/qubes-gui-rust/blob/940754bfefb7325548eece658c307a0c41c9bc7c/qubes-castable/src/lib.rs [2] https://docs.rs/pkbuffer/latest/pkbuffer/derive.Castable.html > > https://rust-for-linux.github.io/docs/kernel/io_buffer/trait.ReadableFromBytes.html > [1] > ~~ Lina
[PATCH 2/2] drm/probe_helper: warning on poll_enabled for issue catching
In order to catch issues in other drivers to ensure proper call
sequence of polling function.
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/2411
Fixes: a4e771729a51("drm/probe_helper: sort out poll_running vs poll_enabled")
Reported-by: Bert Karwatzki
Suggested-by: Dmitry Baryshkov
Signed-off-by: Guchun Chen
---
drivers/gpu/drm/drm_probe_helper.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/drm_probe_helper.c
b/drivers/gpu/drm/drm_probe_helper.c
index 8127be134c39..85e0e80d4a52 100644
--- a/drivers/gpu/drm/drm_probe_helper.c
+++ b/drivers/gpu/drm/drm_probe_helper.c
@@ -852,6 +852,8 @@ EXPORT_SYMBOL(drm_kms_helper_is_poll_worker);
*/
void drm_kms_helper_poll_disable(struct drm_device *dev)
{
+ WARN_ON(!dev->mode_config.poll_enabled);
+
if (dev->mode_config.poll_running)
drm_kms_helper_disable_hpd(dev);
--
2.25.1
[PATCH 1/2] drm/amdgpu: move poll enabled/disable into non DC path
Some amd asics having reliable hotplug support don't call
drm_kms_helper_poll_init in driver init sequence. However,
due to the unified suspend/resume path for all asics, because
the output_poll_work->func is not set for these asics, a warning
arrives when suspending.
[ 90.656049]
[ 90.656050] ? console_unlock+0x4d/0x100
[ 90.656053] ? __irq_work_queue_local+0x27/0x60
[ 90.656056] ? irq_work_queue+0x2b/0x50
[ 90.656057] ? __wake_up_klogd+0x40/0x60
[ 90.656059] __cancel_work_timer+0xed/0x180
[ 90.656061] drm_kms_helper_poll_disable.cold+0x1f/0x2c [drm_kms_helper]
[ 90.656072] amdgpu_device_suspend+0x81/0x170 [amdgpu]
[ 90.656180] amdgpu_pmops_runtime_suspend+0xb5/0x1b0 [amdgpu]
[ 90.656269] pci_pm_runtime_suspend+0x61/0x1b0
drm_kms_helper_poll_enable/disable is valid when poll_init is called in
amdgpu code, which is only used in non DC path. So move such codes into
non-DC path code to get rid of such warnings.
Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/2411
Fixes: a4e771729a51("drm/probe_helper: sort out poll_running vs poll_enabled")
Reported-by: Bert Karwatzki
Suggested-by: Dmitry Baryshkov
Suggested-by: Alex Deucher
Signed-off-by: Guchun Chen
---
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 4
drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 4
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c4a4e2fe6681..da5b0258a237 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,8 +4145,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
DRM_WARN("smart shift update failed\n");
- drm_kms_helper_poll_disable(dev);
-
if (fbcon)
drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, true);
@@ -4243,8 +4241,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool
fbcon)
if (fbcon)
drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, false);
- drm_kms_helper_poll_enable(dev);
-
amdgpu_ras_resume(adev);
if (adev->mode_info.num_crtc) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 503f89a766c3..d60fe7eb5579 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -1618,6 +1618,8 @@ int amdgpu_display_suspend_helper(struct amdgpu_device
*adev)
struct drm_connector_list_iter iter;
int r;
+ drm_kms_helper_poll_disable(dev);
+
/* turn off display hw */
drm_modeset_lock_all(dev);
drm_connector_list_iter_begin(dev, );
@@ -1694,6 +1696,8 @@ int amdgpu_display_resume_helper(struct amdgpu_device
*adev)
drm_modeset_unlock_all(dev);
+ drm_kms_helper_poll_enable(dev);
+
return 0;
}
--
2.25.1
Re: [PATCH v2] drm/i915/gvt: Make use of idr_find and idr_for_each_entry in dmabuf
On 2023.03.03 22:07:18 +0800, Cai Huoqing wrote:
> This patch uses the already existing IDR mechanism to simplify
> and improve the dmabuf code.
>
> Using 'vgpu.object_idr' directly instead of 'dmabuf_obj_list_head'
> or 'dmabuf.list', because the dmabuf_obj can be found by 'idr_find'
> or 'idr_for_each_entry'.
>
> Signed-off-by: Cai Huoqing
> ---
> v1->v2:
> 1.Use idr_find to get the target one and free it instead of free all
> dma objs.
> 2.Revert the original code 'ret' related
> 3.Add '&& !idr_is_empty()' like the original code '&& !list_empty()'
Looks good to me. I'll send it for some regression test before upstream.
Reviewed-by: Zhenyu Wang
Thanks!
>
> v1 link:
>
> https://lore.kernel.org/lkml/20230302115318.79487-1-cai.huoq...@linux.dev/
>
> drivers/gpu/drm/i915/gvt/dmabuf.c | 58 +++
> drivers/gpu/drm/i915/gvt/dmabuf.h | 1 -
> drivers/gpu/drm/i915/gvt/gvt.h| 1 -
> drivers/gpu/drm/i915/gvt/vgpu.c | 1 -
> 4 files changed, 12 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/dmabuf.c
> b/drivers/gpu/drm/i915/gvt/dmabuf.c
> index 6834f9fe40cf..cf619b1ed3ad 100644
> --- a/drivers/gpu/drm/i915/gvt/dmabuf.c
> +++ b/drivers/gpu/drm/i915/gvt/dmabuf.c
> @@ -133,21 +133,15 @@ static void dmabuf_gem_object_free(struct kref *kref)
> struct intel_vgpu_dmabuf_obj *obj =
> container_of(kref, struct intel_vgpu_dmabuf_obj, kref);
> struct intel_vgpu *vgpu = obj->vgpu;
> - struct list_head *pos;
> struct intel_vgpu_dmabuf_obj *dmabuf_obj;
>
> if (vgpu && test_bit(INTEL_VGPU_STATUS_ACTIVE, vgpu->status) &&
> - !list_empty(>dmabuf_obj_list_head)) {
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct
> intel_vgpu_dmabuf_obj, list);
> - if (dmabuf_obj == obj) {
> - list_del(pos);
> - idr_remove(>object_idr,
> -dmabuf_obj->dmabuf_id);
> - kfree(dmabuf_obj->info);
> - kfree(dmabuf_obj);
> - break;
> - }
> + !idr_is_empty(>object_idr)) {
> + dmabuf_obj = idr_find(>object_idr, obj->dmabuf_id);
> + if (dmabuf_obj) {
> + idr_remove(>object_idr, obj->dmabuf_id);
> + kfree(dmabuf_obj->info);
> + kfree(dmabuf_obj);
> }
> } else {
> /* Free the orphan dmabuf_objs here */
> @@ -340,13 +334,12 @@ static struct intel_vgpu_dmabuf_obj *
> pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> struct intel_vgpu_fb_info *latest_info)
> {
> - struct list_head *pos;
> struct intel_vgpu_fb_info *fb_info;
> struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> struct intel_vgpu_dmabuf_obj *ret = NULL;
> + int id;
>
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> list);
> + idr_for_each_entry(>object_idr, dmabuf_obj, id) {
> if (!dmabuf_obj->info)
> continue;
>
> @@ -366,24 +359,6 @@ pick_dmabuf_by_info(struct intel_vgpu *vgpu,
> return ret;
> }
>
> -static struct intel_vgpu_dmabuf_obj *
> -pick_dmabuf_by_num(struct intel_vgpu *vgpu, u32 id)
> -{
> - struct list_head *pos;
> - struct intel_vgpu_dmabuf_obj *dmabuf_obj = NULL;
> - struct intel_vgpu_dmabuf_obj *ret = NULL;
> -
> - list_for_each(pos, >dmabuf_obj_list_head) {
> - dmabuf_obj = list_entry(pos, struct intel_vgpu_dmabuf_obj,
> list);
> - if (dmabuf_obj->dmabuf_id == id) {
> - ret = dmabuf_obj;
> - break;
> - }
> - }
> -
> - return ret;
> -}
> -
> static void update_fb_info(struct vfio_device_gfx_plane_info *gvt_dmabuf,
> struct intel_vgpu_fb_info *fb_info)
> {
> @@ -477,11 +452,6 @@ int intel_vgpu_query_plane(struct intel_vgpu *vgpu, void
> *args)
>
> update_fb_info(gfx_plane_info, _info);
>
> - INIT_LIST_HEAD(_obj->list);
> - mutex_lock(>dmabuf_lock);
> - list_add_tail(_obj->list, >dmabuf_obj_list_head);
> - mutex_unlock(>dmabuf_lock);
> -
> gvt_dbg_dpy("vgpu%d: %s new dmabuf_obj ref %d, id %d\n", vgpu->id,
> __func__, kref_read(_obj->kref), ret);
>
> @@ -508,7 +478,7 @@ int intel_vgpu_get_dmabuf(struct intel_vgpu *vgpu,
> unsigned int dmabuf_id)
>
> mutex_lock(>dmabuf_lock);
>
> - dmabuf_obj = pick_dmabuf_by_num(vgpu, dmabuf_id);
> + dmabuf_obj = idr_find(>object_idr, dmabuf_id);
> if (dmabuf_obj == NULL) {
> gvt_vgpu_err("invalid dmabuf id:%d\n", dmabuf_id);
> ret = -EINVAL;
> @@ -570,23 +540,19 @@ int intel_vgpu_get_dmabuf(struct
Re: [PATCH RFC 01/18] rust: drm: ioctl: Add DRM ioctl abstraction
On 08/03/2023 00.32, Maíra Canal wrote:
> On 3/7/23 11:25, Asahi Lina wrote:
>> DRM drivers need to be able to declare which driver-specific ioctls they
>> support. This abstraction adds the required types and a helper macro to
>> generate the ioctl definition inside the DRM driver.
>>
>> Note that this macro is not usable until further bits of the
>> abstraction are in place (but it will not fail to compile on its own, if
>> not called).
>>
>> Signed-off-by: Asahi Lina
>> ---
>> drivers/gpu/drm/Kconfig | 7 ++
>> rust/bindings/bindings_helper.h | 2 +
>> rust/kernel/drm/ioctl.rs| 147
>>
>> rust/kernel/drm/mod.rs | 5 ++
>> rust/kernel/lib.rs | 2 +
>> 5 files changed, 163 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index dc0f94f02a82..dab8f0f9aa96 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -27,6 +27,13 @@ menuconfig DRM
>>details. You should also select and configure AGP
>>(/dev/agpgart) support if it is available for your platform.
>>
>
> [...]
>
>> +
>> +/// Declare the DRM ioctls for a driver.
>> +///
>> +/// Each entry in the list should have the form:
>> +///
>> +/// `(ioctl_number, argument_type, flags, user_callback),`
>> +///
>> +/// `argument_type` is the type name within the `bindings` crate.
>> +/// `user_callback` should have the following prototype:
>> +///
>> +/// ```
>> +/// fn foo(device: ::drm::device::Device,
>> +///data: bindings::argument_type,
>> +///file: ::drm::file::File,
>> +/// )
>> +/// ```
>> +/// where `Self` is the drm::drv::Driver implementation these ioctls are
>> being declared within.
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// kernel::declare_drm_ioctls! {
>> +/// (FOO_GET_PARAM, drm_foo_get_param, ioctl::RENDER_ALLOW,
>> my_get_param_handler),
>> +/// }
>> +/// ```
>> +///
>> +#[macro_export]
>> +macro_rules! declare_drm_ioctls {
>> +( $(($cmd:ident, $struct:ident, $flags:expr, $func:expr)),* $(,)? ) => {
>> +const IOCTLS: &'static [$crate::drm::ioctl::DrmIoctlDescriptor] = {
>> +const _:() = {
>> +let i: u32 = $crate::bindings::DRM_COMMAND_BASE;
>> +// Assert that all the IOCTLs are in the right order and
>> there are no gaps,
>> +// and that the sizeof of the specified type is correct.
>
> I believe that not necessarily the IOCTLs need to be in the right order and
> with no gaps. For example, armada_drm.h has a gap in between 0x00 and
> 0x02 and exynos_drm.h also have gaps. Moreover, some drivers, like vgem and
> virtgpu, start their IOCTLs with 0x01.
Yeah, we talked about this a bit... do you have any ideas about how to
design this? I think it should be possible with a const function
initializing an array entry by entry, we just need a two-pass macro
(once to determine the max ioctl number, then again to actually output
the implementation).
I'm not sure why drivers would have gaps in the ioctl numbers though...
my idea was that new drivers shouldn't need that as far as I can tell
(you can't remove APIs after the fact due to UAPI stability guarantees,
so as long as you don't have gaps to begin with...). But I guess if
we're reimplementing existing drivers in Rust we'll need this... though
maybe it makes sense to just say it's not supported and require
reimplementations that have holes to just explicitly add dummy ioctls
that return EINVAL? We could even provide such a dummy generic ioctl
handler on the abstraction side, so drivers just have to add it to the
list, or make the macro take a special token that is used for
placeholder ioctls that don't exist (which then creates the NULL
function pointer that the drm core interprets as invalid)...
Basically I'm not sure if it makes sense to fully support noncontiguous
ioctl numbers automagically, or just say drivers need to explicitly list
gaps. I'd love to hear the opinion of other DRM folks about this!
~~ Lina
Re: [PATCH RFC 06/18] rust: drm: gem: shmem: Add DRM shmem helper abstraction
On 08/03/2023 22.38, Maíra Canal wrote:
> On 3/7/23 11:25, Asahi Lina wrote:
>> The DRM shmem helper includes common code useful for drivers which
>> allocate GEM objects as anonymous shmem. Add a Rust abstraction for
>> this. Drivers can choose the raw GEM implementation or the shmem layer,
>> depending on their needs.
>>
>> Signed-off-by: Asahi Lina
>> ---
>> drivers/gpu/drm/Kconfig | 5 +
>> rust/bindings/bindings_helper.h | 2 +
>> rust/helpers.c | 67 +++
>> rust/kernel/drm/gem/mod.rs | 3 +
>> rust/kernel/drm/gem/shmem.rs| 381
>>
>> 5 files changed, 458 insertions(+)
>>
>
> [...]
>
>> +unsafe extern "C" fn gem_create_object(
>> +raw_dev: *mut bindings::drm_device,
>> +size: usize,
>> +) -> *mut bindings::drm_gem_object {
>> +// SAFETY: GEM ensures the device lives as long as its objects live,
>> +// so we can conjure up a reference from thin air and never drop it.
>> +let dev = ManuallyDrop::new(unsafe { device::Device::from_raw(raw_dev)
>> });
>> +
>> +let inner = match T::new(&*dev, size) {
>> +Ok(v) => v,
>> +Err(e) => return e.to_ptr(),
>> +};
>> +
>> +let p = unsafe {
>> +bindings::krealloc(
>> +core::ptr::null(),
>> +ObjectSIZE,
>> +bindings::GFP_KERNEL | bindings::__GFP_ZERO,
>> +) as *mut Object
>> +};
>> +
>> +if p.is_null() {
>> +return ENOMEM.to_ptr();
>> +}
>> +
>> +// SAFETY: p is valid as long as the alloc succeeded
>> +unsafe {
>> +addr_of_mut!((*p).dev).write(dev);
>> +addr_of_mut!((*p).inner).write(inner);
>> +}
>> +
>> +// SAFETY: drm_gem_shmem_object is safe to zero-init, and
>> +// the rest of Object has been initialized
>> +let new: Object = unsafe { *(p as *mut _) };
>> +
>> +new.obj.base.funcs = VTABLE;
>> + new.obj.base
>> +}
>
> It would be nice to allow to set wc inside the gem_create_object callback,
> as some drivers do it so, like v3d, vc4, panfrost, lima...
This is actually a bit tricky to do safely, because we can't just have a
callback that takes the drm_gem_shmem_object instance inside
gem_create_object because it is not fully initialized yet from the point
of view of the gem shmem API. Maybe we could have some sort of temporary
proxy object that only lets you do safe things like set map_wc? Or maybe
the new() callback could return something like a ShmemTemplate type
that contains both the inner data and some miscellaneous fields like the
initial map_wc state?
I think we can also just wait until the first user before we do this
though... the goal of the abstractions is to support the APIs we
actually use. I know you need this for vgem, so please feel free to
implement it as a separate patch! I think it's best if you get credit
for the abstraction changes you need, so we can all work together on the
design so it works for everyone's use cases instead of just having me
make all the decisions ^^ (and it's fine if we have to refactor the APIs!)
~~ Lina
Re: [PATCH 9/9] drm: move ttm_execbuf_util into vmwgfx
On Wed, 2023-03-08 at 10:10 +0100, Christian König wrote: > > Am 08.03.23 um 06:14 schrieb Zack Rusin: > > On Tue, 2023-02-28 at 09:34 +0100, Christian König wrote: > > > VMWGFX is the only remaining user of this and should probably moved over > > > to drm_exec when it starts using GEM as well. > > Is this because vmwgfx piggybacks buffer-id relocations on top of ttm > > validations or > > did you just find it too hard to port it over? I'd prefer to avoid ttm > > moves to > > vmwgfx and at least have a clear idea of what we need to do to port. > > I've just found it to hard to port it over because vmwgfx does some > strange things with the validation code here. > > If you want we can take a deeper look at this together, but I need to > find some time. > > Alternatively just tell me how to do it and I will add that to the patch > set :) I don't want to hold up the set (it looks good btw), because I had to look at something else today and tomorrow. We overload the validation lists to do quite a bit more than just reservations though. There are, I think, four separate things that need to be refactored there (Christian, feel free to skip this section, this is mainly for VMware folks on the team): 1) Relocations - userspace uses the id's of the bo's in the command stream, but on the kernel side those id's are different (or in vmwgfx terminology gem id != mob id), so the buffer id's in the command stream need to be replaced, 2) Resource validation. vmwgfx splits the userspace objects into buffers and resources (shaders, surfaces, contexts). The resources are not buffers but are backed by them. A single buffer can back multiple different resources and sometimes the kernel has to actually allocate a buffer to back a resource and attach it to it (i.e. in common terminology buffer is the memory and resources are placed in it) . Now this shouldn't be in the kernel at all, the resources shouldn't have been kernel objects and instead we should have left them completely to userspace. 3) Coherency tracking. We use validation lists as a central place for tracking which bo's/resources are used in a command buffer and we use it to keep track of which buffers/resources will endup dirty to implement coherency. 4) Central place to allocate memory for relocation/validation nodes. Where we want to endup is with 2 completely gone from the kernel side and 1, 3 and 4 refactored and cleaned up. I think there's at least 4 separate patches to this port, so it's not a trivial thing. We will take a look at this on Friday in more detail to see what we can do. z
[pull] amdgpu, amdkfd drm-fixes-6.3
Hi Dave, Daniel, Fixes for 6.3. The following changes since commit 66305069eb6d17d9190cbcd196f3f7487df47ae8: Merge tag 'drm-misc-fixes-2023-02-23' of git://anongit.freedesktop.org/drm/drm-misc into drm-fixes (2023-03-07 05:42:34 +1000) are available in the Git repository at: https://gitlab.freedesktop.org/agd5f/linux.git tags/amd-drm-fixes-6.3-2023-03-08 for you to fetch changes up to 41f95a0e40903fcf70463fcc060b7faf761b23f6: drm/amdgpu/soc21: Add video cap query support for VCN_4_0_4 (2023-03-08 14:39:09 -0500) amd-drm-fixes-6.3-2023-03-08: amdgpu: - Misc display fixes - UMC 8.10 fixes - Driver unload fixes - NBIO 7.3.0 fix - Error checking fixes for soc15, nv, soc21 read register interface - Fix video cap query for VCN 4.0.4 amdkfd: - Fix BO offset for multi-VMA page migration - Fix return check in doorbell handling Alex Deucher (3): drm/amdgpu: fix error checking in amdgpu_read_mm_registers for soc15 drm/amdgpu: fix error checking in amdgpu_read_mm_registers for soc21 drm/amdgpu: fix error checking in amdgpu_read_mm_registers for nv Candice Li (2): drm/amdgpu: Support umc node harvest config on umc v8_10 drm/amd/pm: Enable ecc_info table support for smu v13_0_10 Harry Wentland (2): drm/display: Don't block HDR_OUTPUT_METADATA on unknown EOTF drm/connector: print max_requested_bpc in state debugfs Mario Limonciello (1): drm/amd: Fix initialization mistake for NBIO 7.3.0 Shashank Sharma (1): drm/amdgpu: fix return value check in kfd Swapnil Patel (1): drm/amd/display: Update clock table to include highest clock setting Veerabadhran Gopalakrishnan (1): drm/amdgpu/soc21: Add video cap query support for VCN_4_0_4 Xiaogang Chen (1): drm/amdkfd: Fix BO offset for multi-VMA page migration lyndonli (2): drm/amdgpu: Fix call trace warning and hang when removing amdgpu device drm/amdgpu: Fix the warning info when removing amdgpu device drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 10 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c| 17 + drivers/gpu/drm/amd/amdgpu/amdgpu_umc.h| 7 +- drivers/gpu/drm/amd/amdgpu/gmc_v11_0.c | 1 - drivers/gpu/drm/amd/amdgpu/nbio_v7_2.c | 14 ++-- drivers/gpu/drm/amd/amdgpu/nv.c| 7 +- drivers/gpu/drm/amd/amdgpu/soc15.c | 5 +- drivers/gpu/drm/amd/amdgpu/soc21.c | 8 ++- drivers/gpu/drm/amd/amdgpu/umc_v8_10.h | 4 +- drivers/gpu/drm/amd/amdkfd/kfd_doorbell.c | 2 +- drivers/gpu/drm/amd/amdkfd/kfd_migrate.c | 17 +++-- .../drm/amd/display/dc/clk_mgr/dcn301/vg_clk_mgr.c | 19 +- .../gpu/drm/amd/pm/swsmu/smu13/smu_v13_0_0_ppt.c | 75 ++ drivers/gpu/drm/display/drm_hdmi_helper.c | 6 +- drivers/gpu/drm/drm_atomic.c | 1 + 16 files changed, 146 insertions(+), 49 deletions(-)
Re: [PATCH 3/3] drm/i915/pmu: Use common freq functions with sysfs
On Tue, 07 Mar 2023 22:12:49 -0800, Belgaumkar, Vinay wrote: > Hi Vinay, > On 3/7/2023 9:33 PM, Ashutosh Dixit wrote: > > Using common freq functions with sysfs in PMU (but without taking > > forcewake) solves the following issues (a) missing support for MTL (b) > > For the requested_freq, we read it only if actual_freq is zero below > (meaning, GT is in C6). So then what is the point of reading it without a > force wake? It will also be zero, correct? Yes agreed. I had tested this and you do see values for requested freq which look correct even when actual freq is 0 even without taking forcewake. That is why I ended up writing Patch 2/3. However what I missed is what you pointed out that 0xa008 is a shadowed register which cannot be read without taking forcewake. It is probably returning the last value which was written to the shadowed write register. As a result I have dropped the "drm/i915/rps: Expose get_requested_frequency_fw for PMU" patch in v2 of this series. Thanks. -- Ashutosh
Re: [PATCH] drm/amdgpu: Remove useless else if
On Wed, Mar 8, 2023 at 10:37 PM Jiapeng Chong
wrote:
>
> The assignment of the else and if branches is the same, so the if else
> here is redundant, so we remove it.
>
> ./drivers/gpu/drm/amd/amdgpu/nv.c:1048:2-4: WARNING: possible condition with
> no effect (if == else).
>
> Reported-by: Abaci Robot
> Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4454
> Signed-off-by: Jiapeng Chong
> ---
> drivers/gpu/drm/amd/amdgpu/nv.c | 18 +-
> 1 file changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
> index 855d390c41de..84803929f7d9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/nv.c
> @@ -1045,19 +1045,11 @@ static int nv_common_late_init(void *handle)
>
> if (amdgpu_sriov_vf(adev)) {
> xgpu_nv_mailbox_get_irq(adev);
> - if (adev->vcn.harvest_config & AMDGPU_VCN_HARVEST_VCN0) {
> - amdgpu_virt_update_sriov_video_codec(adev,
> -
> sriov_sc_video_codecs_encode_array,
> -
> ARRAY_SIZE(sriov_sc_video_codecs_encode_array),
> -
> sriov_sc_video_codecs_decode_array_vcn1,
> -
> ARRAY_SIZE(sriov_sc_video_codecs_decode_array_vcn1));
> - } else {
> - amdgpu_virt_update_sriov_video_codec(adev,
> -
> sriov_sc_video_codecs_encode_array,
> -
> ARRAY_SIZE(sriov_sc_video_codecs_encode_array),
> -
> sriov_sc_video_codecs_decode_array_vcn1,
> -
> ARRAY_SIZE(sriov_sc_video_codecs_decode_array_vcn1));
This should be vcn0. I'll send out a patch. Thanks!
Alex
> - }
> + amdgpu_virt_update_sriov_video_codec(adev,
> +
> sriov_sc_video_codecs_encode_array,
> +
> ARRAY_SIZE(sriov_sc_video_codecs_encode_array),
> +
> sriov_sc_video_codecs_decode_array_vcn1,
> +
> ARRAY_SIZE(sriov_sc_video_codecs_decode_array_vcn1));
> }
>
> return 0;
> --
> 2.20.1.7.g153144c
>
[PATCH 1/2] drm/i915/pmu: Use functions common with sysfs to read actual freq
Expose intel_rps_read_actual_frequency_fw to read the actual freq without
taking forcewake for use by PMU. The code is refactored to use a common set
of functions across sysfs and PMU. Using common functions with sysfs in PMU
solves the issues of missing support for MTL and missing support for older
generations (prior to Gen6). It also future proofs the PMU where sometimes
code has been updated for sysfs and PMU has been missed.
Fixes: 22009b6dad66 ("drm/i915/mtl: Modify CAGF functions for MTL")
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8280
Signed-off-by: Ashutosh Dixit
---
drivers/gpu/drm/i915/gt/intel_rps.c | 46 +++--
drivers/gpu/drm/i915/gt/intel_rps.h | 2 +-
drivers/gpu/drm/i915/i915_pmu.c | 10 +++
3 files changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
b/drivers/gpu/drm/i915/gt/intel_rps.c
index 4d0dc9de23f9..3957c5ee5cba 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -2046,16 +2046,6 @@ void intel_rps_sanitize(struct intel_rps *rps)
rps_disable_interrupts(rps);
}
-u32 intel_rps_read_rpstat_fw(struct intel_rps *rps)
-{
- struct drm_i915_private *i915 = rps_to_i915(rps);
- i915_reg_t rpstat;
-
- rpstat = (GRAPHICS_VER(i915) >= 12) ? GEN12_RPSTAT1 : GEN6_RPSTAT1;
-
- return intel_uncore_read_fw(rps_to_gt(rps)->uncore, rpstat);
-}
-
u32 intel_rps_read_rpstat(struct intel_rps *rps)
{
struct drm_i915_private *i915 = rps_to_i915(rps);
@@ -2089,10 +2079,11 @@ u32 intel_rps_get_cagf(struct intel_rps *rps, u32
rpstat)
return cagf;
}
-static u32 read_cagf(struct intel_rps *rps)
+static u32 __read_cagf(struct intel_rps *rps, bool take_fw)
{
struct drm_i915_private *i915 = rps_to_i915(rps);
struct intel_uncore *uncore = rps_to_uncore(rps);
+ i915_reg_t r = INVALID_MMIO_REG;
u32 freq;
/*
@@ -2100,22 +2091,30 @@ static u32 read_cagf(struct intel_rps *rps)
* registers will return 0 freq when GT is in RC6
*/
if (GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) {
- freq = intel_uncore_read(uncore, MTL_MIRROR_TARGET_WP1);
+ r = MTL_MIRROR_TARGET_WP1;
} else if (GRAPHICS_VER(i915) >= 12) {
- freq = intel_uncore_read(uncore, GEN12_RPSTAT1);
+ r = GEN12_RPSTAT1;
} else if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) {
vlv_punit_get(i915);
freq = vlv_punit_read(i915, PUNIT_REG_GPU_FREQ_STS);
vlv_punit_put(i915);
+ goto exit;
} else if (GRAPHICS_VER(i915) >= 6) {
- freq = intel_uncore_read(uncore, GEN6_RPSTAT1);
+ r = GEN6_RPSTAT1;
} else {
- freq = intel_uncore_read(uncore, MEMSTAT_ILK);
+ r = MEMSTAT_ILK;
}
+ freq = take_fw ? intel_uncore_read(uncore, r) :
intel_uncore_read_fw(uncore, r);
+exit:
return intel_rps_get_cagf(rps, freq);
}
+static u32 read_cagf(struct intel_rps *rps)
+{
+ return __read_cagf(rps, true);
+}
+
u32 intel_rps_read_actual_frequency(struct intel_rps *rps)
{
struct intel_runtime_pm *rpm = rps_to_uncore(rps)->rpm;
@@ -2128,6 +2127,23 @@ u32 intel_rps_read_actual_frequency(struct intel_rps
*rps)
return freq;
}
+static u32 read_cagf_fw(struct intel_rps *rps)
+{
+ return __read_cagf(rps, false);
+}
+
+u32 intel_rps_read_actual_frequency_fw(struct intel_rps *rps)
+{
+ struct intel_runtime_pm *rpm = rps_to_uncore(rps)->rpm;
+ intel_wakeref_t wakeref;
+ u32 freq = 0;
+
+ with_intel_runtime_pm_if_in_use(rpm, wakeref)
+ freq = intel_gpu_freq(rps, read_cagf_fw(rps));
+
+ return freq;
+}
+
u32 intel_rps_read_punit_req(struct intel_rps *rps)
{
struct intel_uncore *uncore = rps_to_uncore(rps);
diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h
b/drivers/gpu/drm/i915/gt/intel_rps.h
index c622962c6bef..2d5b3ef58606 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.h
+++ b/drivers/gpu/drm/i915/gt/intel_rps.h
@@ -39,6 +39,7 @@ int intel_gpu_freq(struct intel_rps *rps, int val);
int intel_freq_opcode(struct intel_rps *rps, int val);
u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1);
u32 intel_rps_read_actual_frequency(struct intel_rps *rps);
+u32 intel_rps_read_actual_frequency_fw(struct intel_rps *rps);
u32 intel_rps_get_requested_frequency(struct intel_rps *rps);
u32 intel_rps_get_min_frequency(struct intel_rps *rps);
u32 intel_rps_get_min_raw_freq(struct intel_rps *rps);
@@ -52,7 +53,6 @@ u32 intel_rps_get_rpn_frequency(struct intel_rps *rps);
u32 intel_rps_read_punit_req(struct intel_rps *rps);
u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps);
u32 intel_rps_read_rpstat(struct intel_rps *rps);
-u32 intel_rps_read_rpstat_fw(struct intel_rps *rps);
void gen6_rps_get_freq_caps(struct intel_rps *rps,
[PATCH 0/2] drm/i915/pmu: Use common freq functions with sysfs
Expose intel_rps_read_actual_frequency_fw to read the actual freq without taking forcewake for use by PMU. The code is refactored to use a common set of functions across sysfs and PMU. Using common functions with sysfs in PMU solves the issues of missing support for MTL and missing support for older generations (prior to Gen6). It also future proofs the PMU where sometimes code has been updated for sysfs and PMU has been missed. Ashutosh Dixit (2): drm/i915/pmu: Use functions common with sysfs to read actual freq drm/i915/pmu: Remove fallback to requested freq for SLPC drivers/gpu/drm/i915/gt/intel_rps.c | 46 +++-- drivers/gpu/drm/i915/gt/intel_rps.h | 2 +- drivers/gpu/drm/i915/i915_pmu.c | 17 +++ 3 files changed, 43 insertions(+), 22 deletions(-) -- 2.38.0
[PATCH 2/2] drm/i915/pmu: Remove fallback to requested freq for SLPC
The fallback to requested freq does not work for SLPC because SLPC does not use 'struct intel_rps'. Also for SLPC requested freq can only be obtained from a hw register after acquiring forcewake which we don't want to do for PMU. Therefore remove fallback to requested freq for SLPC. The actual freq will be 0 when gt is in RC6 which is correct. Also this is rare since PMU freq sampling happens only when gt is unparked. Signed-off-by: Ashutosh Dixit --- drivers/gpu/drm/i915/i915_pmu.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 7ece883a7d95..f697fabed64a 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -393,7 +393,14 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) * frequency. Fortunately, the read should rarely fail! */ val = intel_rps_read_actual_frequency_fw(rps); - if (!val) + + /* +* SLPC does not use 'struct intel_rps'. Also for SLPC +* requested freq can only be obtained after acquiring +* forcewake and reading a hw register. For SLPC just +* let val be 0 +*/ + if (!val && !intel_uc_uses_guc_slpc(>uc)) val = intel_gpu_freq(rps, rps->cur_freq); add_sample_mult(>sample[__I915_SAMPLE_FREQ_ACT], -- 2.38.0
[PATCH] drm/amdgpu: Remove useless else if
The assignment of the else and if branches is the same, so the if else
here is redundant, so we remove it.
./drivers/gpu/drm/amd/amdgpu/nv.c:1048:2-4: WARNING: possible condition with no
effect (if == else).
Reported-by: Abaci Robot
Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4454
Signed-off-by: Jiapeng Chong
---
drivers/gpu/drm/amd/amdgpu/nv.c | 18 +-
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/nv.c b/drivers/gpu/drm/amd/amdgpu/nv.c
index 855d390c41de..84803929f7d9 100644
--- a/drivers/gpu/drm/amd/amdgpu/nv.c
+++ b/drivers/gpu/drm/amd/amdgpu/nv.c
@@ -1045,19 +1045,11 @@ static int nv_common_late_init(void *handle)
if (amdgpu_sriov_vf(adev)) {
xgpu_nv_mailbox_get_irq(adev);
- if (adev->vcn.harvest_config & AMDGPU_VCN_HARVEST_VCN0) {
- amdgpu_virt_update_sriov_video_codec(adev,
-
sriov_sc_video_codecs_encode_array,
-
ARRAY_SIZE(sriov_sc_video_codecs_encode_array),
-
sriov_sc_video_codecs_decode_array_vcn1,
-
ARRAY_SIZE(sriov_sc_video_codecs_decode_array_vcn1));
- } else {
- amdgpu_virt_update_sriov_video_codec(adev,
-
sriov_sc_video_codecs_encode_array,
-
ARRAY_SIZE(sriov_sc_video_codecs_encode_array),
-
sriov_sc_video_codecs_decode_array_vcn1,
-
ARRAY_SIZE(sriov_sc_video_codecs_decode_array_vcn1));
- }
+ amdgpu_virt_update_sriov_video_codec(adev,
+
sriov_sc_video_codecs_encode_array,
+
ARRAY_SIZE(sriov_sc_video_codecs_encode_array),
+
sriov_sc_video_codecs_decode_array_vcn1,
+
ARRAY_SIZE(sriov_sc_video_codecs_decode_array_vcn1));
}
return 0;
--
2.20.1.7.g153144c
[PATCH] drm/amd/display: Use swap() instead of open coding it
Swap is a function interface that provides exchange function. To avoid
code duplication, we can use swap function.
./drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:359:57-58: WARNING
opportunity for swap().
Reported-by: Abaci Robot
Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4448
Signed-off-by: Jiapeng Chong
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index ae994c6c65ac..f6d9bbce15b2 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -352,13 +352,9 @@ static inline void reverse_planes_order(struct
dc_surface_update *array_of_surfa
int planes_count)
{
int i, j;
- struct dc_surface_update surface_updates_temp;
- for (i = 0, j = planes_count - 1; i < j; i++, j--) {
- surface_updates_temp = array_of_surface_update[i];
- array_of_surface_update[i] = array_of_surface_update[j];
- array_of_surface_update[j] = surface_updates_temp;
- }
+ for (i = 0, j = planes_count - 1; i < j; i++, j--)
+ swap(array_of_surface_update[i], array_of_surface_update[j]);
}
/**
--
2.20.1.7.g153144c
Re: [PATCH v3 14/17] drm/amd/display: Add debugfs for testing output colorspace
On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
>
> In order to IGT test colorspace we'll want to print
> the currently enabled colorspace on a stream. We add
> a new debugfs to do so, using the same scheme as
> current bpc reporting.
>
> This might also come in handy when debugging display
> issues.
>
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Joshua Ashton
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> Reviewed-By: Joshua Ashton
> ---
> .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 57 +++
> 1 file changed, 57 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> index 4a5dae578d97..f0022c16b708 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
> @@ -906,6 +906,61 @@ static int amdgpu_current_bpc_show(struct seq_file *m,
> void *data)
> }
> DEFINE_SHOW_ATTRIBUTE(amdgpu_current_bpc);
>
> +/*
> + * Returns the current bpc for the crtc.
> + * Example usage: cat
> /sys/kernel/debug/dri/0/crtc-0/amdgpu_current_colorspace
> + */
> +static int amdgpu_current_colorspace_show(struct seq_file *m, void *data)
> +{
> + struct drm_crtc *crtc = m->private;
> + struct drm_device *dev = crtc->dev;
> + struct dm_crtc_state *dm_crtc_state = NULL;
> + int res = -ENODEV;
> +
> + mutex_lock(>mode_config.mutex);
> + drm_modeset_lock(>mutex, NULL);
> + if (crtc->state == NULL)
> + goto unlock;
> +
> + dm_crtc_state = to_dm_crtc_state(crtc->state);
> + if (dm_crtc_state->stream == NULL)
> + goto unlock;
> +
> + switch (dm_crtc_state->stream->output_color_space) {
> + case COLOR_SPACE_SRGB:
> + seq_printf(m, "RGB");
> + break;
Why does it print "RGB" when it says the color space is sRGB? Looking
at the value when I didn't specify any color space it says RGB. Why is
your default color space sRGB?
> + case COLOR_SPACE_YCBCR601:
> + case COLOR_SPACE_YCBCR601_LIMITED:
> + seq_printf(m, "BT601_YCC");
> + break;
> + case COLOR_SPACE_YCBCR709:
> + case COLOR_SPACE_YCBCR709_LIMITED:
> + seq_printf(m, "BT709_YCC");
> + break;
> + case COLOR_SPACE_ADOBERGB:
> + seq_printf(m, "opRGB");
> + break;
> + case COLOR_SPACE_2020_RGB_FULLRANGE:
> + seq_printf(m, "BT2020_RGB");
> + break;
> + case COLOR_SPACE_2020_YCBCR:
> + seq_printf(m, "BT2020_YCC");
> + break;
> + default:
> + goto unlock;
> + }
> + res = 0;
> +
> +unlock:
> + drm_modeset_unlock(>mutex);
> + mutex_unlock(>mode_config.mutex);
> +
> + return res;
> +}
> +DEFINE_SHOW_ATTRIBUTE(amdgpu_current_colorspace);
> +
> +
> /*
> * Example usage:
> * Disable dsc passthrough, i.e.,: have dsc decoding at converver, not
> external RX
> @@ -3235,6 +3290,8 @@ void crtc_debugfs_init(struct drm_crtc *crtc)
> #endif
> debugfs_create_file("amdgpu_current_bpc", 0644, crtc->debugfs_entry,
> crtc, _current_bpc_fops);
> + debugfs_create_file("amdgpu_current_colorspace", 0644,
> crtc->debugfs_entry,
> + crtc, _current_colorspace_fops);
> }
>
> /*
> --
> 2.39.2
>
Re: [PATCH v3 11/17] drm/amd/display: Send correct DP colorspace infopacket
On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
>
> Look at connector->colorimetry to determine output colorspace.
>
> We don't want to impact current SDR behavior, so
> DRM_MODE_COLORIMETRY_DEFAULT preserves current behavior.
>
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Joshua Ashton
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> Reviewed-By: Joshua Ashton
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 38 +++
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 58fc719bec8d..cdfd09d50ee6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5302,21 +5302,21 @@ get_aspect_ratio(const struct drm_display_mode
> *mode_in)
> }
>
> static enum dc_color_space
> -get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing)
> +get_output_color_space(const struct dc_crtc_timing *dc_crtc_timing,
> + const struct drm_connector_state *connector_state)
> {
> enum dc_color_space color_space = COLOR_SPACE_SRGB;
>
> - switch (dc_crtc_timing->pixel_encoding) {
> - case PIXEL_ENCODING_YCBCR422:
> - case PIXEL_ENCODING_YCBCR444:
> - case PIXEL_ENCODING_YCBCR420:
> - {
> + switch (connector_state->colorspace) {
> + case DRM_MODE_COLORIMETRY_DEFAULT: // ITU601
So, I do get random behavior with DRM_MODE_COLORIMETRY_DEFAULT instead
of the colorimetry that the EDID specifies? That doesn't sound good at
all.
> + if (dc_crtc_timing->pixel_encoding == PIXEL_ENCODING_RGB) {
> + color_space = COLOR_SPACE_SRGB;
> /*
> * 27030khz is the separation point between HDTV and SDTV
> * according to HDMI spec, we use YCbCr709 and YCbCr601
> * respectively
> */
> - if (dc_crtc_timing->pix_clk_100hz > 270300) {
> + } else if (dc_crtc_timing->pix_clk_100hz > 270300) {
> if (dc_crtc_timing->flags.Y_ONLY)
> color_space =
> COLOR_SPACE_YCBCR709_LIMITED;
> @@ -5329,15 +5329,21 @@ get_output_color_space(const struct dc_crtc_timing
> *dc_crtc_timing)
> else
> color_space = COLOR_SPACE_YCBCR601;
> }
> -
> - }
> - break;
> - case PIXEL_ENCODING_RGB:
> - color_space = COLOR_SPACE_SRGB;
> break;
> -
> - default:
> - WARN_ON(1);
> + case DRM_MODE_COLORIMETRY_BT709_YCC:
> + if (dc_crtc_timing->flags.Y_ONLY)
> + color_space = COLOR_SPACE_YCBCR709_LIMITED;
> + else
> + color_space = COLOR_SPACE_YCBCR709;
> + break;
> + case DRM_MODE_COLORIMETRY_OPRGB:
> + color_space = COLOR_SPACE_ADOBERGB;
> + break;
> + case DRM_MODE_COLORIMETRY_BT2020:
> + color_space = COLOR_SPACE_2020_RGB_FULLRANGE;
> + break;
> + case DRM_MODE_COLORIMETRY_BT2020_DEPRECATED:
> + color_space = COLOR_SPACE_2020_YCBCR;
> break;
> }
>
> @@ -5476,7 +5482,7 @@ static void
> fill_stream_properties_from_drm_display_mode(
> }
> }
>
> - stream->output_color_space = get_output_color_space(timing_out);
> + stream->output_color_space = get_output_color_space(timing_out,
> connector_state);
> }
>
> static void fill_audio_info(struct audio_info *audio_info,
> --
> 2.39.2
>
RE: [PATCH 1/2] drm/amdgpu: add flag to enable/disable poll in suspend/resume path
Relying on dc_enabled will be more simple, thanks for your suggestion. I will
send v2 to address this.
Regards,
Guchun
-Original Message-
From: amd-gfx On Behalf Of Alex Deucher
Sent: Thursday, March 9, 2023 12:29 AM
To: Chen, Guchun
Cc: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
dmitry.barysh...@linaro.org; spassw...@web.de; Deucher, Alexander
; Zhang, Hawking
Subject: Re: [PATCH 1/2] drm/amdgpu: add flag to enable/disable poll in
suspend/resume path
On Wed, Mar 8, 2023 at 7:17 AM Guchun Chen wrote:
>
> Some amd asics having reliable hotplug support don't call
> drm_kms_helper_poll_init in driver init sequence. However, due to the
> unified suspend/resume path for all asics, because the
> output_poll_work->func is not set for these asics, a warning arrives
> when suspending.
>
> [ 90.656049]
> [ 90.656050] ? console_unlock+0x4d/0x100
> [ 90.656053] ? __irq_work_queue_local+0x27/0x60
> [ 90.656056] ? irq_work_queue+0x2b/0x50
> [ 90.656057] ? __wake_up_klogd+0x40/0x60
> [ 90.656059] __cancel_work_timer+0xed/0x180
> [ 90.656061] drm_kms_helper_poll_disable.cold+0x1f/0x2c [drm_kms_helper]
> [ 90.656072] amdgpu_device_suspend+0x81/0x170 [amdgpu]
> [ 90.656180] amdgpu_pmops_runtime_suspend+0xb5/0x1b0 [amdgpu]
> [ 90.656269] pci_pm_runtime_suspend+0x61/0x1b0
>
> So add use_kms_poll flag as the initialization check in amdgpu code
> before calling drm_kms_helper_poll_disable/drm_kms_helper_poll_enable
> in suspend/resume path.
>
> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/2411
> Fixes: a4e771729a51("drm/probe_helper: sort out poll_running vs
> poll_enabled")
> Reported-by: Bert Karwatzki
> Suggested-by: Dmitry Baryshkov
> Signed-off-by: Guchun Chen
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 --
> drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c | 1 +
> drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 1 +
> drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 1 +
> drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 1 +
> drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 1 +
> 7 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index c4a4e2fe6681..74af0b8c0d08 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4145,7 +4145,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
> fbcon)
> if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
> DRM_WARN("smart shift update failed\n");
>
> - drm_kms_helper_poll_disable(dev);
> + if (adev->mode_info.use_kms_poll)
> + drm_kms_helper_poll_disable(dev);
>
> if (fbcon)
>
> drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, true); @@
> -4243,7 +4244,8 @@ int amdgpu_device_resume(struct drm_device *dev, bool
> fbcon)
> if (fbcon)
>
> drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper,
> false);
>
> - drm_kms_helper_poll_enable(dev);
> + if (adev->mode_info.use_kms_poll)
> + drm_kms_helper_poll_enable(dev);
>
Since polling is only enabled for analog outputs and DC doesn't support any
analog outputs, I think we can simplify this to
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index c4a4e2fe6681..74af0b8c0d08 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4145,7 +4145,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
fbcon)
if (amdgpu_acpi_smart_shift_update(dev, AMDGPU_SS_DEV_D3))
DRM_WARN("smart shift update failed\n");
- drm_kms_helper_poll_disable(dev);
+ if (!adev->dc_enabled)
+ drm_kms_helper_poll_disable(dev);
if (fbcon)
drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, true); @@
-4243,7 +4244,8 @@ int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
if (fbcon)
drm_fb_helper_set_suspend_unlocked(adev_to_drm(adev)->fb_helper, false);
- drm_kms_helper_poll_enable(dev);
+ if (!adev->dc_enabled)
+ drm_kms_helper_poll_enable(dev);
amdgpu_ras_resume(adev);
Alternatively, we could also just move drm_kms_helper_poll_disable() into
amdgpu_display_suspend_helper() and drm_kms_helper_poll_enable() into
amdgpu_display_resume_helper(), but I'm not sure if the ordering here is
important or not off hand.
Alex
> amdgpu_ras_resume(adev);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> index 32fe05c810c6..d383ea3e8e94 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
> @@ -343,6 +343,7 @@ struct amdgpu_mode_info {
> int disp_priority;
> const struct amdgpu_display_funcs *funcs;
>
Re: [PATCH v3 05/17] drm/connector: Use common colorspace_names array
On Tue, Mar 7, 2023 at 4:12 PM Harry Wentland wrote:
>
> We an use bitfields to track the support ones for HDMI
> and DP. This allows us to print colorspaces in a consistent
> manner without needing to know whether we're dealing with
> DP or HDMI.
>
> Signed-off-by: Harry Wentland
> Cc: Pekka Paalanen
> Cc: Sebastian Wick
> Cc: vitaly.pros...@amd.com
> Cc: Uma Shankar
> Cc: Ville Syrjälä
> Cc: Joshua Ashton
> Cc: Jani Nikula
> Cc: dri-devel@lists.freedesktop.org
> Cc: amd-...@lists.freedesktop.org
> ---
> drivers/gpu/drm/drm_connector.c | 131 +++-
> include/drm/drm_connector.h | 1 +
> 2 files changed, 78 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index ff4af48c029a..7649f0ac454f 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -1012,64 +1012,70 @@ static const struct drm_prop_enum_list
> drm_dp_subconnector_enum_list[] = {
> DRM_ENUM_NAME_FN(drm_get_dp_subconnector_name,
> drm_dp_subconnector_enum_list)
>
> -static const struct drm_prop_enum_list hdmi_colorspaces[] = {
> +
> +static const char * const colorspace_names[] = {
> /* For Default case, driver will set the colorspace */
> - { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> + [DRM_MODE_COLORIMETRY_DEFAULT] = "Default",
> /* Standard Definition Colorimetry based on CEA 861 */
> - { DRM_MODE_COLORIMETRY_SMPTE_170M_YCC, "SMPTE_170M_YCC" },
> - { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> + [DRM_MODE_COLORIMETRY_SMPTE_170M_YCC] = "SMPTE_170M_YCC",
> + [DRM_MODE_COLORIMETRY_BT709_YCC] = "BT709_YCC",
> /* Standard Definition Colorimetry based on IEC 61966-2-4 */
> - { DRM_MODE_COLORIMETRY_XVYCC_601, "XVYCC_601" },
> + [DRM_MODE_COLORIMETRY_XVYCC_601] = "XVYCC_601",
> /* High Definition Colorimetry based on IEC 61966-2-4 */
> - { DRM_MODE_COLORIMETRY_XVYCC_709, "XVYCC_709" },
> + [DRM_MODE_COLORIMETRY_XVYCC_709] = "XVYCC_709",
> /* Colorimetry based on IEC 61966-2-1/Amendment 1 */
> - { DRM_MODE_COLORIMETRY_SYCC_601, "SYCC_601" },
> + [DRM_MODE_COLORIMETRY_SYCC_601] = "SYCC_601",
> /* Colorimetry based on IEC 61966-2-5 [33] */
> - { DRM_MODE_COLORIMETRY_OPYCC_601, "opYCC_601" },
> + [DRM_MODE_COLORIMETRY_OPYCC_601] = "opYCC_601",
> /* Colorimetry based on IEC 61966-2-5 */
> - { DRM_MODE_COLORIMETRY_OPRGB, "opRGB" },
> + [DRM_MODE_COLORIMETRY_OPRGB] = "opRGB",
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> + [DRM_MODE_COLORIMETRY_BT2020_CYCC] = "BT2020_CYCC",
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> + [DRM_MODE_COLORIMETRY_BT2020] = "BT2020",
> /* Colorimetry based on ITU-R BT.2020 */
> - { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
> - /* Added as part of Additional Colorimetry Extension in 861.G */
> - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> - { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED] = "BT2020_DEPRECATED",
> + /* Colorimetry based on SMPTE RP 431-2 */
> + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65] = "P3_RGB_D65",
> + [DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER] = "P3_RGB_Theater",
> + [DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED] = "RGB_WIDE_FIXED",
> + /* Colorimetry based on scRGB (IEC 61966-2-2) */
> + [DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT] = "RGB_WIDE_FLOAT",
> + [DRM_MODE_COLORIMETRY_BT601_YCC] = "BT601_YCC",
> };
>
> +static const u32 hdmi_colorspaces =
> + BIT(DRM_MODE_COLORIMETRY_SMPTE_170M_YCC) |
> + BIT(DRM_MODE_COLORIMETRY_BT709_YCC) |
> + BIT(DRM_MODE_COLORIMETRY_XVYCC_601) |
> + BIT(DRM_MODE_COLORIMETRY_XVYCC_709) |
> + BIT(DRM_MODE_COLORIMETRY_SYCC_601) |
> + BIT(DRM_MODE_COLORIMETRY_OPYCC_601) |
> + BIT(DRM_MODE_COLORIMETRY_OPRGB) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020_CYCC) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020) |
> + BIT(DRM_MODE_COLORIMETRY_BT2020_DEPRECATED) |
> + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65) |
> + BIT(DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER);
> +
> /*
> * As per DP 1.4a spec, 2.2.5.7.5 VSC SDP Payload for Pixel
> Encoding/Colorimetry
> * Format Table 2-120
> */
> -static const struct drm_prop_enum_list dp_colorspaces[] = {
> - /* For Default case, driver will set the colorspace */
> - { DRM_MODE_COLORIMETRY_DEFAULT, "Default" },
> - { DRM_MODE_COLORIMETRY_RGB_WIDE_FIXED, "RGB_Wide_Gamut_Fixed_Point" },
> - /* Colorimetry based on scRGB (IEC 61966-2-2) */
> - { DRM_MODE_COLORIMETRY_RGB_WIDE_FLOAT,
> "RGB_Wide_Gamut_Floating_Point" },
> - /* Colorimetry based on
Re: [PATCH v3 03/17] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Thu, Mar 09, 2023 at 02:05:55AM +0100, Sebastian Wick wrote:
> On Wed, Mar 8, 2023 at 10:09 AM Pekka Paalanen wrote:
> >
> > On Tue, 7 Mar 2023 10:10:53 -0500
> > Harry Wentland wrote:
> >
> > > From: Joshua Ashton
> > >
> > > Userspace has no way of controlling or knowing the pixel encoding
> > > currently, so there is no way for it to ever get the right values here.
> > >
> > > When we do add pixel_encoding control from userspace,we can pick the
> > > right value for the colorimetry packet based on the
> > > pixel_encoding + the colorspace.
> > >
> > > Let's deprecate these values, and have one BT.2020 colorspace entry
> > > that userspace can use.
> > >
> > > v2:
> > > - leave CYCC alone for now; it serves a purpose
> > > - leave BT2020_RGB the new default BT2020
> > >
> > > Signed-off-by: Joshua Ashton
> > > Signed-off-by: Harry Wentland
> > > Reviewed-by: Harry Wentland
> > >
> > > Cc: Pekka Paalanen
> > > Cc: Sebastian Wick
> > > Cc: vitaly.pros...@amd.com
> > > Cc: Uma Shankar
> > > Cc: Ville Syrjälä
> > > Cc: Joshua Ashton
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: amd-...@lists.freedesktop.org
> > > ---
> > > drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++
> > > drivers/gpu/drm/drm_connector.c | 8
> > > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
> > > include/drm/drm_connector.h | 15 +--
> > > 4 files changed, 23 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > index faf5e9efa7d3..05a0d03ffcda 100644
> > > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > @@ -97,8 +97,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> > > #define HDMI_COLORIMETRY_OPYCC_601 (C(3) | EC(3) | ACE(0))
> > > #define HDMI_COLORIMETRY_OPRGB (C(3) | EC(4) |
> > > ACE(0))
> > > #define HDMI_COLORIMETRY_BT2020_CYCC (C(3) | EC(5) | ACE(0))
> > > -#define HDMI_COLORIMETRY_BT2020_RGB (C(3) | EC(6) | ACE(0))
> > > -#define HDMI_COLORIMETRY_BT2020_YCC (C(3) | EC(6) | ACE(0))
> > > +#define HDMI_COLORIMETRY_BT2020 (C(3) | EC(6) |
> > > ACE(0))
> > > #define HDMI_COLORIMETRY_DCI_P3_RGB_D65 (C(3) | EC(7) |
> > > ACE(0))
> > > #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER (C(3) | EC(7) | ACE(1))
> > >
> > > @@ -112,8 +111,8 @@ static const u32 hdmi_colorimetry_val[] = {
> > > [DRM_MODE_COLORIMETRY_OPYCC_601] = HDMI_COLORIMETRY_OPYCC_601,
> > > [DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
> > > [DRM_MODE_COLORIMETRY_BT2020_CYCC] = HDMI_COLORIMETRY_BT2020_CYCC,
> > > - [DRM_MODE_COLORIMETRY_BT2020_RGB] = HDMI_COLORIMETRY_BT2020_RGB,
> > > - [DRM_MODE_COLORIMETRY_BT2020_YCC] = HDMI_COLORIMETRY_BT2020_YCC,
> > > + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED] = HDMI_COLORIMETRY_BT2020,
> > > + [DRM_MODE_COLORIMETRY_BT2020] = HDMI_COLORIMETRY_BT2020,
> > > };
> > >
> > > #undef C
> > > diff --git a/drivers/gpu/drm/drm_connector.c
> > > b/drivers/gpu/drm/drm_connector.c
> > > index 61c29ce74b03..fe7eab15f727 100644
> > > --- a/drivers/gpu/drm/drm_connector.c
> > > +++ b/drivers/gpu/drm/drm_connector.c
> > > @@ -1031,9 +1031,9 @@ static const struct drm_prop_enum_list
> > > hdmi_colorspaces[] = {
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > > + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > > + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
> > > /* Added as part of Additional Colorimetry Extension in 861.G */
> > > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> > > @@ -1054,7 +1054,7 @@ static const struct drm_prop_enum_list
> > > dp_colorspaces[] = {
> > > /* Colorimetry based on SMPTE RP 431-2 */
> > > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > > + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> > > { DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
> > > { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> > > /* Standard Definition Colorimetry based on IEC 61966-2-4 */
> > > @@ -1068,7 +1068,7 @@ static const struct drm_prop_enum_list
> > > dp_colorspaces[] = {
> > > /* Colorimetry based on ITU-R BT.2020 */
> > > { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > > /* Colorimetry based on ITU-R BT.2020 */
> >
Re: [PATCH] drm/rockchip: vop2: add polarity flags to RGB output
On Thu, 2 Mar 2023 13:39:49 +0100, Gerald Loacker wrote: > Use h/v-sync and pixel clock polarity flags for RGB output. For all other > outputs this is already implemented. > > Applied, thanks! [1/1] drm/rockchip: vop2: add polarity flags to RGB output commit: 66ab57574f2c8233550f641ecdc34fd0ef61341d Best regards, -- Heiko Stuebner
Re: [PATCH v6 0/4] drm/rockchip: dw_hdmi: Add 4k@30 support
On Thu, 16 Feb 2023 11:24:43 +0100, Sascha Hauer wrote: > One small fix compared to the last version, when checking for valid > resolutions I accidently compared the current mode width with the > maximum allowed height which resulted in wrong resolutions being > discarded. > > Changes since v5: > - Fix wrong check width vs. height > > [...] Applied, thanks! [1/4] drm/rockchip: vop: limit maximium resolution to hardware capabilities commit: 8e140cb60270ee8b5b41e80806323c668d8d4da9 Dropped the height check from the mode_valid function and adding a suitable comment to the commit message. [2/4] drm/rockchip: dw_hdmi: relax mode_valid hook commit: de13db32b0f89a040b50a51d129b9443159a660a [3/4] drm/rockchip: dw_hdmi: Add support for 4k@30 resolution commit: 83b61f817f43ed67572d1e241c9f552e0a8bfff4 [4/4] drm/rockchip: dw_hdmi: discard modes with unachievable pixelclocks commit: d13b10ec6696b0c523fa21b65c7ff6f246a49560 Best regards, -- Heiko Stuebner
Re: [PATCH] drm/rockchip: use struct_size() in vop2_bind
On Wed, 22 Feb 2023 17:35:33 -0800, Jacob Keller wrote: > Use the overflow-protected struct_size() helper macro to compute the > allocation size of the vop2 data structure. > > Applied, thanks! [1/1] drm/rockchip: use struct_size() in vop2_bind commit: 3b4db36c4cd9e7e49babe931d7117cdba0d04ce0 Best regards, -- Heiko Stuebner
Re: [PATCH v3 03/17] drm/connector: Deprecate split for BT.2020 in drm_colorspace enum
On Wed, Mar 8, 2023 at 10:09 AM Pekka Paalanen wrote:
>
> On Tue, 7 Mar 2023 10:10:53 -0500
> Harry Wentland wrote:
>
> > From: Joshua Ashton
> >
> > Userspace has no way of controlling or knowing the pixel encoding
> > currently, so there is no way for it to ever get the right values here.
> >
> > When we do add pixel_encoding control from userspace,we can pick the
> > right value for the colorimetry packet based on the
> > pixel_encoding + the colorspace.
> >
> > Let's deprecate these values, and have one BT.2020 colorspace entry
> > that userspace can use.
> >
> > v2:
> > - leave CYCC alone for now; it serves a purpose
> > - leave BT2020_RGB the new default BT2020
> >
> > Signed-off-by: Joshua Ashton
> > Signed-off-by: Harry Wentland
> > Reviewed-by: Harry Wentland
> >
> > Cc: Pekka Paalanen
> > Cc: Sebastian Wick
> > Cc: vitaly.pros...@amd.com
> > Cc: Uma Shankar
> > Cc: Ville Syrjälä
> > Cc: Joshua Ashton
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: amd-...@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/display/drm_hdmi_helper.c | 7 +++
> > drivers/gpu/drm/drm_connector.c | 8
> > drivers/gpu/drm/i915/display/intel_dp.c | 14 +++---
> > include/drm/drm_connector.h | 15 +--
> > 4 files changed, 23 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index faf5e9efa7d3..05a0d03ffcda 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -97,8 +97,7 @@ EXPORT_SYMBOL(drm_hdmi_infoframe_set_hdr_metadata);
> > #define HDMI_COLORIMETRY_OPYCC_601 (C(3) | EC(3) | ACE(0))
> > #define HDMI_COLORIMETRY_OPRGB (C(3) | EC(4) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_BT2020_CYCC (C(3) | EC(5) | ACE(0))
> > -#define HDMI_COLORIMETRY_BT2020_RGB (C(3) | EC(6) | ACE(0))
> > -#define HDMI_COLORIMETRY_BT2020_YCC (C(3) | EC(6) | ACE(0))
> > +#define HDMI_COLORIMETRY_BT2020 (C(3) | EC(6) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_DCI_P3_RGB_D65 (C(3) | EC(7) |
> > ACE(0))
> > #define HDMI_COLORIMETRY_DCI_P3_RGB_THEATER (C(3) | EC(7) | ACE(1))
> >
> > @@ -112,8 +111,8 @@ static const u32 hdmi_colorimetry_val[] = {
> > [DRM_MODE_COLORIMETRY_OPYCC_601] = HDMI_COLORIMETRY_OPYCC_601,
> > [DRM_MODE_COLORIMETRY_OPRGB] = HDMI_COLORIMETRY_OPRGB,
> > [DRM_MODE_COLORIMETRY_BT2020_CYCC] = HDMI_COLORIMETRY_BT2020_CYCC,
> > - [DRM_MODE_COLORIMETRY_BT2020_RGB] = HDMI_COLORIMETRY_BT2020_RGB,
> > - [DRM_MODE_COLORIMETRY_BT2020_YCC] = HDMI_COLORIMETRY_BT2020_YCC,
> > + [DRM_MODE_COLORIMETRY_BT2020_DEPRECATED] = HDMI_COLORIMETRY_BT2020,
> > + [DRM_MODE_COLORIMETRY_BT2020] = HDMI_COLORIMETRY_BT2020,
> > };
> >
> > #undef C
> > diff --git a/drivers/gpu/drm/drm_connector.c
> > b/drivers/gpu/drm/drm_connector.c
> > index 61c29ce74b03..fe7eab15f727 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -1031,9 +1031,9 @@ static const struct drm_prop_enum_list
> > hdmi_colorspaces[] = {
> > /* Colorimetry based on ITU-R BT.2020 */
> > { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
> > /* Added as part of Additional Colorimetry Extension in 861.G */
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_THEATER, "DCI-P3_RGB_Theater" },
> > @@ -1054,7 +1054,7 @@ static const struct drm_prop_enum_list
> > dp_colorspaces[] = {
> > /* Colorimetry based on SMPTE RP 431-2 */
> > { DRM_MODE_COLORIMETRY_DCI_P3_RGB_D65, "DCI-P3_RGB_D65" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_RGB, "BT2020_RGB" },
> > + { DRM_MODE_COLORIMETRY_BT2020, "BT2020" },
> > { DRM_MODE_COLORIMETRY_BT601_YCC, "BT601_YCC" },
> > { DRM_MODE_COLORIMETRY_BT709_YCC, "BT709_YCC" },
> > /* Standard Definition Colorimetry based on IEC 61966-2-4 */
> > @@ -1068,7 +1068,7 @@ static const struct drm_prop_enum_list
> > dp_colorspaces[] = {
> > /* Colorimetry based on ITU-R BT.2020 */
> > { DRM_MODE_COLORIMETRY_BT2020_CYCC, "BT2020_CYCC" },
> > /* Colorimetry based on ITU-R BT.2020 */
> > - { DRM_MODE_COLORIMETRY_BT2020_YCC, "BT2020_YCC" },
> > + { DRM_MODE_COLORIMETRY_BT2020_DEPRECATED, "BT2020_DEPRECATED" },
>
> Let's hope no-one complains about missing the old string names in UABI. :-)
>
> Actually, you should write in the commit message why
Re: [PATCH v3 02/17] drm/connector: Add enum documentation to drm_colorspace
On Wed, Mar 8, 2023 at 9:59 AM Pekka Paalanen wrote:
>
> On Tue, 7 Mar 2023 10:10:52 -0500
> Harry Wentland wrote:
>
> > From: Joshua Ashton
> >
> > To match the other enums, and add more information about these values.
> >
> > v2:
> > - Specify where an enum entry comes from
> > - Clarify DEFAULT and NO_DATA behavior
> > - BT.2020 CYCC is "constant luminance"
> > - correct type for BT.601
> >
> > Signed-off-by: Joshua Ashton
> > Signed-off-by: Harry Wentland
> > Reviewed-by: Harry Wentland
>
> Hi,
>
> this effort is really good, but of course I still find things to
> nitpick about. If there is no answer to my questions, then I would
> prefer the documentation to spell out the unknowns and ambiguities.
>
> > Cc: Pekka Paalanen
> > Cc: Sebastian Wick
> > Cc: vitaly.pros...@amd.com
> > Cc: Uma Shankar
> > Cc: Ville Syrjälä
> > Cc: Joshua Ashton
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: amd-...@lists.freedesktop.org
> > ---
> > include/drm/drm_connector.h | 67 +++--
> > 1 file changed, 65 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> > index 6d6a53a6b010..bb078666dc34 100644
> > --- a/include/drm/drm_connector.h
> > +++ b/include/drm/drm_connector.h
> > @@ -363,13 +363,76 @@ enum drm_privacy_screen_status {
> > PRIVACY_SCREEN_ENABLED_LOCKED,
> > };
> >
> > -/*
> > - * This is a consolidated colorimetry list supported by HDMI and
> > +/**
> > + * enum drm_colorspace - color space
> > + *
> > + * This enum is a consolidated colorimetry list supported by HDMI and
> > * DP protocol standard. The respective connectors will register
> > * a property with the subset of this list (supported by that
> > * respective protocol). Userspace will set the colorspace through
> > * a colorspace property which will be created and exposed to
> > * userspace.
> > + *
> > + * DP definitions come from the DP v2.0 spec
> > + * HDMI definitions come from the CTA-861-H spec
> > + *
> > + * @DRM_MODE_COLORIMETRY_DEFAULT:
> > + * Driver specific behavior.
> > + * For DP:
> > + * RGB encoded: sRGB (IEC 61966-2-1)
> > + * YCbCr encoded: ITU-R BT.601 colorimetry format
>
> Does this mean that HDMI behavior is driver-specific while DP behavior
> is as defined?
>
> Is it intentional that YCbCr encoding also uses different RGB-primaries
> than RGB-encoded signal? (BT.601 vs. BT.709/sRGB)
>
> Or do you need to be more explicit on which parts of each spec apply
> (ColourPrimaries vs. TransferCharacteristics vs. MatrixCoefficients in
> CICP parlance)?
>
> E.g. BT.709/sRGB ColourPrimaries with BT.601 MatrixCoefficients.
Yeah, just adding to this: The Default Colorspace is something well
defined. CTA-861 says:
"If bits C0 and C1 are zero, the colorimetry shall correspond to the
default colorimetry defined in Section 5.1"
and in Section 5.1
"In all cases described above, the RGB color space used should be the
RGB color space the Sink declares in the Basic Display Parameters and
Feature Block of its EDID."
If I set DRM_MODE_COLORIMETRY_DEFAULT, I expect the Colorimetry the
EDID reports to be in effect and not some driver specific nonsense.
> > + * @DRM_MODE_COLORIMETRY_NO_DATA:
> > + * Driver specific behavior.
> > + * For HDMI:
> > + * Sets "No Data" in infoframe
>
> Does DEFAULT mean that something else than "No Data" may be set in the
> HDMI infoframe?
>
> If so, since these two have the same value, where is the difference? Is
> DEFAULT purely an UAPI token, and NO_DATA used internally? Or NO_DATA
> used only when crafting actual infoframe packets?
>
> Should NO_DATA be documented to be a strictly driver-internal value,
> and not documented with UAPI?
>
> I am unclear if userspace is using these enum values directly, or do
> they use the string names only.
>
> > + * @DRM_MODE_COLORIMETRY_SMPTE_170M_YCC:
> > + * (HDMI)
> > + * SMPTE ST 170M colorimetry format
>
> Does "colorimetry format" mean that the spec is used in full, for all
> of ColourPrimaries, TransferCharacteristics and MatrixCoefficients?
>
> If yes, good. If not, the wording misleads me.
>
> > + * @DRM_MODE_COLORIMETRY_BT709_YCC:
> > + * (HDMI, DP)
> > + * ITU-R BT.709 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_XVYCC_601:
> > + * (HDMI, DP)
> > + * xvYCC601 colorimetry format
> > + * @DRM_MODE_COLORIMETRY_XVYCC_709:
> > + * (HDMI, DP)
> > + * xvYCC709 colorimetry format
>
> Btw. xvYCC are funny because they require limited quantization range
> encoding, but use the foot- and headroom to encode out-of-nominal-range
> values in order to expand the color gamut with negative and greater
> than unity values.
>
> Just for curiosity, is it in any way possible today to make use of that
> extended color gamut through KMS? Has it ever been possible?
>
> I mean, the KMS color pipeline assumes full-range RGB, so I don't see
> any way to make use of xvYCC.
>
> > + * @DRM_MODE_COLORIMETRY_SYCC_601:
> > +
Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
Hi Ville Thanks for the comments. On 3/8/2023 4:10 PM, Ville Syrjälä wrote: On Wed, Mar 08, 2023 at 03:33:48PM -0800, Rob Clark wrote: On Wed, Mar 8, 2023 at 1:23 PM Ville Syrjälä wrote: On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote: For DRM property blobs created by user mode using drm_property_create_blob(), if the blob value needs to be updated the only way is to destroy the previous blob and create a new one instead. For some of the property blobs, if the size of the blob is more than one page size, kvzalloc() can slow down system as it will first try to allocate physically contiguous memory but upon failure will fall back to non-contiguous (vmalloc) allocation. If the blob property being used is bigger than one page size, in a heavily loaded system, this causes performance issues because some of the blobs are updated on a per-frame basis. To mitigate the performance impact of kvzalloc(), use it only when the size of allocation is less than a page size when creating property blobs Not sure how badly this will eat into the vmalloc area. The reason we had the PAGE_SIZE check to use vzalloc() was specifically to limit the cases which will be affected by this. The percentage of blobs having a size more than a PAGE_SIZE will be the ones for which we will use vzalloc() which is actually good anyway since it cases of heavy memory fragmentation, kvzalloc() will fallback to vmalloc. That percentage should have been very less to begin with otherwise others would have already hit this issue and even those will only benefit from this change in our opinion. For most of the existing blobs, then this change should not affect and for those which it does, it should only benefit (like MSM). Normally I wouldn't expect this to be much of a problem, but we don't appear to restrict CREATEBLOBPROP to DRM_MASTER, which seems like it might have been a mistake.. so perhaps we want to either restrict CREATEBLOBPROP or put an upper threshold limit on total size of all allocated blob props using vmalloc area? Surprisingly few kms ioctls are master-only it seems. Dunno what the use case for all those being non-master really is. I think blob limits in general were disussed at at various points in the past with no conclusion. I guess it's slightly problematic in that if you limit individual max blob size then they just create more smaller ones. If you limit the total size per fd they just open more fds. If you put a total upper limit then it's just a slightly quicker DoS than without the limit. Shrug. BR, -R Is there no GFP flag to avoid the expensive stuff instead? Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/drm_property.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..40c2a3142038 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (!length || length > INT_MAX - sizeof(struct drm_property_blob)) return ERR_PTR(-EINVAL); - blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); + if (sizeof(struct drm_property_blob) + length > PAGE_SIZE) + blob = vzalloc(sizeof(struct drm_property_blob)+length); + else + blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); + if (!blob) return ERR_PTR(-ENOMEM); -- 2.7.4 -- Ville Syrjälä Intel
Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
On Wed, Mar 08, 2023 at 03:33:48PM -0800, Rob Clark wrote: > On Wed, Mar 8, 2023 at 1:23 PM Ville Syrjälä > wrote: > > > > On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote: > > > For DRM property blobs created by user mode using > > > drm_property_create_blob(), if the blob value needs to be updated the > > > only way is to destroy the previous blob and create a new one instead. > > > > > > For some of the property blobs, if the size of the blob is more > > > than one page size, kvzalloc() can slow down system as it will first > > > try to allocate physically contiguous memory but upon failure will > > > fall back to non-contiguous (vmalloc) allocation. > > > > > > If the blob property being used is bigger than one page size, in a > > > heavily loaded system, this causes performance issues because > > > some of the blobs are updated on a per-frame basis. > > > > > > To mitigate the performance impact of kvzalloc(), use it only when > > > the size of allocation is less than a page size when creating property > > > blobs > > > > Not sure how badly this will eat into the vmalloc area. > > Normally I wouldn't expect this to be much of a problem, but we don't > appear to restrict CREATEBLOBPROP to DRM_MASTER, which seems like it > might have been a mistake.. so perhaps we want to either restrict > CREATEBLOBPROP or put an upper threshold limit on total size of all > allocated blob props using vmalloc area? Surprisingly few kms ioctls are master-only it seems. Dunno what the use case for all those being non-master really is. I think blob limits in general were disussed at at various points in the past with no conclusion. I guess it's slightly problematic in that if you limit individual max blob size then they just create more smaller ones. If you limit the total size per fd they just open more fds. If you put a total upper limit then it's just a slightly quicker DoS than without the limit. Shrug. > > BR, > -R > > > Is there no GFP flag to avoid the expensive stuff instead? > > > > > > > > Signed-off-by: Abhinav Kumar > > > --- > > > drivers/gpu/drm/drm_property.c | 6 +- > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_property.c > > > b/drivers/gpu/drm/drm_property.c > > > index dfec479830e4..40c2a3142038 100644 > > > --- a/drivers/gpu/drm/drm_property.c > > > +++ b/drivers/gpu/drm/drm_property.c > > > @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, > > > size_t length, > > > if (!length || length > INT_MAX - sizeof(struct drm_property_blob)) > > > return ERR_PTR(-EINVAL); > > > > > > - blob = kvzalloc(sizeof(struct drm_property_blob)+length, > > > GFP_KERNEL); > > > + if (sizeof(struct drm_property_blob) + length > PAGE_SIZE) > > > + blob = vzalloc(sizeof(struct drm_property_blob)+length); > > > + else > > > + blob = kvzalloc(sizeof(struct drm_property_blob)+length, > > > GFP_KERNEL); > > > + > > > if (!blob) > > > return ERR_PTR(-ENOMEM); > > > > > > -- > > > 2.7.4 > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel
Re: [PATCH v5 3/4] drm/i915/selftests: use nop_clear_range instead of local function
Hi Andrzej, On Wed, Mar 08, 2023 at 04:39:05PM +0100, Andrzej Hajda wrote: > Since nop_clear_range is visible it can be used here. > > Signed-off-by: Andrzej Hajda Reviewed-by: Andi Shyti Thanks, Andi
Re: [PATCH v5 2/4] drm/i915/display: use nop_clear_range instead of local function
Hi Andrzej, On Wed, Mar 08, 2023 at 04:39:04PM +0100, Andrzej Hajda wrote: > Since nop_clear_range is visible it can be used here. > > Signed-off-by: Andrzej Hajda Reviewed-by: Andi Shyti Thanks, Andi
Re: [PATCH v5 1/4] drm/i915/gt: make nop_clear_range public
Hi Andrzej, On Wed, Mar 08, 2023 at 04:39:03PM +0100, Andrzej Hajda wrote: > Function nop_clear_range can be used instead of local implementations. > > Signed-off-by: Andrzej Hajda Reviewed-by: Andi Shyti Andi
Re: [PATCH v4 09/11] arm64: dts: qcom: sm8350: Add display system nodes
On 12/30/2022 7:35 AM, Robert Foss wrote:
Add mdss, mdss_mdp, dsi0, dsi0_phy nodes. With these
nodes the display subsystem is configured to support
one DSI output.
Signed-off-by: Robert Foss
Reviewed-by: Jessica Zhang
Tested-by: Jessica Zhang #SM8350 (HDK)
---
arch/arm64/boot/dts/qcom/sm8350.dtsi | 297 ++-
1 file changed, 293 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8350.dtsi
b/arch/arm64/boot/dts/qcom/sm8350.dtsi
index bdefbbb2e38f..a80c0bf6d7fd 100644
--- a/arch/arm64/boot/dts/qcom/sm8350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8350.dtsi
@@ -3,6 +3,7 @@
* Copyright (c) 2020, Linaro Limited
*/
+#include
#include
#include
#include
@@ -2535,14 +2536,302 @@ usb_2_dwc3: usb@a80 {
};
};
+ mdss: display-subsystem@ae0 {
+ compatible = "qcom,sm8350-mdss";
+ reg = <0 0x0ae0 0 0x1000>;
+ reg-names = "mdss";
+
+ interconnects = <_noc MASTER_MDP0 0 _virt
SLAVE_EBI1 0>,
+ <_noc MASTER_MDP1 0 _virt
SLAVE_EBI1 0>;
+ interconnect-names = "mdp0-mem", "mdp1-mem";
+
+ power-domains = < MDSS_GDSC>;
+ resets = < DISP_CC_MDSS_CORE_BCR>;
+
+ clocks = < DISP_CC_MDSS_AHB_CLK>,
+< GCC_DISP_HF_AXI_CLK>,
+< GCC_DISP_SF_AXI_CLK>,
+< DISP_CC_MDSS_MDP_CLK>;
+ clock-names = "iface", "bus", "nrt_bus", "core";
+
+ interrupts = ;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+
+ iommus = <_smmu 0x820 0x402>;
+
+ status = "disabled";
+
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ dpu_opp_table: opp-table {
+ compatible = "operating-points-v2";
+
+ /* TODO: opp-2 should work with
+* _opp_low_svs, but one some of
+* sm8350_hdk boards reboot using this
+* opp.
+*/
+ opp-2 {
+ opp-hz = /bits/ 64 <2>;
+ required-opps = <_opp_svs>;
+ };
+
+ opp-3 {
+ opp-hz = /bits/ 64 <3>;
+ required-opps = <_opp_svs>;
+ };
+
+ opp-34500 {
+ opp-hz = /bits/ 64 <34500>;
+ required-opps = <_opp_svs_l1>;
+ };
+
+ opp-46000 {
+ opp-hz = /bits/ 64 <46000>;
+ required-opps = <_opp_nom>;
+ };
+ };
+
+ mdss_mdp: display-controller@ae01000 {
+ compatible = "qcom,sm8350-dpu";
+ reg = <0 0x0ae01000 0 0x8f000>,
+ <0 0x0aeb 0 0x2008>;
+ reg-names = "mdp", "vbif";
+
+ clocks = < GCC_DISP_HF_AXI_CLK>,
+ < GCC_DISP_SF_AXI_CLK>,
+ < DISP_CC_MDSS_AHB_CLK>,
+ < DISP_CC_MDSS_MDP_LUT_CLK>,
+ < DISP_CC_MDSS_MDP_CLK>,
+ < DISP_CC_MDSS_VSYNC_CLK>;
+ clock-names = "bus",
+ "nrt_bus",
+ "iface",
+ "lut",
+ "core",
+ "vsync";
+
+ assigned-clocks = <
DISP_CC_MDSS_VSYNC_CLK>;
+ assigned-clock-rates = <1920>;
+
+ operating-points-v2 = <_opp_table>;
+ power-domains = < SM8350_MMCX>;
+
+ interrupt-parent = <>;
+ interrupts = <0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+
Re: [PATCH v4 11/11] arm64: dts: qcom: sm8350-hdk: Enable lt9611uxc dsi-hdmi bridge
On 12/30/2022 7:35 AM, Robert Foss wrote:
The sm8350-hdk ships with the LT9611 UXC DSI/HDMI bridge chip.
In order to toggle the board to enable the HDMI output,
switch #7 & #8 on the rightmost multi-switch package have
to be toggled to On.
Signed-off-by: Robert Foss
Reviewed-by: Jessica Zhang
Tested-by: Jessica Zhang #SM8350 (HDK)
---
arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 105
1 file changed, 105 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
index 1961f941ff83..6b21897c92dc 100644
--- a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
@@ -20,6 +20,17 @@ chosen {
stdout-path = "serial0:115200n8";
};
+ hdmi-connector {
+ compatible = "hdmi-connector";
+ type = "a";
+
+ port {
+ hdmi_con: endpoint {
+ remote-endpoint = <_out>;
+ };
+ };
+ };
+
vph_pwr: vph-pwr-regulator {
compatible = "regulator-fixed";
regulator-name = "vph_pwr";
@@ -29,6 +40,31 @@ vph_pwr: vph-pwr-regulator {
regulator-always-on;
regulator-boot-on;
};
+
+ lt9611_1v2: lt9611-1v2-regulator {
+ compatible = "regulator-fixed";
+ regulator-name = "LT9611_1V2";
+
+ vin-supply = <_pwr>;
+ regulator-min-microvolt = <120>;
+ regulator-max-microvolt = <120>;
+ gpio = < 49 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ regulator-boot-on;
+ };
+
+ lt9611_3v3: lt9611-3v3-regulator {
+ compatible = "regulator-fixed";
+ regulator-name = "LT9611_3V3";
+
+ vin-supply = <_bob>;
+ gpio = < 47 GPIO_ACTIVE_HIGH>;
+ regulator-min-microvolt = <330>;
+ regulator-max-microvolt = <330>;
+ enable-active-high;
+ regulator-boot-on;
+ regulator-always-on;
+ };
};
{
@@ -220,6 +256,15 @@ {
_dsi0 {
vdda-supply = <_l6b_1p2>;
status = "okay";
+
+ ports {
+ port@1 {
+ endpoint {
+ remote-endpoint = <_a>;
+ data-lanes = <0 1 2 3>;
+ };
+ };
+ };
};
_dsi0_phy {
@@ -231,6 +276,46 @@ _dma1 {
status = "okay";
};
+ {
+ clock-frequency = <40>;
+ status = "okay";
+
+ lt9611_codec: hdmi-bridge@2b {
+ compatible = "lontium,lt9611uxc";
+ reg = <0x2b>;
+
+ interrupts-extended = < 50 IRQ_TYPE_EDGE_FALLING>;
+ reset-gpios = < 48 GPIO_ACTIVE_HIGH>;
+
+ vdd-supply = <_1v2>;
+ vcc-supply = <_3v3>;
+
+ pinctrl-names = "default";
+ pinctrl-0 = <_state>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ lt9611_a: endpoint {
+ remote-endpoint = <_out>;
+ };
+ };
+
+ port@2 {
+ reg = <2>;
+
+ lt9611_out: endpoint {
+ remote-endpoint = <_con>;
+ };
+ };
+ };
+ };
+};
+
{
status = "okay";
};
@@ -248,6 +333,10 @@ _id_0 {
status = "okay";
};
+_id_2 {
+ status = "okay";
+};
+
{
status = "okay";
firmware-name = "qcom/sm8350/slpi.mbn";
@@ -544,4 +633,20 @@ usb_hub_enabled_state: usb-hub-enabled-state {
drive-strength = <2>;
output-low;
};
+
+ lt9611_state: lt9611-state {
+ rst {
+ pins = "gpio48";
+ function = "normal";
+
+ output-high;
+ input-disable;
+ };
+
+ irq {
+ pins = "gpio50";
+ function = "gpio";
+ bias-disable;
+ };
+ };
};
--
2.34.1
Re: [PATCH v4 10/11] arm64: dts: qcom: sm8350-hdk: Enable display & dsi nodes
On 12/30/2022 7:35 AM, Robert Foss wrote:
Enable the display subsystem and the dsi0 output for
the sm8350-hdk board.
Signed-off-by: Robert Foss
Reviewed-by: Jessica Zhang
Tested-by: Jessica Zhang #SM8350 (HDK)
---
arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 22 ++
1 file changed, 22 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
index e6deb08c6da0..1961f941ff83 100644
--- a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
@@ -213,10 +213,32 @@ {
firmware-name = "qcom/sm8350/cdsp.mbn";
};
+ {
+ status = "okay";
+};
+
+_dsi0 {
+ vdda-supply = <_l6b_1p2>;
+ status = "okay";
+};
+
+_dsi0_phy {
+ vdds-supply = <_l5b_0p88>;
+ status = "okay";
+};
+
_dma1 {
status = "okay";
};
+ {
+ status = "okay";
+};
+
+_mdp {
+ status = "okay";
+};
+
{
status = "okay";
firmware-name = "qcom/sm8350/modem.mbn";
--
2.34.1
Re: [PATCH v4 06/11] arm64: dts: qcom: sm8350: Add gpio-line-names
On 12/30/2022 7:35 AM, Robert Foss wrote:
Add GPIO line names as described by the sm8350-hdk schematic.
Signed-off-by: Robert Foss
Reviewed-by: Jessica Zhang
Tested-by: Jessica Zhang #SM8350 (HDK)
---
arch/arm64/boot/dts/qcom/sm8350-hdk.dts | 205
1 file changed, 205 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
index 0fcf5bd88fc7..e6deb08c6da0 100644
--- a/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8350-hdk.dts
@@ -233,6 +233,211 @@ {
{
gpio-reserved-ranges = <52 8>;
+
+ gpio-line-names =
+ "APPS_I2C_SDA", /* GPIO_0 */
+ "APPS_I2C_SCL",
+ "FSA_INT_N",
+ "USER_LED3_EN",
+ "SMBUS_SDA_1P8",
+ "SMBUS_SCL_1P8",
+ "2M2_3P3_EN",
+ "ALERT_DUAL_M2_N",
+ "EXP_UART_CTS",
+ "EXP_UART_RFR",
+ "EXP_UART_TX", /* GPIO_10 */
+ "EXP_UART_RX",
+ "NC",
+ "NC",
+ "RCM_MARKER1",
+ "WSA0_EN",
+ "CAM1_RESET_N",
+ "CAM0_RESET_N",
+ "DEBUG_UART_TX",
+ "DEBUG_UART_RX",
+ "TS_I2C_SDA", /* GPIO_20 */
+ "TS_I2C_SCL",
+ "TS_RESET_N",
+ "TS_INT_N",
+ "DISP0_RESET_N",
+ "DISP1_RESET_N",
+ "ETH_RESET",
+ "RCM_MARKER2",
+ "CAM_DC_MIPI_MUX_EN",
+ "CAM_DC_MIPI_MUX_SEL",
+ "AFC_PHY_TA_D_PLUS", /* GPIO_30 */
+ "AFC_PHY_TA_D_MINUS",
+ "PM8008_1_IRQ",
+ "PM8008_1_RESET_N",
+ "PM8008_2_IRQ",
+ "PM8008_2_RESET_N",
+ "CAM_DC_I3C_SDA",
+ "CAM_DC_I3C_SCL",
+ "FP_INT_N",
+ "FP_WUHB_INT_N",
+ "SMB_SPMI_DATA", /* GPIO_40 */
+ "SMB_SPMI_CLK",
+ "USB_HUB_RESET",
+ "FORCE_USB_BOOT",
+ "LRF_IRQ",
+ "NC",
+ "IMU2_INT",
+ "HDMI_3P3_EN",
+ "HDMI_RSTN",
+ "HDMI_1P2_EN",
+ "HDMI_INT", /* GPIO_50 */
+ "USB1_ID",
+ "FP_SPI_MISO",
+ "FP_SPI_MOSI",
+ "FP_SPI_CLK",
+ "FP_SPI_CS_N",
+ "NFC_ESE_SPI_MISO",
+ "NFC_ESE_SPI_MOSI",
+ "NFC_ESE_SPI_CLK",
+ "NFC_ESE_SPI_CS",
+ "NFC_I2C_SDA", /* GPIO_60 */
+ "NFC_I2C_SCLC",
+ "NFC_EN",
+ "NFC_CLK_REQ",
+ "HST_WLAN_EN",
+ "HST_BT_EN",
+ "HST_SW_CTRL",
+ "NC",
+ "HST_BT_UART_CTS",
+ "HST_BT_UART_RFR",
+ "HST_BT_UART_TX", /* GPIO_70 */
+ "HST_BT_UART_RX",
+ "CAM_DC_SPI0_MISO",
+ "CAM_DC_SPI0_MOSI",
+ "CAM_DC_SPI0_CLK",
+ "CAM_DC_SPI0_CS_N",
+ "CAM_DC_SPI1_MISO",
+ "CAM_DC_SPI1_MOSI",
+ "CAM_DC_SPI1_CLK",
+ "CAM_DC_SPI1_CS_N",
+ "HALL_INT_N", /* GPIO_80 */
+ "USB_PHY_PS",
+ "MDP_VSYNC_P",
+ "MDP_VSYNC_S",
+ "ETH_3P3_EN",
+ "RADAR_INT",
+ "NFC_DWL_REQ",
+ "SM_GPIO_87",
+ "WCD_RESET_N",
+ "ALSP_INT_N",
+ "PRESS_INT", /* GPIO_90 */
+ "SAR_INT_N",
+ "SD_CARD_DET_N",
+ "NC",
+ "PCIE0_RESET_N",
+ "PCIE0_CLK_REQ_N",
+ "PCIE0_WAKE_N",
+ "PCIE1_RESET_N",
+ "PCIE1_CLK_REQ_N",
+ "PCIE1_WAKE_N",
+ "CAM_MCLK0", /* GPIO_100 */
+ "CAM_MCLK1",
+ "CAM_MCLK2",
+ "CAM_MCLK3",
+ "CAM_MCLK4",
+ "CAM_MCLK5",
+ "CAM2_RESET_N",
+ "CCI_I2C0_SDA",
+ "CCI_I2C0_SCL",
+ "CCI_I2C1_SDA",
+ "CCI_I2C1_SCL", /* GPIO_110 */
+ "CCI_I2C2_SDA",
+ "CCI_I2C2_SCL",
+ "CCI_I2C3_SDA",
+ "CCI_I2C3_SCL",
+ "CAM5_RESET_N",
+ "CAM4_RESET_N",
+ "CAM3_RESET_N",
+ "IMU1_INT",
+ "MAG_INT_N",
+ "MI2S2_I2S_SCK", /* GPIO_120 */
+ "MI2S2_I2S_DAT0",
+ "MI2S2_I2S_WS",
+ "HIFI_DAC_I2S_MCLK",
+ "MI2S2_I2S_DAT1",
+ "HIFI_DAC_I2S_SCK",
+ "HIFI_DAC_I2S_DAT0",
+ "NC",
+ "HIFI_DAC_I2S_WS",
+ "HST_BT_WLAN_SLIMBUS_CLK",
+
Re: [PATCH v7 RESEND 3/3] drm/mediatek: Enable AR30 and BA30 overlays on MT8195
Hi, Justin:
Justin Green 於 2023年3月8日 週三 下午11:34寫道：
>
Describe more about what and why this patch does. The other
modification looks good to me.
Regards,
Chun-Kuang.
> Tested using "modetest -P" on an MT8195 device.
>
> Signed-off-by: Justin Green
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 21 +++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index a6255e847104..7d26f7055751 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -85,6 +85,22 @@ static const u32 mt8173_formats[] = {
> DRM_FORMAT_YUYV,
> };
>
> +static const u32 mt8195_formats[] = {
> + DRM_FORMAT_XRGB,
> + DRM_FORMAT_ARGB,
> + DRM_FORMAT_ARGB2101010,
> + DRM_FORMAT_BGRX,
> + DRM_FORMAT_BGRA,
> + DRM_FORMAT_BGRA1010102,
> + DRM_FORMAT_ABGR,
> + DRM_FORMAT_XBGR,
> + DRM_FORMAT_RGB888,
> + DRM_FORMAT_BGR888,
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_UYVY,
> + DRM_FORMAT_YUYV,
> +};
> +
> struct mtk_disp_ovl_data {
> unsigned int addr;
> unsigned int gmc_bits;
> @@ -616,8 +632,9 @@ static const struct mtk_disp_ovl_data
> mt8195_ovl_driver_data = {
> .fmt_rgb565_is_0 = true,
> .smi_id_en = true,
> .supports_afbc = true,
> - .formats = mt8173_formats,
> - .num_formats = ARRAY_SIZE(mt8173_formats),
> + .formats = mt8195_formats,
> + .num_formats = ARRAY_SIZE(mt8195_formats),
> + .supports_clrfmt_ext = true,
> };
>
> static const struct of_device_id mtk_disp_ovl_driver_dt_match[] = {
> --
> 2.39.1.456.gfc5497dd1b-goog
>
Re: [PATCH v7 RESEND 2/3] drm/mediatek: Add support for AR30 and BA30 overlays
Hi, Justin:
Justin Green 於 2023年3月8日 週三 下午11:34寫道：
>
> Tested using "modetest -P" on an MT8195 device.
I think you could not test this when only apply the first two patches
of this series, so move the test information to the third patch. In
this patch, you could describe more about what and why this patch
does. The other modification looks good to me.
Regards,
Chun-Kuang.
>
> Signed-off-by: Justin Green
> ---
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 33 +
> 1 file changed, 33 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 8743c8047dc9..a6255e847104 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -41,6 +41,7 @@
> #define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
> #define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
> #define DISP_REG_OVL_ADDR_MT2701 0x0040
> +#define DISP_REG_OVL_CLRFMT_EXT0x02D0
> #define DISP_REG_OVL_ADDR_MT8173 0x0f40
> #define DISP_REG_OVL_ADDR(ovl, n) ((ovl)->data->addr + 0x20 *
> (n))
> #define DISP_REG_OVL_HDR_ADDR(ovl, n) ((ovl)->data->addr + 0x20 *
> (n) + 0x04)
> @@ -61,6 +62,10 @@
> 0 : OVL_CON_CLRFMT_RGB)
> #define OVL_CON_CLRFMT_RGB888(ovl) ((ovl)->data->fmt_rgb565_is_0 ? \
> OVL_CON_CLRFMT_RGB : 0)
> +#define OVL_CON_CLRFMT_BIT_DEPTH_MASK(ovl) (0xFF << 4 * (ovl))
> +#define OVL_CON_CLRFMT_BIT_DEPTH(depth, ovl) (depth << 4 * (ovl))
> +#define OVL_CON_CLRFMT_8_BIT 0x00
> +#define OVL_CON_CLRFMT_10_BIT 0x01
> #defineOVL_CON_AEN BIT(8)
> #defineOVL_CON_ALPHA 0xff
> #defineOVL_CON_VIRT_FLIP BIT(9)
> @@ -89,6 +94,7 @@ struct mtk_disp_ovl_data {
> bool supports_afbc;
> const u32 *formats;
> size_t num_formats;
> + bool supports_clrfmt_ext;
> };
>
> /*
> @@ -218,6 +224,30 @@ static void mtk_ovl_set_afbc(struct mtk_disp_ovl *ovl,
> struct cmdq_pkt *cmdq_pkt
>DISP_REG_OVL_DATAPATH_CON, OVL_LAYER_AFBC_EN(idx));
> }
>
> +static void mtk_ovl_set_bit_depth(struct device *dev, int idx, u32 format,
> + struct cmdq_pkt *cmdq_pkt)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> + unsigned int reg;
> + unsigned int bit_depth = OVL_CON_CLRFMT_8_BIT;
> +
> + if (!ovl->data->supports_clrfmt_ext)
> + return;
> +
> + reg = readl(ovl->regs + DISP_REG_OVL_CLRFMT_EXT);
> + reg &= ~OVL_CON_CLRFMT_BIT_DEPTH_MASK(idx);
> +
> + if (format == DRM_FORMAT_RGBA1010102 ||
> + format == DRM_FORMAT_BGRA1010102 ||
> + format == DRM_FORMAT_ARGB2101010)
> + bit_depth = OVL_CON_CLRFMT_10_BIT;
> +
> + reg |= OVL_CON_CLRFMT_BIT_DEPTH(bit_depth, idx);
> +
> + mtk_ddp_write(cmdq_pkt, reg, >cmdq_reg,
> + ovl->regs, DISP_REG_OVL_CLRFMT_EXT);
> +}
> +
> void mtk_ovl_config(struct device *dev, unsigned int w,
> unsigned int h, unsigned int vrefresh,
> unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
> @@ -332,9 +362,11 @@ static unsigned int ovl_fmt_convert(struct mtk_disp_ovl
> *ovl, unsigned int fmt)
> return OVL_CON_CLRFMT_ARGB;
> case DRM_FORMAT_BGRX:
> case DRM_FORMAT_BGRA:
> + case DRM_FORMAT_BGRA1010102:
> return OVL_CON_CLRFMT_ARGB | OVL_CON_BYTE_SWAP;
> case DRM_FORMAT_XRGB:
> case DRM_FORMAT_ARGB:
> + case DRM_FORMAT_ARGB2101010:
> return OVL_CON_CLRFMT_RGBA;
> case DRM_FORMAT_XBGR:
> case DRM_FORMAT_ABGR:
> @@ -418,6 +450,7 @@ void mtk_ovl_layer_config(struct device *dev, unsigned
> int idx,
> >cmdq_reg, ovl->regs,
> DISP_REG_OVL_PITCH_MSB(idx));
> }
>
> + mtk_ovl_set_bit_depth(dev, idx, fmt, cmdq_pkt);
> mtk_ovl_layer_on(dev, idx, cmdq_pkt);
> }
>
> --
> 2.39.1.456.gfc5497dd1b-goog
>
Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
On Wed, Mar 8, 2023 at 1:23 PM Ville Syrjälä wrote: > > On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote: > > For DRM property blobs created by user mode using > > drm_property_create_blob(), if the blob value needs to be updated the > > only way is to destroy the previous blob and create a new one instead. > > > > For some of the property blobs, if the size of the blob is more > > than one page size, kvzalloc() can slow down system as it will first > > try to allocate physically contiguous memory but upon failure will > > fall back to non-contiguous (vmalloc) allocation. > > > > If the blob property being used is bigger than one page size, in a > > heavily loaded system, this causes performance issues because > > some of the blobs are updated on a per-frame basis. > > > > To mitigate the performance impact of kvzalloc(), use it only when > > the size of allocation is less than a page size when creating property > > blobs > > Not sure how badly this will eat into the vmalloc area. Normally I wouldn't expect this to be much of a problem, but we don't appear to restrict CREATEBLOBPROP to DRM_MASTER, which seems like it might have been a mistake.. so perhaps we want to either restrict CREATEBLOBPROP or put an upper threshold limit on total size of all allocated blob props using vmalloc area? BR, -R > Is there no GFP flag to avoid the expensive stuff instead? > > > > > Signed-off-by: Abhinav Kumar > > --- > > drivers/gpu/drm/drm_property.c | 6 +- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > > index dfec479830e4..40c2a3142038 100644 > > --- a/drivers/gpu/drm/drm_property.c > > +++ b/drivers/gpu/drm/drm_property.c > > @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, > > size_t length, > > if (!length || length > INT_MAX - sizeof(struct drm_property_blob)) > > return ERR_PTR(-EINVAL); > > > > - blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); > > + if (sizeof(struct drm_property_blob) + length > PAGE_SIZE) > > + blob = vzalloc(sizeof(struct drm_property_blob)+length); > > + else > > + blob = kvzalloc(sizeof(struct drm_property_blob)+length, > > GFP_KERNEL); > > + > > if (!blob) > > return ERR_PTR(-ENOMEM); > > > > -- > > 2.7.4 > > -- > Ville Syrjälä > Intel
[PATCH] drm/i915/selftests: keep same cache settings as timeline
From: Fei Yang
On MTL, objects allocated through i915_gem_object_create_internal() are
mapped as uncached in GPU by default because HAS_LLC is false. However
in the live_hwsp_read selftest these watcher objects are mapped as WB
on CPU side. The conseqence is that the updates done by the GPU are not
immediately visible to CPU, thus the selftest is randomly failing due to
the stale data in CPU cache. Solution can be either setting WC for CPU +
UC for GPU, or WB for CPU + 1-way coherent WB for GPU.
To keep the consistency, let's simply inherit the same cache settings
from the timeline, which is WB for CPU + 1-way coherent WB for GPU,
because this test is supposed to emulate the behavior of the timeline
anyway.
v2: copy cache settings from timeline instead of setting it to WC
(Suggested by Chris)
Signed-off-by: Fei Yang
Reviewed-by: Chris Wilson
Acked-by: Jonathan Cavitt
Acked-by: Matt Roper
---
drivers/gpu/drm/i915/gt/selftest_timeline.c | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/selftest_timeline.c
b/drivers/gpu/drm/i915/gt/selftest_timeline.c
index 522d0190509c..631aaed9bc3d 100644
--- a/drivers/gpu/drm/i915/gt/selftest_timeline.c
+++ b/drivers/gpu/drm/i915/gt/selftest_timeline.c
@@ -825,7 +825,8 @@ static bool cmp_gte(u32 a, u32 b)
return a >= b;
}
-static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt)
+static int setup_watcher(struct hwsp_watcher *w, struct intel_gt *gt,
+struct intel_timeline *tl)
{
struct drm_i915_gem_object *obj;
struct i915_vma *vma;
@@ -834,7 +835,10 @@ static int setup_watcher(struct hwsp_watcher *w, struct
intel_gt *gt)
if (IS_ERR(obj))
return PTR_ERR(obj);
- w->map = i915_gem_object_pin_map_unlocked(obj, I915_MAP_WB);
+ /* keep the same cache settings as timeline */
+ i915_gem_object_set_cache_coherency(obj,
tl->hwsp_ggtt->obj->cache_level);
+ w->map = i915_gem_object_pin_map_unlocked(obj,
+ page_unmask_bits(tl->hwsp_ggtt->obj->mm.mapping));
if (IS_ERR(w->map)) {
i915_gem_object_put(obj);
return PTR_ERR(w->map);
@@ -1004,8 +1008,10 @@ static int live_hwsp_read(void *arg)
if (!tl->has_initial_breadcrumb)
goto out_free;
+ selftest_tl_pin(tl);
+
for (i = 0; i < ARRAY_SIZE(watcher); i++) {
- err = setup_watcher([i], gt);
+ err = setup_watcher([i], gt, tl);
if (err)
goto out;
}
@@ -1160,6 +1166,8 @@ static int live_hwsp_read(void *arg)
for (i = 0; i < ARRAY_SIZE(watcher); i++)
cleanup_watcher([i]);
+ intel_timeline_unpin(tl);
+
if (igt_flush_test(gt->i915))
err = -EIO;
--
2.25.1
Re: [PATCH v7 RESEND 1/3] drm/mediatek: Refactor pixel format logic
Hi, Justin:
Justin Green 於 2023年3月8日 週三 下午11:34寫道：
>
> Add an DDP component interface for querying pixel format support and move list
> of supported pixel formats into DDP components instead of mtk_drm_plane.c
Reviewed-by: Chun-Kuang Hu
>
> Tested by running Chrome on an MT8195.
>
> Signed-off-by: Justin Green
> ---
> drivers/gpu/drm/mediatek/mtk_disp_drv.h | 4 ++
> drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 44 +
> drivers/gpu/drm/mediatek/mtk_disp_rdma.c| 38 ++
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 4 +-
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 4 ++
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 20 ++
> drivers/gpu/drm/mediatek/mtk_drm_plane.c| 24 ---
> drivers/gpu/drm/mediatek/mtk_drm_plane.h| 3 +-
> 8 files changed, 123 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 33e61a136bbc..0df6a06defb8 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -96,6 +96,8 @@ void mtk_ovl_register_vblank_cb(struct device *dev,
> void mtk_ovl_unregister_vblank_cb(struct device *dev);
> void mtk_ovl_enable_vblank(struct device *dev);
> void mtk_ovl_disable_vblank(struct device *dev);
> +const u32 *mtk_ovl_get_formats(struct device *dev);
> +size_t mtk_ovl_get_num_formats(struct device *dev);
>
> void mtk_rdma_bypass_shadow(struct device *dev);
> int mtk_rdma_clk_enable(struct device *dev);
> @@ -115,6 +117,8 @@ void mtk_rdma_register_vblank_cb(struct device *dev,
> void mtk_rdma_unregister_vblank_cb(struct device *dev);
> void mtk_rdma_enable_vblank(struct device *dev);
> void mtk_rdma_disable_vblank(struct device *dev);
> +const u32 *mtk_rdma_get_formats(struct device *dev);
> +size_t mtk_rdma_get_num_formats(struct device *dev);
>
> int mtk_mdp_rdma_clk_enable(struct device *dev);
> void mtk_mdp_rdma_clk_disable(struct device *dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 84daeaffab6a..8743c8047dc9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -66,6 +66,20 @@
> #defineOVL_CON_VIRT_FLIP BIT(9)
> #defineOVL_CON_HORZ_FLIP BIT(10)
>
> +static const u32 mt8173_formats[] = {
> + DRM_FORMAT_XRGB,
> + DRM_FORMAT_ARGB,
> + DRM_FORMAT_BGRX,
> + DRM_FORMAT_BGRA,
> + DRM_FORMAT_ABGR,
> + DRM_FORMAT_XBGR,
> + DRM_FORMAT_RGB888,
> + DRM_FORMAT_BGR888,
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_UYVY,
> + DRM_FORMAT_YUYV,
> +};
> +
> struct mtk_disp_ovl_data {
> unsigned int addr;
> unsigned int gmc_bits;
> @@ -73,6 +87,8 @@ struct mtk_disp_ovl_data {
> bool fmt_rgb565_is_0;
> bool smi_id_en;
> bool supports_afbc;
> + const u32 *formats;
> + size_t num_formats;
> };
>
> /*
> @@ -138,6 +154,20 @@ void mtk_ovl_disable_vblank(struct device *dev)
> writel_relaxed(0x0, ovl->regs + DISP_REG_OVL_INTEN);
> }
>
> +const u32 *mtk_ovl_get_formats(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + return ovl->data->formats;
> +}
> +
> +size_t mtk_ovl_get_num_formats(struct device *dev)
> +{
> + struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> +
> + return ovl->data->num_formats;
> +}
> +
> int mtk_ovl_clk_enable(struct device *dev)
> {
> struct mtk_disp_ovl *ovl = dev_get_drvdata(dev);
> @@ -495,6 +525,8 @@ static const struct mtk_disp_ovl_data
> mt2701_ovl_driver_data = {
> .gmc_bits = 8,
> .layer_nr = 4,
> .fmt_rgb565_is_0 = false,
> + .formats = mt8173_formats,
> + .num_formats = ARRAY_SIZE(mt8173_formats),
> };
>
> static const struct mtk_disp_ovl_data mt8173_ovl_driver_data = {
> @@ -502,6 +534,8 @@ static const struct mtk_disp_ovl_data
> mt8173_ovl_driver_data = {
> .gmc_bits = 8,
> .layer_nr = 4,
> .fmt_rgb565_is_0 = true,
> + .formats = mt8173_formats,
> + .num_formats = ARRAY_SIZE(mt8173_formats),
> };
>
> static const struct mtk_disp_ovl_data mt8183_ovl_driver_data = {
> @@ -509,6 +543,8 @@ static const struct mtk_disp_ovl_data
> mt8183_ovl_driver_data = {
> .gmc_bits = 10,
> .layer_nr = 4,
> .fmt_rgb565_is_0 = true,
> + .formats = mt8173_formats,
> + .num_formats = ARRAY_SIZE(mt8173_formats),
> };
>
> static const struct mtk_disp_ovl_data mt8183_ovl_2l_driver_data = {
> @@ -516,6 +552,8 @@ static const struct mtk_disp_ovl_data
> mt8183_ovl_2l_driver_data = {
> .gmc_bits = 10,
> .layer_nr = 2,
> .fmt_rgb565_is_0 = true,
> + .formats = mt8173_formats,
> + .num_formats = ARRAY_SIZE(mt8173_formats),
> };
>
> static const struct mtk_disp_ovl_data
Re: drivers/gpu/drm/bridge/fsl-ldb.c:101: possible loss of information.
On Tue, Mar 07, 2023 at 04:45:24PM +, David Binderman wrote:
> Hello there,
>
> I just ran the static analyser "cppcheck" over the source code of
> linux-6.2-rc1. It said:
>
> linux-6.3-rc1/drivers/gpu/drm/bridge/fsl-ldb.c:101:3: style: int
> result is returned as long value. If the return value is long to avoid
> loss of information, then you have loss of information.
> [truncLongCastReturn]
>
> Source code is
>
> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int
> clock)
> {
> if (fsl_ldb->lvds_dual_link)
> return clock * 3500;
> else
> return clock * 7000;
> }
>
> Depending on the range of the value of clock, maybe unsigned long
> literals, like 3500UL, should have been used ?
We could, but I don't think it will make any difference in practice as
the maximum pixel clock frequency supported by the SoC is 80MHz (per
LVDS channel). That would result in a 560MHz frequency returned by this
function, well below the 31 bits limit.
--
Regards,
Laurent Pinchart
Re: [PATCH v4 27/30] drm/msm/dpu: add support for wide planes
On 3/3/2023 4:57 AM, Dmitry Baryshkov wrote:
Typically SSPP can support rectangle with width up to 2560. However it's
possible to use multirect feature and split source to use the SSPP to
output two consecutive rectangles. This commit brings in this capability
to support wider screen resolutions.
Signed-off-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 19 +++-
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 125 +++---
drivers/gpu/drm/msm/disp/dpu1/dpu_plane.h | 4 +
3 files changed, 133 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e651e4593280..03034ec8ed1b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -480,6 +480,15 @@ static void _dpu_crtc_blend_setup_mixer(struct drm_crtc
*crtc,
format, fb ? fb->modifier : 0,
>pipe, 0, stage_cfg);
+ if (pstate->r_pipe.sspp) {
+ set_bit(pstate->r_pipe.sspp->idx, fetch_active);
+ _dpu_crtc_blend_setup_pipe(crtc, plane,
+ mixer, cstate->num_mixers,
+ pstate->stage,
+ format, fb ? fb->modifier :
0,
+ >pipe, 1, stage_cfg);
+ }
+
/* blend config update */
for (lm_idx = 0; lm_idx < cstate->num_mixers; lm_idx++) {
_dpu_crtc_setup_blend_cfg(mixer + lm_idx, pstate,
format);
@@ -1316,10 +1325,16 @@ static int _dpu_debugfs_status_show(struct seq_file *s,
void *data)
seq_printf(s, "\tdst x:%4d dst_y:%4d dst_w:%4d dst_h:%4d\n",
state->crtc_x, state->crtc_y, state->crtc_w,
state->crtc_h);
- seq_printf(s, "\tsspp:%s\n",
+ seq_printf(s, "\tsspp[0]:%s\n",
pstate->pipe.sspp->cap->name);
- seq_printf(s, "\tmultirect: mode: %d index: %d\n",
+ seq_printf(s, "\tmultirect[0]: mode: %d index: %d\n",
pstate->pipe.multirect_mode,
pstate->pipe.multirect_index);
+ if (pstate->r_pipe.sspp) {
+ seq_printf(s, "\tsspp[1]:%s\n",
+ pstate->r_pipe.sspp->cap->name);
+ seq_printf(s, "\tmultirect[1]: mode: %d index: %d\n",
+ pstate->r_pipe.multirect_mode,
pstate->r_pipe.multirect_index);
+ }
seq_puts(s, "\n");
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
index 3d798b939faa..06fbe5ef7ff2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c
@@ -701,6 +701,10 @@ static void _dpu_plane_color_fill(struct dpu_plane *pdpu,
/* update sspp */
_dpu_plane_color_fill_pipe(pstate, >pipe,
>pipe_cfg.dst_rect,
fill_color, fmt);
+
+ if (pstate->r_pipe.sspp)
+ _dpu_plane_color_fill_pipe(pstate, >r_pipe,
>r_pipe_cfg.dst_rect,
+ fill_color, fmt);
}
int dpu_plane_validate_multirect_v2(struct dpu_multirect_plane_states *plane)
@@ -958,9 +962,12 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
int ret = 0, min_scale;
struct dpu_plane *pdpu = to_dpu_plane(plane);
struct dpu_plane_state *pstate = to_dpu_plane_state(new_plane_state);
+ struct dpu_sw_pipe *pipe = >pipe;
+ struct dpu_sw_pipe *r_pipe = >r_pipe;
const struct drm_crtc_state *crtc_state = NULL;
const struct dpu_format *fmt;
struct dpu_sw_pipe_cfg *pipe_cfg = >pipe_cfg;
+ struct dpu_sw_pipe_cfg *r_pipe_cfg = >r_pipe_cfg;
struct drm_rect fb_rect = { 0 };
uint32_t max_linewidth;
unsigned int rotation;
@@ -984,8 +991,11 @@ static int dpu_plane_atomic_check(struct drm_plane *plane,
if (!new_plane_state->visible)
return 0;
- pstate->pipe.multirect_index = DPU_SSPP_RECT_SOLO;
- pstate->pipe.multirect_mode = DPU_SSPP_MULTIRECT_NONE;
+ pipe->multirect_index = DPU_SSPP_RECT_SOLO;
+ pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
+ r_pipe->multirect_index = DPU_SSPP_RECT_SOLO;
+ r_pipe->multirect_mode = DPU_SSPP_MULTIRECT_NONE;
+ r_pipe->sspp = NULL;
pstate->stage = DPU_STAGE_0 + pstate->base.normalized_zpos;
if (pstate->stage >= pdpu->catalog->caps->max_mixer_blendstages) {
@@ -1017,19 +1027,58 @@ static int dpu_plane_atomic_check(struct drm_plane
*plane,
max_linewidth = pdpu->catalog->caps->max_linewidth;
- /* check decimated source width */
if
Re: [PATCH v4 1/4] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On Wednesday 08 March 2023 21:01:09 Luis Chamberlain wrote: > On Wed, Mar 08, 2023 at 09:07:07PM +0800, Baoquan He wrote: > > From: Arnd Bergmann > > > > ioremap_uc() is only meaningful on old x86-32 systems with the PAT > > extension, and on ia64 with its slightly unconventional ioremap() > > behavior, everywhere else this is the same as ioremap() anyway. > > > > Change the only driver that still references ioremap_uc() to only do so > > on x86-32/ia64 in order to allow removing that interface at some > > point in the future for the other architectures. > > > > On some architectures, ioremap_uc() just returns NULL, changing > > the driver to call ioremap() means that they now have a chance > > of working correctly. > > > > Signed-off-by: Arnd Bergmann > > Signed-off-by: Baoquan He > > Cc: Helge Deller > > Cc: Thomas Zimmermann > > Cc: Christophe Leroy > > Cc: linux-fb...@vger.kernel.org > > Cc: dri-devel@lists.freedesktop.org > > Reviewed-by: Luis Chamberlain > > Is anyone using this driver these days? How often do fbdev drivers get > audited to see what can be nuked? Older servers have integrated ATI Rage XL chips and this is the only driver for it. -- Ondrej Zary
Re: [PATCH] Fixes dt-bindings: display: mediatek: Fix the fallback for mediatek,mt8186-disp-ccorr
On Mon, Mar 06, 2023 at 02:40:11PM +0100, Alexandre Mergnat wrote:
> The item which have the mediatek,mt8192-disp-ccorr const compatible already
> exist above. Remove duplicated fallback.
Drop 'Fixes ' at the beginning of the subject.
>
> Fixes: 137272ef1b0f ("dt-bindings: display: mediatek: Fix the fallback for
> mediatek,mt8186-disp-ccorr")
>
No blank line here.
> Signed-off-by: Alexandre Mergnat
> ---
> Fix MTK color correction binding
>
> The fallback compatible has been duplicated in the 137272ef1b0f commit.
>
> To: Chun-Kuang Hu
> To: Philipp Zabel
> To: David Airlie
> To: Daniel Vetter
> To: Rob Herring
> To: Krzysztof Kozlowski
> To: Matthias Brugger
> To: AngeloGioacchino Del Regno
> To: Allen-KH Cheng
> Cc: Rob Herring
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: devicet...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> ---
> Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml | 3
> ---
> 1 file changed, 3 deletions(-)
With the above,
Reviewed-by: Rob Herring
Re: [regression] RPI4B drm vc4: no crtc or sizes since 5.17 (works in 5.16; and still broken in at least 6.1)
AL13N schreef op 2023-03-08 22:16: Maxime Ripard schreef op 2023-03-08 13:35: Hi, On Tue, Mar 07, 2023 at 05:10:16PM +, Dave Stevenson wrote: On Tue, 7 Mar 2023 at 16:25, AL13N wrote: > AL13N schreef op 2023-03-06 17:34: > > I have a RPI4B connected on 2nd HDMI port (furthest away from power) > > to a 4K TV, which works until 5.16, from 5.17 there is no X (or > > plymouth), the cause of no X is that EDID gives nothing, and in the > > journal; there is "Cannot find any crct or sizes". Only the kernel is > > changed for this. > > > > In 5.16 instead of this message there is a bunch of hex lines prefixed > > with BAD. > > > > It is still broken in 6.1 at the very least. > > > > I donno if this is related to this part, but I wanted to try a newer > > kernel, because the RPI4 seems to do all the video decoding in > > software and cannot seem to handle it. > > > > > > logs: > > vc4-drm gpu: bound fef05700.hdmi (ops vc4_hdmi_ops [vc4]) > > vc4-drm gpu: bound fe004000.txp (ops vc4_txp_ops [vc4]) > > vc4-drm gpu: bound fe206000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fe207000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fe20a000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fe216000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fec12000.pixelvalve (ops vc4_crtc_ops [vc4]) > > checking generic (3ea81000 12c000) vs hw (0 ) > > fb0: switching to vc4 from simple > > Console: switching to colour dummy device 80x25 > > [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0 > > vc4-drm gpu: [drm] Cannot find any crtc or sizes > > 5.16 log has: > > vc4-drm gpu: bound fef05700.hdmi (ops vc4_hdmi_ops [vc4]) > vc4-drm gpu: bound fe004000.txp (ops vc4_txp_ops [vc4]) > vc4-drm gpu: bound fe206000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fe207000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fe20a000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fe216000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fec12000.pixelvalve (ops vc4_crtc_ops [vc4]) > [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0 > [00] BAD 00 ff ff ff ff ff ff 00 36 74 00 00 00 00 00 00 > [00] BAD 0b 1f 01 03 00 23 01 78 0a cf 74 a3 57 4c b0 23 > [00] BAD 09 48 4c 00 00 00 01 01 01 ff 01 ff ff 01 01 01 > [00] BAD 01 01 01 01 01 20 08 e8 00 30 f2 70 5a 80 b0 58 > [00] BAD 8a 00 c4 8e 21 00 00 1e 02 3a 80 18 71 38 2d 40 > [00] BAD 58 2c 45 00 c4 8e 21 00 00 1e 00 00 00 fc 00 53 > [00] BAD 41 4c 4f 52 41 0a 20 20 20 20 20 20 00 00 00 fd > [00] BAD 00 3b 46 1f 8c 3c 00 0a 20 20 20 20 20 20 01 aa > Console: switching to colour frame buffer device 240x67 > vc4-drm gpu: [drm] fb0: vc4drmfb frame buffer device > > > i donno what this bad is, but it doesn't happen in 5.17... maybe these > BAD got filtered out, but they did end up working for me? or something? > i donno... Run it through edid-decode - the checksum is wrong. Block 0, Base EDID: EDID Structure Version & Revision: 1.3 Vendor & Product Identification: Manufacturer: MST Model: 0 Made in: week 11 of 2021 Basic Display Parameters & Features: Analog display Input voltage level: 0.7/0.3 V Blank level equals black level Maximum image size: 35 cm x 1 cm Gamma: 2.20 RGB color display First detailed timing is the preferred timing Color Characteristics: Red : 0.6396, 0.3398 Green: 0.2998, 0.6904 Blue : 0.1376, 0.0380 White: 0.2822, 0.2968 Established Timings I & II: none Standard Timings: GTF : 2288x1432 61.000 Hz 16:10 90.463 kHz 282.245 MHz Detailed Timing Descriptors: DTD 1: 3840x2160 60.000 Hz 16:9 135.000 kHz 594.000 MHz (708 mm x 398 mm) Hfront 176 Hsync 88 Hback 296 Hpol P Vfront8 Vsync 10 Vback 72 Vpol P DTD 2: 1920x1080 60.000 Hz 16:967.500 kHz 148.500 MHz (708 mm x 398 mm) Hfront 88 Hsync 44 Hback 148 Hpol P Vfront4 Vsync 5 Vback 36 Vpol P Display Product Name: 'SALORA' Display Range Limits: Monitor ranges (GTF): 59-70 Hz V, 31-140 kHz H, max dotclock 600 MHz Extension blocks: 1 Checksum: 0xaa (should be 0xeb) Weird that it also says that it's an analog display when it's connected over HDMI. Something rather bizarre there, and I think it'll hit problems in drm_edid at [1] as we end up with a connector having no color_formats defined. I was discussing this with Maxime only last week, but in relation to VGA monitors connected through HDMI to VGA adapters without rewriting the EDID. If you have an issue between 5.16 and 5.17, then I'd guess at [2] and your monitor not asserting hotplug correctly. The raw hotplug status is reported in /sys/kernel/debug/dri/N/hdmi0_regs (N will be either 0 or 1 depending on the probe order of the vc4 and v3d drivers). Grep for HDMI_HOTPLUG. If it's an
Re: [PATCH] fbdev: tgafb: Fix potential divide by zero
On 3/7/23 14:08, harperchen wrote:
fb_set_var would by called when user invokes ioctl with cmd
FBIOPUT_VSCREENINFO. User-provided data would finally reach
tgafb_check_var. In case var->pixclock is assigned to zero,
divide by zero would occur when checking whether reciprocal
of var->pixclock is too high.
Similar crashes have happened in other fbdev drivers. There
is no check and modification on var->pixclock along the call
chain to tgafb_check_var. We believe it could also be triggered
in driver tgafb from user site.
Signed-off-by: harperchen
Could you provide a real name?
Otherwise applied to fbdev git tree.
Thanks!
Helge
---
drivers/video/fbdev/tgafb.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/video/fbdev/tgafb.c b/drivers/video/fbdev/tgafb.c
index 14d37c49633c..b44004880f0d 100644
--- a/drivers/video/fbdev/tgafb.c
+++ b/drivers/video/fbdev/tgafb.c
@@ -173,6 +173,9 @@ tgafb_check_var(struct fb_var_screeninfo *var, struct
fb_info *info)
{
struct tga_par *par = (struct tga_par *)info->par;
+ if (!var->pixclock)
+ return -EINVAL;
+
if (par->tga_type == TGA_TYPE_8PLANE) {
if (var->bits_per_pixel != 8)
return -EINVAL;
Re: [RFC v2 0/6] drm/amd/display: Pass proper parent for DM backlight device v2
Hi, On 3/8/23 22:58, Hans de Goede wrote: > Hi All, > > Here is version 2 of my patch series to pass the proper parent device > to backlight_device_register(). > > New in version 2 is delaying the registering of the backlight_dev till > after the drm_connector is registered by doing it from > drm_connector_funcs.late_register. > > This involves first reworking the code a bit to allow delaying > the registering, so this has turned from a single patch into > a 6 patch set. > > Regards, > > Hans p.s. Like last time this series is marked as RFC because I don't have hw to test the fix myself. The previous version was tested by 2 reporters of: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730 I hope to get test results from them for this new version soon. > > > Hans de Goede (6): > drm/amd/display/amdgpu_dm: Fix backlight_device_register() error > handling > drm/amd/display/amdgpu_dm: Refactor register_backlight_device() > drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector > drm/amd/display/amdgpu_dm: Move most backlight setup into > setup_backlight_device() > drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() > take an amdgpu_dm_connector > drm/amd/display: Pass proper parent for DM backlight device > registration v2 > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + > 2 files changed, 44 insertions(+), 56 deletions(-) >
Re: [PATCH] MAINTAINERS: orphan SIS FRAMEBUFFER DRIVER
On 3/8/23 08:19, Lukas Bulwahn wrote: This was triggered by the fact that the webpage: http://www.winischhofer.net/linuxsisvga.shtml cannot be reached anymore. Thomas Winischhofer is still reachable at the given email address, but he has not been active since 2005. Mark the SIS FRAMEBUFFER DRIVER as orphan to reflect the current state. Signed-off-by: Lukas Bulwahn applied to fbdev git tree. Thanks! Helge --- MAINTAINERS | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index 5d8f46f35aa4..354577534aef 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -19191,9 +19191,7 @@ W: http://www.brownhat.org/sis900.html F:drivers/net/ethernet/sis/sis900.* SIS FRAMEBUFFER DRIVER -M: Thomas Winischhofer -S: Maintained -W: http://www.winischhofer.net/linuxsisvga.shtml +S: Orphan F:Documentation/fb/sisfb.rst F:drivers/video/fbdev/sis/ F:include/video/sisfb.h
Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Include timeline seqno in error capture
On Wed, 2023-03-08 at 14:02 -0800, Teres Alexis, Alan Previn wrote: > On Thu, 2023-02-16 at 18:24 -0800, john.c.harri...@intel.com wrote: > > From: John Harrison > > > > The seqno value actually written out to memory is no longer in the > > regular HWSP. Instead, it is now in its own private timeline buffer. > > Thus, it is no longer visible in an error capture. So, explicitly read > > the value and include that in the capture. > > > > Signed-off-by: John Harrison > alan: snip. > > simple one ... LGTM > Reviewed-by: Alan Previn alan: i just realized i missed something. On the following hunk, seqno printout should be using a %u format specifier since we could use the upper most bit of that 32 bit value: Consider above a conditional RB (based on this fix) - sorry about that. @@ -505,6 +505,7 @@ static void error_print_context(struct drm_i915_error_state_buf *m, header, ctx->comm, ctx->pid, ctx->sched_attr.priority, ctx->guilty, ctx->active, ctx->total_runtime, ctx->avg_runtime); + err_printf(m, " context timeline seqno %d\n", ctx->hwsp_seqno);
Re: [Intel-gfx] [PATCH v2 3/3] drm/i915: Include timeline seqno in error capture
On Thu, 2023-02-16 at 18:24 -0800, john.c.harri...@intel.com wrote: > From: John Harrison > > The seqno value actually written out to memory is no longer in the > regular HWSP. Instead, it is now in its own private timeline buffer. > Thus, it is no longer visible in an error capture. So, explicitly read > the value and include that in the capture. > > Signed-off-by: John Harrison alan: snip. simple one ... LGTM Reviewed-by: Alan Previn
[RFC v2 5/6] drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() take an amdgpu_dm_connector
Make amdgpu_dm_register_backlight_device() take an amdgpu_dm_connector
pointer to the connector for which it should register the backlight
as its only argument.
This is a preparation patch for moving the actual backlight class device
registering to drm_connector_funcs.late_register.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +--
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 757202af2eec..038bf897cc28 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4140,13 +4140,15 @@ static const struct backlight_ops
amdgpu_dm_backlight_ops = {
};
static void
-amdgpu_dm_register_backlight_device(struct amdgpu_display_manager *dm)
+amdgpu_dm_register_backlight_device(struct amdgpu_dm_connector *aconnector)
{
- char bl_name[16];
+ struct drm_device *drm = aconnector->base.dev;
+ struct amdgpu_display_manager *dm = _to_adev(drm)->dm;
struct backlight_properties props = { 0 };
+ char bl_name[16];
if (!acpi_video_backlight_use_native()) {
- drm_info(adev_to_drm(dm->adev), "Skipping amdgpu DM backlight
registration\n");
+ drm_info(drm, "Skipping amdgpu DM backlight registration\n");
/* Try registering an ACPI video backlight device instead. */
acpi_video_register_backlight();
return;
@@ -4157,17 +4159,15 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
props.type = BACKLIGHT_RAW;
snprintf(bl_name, sizeof(bl_name), "amdgpu_bl%d",
-adev_to_drm(dm->adev)->primary->index + dm->num_of_edps);
+drm->primary->index + aconnector->bl_idx);
- dm->backlight_dev[dm->num_of_edps] = backlight_device_register(bl_name,
-
adev_to_drm(dm->adev)->dev,
- dm,
-
_dm_backlight_ops,
- );
+ dm->backlight_dev[aconnector->bl_idx] =
+ backlight_device_register(bl_name, drm->dev, dm,
+ _dm_backlight_ops, );
- if (IS_ERR(dm->backlight_dev[dm->num_of_edps])) {
+ if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) {
DRM_ERROR("DM: Backlight registration failed!\n");
- dm->backlight_dev[dm->num_of_edps] = NULL;
+ dm->backlight_dev[aconnector->bl_idx] = NULL;
} else
DRM_DEBUG_DRIVER("DM: Registered Backlight device: %s\n",
bl_name);
}
@@ -4233,7 +4233,7 @@ static void setup_backlight_device(struct
amdgpu_display_manager *dm,
amdgpu_dm_update_backlight_caps(dm, bl_idx);
dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL;
- amdgpu_dm_register_backlight_device(dm);
+ amdgpu_dm_register_backlight_device(aconnector);
if (!dm->backlight_dev[bl_idx]) {
aconnector->bl_idx = -1;
return;
--
2.39.1
[RFC v2 6/6] drm/amd/display: Pass proper parent for DM backlight device registration v2
The parent for the backlight device should be the drm-connector object,
not the PCI device.
Userspace relies on this to be able to detect which backlight class device
to use on hybrid gfx devices where there may be multiple native (raw)
backlight devices registered.
Specifically gnome-settings-daemon expects the parent device to have
an "enabled" sysfs attribute (as drm_connector devices do) and tests
that this returns "enabled" when read.
This aligns the parent of the backlight device with i915, nouveau, radeon.
Note that drivers/gpu/drm/amd/amdgpu/atombios_encoders.c also already
uses the drm_connector as parent, only amdgpu_dm.c used the PCI device
as parent before this change.
Changes in v2:
Together with changing the parent, also move the registration to
drm_connector_funcs.late_register() this is necessary because the parent
device (which now is the drm_connector) must be registered before
the backlight class device is, otherwise the backlight class device ends
up without any parent set at all.
This brings the backlight class device registration timing inline with
nouveau and i915 which also use drm_connector_funcs.late_register()
for this.
Note this slightly changes backlight_device_register() error handling,
instead of not increasing dm->num_of_edps and re-using the current
bl_idx for a potential other backlight device, dm->backlight_dev[bl_idx]
is now simply left NULL on failure. This is ok because all code
looking at dm->backlight_dev[i] also checks it is not NULL.
Link: https://gitlab.gnome.org/GNOME/gnome-settings-daemon/-/issues/730
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 038bf897cc28..051074d5812f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4162,7 +4162,7 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_dm_connector *aconnector)
drm->primary->index + aconnector->bl_idx);
dm->backlight_dev[aconnector->bl_idx] =
- backlight_device_register(bl_name, drm->dev, dm,
+ backlight_device_register(bl_name, aconnector->base.kdev, dm,
_dm_backlight_ops, );
if (IS_ERR(dm->backlight_dev[aconnector->bl_idx])) {
@@ -4232,13 +4232,6 @@ static void setup_backlight_device(struct
amdgpu_display_manager *dm,
amdgpu_dm_update_backlight_caps(dm, bl_idx);
dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL;
-
- amdgpu_dm_register_backlight_device(aconnector);
- if (!dm->backlight_dev[bl_idx]) {
- aconnector->bl_idx = -1;
- return;
- }
-
dm->backlight_link[bl_idx] = link;
dm->num_of_edps++;
@@ -6297,6 +6290,8 @@ amdgpu_dm_connector_late_register(struct drm_connector
*connector)
to_amdgpu_dm_connector(connector);
int r;
+ amdgpu_dm_register_backlight_device(amdgpu_dm_connector);
+
if ((connector->connector_type == DRM_MODE_CONNECTOR_DisplayPort) ||
(connector->connector_type == DRM_MODE_CONNECTOR_eDP)) {
amdgpu_dm_connector->dm_dp_aux.aux.dev = connector->kdev;
--
2.39.1
[RFC v2 3/6] drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector
Currently functions like update_connector_ext_caps() and
amdgpu_dm_connector_destroy() are iterating over dm->backlight_link[i]
to find the index of the (optional) backlight_dev associated with
the connector.
Instead make register_backlight_device() store the dm->backlight_dev[]
index used for the connector inside the amdgpu_dm_connector struct.
This removes the need to iterate over the dm->backlight_link[]
array and this is necessary as a preparation patch for moving
the actual backlight_device_register()
call to drm_connector_funcs.late_register.
While reworking update_connector_ext_caps() also remove the aconnector
and aconnector->dc_link NULL checks in this function. These are both
never NULL and are unconditionally derefed in its callers.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 42 +++
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 +
2 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 1b5efa56ec15..eb1f2073b0cf 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2936,30 +2936,18 @@ static struct drm_mode_config_helper_funcs
amdgpu_dm_mode_config_helperfuncs = {
static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
{
struct amdgpu_dm_backlight_caps *caps;
- struct amdgpu_display_manager *dm;
struct drm_connector *conn_base;
struct amdgpu_device *adev;
- struct dc_link *link = NULL;
struct drm_luminance_range_info *luminance_range;
- int i;
-
- if (!aconnector || !aconnector->dc_link)
- return;
- link = aconnector->dc_link;
- if (link->connector_signal != SIGNAL_TYPE_EDP)
+ if (aconnector->bl_idx == -1 ||
+ aconnector->dc_link->connector_signal != SIGNAL_TYPE_EDP)
return;
conn_base = >base;
adev = drm_to_adev(conn_base->dev);
- dm = >dm;
- for (i = 0; i < dm->num_of_edps; i++) {
- if (link == dm->backlight_link[i])
- break;
- }
- if (i >= dm->num_of_edps)
- return;
- caps = >backlight_caps[i];
+
+ caps = >dm.backlight_caps[aconnector->bl_idx];
caps->ext_caps = >dc_link->dpcd_sink_ext_caps;
caps->aux_support = false;
@@ -4229,8 +4217,9 @@ static int initialize_plane(struct amdgpu_display_manager
*dm,
static void register_backlight_device(struct amdgpu_display_manager *dm,
- struct dc_link *link)
+ struct amdgpu_dm_connector *aconnector)
{
+ struct dc_link *link = aconnector->dc_link;
int bl_idx = dm->num_of_edps;
if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) ||
@@ -4242,9 +4231,13 @@ static void register_backlight_device(struct
amdgpu_display_manager *dm,
return;
}
+ aconnector->bl_idx = bl_idx;
+
amdgpu_dm_register_backlight_device(dm);
- if (!dm->backlight_dev[bl_idx])
+ if (!dm->backlight_dev[bl_idx]) {
+ aconnector->bl_idx = -1;
return;
+ }
dm->backlight_link[bl_idx] = link;
dm->num_of_edps++;
@@ -4430,7 +4423,7 @@ static int amdgpu_dm_initialize_drm_device(struct
amdgpu_device *adev)
if (ret) {
amdgpu_dm_update_connector_after_detect(aconnector);
- register_backlight_device(dm, link);
+ register_backlight_device(dm, aconnector);
if (dm->num_of_edps)
update_connector_ext_caps(aconnector);
@@ -6211,10 +6204,8 @@ static void amdgpu_dm_connector_unregister(struct
drm_connector *connector)
static void amdgpu_dm_connector_destroy(struct drm_connector *connector)
{
struct amdgpu_dm_connector *aconnector =
to_amdgpu_dm_connector(connector);
- const struct dc_link *link = aconnector->dc_link;
struct amdgpu_device *adev = drm_to_adev(connector->dev);
struct amdgpu_display_manager *dm = >dm;
- int i;
/*
* Call only if mst_mgr was initialized before since it's not done
@@ -6223,11 +6214,9 @@ static void amdgpu_dm_connector_destroy(struct
drm_connector *connector)
if (aconnector->mst_mgr.dev)
drm_dp_mst_topology_mgr_destroy(>mst_mgr);
- for (i = 0; i < dm->num_of_edps; i++) {
- if ((link == dm->backlight_link[i]) && dm->backlight_dev[i]) {
- backlight_device_unregister(dm->backlight_dev[i]);
- dm->backlight_dev[i] = NULL;
- }
+ if (aconnector->bl_idx != -1) {
+
[RFC v2 4/6] drm/amd/display/amdgpu_dm: Move most backlight setup into setup_backlight_device()
Rename register_backlight_device() to setup_backlight_device()
and move all backlight setup related calls from
amdgpu_dm_register_backlight_device() and from
amdgpu_dm_initialize_drm_device() there.
This leaves amdgpu_dm_register_backlight_device() dealing purely
with registering the actual backlight class device.
This is a preparation patch for moving the actual backlight class device
registering to drm_connector_funcs.late_register.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 -
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index eb1f2073b0cf..757202af2eec 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4145,9 +4145,6 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
char bl_name[16];
struct backlight_properties props = { 0 };
- amdgpu_dm_update_backlight_caps(dm, dm->num_of_edps);
- dm->brightness[dm->num_of_edps] = AMDGPU_MAX_BL_LEVEL;
-
if (!acpi_video_backlight_use_native()) {
drm_info(adev_to_drm(dm->adev), "Skipping amdgpu DM backlight
registration\n");
/* Try registering an ACPI video backlight device instead. */
@@ -4216,8 +4213,8 @@ static int initialize_plane(struct amdgpu_display_manager
*dm,
}
-static void register_backlight_device(struct amdgpu_display_manager *dm,
- struct amdgpu_dm_connector *aconnector)
+static void setup_backlight_device(struct amdgpu_display_manager *dm,
+ struct amdgpu_dm_connector *aconnector)
{
struct dc_link *link = aconnector->dc_link;
int bl_idx = dm->num_of_edps;
@@ -4233,6 +4230,9 @@ static void register_backlight_device(struct
amdgpu_display_manager *dm,
aconnector->bl_idx = bl_idx;
+ amdgpu_dm_update_backlight_caps(dm, bl_idx);
+ dm->brightness[bl_idx] = AMDGPU_MAX_BL_LEVEL;
+
amdgpu_dm_register_backlight_device(dm);
if (!dm->backlight_dev[bl_idx]) {
aconnector->bl_idx = -1;
@@ -4241,6 +4241,8 @@ static void register_backlight_device(struct
amdgpu_display_manager *dm,
dm->backlight_link[bl_idx] = link;
dm->num_of_edps++;
+
+ update_connector_ext_caps(aconnector);
}
static void amdgpu_set_panel_orientation(struct drm_connector *connector);
@@ -4423,10 +4425,7 @@ static int amdgpu_dm_initialize_drm_device(struct
amdgpu_device *adev)
if (ret) {
amdgpu_dm_update_connector_after_detect(aconnector);
- register_backlight_device(dm, aconnector);
-
- if (dm->num_of_edps)
- update_connector_ext_caps(aconnector);
+ setup_backlight_device(dm, aconnector);
if (psr_feature_enabled)
amdgpu_dm_set_psr_caps(link);
--
2.39.1
[RFC v2 2/6] drm/amd/display/amdgpu_dm: Refactor register_backlight_device()
Refactor register_backlight_device():
1) Turn the connector-type + signal check into an early exit
condition to avoid the indentation level of the rest of the code
2) Add an array bounds check for the arrays indexed by dm->num_of_edps
3) register_backlight_device() always increases dm->num_of_edps if
amdgpu_dm_register_backlight_device() has assigned a backlight_dev to
the current dm->backlight_link[dm->num_of_edps] slot.
So on its next call dm->backlight_dev[dm->num_of_edps] always point to
the next empty slot and the "if (!dm->backlight_dev[dm->num_of_edps])"
check will thus always succeed and can be removed.
4) Add a bl_idx local variable to use as array index, rather then
using dm->num_of_edps to improve the code readability.
Signed-off-by: Hans de Goede
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 ++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 42b88ab5552d..1b5efa56ec15 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4231,21 +4231,23 @@ static int initialize_plane(struct
amdgpu_display_manager *dm,
static void register_backlight_device(struct amdgpu_display_manager *dm,
struct dc_link *link)
{
- if ((link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) &&
- link->type != dc_connection_none) {
- /*
-* Event if registration failed, we should continue with
-* DM initialization because not having a backlight control
-* is better then a black screen.
-*/
- if (!dm->backlight_dev[dm->num_of_edps])
- amdgpu_dm_register_backlight_device(dm);
+ int bl_idx = dm->num_of_edps;
- if (dm->backlight_dev[dm->num_of_edps]) {
- dm->backlight_link[dm->num_of_edps] = link;
- dm->num_of_edps++;
- }
+ if (!(link->connector_signal & (SIGNAL_TYPE_EDP | SIGNAL_TYPE_LVDS)) ||
+ link->type == dc_connection_none)
+ return;
+
+ if (dm->num_of_edps >= AMDGPU_DM_MAX_NUM_EDP) {
+ drm_warn(adev_to_drm(dm->adev), "Too much eDP connections,
skipping backlight setup for additional eDPs\n");
+ return;
}
+
+ amdgpu_dm_register_backlight_device(dm);
+ if (!dm->backlight_dev[bl_idx])
+ return;
+
+ dm->backlight_link[bl_idx] = link;
+ dm->num_of_edps++;
}
static void amdgpu_set_panel_orientation(struct drm_connector *connector);
--
2.39.1
[RFC v2 1/6] drm/amd/display/amdgpu_dm: Fix backlight_device_register() error handling
backlight_device_register() returns an ERR_PTR on error, but other code
such as amdgpu_dm_connector_destroy() assumes dm->backlight_dev[i] is NULL
if no backlight is registered.
Clear dm->backlight_dev[i] on registration failure, to avoid other code
trying to deref an ERR_PTR pointer.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 009ef917dad4..42b88ab5552d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4180,9 +4180,10 @@ amdgpu_dm_register_backlight_device(struct
amdgpu_display_manager *dm)
_dm_backlight_ops,
);
- if (IS_ERR(dm->backlight_dev[dm->num_of_edps]))
+ if (IS_ERR(dm->backlight_dev[dm->num_of_edps])) {
DRM_ERROR("DM: Backlight registration failed!\n");
- else
+ dm->backlight_dev[dm->num_of_edps] = NULL;
+ } else
DRM_DEBUG_DRIVER("DM: Registered Backlight device: %s\n",
bl_name);
}
--
2.39.1
[RFC v2 0/6] drm/amd/display: Pass proper parent for DM backlight device v2
Hi All, Here is version 2 of my patch series to pass the proper parent device to backlight_device_register(). New in version 2 is delaying the registering of the backlight_dev till after the drm_connector is registered by doing it from drm_connector_funcs.late_register. This involves first reworking the code a bit to allow delaying the registering, so this has turned from a single patch into a 6 patch set. Regards, Hans Hans de Goede (6): drm/amd/display/amdgpu_dm: Fix backlight_device_register() error handling drm/amd/display/amdgpu_dm: Refactor register_backlight_device() drm/amd/display/amdgpu_dm: Add a bl_idx to amdgpu_dm_connector drm/amd/display/amdgpu_dm: Move most backlight setup into setup_backlight_device() drm/amd/display/amdgpu_dm: Make amdgpu_dm_register_backlight_device() take an amdgpu_dm_connector drm/amd/display: Pass proper parent for DM backlight device registration v2 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 99 --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 + 2 files changed, 44 insertions(+), 56 deletions(-) -- 2.39.1
Re: [PATCH v4 1/4] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On 3/8/23 22:34, Arnd Bergmann wrote: 0On Wed, Mar 8, 2023, at 21:01, Luis Chamberlain wrote: On Wed, Mar 08, 2023 at 09:07:07PM +0800, Baoquan He wrote: From: Arnd Bergmann ioremap_uc() is only meaningful on old x86-32 systems with the PAT extension, and on ia64 with its slightly unconventional ioremap() behavior, everywhere else this is the same as ioremap() anyway. Change the only driver that still references ioremap_uc() to only do so on x86-32/ia64 in order to allow removing that interface at some point in the future for the other architectures. On some architectures, ioremap_uc() just returns NULL, changing the driver to call ioremap() means that they now have a chance of working correctly. Signed-off-by: Arnd Bergmann Signed-off-by: Baoquan He Cc: Helge Deller Cc: Thomas Zimmermann Cc: Christophe Leroy Cc: linux-fb...@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Reviewed-by: Luis Chamberlain Is anyone using this driver these days? How often do fbdev drivers get audited to see what can be nuked? Geert already mentioned that this one is likely used on old powermac systems. and the latest generation of parisc machines use it too. In addition, on parisc machines it's also important to map all io-space memory uncacheable. Since ioremap() takes care of it anyway, the ioremap_uc() was simply referencing the call to ioremap(). Helge I think my arm boardfile removal orphaned some other fbdev drivers though. I removed the ones that can no longer be enabled, but think a bunch of other ones are still selectable but have no platform_device definition or DT support: FB_PXA168, FB_DA8XX, FB_MX3, and MMP_FB. These four platforms are all still supported with DT, but over time it gets less likely that anyone is still interested in adding DT support to the fbdev drivers. Arnd
[PATCH] drm/amdkfd: fix potential kgd_mem UAFs
kgd_mem should be accessed with p->mutex locked, or it could have been
freed by kfd_ioctl_free_memory_of_gpu.
Signed-off-by: Chia-I Wu
---
drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
index 6d291aa6386bd..3c630114210d6 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
@@ -1293,14 +1293,14 @@ static int kfd_ioctl_map_memory_to_gpu(struct file
*filep,
args->n_success = i+1;
}
- mutex_unlock(>mutex);
-
err = amdgpu_amdkfd_gpuvm_sync_memory(dev->adev, (struct kgd_mem *)
mem, true);
if (err) {
pr_debug("Sync memory failed, wait interrupted by user
signal\n");
goto sync_memory_failed;
}
+ mutex_unlock(>mutex);
+
/* Flush TLBs after waiting for the page table updates to complete */
for (i = 0; i < args->n_devices; i++) {
peer_pdd = kfd_process_device_data_by_id(p, devices_arr[i]);
@@ -1316,9 +1316,9 @@ static int kfd_ioctl_map_memory_to_gpu(struct file *filep,
bind_process_to_device_failed:
get_mem_obj_from_handle_failed:
map_memory_to_gpu_failed:
+sync_memory_failed:
mutex_unlock(>mutex);
copy_from_user_failed:
-sync_memory_failed:
kfree(devices_arr);
return err;
@@ -1332,6 +1332,7 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file
*filep,
void *mem;
long err = 0;
uint32_t *devices_arr = NULL, i;
+ bool flush_tlb;
if (!args->n_devices) {
pr_debug("Device IDs array empty\n");
@@ -1384,16 +1385,19 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file
*filep,
}
args->n_success = i+1;
}
- mutex_unlock(>mutex);
- if (kfd_flush_tlb_after_unmap(pdd->dev)) {
+ flush_tlb = kfd_flush_tlb_after_unmap(pdd->dev);
+ if (flush_tlb) {
err = amdgpu_amdkfd_gpuvm_sync_memory(pdd->dev->adev,
(struct kgd_mem *) mem, true);
if (err) {
pr_debug("Sync memory failed, wait interrupted by user
signal\n");
goto sync_memory_failed;
}
+ }
+ mutex_unlock(>mutex);
+ if (flush_tlb) {
/* Flush TLBs after waiting for the page table updates to
complete */
for (i = 0; i < args->n_devices; i++) {
peer_pdd = kfd_process_device_data_by_id(p,
devices_arr[i]);
@@ -1409,9 +1413,9 @@ static int kfd_ioctl_unmap_memory_from_gpu(struct file
*filep,
bind_process_to_device_failed:
get_mem_obj_from_handle_failed:
unmap_memory_from_gpu_failed:
+sync_memory_failed:
mutex_unlock(>mutex);
copy_from_user_failed:
-sync_memory_failed:
kfree(devices_arr);
return err;
}
--
2.40.0.rc1.284.g88254d51c5-goog
Re: [PATCH v4 1/4] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
0On Wed, Mar 8, 2023, at 21:01, Luis Chamberlain wrote: > On Wed, Mar 08, 2023 at 09:07:07PM +0800, Baoquan He wrote: >> From: Arnd Bergmann >> >> ioremap_uc() is only meaningful on old x86-32 systems with the PAT >> extension, and on ia64 with its slightly unconventional ioremap() >> behavior, everywhere else this is the same as ioremap() anyway. >> >> Change the only driver that still references ioremap_uc() to only do so >> on x86-32/ia64 in order to allow removing that interface at some >> point in the future for the other architectures. >> >> On some architectures, ioremap_uc() just returns NULL, changing >> the driver to call ioremap() means that they now have a chance >> of working correctly. >> >> Signed-off-by: Arnd Bergmann >> Signed-off-by: Baoquan He >> Cc: Helge Deller >> Cc: Thomas Zimmermann >> Cc: Christophe Leroy >> Cc: linux-fb...@vger.kernel.org >> Cc: dri-devel@lists.freedesktop.org > > Reviewed-by: Luis Chamberlain > > Is anyone using this driver these days? How often do fbdev drivers get > audited to see what can be nuked? Geert already mentioned that this one is likely used on old powermac systems. I think my arm boardfile removal orphaned some other fbdev drivers though. I removed the ones that can no longer be enabled, but think a bunch of other ones are still selectable but have no platform_device definition or DT support: FB_PXA168, FB_DA8XX, FB_MX3, and MMP_FB. These four platforms are all still supported with DT, but over time it gets less likely that anyone is still interested in adding DT support to the fbdev drivers. Arnd
Re: [RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
On Wed, Mar 08, 2023 at 12:02:42PM -0800, Abhinav Kumar wrote: > For DRM property blobs created by user mode using > drm_property_create_blob(), if the blob value needs to be updated the > only way is to destroy the previous blob and create a new one instead. > > For some of the property blobs, if the size of the blob is more > than one page size, kvzalloc() can slow down system as it will first > try to allocate physically contiguous memory but upon failure will > fall back to non-contiguous (vmalloc) allocation. > > If the blob property being used is bigger than one page size, in a > heavily loaded system, this causes performance issues because > some of the blobs are updated on a per-frame basis. > > To mitigate the performance impact of kvzalloc(), use it only when > the size of allocation is less than a page size when creating property > blobs Not sure how badly this will eat into the vmalloc area. Is there no GFP flag to avoid the expensive stuff instead? > > Signed-off-by: Abhinav Kumar > --- > drivers/gpu/drm/drm_property.c | 6 +- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > index dfec479830e4..40c2a3142038 100644 > --- a/drivers/gpu/drm/drm_property.c > +++ b/drivers/gpu/drm/drm_property.c > @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t > length, > if (!length || length > INT_MAX - sizeof(struct drm_property_blob)) > return ERR_PTR(-EINVAL); > > - blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); > + if (sizeof(struct drm_property_blob) + length > PAGE_SIZE) > + blob = vzalloc(sizeof(struct drm_property_blob)+length); > + else > + blob = kvzalloc(sizeof(struct drm_property_blob)+length, > GFP_KERNEL); > + > if (!blob) > return ERR_PTR(-ENOMEM); > > -- > 2.7.4 -- Ville Syrjälä Intel
Re: [regression] RPI4B drm vc4: no crtc or sizes since 5.17 (works in 5.16; and still broken in at least 6.1)
Maxime Ripard schreef op 2023-03-08 13:35: Hi, On Tue, Mar 07, 2023 at 05:10:16PM +, Dave Stevenson wrote: On Tue, 7 Mar 2023 at 16:25, AL13N wrote: > AL13N schreef op 2023-03-06 17:34: > > I have a RPI4B connected on 2nd HDMI port (furthest away from power) > > to a 4K TV, which works until 5.16, from 5.17 there is no X (or > > plymouth), the cause of no X is that EDID gives nothing, and in the > > journal; there is "Cannot find any crct or sizes". Only the kernel is > > changed for this. > > > > In 5.16 instead of this message there is a bunch of hex lines prefixed > > with BAD. > > > > It is still broken in 6.1 at the very least. > > > > I donno if this is related to this part, but I wanted to try a newer > > kernel, because the RPI4 seems to do all the video decoding in > > software and cannot seem to handle it. > > > > > > logs: > > vc4-drm gpu: bound fef05700.hdmi (ops vc4_hdmi_ops [vc4]) > > vc4-drm gpu: bound fe004000.txp (ops vc4_txp_ops [vc4]) > > vc4-drm gpu: bound fe206000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fe207000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fe20a000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fe216000.pixelvalve (ops vc4_crtc_ops [vc4]) > > vc4-drm gpu: bound fec12000.pixelvalve (ops vc4_crtc_ops [vc4]) > > checking generic (3ea81000 12c000) vs hw (0 ) > > fb0: switching to vc4 from simple > > Console: switching to colour dummy device 80x25 > > [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0 > > vc4-drm gpu: [drm] Cannot find any crtc or sizes > > 5.16 log has: > > vc4-drm gpu: bound fef05700.hdmi (ops vc4_hdmi_ops [vc4]) > vc4-drm gpu: bound fe004000.txp (ops vc4_txp_ops [vc4]) > vc4-drm gpu: bound fe206000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fe207000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fe20a000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fe216000.pixelvalve (ops vc4_crtc_ops [vc4]) > vc4-drm gpu: bound fec12000.pixelvalve (ops vc4_crtc_ops [vc4]) > [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0 > [00] BAD 00 ff ff ff ff ff ff 00 36 74 00 00 00 00 00 00 > [00] BAD 0b 1f 01 03 00 23 01 78 0a cf 74 a3 57 4c b0 23 > [00] BAD 09 48 4c 00 00 00 01 01 01 ff 01 ff ff 01 01 01 > [00] BAD 01 01 01 01 01 20 08 e8 00 30 f2 70 5a 80 b0 58 > [00] BAD 8a 00 c4 8e 21 00 00 1e 02 3a 80 18 71 38 2d 40 > [00] BAD 58 2c 45 00 c4 8e 21 00 00 1e 00 00 00 fc 00 53 > [00] BAD 41 4c 4f 52 41 0a 20 20 20 20 20 20 00 00 00 fd > [00] BAD 00 3b 46 1f 8c 3c 00 0a 20 20 20 20 20 20 01 aa > Console: switching to colour frame buffer device 240x67 > vc4-drm gpu: [drm] fb0: vc4drmfb frame buffer device > > > i donno what this bad is, but it doesn't happen in 5.17... maybe these > BAD got filtered out, but they did end up working for me? or something? > i donno... Run it through edid-decode - the checksum is wrong. Block 0, Base EDID: EDID Structure Version & Revision: 1.3 Vendor & Product Identification: Manufacturer: MST Model: 0 Made in: week 11 of 2021 Basic Display Parameters & Features: Analog display Input voltage level: 0.7/0.3 V Blank level equals black level Maximum image size: 35 cm x 1 cm Gamma: 2.20 RGB color display First detailed timing is the preferred timing Color Characteristics: Red : 0.6396, 0.3398 Green: 0.2998, 0.6904 Blue : 0.1376, 0.0380 White: 0.2822, 0.2968 Established Timings I & II: none Standard Timings: GTF : 2288x1432 61.000 Hz 16:10 90.463 kHz 282.245 MHz Detailed Timing Descriptors: DTD 1: 3840x2160 60.000 Hz 16:9 135.000 kHz 594.000 MHz (708 mm x 398 mm) Hfront 176 Hsync 88 Hback 296 Hpol P Vfront8 Vsync 10 Vback 72 Vpol P DTD 2: 1920x1080 60.000 Hz 16:967.500 kHz 148.500 MHz (708 mm x 398 mm) Hfront 88 Hsync 44 Hback 148 Hpol P Vfront4 Vsync 5 Vback 36 Vpol P Display Product Name: 'SALORA' Display Range Limits: Monitor ranges (GTF): 59-70 Hz V, 31-140 kHz H, max dotclock 600 MHz Extension blocks: 1 Checksum: 0xaa (should be 0xeb) Weird that it also says that it's an analog display when it's connected over HDMI. Something rather bizarre there, and I think it'll hit problems in drm_edid at [1] as we end up with a connector having no color_formats defined. I was discussing this with Maxime only last week, but in relation to VGA monitors connected through HDMI to VGA adapters without rewriting the EDID. If you have an issue between 5.16 and 5.17, then I'd guess at [2] and your monitor not asserting hotplug correctly. The raw hotplug status is reported in /sys/kernel/debug/dri/N/hdmi0_regs (N will be either 0 or 1 depending on the probe order of the vc4 and v3d drivers). Grep for HDMI_HOTPLUG. If it's an option, bisecting between 5.16 and 5.17 which
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 20:45 schrieb Asahi Lina: On 09/03/2023 04.12, Christian König wrote: Am 08.03.23 um 20:05 schrieb Asahi Lina: [SNIP] Well it's not the better way, it's the only way that works. I have to admit that my bet on your intentions was wrong, but even that use case doesn't work correctly. See when your callback returns false it is perfectly possible that all hw fences are signaled between returning that information and processing it. The result would be that the scheduler goes to sleep and never wakes up again. That can't happen, because it will just go into another iteration of the drm_sched main loop since there is an entity available still. Rather there is probably the opposite bug in this patch: the can_run_job logic should be moved into the wait_event_interruptible() condition check, otherwise I think it can end up busy-looping since the condition itself can be true even when the can_run_job check blocks it. But there is no risk of it going to sleep and never waking up because job completions will wake up the waitqueue by definition, and that happens after the driver-side queues are popped. If this problem could happen, then the existing hw_submission_limit logic would be broken in the same way. It is logically equivalent in how it works. Basically, if properly done in wait_event_interruptible, it is exactly the logic of that macro that prevents this race condition and makes everything work at all. Without it, drm_sched would be completely broken. As I said we exercised those ideas before and yes this approach here came up before as well and no it doesn't work. It can never deadlock with this patch as it stands (though it could busy loop), and if properly moved into the wait_event_interruptible(), it would also never busy loop and work entirely as intended. The actual API change is sound. I don't know why you're trying so hard to convince everyone that this approach is fundamentally broken... It might be a bad idea for other reasons, it might encourage incorrect usage, it might not be the best option, there are plenty of arguments you can make... but you just keep trying to make an argument that it just can't work at all for some reason. Why? I already said I'm happy dropping it in favor of the fences... Well because it is broken. When you move the check into the wait_event_interruptible condition then who is going to call wait_event_interruptible when the condition changes? I think you mean wake_up_interruptible(). That would be drm_sched_job_done(), on the fence callback when a job completes, which as I keep saying is the same logic used for hw_rq_count/hw_submission_limit tracking. As the documentation to wait_event says: * wake_up() has to be called after changing any variable that could * change the result of the wait condition. So what you essentially try to do here is to skip that and say drm_sched_job_done() would call that anyway, but when you read any variable to determine that state then as far as I can see nothing is guarantying that order. The only other possibility how you could use the callback correctly would be to call drm_fence_is_signaled() to query the state of your hw submission from the same fence which is then signaled. But then the question is once more why you don't give that fence directly to the scheduler? Please think about it for a second, Yeah, I'm trying to really follow your intentions here. But that doesn't really makes sense. Either you are trying to do something invalid or you are trying to circumvent the object model somehow and add a shortcut for the signaling API. Both would be more than fishy. Regards, Christian. it's really not that complicated to see why it works: - Driver pops off completed commands <-- can_run_job condition satisfied - Driver signals fence - drm_sched_job_done_cb() - drm_sched_job_done() - atomic_dec(>hw_rq_count); <-- hw_submission_limit satisfied - ... - wake_up_interruptible(>wake_up_worker); ^- happens after both conditions are potentially satisfied It really is completely equivalent to just making the hw_rq_count logic customizable by the driver. The actual flow is the same. As long as the driver guarantees it satisfies the can_run_job() condition before signaling the completion fence that triggered that change, it works fine. As I said this idea came up before and was rejected multiple times. Maybe it was a different idea, or maybe it was rejected for other reasons, or maybe it was wrongly rejected for being broken when it isn't ^^ ~~ Lina
[RFC] drm: property: use vzalloc() instead of kvzalloc() for large blobs
For DRM property blobs created by user mode using drm_property_create_blob(), if the blob value needs to be updated the only way is to destroy the previous blob and create a new one instead. For some of the property blobs, if the size of the blob is more than one page size, kvzalloc() can slow down system as it will first try to allocate physically contiguous memory but upon failure will fall back to non-contiguous (vmalloc) allocation. If the blob property being used is bigger than one page size, in a heavily loaded system, this causes performance issues because some of the blobs are updated on a per-frame basis. To mitigate the performance impact of kvzalloc(), use it only when the size of allocation is less than a page size when creating property blobs Signed-off-by: Abhinav Kumar --- drivers/gpu/drm/drm_property.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..40c2a3142038 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -561,7 +561,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (!length || length > INT_MAX - sizeof(struct drm_property_blob)) return ERR_PTR(-EINVAL); - blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); + if (sizeof(struct drm_property_blob) + length > PAGE_SIZE) + blob = vzalloc(sizeof(struct drm_property_blob)+length); + else + blob = kvzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); + if (!blob) return ERR_PTR(-ENOMEM); -- 2.7.4
Re: [PATCH v4 1/4] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On Wed, Mar 08, 2023 at 09:07:07PM +0800, Baoquan He wrote: > From: Arnd Bergmann > > ioremap_uc() is only meaningful on old x86-32 systems with the PAT > extension, and on ia64 with its slightly unconventional ioremap() > behavior, everywhere else this is the same as ioremap() anyway. > > Change the only driver that still references ioremap_uc() to only do so > on x86-32/ia64 in order to allow removing that interface at some > point in the future for the other architectures. > > On some architectures, ioremap_uc() just returns NULL, changing > the driver to call ioremap() means that they now have a chance > of working correctly. > > Signed-off-by: Arnd Bergmann > Signed-off-by: Baoquan He > Cc: Helge Deller > Cc: Thomas Zimmermann > Cc: Christophe Leroy > Cc: linux-fb...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org Reviewed-by: Luis Chamberlain Is anyone using this driver these days? How often do fbdev drivers get audited to see what can be nuked? Luis
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 09/03/2023 04.12, Christian König wrote: > Am 08.03.23 um 20:05 schrieb Asahi Lina: >> [SNIP] >>> Well it's not the better way, it's the only way that works. >>> >>> I have to admit that my bet on your intentions was wrong, but even that >>> use case doesn't work correctly. >>> >>> See when your callback returns false it is perfectly possible that all >>> hw fences are signaled between returning that information and processing it. >>> >>> The result would be that the scheduler goes to sleep and never wakes up >>> again. >> That can't happen, because it will just go into another iteration of the >> drm_sched main loop since there is an entity available still. >> >> Rather there is probably the opposite bug in this patch: the can_run_job >> logic should be moved into the wait_event_interruptible() condition >> check, otherwise I think it can end up busy-looping since the condition >> itself can be true even when the can_run_job check blocks it. >> >> But there is no risk of it going to sleep and never waking up because >> job completions will wake up the waitqueue by definition, and that >> happens after the driver-side queues are popped. If this problem could >> happen, then the existing hw_submission_limit logic would be broken in >> the same way. It is logically equivalent in how it works. >> >> Basically, if properly done in wait_event_interruptible, it is exactly >> the logic of that macro that prevents this race condition and makes >> everything work at all. Without it, drm_sched would be completely broken. >> >>> As I said we exercised those ideas before and yes this approach here >>> came up before as well and no it doesn't work. >> It can never deadlock with this patch as it stands (though it could busy >> loop), and if properly moved into the wait_event_interruptible(), it >> would also never busy loop and work entirely as intended. The actual API >> change is sound. >> >> I don't know why you're trying so hard to convince everyone that this >> approach is fundamentally broken... It might be a bad idea for other >> reasons, it might encourage incorrect usage, it might not be the best >> option, there are plenty of arguments you can make... but you just keep >> trying to make an argument that it just can't work at all for some >> reason. Why? I already said I'm happy dropping it in favor of the fences... > > Well because it is broken. > > When you move the check into the wait_event_interruptible condition then > who is going to call wait_event_interruptible when the condition changes? I think you mean wake_up_interruptible(). That would be drm_sched_job_done(), on the fence callback when a job completes, which as I keep saying is the same logic used for hw_rq_count/hw_submission_limit tracking. Please think about it for a second, it's really not that complicated to see why it works: - Driver pops off completed commands <-- can_run_job condition satisfied - Driver signals fence - drm_sched_job_done_cb() - drm_sched_job_done() - atomic_dec(>hw_rq_count); <-- hw_submission_limit satisfied - ... - wake_up_interruptible(>wake_up_worker); ^- happens after both conditions are potentially satisfied It really is completely equivalent to just making the hw_rq_count logic customizable by the driver. The actual flow is the same. As long as the driver guarantees it satisfies the can_run_job() condition before signaling the completion fence that triggered that change, it works fine. > As I said this idea came up before and was rejected multiple times. Maybe it was a different idea, or maybe it was rejected for other reasons, or maybe it was wrongly rejected for being broken when it isn't ^^ ~~ Lina
Re: [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down
On 09/03/2023 03.12, Christian König wrote: > Am 08.03.23 um 18:32 schrieb Asahi Lina: >> [SNIP] >> Yes but... none of this cleans up jobs that are already submitted by the >> scheduler and in its pending list, with registered completion callbacks, >> which were already popped off of the entities. >> >> *That* is the problem this patch fixes! > > Ah! Yes that makes more sense now. > >>> We could add a warning when users of this API doesn't do this >>> correctly, but cleaning up incorrect API use is clearly something we >>> don't want here. >> It is the job of the Rust abstractions to make incorrect API use that >> leads to memory unsafety impossible. So even if you don't want that in >> C, it's my job to do that for Rust... and right now, I just can't >> because drm_sched doesn't provide an API that can be safely wrapped >> without weird bits of babysitting functionality on top (like tracking >> jobs outside or awkwardly making jobs hold a reference to the scheduler >> and defer dropping it to another thread). > > Yeah, that was discussed before but rejected. > > The argument was that upper layer needs to wait for the hw to become > idle before the scheduler can be destroyed anyway. Unfortunately, that's not a requirement you can encode in the Rust type system easily as far as I know, and Rust safety rules mean we need to make it safe even if the upper layer doesn't do this... (or else we have to mark the entire drm_sched abstraction unsafe, but that would be a pity). I know it's a different way of thinking, but it has pretty clear benefits since with Rust you can actually guarantee that things are safe overall by just auditing explicitly unsafe code. If we just mark all of drm_sched unsafe, that means we now need to audit all details about how the driver uses it for safety. It makes more sense to just make the abstraction safe, which is much easier to audit. Right now, it is not possible to create a safe Rust abstraction for drm_sched without doing something like duplicating all job tracking in the abstraction, or the above backreference + deferred cleanup mess, or something equally silly. So let's just fix the C side please ^^ >>> Nope, as far as I can see this is just not correctly tearing down the >>> objects in the right order. >> There's no API to clean up in-flight jobs in a drm_sched at all. >> Destroying an entity won't do it. So there is no reasonable way to do >> this at all... > > Yes, this was removed. > >>> So you are trying to do something which is not supposed to work in the >>> first place. >> I need to make things that aren't supposed to work impossible to do in >> the first place, or at least fail gracefully instead of just oopsing >> like drm_sched does today... >> >> If you're convinced there's a way to do this, can you tell me exactly >> what code sequence I need to run to safely shut down a scheduler >> assuming all entities are already destroyed? You can't ask me for a list >> of pending jobs (the scheduler knows this, it doesn't make any sense to >> duplicate that outside), and you can't ask me to just not do this until >> all jobs complete execution (because then we either end up with the >> messy deadlock situation I described if I take a reference, or more >> duplicative in-flight job count tracking and blocking in the free path >> of the Rust abstraction, which doesn't make any sense either). > > Good question. We don't have anybody upstream which uses the scheduler > lifetime like this. > > Essentially the job list in the scheduler is something we wanted to > remove because it causes tons of race conditions during hw recovery. > > When you tear down the firmware queue how do you handle already > submitted jobs there? The firmware queue is itself reference counted and any firmware queue that has acquired an event notification resource (that is, which is busy with running or upcoming jobs) hands off a reference to itself into the event subsystem, so it can get notified of job completions by the firmware. Then once it becomes idle it unregisters itself, and at that point if it has no owning userspace queue, that would be the last reference and it gets dropped. So we don't tear down firmware queues until they are idle. (There is a subtle deadlock break in the event module to make this work out, where we clone a reference to the queue and drop the event subsystem lock before signaling it of completions, so it can call back in and take the lock as it unregisters itself if needed. Then the actual teardown happens when the signaling is complete and that reference clone is the last one to get dropped.) If a queue is idle at the firmware level but has upcoming jobs queued in drm_sched, when those get deleted as part of an explicit drm_sched teardown (free_job()) the queue notices it lost its upcoming jobs and relinquishes the event resource if there are no running jobs. I'm not even sure exactly what order this all happens in in practice (it
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 20:05 schrieb Asahi Lina: [SNIP] Well it's not the better way, it's the only way that works. I have to admit that my bet on your intentions was wrong, but even that use case doesn't work correctly. See when your callback returns false it is perfectly possible that all hw fences are signaled between returning that information and processing it. The result would be that the scheduler goes to sleep and never wakes up again. That can't happen, because it will just go into another iteration of the drm_sched main loop since there is an entity available still. Rather there is probably the opposite bug in this patch: the can_run_job logic should be moved into the wait_event_interruptible() condition check, otherwise I think it can end up busy-looping since the condition itself can be true even when the can_run_job check blocks it. But there is no risk of it going to sleep and never waking up because job completions will wake up the waitqueue by definition, and that happens after the driver-side queues are popped. If this problem could happen, then the existing hw_submission_limit logic would be broken in the same way. It is logically equivalent in how it works. Basically, if properly done in wait_event_interruptible, it is exactly the logic of that macro that prevents this race condition and makes everything work at all. Without it, drm_sched would be completely broken. As I said we exercised those ideas before and yes this approach here came up before as well and no it doesn't work. It can never deadlock with this patch as it stands (though it could busy loop), and if properly moved into the wait_event_interruptible(), it would also never busy loop and work entirely as intended. The actual API change is sound. I don't know why you're trying so hard to convince everyone that this approach is fundamentally broken... It might be a bad idea for other reasons, it might encourage incorrect usage, it might not be the best option, there are plenty of arguments you can make... but you just keep trying to make an argument that it just can't work at all for some reason. Why? I already said I'm happy dropping it in favor of the fences... Well because it is broken. When you move the check into the wait_event_interruptible condition then who is going to call wait_event_interruptible when the condition changes? As I said this idea came up before and was rejected multiple times. Regards, Christian. It's intended to mirror the hw_submission_limit logic. If you think this is broken, then that's broken too. They are equivalent mechanisms. This particular issue aside, fairness in global resource allocation is a conversation I'd love to have! Right now the driver doesn't try to ensure that, a queue can easily monopolize certain hardware resources (though one queue can only monopolize one of each, so you'd need something like 63 queues with 63 distinct VMs all submitting free-running jobs back to back in order to starve other queues of resources forever). For starters, one thing I'm thinking of doing is reserving certain subsets of hardware resources for queues with a given priority, so you can at least guarantee forward progress of higher-priority queues when faced with misbehaving lower-priority queues. But if we want to guarantee proper fairness, I think I'll have to start doing things like switching to a CPU-roundtrip submission model when resources become scarce (to guarantee that queues actually release the resources once in a while) and then figure out how to add fairness to the allocation code... But let's have that conversation when we talk about the driver (or maybe on IRC or something?), right now I'm more interested in getting the abstractions reviewed ^^ Well that stuff is highly problematic as well. The fairness aside you risk starvation which in turn breaks the guarantee of forward progress. In this particular case you can catch this with a timeout for the hw operation, but you should consider blocking that from the sw side as well. In the current state I actually think it's not really that problematic, because the resources are acquired directly in the ioctl path. So that can block if starved, but if that can cause overall forward progress to stop because some fence doesn't get signaled, then so can just not doing the ioctl in the first place, so there's not much point (userspace can always misbehave with its fence usage...). By the time anything gets submitted to drm_sched, the resources are already guaranteed to be acquired, we never block in the run callback. It needs to be fixed of course, but if the threat model is a malicious GPU process, well, there are many other ways to DoS your system... and I don't think it's very likely that 63+ queues (which usually means 63+ processes with OpenGL) will end up accidentally starving the GPU in a tight loop at the same time. I'd love to hear about real-world scenarios where this kind of thing has been a real problem and not just
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
On 09/03/2023 02.57, Christian König wrote: > Am 08.03.23 um 17:44 schrieb Asahi Lina: >> On 09/03/2023 00.30, Christian König wrote: >>> Am 08.03.23 um 15:53 schrieb Asahi Lina: [SNIP] > The background is that core memory management requires that signaling a > fence only depends on signaling other fences and hardware progress and > nothing else. Otherwise you immediately run into problems because of > circle dependencies or what we call infinite fences. And hardware progress is exactly the only dependency here... >>> Well then you should have a fence for that hardware progress. >> I do, it's the prior job hardware completion fences that drm_sched >> already knows about! >> >> Yes, I could return those in the prepare callback, it just means I need >> to start stashing fence references in the underlying firmware job queue >> command objects so I can find out what is the oldest pending fence is, >> and return it when a queue is full. As long as drm_sched doesn't mind if >> I keep giving it fences (since multiple commands can have to complete >> before there is space) or the occasional already signaled fence (because >> this process is inherently racy), it should work fine. > > Well this handling is intentional and necessary, but see below for a > more in deep explanation. > >> If you think this is the better way, I'll do it that way and drop this >> patch. It just seemed simpler to do it with another callback, since >> drm_sched is already tracking those fences and doing a hardware queue >> limit check anyway, and that way I can avoid tracking the fences down >> into the hardware queue code... * > > Well it's not the better way, it's the only way that works. > > I have to admit that my bet on your intentions was wrong, but even that > use case doesn't work correctly. > > See when your callback returns false it is perfectly possible that all > hw fences are signaled between returning that information and processing it. > > The result would be that the scheduler goes to sleep and never wakes up > again. That can't happen, because it will just go into another iteration of the drm_sched main loop since there is an entity available still. Rather there is probably the opposite bug in this patch: the can_run_job logic should be moved into the wait_event_interruptible() condition check, otherwise I think it can end up busy-looping since the condition itself can be true even when the can_run_job check blocks it. But there is no risk of it going to sleep and never waking up because job completions will wake up the waitqueue by definition, and that happens after the driver-side queues are popped. If this problem could happen, then the existing hw_submission_limit logic would be broken in the same way. It is logically equivalent in how it works. Basically, if properly done in wait_event_interruptible, it is exactly the logic of that macro that prevents this race condition and makes everything work at all. Without it, drm_sched would be completely broken. > As I said we exercised those ideas before and yes this approach here > came up before as well and no it doesn't work. It can never deadlock with this patch as it stands (though it could busy loop), and if properly moved into the wait_event_interruptible(), it would also never busy loop and work entirely as intended. The actual API change is sound. I don't know why you're trying so hard to convince everyone that this approach is fundamentally broken... It might be a bad idea for other reasons, it might encourage incorrect usage, it might not be the best option, there are plenty of arguments you can make... but you just keep trying to make an argument that it just can't work at all for some reason. Why? I already said I'm happy dropping it in favor of the fences... It's intended to mirror the hw_submission_limit logic. If you think this is broken, then that's broken too. They are equivalent mechanisms. >> This particular issue aside, fairness in global resource allocation is a >> conversation I'd love to have! Right now the driver doesn't try to >> ensure that, a queue can easily monopolize certain hardware resources >> (though one queue can only monopolize one of each, so you'd need >> something like 63 queues with 63 distinct VMs all submitting >> free-running jobs back to back in order to starve other queues of >> resources forever). For starters, one thing I'm thinking of doing is >> reserving certain subsets of hardware resources for queues with a given >> priority, so you can at least guarantee forward progress of >> higher-priority queues when faced with misbehaving lower-priority >> queues. But if we want to guarantee proper fairness, I think I'll have >> to start doing things like switching to a CPU-roundtrip submission model >> when resources become scarce (to guarantee that queues actually release >> the resources once in a while) and then figure out how to add fairness >> to the allocation code... >> >> But
Re: [PATCH v4 06/12] dt-bindings: gpu: mali-bifrost: Add support for MediaTek MT8186
On Tue, 28 Feb 2023 11:26:58 +0100, AngeloGioacchino Del Regno wrote: > MT8186 has a Mali-G52 MC2 2EE GPU (two cores): add a binding with > two power domains (one per core) for it. > > Signed-off-by: AngeloGioacchino Del Regno > > Reviewed-by: Chen-Yu Tsai > --- > .../bindings/gpu/arm,mali-bifrost.yaml | 18 ++ > 1 file changed, 18 insertions(+) > Reviewed-by: Rob Herring
Re: [PATCH v4 03/12] dt-bindings: gpu: mali-bifrost: Fix power-domain-names validation
On Tue, Feb 28, 2023 at 11:26:55AM +0100, AngeloGioacchino Del Regno wrote:
> Commit ("dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183")
> incorrectly introduced power domain names for MT8183, causing
> validation issues.
>
> Add power-domain-names to the base schema, allowing a maximum of
> five elements; since platforms having a single power domain don't
> need any actual domain name, disallow that for each sub-schema.
>
> Fixes: a7a596cd3115 ("dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183")
> Signed-off-by: AngeloGioacchino Del Regno
>
> ---
> .../devicetree/bindings/gpu/arm,mali-bifrost.yaml | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> index 5b7f1c9d2b30..bf0f7f1f71e0 100644
> --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml
> @@ -65,6 +65,10 @@ properties:
> minItems: 1
> maxItems: 5
>
> + power-domain-names:
> +minItems: 1
If you are disallowing for a single domain, then this could be 2...
Reviewed-by: Rob Herring
> +maxItems: 5
> +
>resets:
> minItems: 1
> maxItems: 3
> @@ -112,6 +116,7 @@ allOf:
>properties:
> power-domains:
>maxItems: 1
> +power-domain-names: false
>required:
> - resets
>- if:
> @@ -136,6 +141,7 @@ allOf:
> - const: bus_ace
> power-domains:
>maxItems: 1
> +power-domain-names: false
> resets:
>minItems: 3
> reset-names:
> @@ -186,6 +192,7 @@ allOf:
> - const: bus
> power-domains:
>maxItems: 1
> +power-domain-names: false
>required:
> - clock-names
>
> --
> 2.39.2
>
Re: [PATCH v4 02/12] dt-bindings: gpu: mali-bifrost: Set power-domains maxItems to 5
On Tue, 28 Feb 2023 11:26:54 +0100, AngeloGioacchino Del Regno wrote: > In preparation for adding (and fixing) power-domain-names and MediaTek > MT8192 bindings, allow up to five items for power-domains. > > Signed-off-by: AngeloGioacchino Del Regno > > --- > Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Reviewed-by: Rob Herring
Re: [PATCH v4 01/12] dt-bindings: gpu: mali-bifrost: Split out MediaTek power-domains variation
On Tue, 28 Feb 2023 11:26:53 +0100, AngeloGioacchino Del Regno wrote: > In preparation for adding new bindings for new MediaTek SoCs, split out > the power-domains variation from the `else` in the current > mediatek,mt8183-mali conditional. > > The sram-supply part is left in place to be disallowed for anything > that is not compatible with "mediatek,mt8183-mali" as this regulator > is MediaTek-specific and it is, and will ever be, used only for this > specific string due to the addition of the mediatek-regulator-coupler > driver. > > Signed-off-by: AngeloGioacchino Del Regno > > --- > .../devicetree/bindings/gpu/arm,mali-bifrost.yaml| 9 +++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > Reviewed-by: Rob Herring
Re: [PATCH v3 09/10] dt-bindings: display/msm: dsi-controller-main: Add SM6115
On Tue, 07 Mar 2023 14:01:47 +0100, Konrad Dybcio wrote: > Add a compatible for the DSI on SM6115. > > Signed-off-by: Konrad Dybcio > --- > .../devicetree/bindings/display/msm/dsi-controller-main.yaml | 2 ++ > .../devicetree/bindings/display/msm/qcom,sm6115-mdss.yaml | 8 > +++- > 2 files changed, 9 insertions(+), 1 deletion(-) > Acked-by: Rob Herring
Re: [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down
Am 08.03.23 um 18:39 schrieb aly...@rosenzweig.io: You can't ask me for a list of pending jobs (the scheduler knows this, it doesn't make any sense to duplicate that outside) Silly question: could you add a new exported function to drm_sched to get the list of pending jobs, to be used by the Rust abstraction internally? IDK if that makes any sense. I was thinking about something similar as well. The problem is that you could only use this function from the scheduler thread itself, e.g. from one of its callback functions. Christian.
Re: [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down
Am 08.03.23 um 18:32 schrieb Asahi Lina: [SNIP] Yes but... none of this cleans up jobs that are already submitted by the scheduler and in its pending list, with registered completion callbacks, which were already popped off of the entities. *That* is the problem this patch fixes! Ah! Yes that makes more sense now. We could add a warning when users of this API doesn't do this correctly, but cleaning up incorrect API use is clearly something we don't want here. It is the job of the Rust abstractions to make incorrect API use that leads to memory unsafety impossible. So even if you don't want that in C, it's my job to do that for Rust... and right now, I just can't because drm_sched doesn't provide an API that can be safely wrapped without weird bits of babysitting functionality on top (like tracking jobs outside or awkwardly making jobs hold a reference to the scheduler and defer dropping it to another thread). Yeah, that was discussed before but rejected. The argument was that upper layer needs to wait for the hw to become idle before the scheduler can be destroyed anyway. Right now, it is not possible to create a safe Rust abstraction for drm_sched without doing something like duplicating all job tracking in the abstraction, or the above backreference + deferred cleanup mess, or something equally silly. So let's just fix the C side please ^^ Nope, as far as I can see this is just not correctly tearing down the objects in the right order. There's no API to clean up in-flight jobs in a drm_sched at all. Destroying an entity won't do it. So there is no reasonable way to do this at all... Yes, this was removed. So you are trying to do something which is not supposed to work in the first place. I need to make things that aren't supposed to work impossible to do in the first place, or at least fail gracefully instead of just oopsing like drm_sched does today... If you're convinced there's a way to do this, can you tell me exactly what code sequence I need to run to safely shut down a scheduler assuming all entities are already destroyed? You can't ask me for a list of pending jobs (the scheduler knows this, it doesn't make any sense to duplicate that outside), and you can't ask me to just not do this until all jobs complete execution (because then we either end up with the messy deadlock situation I described if I take a reference, or more duplicative in-flight job count tracking and blocking in the free path of the Rust abstraction, which doesn't make any sense either). Good question. We don't have anybody upstream which uses the scheduler lifetime like this. Essentially the job list in the scheduler is something we wanted to remove because it causes tons of race conditions during hw recovery. When you tear down the firmware queue how do you handle already submitted jobs there? Regards, Christian. ~~ Lina
Re: [PATCH] dt-bindings: yamllint: Require a space after a comment '#'
On Fri, 03 Mar 2023 15:42:23 -0600, Rob Herring wrote: > Enable yamllint to check the prefered commenting style of requiring a > space after a comment character '#'. Fix the cases in the tree which > have a warning with this enabled. Most cases just need a space after the > '#'. A couple of cases with comments which were not intended to be > comments are revealed. Those were in ti,sa2ul.yaml, ti,cal.yaml, and > brcm,bcmgenet.yaml. > > Signed-off-by: Rob Herring > --- > Cc: Krzysztof Kozlowski > Cc: Stephen Boyd > Cc: Herbert Xu > Cc: "David S. Miller" > Cc: Rob Clark > Cc: Abhinav Kumar > Cc: Dmitry Baryshkov > Cc: Sean Paul > Cc: Thomas Gleixner > Cc: Marc Zyngier > Cc: Mauro Carvalho Chehab > Cc: Eric Dumazet > Cc: Jakub Kicinski > Cc: Paolo Abeni > Cc: Andrew Lunn > Cc: Heiner Kallweit > Cc: Vinod Koul > Cc: Kishon Vijay Abraham I > Cc: Mark Brown > Cc: Conor Dooley > Cc: linux-...@vger.kernel.org > Cc: linux-cry...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: freedr...@lists.freedesktop.org > Cc: linux-me...@vger.kernel.org > Cc: net...@vger.kernel.org > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-...@lists.infradead.org > Cc: linux-g...@vger.kernel.org > Cc: alsa-de...@alsa-project.org > Cc: linux-ri...@lists.infradead.org > Cc: linux-...@vger.kernel.org > --- > Documentation/devicetree/bindings/.yamllint | 2 +- > .../bindings/clock/qcom,a53pll.yaml | 4 ++-- > .../devicetree/bindings/crypto/ti,sa2ul.yaml | 4 ++-- > .../bindings/display/msm/qcom,mdp5.yaml | 2 +- > .../interrupt-controller/arm,gic.yaml | 4 ++-- > .../loongson,pch-msi.yaml | 2 +- > .../bindings/media/renesas,vin.yaml | 4 ++-- > .../devicetree/bindings/media/ti,cal.yaml | 4 ++-- > .../bindings/net/brcm,bcmgenet.yaml | 2 -- > .../bindings/net/cortina,gemini-ethernet.yaml | 6 ++--- > .../devicetree/bindings/net/mdio-gpio.yaml| 4 ++-- > .../phy/marvell,armada-cp110-utmi-phy.yaml| 2 +- > .../bindings/phy/phy-stm32-usbphyc.yaml | 2 +- > .../phy/qcom,sc7180-qmp-usb3-dp-phy.yaml | 2 +- > .../bindings/pinctrl/pinctrl-mt8192.yaml | 2 +- > .../regulator/nxp,pca9450-regulator.yaml | 8 +++ > .../regulator/rohm,bd71828-regulator.yaml | 20 > .../regulator/rohm,bd71837-regulator.yaml | 6 ++--- > .../regulator/rohm,bd71847-regulator.yaml | 6 ++--- > .../bindings/soc/renesas/renesas.yaml | 2 +- > .../devicetree/bindings/soc/ti/ti,pruss.yaml | 2 +- > .../bindings/sound/amlogic,axg-tdm-iface.yaml | 2 +- > .../bindings/sound/qcom,lpass-rx-macro.yaml | 4 ++-- > .../bindings/sound/qcom,lpass-tx-macro.yaml | 4 ++-- > .../bindings/sound/qcom,lpass-va-macro.yaml | 4 ++-- > .../sound/qcom,q6dsp-lpass-ports.yaml | 2 +- > .../bindings/sound/simple-card.yaml | 24 +-- > .../bindings/spi/microchip,mpfs-spi.yaml | 2 +- > 28 files changed, 65 insertions(+), 67 deletions(-) > Applied, thanks!
Re: [PATCH RFC 10/18] drm/scheduler: Add can_run_job callback
Am 08.03.23 um 17:44 schrieb Asahi Lina: On 09/03/2023 00.30, Christian König wrote: Am 08.03.23 um 15:53 schrieb Asahi Lina: [SNIP] The background is that core memory management requires that signaling a fence only depends on signaling other fences and hardware progress and nothing else. Otherwise you immediately run into problems because of circle dependencies or what we call infinite fences. And hardware progress is exactly the only dependency here... Well then you should have a fence for that hardware progress. I do, it's the prior job hardware completion fences that drm_sched already knows about! Yes, I could return those in the prepare callback, it just means I need to start stashing fence references in the underlying firmware job queue command objects so I can find out what is the oldest pending fence is, and return it when a queue is full. As long as drm_sched doesn't mind if I keep giving it fences (since multiple commands can have to complete before there is space) or the occasional already signaled fence (because this process is inherently racy), it should work fine. Well this handling is intentional and necessary, but see below for a more in deep explanation. If you think this is the better way, I'll do it that way and drop this patch. It just seemed simpler to do it with another callback, since drm_sched is already tracking those fences and doing a hardware queue limit check anyway, and that way I can avoid tracking the fences down into the hardware queue code... * Well it's not the better way, it's the only way that works. I have to admit that my bet on your intentions was wrong, but even that use case doesn't work correctly. See when your callback returns false it is perfectly possible that all hw fences are signaled between returning that information and processing it. The result would be that the scheduler goes to sleep and never wakes up again. That's why we have that rule that all dependencies need to be expressed by those dma_fence objects, cause those are design with such races in mind. (But I still maintain what I'm trying to do here is entirely correct and deadlock-free! If you prefer I use prepare_job and return prior job fences from that instead, that's very different from NAKing the patch saying it's broken...) As I said we exercised those ideas before and yes this approach here came up before as well and no it doesn't work. * If you're wondering how the fences get signaled at all then: callback closures that capture a reference to the fence when firmware commands are constructed and submitted. I know, I know, fancy Rust stuff... ^^ If you'd rather have me use the fences for the blocking, I'll probably just drop the signaling bit from the closures so we don't need to keep two redundant fence references in different places per command. I still need the closures for command completion processing though, since I use them to process statistics too... I see that we have a disconnection here. As far as I can see you can use the can_run callback in only three ways: 1. To check for some userspace dependency (We don't need to discuss that, it's evil and we both know it). 2. You check for some hw resource availability. Similar to VMID on amdgpu hw. This is what I think you do here (but I might be wrong). It isn't... I agree, it would be problematic. It doesn't make any sense to check for global resources this way, not just because you might deadlock but also because there might be nothing to signal to the scheduler that a resource was freed at all once it is! But this would be extremely problematic because you can then live lock. E.g. queue A keeps submitting jobs which take only a few resources and by doing so delays submitting jobs from queue B indefinitely. This particular issue aside, fairness in global resource allocation is a conversation I'd love to have! Right now the driver doesn't try to ensure that, a queue can easily monopolize certain hardware resources (though one queue can only monopolize one of each, so you'd need something like 63 queues with 63 distinct VMs all submitting free-running jobs back to back in order to starve other queues of resources forever). For starters, one thing I'm thinking of doing is reserving certain subsets of hardware resources for queues with a given priority, so you can at least guarantee forward progress of higher-priority queues when faced with misbehaving lower-priority queues. But if we want to guarantee proper fairness, I think I'll have to start doing things like switching to a CPU-roundtrip submission model when resources become scarce (to guarantee that queues actually release the resources once in a while) and then figure out how to add fairness to the allocation code... But let's have that conversation when we talk about the driver (or maybe on IRC or something?), right now I'm more interested in getting the abstractions reviewed ^^ Well that stuff is highly problematic as
[PATCH] drm/bridge: adv7511: fix race condition bug in adv7511_remove due to unfinished work
In adv7511_probe, adv7511->hpd_work is bound with adv7511_hpd_work.
If we call adv7511_remove with a unfinished work. There may be a
race condition where bridge->hpd_mutex was destroyed by
drm_bridge_remove and used in adv7511_hpd_work in drm_bridge_hpd_notify.
Fix it by canceling the work before cleanup in adv7511_remove.
Signed-off-by: Zheng Wang
---
drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index ddceafa7b637..9bf72dd6c1d3 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1349,6 +1349,7 @@ static void adv7511_remove(struct i2c_client *i2c)
{
struct adv7511 *adv7511 = i2c_get_clientdata(i2c);
+ cancel_work_sync(>hpd_work);
adv7511_uninit_regulators(adv7511);
drm_bridge_remove(>bridge);
--
2.25.1
Re: [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down
> You can't ask me for a list > of pending jobs (the scheduler knows this, it doesn't make any sense to > duplicate that outside) Silly question: could you add a new exported function to drm_sched to get the list of pending jobs, to be used by the Rust abstraction internally? IDK if that makes any sense.
Re: [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down
On 09/03/2023 02.39, aly...@rosenzweig.io wrote: >> You can't ask me for a list >> of pending jobs (the scheduler knows this, it doesn't make any sense to >> duplicate that outside) > > Silly question: could you add a new exported function to drm_sched to get the > list of pending jobs, to be used by the Rust abstraction internally? IDK if > that makes any sense. The drm_sched struct is public, we could just go in there and do it anyway... but then I need to figure out how to do `list_for_each_entry_safe` in Rust and this all makes very little sense when it's clearly the scheduler's job to provide some form of cleanup function users can use to do it... I mean, I guess I can do that if Christian is adamantly against providing a safe C API, but it's clearly not the right solution and I hope this is not the approach maintainers take with Rust abstractions, because that's going to make our lives a lot harder for no good reason, and it also means C users don't get any of the benefits of Rust abstraction work if the APIs can't be improved at all along with it. ~~ Lina
Re: [PATCH] drm/amdkfd: fix a potential double free in pqm_create_queue
On 2023-03-07 19:19, Chia-I Wu wrote: Set *q to NULL on errors, otherwise pqm_create_queue would free it again. Signed-off-by: Chia-I Wu Thank you! I'm applying this patch to amd-staging-drm-next. Reviewed-by: Felix Kuehling --- drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c index 5137476ec18e6..4236539d9f932 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process_queue_manager.c @@ -218,8 +218,8 @@ static int init_user_queue(struct process_queue_manager *pqm, return 0; cleanup: - if (dev->shared_resources.enable_mes) - uninit_queue(*q); + uninit_queue(*q); + *q = NULL; return retval; }
Re: [PATCH RFC 11/18] drm/scheduler: Clean up jobs when the scheduler is torn down
On 09/03/2023 00.42, Christian König wrote:
> Am 08.03.23 um 16:18 schrieb Asahi Lina:
>> On 08/03/2023 19.03, Christian König wrote:
>>> Am 08.03.23 um 10:57 schrieb Maarten Lankhorst:
On 2023-03-07 15:25, Asahi Lina wrote:
> drm_sched_fini() currently leaves any pending jobs dangling, which
> causes segfaults and other badness when job completion fences are
> signaled after the scheduler is torn down.
>
> Explicitly detach all jobs from their completion callbacks and free
> them. This makes it possible to write a sensible safe abstraction for
> drm_sched, without having to externally duplicate the tracking of
> in-flight jobs.
>
> This shouldn't regress any existing drivers, since calling
> drm_sched_fini() with any pending jobs is broken and this change should
> be a no-op if there are no pending jobs.
>
> Signed-off-by: Asahi Lina
> ---
> drivers/gpu/drm/scheduler/sched_main.c | 27
> +--
> 1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 5c0add2c7546..0aab1e0aebdd 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1119,10 +1119,33 @@ EXPORT_SYMBOL(drm_sched_init);
> void drm_sched_fini(struct drm_gpu_scheduler *sched)
> {
> struct drm_sched_entity *s_entity;
> + struct drm_sched_job *s_job, *tmp;
> int i;
> - if (sched->thread)
> - kthread_stop(sched->thread);
> + if (!sched->thread)
> + return;
> +
> + /*
> + * Stop the scheduler, detaching all jobs from their hardware
> callbacks
> + * and cleaning up complete jobs.
> + */
> + drm_sched_stop(sched, NULL);
> +
> + /*
> + * Iterate through the pending job list and free all jobs.
> + * This assumes the driver has either guaranteed jobs are
> already stopped, or that
> + * otherwise it is responsible for keeping any necessary data
> structures for
> + * in-progress jobs alive even when the free_job() callback is
> called early (e.g. by
> + * putting them in its own queue or doing its own refcounting).
> + */
> + list_for_each_entry_safe(s_job, tmp, >pending_list, list) {
> + spin_lock(>job_list_lock);
> + list_del_init(_job->list);
> + spin_unlock(>job_list_lock);
> + sched->ops->free_job(s_job);
> + }
I would stop the kthread first, then delete all jobs without spinlock
since nothing else can race against sched_fini?
If you do need the spinlock, It would need to guard
list_for_each_entry too.
>>> Well this case here actually should not happen in the first place.
>> "This should not happen in the first place" is how you end up with C
>> APIs that have corner cases that lead to kernel oopses...
>>
>> The idea with Rust abstractions is that it needs to be actually
>> impossible to create memory safety problems for the user of the
>> abstraction, you can't impose arbitrary constraints like "you must wait
>> for all jobs to finish before destroying the scheduler"... it needs to
>> be intrinsically safe.
>>
>>> Jobs depend on their device, so as long as there are jobs there should
>>> also be a reference to the scheduler.
>> These schedulers are created dynamically per userspace queue. The memory
>> management and reference counting involved make it safe to destroy the
>> scheduler even when behind the scenes hardware jobs are still running,
>> as long as drm_sched itself doesn't crash on fences firing without a
>> scheduler (which is what this patch fixes).
>
> We have originally rejected that approach, but I still think it might
> work if done right.
>
>> This is the power of Rust: it forces you to architect your code in a way
>> that you don't have complex high-level dependencies that span the entire
>> driver and are difficult to prove hold. In my driver, you can kill a
>> process and that destroys the drm_sched, closes all GEM objects,
>> everything, even if the GPU is still running jobs from that process. The
>> worst that can happen is that the GPU faults as in-use userspace buffers
>> are unmapped out from under the running user job, but that's fine (GPU
>> faults are recoverable). The actual firmware resources, queues, etc. in
>> use are all kept alive until the commands finish executing (or fault,
>> which is just an abnormal completion), even if the userspace process
>> that owned them is long gone. I've tested this extensively by doing
>> things like large-resolution glmark runs in a loop that get `kill -9`'d
>> repeatedly, and it works very well! Tons of GPU faults but no firmware
>> crashes, no oopses, nothing. And the firmware *will* crash irrecoverably
>> if anything
Re: [PATCH] gpu: host1x: fix uninitialized variable use
On 08/03/2023 16:56, Nathan Chancellor wrote:
Ping? This warning is now in 6.3-rc1.
Thierry is away at the moment.
David, Daniel, do you want to pick this up directly in the meantime as a
fix for 6.3? Mikko has already reviewed and FWIW ...
Reviewed-by: Jon Hunter
Thanks
Jon
On Thu, Feb 23, 2023 at 09:28:28AM -0700, Nathan Chancellor wrote:
Hi Thierry, Daniel, and David,
On Fri, Jan 27, 2023 at 11:14:00PM +0100, Arnd Bergmann wrote:
From: Arnd Bergmann
The error handling for platform_get_irq() failing no longer
works after a recent change, clang now points this out with
a warning:
drivers/gpu/host1x/dev.c:520:6: error: variable 'syncpt_irq' is uninitialized
when used here [-Werror,-Wuninitialized]
if (syncpt_irq < 0)
^~
Fix this by removing the variable and checking the correct
error status.
Fixes: 625d4ffb438c ("gpu: host1x: Rewrite syncpoint interrupt handling")
Signed-off-by: Arnd Bergmann
---
drivers/gpu/host1x/dev.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 4872d183d860..aae2efeef503 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -487,7 +487,6 @@ static int host1x_get_resets(struct host1x *host)
static int host1x_probe(struct platform_device *pdev)
{
struct host1x *host;
- int syncpt_irq;
int err;
host = devm_kzalloc(>dev, sizeof(*host), GFP_KERNEL);
@@ -517,8 +516,8 @@ static int host1x_probe(struct platform_device *pdev)
}
host->syncpt_irq = platform_get_irq(pdev, 0);
- if (syncpt_irq < 0)
- return syncpt_irq;
+ if (host->syncpt_irq < 0)
+ return host->syncpt_irq;
mutex_init(>devices_lock);
INIT_LIST_HEAD(>devices);
--
2.39.0
Apologies if this has been reported already or has a solution in
progress but mainline is now broken because this change got separated
from the change it is fixing:
https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/4249931209/jobs/7391912774
https://storage.tuxsuite.com/public/clangbuiltlinux/continuous-integration2/builds/2M7y9HpiXB13qiC2mkHMyeZOcLW/build.log
I see this change sitting in the drm-tegra tree [1], which is getting
merged into -next, so it is fixed there, which is why we did not notice
any issues until the drm-next tree was merged into mainline. Can this be
fast tracked to Linus to unbreak clang builds with -Werror?
[1]:
https://gitlab.freedesktop.org/drm/tegra/-/commit/b9930311641cf2ed905a84aabe27e8f3868aee4a
--
nvpublic
