Re: [PATCH v2 02/10] dt-bindings: gpu: mali-bifrost: Split out MediaTek power-domains variation
On Tue, Feb 21, 2023 at 11:37 PM AngeloGioacchino Del Regno wrote: > > In preparation for adding new bindings for new MediaTek SoCs, split out > the power-domain-names and power-domainsvariation from the `else` in ^ missing space Otherwise, Reviewed-by: Chen-Yu Tsai > 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 | 15 --- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml > b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml > index 02699d389be1..ac174c17e25f 100644 > --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml > +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml > @@ -145,6 +145,18 @@ allOf: > - power-domains > - resets > - reset-names > + - if: > + not: > +properties: > + compatible: > +contains: > + enum: > +- mediatek,mt8183-mali > +then: > + properties: > +power-domains: > + maxItems: 1 > +power-domain-names: false >- if: >properties: > compatible: > @@ -166,9 +178,6 @@ allOf: > - power-domain-names > else: >properties: > -power-domains: > - maxItems: 1 > -power-domain-names: false > sram-supply: false >- if: >properties: > -- > 2.39.2 >
Re: [PATCH v2 00/10] Panfrost: Improve and add MediaTek SoCs support
On Tue, Feb 21, 2023 at 04:37:30PM +0100, AngeloGioacchino Del Regno wrote: > Changes in v2: > - Add power-domain-names commit from Chen-Yu to the series > - Kept sram-supply in base schema, overridden for non-MediaTek > - Added Reviewed-by tags from Steven Price to the driver commits >(as released in reply to v1's cover letter - thanks!) > > This series adds support for new MediaTek SoCs (MT8186/MT8192/MT8195) > and improves MT8183 support: since the mtk-regulator-coupler driver > was picked, it is now useless for Panfrost to look for, and manage, > two regulators (GPU Vcore and GPU SRAM) on MediaTek; > > The aforementioned driver will take care of keeping the voltage > relation (/constraints) of the two regulators on its own when a > voltage change request is sent to the Vcore, solving the old time > issue with not working DVFS on Panfrost+MediaTek (due to devfreq > supporting only single regulator). > > In the specific case of MT8183, in order to not break the ABI, it > was necessary to add a new compatible for enabling DVFS. > > Alyssa Rosenzweig (3): > drm/panfrost: Increase MAX_PM_DOMAINS to 5 > drm/panfrost: Add the MT8192 GPU ID > drm/panfrost: Add mediatek,mt8192-mali compatible > > AngeloGioacchino Del Regno (6): > dt-bindings: gpu: mali-bifrost: Split out MediaTek power-domains > variation > dt-bindings: gpu: mali-bifrost: Allow up to 5 power domains for MT8192 > dt-bindings: gpu: mali-bifrost: Add compatible for MT8195 SoC > dt-bindings: gpu: mali-bifrost: Add new MT8183 compatible > dt-bindings: gpu: mali-bifrost: Add a compatible for MediaTek MT8186 > drm/panfrost: Add new compatible for Mali on the MT8183 SoC > > Chen-Yu Tsai (1): > dt-bindings: gpu: mali-bifrost: Add power-domain-names to base schema > > .../bindings/gpu/arm,mali-bifrost.yaml| 67 ++- > drivers/gpu/drm/panfrost/panfrost_device.h| 2 +- > drivers/gpu/drm/panfrost/panfrost_drv.c | 28 > drivers/gpu/drm/panfrost/panfrost_gpu.c | 8 +++ > 4 files changed, 101 insertions(+), 4 deletions(-) Tested-by: Chen-Yu Tsai on MT8183, MT8186, MT8192, MT8195 with glmark2.
Re: [PATCH 2/2] drm: rcar-du: Disable alpha blending for DU planes used with VSP
On 22/02/2023 07:06, Laurent Pinchart wrote: When the input to a DU channel comes from a VSP, the DU doesn't perform any blending operation. Select XRGB instead of ARGB to ensure that the corresponding registers don't get written with invalid values. Signed-off-by: Laurent Pinchart --- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index fe90be51d64e..45c05d0ffc70 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -73,7 +73,7 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) .src.y2 = mode->vdisplay << 16, .zpos = 0, }, - .format = rcar_du_format_info(DRM_FORMAT_ARGB), + .format = rcar_du_format_info(DRM_FORMAT_XRGB), .source = RCAR_DU_PLANE_VSPD1, .colorkey = 0, }; Reviewed-by: Tomi Valkeinen Tomi
Re: [PATCH v2 10/10] drm/panfrost: Add new compatible for Mali on the MT8183 SoC
On Tue, Feb 21, 2023 at 11:37 PM AngeloGioacchino Del Regno wrote: > > The "mediatek,mt8183-mali" compatible uses platform data that calls for > getting (and managing) two regulators ("mali" and "sram") but devfreq > does not support this usecase, resulting in DVFS not working. > > Since a lot of MediaTek SoCs need to set the voltages for the GPU SRAM > regulator in a specific relation to the GPU VCORE regulator, a MediaTek > SoC specific driver was introduced to automatically satisfy, through > coupling, these constraints: this means that there is at all no need to > manage both regulators in panfrost but to otherwise just manage the main > "mali" (-> gpu vcore) regulator instead. > > Keeping in mind that we cannot break the ABI, the most sensible route > (avoiding hacks and uselessly overcomplicated code) to get a MT8183 > node with one power supply was to add a new "mediatek,mt8183b-mali" > compatible, which effectively deprecates the former. > > Signed-off-by: AngeloGioacchino Del Regno > > Reviewed-by: Steven Price Reviewed-by: Chen-Yu Tsai
[PATCH 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 2422d6ef5777..45655efc9468 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c @@ -121,7 +121,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"; @@ -179,7 +179,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(&i915->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 49a7c00c0664..7ebac255865f 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" @@ -1750,6 +1751,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)) @@ -1875,6 +1883,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; + str
[PATCH 5/7] drm/i915/display: 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] Cc: Uma Shankar Cc: Jani Nikula Signed-off-by: Suraj Kandpal --- drivers/gpu/drm/i915/display/icl_dsi.c| 2 - drivers/gpu/drm/i915/display/intel_dp.c | 7 ++- drivers/gpu/drm/i915/display/intel_vdsc.c | 74 ++- 3 files changed, 75 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c index 05e749861658..7065203460d3 100644 --- a/drivers/gpu/drm/i915/display/icl_dsi.c +++ b/drivers/gpu/drm/i915/display/icl_dsi.c @@ -1534,8 +1534,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 f2fb3ec2dd99..95e9d0365e23 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1466,9 +1466,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) { diff --git a/drivers/gpu/drm/i915/display/intel_vdsc.c b/drivers/gpu/drm/i915/display/intel_vdsc.c index ed16f63d6355..32997c9773aa 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.c +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c @@ -460,14 +460,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: +* -We need to double the current bpp. +* -second_line_bpg_offset is 12 in general and equal to 2*(slice_height-1) if slice +* height < 8. +* -second_line_offset_adj is 512 as shown by emperical values to yeild best chroma +* preservation in second line. +* -nsl_bpg_offset is calculated as second_line_offset/slice_height -1 then rounded +* up to 16 fractional bits, we left shift second line offset by 11 to preserve 11 +* fractional bits. +*/ + if (vdsc_cfg->native_420) { + vdsc_cfg->bits_per_pixel <<= 1; + + if (vdsc_cfg->slice_height >= 8) + vdsc_cfg->second_line_bpg_offset = 12; + else + vdsc_cfg->second_line_bpg_offset = + 2 * (vdsc_cfg->slice_height - 1); + + vdsc_cfg->second_line_offset_adj = 512; + vdsc_cfg->nsl_bpg_offset = DIV_ROUND_UP(vdsc_cfg->second_line_bpg_offset << 11, + vdsc_cfg->slice_height - 1); + } + vdsc_cfg->bits_per_component =
[PATCH 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 --- 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 32997c9773aa..a9585f493318 100644 --- a/drivers/gpu/drm/i915/display/intel_vdsc.c +++ b/drivers/gpu/drm/i915/display/intel_vdsc.c @@ -447,6 +447,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); @@ -455,11 +478,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(&dev_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 4/7] drm/i915: 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 --- .../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, 7, 7, 6, 6, 5, 5, 4, 4, 3,
[PATCH 3/7] drm/i915: Adding the new registers for DSC
Adding new DSC register which are introducted MTL onwards Signed-off-by: Suraj Kandpal Reviewed-by: Vandita Kulkarni --- drivers/gpu/drm/i915/i915_reg.h | 28 1 file changed, 28 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 596efc940ee7..9e25e21d37e4 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7715,6 +7715,8 @@ enum skl_power_gate { #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) @@ -7959,6 +7961,32 @@ enum skl_power_gate { #define DSC_SLICE_PER_LINE(slice_per_line)((slice_per_line) << 16) #define DSC_SLICE_CHUNK_SIZE(slice_chunk_size) ((slice_chunk_size) << 0) +/* 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 Rate Control Buffer Threshold Registers */ #define DSCA_RC_BUF_THRESH_0 _MMIO(0x6B230) #define DSCA_RC_BUF_THRESH_0_UDW _MMIO(0x6B230 + 4) -- 2.25.1
[PATCH 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 --- 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 fe98c7dec193..f2fb3ec2dd99 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1491,6 +1491,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, @@ -1511,6 +1536,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 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) Signed-off-by: Ankit Nautiyal --- 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..41da8eb4801e 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 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: Adding the new registers for DSC drm/i915: Enable YCbCr420 for VDSC drm/i915/display: 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 | 39 +++- .../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 | 108 +- drivers/gpu/drm/i915/i915_reg.h | 28 +++ include/drm/display/drm_dp_helper.h | 13 ++ 11 files changed, 442 insertions(+), 24 deletions(-) -- 2.25.1
[PATCH 2/2] drm: rcar-du: Disable alpha blending for DU planes used with VSP
When the input to a DU channel comes from a VSP, the DU doesn't perform any blending operation. Select XRGB instead of ARGB to ensure that the corresponding registers don't get written with invalid values. Signed-off-by: Laurent Pinchart --- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index fe90be51d64e..45c05d0ffc70 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -73,7 +73,7 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) .src.y2 = mode->vdisplay << 16, .zpos = 0, }, - .format = rcar_du_format_info(DRM_FORMAT_ARGB), + .format = rcar_du_format_info(DRM_FORMAT_XRGB), .source = RCAR_DU_PLANE_VSPD1, .colorkey = 0, }; -- Regards, Laurent Pinchart
[PATCH 1/2] drm: rcar-du: Don't write unimplemented ESCR and OTAR registers on Gen3
The ESCR and OTAR registers are not present in all DU channels on Gen3 SoCs. ESCR only exists in channels that can be routed to an LVDS or DPAD, and OTAR in channels that can be routed to a DPAD. Skip writing those registers for other channels. This replaces the DU gen check, as Gen4 doesn't have LVDS or DPAD outputs. Signed-off-by: Laurent Pinchart --- drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 5e552b326162..d6d29be6b4f4 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -298,12 +298,25 @@ static void rcar_du_crtc_set_display_timing(struct rcar_du_crtc *rcrtc) escr = params.escr; } - if (rcdu->info->gen < 4) { + /* +* The ESCR register only exists in DU channels that can output to an +* LVDS or DPAT, and the OTAR register in DU channels that can output +* to a DPAD. +*/ + if ((rcdu->info->routes[RCAR_DU_OUTPUT_DPAD0].possible_crtcs | +rcdu->info->routes[RCAR_DU_OUTPUT_DPAD1].possible_crtcs | +rcdu->info->routes[RCAR_DU_OUTPUT_LVDS0].possible_crtcs | +rcdu->info->routes[RCAR_DU_OUTPUT_LVDS1].possible_crtcs) & + BIT(rcrtc->index)) { dev_dbg(rcrtc->dev->dev, "%s: ESCR 0x%08x\n", __func__, escr); rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? ESCR13 : ESCR02, escr); + } + + if ((rcdu->info->routes[RCAR_DU_OUTPUT_DPAD0].possible_crtcs | +rcdu->info->routes[RCAR_DU_OUTPUT_DPAD1].possible_crtcs) & + BIT(rcrtc->index)) rcar_du_crtc_write(rcrtc, rcrtc->index % 2 ? OTAR13 : OTAR02, 0); - } /* Signal polarities */ dsmr = ((mode->flags & DRM_MODE_FLAG_PVSYNC) ? DSMR_VSL : 0) -- Regards, Laurent Pinchart
[PATCH 0/2] drm: rcar-du: Avoid writing reserved register fields
Hello, This patch series addresses writes to reserved register fields or reserved registers. Depending on the DU variant, some registers or register fields are marked as reserved, but the rcar-du driver writes them unconditionally. There is a high chance that those registers and fields are simply ignored, as shown by the lack of known issue. However, high chances don't satisfy functional safety requirements when they don't match the documentation. As there is no chance of datasheet updates that will document these reserved fields as safe to be written with non-zero values, update the driver to comply with the documentation. Laurent Pinchart (2): drm: rcar-du: Don't write unimplemented ESCR and OTAR registers on Gen3 drm: rcar-du: Disable alpha blending for DU planes used with VSP drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 17 +++-- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 2 +- 2 files changed, 16 insertions(+), 3 deletions(-) -- Regards, Laurent Pinchart
Re: [PATCH v12 07/10] drm/bridge: anx7625: Register Type C mode switches
Hi Andy, Thanks for the review. On Tue, Feb 21, 2023 at 7:36 PM Andy Shevchenko wrote: > > On Tue, Feb 21, 2023 at 05:50:51PM +0800, Pin-yen Lin wrote: > > Register USB Type-C mode switches when the "mode-switch" property and > > relevant ports are available in Device Tree. Configure the crosspoint > > switch based on the entered alternate mode for a specific Type-C > > connector. > > > > Crosspoint switch can also be used for switching the output signal for > > different orientations of a single USB Type-C connector, but the > > orientation switch is not implemented yet. A TODO is added for this. > > ... > > > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx) > > +{ > > + int i; > > unsigned? > > + Blank line. > > > + /* Check if both ports available and do nothing to retain the current > > one */ > > + if (ctx->port_data[0].dp_connected && ctx->port_data[1].dp_connected) > > + return; > > + > > + for (i = 0; i < 2; i++) { > > + if (ctx->port_data[i].dp_connected) > > + anx7625_set_crosspoint_switch(ctx, > > + > > ctx->port_data[i].orientation); > > + } > > +} > > ... > > > + ctx->port_data[port->port_num].dp_connected = > > + state->alt && state->alt->svid == USB_TYPEC_DP_SID && > > I would move the first parameter of && to the separate line for slightly > better > readability. > > > + state->alt->mode == USB_TYPEC_DP_MODE; > > ... > > > + for (i = 0; i < switch_desc->num_typec_switches; i++) { > > + struct drm_dp_typec_port_data *port = > > &switch_desc->typec_ports[i]; > > + struct fwnode_handle *fwnode = port->fwnode; > > + > > + num_lanes = fwnode_property_count_u32(fwnode, "data-lanes"); > > > + > > Redundant blank line. > > > + if (num_lanes < 0) { > > + dev_err(dev, > > + "Error on getting data lanes count from > > %pfwP: %d\n", > > + fwnode, num_lanes); > > > + ret = num_lanes; > > Can be written differently: > > > + goto unregister_mux; > > + } > > ret = ... > if (ret < 0) { > ... > } > num_lanes = ret; > > > What if it's 0? The binding does not allow that, so I don't think we should check it here. I'll address other comments in the next version. Regards, Pin-yen > > > + ret = fwnode_property_read_u32_array(fwnode, "data-lanes", > > + dp_lanes, num_lanes); > > + if (ret) { > > + dev_err(dev, > > + "Failed to read the data-lanes variable: > > %d\n", > > + ret); > > + goto unregister_mux; > > + } > > + > > + ctx->port_data[i].orientation = (dp_lanes[0] / 2 == 0) ? > > + TYPEC_ORIENTATION_REVERSE : TYPEC_ORIENTATION_NORMAL; > > + ctx->port_data[i].dp_connected = false; > > + } > > + complete_all(&ctx->mux_register); > > + > > + return 0; > > + > > +unregister_mux: > > + complete_all(&ctx->mux_register); > > + anx7625_unregister_typec_switches(ctx); > > + return ret; > > +} > > -- > With Best Regards, > Andy Shevchenko > >
Re: [PATCH v12 06/10] drm/bridge: Remove redundant i2c_client in anx7625/it6505
On Tue, Feb 21, 2023 at 5:51 PM Pin-yen Lin wrote: > > These two drivers embed a i2c_client in there private driver data, but This should be "their private driver data". I'll fix this in the next version. Pin-yen > only strict device is actually needed. Replace the i2c_client reference > with a struct device one. > > Signed-off-by: Pin-yen Lin > --- > > Changes in v12: > - New in v12 > > drivers/gpu/drm/bridge/analogix/anx7625.c | 96 > drivers/gpu/drm/bridge/analogix/anx7625.h | 2 +- > drivers/gpu/drm/bridge/ite-it6505.c | 128 +++--- > 3 files changed, 113 insertions(+), 113 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c > b/drivers/gpu/drm/bridge/analogix/anx7625.c > index 486b5099f5dd..cd628a2e2e50 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -207,7 +207,7 @@ static int anx7625_read_ctrl_status_p0(struct > anx7625_data *ctx) > > static int wait_aux_op_finish(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int val; > int ret; > > @@ -234,7 +234,7 @@ static int wait_aux_op_finish(struct anx7625_data *ctx) > static int anx7625_aux_trans(struct anx7625_data *ctx, u8 op, u32 address, > u8 len, u8 *buf) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int ret; > u8 addrh, addrm, addrl; > u8 cmd; > @@ -427,7 +427,7 @@ static int anx7625_odfc_config(struct anx7625_data *ctx, >u8 post_divider) > { > int ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Config input reference clock frequency 27MHz/19.2MHz */ > ret = anx7625_write_and(ctx, ctx->i2c.rx_p1_client, > MIPI_DIGITAL_PLL_16, > @@ -477,7 +477,7 @@ static int anx7625_set_k_value(struct anx7625_data *ctx) > > static int anx7625_dsi_video_timing_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > unsigned long m, n; > u16 htotal; > int ret; > @@ -575,7 +575,7 @@ static int anx7625_dsi_video_timing_config(struct > anx7625_data *ctx) > static int anx7625_swap_dsi_lane3(struct anx7625_data *ctx) > { > int val; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Swap MIPI-DSI data lane 3 P and N */ > val = anx7625_reg_read(ctx, ctx->i2c.rx_p1_client, MIPI_SWAP); > @@ -592,7 +592,7 @@ static int anx7625_api_dsi_config(struct anx7625_data > *ctx) > > { > int val, ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Swap MIPI-DSI data lane 3 P and N */ > ret = anx7625_swap_dsi_lane3(ctx); > @@ -657,7 +657,7 @@ static int anx7625_api_dsi_config(struct anx7625_data > *ctx) > > static int anx7625_dsi_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int ret; > > DRM_DEV_DEBUG_DRIVER(dev, "config dsi.\n"); > @@ -689,7 +689,7 @@ static int anx7625_dsi_config(struct anx7625_data *ctx) > > static int anx7625_api_dpi_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > u16 freq = ctx->dt.pixelclock.min / 1000; > int ret; > > @@ -720,7 +720,7 @@ static int anx7625_api_dpi_config(struct anx7625_data > *ctx) > > static int anx7625_dpi_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int ret; > > DRM_DEV_DEBUG_DRIVER(dev, "config dpi\n"); > @@ -765,7 +765,7 @@ static int anx7625_read_flash_status(struct anx7625_data > *ctx) > static int anx7625_hdcp_key_probe(struct anx7625_data *ctx) > { > int ret, val; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > u8 ident[FLASH_BUF_LEN]; > > ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client, > @@ -815,7 +815,7 @@ static int anx7625_hdcp_key_probe(struct anx7625_data > *ctx) > static int anx7625_hdcp_key_load(struct anx7625_data *ctx) > { > int ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Select HDCP 1.4 KEY */ > ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client, > @@ -843,7 +843,7 @@ static int anx7625_hdcp_key_load(struct anx7625_data *ctx) > static int anx7625_hdcp_disable(struct anx7625_data *ctx) > { > int ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > dev_dbg(dev, "disable HDCP 1.4\n"); > > @@ -864,7 +864,7 @@ static int anx7625
Re: [PATCH v2 09/10] drm/panfrost: Add mediatek, mt8192-mali compatible
On Tue, Feb 21, 2023 at 11:37 PM AngeloGioacchino Del Regno wrote: > > From: Alyssa Rosenzweig > > Required for Mali-G57 on the Mediatek MT8192 and MT8195, which > uses even more power domains than the MT8183 before it. > > Signed-off-by: Alyssa Rosenzweig > [Angelo: Removed unneeded "sram" supply, added mt8195 to commit description] > Co-developed-by: AngeloGioacchino Del Regno > > Signed-off-by: AngeloGioacchino Del Regno > > Reviewed-by: Steven Price Reviewed-by: Chen-Yu Tsai
Re: [PATCH v2 07/10] drm/panfrost: Increase MAX_PM_DOMAINS to 5
On Tue, Feb 21, 2023 at 11:37 PM AngeloGioacchino Del Regno wrote: > > From: Alyssa Rosenzweig > > Increase the MAX_PM_DOMAINS constant from 3 to 5, to support the > extra power domains required by the Mali-G57 on the MT8192. > > Signed-off-by: Alyssa Rosenzweig > Signed-off-by: AngeloGioacchino Del Regno > > Reviewed-by: Steven Price Reviewed-by: Chen-Yu Tsai
Re: [PATCH v2 08/10] drm/panfrost: Add the MT8192 GPU ID
On Tue, Feb 21, 2023 at 11:37 PM AngeloGioacchino Del Regno wrote: > > From: Alyssa Rosenzweig > > MediaTek MT8192 has a Mali-G57 with a special GPU ID. Add its GPU ID, > but treat it as otherwise identical to a standard Mali-G57. > > We do _not_ fix up the GPU ID here -- userspace needs to be aware of the > special GPU ID, in case we find functional differences between > MediaTek's implementation and the standard Mali-G57 down the line. > Signed-off-by: Alyssa Rosenzweig > Signed-off-by: AngeloGioacchino Del Regno > > Reviewed-by: Steven Price Reviewed-by: Chen-Yu Tsai FYI MT8195 has a minor revision for Mali-G57. See https://crrev.com/c/2834981 The commit doesn't say what issues were resolved or still linger though. > --- > drivers/gpu/drm/panfrost/panfrost_gpu.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c > b/drivers/gpu/drm/panfrost/panfrost_gpu.c > index 6452e4e900dd..d28b99732dde 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c > @@ -204,6 +204,14 @@ static const struct panfrost_model gpu_models[] = { > > GPU_MODEL(g57, 0x9001, > GPU_REV(g57, 0, 0)), > + > + /* MediaTek MT8192 has a Mali-G57 with a different GPU ID from the > +* standard. Arm's driver does not appear to handle this model. > +* ChromeOS has a hack downstream for it. Treat it as equivalent to > +* standard Mali-G57 for now. > +*/ > + GPU_MODEL(g57, 0x9003, > + GPU_REV(g57, 0, 0)), > }; > > static void panfrost_gpu_init_features(struct panfrost_device *pfdev) > -- > 2.39.2 >
Re: [PATCH v2] drm/amdkfd: Fix an illegal memory access
Am 2023-02-21 um 22:05 schrieb qu.hu...@linux.dev: In the kfd_wait_on_events() function, the kfd_event_waiter structure is allocated by alloc_event_waiters(), but the event field of the waiter structure is not initialized; When copy_from_user() fails in the kfd_wait_on_events() function, it will enter exception handling to release the previously allocated memory of the waiter structure; Due to the event field of the waiters structure being accessed in the free_waiters() function, this results in illegal memory access and system crash, here is the crash log: localhost kernel: RIP: 0010:native_queued_spin_lock_slowpath+0x185/0x1e0 localhost kernel: RSP: 0018:aa53c362bd60 EFLAGS: 00010082 localhost kernel: RAX: ff3d3d6bff4007cb RBX: 0282 RCX: 002c localhost kernel: RDX: 9e855eeacb80 RSI: 279c RDI: e7088f6a21d0 localhost kernel: RBP: e7088f6a21d0 R08: 002c R09: aa53c362be64 localhost kernel: R10: aa53c362bbd8 R11: 0001 R12: 0002 localhost kernel: R13: 9e7ead15d600 R14: R15: 9e7ead15d698 localhost kernel: FS: 152a3d111700() GS:9e855ee8() knlGS: localhost kernel: CS: 0010 DS: ES: CR0: 80050033 localhost kernel: CR2: 15293810 CR3: 00044d7a4000 CR4: 003506e0 localhost kernel: Call Trace: localhost kernel: _raw_spin_lock_irqsave+0x30/0x40 localhost kernel: remove_wait_queue+0x12/0x50 localhost kernel: kfd_wait_on_events+0x1b6/0x490 [hydcu] localhost kernel: ? ftrace_graph_caller+0xa0/0xa0 localhost kernel: kfd_ioctl+0x38c/0x4a0 [hydcu] localhost kernel: ? kfd_ioctl_set_trap_handler+0x70/0x70 [hydcu] localhost kernel: ? kfd_ioctl_create_queue+0x5a0/0x5a0 [hydcu] localhost kernel: ? ftrace_graph_caller+0xa0/0xa0 localhost kernel: __x64_sys_ioctl+0x8e/0xd0 localhost kernel: ? syscall_trace_enter.isra.18+0x143/0x1b0 localhost kernel: do_syscall_64+0x33/0x80 localhost kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9 localhost kernel: RIP: 0033:0x152a4dff68d7 Changes since v1: * Allocate the waiter structure using kzalloc, removing the initialization of activated; * '(event_waiters) &&' in the 'for' loop has also been removed. Signed-off-by: Qu Huang --- drivers/gpu/drm/amd/amdkfd/kfd_events.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c index 729d26d..bb54f6c 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c @@ -780,13 +780,12 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events) event_waiters = kmalloc_array(num_events, sizeof(struct kfd_event_waiter), - GFP_KERNEL); + GFP_KERNEL | __GFP_ZERO); This is basically the same as kcalloc. Why not just use that? No need to send another patch. I'll fix it up on my end and apply the patch to amd-staging-drm-next. Reviewed-by: Felix Kuehling Thanks, Felix if (!event_waiters) return NULL; - for (i = 0; (event_waiters) && (i < num_events) ; i++) { + for (i = 0; i < num_events; i++) { init_wait(&event_waiters[i].wait); - event_waiters[i].activated = false; } return event_waiters; -- 1.8.3.1
[drm-misc:for-linux-next 1/1] drivers/gpu/drm/msm/msm_fbdev.c:144:6: warning: variable 'helper' is used uninitialized whenever 'if' condition is true
tree: git://anongit.freedesktop.org/drm/drm-misc for-linux-next head: 3fb1f62f80a1d249260db5ea9e22c51e52fab9ae commit: 3fb1f62f80a1d249260db5ea9e22c51e52fab9ae [1/1] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini() config: arm-buildonly-randconfig-r004-20230219 (https://download.01.org/0day-ci/archive/20230222/202302220810.9dymwcq8-...@intel.com/config) compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project db89896bbbd2251fff457699635acbbedeead27f) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install arm cross compiling tool for clang build # apt-get install binutils-arm-linux-gnueabi git remote add drm-misc git://anongit.freedesktop.org/drm/drm-misc git fetch --no-tags drm-misc for-linux-next git checkout 3fb1f62f80a1d249260db5ea9e22c51e52fab9ae # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/gpu/drm/msm/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot | Link: https://lore.kernel.org/oe-kbuild-all/202302220810.9dymwcq8-...@intel.com/ All warnings (new ones prefixed by >>): >> drivers/gpu/drm/msm/msm_fbdev.c:144:6: warning: variable 'helper' is used >> uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (!fbdev) ^~ drivers/gpu/drm/msm/msm_fbdev.c:173:26: note: uninitialized use occurs here drm_fb_helper_unprepare(helper); ^~ drivers/gpu/drm/msm/msm_fbdev.c:144:2: note: remove the 'if' if its condition is always false if (!fbdev) ^~~ drivers/gpu/drm/msm/msm_fbdev.c:140:30: note: initialize the variable 'helper' to silence this warning struct drm_fb_helper *helper; ^ = NULL 1 warning generated. vim +144 drivers/gpu/drm/msm/msm_fbdev.c c8afe684c95cd17 Rob Clark 2013-06-26 134 c8afe684c95cd17 Rob Clark 2013-06-26 135 /* initialize fbdev helper */ c8afe684c95cd17 Rob Clark 2013-06-26 136 struct drm_fb_helper *msm_fbdev_init(struct drm_device *dev) c8afe684c95cd17 Rob Clark 2013-06-26 137 { c8afe684c95cd17 Rob Clark 2013-06-26 138 struct msm_drm_private *priv = dev->dev_private; c8afe684c95cd17 Rob Clark 2013-06-26 139 struct msm_fbdev *fbdev = NULL; c8afe684c95cd17 Rob Clark 2013-06-26 140 struct drm_fb_helper *helper; 8f67da335d08bc0 Hai Li2014-06-18 141 int ret; c8afe684c95cd17 Rob Clark 2013-06-26 142 c8afe684c95cd17 Rob Clark 2013-06-26 143 fbdev = kzalloc(sizeof(*fbdev), GFP_KERNEL); c8afe684c95cd17 Rob Clark 2013-06-26 @144 if (!fbdev) c8afe684c95cd17 Rob Clark 2013-06-26 145 goto fail; c8afe684c95cd17 Rob Clark 2013-06-26 146 c8afe684c95cd17 Rob Clark 2013-06-26 147 helper = &fbdev->base; c8afe684c95cd17 Rob Clark 2013-06-26 148 6c80a93be62d398 Thomas Zimmermann 2023-01-25 149 drm_fb_helper_prepare(dev, helper, 32, &msm_fb_helper_funcs); c8afe684c95cd17 Rob Clark 2013-06-26 150 2dea2d1182179e7 Pankaj Bharadiya 2020-03-05 151 ret = drm_fb_helper_init(dev, helper); c8afe684c95cd17 Rob Clark 2013-06-26 152 if (ret) { 6a41da17e87dee2 Mamta Shukla 2018-10-20 153 DRM_DEV_ERROR(dev->dev, "could not init fbdev: ret=%d\n", ret); c8afe684c95cd17 Rob Clark 2013-06-26 154 goto fail; c8afe684c95cd17 Rob Clark 2013-06-26 155 } c8afe684c95cd17 Rob Clark 2013-06-26 156 23c259722d0eddf Jeffrey Hugo 2019-06-28 157 /* the fw fb could be anywhere in memory */ 97c9bfe3f6605d4 Thomas Zimmermann 2021-06-29 158 ret = drm_aperture_remove_framebuffers(false, dev->driver); 6848c291a54f8cd Thomas Zimmermann 2021-04-12 159 if (ret) 6848c291a54f8cd Thomas Zimmermann 2021-04-12 160 goto fini; 23c259722d0eddf Jeffrey Hugo 2019-06-28 161 6c80a93be62d398 Thomas Zimmermann 2023-01-25 162 ret = drm_fb_helper_initial_config(helper); 01934c2a6918821 Thierry Reding2014-12-19 163 if (ret) 01934c2a6918821 Thierry Reding2014-12-19 164 goto fini; c8afe684c95cd17 Rob Clark 2013-06-26 165 c8afe684c95cd17 Rob Clark 2013-06-26 166 priv->fbdev = helper; c8afe684c95cd17 Rob Clark 2013-06-26 167 c8afe684c95cd17 Rob Clark 2013-06-26 168 r
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 3:20 PM Rob Clark wrote: > > On Tue, Feb 21, 2023 at 2:46 PM Ville Syrjälä > wrote: > > > > On Tue, Feb 21, 2023 at 02:28:10PM -0800, Rob Clark wrote: > > > On Tue, Feb 21, 2023 at 1:48 PM Ville Syrjälä > > > wrote: > > > > > > > > On Tue, Feb 21, 2023 at 11:39:40PM +0200, Ville Syrjälä wrote: > > > > > On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > > > > > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > > > > > wrote: > > > > > > > > > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on > > > > > > > > > > > fences that an > > > > > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > > > > --- > > > > > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > > @@ -980,6 +980,38 @@ u64 > > > > > > > > > > > drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > > > > > > > > } > > > > > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the > > > > > > > > > > > next vblank > > > > > > > > > > > + * @crtc: the crtc for which to calculate next vblank > > > > > > > > > > > time > > > > > > > > > > > + * @vblanktime: pointer to time to receive the next > > > > > > > > > > > vblank timestamp. > > > > > > > > > > > + * > > > > > > > > > > > + * Calculate the expected time of the next vblank based > > > > > > > > > > > on time of previous > > > > > > > > > > > + * vblank and frame duration > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > for VRR this targets the highest frame rate possible for > > > > > > > > > > the current > > > > > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > > > > > mode->crtc_clock. Presumably for VRR that ends up being a > > > > > > > > > maximum? > > > > > > > > > > > > > > > > I don't know. :-) > > > > > > > > > > > > > > At least for i915 this will give you the maximum frame > > > > > > > duration. > > > > > > > > > > > > > > Also this does not calculate the the start of vblank, it > > > > > > > calculates the start of active video. > > > > > > > > > > > > AFAIU, vsync_end/vsync_start are in units of line, so I could do > > > > > > something like: > > > > > > > > > > > > vsync_lines = vblank->hwmode.vsync_end - > > > > > > vblank->hwmode.vsync_start; > > > > > > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > > > > > > framedur = ns_to_ktime(vblank->framedur_ns); > > > > > > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, > > > > > > vsyncdur)); > > > > > > > > > > Something like this should work: > > > > > vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal > > > > > deadline = vblanktime + vblank_start > > > > > > > > > > That would be the expected time when the next start of vblank > > > > > happens. > > > > > > > > Except that drm_vblank_count_and_time() will give you the last > > > > sampled timestamp, which may be long ago in the past. Would > > > > need to add an _accurate version of that if we want to be > > > > guaranteed a fresh sample. > > > > > > IIRC the only time we wouldn't have a fresh sample is if the screen > > > has been idle for some time? > > > > IIRC "some time" == 1 idle frame, for any driver that sets > > vblank_disable_immediate. > > > > hmm, ok so it should be still good down to 30fps ;-) > > I thought we calculated based on frame # and line # on hw that > supported that? But it's been a while since looking at vblank code looks like drm_get_last_vbltimestamp() is what I want.. > BR, > -R > > > > In which case, I think that doesn't > > > matter. > > > > > > BR, > > > -R > > > > > > > > > > > -- > > > > Ville Syrjälä > > > > Intel > > > > -- > > Ville Syrjälä > > Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 2:46 PM Ville Syrjälä wrote: > > On Tue, Feb 21, 2023 at 02:28:10PM -0800, Rob Clark wrote: > > On Tue, Feb 21, 2023 at 1:48 PM Ville Syrjälä > > wrote: > > > > > > On Tue, Feb 21, 2023 at 11:39:40PM +0200, Ville Syrjälä wrote: > > > > On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > > > > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > > > > wrote: > > > > > > > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences > > > > > > > > > > that an > > > > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > > > --- > > > > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > > @@ -980,6 +980,38 @@ u64 > > > > > > > > > > drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > > > > > > > } > > > > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the > > > > > > > > > > next vblank > > > > > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > > > > > timestamp. > > > > > > > > > > + * > > > > > > > > > > + * Calculate the expected time of the next vblank based on > > > > > > > > > > time of previous > > > > > > > > > > + * vblank and frame duration > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > for VRR this targets the highest frame rate possible for the > > > > > > > > > current > > > > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > > > > mode->crtc_clock. Presumably for VRR that ends up being a > > > > > > > > maximum? > > > > > > > > > > > > > > I don't know. :-) > > > > > > > > > > > > At least for i915 this will give you the maximum frame > > > > > > duration. > > > > > > > > > > > > Also this does not calculate the the start of vblank, it > > > > > > calculates the start of active video. > > > > > > > > > > AFAIU, vsync_end/vsync_start are in units of line, so I could do > > > > > something like: > > > > > > > > > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > > > > > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > > > > > framedur = ns_to_ktime(vblank->framedur_ns); > > > > > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); > > > > > > > > Something like this should work: > > > > vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal > > > > deadline = vblanktime + vblank_start > > > > > > > > That would be the expected time when the next start of vblank > > > > happens. > > > > > > Except that drm_vblank_count_and_time() will give you the last > > > sampled timestamp, which may be long ago in the past. Would > > > need to add an _accurate version of that if we want to be > > > guaranteed a fresh sample. > > > > IIRC the only time we wouldn't have a fresh sample is if the screen > > has been idle for some time? > > IIRC "some time" == 1 idle frame, for any driver that sets > vblank_disable_immediate. > hmm, ok so it should be still good down to 30fps ;-) I thought we calculated based on frame # and line # on hw that supported that? But it's been a while since looking at vblank code BR, -R > > In which case, I think that doesn't > > matter. > > > > BR, > > -R > > > > > > > > -- > > > Ville Syrjälä > > > Intel > > -- > Ville Syrjälä > Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 02:28:10PM -0800, Rob Clark wrote: > On Tue, Feb 21, 2023 at 1:48 PM Ville Syrjälä > wrote: > > > > On Tue, Feb 21, 2023 at 11:39:40PM +0200, Ville Syrjälä wrote: > > > On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > > > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > > > wrote: > > > > > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > > > Rob Clark wrote: > > > > > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > > > wrote: > > > > > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences > > > > > > > > > that an > > > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > > --- > > > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > > > @@ -980,6 +980,38 @@ u64 > > > > > > > > > drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > > > > > > } > > > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the > > > > > > > > > next vblank > > > > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > > > > timestamp. > > > > > > > > > + * > > > > > > > > > + * Calculate the expected time of the next vblank based on > > > > > > > > > time of previous > > > > > > > > > + * vblank and frame duration > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > for VRR this targets the highest frame rate possible for the > > > > > > > > current > > > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > > > mode->crtc_clock. Presumably for VRR that ends up being a > > > > > > > maximum? > > > > > > > > > > > > I don't know. :-) > > > > > > > > > > At least for i915 this will give you the maximum frame > > > > > duration. > > > > > > > > > > Also this does not calculate the the start of vblank, it > > > > > calculates the start of active video. > > > > > > > > AFAIU, vsync_end/vsync_start are in units of line, so I could do > > > > something like: > > > > > > > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > > > > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > > > > framedur = ns_to_ktime(vblank->framedur_ns); > > > > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); > > > > > > Something like this should work: > > > vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal > > > deadline = vblanktime + vblank_start > > > > > > That would be the expected time when the next start of vblank > > > happens. > > > > Except that drm_vblank_count_and_time() will give you the last > > sampled timestamp, which may be long ago in the past. Would > > need to add an _accurate version of that if we want to be > > guaranteed a fresh sample. > > IIRC the only time we wouldn't have a fresh sample is if the screen > has been idle for some time? IIRC "some time" == 1 idle frame, for any driver that sets vblank_disable_immediate. > In which case, I think that doesn't > matter. > > BR, > -R > > > > > -- > > Ville Syrjälä > > Intel -- Ville Syrjälä Intel
Re: [Freedreno] [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 1:48 PM Ville Syrjälä wrote: > > On Tue, Feb 21, 2023 at 11:39:40PM +0200, Ville Syrjälä wrote: > > On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > > wrote: > > > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > > Rob Clark wrote: > > > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > > wrote: > > > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > > Rob Clark wrote: > > > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences > > > > > > > > that an > > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > --- > > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > > > drm_crtc *crtc, > > > > > > > > } > > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > > > +/** > > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > > > vblank > > > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > > > timestamp. > > > > > > > > + * > > > > > > > > + * Calculate the expected time of the next vblank based on > > > > > > > > time of previous > > > > > > > > + * vblank and frame duration > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > for VRR this targets the highest frame rate possible for the > > > > > > > current > > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > > > > > I don't know. :-) > > > > > > > > At least for i915 this will give you the maximum frame > > > > duration. > > > > > > > > Also this does not calculate the the start of vblank, it > > > > calculates the start of active video. > > > > > > AFAIU, vsync_end/vsync_start are in units of line, so I could do > > > something like: > > > > > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > > > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > > > framedur = ns_to_ktime(vblank->framedur_ns); > > > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); > > > > Something like this should work: > > vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal > > deadline = vblanktime + vblank_start > > > > That would be the expected time when the next start of vblank > > happens. > > Except that drm_vblank_count_and_time() will give you the last > sampled timestamp, which may be long ago in the past. Would > need to add an _accurate version of that if we want to be > guaranteed a fresh sample. IIRC the only time we wouldn't have a fresh sample is if the screen has been idle for some time? In which case, I think that doesn't matter. BR, -R > > -- > Ville Syrjälä > Intel
Re: [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 11:39:40PM +0200, Ville Syrjälä wrote: > On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > > wrote: > > > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > > Rob Clark wrote: > > > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > > wrote: > > > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > > Rob Clark wrote: > > > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences that > > > > > > > an > > > > > > > atomic update is waiting on. > > > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > --- > > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > > > > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > > drm_crtc *crtc, > > > > > > > } > > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > > > +/** > > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > > vblank > > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > > timestamp. > > > > > > > + * > > > > > > > + * Calculate the expected time of the next vblank based on time > > > > > > > of previous > > > > > > > + * vblank and frame duration > > > > > > > > > > > > Hi, > > > > > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > > > VRR mode, right? > > > > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > > > I don't know. :-) > > > > > > At least for i915 this will give you the maximum frame > > > duration. > > > > > > Also this does not calculate the the start of vblank, it > > > calculates the start of active video. > > > > AFAIU, vsync_end/vsync_start are in units of line, so I could do something > > like: > > > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > > framedur = ns_to_ktime(vblank->framedur_ns); > > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); > > Something like this should work: > vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal > deadline = vblanktime + vblank_start > > That would be the expected time when the next start of vblank > happens. Except that drm_vblank_count_and_time() will give you the last sampled timestamp, which may be long ago in the past. Would need to add an _accurate version of that if we want to be guaranteed a fresh sample. -- Ville Syrjälä Intel
Re: [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 11:54:55AM -0800, Rob Clark wrote: > On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä > wrote: > > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > Rob Clark wrote: > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > wrote: > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > Rob Clark wrote: > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > > atomic update is waiting on. > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > --- > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > drm_crtc *crtc, > > > > > > } > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > +/** > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > vblank > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > timestamp. > > > > > > + * > > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > > previous > > > > > > + * vblank and frame duration > > > > > > > > > > Hi, > > > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > > VRR mode, right? > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > I don't know. :-) > > > > At least for i915 this will give you the maximum frame > > duration. > > > > Also this does not calculate the the start of vblank, it > > calculates the start of active video. > > AFAIU, vsync_end/vsync_start are in units of line, so I could do something > like: > > vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; > vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); > framedur = ns_to_ktime(vblank->framedur_ns); > *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); Something like this should work: vblank_start = framedur_ns * crtc_vblank_start / crtc_vtotal deadline = vblanktime + vblank_start That would be the expected time when the next start of vblank happens. -- Ville Syrjälä Intel
[PATCH] dt-bindings: display: Start the info graphics with HS/VS change
The VS signal change is synchronized to HS signal change, start the info graphics with that event, instead of having that event occur in the middle of it. Scope trace of DPI bus with HS/VS active HIGH looks as follows: ...__ VS...___/__ __\__... HS...___/ \___/ \__...__/ \___... ^^ || |Used to start here -' | '--- Start info graphics here Signed-off-by: Marek Vasut --- Cc: Daniel Vetter Cc: David Airlie Cc: Krzysztof Kozlowski Cc: Rob Herring Cc: Sam Ravnborg Cc: Thierry Reding Cc: devicet...@vger.kernel.org Cc: dri-devel@lists.freedesktop.org --- .../bindings/display/panel/panel-timing.yaml | 46 +-- 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml index 0d317e61edd8f..aea69b84ca5d8 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-timing.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-timing.yaml @@ -17,29 +17,29 @@ description: | The parameters are defined as seen in the following illustration. - +--+-+--+---+ - | |^| | | - | ||vback_porch | | | - | |v| | | - +--###--+---+ - | #^# | | - | #|# | | - | hback #|# hfront | hsync | - | porch #| hactive # porch | len | - |<>#<---+--->#<>|<->| - | #|# | | - | #|vactive # | | - | #|# | | - | #v# | | - +--###--+---+ - | |^| | | - | ||vfront_porch| | | - | |v| | | - +--+-+--+---+ - | |^| | | - | ||vsync_len | | | - | |v| | | - +--+-+--+---+ + +---+--+-+--+ + | | |^| | + | | ||vsync_len | | + | | |v| | + +---+--+-+--+ + | | |^| | + | | ||vback_porch | | + | | |v| | + +---+--###--+ + | | #^# | + | | #|# | + | hsync | hback #|# hfront | + | len | porch #| hactive # porch | + |<->|<>#<---+--->#<>| + | | #|# | + | | #|vactive # | + | | #|# | + | | #v# | + +---+--###--+ + | | |^| | + | | ||vfront_porch| | + | | |v| | + +---+--+-+--+ The following is the panel timings shown with time on the x-axis. -- 2.39.1
Re: [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä wrote: > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > On Mon, 20 Feb 2023 07:55:41 -0800 > > Rob Clark wrote: > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > wrote: > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > Rob Clark wrote: > > > > > > > > > From: Rob Clark > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > atomic update is waiting on. > > > > > > > > > > Signed-off-by: Rob Clark > > > > > --- > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > include/drm/drm_vblank.h | 1 + > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > drm_crtc *crtc, > > > > > } > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > +/** > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next vblank > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > + * @vblanktime: pointer to time to receive the next vblank timestamp. > > > > > + * > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > previous > > > > > + * vblank and frame duration > > > > > > > > Hi, > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > VRR mode, right? > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > I don't know. :-) > > At least for i915 this will give you the maximum frame > duration. > > Also this does not calculate the the start of vblank, it > calculates the start of active video. AFAIU, vsync_end/vsync_start are in units of line, so I could do something like: vsync_lines = vblank->hwmode.vsync_end - vblank->hwmode.vsync_start; vsyncdur = ns_to_ktime(vblank->linedur_ns * vsync_lines); framedur = ns_to_ktime(vblank->framedur_ns); *vblanktime = ktime_add(*vblanktime, ktime_sub(framedur, vsyncdur)); ? BR, -R > > > > > You need a number of clock cycles in addition to the clock frequency, > > and that could still be minimum, maximum, the last realized one, ... > > > > VRR works by adjusting the front porch length IIRC. > > > > > > Thanks, > > pq > > > > > BR, > > > -R > > > > > > > > > > > > > > Thanks, > > > > pq > > > > > > > > > + */ > > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > > *vblanktime) > > > > > +{ > > > > > + unsigned int pipe = drm_crtc_index(crtc); > > > > > + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe]; > > > > > + u64 count; > > > > > + > > > > > + if (!vblank->framedur_ns) > > > > > + return -EINVAL; > > > > > + > > > > > + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime); > > > > > + > > > > > + /* > > > > > + * If we don't get a valid count, then we probably also don't > > > > > + * have a valid time: > > > > > + */ > > > > > + if (!count) > > > > > + return -EINVAL; > > > > > + > > > > > + *vblanktime = ktime_add(*vblanktime, > > > > > ns_to_ktime(vblank->framedur_ns)); > > > > > + > > > > > + return 0; > > > > > +} > > > > > +EXPORT_SYMBOL(drm_crtc_next_vblank_time); > > > > > + > > > > > static void send_vblank_event(struct drm_device *dev, > > > > > struct drm_pending_vblank_event *e, > > > > > u64 seq, ktime_t now) > > > > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h > > > > > index 733a3e2d1d10..a63bc2c92f3c 100644 > > > > > --- a/include/drm/drm_vblank.h > > > > > +++ b/include/drm/drm_vblank.h > > > > > @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device > > > > > *dev); > > > > > u64 drm_crtc_vblank_count(struct drm_crtc *crtc); > > > > > u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > > ktime_t *vblanktime); > > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > > *vblanktime); > > > > > void drm_crtc_send_vblank_event(struct drm_crtc *crtc, > > > > > struct drm_pending_vblank_event *e); > > > > > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc, > > > > > > > > > > -- > Ville Syrjälä > Intel
Re: [PATCH v4 08/14] drm/scheduler: Add fence deadline support
On 2023-02-18 16:15, Rob Clark wrote: > As the finished fence is the one that is exposed to userspace, and > therefore the one that other operations, like atomic update, would > block on, we need to propagate the deadline from from the finished > fence to the actual hw fence. > > v2: Split into drm_sched_fence_set_parent() (ckoenig) > v3: Ensure a thread calling drm_sched_fence_set_deadline_finished() sees > fence->parent set before drm_sched_fence_set_parent() does this > test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT). > > Signed-off-by: Rob Clark Looks good. This patch is Acked-by: Luben Tuikov -- Regards, Luben > --- > drivers/gpu/drm/scheduler/sched_fence.c | 46 + > drivers/gpu/drm/scheduler/sched_main.c | 2 +- > include/drm/gpu_scheduler.h | 8 + > 3 files changed, 55 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c > b/drivers/gpu/drm/scheduler/sched_fence.c > index 7fd869520ef2..43e2d4f5fe3b 100644 > --- a/drivers/gpu/drm/scheduler/sched_fence.c > +++ b/drivers/gpu/drm/scheduler/sched_fence.c > @@ -123,6 +123,37 @@ static void drm_sched_fence_release_finished(struct > dma_fence *f) > dma_fence_put(&fence->scheduled); > } > > +static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, > + ktime_t deadline) > +{ > + struct drm_sched_fence *fence = to_drm_sched_fence(f); > + struct dma_fence *parent; > + unsigned long flags; > + > + spin_lock_irqsave(&fence->lock, flags); > + > + /* If we already have an earlier deadline, keep it: */ > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && > + ktime_before(fence->deadline, deadline)) { > + spin_unlock_irqrestore(&fence->lock, flags); > + return; > + } > + > + fence->deadline = deadline; > + set_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags); > + > + spin_unlock_irqrestore(&fence->lock, flags); > + > + /* > + * smp_load_aquire() to ensure that if we are racing another > + * thread calling drm_sched_fence_set_parent(), that we see > + * the parent set before it calls test_bit(HAS_DEADLINE_BIT) > + */ > + parent = smp_load_acquire(&fence->parent); > + if (parent) > + dma_fence_set_deadline(parent, deadline); > +} > + > static const struct dma_fence_ops drm_sched_fence_ops_scheduled = { > .get_driver_name = drm_sched_fence_get_driver_name, > .get_timeline_name = drm_sched_fence_get_timeline_name, > @@ -133,6 +164,7 @@ static const struct dma_fence_ops > drm_sched_fence_ops_finished = { > .get_driver_name = drm_sched_fence_get_driver_name, > .get_timeline_name = drm_sched_fence_get_timeline_name, > .release = drm_sched_fence_release_finished, > + .set_deadline = drm_sched_fence_set_deadline_finished, > }; > > struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) > @@ -147,6 +179,20 @@ struct drm_sched_fence *to_drm_sched_fence(struct > dma_fence *f) > } > EXPORT_SYMBOL(to_drm_sched_fence); > > +void drm_sched_fence_set_parent(struct drm_sched_fence *s_fence, > + struct dma_fence *fence) > +{ > + /* > + * smp_store_release() to ensure another thread racing us > + * in drm_sched_fence_set_deadline_finished() sees the > + * fence's parent set before test_bit() > + */ > + smp_store_release(&s_fence->parent, dma_fence_get(fence)); > + if (test_bit(DMA_FENCE_FLAG_HAS_DEADLINE_BIT, > + &s_fence->finished.flags)) > + dma_fence_set_deadline(fence, s_fence->deadline); > +} > + > struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity > *entity, > void *owner) > { > diff --git a/drivers/gpu/drm/scheduler/sched_main.c > b/drivers/gpu/drm/scheduler/sched_main.c > index 4e6ad6e122bc..007f98c48f8d 100644 > --- a/drivers/gpu/drm/scheduler/sched_main.c > +++ b/drivers/gpu/drm/scheduler/sched_main.c > @@ -1019,7 +1019,7 @@ static int drm_sched_main(void *param) > drm_sched_fence_scheduled(s_fence); > > if (!IS_ERR_OR_NULL(fence)) { > - s_fence->parent = dma_fence_get(fence); > + drm_sched_fence_set_parent(s_fence, fence); > /* Drop for original kref_init of the fence */ > dma_fence_put(fence); > > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h > index 9db9e5e504ee..8b31a954a44d 100644 > --- a/include/drm/gpu_scheduler.h > +++ b/include/drm/gpu_scheduler.h > @@ -280,6 +280,12 @@ struct drm_sched_fence { > */ > struct dma_fencefinished; > > + /** > + * @deadline: deadline set on &drm_sched_fence.finished which > + * potentially needs to be propagated to &drm_sched_fence.parent > + */ > + ktime_t
Re: [PATCH] drm/amdkfd: Fix an illegal memory access
Le 21/02/2023 à 17:26, Felix Kuehling a écrit : On 2023-02-21 06:35, qu.huang-fxuvxftifdnyg1zeobx...@public.gmane.org wrote: From: Qu Huang In the kfd_wait_on_events() function, the kfd_event_waiter structure is allocated by alloc_event_waiters(), but the event field of the waiter structure is not initialized; When copy_from_user() fails in the kfd_wait_on_events() function, it will enter exception handling to release the previously allocated memory of the waiter structure; Due to the event field of the waiters structure being accessed in the free_waiters() function, this results in illegal memory access and system crash, here is the crash log: localhost kernel: RIP: 0010:native_queued_spin_lock_slowpath+0x185/0x1e0 localhost kernel: RSP: 0018:aa53c362bd60 EFLAGS: 00010082 localhost kernel: RAX: ff3d3d6bff4007cb RBX: 0282 RCX: 002c localhost kernel: RDX: 9e855eeacb80 RSI: 279c RDI: e7088f6a21d0 localhost kernel: RBP: e7088f6a21d0 R08: 002c R09: aa53c362be64 localhost kernel: R10: aa53c362bbd8 R11: 0001 R12: 0002 localhost kernel: R13: 9e7ead15d600 R14: R15: 9e7ead15d698 localhost kernel: FS: 152a3d111700() GS:9e855ee8() knlGS: localhost kernel: CS: 0010 DS: ES: CR0: 80050033 localhost kernel: CR2: 15293810 CR3: 00044d7a4000 CR4: 003506e0 localhost kernel: Call Trace: localhost kernel: _raw_spin_lock_irqsave+0x30/0x40 localhost kernel: remove_wait_queue+0x12/0x50 localhost kernel: kfd_wait_on_events+0x1b6/0x490 [hydcu] localhost kernel: ? ftrace_graph_caller+0xa0/0xa0 localhost kernel: kfd_ioctl+0x38c/0x4a0 [hydcu] localhost kernel: ? kfd_ioctl_set_trap_handler+0x70/0x70 [hydcu] localhost kernel: ? kfd_ioctl_create_queue+0x5a0/0x5a0 [hydcu] localhost kernel: ? ftrace_graph_caller+0xa0/0xa0 localhost kernel: __x64_sys_ioctl+0x8e/0xd0 localhost kernel: ? syscall_trace_enter.isra.18+0x143/0x1b0 localhost kernel: do_syscall_64+0x33/0x80 localhost kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9 localhost kernel: RIP: 0033:0x152a4dff68d7 Signed-off-by: Qu Huang --- drivers/gpu/drm/amd/amdkfd/kfd_events.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c index 729d26d..e5faaad 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c @@ -787,6 +787,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events) for (i = 0; (event_waiters) && (i < num_events) ; i++) { init_wait(&event_waiters[i].wait); event_waiters[i].activated = false; + event_waiters[i].event = NULL; Thank you for catching this. We're often lazy about initializing things to NULL or 0 because most of our data structures are allocated with kzalloc or similar. I'm not sure why we're not doing this here. If we allocated event_waiters with kcalloc, we could also remove the initialization of activated. I think that would be the cleaner and safer solution. Hi, I think that the '(event_waiters) &&' in the 'for' can also be removed. 'event_waiters' is already NULL tested a few lines above Just my 2c. CJ Regards, Felix } return event_waiters; -- 1.8.3.1
[PATCH v4 4/4] drm/msm/mdp4: Remove empty prepare_commit() function
Remove empty prepare_commit() function from MDP4 driver. Signed-off-by: Jessica Zhang Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 - 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c index 9a1a0769575d..6e37072ed302 100644 --- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c +++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c @@ -84,10 +84,6 @@ static void mdp4_disable_commit(struct msm_kms *kms) mdp4_disable(mdp4_kms); } -static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *state) -{ -} - static void mdp4_flush_commit(struct msm_kms *kms, unsigned crtc_mask) { /* TODO */ @@ -154,7 +150,6 @@ static const struct mdp_kms_funcs kms_funcs = { .disable_vblank = mdp4_disable_vblank, .enable_commit = mdp4_enable_commit, .disable_commit = mdp4_disable_commit, - .prepare_commit = mdp4_prepare_commit, .flush_commit= mdp4_flush_commit, .wait_flush = mdp4_wait_flush, .complete_commit = mdp4_complete_commit, -- 2.39.2
[PATCH v4 3/4] drm/msm/dpu: Remove empty prepare_commit() function
Now that the TE setup has been moved to prepare_for_kickoff(), we have not prepare_commit() callbacks left. This makes dpu_encoder_prepare_commit() do nothing. Remove prepare_commit() from DPU driver. Changes in V3: - Reworded commit message to be more clear - Corrected spelling mistake in commit message Changes in V4: - Reworded commit message for clarity Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 - 3 files changed, 47 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c index dcceed91aed8..35e120b5ef53 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -2090,25 +2090,6 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc) ctl->ops.clear_pending_flush(ctl); } -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc) -{ - struct dpu_encoder_virt *dpu_enc; - struct dpu_encoder_phys *phys; - int i; - - if (!drm_enc) { - DPU_ERROR("invalid encoder\n"); - return; - } - dpu_enc = to_dpu_encoder_virt(drm_enc); - - for (i = 0; i < dpu_enc->num_phys_encs; i++) { - phys = dpu_enc->phys_encs[i]; - if (phys->ops.prepare_commit) - phys->ops.prepare_commit(phys); - } -} - #ifdef CONFIG_DEBUG_FS static int _dpu_encoder_status_show(struct seq_file *s, void *data) { diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 9e7236ef34e6..2c9ef8d1b877 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -146,13 +146,6 @@ struct drm_encoder *dpu_encoder_init( int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc, struct msm_display_info *disp_info); -/** - * dpu_encoder_prepare_commit - prepare encoder at the very beginning of an - * atomic commit, before any registers are written - * @drm_enc:Pointer to previously created drm encoder structure - */ -void dpu_encoder_prepare_commit(struct drm_encoder *drm_enc); - /** * dpu_encoder_set_idle_timeout - set the idle timeout for video *and command mode encoders. diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c index 165958d47ec6..6f7ddbf0d9b7 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c @@ -425,26 +425,6 @@ static ktime_t dpu_kms_vsync_time(struct msm_kms *kms, struct drm_crtc *crtc) return ktime_get(); } -static void dpu_kms_prepare_commit(struct msm_kms *kms, - struct drm_atomic_state *state) -{ - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - struct drm_encoder *encoder; - int i; - - if (!kms) - return; - - /* Call prepare_commit for all affected encoders */ - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - drm_for_each_encoder_mask(encoder, crtc->dev, - crtc_state->encoder_mask) { - dpu_encoder_prepare_commit(encoder); - } - } -} - static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask) { struct dpu_kms *dpu_kms = to_dpu_kms(kms); @@ -949,7 +929,6 @@ static const struct msm_kms_funcs kms_funcs = { .enable_commit = dpu_kms_enable_commit, .disable_commit = dpu_kms_disable_commit, .vsync_time = dpu_kms_vsync_time, - .prepare_commit = dpu_kms_prepare_commit, .flush_commit= dpu_kms_flush_commit, .wait_flush = dpu_kms_wait_flush, .complete_commit = dpu_kms_complete_commit, -- 2.39.2
[PATCH v4 2/4] drm/msm: Check for NULL before calling prepare_commit()
Add a NULL check before calling prepare_commit() in msm_atomic_commit_tail() Signed-off-by: Jessica Zhang Reviewed-by: Dmitry Baryshkov --- drivers/gpu/drm/msm/msm_atomic.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 1686fbb611fd..c8a0a5cc5ca5 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -206,7 +206,8 @@ void msm_atomic_commit_tail(struct drm_atomic_state *state) * Now that there is no in-progress flush, prepare the * current update: */ - kms->funcs->prepare_commit(kms, state); + if (kms->funcs->prepare_commit) + kms->funcs->prepare_commit(kms, state); /* * Push atomic updates down to hardware: -- 2.39.2
[PATCH v4 1/4] drm/msm/dpu: Move TE setup to prepare_for_kickoff()
Currently, DPU will enable TE during prepare_commit(). However, this will cause a crash and reboot to sahara when trying to read/write to register in get_autorefresh_config(), because the core clock rates aren't set at that time. This used to work because phys_enc->hw_pp is only initialized in mode set [1], so the first prepare_commit() will return before any register read/write as hw_pp would be NULL. However, when we try to implement support for INTF TE, we will run into the clock issue described above as hw_intf will *not* be NULL on the first prepare_commit(). This is because the initialization of dpu_enc->hw_intf has been moved to dpu_encoder_setup() [2]. To avoid this issue, let's enable TE during prepare_for_kickoff() instead as the core clock rates are guaranteed to be set then. Depends on: "Implement tearcheck support on INTF block" [3] Changes in V3: - Added function prototypes - Reordered function definitions to make change more legible - Removed prepare_commit() function from dpu_encoder_phys_cmd Changes in V4: - Reworded commit message to be more specific - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() prototype [1] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L1109 [2] https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c#L2339 [3] https://patchwork.freedesktop.org/series/112332/ Signed-off-by: Jessica Zhang --- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c index cb05036f2916..98958c75864a 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c @@ -40,6 +40,8 @@ #define DPU_ENC_MAX_POLL_TIMEOUT_US2000 +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc); + static bool dpu_encoder_phys_cmd_is_master(struct dpu_encoder_phys *phys_enc) { return (phys_enc->split_role != ENC_ROLE_SLAVE); @@ -611,6 +613,8 @@ static void dpu_encoder_phys_cmd_prepare_for_kickoff( phys_enc->hw_pp->idx - PINGPONG_0); } + dpu_encoder_phys_cmd_enable_te(phys_enc); + DPU_DEBUG_CMDENC(cmd_enc, "pp:%d pending_cnt %d\n", phys_enc->hw_pp->idx - PINGPONG_0, atomic_read(&phys_enc->pending_kickoff_cnt)); @@ -641,8 +645,7 @@ static bool dpu_encoder_phys_cmd_is_ongoing_pptx( return false; } -static void dpu_encoder_phys_cmd_prepare_commit( - struct dpu_encoder_phys *phys_enc) +static void dpu_encoder_phys_cmd_enable_te(struct dpu_encoder_phys *phys_enc) { struct dpu_encoder_phys_cmd *cmd_enc = to_dpu_encoder_phys_cmd(phys_enc); @@ -799,7 +802,6 @@ static void dpu_encoder_phys_cmd_trigger_start( static void dpu_encoder_phys_cmd_init_ops( struct dpu_encoder_phys_ops *ops) { - ops->prepare_commit = dpu_encoder_phys_cmd_prepare_commit; ops->is_master = dpu_encoder_phys_cmd_is_master; ops->atomic_mode_set = dpu_encoder_phys_cmd_atomic_mode_set; ops->enable = dpu_encoder_phys_cmd_enable; -- 2.39.2
[PATCH v4 0/4] Move TE setup to prepare_for_kickoff()
Move TE setup to prepare_for_kickoff() and remove empty prepare_commit() functions in both MDP4 and DPU drivers. Changes in V2: - Added changes to remove empty prepare_commit() functions Changes in V3: - Reordered "drm/msm/dpu: Move TE setup to prepare_for_kickoff()" for clarity - Fixed spelling mistakes and wording issues - Picked up "Reviewed-by" tags for patches [2/4] and [4/4] Changes in V4: - Reworded commit messages in [1/4] and [3/4] for clarity - Removed dpu_encoder_phys_cmd_is_ongoing_pptx() function prototype Jessica Zhang (4): drm/msm/dpu: Move TE setup to prepare_for_kickoff() drm/msm: Check for NULL before calling prepare_commit() drm/msm/dpu: Remove empty prepare_commit() function drm/msm/mdp4: Remove empty prepare_commit() function drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 19 - drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 7 --- .../drm/msm/disp/dpu1/dpu_encoder_phys_cmd.c | 8 --- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 21 --- drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 5 - drivers/gpu/drm/msm/msm_atomic.c | 3 ++- 6 files changed, 7 insertions(+), 56 deletions(-) -- 2.39.2
Re: [PATCH drm-next v2 04/16] maple_tree: add flag MT_FLAGS_LOCK_NONE
On Tue, Feb 21, 2023 at 03:37:49PM +0100, Danilo Krummrich wrote: > On Mon, Feb 20, 2023 at 08:33:35PM +, Matthew Wilcox wrote: > > On Mon, Feb 20, 2023 at 06:06:03PM +0100, Danilo Krummrich wrote: > > > On 2/20/23 16:10, Matthew Wilcox wrote: > > > > This is why we like people to use the spinlock embedded in the tree. > > > > There's nothing for the user to care about. If the access really is > > > > serialised, acquiring/releasing the uncontended spinlock is a minimal > > > > cost compared to all the other things that will happen while modifying > > > > the tree. > > > > > > I think as for the users of the GPUVA manager we'd have two cases: > > > > > > 1) Accesses to the manager (and hence the tree) are serialized, no lock > > > needed. > > > > > > 2) Multiple operations on the tree must be locked in order to make them > > > appear atomic. > > > > Could you give an example here of what you'd like to do? Ideally > > something complicated so I don't say "Oh, you can just do this" when > > there's a more complex example for which "this" won't work. I'm sure > > that's embedded somewhere in the next 20-odd patches, but it's probably > > quicker for you to describe in terms of tree operations that have to > > appear atomic than for me to try to figure it out. > > > > Absolutely, not gonna ask you to read all of that. :-) > > One thing the GPUVA manager does is to provide drivers the (sub-)operations > that need to be processed in order to fulfill a map or unmap request from > userspace. For instance, when userspace asks the driver to map some memory > the GPUVA manager calculates which existing mappings must be removed, split up > or can be merged with the newly requested mapping. > > A driver has two ways to fetch those operations from the GPUVA manager. It can > either obtain a list of operations or receive a callback for each operation > generated by the GPUVA manager. > > In both cases the GPUVA manager walks the maple tree, which keeps track of > existing mappings, for the given range in __drm_gpuva_sm_map() (only > considering > the map case, since the unmap case is a subset basically). For each mapping > found in the given range the driver, as mentioned, either receives a callback > or > a list entry is added to the list of operations. > > Typically, for each operation / callback one entry within the maple tree is > removed and, optionally at the beginning and end of a new mapping's range, a > new entry is inserted. An of course, as the last operation, there is the new > mapping itself to insert. > > The GPUVA manager delegates locking responsibility to the drivers. Typically, > a driver either serializes access to the VA space managed by the GPUVA manager > (no lock needed) or need to lock the processing of a full set of operations > generated by the GPUVA manager. OK, that all makes sense. It does make sense to have the driver use its own mutex and then take the spinlock inside the maple tree code. It shouldn't ever be contended. > > > In either case the embedded spinlock wouldn't be useful, we'd either need > > > an > > > external lock or no lock at all. > > > > > > If there are any internal reasons why specific tree operations must be > > > mutually excluded (such as those you explain below), wouldn't it make more > > > sense to always have the internal lock and, optionally, allow users to > > > specify an external lock additionally? > > > > So the way this works for the XArray, which is a little older than the > > Maple tree, is that we always use the internal spinlock for > > modifications (possibly BH or IRQ safe), and if someone wants to > > use an external mutex to make some callers atomic with respect to each > > other, they're free to do so. In that case, the XArray doesn't check > > the user's external locking at all, because it really can't know. > > > > I'd advise taking that approach; if there's really no way to use the > > internal spinlock to make your complicated updates appear atomic > > then just let the maple tree use its internal spinlock, and you can > > also use your external mutex however you like. > > > > That sounds like the right thing to do. > > However, I'm using the advanced API of the maple tree (and that's the reason > why the above example appears a little more detailed than needed) because I > think with the normal API I can't insert / remove tree entries while walking > the tree, right? Right. The normal API is for simple operations while the advanced API is for doing compound operations. > As by the documentation the advanced API, however, doesn't take care of > locking > itself, hence just letting the maple tree use its internal spinlock doesn't > really work - I need to take care of that myself, right? Yes; once you're using the advanced API, you get to compose the entire operation yourself. > It feels a bit weird that I, as a user of the API, would need to lock certain > (or all?) mas_*() functions with the internal spinlock
Re: [PATCH] drm/i915/sseu: fix max_subslices array-index-out-of-bounds access
On Tue, Feb 21, 2023 at 09:01:24AM +, Tvrtko Ursulin wrote: > > > On 20/02/2023 17:18, Andrea Righi wrote: > > It seems that commit bc3c5e0809ae ("drm/i915/sseu: Don't try to store EU > > mask internally in UAPI format") exposed a potential out-of-bounds > > access, reported by UBSAN as following on a laptop with a gen 11 i915 > > card: > > > >UBSAN: array-index-out-of-bounds in > > drivers/gpu/drm/i915/gt/intel_sseu.c:65:27 > >index 6 is out of range for type 'u16 [6]' > >CPU: 2 PID: 165 Comm: systemd-udevd Not tainted 6.2.0-9-generic #9-Ubuntu > >Hardware name: Dell Inc. XPS 13 9300/077Y9N, BIOS 1.11.0 03/22/2022 > >Call Trace: > > > > show_stack+0x4e/0x61 > > dump_stack_lvl+0x4a/0x6f > > dump_stack+0x10/0x18 > > ubsan_epilogue+0x9/0x3a > > __ubsan_handle_out_of_bounds.cold+0x42/0x47 > > gen11_compute_sseu_info+0x121/0x130 [i915] > > intel_sseu_info_init+0x15d/0x2b0 [i915] > > intel_gt_init_mmio+0x23/0x40 [i915] > > i915_driver_mmio_probe+0x129/0x400 [i915] > > ? intel_gt_probe_all+0x91/0x2e0 [i915] > > i915_driver_probe+0xe1/0x3f0 [i915] > > ? drm_privacy_screen_get+0x16d/0x190 [drm] > > ? acpi_dev_found+0x64/0x80 > > i915_pci_probe+0xac/0x1b0 [i915] > > ... > > > > According to the definition of sseu_dev_info, eu_mask->hsw is limited to > > a maximum of GEN_MAX_SS_PER_HSW_SLICE (6) sub-slices, but > > gen11_sseu_info_init() can potentially set 8 sub-slices, in the > > !IS_JSL_EHL(gt->i915) case. > > > > Fix this by reserving up to 8 slots for max_subslices in the eu_mask > > struct. > > > > Reported-by: Emil Renner Berthing > > Signed-off-by: Andrea Righi > > Looks like bug was probably introduced in: > > Fixes: bc3c5e0809ae ("drm/i915/sseu: Don't try to store EU mask internally in > UAPI format") > Cc: Matt Roper > Cc: Balasubramani Vivekanandan > Cc: # v6.0+ > > Adding Matt to cross-check. Yep, looks like there's one specific SKU of ICL that has 8 subslices that we overlooked previously. Reviewed-by: Matt Roper > > Regards, > > Tvrtko > > > --- > > drivers/gpu/drm/i915/gt/intel_sseu.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_sseu.h > > b/drivers/gpu/drm/i915/gt/intel_sseu.h > > index aa87d3832d60..d7e8c374f153 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_sseu.h > > +++ b/drivers/gpu/drm/i915/gt/intel_sseu.h > > @@ -27,7 +27,7 @@ struct drm_printer; > >* is only relevant to pre-Xe_HP platforms (Xe_HP and beyond use the > >* I915_MAX_SS_FUSE_BITS value below). > >*/ > > -#define GEN_MAX_SS_PER_HSW_SLICE 6 > > +#define GEN_MAX_SS_PER_HSW_SLICE 8 > > /* > >* Maximum number of 32-bit registers used by hardware to express the -- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
Re: [PATCH drm-next v2 05/16] drm: manager to keep track of GPUs VA mappings
* Danilo Krummrich [230217 08:45]: > Add infrastructure to keep track of GPU virtual address (VA) mappings > with a decicated VA space manager implementation. > > New UAPIs, motivated by Vulkan sparse memory bindings graphics drivers > start implementing, allow userspace applications to request multiple and > arbitrary GPU VA mappings of buffer objects. The DRM GPU VA manager is > intended to serve the following purposes in this context. > > 1) Provide infrastructure to track GPU VA allocations and mappings, >making use of the maple_tree. > > 2) Generically connect GPU VA mappings to their backing buffers, in >particular DRM GEM objects. > > 3) Provide a common implementation to perform more complex mapping >operations on the GPU VA space. In particular splitting and merging >of GPU VA mappings, e.g. for intersecting mapping requests or partial >unmap requests. > > Suggested-by: Dave Airlie > Signed-off-by: Danilo Krummrich > --- > Documentation/gpu/drm-mm.rst| 31 + > drivers/gpu/drm/Makefile|1 + > drivers/gpu/drm/drm_gem.c |3 + > drivers/gpu/drm/drm_gpuva_mgr.c | 1704 +++ > include/drm/drm_drv.h |6 + > include/drm/drm_gem.h | 75 ++ > include/drm/drm_gpuva_mgr.h | 714 + > 7 files changed, 2534 insertions(+) > create mode 100644 drivers/gpu/drm/drm_gpuva_mgr.c > create mode 100644 include/drm/drm_gpuva_mgr.h > > diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst > index a52e6f4117d6..c9f120cfe730 100644 > --- a/Documentation/gpu/drm-mm.rst > +++ b/Documentation/gpu/drm-mm.rst > @@ -466,6 +466,37 @@ DRM MM Range Allocator Function References > .. kernel-doc:: drivers/gpu/drm/drm_mm.c > :export: > ... > + > +/** > + * drm_gpuva_remove_iter - removes the iterators current element > + * @it: the &drm_gpuva_iterator > + * > + * This removes the element the iterator currently points to. > + */ > +void > +drm_gpuva_iter_remove(struct drm_gpuva_iterator *it) > +{ > + mas_erase(&it->mas); > +} > +EXPORT_SYMBOL(drm_gpuva_iter_remove); > + > +/** > + * drm_gpuva_insert - insert a &drm_gpuva > + * @mgr: the &drm_gpuva_manager to insert the &drm_gpuva in > + * @va: the &drm_gpuva to insert > + * @addr: the start address of the GPU VA > + * @range: the range of the GPU VA > + * > + * Insert a &drm_gpuva with a given address and range into a > + * &drm_gpuva_manager. > + * > + * Returns: 0 on success, negative error code on failure. > + */ > +int > +drm_gpuva_insert(struct drm_gpuva_manager *mgr, > + struct drm_gpuva *va) > +{ > + u64 addr = va->va.addr; > + u64 range = va->va.range; > + MA_STATE(mas, &mgr->va_mt, addr, addr + range - 1); > + struct drm_gpuva_region *reg = NULL; > + int ret; > + > + if (unlikely(!drm_gpuva_in_mm_range(mgr, addr, range))) > + return -EINVAL; > + > + if (unlikely(drm_gpuva_in_kernel_region(mgr, addr, range))) > + return -EINVAL; > + > + if (mgr->flags & DRM_GPUVA_MANAGER_REGIONS) { > + reg = drm_gpuva_in_region(mgr, addr, range); > + if (unlikely(!reg)) > + return -EINVAL; > + } > + - > + if (unlikely(drm_gpuva_find_first(mgr, addr, range))) > + return -EEXIST; > + > + ret = mas_store_gfp(&mas, va, GFP_KERNEL); mas_walk() will set the internal maple state to the limits to what it finds. So, instead of an iterator, you can use the walk function and ensure there is a large enough area in the existing NULL: /* * Nothing at addr, mas now points to the location where the store would * happen */ if (mas_walk(&mas)) return -EEXIST; /* The NULL entry ends at mas.last, make sure there is room */ if (mas.last < (addr + range - 1)) return -EEXIST; /* Limit the store size to the correct end address, and store */ mas.last = addr + range - 1; ret = mas_store_gfp(&mas, va, GFP_KERNEL); > + if (unlikely(ret)) > + return ret; > + > + va->mgr = mgr; > + va->region = reg; > + > + return 0; > +} > +EXPORT_SYMBOL(drm_gpuva_insert); > + > +/** > + * drm_gpuva_remove - remove a &drm_gpuva > + * @va: the &drm_gpuva to remove > + * > + * This removes the given &va from the underlaying tree. > + */ > +void > +drm_gpuva_remove(struct drm_gpuva *va) > +{ > + MA_STATE(mas, &va->mgr->va_mt, va->va.addr, 0); > + > + mas_erase(&mas); > +} > +EXPORT_SYMBOL(drm_gpuva_remove); > + ... > +/** > + * drm_gpuva_find_first - find the first &drm_gpuva in the given range > + * @mgr: the &drm_gpuva_manager to search in > + * @addr: the &drm_gpuvas address > + * @range: the &drm_gpuvas range > + * > + * Returns: the first &drm_gpuva within the given range > + */ > +struct drm_gpuva * > +drm_gpuva_find_first(struct drm_gpuva_manager *mgr, > + u64 addr, u64 range) > +{ > + MA_STATE(mas, &mgr->va_mt, addr, 0); > + > + return mas_find(&mas,
Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, Feb 21, 2023 at 8:01 AM Sebastian Wick wrote: > > On Tue, Feb 21, 2023 at 9:38 AM Pekka Paalanen wrote: > > > > On Mon, 20 Feb 2023 08:14:47 -0800 > > Rob Clark wrote: > > > > > On Mon, Feb 20, 2023 at 12:53 AM Pekka Paalanen > > > wrote: > > > > > > > > On Sat, 18 Feb 2023 13:15:49 -0800 > > > > Rob Clark wrote: > > > > > > > > > From: Rob Clark > > > > > > > > > > Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent > > > > > wait (as opposed to a "housekeeping" wait to know when to cleanup > > > > > after > > > > > some work has completed). Usermode components of GPU driver stacks > > > > > often poll() on fence fd's to know when it is safe to do things like > > > > > free or reuse a buffer, but they can also poll() on a fence fd when > > > > > waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag > > > > > lets the kernel differentiate these two cases. > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > Hi, > > > > > > > > where would the UAPI documentation of this go? > > > > It seems to be missing. > > > > > > Good question, I am not sure. The poll() man page has a description, > > > but my usage doesn't fit that _exactly_ (but OTOH the description is a > > > bit vague). > > > > > > > If a Wayland compositor is polling application fences to know which > > > > client buffer to use in its rendering, should the compositor poll with > > > > PRI or not? If a compositor polls with PRI, then all fences from all > > > > applications would always be PRI. Would that be harmful somehow or > > > > would it be beneficial? > > > > > > I think a compositor would rather use the deadline ioctl and then poll > > > without PRI. Otherwise you are giving an urgency signal to the fence > > > signaller which might not necessarily be needed. > > > > > > The places where I expect PRI to be useful is more in mesa (things > > > like glFinish(), readpix, and other similar sorts of blocking APIs) > > > > Sounds good. Docs... ;-) > > > > Hmm, so a compositor should set the deadline when it processes the > > wl_surface.commit, and not when it actually starts repainting, to give > > time for the driver to react and the GPU to do some more work. The > > deadline would be the time when the compositor starts its repaint, so > > it knows if the buffer is ready or not. > > Technically we don't know when the commit is supposed to be shown. > Just passing a deadline of the next possible deadline however is > probably a good enough guess for this feature to be useful. > > One thing that neither API allows us to do is tell the kernel in > advance when we're going to submit work and what the deadline for it > is and unfortunately that work is the most timing sensitive. Presumably you are talking about the final compositing step? Elsewhere in this series that atomic wait-for-fences step sets the deadline hint. BR, -R > > > > > > Thanks, > > pq > > > > > > > > > > BR, > > > -R > > > > > > > > > > > > > > > Thanks, > > > > pq > > > > > > > > > --- > > > > > drivers/dma-buf/sync_file.c | 8 > > > > > 1 file changed, 8 insertions(+) > > > > > > > > > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > > > > > index fb6ca1032885..c30b2085ee0a 100644 > > > > > --- a/drivers/dma-buf/sync_file.c > > > > > +++ b/drivers/dma-buf/sync_file.c > > > > > @@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file > > > > > *file, poll_table *wait) > > > > > { > > > > > struct sync_file *sync_file = file->private_data; > > > > > > > > > > + /* > > > > > + * The POLLPRI/EPOLLPRI flag can be used to signal that > > > > > + * userspace wants the fence to signal ASAP, express this > > > > > + * as an immediate deadline. > > > > > + */ > > > > > + if (poll_requested_events(wait) & EPOLLPRI) > > > > > + dma_fence_set_deadline(sync_file->fence, ktime_get()); > > > > > + > > > > > poll_wait(file, &sync_file->wq, wait); > > > > > > > > > > if (list_empty(&sync_file->cb.node) && > > > > > > >
Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, Feb 21, 2023 at 8:48 AM Luben Tuikov wrote: > > On 2023-02-20 11:14, Rob Clark wrote: > > On Mon, Feb 20, 2023 at 12:53 AM Pekka Paalanen wrote: > >> > >> On Sat, 18 Feb 2023 13:15:49 -0800 > >> Rob Clark wrote: > >> > >>> From: Rob Clark > >>> > >>> Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent > >>> wait (as opposed to a "housekeeping" wait to know when to cleanup after > >>> some work has completed). Usermode components of GPU driver stacks > >>> often poll() on fence fd's to know when it is safe to do things like > >>> free or reuse a buffer, but they can also poll() on a fence fd when > >>> waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag > >>> lets the kernel differentiate these two cases. > >>> > >>> Signed-off-by: Rob Clark > >> > >> Hi, > >> > >> where would the UAPI documentation of this go? > >> It seems to be missing. > > > > Good question, I am not sure. The poll() man page has a description, > > but my usage doesn't fit that _exactly_ (but OTOH the description is a > > bit vague). > > > >> If a Wayland compositor is polling application fences to know which > >> client buffer to use in its rendering, should the compositor poll with > >> PRI or not? If a compositor polls with PRI, then all fences from all > >> applications would always be PRI. Would that be harmful somehow or > >> would it be beneficial? > > > > I think a compositor would rather use the deadline ioctl and then poll > > without PRI. Otherwise you are giving an urgency signal to the fence > > signaller which might not necessarily be needed. > > > > The places where I expect PRI to be useful is more in mesa (things > > like glFinish(), readpix, and other similar sorts of blocking APIs) > Hi, > > Hmm, but then user-space could do the opposite, namely, submit work as > usual--never > using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. > That seems > like a possible usage pattern, unintended--maybe, but possible. Do we want to > discourage > this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with > the current > time, and then wouldn't that be equivalent to (E)POLLPRI? Yeah, (E)POLLPRI isn't strictly needed if we have SET_DEADLINE. It is slightly more convenient if you want an immediate deadline (single syscall instead of two), but not strictly needed. OTOH it piggy-backs on existing UABI. BR, -R
Re: [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 5:01 AM Ville Syrjälä wrote: > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > On Mon, 20 Feb 2023 07:55:41 -0800 > > Rob Clark wrote: > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > wrote: > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > Rob Clark wrote: > > > > > > > > > From: Rob Clark > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > atomic update is waiting on. > > > > > > > > > > Signed-off-by: Rob Clark > > > > > --- > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > include/drm/drm_vblank.h | 1 + > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > drm_crtc *crtc, > > > > > } > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > +/** > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next vblank > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > + * @vblanktime: pointer to time to receive the next vblank timestamp. > > > > > + * > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > previous > > > > > + * vblank and frame duration > > > > > > > > Hi, > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > VRR mode, right? > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > I don't know. :-) > > At least for i915 this will give you the maximum frame > duration. I suppose one could argue that maximum frame duration is the actual deadline. Anything less is just moar fps, but not going to involve stalling until vblank N+1, AFAIU > Also this does not calculate the the start of vblank, it > calculates the start of active video. Probably something like end of previous frame's video.. might not be _exactly_ correct (because some buffering involved), but OTOH on the GPU side, I expect the driver to set a timer for a few ms or so before the deadline. So there is some wiggle room. BR, -R > > > > You need a number of clock cycles in addition to the clock frequency, > > and that could still be minimum, maximum, the last realized one, ... > > > > VRR works by adjusting the front porch length IIRC. > > > > > > Thanks, > > pq > > > > > BR, > > > -R > > > > > > > > > > > > > > Thanks, > > > > pq > > > > > > > > > + */ > > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > > *vblanktime) > > > > > +{ > > > > > + unsigned int pipe = drm_crtc_index(crtc); > > > > > + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe]; > > > > > + u64 count; > > > > > + > > > > > + if (!vblank->framedur_ns) > > > > > + return -EINVAL; > > > > > + > > > > > + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime); > > > > > + > > > > > + /* > > > > > + * If we don't get a valid count, then we probably also don't > > > > > + * have a valid time: > > > > > + */ > > > > > + if (!count) > > > > > + return -EINVAL; > > > > > + > > > > > + *vblanktime = ktime_add(*vblanktime, > > > > > ns_to_ktime(vblank->framedur_ns)); > > > > > + > > > > > + return 0; > > > > > +} > > > > > +EXPORT_SYMBOL(drm_crtc_next_vblank_time); > > > > > + > > > > > static void send_vblank_event(struct drm_device *dev, > > > > > struct drm_pending_vblank_event *e, > > > > > u64 seq, ktime_t now) > > > > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h > > > > > index 733a3e2d1d10..a63bc2c92f3c 100644 > > > > > --- a/include/drm/drm_vblank.h > > > > > +++ b/include/drm/drm_vblank.h > > > > > @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device > > > > > *dev); > > > > > u64 drm_crtc_vblank_count(struct drm_crtc *crtc); > > > > > u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > > ktime_t *vblanktime); > > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > > *vblanktime); > > > > > void drm_crtc_send_vblank_event(struct drm_crtc *crtc, > > > > > struct drm_pending_vblank_event *e); > > > > > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc, > > > > > > > > > > -- > Ville Syrjälä > Intel
Re: [Intel-gfx] [PATCH 7/7] drm/ttm: cleanup ttm_range_mgr_node
On Fri, 17 Feb 2023 at 12:23, Christian König wrote: > > We don't need multiple drm_mm nodes any more. Clean that up and remove > the extra complexity. > > Signed-off-by: Christian König Reviewed-by: Matthew Auld
Re: [PATCH 5/7] drm/gem: Remove BUG_ON in drm_gem_private_object_init
On Fri, 17 Feb 2023 at 12:23, Christian König wrote: > > From: Somalapuram Amaranath > > ttm_resource can allocate size in bytes to support less than page size. > > Signed-off-by: Somalapuram Amaranath > Reviewed-by: Christian König > Signed-off-by: Christian König > Link: > https://patchwork.freedesktop.org/patch/msgid/20230208090106.9659-1-amaranath.somalapu...@amd.com > --- > drivers/gpu/drm/drm_gem.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c > index aa15c52ae182..5a3ca3363f82 100644 > --- a/drivers/gpu/drm/drm_gem.c > +++ b/drivers/gpu/drm/drm_gem.c > @@ -152,8 +152,6 @@ EXPORT_SYMBOL(drm_gem_object_init); > void drm_gem_private_object_init(struct drm_device *dev, > struct drm_gem_object *obj, size_t size) > { > - BUG_ON((size & (PAGE_SIZE - 1)) != 0); > - There are also some comments in drm_gem_{get, put}_pages referring to this exact BUG_ON(), which could do with updating now. > obj->dev = dev; > obj->filp = NULL; > > -- > 2.34.1 >
Re: [PATCH v5 09/14] drm/syncobj: Add deadline support for syncobj waits
On Tue, Feb 21, 2023 at 12:53 AM Pekka Paalanen wrote: > > On Mon, 20 Feb 2023 12:18:56 -0800 > Rob Clark wrote: > > > From: Rob Clark > > > > Add a new flag to let userspace provide a deadline as a hint for syncobj > > and timeline waits. This gives a hint to the driver signaling the > > backing fences about how soon userspace needs it to compete work, so it > > can addjust GPU frequency accordingly. An immediate deadline can be > > given to provide something equivalent to i915 "wait boost". > > > > v2: Use absolute u64 ns value for deadline hint, drop cap and driver > > feature flag in favor of allowing count_handles==0 as a way for > > userspace to probe kernel for support of new flag > > > > Signed-off-by: Rob Clark > > --- > > drivers/gpu/drm/drm_syncobj.c | 59 +++ > > include/uapi/drm/drm.h| 5 +++ > > 2 files changed, 51 insertions(+), 13 deletions(-) > > ... > > > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h > > index 642808520d92..aefc8cc743e0 100644 > > --- a/include/uapi/drm/drm.h > > +++ b/include/uapi/drm/drm.h > > @@ -887,6 +887,7 @@ struct drm_syncobj_transfer { > > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0) > > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1) > > #define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2) /* wait for time > > point to become available */ > > +#define DRM_SYNCOBJ_WAIT_FLAGS_WAIT_DEADLINE (1 << 3) /* set fence > > deadline based to deadline_nsec/sec */ > > Hi, > > where is the UAPI documentation explaining what is a "fence deadline" > and what setting it does here? It's with the rest of the drm_syncobj UAPI docs ;-) BR, -R > btw. no nsec/sec anymore. > > > Thanks, > pq > > > > struct drm_syncobj_wait { > > __u64 handles; > > /* absolute timeout */ > > @@ -895,6 +896,8 @@ struct drm_syncobj_wait { > > __u32 flags; > > __u32 first_signaled; /* only valid when not waiting all */ > > __u32 pad; > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */ > > + __u64 deadline_ns; > > }; > > > > struct drm_syncobj_timeline_wait { > > @@ -907,6 +910,8 @@ struct drm_syncobj_timeline_wait { > > __u32 flags; > > __u32 first_signaled; /* only valid when not waiting all */ > > __u32 pad; > > + /* Deadline hint to set on backing fence(s) in CLOCK_MONOTONIC: */ > > + __u64 deadline_ns; > > }; > > > > >
[PATCH] dt-bindings: display: bridge: parade, ps8622: convert to dtschema
Convert the Parade PS8622/PS8625 DisplayPort to LVDS Converter bindings to DT schema. Changes during conversion: add missing vdd12-supply, used by Linux driver. Signed-off-by: Krzysztof Kozlowski --- .../display/bridge/parade,ps8622.yaml | 115 ++ .../bindings/display/bridge/ps8622.txt| 31 - 2 files changed, 115 insertions(+), 31 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/parade,ps8622.yaml delete mode 100644 Documentation/devicetree/bindings/display/bridge/ps8622.txt diff --git a/Documentation/devicetree/bindings/display/bridge/parade,ps8622.yaml b/Documentation/devicetree/bindings/display/bridge/parade,ps8622.yaml new file mode 100644 index ..e6397ac2048b --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/parade,ps8622.yaml @@ -0,0 +1,115 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/bridge/parade,ps8622.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Parade PS8622/PS8625 DisplayPort to LVDS Converter + +maintainers: + - Krzysztof Kozlowski + +properties: + compatible: +enum: + - parade,ps8622 + - parade,ps8625 + + reg: +maxItems: 1 + + lane-count: +$ref: /schemas/types.yaml#/definitions/uint32 +enum: [1, 2] +description: Number of DP lanes to use. + + use-external-pwm: +type: boolean +description: Backlight will be controlled by an external PWM. + + reset-gpios: +maxItems: 1 +description: GPIO connected to RST_ pin. + + sleep-gpios: +maxItems: 1 +description: GPIO connected to PD_ pin. + + vdd12-supply: true + + ports: +$ref: /schemas/graph.yaml#/properties/ports + +properties: + port@0: +$ref: /schemas/graph.yaml#/properties/port +description: Video port for LVDS output. + + port@1: +$ref: /schemas/graph.yaml#/properties/port +description: Video port for DisplayPort input. + +required: + - port@0 + - port@1 + +required: + - compatible + - reg + - reset-gpios + - sleep-gpios + - ports + +allOf: + - if: + properties: +compatible: + const: parade,ps8622 +then: + properties: +lane-count: + const: 1 +else: + properties: +lane-count: + const: 2 + +additionalProperties: false + +examples: + - | +#include +i2c { +#address-cells = <1>; +#size-cells = <0>; + +lvds-bridge@48 { +compatible = "parade,ps8625"; +reg = <0x48>; +sleep-gpios = <&gpx3 5 GPIO_ACTIVE_HIGH>; +reset-gpios = <&gpy7 7 GPIO_ACTIVE_HIGH>; +lane-count = <2>; +use-external-pwm; + +ports { +#address-cells = <1>; +#size-cells = <0>; + +port@0 { +reg = <0>; + +bridge_out: endpoint { +remote-endpoint = <&panel_in>; +}; +}; + +port@1 { +reg = <1>; + +bridge_in: endpoint { +remote-endpoint = <&dp_out>; +}; +}; +}; +}; +}; diff --git a/Documentation/devicetree/bindings/display/bridge/ps8622.txt b/Documentation/devicetree/bindings/display/bridge/ps8622.txt deleted file mode 100644 index c989c3807f2b.. --- a/Documentation/devicetree/bindings/display/bridge/ps8622.txt +++ /dev/null @@ -1,31 +0,0 @@ -ps8622-bridge bindings - -Required properties: - - compatible: "parade,ps8622" or "parade,ps8625" - - reg: first i2c address of the bridge - - sleep-gpios: OF device-tree gpio specification for PD_ pin. - - reset-gpios: OF device-tree gpio specification for RST_ pin. - -Optional properties: - - lane-count: number of DP lanes to use - - use-external-pwm: backlight will be controlled by an external PWM - - video interfaces: Device node can contain video interface port - nodes for panel according to [1]. - -[1]: Documentation/devicetree/bindings/media/video-interfaces.txt - -Example: - lvds-bridge@48 { - compatible = "parade,ps8622"; - reg = <0x48>; - sleep-gpios = <&gpc3 6 1 0 0>; - reset-gpios = <&gpc3 1 1 0 0>; - lane-count = <1>; - ports { - port@0 { - bridge_out: endpoint { - remote-endpoint = <&panel_in>; - }; - }; - }; - }; -- 2.34.1
Re: [PATCH drm-next v2 03/16] maple_tree: split up MA_STATE() macro
* Danilo Krummrich [230220 09:38]: > On 2/17/23 19:34, Liam R. Howlett wrote: > > * Danilo Krummrich [230217 08:44]: > > > Split up the MA_STATE() macro such that components using the maple tree > > > can easily inherit from struct ma_state and build custom tree walk > > > macros to hide their internals from users. > > > > > > Example: > > > > > > struct sample_iter { > > > struct ma_state mas; > > > struct sample_mgr *mgr; > > > struct sample_entry *entry; > > > }; > > > > > > \#define SAMPLE_ITER(name, __mgr) \ > > > struct sample_iter name = { \ > > > .mas = __MA_STATE(&(__mgr)->mt, 0, 0), > > > .mgr = __mgr, > > > .entry = NULL, > > > } > > > > I see this patch is to allow for anonymous maple states, this looks > > good. > > > > I've a lengthy comment about the iterator that I'm adding here to head > > off anyone that may copy your example below. > > > > > > > > \#define sample_iter_for_each_range(it__, start__, end__) \ > > > for ((it__).mas.index = start__, (it__).entry = mas_find(&(it__).mas, > > > end__ - 1); \ > > >(it__).entry; (it__).entry = mas_find(&(it__).mas, end__ - 1)) > > > > I see you've added something like the above in your patch set as well. > > I'd like to point out that the index isn't the only state information > > that needs to be altered here, and in fact, this could go very wrong. > > > > The maple state has a node and an offset within that node. If you set > > the index to lower than the current position of your iterator and call > > mas_find() then what happens is somewhat undefined. I expect you will > > get the wrong value (most likely either the current value or the very > > next one that the iterator is already pointing to). I believe you have > > been using a fresh maple state for each iterator in your patches, but I > > haven't had a deep look into your code yet. > > Yes, I'm aware that I'd need to reset the whole iterator in order to re-use > it. Okay, good. The way you have it written makes it unsafe to just call without knowledge of the state and that will probably end poorly over the long run. If it's always starting from MAS_START then it's probably safer to just initialize when you want to use it to the correct start address. > > Regarding the other considerations of the iterator design please see my > answer to Matthew. > > > > > We have methods of resetting the iterator and set the range (mas_set() > > and mas_set_range()) which are safe for what you are doing, but they > > will start the walk from the root node to the index again. > > > > So, if you know what you are doing is safe, then the way you have > > written it will work, but it's worth mentioning that this could occur. > > > > It is also worth pointing out that it would be much safer to use a > > function to do the above so you get type safety.. and I was asked to add > > this to the VMA interface by Linus [1], which is on its way upstream [2]. > > > > 1. > > https://lore.kernel.org/linux-mm/CAHk-=wg9wqxbgkndkd2bqocnn73rdswuwsavbb7t-tekyke...@mail.gmail.com/ > > 2. > > https://lore.kernel.org/linux-mm/20230120162650.984577-1-liam.howl...@oracle.com/ > > You mean having wrappers like sample_find() instead of directly using > mas_find()? I'm not sure you need to go that low level, but I would ensure I have a store/load function that ensures the correct type being put in/read from are correct on compile - especially since you seem to have two trees to track two different sets of things. That iterator is probably safe since the type is defined within itself. > > > > > > > > > Signed-off-by: Danilo Krummrich > > > --- > > > include/linux/maple_tree.h | 7 +-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/maple_tree.h b/include/linux/maple_tree.h > > > index e594db58a0f1..ca04c900e51a 100644 > > > --- a/include/linux/maple_tree.h > > > +++ b/include/linux/maple_tree.h > > > @@ -424,8 +424,8 @@ struct ma_wr_state { > > > #define MA_ERROR(err) \ > > > ((struct maple_enode *)(((unsigned long)err << 2) | > > > 2UL)) > > > -#define MA_STATE(name, mt, first, end) > > > \ > > > - struct ma_state name = {\ > > > +#define __MA_STATE(mt, first, end) > > > \ > > > + { \ > > > .tree = mt, > > > \ > > > .index = first, > > > \ > > > .last = end, > > > \ > > > @@ -435,6 +435,9 @@ struct ma_wr_state { > > > .alloc = NULL, > > > \ > > > } > > > +#define MA_STATE(name, mt, first, end) > > > \ > > > + struct ma_s
Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On 2023-02-20 11:14, Rob Clark wrote: > On Mon, Feb 20, 2023 at 12:53 AM Pekka Paalanen wrote: >> >> On Sat, 18 Feb 2023 13:15:49 -0800 >> Rob Clark wrote: >> >>> From: Rob Clark >>> >>> Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent >>> wait (as opposed to a "housekeeping" wait to know when to cleanup after >>> some work has completed). Usermode components of GPU driver stacks >>> often poll() on fence fd's to know when it is safe to do things like >>> free or reuse a buffer, but they can also poll() on a fence fd when >>> waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag >>> lets the kernel differentiate these two cases. >>> >>> Signed-off-by: Rob Clark >> >> Hi, >> >> where would the UAPI documentation of this go? >> It seems to be missing. > > Good question, I am not sure. The poll() man page has a description, > but my usage doesn't fit that _exactly_ (but OTOH the description is a > bit vague). > >> If a Wayland compositor is polling application fences to know which >> client buffer to use in its rendering, should the compositor poll with >> PRI or not? If a compositor polls with PRI, then all fences from all >> applications would always be PRI. Would that be harmful somehow or >> would it be beneficial? > > I think a compositor would rather use the deadline ioctl and then poll > without PRI. Otherwise you are giving an urgency signal to the fence > signaller which might not necessarily be needed. > > The places where I expect PRI to be useful is more in mesa (things > like glFinish(), readpix, and other similar sorts of blocking APIs) Hi, Hmm, but then user-space could do the opposite, namely, submit work as usual--never using the SET_DEADLINE ioctl, and then at the end, poll using (E)POLLPRI. That seems like a possible usage pattern, unintended--maybe, but possible. Do we want to discourage this? Wouldn't SET_DEADLINE be enough? I mean, one can call SET_DEADLINE with the current time, and then wouldn't that be equivalent to (E)POLLPRI? -- Regards, Luben
Re: [PATCH 0/4] drm/displayid: use primary use case to figure out non-desktop
On 2/20/23 18:44, Dmitry Osipenko wrote: > On 2/16/23 23:44, Jani Nikula wrote: >> Mostly this is prep work and plumbing for easier use of displayid >> structure version and primary use case for parsing the displayid blocks, >> but it can be nicely used for figuring out non-desktop too. >> >> Completely untested. :) >> >> BR, >> Jani. >> >> Cc: Iaroslav Boliukin >> Cc: Dmitry Osipenko >> >> Jani Nikula (4): >> drm/displayid: add displayid_get_header() and check bounds better >> drm/displayid: return struct displayid_header from >> validate_displayid() >> drm/displayid: provide access to DisplayID version and primary use >> case >> drm/edid: update non-desktop use also from DisplayID >> >> drivers/gpu/drm/drm_displayid.c | 62 - >> drivers/gpu/drm/drm_edid.c | 25 + >> include/drm/drm_displayid.h | 12 ++- >> 3 files changed, 89 insertions(+), 10 deletions(-) >> > > It works now without the EDID quirk, thanks! > > Tested-by: Dmitry Osipenko > I'm going to apply this to misc-next later this week if there won't be any objections. -- Best regards, Dmitry
Re: [PATCH 5/4] drm/edid: parse Tiled Display Topology Data Block for DisplayID 2.0
On 2/17/23 13:46, Jani Nikula wrote: > Currently we only parse the Tiled Display Topology Data Block for > DisplayID structure version 1.2, but not 2.0. The contents seem to be > the same for both, so expand the parsing to structure version 2.0. > > Note that DisplayID spec version is not the same as DisplayID structure > version; DisplayID 1.3 uses structure version 1.2, and DisplayID 2.0-2.1 > use structure version 2.0. Lovely. > > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_edid.c | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 03ad53a1ba82..ebab862b8b1a 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -7267,6 +7267,15 @@ static void drm_parse_tiled_block(struct drm_connector > *connector, > } > } > > +static bool displayid_is_tiled_block(const struct displayid_iter *iter, > + const struct displayid_block *block) > +{ > + return (displayid_version(iter) == DISPLAY_ID_STRUCTURE_VER_12 && > + block->tag == DATA_BLOCK_TILED_DISPLAY) || > + (displayid_version(iter) == DISPLAY_ID_STRUCTURE_VER_20 && > + block->tag == DATA_BLOCK_2_TILED_DISPLAY_TOPOLOGY); > +} > + > static void _drm_update_tile_info(struct drm_connector *connector, > const struct drm_edid *drm_edid) > { > @@ -7277,7 +7286,7 @@ static void _drm_update_tile_info(struct drm_connector > *connector, > > displayid_iter_edid_begin(drm_edid, &iter); > displayid_iter_for_each(block, &iter) { > - if (block->tag == DATA_BLOCK_TILED_DISPLAY) > + if (displayid_is_tiled_block(&iter, block)) > drm_parse_tiled_block(connector, block); > } > displayid_iter_end(&iter); I don't have display setup to test this, but looks okay. Reviewed-by: Dmitry Osipenko -- Best regards, Dmitry
Re: [PATCH 4/4] drm/edid: update non-desktop use also from DisplayID
On 2/16/23 23:45, Jani Nikula wrote: > Use the DisplayID 2.0 primary use case information to deduce whether > this is a head-mounted display, and should not be used for desktop. > > Cc: Iaroslav Boliukin > Cc: Dmitry Osipenko > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_edid.c | 25 + > 1 file changed, 25 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 3d0a4da661bc..03ad53a1ba82 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -6433,6 +6433,29 @@ static void drm_reset_display_info(struct > drm_connector *connector) > info->quirks = 0; > } > > +static void update_displayid_info(struct drm_connector *connector, > + const struct drm_edid *drm_edid) > +{ > + struct drm_display_info *info = &connector->display_info; > + const struct displayid_block *block; > + struct displayid_iter iter; > + > + displayid_iter_edid_begin(drm_edid, &iter); > + displayid_iter_for_each(block, &iter) { > + if (displayid_version(&iter) == DISPLAY_ID_STRUCTURE_VER_20 && > + (displayid_primary_use(&iter) == > PRIMARY_USE_HEAD_MOUNTED_VR || > + displayid_primary_use(&iter) == > PRIMARY_USE_HEAD_MOUNTED_AR)) > + info->non_desktop = true; > + > + /* > + * We're only interested in the base section here, no need to > + * iterate further. > + */ > + break; > + } > + displayid_iter_end(&iter); > +} > + > static void update_display_info(struct drm_connector *connector, > const struct drm_edid *drm_edid) > { > @@ -6463,6 +6486,8 @@ static void update_display_info(struct drm_connector > *connector, > info->color_formats |= DRM_COLOR_FORMAT_RGB444; > drm_parse_cea_ext(connector, drm_edid); > > + update_displayid_info(connector, drm_edid); > + > /* >* Digital sink with "DFP 1.x compliant TMDS" according to EDID 1.3? >* Tested-by: Dmitry Osipenko # HTC VIVE Pro 2 Reviewed-by: Dmitry Osipenko -- Best regards, Dmitry
Re: [PATCH 3/4] drm/displayid: provide access to DisplayID version and primary use case
On 2/16/23 23:45, Jani Nikula wrote: > The DisplayID structure version and primary use case are stored in the > DisplayID Base Section. We should be checking them in a number of places > when parsing the DisplayID blocks. Currently, we completely ignore the > primary use case, and just look at the block tags without cross-checking > against structure version. > > Store the version and primary use case in the DisplayID iterator, and > provide accessors to them. In general, the information is needed when > iterating the blocks, and this is a convenient place to both store and > retrieve the information during parsing. > > Promote using accessors rather than users poking at the iterator > directly. > > Cc: Iaroslav Boliukin > Cc: Dmitry Osipenko > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_displayid.c | 30 ++ > include/drm/drm_displayid.h | 12 +++- > 2 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c > index 0de9b5530393..9edc111be7ee 100644 > --- a/drivers/gpu/drm/drm_displayid.c > +++ b/drivers/gpu/drm/drm_displayid.c > @@ -123,6 +123,9 @@ __displayid_iter_next(struct displayid_iter *iter) > } > > for (;;) { > + /* The first section we encounter is the base section */ > + bool base_section = !iter->section; > + > iter->section = drm_find_displayid_extension(iter->drm_edid, >&iter->length, >&iter->idx, > @@ -132,6 +135,18 @@ __displayid_iter_next(struct displayid_iter *iter) > return NULL; > } > > + /* Save the structure version and primary use case. */ > + if (base_section) { > + const struct displayid_header *base; > + > + base = displayid_get_header(iter->section, iter->length, > + iter->idx); > + if (!IS_ERR(base)) { > + iter->version = base->rev; > + iter->primary_use = base->prod_id; > + } > + } > + > iter->idx += sizeof(struct displayid_header); > > block = displayid_iter_block(iter); > @@ -144,3 +159,18 @@ void displayid_iter_end(struct displayid_iter *iter) > { > memset(iter, 0, sizeof(*iter)); > } > + > +/* DisplayID Structure Version/Revision from the Base Section. */ > +u8 displayid_version(const struct displayid_iter *iter) > +{ > + return iter->version; > +} > + > +/* > + * DisplayID Primary Use Case (2.0+) or Product Type Identifier (1.0-1.3) > from > + * the Base Section. > + */ > +u8 displayid_primary_use(const struct displayid_iter *iter) > +{ > + return iter->primary_use; > +} > diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h > index 49649eb8447e..566497eeb3b8 100644 > --- a/include/drm/drm_displayid.h > +++ b/include/drm/drm_displayid.h > @@ -139,7 +139,11 @@ struct displayid_vesa_vendor_specific_block { > u8 mso; > } __packed; > > -/* DisplayID iteration */ > +/* > + * DisplayID iteration. > + * > + * Do not access directly, this is private. > + */ > struct displayid_iter { > const struct drm_edid *drm_edid; > > @@ -147,6 +151,9 @@ struct displayid_iter { > int length; > int idx; > int ext_index; > + > + u8 version; > + u8 primary_use; > }; > > void displayid_iter_edid_begin(const struct drm_edid *drm_edid, > @@ -157,4 +164,7 @@ __displayid_iter_next(struct displayid_iter *iter); > while (((__block) = __displayid_iter_next(__iter))) > void displayid_iter_end(struct displayid_iter *iter); > > +u8 displayid_version(const struct displayid_iter *iter); > +u8 displayid_primary_use(const struct displayid_iter *iter); > + > #endif Tested-by: Dmitry Osipenko Reviewed-by: Dmitry Osipenko -- Best regards, Dmitry
Re: [PATCH 2/4] drm/displayid: return struct displayid_header from validate_displayid()
On 2/16/23 23:44, Jani Nikula wrote: > Avoid figuring out the header pointer multiple times. > > Cc: Iaroslav Boliukin > Cc: Dmitry Osipenko > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_displayid.c | 17 - > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c > index 7d03159dc146..0de9b5530393 100644 > --- a/drivers/gpu/drm/drm_displayid.c > +++ b/drivers/gpu/drm/drm_displayid.c > @@ -20,7 +20,8 @@ displayid_get_header(const u8 *displayid, int length, int > index) > return base; > } > > -static int validate_displayid(const u8 *displayid, int length, int idx) > +static const struct displayid_header * > +validate_displayid(const u8 *displayid, int length, int idx) > { > int i, dispid_length; > u8 csum = 0; > @@ -28,7 +29,7 @@ static int validate_displayid(const u8 *displayid, int > length, int idx) > > base = displayid_get_header(displayid, length, idx); > if (IS_ERR(base)) > - return PTR_ERR(base); > + return base; > > DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n", > base->rev, base->bytes, base->prod_id, base->ext_count); > @@ -36,16 +37,16 @@ static int validate_displayid(const u8 *displayid, int > length, int idx) > /* +1 for DispID checksum */ > dispid_length = sizeof(*base) + base->bytes + 1; > if (dispid_length > length - idx) > - return -EINVAL; > + return ERR_PTR(-EINVAL); > > for (i = 0; i < dispid_length; i++) > csum += displayid[idx + i]; > if (csum) { > DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum); > - return -EINVAL; > + return ERR_PTR(-EINVAL); > } > > - return 0; > + return base; > } > > static const u8 *drm_find_displayid_extension(const struct drm_edid > *drm_edid, > @@ -54,7 +55,6 @@ static const u8 *drm_find_displayid_extension(const struct > drm_edid *drm_edid, > { > const u8 *displayid = drm_find_edid_extension(drm_edid, DISPLAYID_EXT, > ext_index); > const struct displayid_header *base; > - int ret; > > if (!displayid) > return NULL; > @@ -63,11 +63,10 @@ static const u8 *drm_find_displayid_extension(const > struct drm_edid *drm_edid, > *length = EDID_LENGTH - 1; > *idx = 1; > > - ret = validate_displayid(displayid, *length, *idx); > - if (ret) > + base = validate_displayid(displayid, *length, *idx); > + if (IS_ERR(base)) > return NULL; > > - base = (const struct displayid_header *)&displayid[*idx]; > *length = *idx + sizeof(*base) + base->bytes; > > return displayid; Tested-by: Dmitry Osipenko Reviewed-by: Dmitry Osipenko -- Best regards, Dmitry
Re: [PATCH 1/4] drm/displayid: add displayid_get_header() and check bounds better
On 2/16/23 23:44, Jani Nikula wrote: > Add a helper to get a pointer to struct displayid_header. To be > pedantic, add buffer overflow checks to not touch the base if that > itself would overflow. > > Cc: Iaroslav Boliukin > Cc: Dmitry Osipenko > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/drm_displayid.c | 17 - > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_displayid.c b/drivers/gpu/drm/drm_displayid.c > index 38ea8203df45..7d03159dc146 100644 > --- a/drivers/gpu/drm/drm_displayid.c > +++ b/drivers/gpu/drm/drm_displayid.c > @@ -7,13 +7,28 @@ > #include > #include > > +static const struct displayid_header * > +displayid_get_header(const u8 *displayid, int length, int index) > +{ > + const struct displayid_header *base; > + > + if (sizeof(*base) > length - index) > + return ERR_PTR(-EINVAL); > + > + base = (const struct displayid_header *)&displayid[index]; > + > + return base; > +} > + > static int validate_displayid(const u8 *displayid, int length, int idx) > { > int i, dispid_length; > u8 csum = 0; > const struct displayid_header *base; > > - base = (const struct displayid_header *)&displayid[idx]; > + base = displayid_get_header(displayid, length, idx); > + if (IS_ERR(base)) > + return PTR_ERR(base); > > DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n", > base->rev, base->bytes, base->prod_id, base->ext_count); Tested-by: Dmitry Osipenko Reviewed-by: Dmitry Osipenko -- Best regards, Dmitry
Re: [PATCH 1/4] drm/gem-vram: handle NULL bo->resource in move callback
Am 21.02.23 um 17:26 schrieb Matthew Auld: On 21/02/2023 16:17, Christian König wrote: Am 21.02.23 um 17:13 schrieb Matthew Auld: On 10/02/2023 11:03, Christian König wrote: Am 08.02.23 um 15:53 schrieb Matthew Auld: The ttm BO now initially has NULL bo->resource, and leaves the driver the handle that. However it looks like we forgot to handle that for ttm_bo_move_memcpy() users, like with vram-gem, since it just silently returns zero. This seems to then trigger warnings like: WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?) Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, otherwise do the multi-hop sequence to ensure can safely call into ttm_bo_move_memcpy(), since it might also need to clear the memory. This should give the same behaviour as before. While we are here let's also treat calling ttm_bo_move_memcpy() with NULL bo->resource as programmer error, where expectation is that upper layers should now handle it. Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation") Signed-off-by: Matthew Auld Cc: Christian König Oh, I wasn't aware that this broke at so many places. Especially radeon was tested earlier in the development of the patch set. Thanks for looking into that, the radeon patch has my rb and the rest of the series is Acked-by: Christian König . Should we go ahead and land this? (minus patch 3 since that is already fixed by vmware folks). Yeah, sure go ahead. I assume this has to go via some drm-misc type tree, for which I don't currently have commit rights. Can you help with merging this? Sure, going to push the series tomorrow. Thanks, Christian. Thanks, Christian. Regards, Christian. --- drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++ drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index d40b3edb52d0..0bea3df2a16d 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -916,6 +916,17 @@ static int bo_driver_move(struct ttm_buffer_object *bo, { struct drm_gem_vram_object *gbo; + if (!bo->resource) { + if (new_mem->mem_type != TTM_PL_SYSTEM) { + hop->mem_type = TTM_PL_SYSTEM; + hop->flags = TTM_PL_FLAG_TEMPORARY; + return -EMULTIHOP; + } + + ttm_bo_move_null(bo, new_mem); + return 0; + } + gbo = drm_gem_vram_of_bo(bo); return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d9d2b0903b22..fd9fd3d15101 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, bool clear; int ret = 0; - if (!src_mem) - return 0; + if (WARN_ON(!src_mem)) + return -EINVAL; src_man = ttm_manager_type(bdev, src_mem->mem_type); if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
Re: [PATCH] drm/amdkfd: Fix an illegal memory access
On 2023-02-21 06:35, qu.hu...@linux.dev wrote: From: Qu Huang In the kfd_wait_on_events() function, the kfd_event_waiter structure is allocated by alloc_event_waiters(), but the event field of the waiter structure is not initialized; When copy_from_user() fails in the kfd_wait_on_events() function, it will enter exception handling to release the previously allocated memory of the waiter structure; Due to the event field of the waiters structure being accessed in the free_waiters() function, this results in illegal memory access and system crash, here is the crash log: localhost kernel: RIP: 0010:native_queued_spin_lock_slowpath+0x185/0x1e0 localhost kernel: RSP: 0018:aa53c362bd60 EFLAGS: 00010082 localhost kernel: RAX: ff3d3d6bff4007cb RBX: 0282 RCX: 002c localhost kernel: RDX: 9e855eeacb80 RSI: 279c RDI: e7088f6a21d0 localhost kernel: RBP: e7088f6a21d0 R08: 002c R09: aa53c362be64 localhost kernel: R10: aa53c362bbd8 R11: 0001 R12: 0002 localhost kernel: R13: 9e7ead15d600 R14: R15: 9e7ead15d698 localhost kernel: FS: 152a3d111700() GS:9e855ee8() knlGS: localhost kernel: CS: 0010 DS: ES: CR0: 80050033 localhost kernel: CR2: 15293810 CR3: 00044d7a4000 CR4: 003506e0 localhost kernel: Call Trace: localhost kernel: _raw_spin_lock_irqsave+0x30/0x40 localhost kernel: remove_wait_queue+0x12/0x50 localhost kernel: kfd_wait_on_events+0x1b6/0x490 [hydcu] localhost kernel: ? ftrace_graph_caller+0xa0/0xa0 localhost kernel: kfd_ioctl+0x38c/0x4a0 [hydcu] localhost kernel: ? kfd_ioctl_set_trap_handler+0x70/0x70 [hydcu] localhost kernel: ? kfd_ioctl_create_queue+0x5a0/0x5a0 [hydcu] localhost kernel: ? ftrace_graph_caller+0xa0/0xa0 localhost kernel: __x64_sys_ioctl+0x8e/0xd0 localhost kernel: ? syscall_trace_enter.isra.18+0x143/0x1b0 localhost kernel: do_syscall_64+0x33/0x80 localhost kernel: entry_SYSCALL_64_after_hwframe+0x44/0xa9 localhost kernel: RIP: 0033:0x152a4dff68d7 Signed-off-by: Qu Huang --- drivers/gpu/drm/amd/amdkfd/kfd_events.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_events.c b/drivers/gpu/drm/amd/amdkfd/kfd_events.c index 729d26d..e5faaad 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_events.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_events.c @@ -787,6 +787,7 @@ static struct kfd_event_waiter *alloc_event_waiters(uint32_t num_events) for (i = 0; (event_waiters) && (i < num_events) ; i++) { init_wait(&event_waiters[i].wait); event_waiters[i].activated = false; + event_waiters[i].event = NULL; Thank you for catching this. We're often lazy about initializing things to NULL or 0 because most of our data structures are allocated with kzalloc or similar. I'm not sure why we're not doing this here. If we allocated event_waiters with kcalloc, we could also remove the initialization of activated. I think that would be the cleaner and safer solution. Regards, Felix } return event_waiters; -- 1.8.3.1
Re: [PATCH 1/4] drm/gem-vram: handle NULL bo->resource in move callback
On 21/02/2023 16:17, Christian König wrote: Am 21.02.23 um 17:13 schrieb Matthew Auld: On 10/02/2023 11:03, Christian König wrote: Am 08.02.23 um 15:53 schrieb Matthew Auld: The ttm BO now initially has NULL bo->resource, and leaves the driver the handle that. However it looks like we forgot to handle that for ttm_bo_move_memcpy() users, like with vram-gem, since it just silently returns zero. This seems to then trigger warnings like: WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?) Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, otherwise do the multi-hop sequence to ensure can safely call into ttm_bo_move_memcpy(), since it might also need to clear the memory. This should give the same behaviour as before. While we are here let's also treat calling ttm_bo_move_memcpy() with NULL bo->resource as programmer error, where expectation is that upper layers should now handle it. Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation") Signed-off-by: Matthew Auld Cc: Christian König Oh, I wasn't aware that this broke at so many places. Especially radeon was tested earlier in the development of the patch set. Thanks for looking into that, the radeon patch has my rb and the rest of the series is Acked-by: Christian König . Should we go ahead and land this? (minus patch 3 since that is already fixed by vmware folks). Yeah, sure go ahead. I assume this has to go via some drm-misc type tree, for which I don't currently have commit rights. Can you help with merging this? Thanks, Christian. Regards, Christian. --- drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++ drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index d40b3edb52d0..0bea3df2a16d 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -916,6 +916,17 @@ static int bo_driver_move(struct ttm_buffer_object *bo, { struct drm_gem_vram_object *gbo; + if (!bo->resource) { + if (new_mem->mem_type != TTM_PL_SYSTEM) { + hop->mem_type = TTM_PL_SYSTEM; + hop->flags = TTM_PL_FLAG_TEMPORARY; + return -EMULTIHOP; + } + + ttm_bo_move_null(bo, new_mem); + return 0; + } + gbo = drm_gem_vram_of_bo(bo); return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d9d2b0903b22..fd9fd3d15101 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, bool clear; int ret = 0; - if (!src_mem) - return 0; + if (WARN_ON(!src_mem)) + return -EINVAL; src_man = ttm_manager_type(bdev, src_mem->mem_type); if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
Re: [PATCH 1/4] drm/gem-vram: handle NULL bo->resource in move callback
Am 21.02.23 um 17:13 schrieb Matthew Auld: On 10/02/2023 11:03, Christian König wrote: Am 08.02.23 um 15:53 schrieb Matthew Auld: The ttm BO now initially has NULL bo->resource, and leaves the driver the handle that. However it looks like we forgot to handle that for ttm_bo_move_memcpy() users, like with vram-gem, since it just silently returns zero. This seems to then trigger warnings like: WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?) Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, otherwise do the multi-hop sequence to ensure can safely call into ttm_bo_move_memcpy(), since it might also need to clear the memory. This should give the same behaviour as before. While we are here let's also treat calling ttm_bo_move_memcpy() with NULL bo->resource as programmer error, where expectation is that upper layers should now handle it. Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation") Signed-off-by: Matthew Auld Cc: Christian König Oh, I wasn't aware that this broke at so many places. Especially radeon was tested earlier in the development of the patch set. Thanks for looking into that, the radeon patch has my rb and the rest of the series is Acked-by: Christian König . Should we go ahead and land this? (minus patch 3 since that is already fixed by vmware folks). Yeah, sure go ahead. Thanks, Christian. Regards, Christian. --- drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++ drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index d40b3edb52d0..0bea3df2a16d 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -916,6 +916,17 @@ static int bo_driver_move(struct ttm_buffer_object *bo, { struct drm_gem_vram_object *gbo; + if (!bo->resource) { + if (new_mem->mem_type != TTM_PL_SYSTEM) { + hop->mem_type = TTM_PL_SYSTEM; + hop->flags = TTM_PL_FLAG_TEMPORARY; + return -EMULTIHOP; + } + + ttm_bo_move_null(bo, new_mem); + return 0; + } + gbo = drm_gem_vram_of_bo(bo); return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d9d2b0903b22..fd9fd3d15101 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, bool clear; int ret = 0; - if (!src_mem) - return 0; + if (WARN_ON(!src_mem)) + return -EINVAL; src_man = ttm_manager_type(bdev, src_mem->mem_type); if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
Re: [PATCH 1/4] drm/gem-vram: handle NULL bo->resource in move callback
On 10/02/2023 11:03, Christian König wrote: Am 08.02.23 um 15:53 schrieb Matthew Auld: The ttm BO now initially has NULL bo->resource, and leaves the driver the handle that. However it looks like we forgot to handle that for ttm_bo_move_memcpy() users, like with vram-gem, since it just silently returns zero. This seems to then trigger warnings like: WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_gem_vram_helper.c:255 drm_gem_vram_offset (??:?) Fix this by calling move_null() if the new resource is TTM_PL_SYSTEM, otherwise do the multi-hop sequence to ensure can safely call into ttm_bo_move_memcpy(), since it might also need to clear the memory. This should give the same behaviour as before. While we are here let's also treat calling ttm_bo_move_memcpy() with NULL bo->resource as programmer error, where expectation is that upper layers should now handle it. Fixes: 180253782038 ("drm/ttm: stop allocating dummy resources during BO creation") Signed-off-by: Matthew Auld Cc: Christian König Oh, I wasn't aware that this broke at so many places. Especially radeon was tested earlier in the development of the patch set. Thanks for looking into that, the radeon patch has my rb and the rest of the series is Acked-by: Christian König . Should we go ahead and land this? (minus patch 3 since that is already fixed by vmware folks). Regards, Christian. --- drivers/gpu/drm/drm_gem_vram_helper.c | 11 +++ drivers/gpu/drm/ttm/ttm_bo_util.c | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_vram_helper.c b/drivers/gpu/drm/drm_gem_vram_helper.c index d40b3edb52d0..0bea3df2a16d 100644 --- a/drivers/gpu/drm/drm_gem_vram_helper.c +++ b/drivers/gpu/drm/drm_gem_vram_helper.c @@ -916,6 +916,17 @@ static int bo_driver_move(struct ttm_buffer_object *bo, { struct drm_gem_vram_object *gbo; + if (!bo->resource) { + if (new_mem->mem_type != TTM_PL_SYSTEM) { + hop->mem_type = TTM_PL_SYSTEM; + hop->flags = TTM_PL_FLAG_TEMPORARY; + return -EMULTIHOP; + } + + ttm_bo_move_null(bo, new_mem); + return 0; + } + gbo = drm_gem_vram_of_bo(bo); return drm_gem_vram_bo_driver_move(gbo, evict, ctx, new_mem); diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index d9d2b0903b22..fd9fd3d15101 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -157,8 +157,8 @@ int ttm_bo_move_memcpy(struct ttm_buffer_object *bo, bool clear; int ret = 0; - if (!src_mem) - return 0; + if (WARN_ON(!src_mem)) + return -EINVAL; src_man = ttm_manager_type(bdev, src_mem->mem_type); if (ttm && ((ttm->page_flags & TTM_TT_FLAG_SWAPPED) ||
Re: [PATCH v4 06/14] dma-buf/sync_file: Support (E)POLLPRI
On Tue, Feb 21, 2023 at 9:38 AM Pekka Paalanen wrote: > > On Mon, 20 Feb 2023 08:14:47 -0800 > Rob Clark wrote: > > > On Mon, Feb 20, 2023 at 12:53 AM Pekka Paalanen wrote: > > > > > > On Sat, 18 Feb 2023 13:15:49 -0800 > > > Rob Clark wrote: > > > > > > > From: Rob Clark > > > > > > > > Allow userspace to use the EPOLLPRI/POLLPRI flag to indicate an urgent > > > > wait (as opposed to a "housekeeping" wait to know when to cleanup after > > > > some work has completed). Usermode components of GPU driver stacks > > > > often poll() on fence fd's to know when it is safe to do things like > > > > free or reuse a buffer, but they can also poll() on a fence fd when > > > > waiting to read back results from the GPU. The EPOLLPRI/POLLPRI flag > > > > lets the kernel differentiate these two cases. > > > > > > > > Signed-off-by: Rob Clark > > > > > > Hi, > > > > > > where would the UAPI documentation of this go? > > > It seems to be missing. > > > > Good question, I am not sure. The poll() man page has a description, > > but my usage doesn't fit that _exactly_ (but OTOH the description is a > > bit vague). > > > > > If a Wayland compositor is polling application fences to know which > > > client buffer to use in its rendering, should the compositor poll with > > > PRI or not? If a compositor polls with PRI, then all fences from all > > > applications would always be PRI. Would that be harmful somehow or > > > would it be beneficial? > > > > I think a compositor would rather use the deadline ioctl and then poll > > without PRI. Otherwise you are giving an urgency signal to the fence > > signaller which might not necessarily be needed. > > > > The places where I expect PRI to be useful is more in mesa (things > > like glFinish(), readpix, and other similar sorts of blocking APIs) > > Sounds good. Docs... ;-) > > Hmm, so a compositor should set the deadline when it processes the > wl_surface.commit, and not when it actually starts repainting, to give > time for the driver to react and the GPU to do some more work. The > deadline would be the time when the compositor starts its repaint, so > it knows if the buffer is ready or not. Technically we don't know when the commit is supposed to be shown. Just passing a deadline of the next possible deadline however is probably a good enough guess for this feature to be useful. One thing that neither API allows us to do is tell the kernel in advance when we're going to submit work and what the deadline for it is and unfortunately that work is the most timing sensitive. > > > Thanks, > pq > > > > > > BR, > > -R > > > > > > > > > > > Thanks, > > > pq > > > > > > > --- > > > > drivers/dma-buf/sync_file.c | 8 > > > > 1 file changed, 8 insertions(+) > > > > > > > > diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c > > > > index fb6ca1032885..c30b2085ee0a 100644 > > > > --- a/drivers/dma-buf/sync_file.c > > > > +++ b/drivers/dma-buf/sync_file.c > > > > @@ -192,6 +192,14 @@ static __poll_t sync_file_poll(struct file *file, > > > > poll_table *wait) > > > > { > > > > struct sync_file *sync_file = file->private_data; > > > > > > > > + /* > > > > + * The POLLPRI/EPOLLPRI flag can be used to signal that > > > > + * userspace wants the fence to signal ASAP, express this > > > > + * as an immediate deadline. > > > > + */ > > > > + if (poll_requested_events(wait) & EPOLLPRI) > > > > + dma_fence_set_deadline(sync_file->fence, ktime_get()); > > > > + > > > > poll_wait(file, &sync_file->wq, wait); > > > > > > > > if (list_empty(&sync_file->cb.node) && > > > >
[PATCH 3/4] drm/ast: Rename struct ast_private to struct ast_device
The data structure struct ast_private represents an AST device. Its name comes from the time when it was allocated and stored separately in struct drm_device.dev_private. The DRM device is now embedded, so rename struct ast_private to struct ast_device. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_dp.c| 10 ++-- drivers/gpu/drm/ast/ast_dp501.c | 40 +++--- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.h | 40 +++--- drivers/gpu/drm/ast/ast_i2c.c | 8 +-- drivers/gpu/drm/ast/ast_main.c | 24 - drivers/gpu/drm/ast/ast_mm.c| 4 +- drivers/gpu/drm/ast/ast_mode.c | 78 +-- drivers/gpu/drm/ast/ast_post.c | 94 - 9 files changed, 150 insertions(+), 150 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c index 56483860306b..9e34297d836d 100644 --- a/drivers/gpu/drm/ast/ast_dp.c +++ b/drivers/gpu/drm/ast/ast_dp.c @@ -9,7 +9,7 @@ int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata) { - struct ast_private *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_private(dev); u8 i = 0, j = 0; /* @@ -125,7 +125,7 @@ void ast_dp_launch(struct drm_device *dev, u8 bPower) u8 bDPTX = 0; u8 bDPExecute = 1; - struct ast_private *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_private(dev); // S3 come back, need more time to wait BMC ready. if (bPower) WaitCount = 300; @@ -172,7 +172,7 @@ void ast_dp_launch(struct drm_device *dev, u8 bPower) void ast_dp_power_on_off(struct drm_device *dev, bool on) { - struct ast_private *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_private(dev); // Read and Turn off DP PHY sleep u8 bE3 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xE3, AST_DP_VIDEO_ENABLE); @@ -188,7 +188,7 @@ void ast_dp_power_on_off(struct drm_device *dev, bool on) void ast_dp_set_on_off(struct drm_device *dev, bool on) { - struct ast_private *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_private(dev); u8 video_on_off = on; // Video On/Off @@ -208,7 +208,7 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on) void ast_dp_set_mode(struct drm_crtc *crtc, struct ast_vbios_mode_info *vbios_mode) { - struct ast_private *ast = to_ast_private(crtc->dev); + struct ast_device *ast = to_ast_private(crtc->dev); u32 ulRefreshRateIndex; u8 ModeIdx; diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index 4f75a9efb610..bb56a1c73968 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -10,7 +10,7 @@ MODULE_FIRMWARE("ast_dp501_fw.bin"); static void ast_release_firmware(void *data) { - struct ast_private *ast = data; + struct ast_device *ast = data; release_firmware(ast->dp501_fw); ast->dp501_fw = NULL; @@ -18,7 +18,7 @@ static void ast_release_firmware(void *data) static int ast_load_dp501_microcode(struct drm_device *dev) { - struct ast_private *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_private(dev); int ret; ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); @@ -28,7 +28,7 @@ static int ast_load_dp501_microcode(struct drm_device *dev) return devm_add_action_or_reset(dev->dev, ast_release_firmware, ast); } -static void send_ack(struct ast_private *ast) +static void send_ack(struct ast_device *ast) { u8 sendack; sendack = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0x9b, 0xff); @@ -36,7 +36,7 @@ static void send_ack(struct ast_private *ast) ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0x9b, 0x00, sendack); } -static void send_nack(struct ast_private *ast) +static void send_nack(struct ast_device *ast) { u8 sendack; sendack = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0x9b, 0xff); @@ -44,7 +44,7 @@ static void send_nack(struct ast_private *ast) ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0x9b, 0x00, sendack); } -static bool wait_ack(struct ast_private *ast) +static bool wait_ack(struct ast_device *ast) { u8 waitack; u32 retry = 0; @@ -60,7 +60,7 @@ static bool wait_ack(struct ast_private *ast) return false; } -static bool wait_nack(struct ast_private *ast) +static bool wait_nack(struct ast_device *ast) { u8 waitack; u32 retry = 0; @@ -76,18 +76,18 @@ static bool wait_nack(struct ast_private *ast) return false; } -static void set_cmd_trigger(struct ast_private *ast) +static void set_cmd_trigger(struct ast_device *ast) { ast_set_index_reg_mask(ast, AST_IO_CRTC_PORT, 0x9b, ~0x40, 0x40); } -static void clear_cmd_trigger(struct ast_private *ast) +static void clear_cmd_trigger(struct ast_device
[PATCH 0/4] drm/ast: Use struct ast_device
Rename struct ast_private to struct ast_device. The old name comes from the time when struct drm_device.dev_private was still in use. Also update the upcast helper. The renaming touches ast's I/O-helper macros, which generate warnings with checkpatch. So fix the I/O helpers in the first two patches of this patchset. Tested on AST2100 hardware. Thomas Zimmermann (4): drm/ast: Remove little-endianism from I/O helpers drm/ast: Rework definition of I/O read and write helpers drm/ast: Rename struct ast_private to struct ast_device drm/ast: Rename to_ast_private() to to_ast_device() drivers/gpu/drm/ast/ast_dp.c| 10 ++-- drivers/gpu/drm/ast/ast_dp501.c | 40 +++--- drivers/gpu/drm/ast/ast_drv.c | 2 +- drivers/gpu/drm/ast/ast_drv.h | 84 - drivers/gpu/drm/ast/ast_i2c.c | 8 +-- drivers/gpu/drm/ast/ast_main.c | 24 - drivers/gpu/drm/ast/ast_mm.c| 4 +- drivers/gpu/drm/ast/ast_mode.c | 78 +-- drivers/gpu/drm/ast/ast_post.c | 94 - 9 files changed, 163 insertions(+), 181 deletions(-) -- 2.39.2
[PATCH 1/4] drm/ast: Remove little-endianism from I/O helpers
Replace one call to ast_io_write16() with two calls to ast_io_write8() in ast_set_index_reg(). The combined 16-bit-wide write of an index register and the corresponding data register only works on little- endian systems. Write both registers independent from each other. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_drv.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index d51b81fea9c8..1f46162adb64 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -282,7 +282,9 @@ static inline void ast_set_index_reg(struct ast_private *ast, uint32_t base, uint8_t index, uint8_t val) { - ast_io_write16(ast, base, ((u16)val << 8) | index); + ast_io_write8(ast, base, index); + ++base; + ast_io_write8(ast, base, val); } void ast_set_index_reg_mask(struct ast_private *ast, -- 2.39.2
[PATCH 4/4] drm/ast: Rename to_ast_private() to to_ast_device()
The helper to_ast_private() now upcasts to struct ast_device. Rename it accordingly. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_dp.c| 10 +- drivers/gpu/drm/ast/ast_dp501.c | 20 ++-- drivers/gpu/drm/ast/ast_drv.h | 2 +- drivers/gpu/drm/ast/ast_i2c.c | 8 drivers/gpu/drm/ast/ast_main.c | 6 +++--- drivers/gpu/drm/ast/ast_mode.c | 30 +++--- drivers/gpu/drm/ast/ast_post.c | 16 7 files changed, 46 insertions(+), 46 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_dp.c b/drivers/gpu/drm/ast/ast_dp.c index 9e34297d836d..fbb070f63e36 100644 --- a/drivers/gpu/drm/ast/ast_dp.c +++ b/drivers/gpu/drm/ast/ast_dp.c @@ -9,7 +9,7 @@ int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); u8 i = 0, j = 0; /* @@ -125,7 +125,7 @@ void ast_dp_launch(struct drm_device *dev, u8 bPower) u8 bDPTX = 0; u8 bDPExecute = 1; - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); // S3 come back, need more time to wait BMC ready. if (bPower) WaitCount = 300; @@ -172,7 +172,7 @@ void ast_dp_launch(struct drm_device *dev, u8 bPower) void ast_dp_power_on_off(struct drm_device *dev, bool on) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); // Read and Turn off DP PHY sleep u8 bE3 = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xE3, AST_DP_VIDEO_ENABLE); @@ -188,7 +188,7 @@ void ast_dp_power_on_off(struct drm_device *dev, bool on) void ast_dp_set_on_off(struct drm_device *dev, bool on) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); u8 video_on_off = on; // Video On/Off @@ -208,7 +208,7 @@ void ast_dp_set_on_off(struct drm_device *dev, bool on) void ast_dp_set_mode(struct drm_crtc *crtc, struct ast_vbios_mode_info *vbios_mode) { - struct ast_device *ast = to_ast_private(crtc->dev); + struct ast_device *ast = to_ast_device(crtc->dev); u32 ulRefreshRateIndex; u8 ModeIdx; diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c index bb56a1c73968..1bc35a992369 100644 --- a/drivers/gpu/drm/ast/ast_dp501.c +++ b/drivers/gpu/drm/ast/ast_dp501.c @@ -18,7 +18,7 @@ static void ast_release_firmware(void *data) static int ast_load_dp501_microcode(struct drm_device *dev) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); int ret; ret = request_firmware(&ast->dp501_fw, "ast_dp501_fw.bin", dev->dev); @@ -106,7 +106,7 @@ static bool wait_fw_ready(struct ast_device *ast) static bool ast_write_cmd(struct drm_device *dev, u8 data) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); int retry = 0; if (wait_nack(ast)) { send_nack(ast); @@ -128,7 +128,7 @@ static bool ast_write_cmd(struct drm_device *dev, u8 data) static bool ast_write_data(struct drm_device *dev, u8 data) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); if (wait_nack(ast)) { send_nack(ast); @@ -146,7 +146,7 @@ static bool ast_write_data(struct drm_device *dev, u8 data) #if 0 static bool ast_read_data(struct drm_device *dev, u8 *data) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); u8 tmp; *data = 0; @@ -185,7 +185,7 @@ static u32 get_fw_base(struct ast_device *ast) bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); u32 i, data; u32 boot_address; @@ -204,7 +204,7 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size) static bool ast_launch_m68k(struct drm_device *dev) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); u32 i, data, len = 0; u32 boot_address; u8 *fw_addr = NULL; @@ -274,7 +274,7 @@ static bool ast_launch_m68k(struct drm_device *dev) bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev); u32 i, boot_address, offset, data; u32 *pEDIDidx; @@ -334,7 +334,7 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) static bool ast_init_dvo(struct drm_device *dev) { - struct ast_device *ast = to_ast_private(dev); + struct ast_device *ast = to_ast_device(dev);
[PATCH 2/4] drm/ast: Rework definition of I/O read and write helpers
Ast defines a number of I/O helpers for accessing hardware. Only 4 of the many generated functions are actually used. Replace the respective generator macros with those 4 functions. No functional changes. Signed-off-by: Thomas Zimmermann --- drivers/gpu/drm/ast/ast_drv.h | 48 ++- 1 file changed, 14 insertions(+), 34 deletions(-) diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h index 1f46162adb64..6e5ed5bafdc1 100644 --- a/drivers/gpu/drm/ast/ast_drv.h +++ b/drivers/gpu/drm/ast/ast_drv.h @@ -238,45 +238,25 @@ struct ast_private *ast_device_create(const struct drm_driver *drv, #define AST_IO_VGACRCB_HWC_ENABLED BIT(1) #define AST_IO_VGACRCB_HWC_16BPP BIT(0) /* set: ARGB, cleared: 2bpp palette */ -#define __ast_read(x) \ -static inline u##x ast_read##x(struct ast_private *ast, u32 reg) { \ -u##x val = 0;\ -val = ioread##x(ast->regs + reg); \ -return val;\ +static inline u32 ast_read32(struct ast_private *ast, u32 reg) +{ + return ioread32(ast->regs + reg); } -__ast_read(8); -__ast_read(16); -__ast_read(32) - -#define __ast_io_read(x) \ -static inline u##x ast_io_read##x(struct ast_private *ast, u32 reg) { \ -u##x val = 0;\ -val = ioread##x(ast->ioregs + reg); \ -return val;\ +static inline void ast_write32(struct ast_private *ast, u32 reg, u32 val) +{ + iowrite32(val, ast->regs + reg); } -__ast_io_read(8); -__ast_io_read(16); -__ast_io_read(32); - -#define __ast_write(x) \ -static inline void ast_write##x(struct ast_private *ast, u32 reg, u##x val) {\ - iowrite##x(val, ast->regs + reg);\ - } - -__ast_write(8); -__ast_write(16); -__ast_write(32); - -#define __ast_io_write(x) \ -static inline void ast_io_write##x(struct ast_private *ast, u32 reg, u##x val) {\ - iowrite##x(val, ast->ioregs + reg);\ - } +static inline u8 ast_io_read8(struct ast_private *ast, u32 reg) +{ + return ioread8(ast->ioregs + reg); +} -__ast_io_write(8); -__ast_io_write(16); -#undef __ast_io_write +static inline void ast_io_write8(struct ast_private *ast, u32 reg, u8 val) +{ + iowrite8(val, ast->ioregs + reg); +} static inline void ast_set_index_reg(struct ast_private *ast, uint32_t base, uint8_t index, -- 2.39.2
Re: [PATCH v3 1/5] dt-bindings: display: bridge: Add ddc-i2c-bus for anx7688
On Sat, Feb 18, 2023 at 07:17:08PM +0800, Pin-yen Lin wrote: > Introduce a optional "ddc-i2c-bus" property for anx7688 bridge. This > allows the bridge to register a .get_edid callback. What's .get_edid? This is a binding and is independent of Linux. > > Signed-off-by: Pin-yen Lin > --- > > Changes in v3: > - New in v3 > > .../bindings/display/bridge/google,cros-ec-anx7688.yaml | 5 + > 1 file changed, 5 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml > > b/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml > index a44d025d33bd..9d5ce8172e88 100644 > --- > a/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml > +++ > b/Documentation/devicetree/bindings/display/bridge/google,cros-ec-anx7688.yaml > @@ -25,6 +25,10 @@ properties: > maxItems: 1 > description: I2C address of the device. > > + ddc-i2c-bus: > +description: phandle link to the I2C controller used for DDC EDID probing > +$ref: /schemas/types.yaml#/definitions/phandle > + No, this belongs in the connector node. The DDC signals are routed to the connector, not the bridge chip. Rob
[PATCH v2 05/10] dt-bindings: gpu: mali-bifrost: Add new MT8183 compatible
Since new platform data was required in Panfrost for getting GPU DVFS finally working on MediaTek SoCs, add a new "mediatek,mt8183b-mali" compatible. Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Rob Herring --- .../bindings/gpu/arm,mali-bifrost.yaml| 19 +++ 1 file changed, 19 insertions(+) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 4d9ab4702582..be18b161959b 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -19,6 +19,7 @@ properties: - enum: - amlogic,meson-g12a-mali - mediatek,mt8183-mali + - mediatek,mt8183b-mali - realtek,rtd1619-mali - renesas,r9a07g044-mali - renesas,r9a07g054-mali @@ -157,6 +158,7 @@ allOf: contains: enum: - mediatek,mt8183-mali +- mediatek,mt8183b-mali - mediatek,mt8192-mali then: properties: @@ -185,6 +187,23 @@ allOf: else: properties: sram-supply: false + - if: + properties: +compatible: + contains: +const: mediatek,mt8183b-mali +then: + properties: +power-domains: + minItems: 3 +power-domain-names: + items: +- const: core0 +- const: core1 +- const: core2 + required: +- power-domains +- power-domain-names - if: properties: compatible: -- 2.39.2
[PATCH v2 07/10] drm/panfrost: Increase MAX_PM_DOMAINS to 5
From: Alyssa Rosenzweig Increase the MAX_PM_DOMAINS constant from 3 to 5, to support the extra power domains required by the Mali-G57 on the MT8192. Signed-off-by: Alyssa Rosenzweig Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Steven Price --- drivers/gpu/drm/panfrost/panfrost_device.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index d9ba68cffb77..b0126b9fbadc 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -23,7 +23,7 @@ struct panfrost_job; struct panfrost_perfcnt; #define NUM_JOB_SLOTS 3 -#define MAX_PM_DOMAINS 3 +#define MAX_PM_DOMAINS 5 struct panfrost_features { u16 id; -- 2.39.2
[PATCH v2 09/10] drm/panfrost: Add mediatek,mt8192-mali compatible
From: Alyssa Rosenzweig Required for Mali-G57 on the Mediatek MT8192 and MT8195, which uses even more power domains than the MT8183 before it. Signed-off-by: Alyssa Rosenzweig [Angelo: Removed unneeded "sram" supply, added mt8195 to commit description] Co-developed-by: AngeloGioacchino Del Regno Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Steven Price --- drivers/gpu/drm/panfrost/panfrost_drv.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index abb0dadd8f63..5d25e77e1037 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -663,6 +663,16 @@ static const struct panfrost_compatible mediatek_mt8183_data = { .pm_domain_names = mediatek_mt8183_pm_domains, }; +static const char * const mediatek_mt8192_supplies[] = { "mali", NULL }; +static const char * const mediatek_mt8192_pm_domains[] = { "core0", "core1", "core2", + "core3", "core4" }; +static const struct panfrost_compatible mediatek_mt8192_data = { + .num_supplies = ARRAY_SIZE(mediatek_mt8192_supplies) - 1, + .supply_names = mediatek_mt8192_supplies, + .num_pm_domains = ARRAY_SIZE(mediatek_mt8192_pm_domains), + .pm_domain_names = mediatek_mt8192_pm_domains, +}; + static const struct of_device_id dt_match[] = { /* Set first to probe before the generic compatibles */ { .compatible = "amlogic,meson-gxm-mali", @@ -681,6 +691,7 @@ static const struct of_device_id dt_match[] = { { .compatible = "arm,mali-bifrost", .data = &default_data, }, { .compatible = "arm,mali-valhall-jm", .data = &default_data, }, { .compatible = "mediatek,mt8183-mali", .data = &mediatek_mt8183_data }, + { .compatible = "mediatek,mt8192-mali", .data = &mediatek_mt8192_data }, {} }; MODULE_DEVICE_TABLE(of, dt_match); -- 2.39.2
[PATCH v2 06/10] dt-bindings: gpu: mali-bifrost: Add a compatible for MediaTek MT8186
Get GPU support on MT8186 by adding its compatible. Signed-off-by: AngeloGioacchino Del Regno --- Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index be18b161959b..43a841d4e94d 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -15,6 +15,11 @@ properties: compatible: oneOf: + - items: + - enum: + - mediatek,mt8186-mali + - const: mediatek,mt8183b-mali + - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable - items: - enum: - amlogic,meson-g12a-mali -- 2.39.2
[PATCH v2 08/10] drm/panfrost: Add the MT8192 GPU ID
From: Alyssa Rosenzweig MediaTek MT8192 has a Mali-G57 with a special GPU ID. Add its GPU ID, but treat it as otherwise identical to a standard Mali-G57. We do _not_ fix up the GPU ID here -- userspace needs to be aware of the special GPU ID, in case we find functional differences between MediaTek's implementation and the standard Mali-G57 down the line. Signed-off-by: Alyssa Rosenzweig Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Steven Price --- drivers/gpu/drm/panfrost/panfrost_gpu.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c index 6452e4e900dd..d28b99732dde 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -204,6 +204,14 @@ static const struct panfrost_model gpu_models[] = { GPU_MODEL(g57, 0x9001, GPU_REV(g57, 0, 0)), + + /* MediaTek MT8192 has a Mali-G57 with a different GPU ID from the +* standard. Arm's driver does not appear to handle this model. +* ChromeOS has a hack downstream for it. Treat it as equivalent to +* standard Mali-G57 for now. +*/ + GPU_MODEL(g57, 0x9003, + GPU_REV(g57, 0, 0)), }; static void panfrost_gpu_init_features(struct panfrost_device *pfdev) -- 2.39.2
[PATCH v2 10/10] drm/panfrost: Add new compatible for Mali on the MT8183 SoC
The "mediatek,mt8183-mali" compatible uses platform data that calls for getting (and managing) two regulators ("mali" and "sram") but devfreq does not support this usecase, resulting in DVFS not working. Since a lot of MediaTek SoCs need to set the voltages for the GPU SRAM regulator in a specific relation to the GPU VCORE regulator, a MediaTek SoC specific driver was introduced to automatically satisfy, through coupling, these constraints: this means that there is at all no need to manage both regulators in panfrost but to otherwise just manage the main "mali" (-> gpu vcore) regulator instead. Keeping in mind that we cannot break the ABI, the most sensible route (avoiding hacks and uselessly overcomplicated code) to get a MT8183 node with one power supply was to add a new "mediatek,mt8183b-mali" compatible, which effectively deprecates the former. Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Steven Price --- drivers/gpu/drm/panfrost/panfrost_drv.c | 17 + 1 file changed, 17 insertions(+) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 5d25e77e1037..14cdeaeeb5c4 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -654,6 +654,14 @@ static const struct panfrost_compatible amlogic_data = { .vendor_quirk = panfrost_gpu_amlogic_quirk, }; +/* + * The old data with two power supplies for MT8183 is here only to + * keep retro-compatibility with older devicetrees, as DVFS will + * not work with this one. + * + * On new devicetrees please use the _b variant with a single and + * coupled regulators instead. + */ static const char * const mediatek_mt8183_supplies[] = { "mali", "sram", NULL }; static const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1", "core2" }; static const struct panfrost_compatible mediatek_mt8183_data = { @@ -663,6 +671,14 @@ static const struct panfrost_compatible mediatek_mt8183_data = { .pm_domain_names = mediatek_mt8183_pm_domains, }; +static const char * const mediatek_mt8183_b_supplies[] = { "mali", NULL }; +static const struct panfrost_compatible mediatek_mt8183_b_data = { + .num_supplies = ARRAY_SIZE(mediatek_mt8183_b_supplies) - 1, + .supply_names = mediatek_mt8183_b_supplies, + .num_pm_domains = ARRAY_SIZE(mediatek_mt8183_pm_domains), + .pm_domain_names = mediatek_mt8183_pm_domains, +}; + static const char * const mediatek_mt8192_supplies[] = { "mali", NULL }; static const char * const mediatek_mt8192_pm_domains[] = { "core0", "core1", "core2", "core3", "core4" }; @@ -691,6 +707,7 @@ static const struct of_device_id dt_match[] = { { .compatible = "arm,mali-bifrost", .data = &default_data, }, { .compatible = "arm,mali-valhall-jm", .data = &default_data, }, { .compatible = "mediatek,mt8183-mali", .data = &mediatek_mt8183_data }, + { .compatible = "mediatek,mt8183b-mali", .data = &mediatek_mt8183_b_data }, { .compatible = "mediatek,mt8192-mali", .data = &mediatek_mt8192_data }, {} }; -- 2.39.2
[PATCH v2 04/10] dt-bindings: gpu: mali-bifrost: Add compatible for MT8195 SoC
The MediaTek MT8195 SoC has a Mali G57 MC5 (Valhall-JM) and has the same number of power domains and requirements as MT8192 in terms of bindings. Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Rob Herring --- Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 65fe139ceb83..4d9ab4702582 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -25,6 +25,11 @@ properties: - rockchip,px30-mali - rockchip,rk3568-mali - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable + - items: + - enum: + - mediatek,mt8195-mali + - const: mediatek,mt8192-mali + - const: arm,mali-valhall-jm # Mali Valhall GPU model/revision is fully discoverable - items: - enum: - mediatek,mt8192-mali -- 2.39.2
[PATCH v2 01/10] dt-bindings: gpu: mali-bifrost: Add power-domain-names to base schema
From: Chen-Yu Tsai In commit a7a596cd3115 ("dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183"), "power-domain-names" was added to the mt8183-mali sub-schema, but was not added to the base mali-bifrost schema. Because validation happens for the base schema and any sub-schemas separately, this causes errors to be emitted when validating the MT8183 device trees. Add power-domain-names to the base schema to silence the error. Fixes: a7a596cd3115 ("dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183") Signed-off-by: Chen-Yu Tsai Signed-off-by: AngeloGioacchino Del Regno --- Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 78964c140b46..02699d389be1 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -65,6 +65,8 @@ properties: minItems: 1 maxItems: 3 + power-domain-names: true + resets: minItems: 1 maxItems: 3 @@ -166,6 +168,7 @@ allOf: properties: power-domains: maxItems: 1 +power-domain-names: false sram-supply: false - if: properties: -- 2.39.2
[PATCH v2 03/10] dt-bindings: gpu: mali-bifrost: Allow up to 5 power domains for MT8192
MediaTek MT8192 (and similar) needs five power domains for the Mali GPU and no sram-supply: change the binding to allow so. Fixes: 5d82e74a97c2 ("dt-bindings: Add compatible for Mali Valhall (JM)") Signed-off-by: AngeloGioacchino Del Regno Reviewed-by: Rob Herring --- .../bindings/gpu/arm,mali-bifrost.yaml| 22 ++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index ac174c17e25f..65fe139ceb83 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -63,7 +63,7 @@ properties: power-domains: minItems: 1 -maxItems: 3 +maxItems: 5 power-domain-names: true @@ -152,6 +152,7 @@ allOf: contains: enum: - mediatek,mt8183-mali +- mediatek,mt8192-mali then: properties: power-domains: @@ -179,6 +180,25 @@ allOf: else: properties: sram-supply: false + - if: + properties: +compatible: + contains: +const: mediatek,mt8192-mali +then: + properties: +power-domains: + minItems: 5 +power-domain-names: + items: +- const: core0 +- const: core1 +- const: core2 +- const: core3 +- const: core4 + required: +- power-domains +- power-domain-names - if: properties: compatible: -- 2.39.2
[PATCH v2 02/10] dt-bindings: gpu: mali-bifrost: Split out MediaTek power-domains variation
In preparation for adding new bindings for new MediaTek SoCs, split out the power-domain-names and power-domainsvariation 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 | 15 --- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 02699d389be1..ac174c17e25f 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -145,6 +145,18 @@ allOf: - power-domains - resets - reset-names + - if: + not: +properties: + compatible: +contains: + enum: +- mediatek,mt8183-mali +then: + properties: +power-domains: + maxItems: 1 +power-domain-names: false - if: properties: compatible: @@ -166,9 +178,6 @@ allOf: - power-domain-names else: properties: -power-domains: - maxItems: 1 -power-domain-names: false sram-supply: false - if: properties: -- 2.39.2
[PATCH v2 00/10] Panfrost: Improve and add MediaTek SoCs support
Changes in v2: - Add power-domain-names commit from Chen-Yu to the series - Kept sram-supply in base schema, overridden for non-MediaTek - Added Reviewed-by tags from Steven Price to the driver commits (as released in reply to v1's cover letter - thanks!) This series adds support for new MediaTek SoCs (MT8186/MT8192/MT8195) and improves MT8183 support: since the mtk-regulator-coupler driver was picked, it is now useless for Panfrost to look for, and manage, two regulators (GPU Vcore and GPU SRAM) on MediaTek; The aforementioned driver will take care of keeping the voltage relation (/constraints) of the two regulators on its own when a voltage change request is sent to the Vcore, solving the old time issue with not working DVFS on Panfrost+MediaTek (due to devfreq supporting only single regulator). In the specific case of MT8183, in order to not break the ABI, it was necessary to add a new compatible for enabling DVFS. Alyssa Rosenzweig (3): drm/panfrost: Increase MAX_PM_DOMAINS to 5 drm/panfrost: Add the MT8192 GPU ID drm/panfrost: Add mediatek,mt8192-mali compatible AngeloGioacchino Del Regno (6): dt-bindings: gpu: mali-bifrost: Split out MediaTek power-domains variation dt-bindings: gpu: mali-bifrost: Allow up to 5 power domains for MT8192 dt-bindings: gpu: mali-bifrost: Add compatible for MT8195 SoC dt-bindings: gpu: mali-bifrost: Add new MT8183 compatible dt-bindings: gpu: mali-bifrost: Add a compatible for MediaTek MT8186 drm/panfrost: Add new compatible for Mali on the MT8183 SoC Chen-Yu Tsai (1): dt-bindings: gpu: mali-bifrost: Add power-domain-names to base schema .../bindings/gpu/arm,mali-bifrost.yaml| 67 ++- drivers/gpu/drm/panfrost/panfrost_device.h| 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 28 drivers/gpu/drm/panfrost/panfrost_gpu.c | 8 +++ 4 files changed, 101 insertions(+), 4 deletions(-) -- 2.39.2
Re: [PATCH drm-next v2 01/16] drm: execution context for GEM buffers
On 2/17/23 17:00, Christian König wrote: Am 17.02.23 um 14:44 schrieb Danilo Krummrich: From: Christian König This adds the infrastructure for an execution context for GEM buffers which is similar to the existinc TTMs execbuf util and intended to replace it in the long term. The basic functionality is that we abstracts the necessary loop to lock many different GEM buffers with automated deadlock and duplicate handling. v2: drop xarray and use dynamic resized array instead, the locking overhead is unecessary and measureable. Question for Danilo and probably others: Does Nouveau make use of the duplicate tracking at some point? I'm not iterating duplicates or do something with them explicitly. However, I rely on drm_exec being able to deal with duplicates in general. Background is that I only have two or three use cases for this in radeon/amdgpu and would like to make it an optional feature. Going to take a look at the rest of this series next week. Regards, Christian. Signed-off-by: Christian König --- Documentation/gpu/drm-mm.rst | 12 ++ drivers/gpu/drm/Kconfig | 6 + drivers/gpu/drm/Makefile | 2 + drivers/gpu/drm/amd/amdgpu/Kconfig | 1 + drivers/gpu/drm/drm_exec.c | 295 + include/drm/drm_exec.h | 144 ++ 6 files changed, 460 insertions(+) create mode 100644 drivers/gpu/drm/drm_exec.c create mode 100644 include/drm/drm_exec.h diff --git a/Documentation/gpu/drm-mm.rst b/Documentation/gpu/drm-mm.rst index a79fd3549ff8..a52e6f4117d6 100644 --- a/Documentation/gpu/drm-mm.rst +++ b/Documentation/gpu/drm-mm.rst @@ -493,6 +493,18 @@ DRM Sync Objects .. kernel-doc:: drivers/gpu/drm/drm_syncobj.c :export: +DRM Execution context += + +.. kernel-doc:: drivers/gpu/drm/drm_exec.c + :doc: Overview + +.. kernel-doc:: include/drm/drm_exec.h + :internal: + +.. kernel-doc:: drivers/gpu/drm/drm_exec.c + :export: + GPU Scheduler = diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index f42d4c6a19f2..1573d658fbb5 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -200,6 +200,12 @@ config DRM_TTM GPU memory types. Will be enabled automatically if a device driver uses it. +config DRM_EXEC + tristate + depends on DRM + help + Execution context for command submissions + config DRM_BUDDY tristate depends on DRM diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index ab4460fcd63f..d40defbb0347 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -78,6 +78,8 @@ obj-$(CONFIG_DRM_PANEL_ORIENTATION_QUIRKS) += drm_panel_orientation_quirks.o # # Memory-management helpers # +# +obj-$(CONFIG_DRM_EXEC) += drm_exec.o obj-$(CONFIG_DRM_BUDDY) += drm_buddy.o diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig index 5341b6b242c3..279fb3bba810 100644 --- a/drivers/gpu/drm/amd/amdgpu/Kconfig +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig @@ -11,6 +11,7 @@ config DRM_AMDGPU select DRM_SCHED select DRM_TTM select DRM_TTM_HELPER + select DRM_EXEC select POWER_SUPPLY select HWMON select I2C diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c new file mode 100644 index ..ed2106c22786 --- /dev/null +++ b/drivers/gpu/drm/drm_exec.c @@ -0,0 +1,295 @@ +/* SPDX-License-Identifier: GPL-2.0 OR MIT */ + +#include +#include +#include + +/** + * DOC: Overview + * + * This component mainly abstracts the retry loop necessary for locking + * multiple GEM objects while preparing hardware operations (e.g. command + * submissions, page table updates etc..). + * + * If a contention is detected while locking a GEM object the cleanup procedure + * unlocks all previously locked GEM objects and locks the contended one first + * before locking any further objects. + * + * After an object is locked fences slots can optionally be reserved on the + * dma_resv object inside the GEM object. + * + * A typical usage pattern should look like this:: + * + * struct drm_gem_object *obj; + * struct drm_exec exec; + * unsigned long index; + * int ret; + * + * drm_exec_init(&exec, true); + * drm_exec_while_not_all_locked(&exec) { + * ret = drm_exec_prepare_obj(&exec, boA, 1); + * drm_exec_continue_on_contention(&exec); + * if (ret) + * goto error; + * + * ret = drm_exec_lock(&exec, boB, 1); + * drm_exec_continue_on_contention(&exec); + * if (ret) + * goto error; + * } + * + * drm_exec_for_each_locked_object(&exec, index, obj) { + * dma_resv_add_fence(obj->resv, fence, DMA_RESV_USAGE_READ); + * ... + * } + * drm_exec_fini(&exec); + * + * See struct dma_exec for more details. + */ + +/* Dummy value used to initially enter the retry loop */ +#define DRM_EXEC_DUMMY (void*)~
Re: [PATCH drm-next v2 04/16] maple_tree: add flag MT_FLAGS_LOCK_NONE
On Mon, Feb 20, 2023 at 08:33:35PM +, Matthew Wilcox wrote: > On Mon, Feb 20, 2023 at 06:06:03PM +0100, Danilo Krummrich wrote: > > On 2/20/23 16:10, Matthew Wilcox wrote: > > > This is why we like people to use the spinlock embedded in the tree. > > > There's nothing for the user to care about. If the access really is > > > serialised, acquiring/releasing the uncontended spinlock is a minimal > > > cost compared to all the other things that will happen while modifying > > > the tree. > > > > I think as for the users of the GPUVA manager we'd have two cases: > > > > 1) Accesses to the manager (and hence the tree) are serialized, no lock > > needed. > > > > 2) Multiple operations on the tree must be locked in order to make them > > appear atomic. > > Could you give an example here of what you'd like to do? Ideally > something complicated so I don't say "Oh, you can just do this" when > there's a more complex example for which "this" won't work. I'm sure > that's embedded somewhere in the next 20-odd patches, but it's probably > quicker for you to describe in terms of tree operations that have to > appear atomic than for me to try to figure it out. > Absolutely, not gonna ask you to read all of that. :-) One thing the GPUVA manager does is to provide drivers the (sub-)operations that need to be processed in order to fulfill a map or unmap request from userspace. For instance, when userspace asks the driver to map some memory the GPUVA manager calculates which existing mappings must be removed, split up or can be merged with the newly requested mapping. A driver has two ways to fetch those operations from the GPUVA manager. It can either obtain a list of operations or receive a callback for each operation generated by the GPUVA manager. In both cases the GPUVA manager walks the maple tree, which keeps track of existing mappings, for the given range in __drm_gpuva_sm_map() (only considering the map case, since the unmap case is a subset basically). For each mapping found in the given range the driver, as mentioned, either receives a callback or a list entry is added to the list of operations. Typically, for each operation / callback one entry within the maple tree is removed and, optionally at the beginning and end of a new mapping's range, a new entry is inserted. An of course, as the last operation, there is the new mapping itself to insert. The GPUVA manager delegates locking responsibility to the drivers. Typically, a driver either serializes access to the VA space managed by the GPUVA manager (no lock needed) or need to lock the processing of a full set of operations generated by the GPUVA manager. > > In either case the embedded spinlock wouldn't be useful, we'd either need an > > external lock or no lock at all. > > > > If there are any internal reasons why specific tree operations must be > > mutually excluded (such as those you explain below), wouldn't it make more > > sense to always have the internal lock and, optionally, allow users to > > specify an external lock additionally? > > So the way this works for the XArray, which is a little older than the > Maple tree, is that we always use the internal spinlock for > modifications (possibly BH or IRQ safe), and if someone wants to > use an external mutex to make some callers atomic with respect to each > other, they're free to do so. In that case, the XArray doesn't check > the user's external locking at all, because it really can't know. > > I'd advise taking that approach; if there's really no way to use the > internal spinlock to make your complicated updates appear atomic > then just let the maple tree use its internal spinlock, and you can > also use your external mutex however you like. > That sounds like the right thing to do. However, I'm using the advanced API of the maple tree (and that's the reason why the above example appears a little more detailed than needed) because I think with the normal API I can't insert / remove tree entries while walking the tree, right? As by the documentation the advanced API, however, doesn't take care of locking itself, hence just letting the maple tree use its internal spinlock doesn't really work - I need to take care of that myself, right? It feels a bit weird that I, as a user of the API, would need to lock certain (or all?) mas_*() functions with the internal spinlock in order to protect (future) internal features of the tree, such as the slab cache defragmentation you mentioned. Because from my perspective, as the generic component that tells it's users (the drivers) to take care of locking VA space operations (and hence tree operations) I don't have an own purpose of this internal spinlock, right? Also I'm a little confused how I'd know where to take the spinlock? E.g. for inserting entries in the tree I use mas_store_gfp() with GFP_KERNEL.
Re: [PATCH v2 1/9] drm/vc4: Switch to container_of_const
On 2/21/23 12:38, Maxime Ripard wrote: > Hi Hans, > > On Sat, Feb 18, 2023 at 11:45:04AM +0100, Hans Verkuil wrote: >>> diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c >>> index 86d629e45307..d0a00ed42cb0 100644 >>> --- a/drivers/gpu/drm/vc4/vc4_bo.c >>> +++ b/drivers/gpu/drm/vc4/vc4_bo.c >>> @@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object >>> *gem_bo) >>> static void vc4_bo_cache_time_work(struct work_struct *work) >>> { >>> struct vc4_dev *vc4 = >>> - container_of(work, struct vc4_dev, bo_cache.time_work); >>> + container_of_const(work, struct vc4_dev, bo_cache.time_work); >> >> ...I think this is misleading. It's definitely not const, so switching to >> container_of_const suggests that there is some 'constness' involved, which >> isn't the case. I'd leave this just as 'container_of'. This also reduces the >> size of the patch, since this is done in quite a few places. > > The name threw me off too, but it's supposed to keep the argument > pointer constness, not always take and return a const pointer. I still > believe that it's beneficial since, if the work pointer was ever to > change constness, we would have that additional check. If both inner (work) and outer (vc4) pointers are non-const, then there is no sense in switching to container_of_const. I don't see it used like that elsewhere in the kernel. It only makes sense to use it if the inner pointer might be const. If the work pointer (in this example) would ever become const, then the regular container_of macro would report a warning, that's something that was added in commit 7376e561fd2e. So preemptively switching to container_of_const appears unnecessary to me. Regards, Hans
[Bug 217065] Linux 6.2.0 - AMDGPU cannot start, ends in a blank screen.
https://bugzilla.kernel.org/show_bug.cgi?id=217065 Artem S. Tashkinov (a...@gmx.com) changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |ANSWERED --- Comment #1 from Artem S. Tashkinov (a...@gmx.com) --- Please report here instead: https://gitlab.freedesktop.org/drm/amd/-/issues -- 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 v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 03:11:33PM +0200, Pekka Paalanen wrote: > On Tue, 21 Feb 2023 15:01:35 +0200 > Ville Syrjälä wrote: > > > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > > On Mon, 20 Feb 2023 07:55:41 -0800 > > > Rob Clark wrote: > > > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > > wrote: > > > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > > Rob Clark wrote: > > > > > > > > > > > From: Rob Clark > > > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > > atomic update is waiting on. > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > --- > > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > > include/drm/drm_vblank.h | 1 + > > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > > drm_crtc *crtc, > > > > > > } > > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > > > +/** > > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next > > > > > > vblank > > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > > + * @vblanktime: pointer to time to receive the next vblank > > > > > > timestamp. > > > > > > + * > > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > > previous > > > > > > + * vblank and frame duration > > > > > > > > > > Hi, > > > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > > VRR mode, right? > > > > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > > > I don't know. :-) > > > > At least for i915 this will give you the maximum frame > > duration. > > Really maximum duration? So minimum VRR frequency? Yes. Doing otherwise would complicate the actual timestamp calculation even further. The actual timestamps i915 generates will however match the start of active video, regardless of how long vblank was extended. The only exception might be if you query the timestamp during vblank but VRR exit has not yet been triggered, ie. not commit has been made during the frame. In that case the timestamp will correspond to the max frame duration, which may or may not end up being the case. Depends totally whether a commit will still happen during the vblank to trigger an early vblank exit. > > > Also this does not calculate the the start of vblank, it > > calculates the start of active video. > > Oh indeed, so it's too late. What one would actually need for the > deadline is the driver's deadline to present for the immediately next > start of active video. > > And with VRR that should probably aim for the maximum frame frequency, > not minimum? Yeah, max frame rate seems like the easiest thing to use there. The other option might be some average value based on recent history, but figuring tht out would seem like a lot more work. -- Ville Syrjälä Intel
Re: [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, 21 Feb 2023 15:01:35 +0200 Ville Syrjälä wrote: > On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > > On Mon, 20 Feb 2023 07:55:41 -0800 > > Rob Clark wrote: > > > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen > > > wrote: > > > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > > Rob Clark wrote: > > > > > > > > > From: Rob Clark > > > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > > atomic update is waiting on. > > > > > > > > > > Signed-off-by: Rob Clark > > > > > --- > > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > > include/drm/drm_vblank.h | 1 + > > > > > 2 files changed, 33 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c > > > > > b/drivers/gpu/drm/drm_vblank.c > > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct > > > > > drm_crtc *crtc, > > > > > } > > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > > > +/** > > > > > + * drm_crtc_next_vblank_time - calculate the time of the next vblank > > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > > + * @vblanktime: pointer to time to receive the next vblank timestamp. > > > > > + * > > > > > + * Calculate the expected time of the next vblank based on time of > > > > > previous > > > > > + * vblank and frame duration > > > > > > > > Hi, > > > > > > > > for VRR this targets the highest frame rate possible for the current > > > > VRR mode, right? > > > > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > > > I don't know. :-) > > At least for i915 this will give you the maximum frame > duration. Really maximum duration? So minimum VRR frequency? > Also this does not calculate the the start of vblank, it > calculates the start of active video. Oh indeed, so it's too late. What one would actually need for the deadline is the driver's deadline to present for the immediately next start of active video. And with VRR that should probably aim for the maximum frame frequency, not minimum? Thanks, pq > > > > > You need a number of clock cycles in addition to the clock frequency, > > and that could still be minimum, maximum, the last realized one, ... > > > > VRR works by adjusting the front porch length IIRC. > > > > > > Thanks, > > pq > > > > > BR, > > > -R > > > > > > > > > > > > > > Thanks, > > > > pq > > > > > > > > > + */ > > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > > *vblanktime) > > > > > +{ > > > > > + unsigned int pipe = drm_crtc_index(crtc); > > > > > + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe]; > > > > > + u64 count; > > > > > + > > > > > + if (!vblank->framedur_ns) > > > > > + return -EINVAL; > > > > > + > > > > > + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime); > > > > > + > > > > > + /* > > > > > + * If we don't get a valid count, then we probably also don't > > > > > + * have a valid time: > > > > > + */ > > > > > + if (!count) > > > > > + return -EINVAL; > > > > > + > > > > > + *vblanktime = ktime_add(*vblanktime, > > > > > ns_to_ktime(vblank->framedur_ns)); > > > > > + > > > > > + return 0; > > > > > +} > > > > > +EXPORT_SYMBOL(drm_crtc_next_vblank_time); > > > > > + > > > > > static void send_vblank_event(struct drm_device *dev, > > > > > struct drm_pending_vblank_event *e, > > > > > u64 seq, ktime_t now) > > > > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h > > > > > index 733a3e2d1d10..a63bc2c92f3c 100644 > > > > > --- a/include/drm/drm_vblank.h > > > > > +++ b/include/drm/drm_vblank.h > > > > > @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device > > > > > *dev); > > > > > u64 drm_crtc_vblank_count(struct drm_crtc *crtc); > > > > > u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > > ktime_t *vblanktime); > > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > > *vblanktime); > > > > > void drm_crtc_send_vblank_event(struct drm_crtc *crtc, > > > > > struct drm_pending_vblank_event *e); > > > > > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc, > > > > > > > > > pgp0k2h598k2l.pgp Description: OpenPGP digital signature
Re: [PATCH v4 10/14] drm/vblank: Add helper to get next vblank time
On Tue, Feb 21, 2023 at 10:45:51AM +0200, Pekka Paalanen wrote: > On Mon, 20 Feb 2023 07:55:41 -0800 > Rob Clark wrote: > > > On Mon, Feb 20, 2023 at 1:08 AM Pekka Paalanen wrote: > > > > > > On Sat, 18 Feb 2023 13:15:53 -0800 > > > Rob Clark wrote: > > > > > > > From: Rob Clark > > > > > > > > Will be used in the next commit to set a deadline on fences that an > > > > atomic update is waiting on. > > > > > > > > Signed-off-by: Rob Clark > > > > --- > > > > drivers/gpu/drm/drm_vblank.c | 32 > > > > include/drm/drm_vblank.h | 1 + > > > > 2 files changed, 33 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c > > > > index 2ff31717a3de..caf25ebb34c5 100644 > > > > --- a/drivers/gpu/drm/drm_vblank.c > > > > +++ b/drivers/gpu/drm/drm_vblank.c > > > > @@ -980,6 +980,38 @@ u64 drm_crtc_vblank_count_and_time(struct drm_crtc > > > > *crtc, > > > > } > > > > EXPORT_SYMBOL(drm_crtc_vblank_count_and_time); > > > > > > > > +/** > > > > + * drm_crtc_next_vblank_time - calculate the time of the next vblank > > > > + * @crtc: the crtc for which to calculate next vblank time > > > > + * @vblanktime: pointer to time to receive the next vblank timestamp. > > > > + * > > > > + * Calculate the expected time of the next vblank based on time of > > > > previous > > > > + * vblank and frame duration > > > > > > Hi, > > > > > > for VRR this targets the highest frame rate possible for the current > > > VRR mode, right? > > > > > > > It is based on vblank->framedur_ns which is in turn based on > > mode->crtc_clock. Presumably for VRR that ends up being a maximum? > > I don't know. :-) At least for i915 this will give you the maximum frame duration. Also this does not calculate the the start of vblank, it calculates the start of active video. > > You need a number of clock cycles in addition to the clock frequency, > and that could still be minimum, maximum, the last realized one, ... > > VRR works by adjusting the front porch length IIRC. > > > Thanks, > pq > > > BR, > > -R > > > > > > > > > > Thanks, > > > pq > > > > > > > + */ > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > *vblanktime) > > > > +{ > > > > + unsigned int pipe = drm_crtc_index(crtc); > > > > + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[pipe]; > > > > + u64 count; > > > > + > > > > + if (!vblank->framedur_ns) > > > > + return -EINVAL; > > > > + > > > > + count = drm_vblank_count_and_time(crtc->dev, pipe, vblanktime); > > > > + > > > > + /* > > > > + * If we don't get a valid count, then we probably also don't > > > > + * have a valid time: > > > > + */ > > > > + if (!count) > > > > + return -EINVAL; > > > > + > > > > + *vblanktime = ktime_add(*vblanktime, > > > > ns_to_ktime(vblank->framedur_ns)); > > > > + > > > > + return 0; > > > > +} > > > > +EXPORT_SYMBOL(drm_crtc_next_vblank_time); > > > > + > > > > static void send_vblank_event(struct drm_device *dev, > > > > struct drm_pending_vblank_event *e, > > > > u64 seq, ktime_t now) > > > > diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h > > > > index 733a3e2d1d10..a63bc2c92f3c 100644 > > > > --- a/include/drm/drm_vblank.h > > > > +++ b/include/drm/drm_vblank.h > > > > @@ -230,6 +230,7 @@ bool drm_dev_has_vblank(const struct drm_device > > > > *dev); > > > > u64 drm_crtc_vblank_count(struct drm_crtc *crtc); > > > > u64 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc, > > > > ktime_t *vblanktime); > > > > +int drm_crtc_next_vblank_time(struct drm_crtc *crtc, ktime_t > > > > *vblanktime); > > > > void drm_crtc_send_vblank_event(struct drm_crtc *crtc, > > > > struct drm_pending_vblank_event *e); > > > > void drm_crtc_arm_vblank_event(struct drm_crtc *crtc, > > > > -- Ville Syrjälä Intel
a test email
this is a test email, Sorry to be a bother
[PULL] drm-misc-next-fixes
Hi Dave and Daniel, here's the final PR for drm-misc-next-fixes for this release cycle. Best regards Thomas drm-misc-next-fixes-2023-02-21: Short summary of fixes pull: Fixes GEM SHMEM locking and generic fbdev hotplugging. Constifies dma_buf kobj type. The following changes since commit 38b2d8efd03d2e56431b611e3523f0158306451d: drm: panel-orientation-quirks: Add quirk for Lenovo IdeaPad Duet 3 10IGL5 (2023-02-15 10:46:05 +0100) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-next-fixes-2023-02-21 for you to fetch changes up to 3fb1f62f80a1d249260db5ea9e22c51e52fab9ae: drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini() (2023-02-21 13:26:18 +0100) Short summary of fixes pull: Fixes GEM SHMEM locking and generic fbdev hotplugging. Constifies dma_buf kobj type. Asahi Lina (1): drm/shmem-helper: Fix locking for drm_gem_shmem_get_pages_sgt() Thomas Weißschuh (1): dma-buf: make kobj_type structure constant Thomas Zimmermann (1): drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini() drivers/dma-buf/dma-buf-sysfs-stats.c | 2 +- drivers/gpu/drm/armada/armada_fbdev.c | 3 ++ drivers/gpu/drm/drm_fb_helper.c| 2 -- drivers/gpu/drm/drm_fbdev_generic.c| 2 ++ drivers/gpu/drm/drm_gem_shmem_helper.c | 54 +++--- drivers/gpu/drm/exynos/exynos_drm_fbdev.c | 3 +- drivers/gpu/drm/gma500/framebuffer.c | 2 ++ drivers/gpu/drm/i915/display/intel_fbdev.c | 1 + drivers/gpu/drm/msm/msm_fbdev.c| 2 ++ drivers/gpu/drm/omapdrm/omap_fbdev.c | 2 ++ drivers/gpu/drm/radeon/radeon_fb.c | 2 ++ drivers/gpu/drm/tegra/fb.c | 1 + 12 files changed, 52 insertions(+), 24 deletions(-) -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()
Thomas Zimmermann writes: > Hi > > Am 21.02.23 um 11:27 schrieb Javier Martinez Canillas: >> Thomas Zimmermann writes: >> >>> Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the >>> calling fbdev implementation. Avoids a possible stale mutex with >>> generic fbdev code. >>> >>> As indicated by its name, drm_fb_helper_prepare() prepares struct >>> drm_fb_helper before setting up the fbdev support with a call to >>> drm_fb_helper_init(). In legacy fbdev emulation, this happens next >>> to each other. If successful, drm_fb_helper_fini() later tear down >>> the fbdev device and also unprepare via drm_fb_helper_unprepare(). >>> >>> Generic fbdev emulation prepares struct drm_fb_helper immediately >>> after allocating the instance. It only calls drm_fb_helper_init() >>> as part of processing a hotplug event. If the hotplug-handling fails, >>> it runs drm_fb_helper_fini(). This unprepares the fb-helper instance >>> and the next hotplug event runs on stale data. >>> >>> Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini() >>> into the fbdev implementations. Call it right before freeing the >>> fb-helper instance. >>> >>> Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()") >> >> I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize >> client unregistering") instead? Because commit 4825797c36da just added a >> wrapper function for mutex_destroy(&fb_helper->lock), but it was commit >> 032116bbe152 that made drm_fbdev_cleanup() to call that helper function. > > Good point. After looking through the recent fbdev commits, I'll use > commit 643231b28380 ("drm/fbdev-generic: Minimize hotplug error > handling") for the tag. This is the one that added the call to > drm_fb_helper_fini() to the client's hotplug handler. And _fini() > currently does the _unprepare(), when it shouldn't. > Ah, much better indeed. -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Re: [PATCH] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()
Hi Am 21.02.23 um 11:27 schrieb Javier Martinez Canillas: Thomas Zimmermann writes: Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the calling fbdev implementation. Avoids a possible stale mutex with generic fbdev code. As indicated by its name, drm_fb_helper_prepare() prepares struct drm_fb_helper before setting up the fbdev support with a call to drm_fb_helper_init(). In legacy fbdev emulation, this happens next to each other. If successful, drm_fb_helper_fini() later tear down the fbdev device and also unprepare via drm_fb_helper_unprepare(). Generic fbdev emulation prepares struct drm_fb_helper immediately after allocating the instance. It only calls drm_fb_helper_init() as part of processing a hotplug event. If the hotplug-handling fails, it runs drm_fb_helper_fini(). This unprepares the fb-helper instance and the next hotplug event runs on stale data. Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini() into the fbdev implementations. Call it right before freeing the fb-helper instance. Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()") I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize client unregistering") instead? Because commit 4825797c36da just added a wrapper function for mutex_destroy(&fb_helper->lock), but it was commit 032116bbe152 that made drm_fbdev_cleanup() to call that helper function. Good point. After looking through the recent fbdev commits, I'll use commit 643231b28380 ("drm/fbdev-generic: Minimize hotplug error handling") for the tag. This is the one that added the call to drm_fb_helper_fini() to the client's hotplug handler. And _fini() currently does the _unprepare(), when it shouldn't. Cc: Thomas Zimmermann Cc: Javier Martinez Canillas Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: David Airlie Cc: Daniel Vetter Cc: dri-devel@lists.freedesktop.org Signed-off-by: Thomas Zimmermann --- The change itself looks good to me. Reviewed-by: Javier Martinez Canillas Thanks a lot. Best regards Thomas -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v12 10/10] drm/bridge: it6505: Register Type C mode switches
On Tue, Feb 21, 2023 at 05:50:54PM +0800, Pin-yen Lin wrote: > Register USB Type-C mode switches when the "mode-switch" property and > relevant port are available in Device Tree. Configure the "lane_swap" > state based on the entered alternate mode for a specific Type-C > connector, which ends up updating the lane swap registers of the it6505 > chip. ... > +static void it6505_typec_ports_update(struct it6505 *it6505) > +{ > + int i; unsigned? (and just note that here you have already blank line which is good) > + /* Check if both ports available and do nothing to retain the current > one */ > + if (it6505->port_data[0].dp_connected && > it6505->port_data[1].dp_connected) > + return; > + > + for (i = 0; i < 2; i++) { > + if (it6505->port_data[i].dp_connected) > + it6505->lane_swap = it6505->port_data[i].lane_swap; > + } > +} ,,, > + it6505->port_data[port->port_num].dp_connected = > + state->alt && state->alt->svid == USB_TYPEC_DP_SID && > + state->alt->mode == USB_TYPEC_DP_MODE; Split first parameter? ... > + it6505->port_data = devm_kcalloc( > + dev, switch_desc->num_typec_switches, Strange indentation. > + sizeof(struct it6505_typec_port_data), GFP_KERNEL); > + Redundant blank line. > + if (!it6505->port_data) { > + ret = -ENOMEM; > + goto unregister_mux; > + } ... > + num_lanes = fwnode_property_count_u32(fwnode, "data-lanes"); > + > + if (num_lanes < 0) { > + dev_err(dev, > + "Error on getting data lanes count from %pfwP: > %d\n", > + fwnode, num_lanes); > + ret = num_lanes; > + goto unregister_mux; > + } Same two comments as per previous patch of similar semantics. -- With Best Regards, Andy Shevchenko
Re: [PATCH v12 05/10] drm/bridge: anx7625: Check for Type-C during panel registration
On Tue, Feb 21, 2023 at 05:50:49PM +0800, Pin-yen Lin wrote: > The output port endpoints can be connected to USB-C connectors. > Running drm_of_find_panel_or_bridge() with such endpoints leads to > a continuous return value of -EPROBE_DEFER, even though there is > no panel present. > > To avoid this, check for the existence of a "mode-switch" property in > the port endpoint, and skip panel registration completely if so. ... > + port_node = of_graph_get_port_by_id(np, 1); > + fwnode_for_each_typec_mode_switch(&port_node->fwnode, fwnode) { > + fwnode_handle_put(fwnode); > + return 0; > + } With the proposed count API: unsigned int count; ... port_node = ... count = typec_mode_switch_node_count(&port_node->fwnode); if (count == 0) return 0; -- With Best Regards, Andy Shevchenko
Re: [PATCH v12 03/10] drm/display: Add Type-C switch helpers
On Tue, Feb 21, 2023 at 05:50:47PM +0800, Pin-yen Lin wrote: > Add helpers to register and unregister Type-C "switches" for bridges > capable of switching their output between two downstream devices. > > The helper registers USB Type-C mode switches when the "mode-switch" > and the "reg" properties are available in Device Tree. > > Signed-off-by: Pin-yen Lin ... > + fwnode_for_each_typec_mode_switch(port, sw) > + switch_desc->num_typec_switches++; > + > + if (!switch_desc->num_typec_switches) { > + dev_dbg(dev, "No Type-C switches node found\n"); > + return 0; > + } What about static inline unsigned int typec_mode_switch_node_count(... *port) { ... *sw; unsigned int count = 0; for_each_typec_mode_switch_node(port, sw) count++; return count; } And then it seems something like unsigned int count; count = typec_mode_switch_node_count(port); if (!count) { ... } _switches = count; ... > + switch_desc->typec_ports = devm_kcalloc( > + dev, switch_desc->num_typec_switches, Strange indentation. > + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL); > + Redundant blank line. > + if (!switch_desc->typec_ports) > + return -ENOMEM; ... > +void drm_dp_unregister_typec_switches(struct drm_dp_typec_switch_desc > *switch_desc) > +{ > + int i; unsigned? > + for (i = 0; i < switch_desc->num_typec_switches; i++) > + typec_mux_unregister(switch_desc->typec_ports[i].typec_mux); > +} ... > #include > #include > +#include I don't see users of this. But a few forward declarations are missing. > #include > #include ... > +#define fwnode_for_each_typec_mode_switch(port, sw) \ > + fwnode_for_each_child_node((port), (sw))\ > + for_each_if(fwnode_property_present((sw), "mode-switch")) Please don't use fwnode namespace (see above), something like #define for_each_typec_mode_switch_node(port, sw) \ ... -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range
On 2/20/23 16:23, Dave Stevenson wrote: > Hi Hans > > On Sat, 18 Feb 2023 at 11:33, Hans Verkuil wrote: >> >> Hi Maxime, Dave, >> >> On 26/01/2023 14:46, Maxime Ripard wrote: >>> From: Dave Stevenson >>> >>> Copy Intel's "Broadcast RGB" property semantics to add manual override >>> of the HDMI pixel range for monitors that don't abide by the content >>> of the AVI Infoframe. >> >> Do we have to copy that property as-is? > > Firstly I'll agree with your later comment that having this control > allows testing of a range of output modes, and working around HDMI > sinks that have dodgy implementations. > (In our vendor kernel we now also have a property to override the > kernel chosen output format to enable testing of YCbCr4:4:4 and 4:2:2 > output). > > The DRM subsystem has the requirement for an open-source userspace > project to be using any new property before it will be merged [1]. > This property already exists for i915 and gma-500, therefore avoids > that requirement. > > There are objections to the UAPI for Broadcast RGB [2], but if it's > good enough for the existing implementations then there can be no > objection to it being implemented in the same way for other drivers. > Otherwise it is a missing feature of the DRM API, and the linked > discussion is realistically at least a year away from being resolved. > Why bury our heads in the sand for that period? > > [1] > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > [2] https://lists.freedesktop.org/archives/dri-devel/2023-February/391061.html > >> First of all, I think this should really be a drm-level property, rather than >> a driver property: RGB Quantization Range mismatches are the bane of my life, >> and I think a way to override this would help everyone. > > Linked to above, if it were the preferred method for controlling this > then I would expect it to become a drm-level property. > >> Secondly, I hate the name they came up with: 'Broadcast RGB' is pretty >> meaningless. >> Can't we stick to something closer to what the CTA-861/HDMI specs use, which >> is >> 'RGB Quantization Range'? So either use that, or just 'RGB Range'. >> >> In addition, 'Limited 16:235' should just be 'Limited' since the actual range >> depends on the bits-per-color-component. > > It's documented UAPI with those names[3], therefore any change would > be a change to user-space's expectation and a regression. At least by > sticking with the same names then any user space implementation that > exists for i915 or gma-500 will also work in the same way on vc4. > > [3] > https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#existing-kms-properties Thank you for linking to these threads. It's a terrible name, but it is still better to have this property than to not have it :-) > >>> >>> Signed-off-by: Dave Stevenson >>> Signed-off-by: Maxime Ripard >>> --- >>> drivers/gpu/drm/vc4/vc4_hdmi.c | 97 >>> -- >>> drivers/gpu/drm/vc4/vc4_hdmi.h | 9 >>> 2 files changed, 102 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c >>> index 4b3bf77bb5cd..78749c6fa837 100644 >>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c >>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c >>> @@ -150,10 +150,16 @@ static bool vc4_hdmi_mode_needs_scrambling(const >>> struct drm_display_mode *mode, >>> } >>> >>> static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi, >>> -const struct drm_display_mode *mode) >>> +struct vc4_hdmi_connector_state >>> *vc4_state) >>> { >>> + const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode; >>> struct drm_display_info *display = &vc4_hdmi->connector.display_info; >>> >>> + if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED) >>> + return false; >>> + else if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL) >>> + return true; >>> + >>> return !display->is_hdmi || >>> drm_default_rgb_quant_range(mode) == >>> HDMI_QUANTIZATION_RANGE_FULL; >>> } >>> @@ -524,8 +530,12 @@ static int vc4_hdmi_connector_atomic_check(struct >>> drm_connector *connector, >>> { >>> struct drm_connector_state *old_state = >>> drm_atomic_get_old_connector_state(state, connector); >>> + struct vc4_hdmi_connector_state *old_vc4_state = >>> + conn_state_to_vc4_hdmi_conn_state(old_state); >>> struct drm_connector_state *new_state = >>> drm_atomic_get_new_connector_state(state, connector); >>> + struct vc4_hdmi_connector_state *new_vc4_state = >>> + conn_state_to_vc4_hdmi_conn_state(new_state); >>> struct drm_crtc *crtc = new_state->crtc; >>> >>> if (!crtc) >>> @@ -558,6 +568,7 @@ static int vc4_hdmi_connector_atomic_check(struct >>> drm_connector *connector, >>> } >>> >>>
Re: [PATCH v2 1/9] drm/vc4: Switch to container_of_const
Hi Hans, On Sat, Feb 18, 2023 at 11:45:04AM +0100, Hans Verkuil wrote: > > diff --git a/drivers/gpu/drm/vc4/vc4_bo.c b/drivers/gpu/drm/vc4/vc4_bo.c > > index 86d629e45307..d0a00ed42cb0 100644 > > --- a/drivers/gpu/drm/vc4/vc4_bo.c > > +++ b/drivers/gpu/drm/vc4/vc4_bo.c > > @@ -609,7 +609,7 @@ static void vc4_free_object(struct drm_gem_object > > *gem_bo) > > static void vc4_bo_cache_time_work(struct work_struct *work) > > { > > struct vc4_dev *vc4 = > > - container_of(work, struct vc4_dev, bo_cache.time_work); > > + container_of_const(work, struct vc4_dev, bo_cache.time_work); > > ...I think this is misleading. It's definitely not const, so switching to > container_of_const suggests that there is some 'constness' involved, which > isn't the case. I'd leave this just as 'container_of'. This also reduces the > size of the patch, since this is done in quite a few places. The name threw me off too, but it's supposed to keep the argument pointer constness, not always take and return a const pointer. I still believe that it's beneficial since, if the work pointer was ever to change constness, we would have that additional check. Maxime
Re: [PATCH v2 3/9] drm/vc4: hdmi: Add Broadcast RGB property to allow override of RGB range
On Mon, Feb 20, 2023 at 4:24 PM Dave Stevenson wrote: > > Hi Hans > > On Sat, 18 Feb 2023 at 11:33, Hans Verkuil wrote: > > > > Hi Maxime, Dave, > > > > On 26/01/2023 14:46, Maxime Ripard wrote: > > > From: Dave Stevenson > > > > > > Copy Intel's "Broadcast RGB" property semantics to add manual override > > > of the HDMI pixel range for monitors that don't abide by the content > > > of the AVI Infoframe. > > > > Do we have to copy that property as-is? > > Firstly I'll agree with your later comment that having this control > allows testing of a range of output modes, and working around HDMI > sinks that have dodgy implementations. > (In our vendor kernel we now also have a property to override the > kernel chosen output format to enable testing of YCbCr4:4:4 and 4:2:2 > output). > > The DRM subsystem has the requirement for an open-source userspace > project to be using any new property before it will be merged [1]. > This property already exists for i915 and gma-500, therefore avoids > that requirement. > > There are objections to the UAPI for Broadcast RGB [2], but if it's > good enough for the existing implementations then there can be no > objection to it being implemented in the same way for other drivers. > Otherwise it is a missing feature of the DRM API, and the linked > discussion is realistically at least a year away from being resolved. > Why bury our heads in the sand for that period? > > [1] > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements > [2] https://lists.freedesktop.org/archives/dri-devel/2023-February/391061.html FWIW, from a user space perspective I'm still very much in favor of adding the `Broadcast RGB` property to more drivers and even moving it to drm core. Splitting things into color pipeline control properties and infoframe/connector properties is much harder than it seems at first and IMO needs a more holistic approach that you can't get from just converting the one property. > > > First of all, I think this should really be a drm-level property, rather > > than > > a driver property: RGB Quantization Range mismatches are the bane of my > > life, > > and I think a way to override this would help everyone. > > Linked to above, if it were the preferred method for controlling this > then I would expect it to become a drm-level property. > > > Secondly, I hate the name they came up with: 'Broadcast RGB' is pretty > > meaningless. > > Can't we stick to something closer to what the CTA-861/HDMI specs use, > > which is > > 'RGB Quantization Range'? So either use that, or just 'RGB Range'. > > > > In addition, 'Limited 16:235' should just be 'Limited' since the actual > > range > > depends on the bits-per-color-component. > > It's documented UAPI with those names[3], therefore any change would > be a change to user-space's expectation and a regression. At least by > sticking with the same names then any user space implementation that > exists for i915 or gma-500 will also work in the same way on vc4. > > [3] > https://www.kernel.org/doc/html/latest/gpu/drm-kms.html#existing-kms-properties > > > > > > > Signed-off-by: Dave Stevenson > > > Signed-off-by: Maxime Ripard > > > --- > > > drivers/gpu/drm/vc4/vc4_hdmi.c | 97 > > > -- > > > drivers/gpu/drm/vc4/vc4_hdmi.h | 9 > > > 2 files changed, 102 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > index 4b3bf77bb5cd..78749c6fa837 100644 > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c > > > @@ -150,10 +150,16 @@ static bool vc4_hdmi_mode_needs_scrambling(const > > > struct drm_display_mode *mode, > > > } > > > > > > static bool vc4_hdmi_is_full_range_rgb(struct vc4_hdmi *vc4_hdmi, > > > -const struct drm_display_mode *mode) > > > +struct vc4_hdmi_connector_state > > > *vc4_state) > > > { > > > + const struct drm_display_mode *mode = > > > &vc4_hdmi->saved_adjusted_mode; > > > struct drm_display_info *display = > > > &vc4_hdmi->connector.display_info; > > > > > > + if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_LIMITED) > > > + return false; > > > + else if (vc4_state->broadcast_rgb == VC4_HDMI_BROADCAST_RGB_FULL) > > > + return true; > > > + > > > return !display->is_hdmi || > > > drm_default_rgb_quant_range(mode) == > > > HDMI_QUANTIZATION_RANGE_FULL; > > > } > > > @@ -524,8 +530,12 @@ static int vc4_hdmi_connector_atomic_check(struct > > > drm_connector *connector, > > > { > > > struct drm_connector_state *old_state = > > > drm_atomic_get_old_connector_state(state, connector); > > > + struct vc4_hdmi_connector_state *old_vc4_state = > > > + conn_state_to_vc4_hdmi_conn_state(old_state); > > > struct drm_connector_state *new_state
Re: [PATCH v12 06/10] drm/bridge: Remove redundant i2c_client in anx7625/it6505
On Tue, Feb 21, 2023 at 05:50:50PM +0800, Pin-yen Lin wrote: > These two drivers embed a i2c_client in there private driver data, but > only strict device is actually needed. Replace the i2c_client reference > with a struct device one. LGTM, Reviewed-by: Andy Shevchenko > Signed-off-by: Pin-yen Lin > --- > > Changes in v12: > - New in v12 > > drivers/gpu/drm/bridge/analogix/anx7625.c | 96 > drivers/gpu/drm/bridge/analogix/anx7625.h | 2 +- > drivers/gpu/drm/bridge/ite-it6505.c | 128 +++--- > 3 files changed, 113 insertions(+), 113 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c > b/drivers/gpu/drm/bridge/analogix/anx7625.c > index 486b5099f5dd..cd628a2e2e50 100644 > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c > @@ -207,7 +207,7 @@ static int anx7625_read_ctrl_status_p0(struct > anx7625_data *ctx) > > static int wait_aux_op_finish(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int val; > int ret; > > @@ -234,7 +234,7 @@ static int wait_aux_op_finish(struct anx7625_data *ctx) > static int anx7625_aux_trans(struct anx7625_data *ctx, u8 op, u32 address, >u8 len, u8 *buf) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int ret; > u8 addrh, addrm, addrl; > u8 cmd; > @@ -427,7 +427,7 @@ static int anx7625_odfc_config(struct anx7625_data *ctx, > u8 post_divider) > { > int ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Config input reference clock frequency 27MHz/19.2MHz */ > ret = anx7625_write_and(ctx, ctx->i2c.rx_p1_client, MIPI_DIGITAL_PLL_16, > @@ -477,7 +477,7 @@ static int anx7625_set_k_value(struct anx7625_data *ctx) > > static int anx7625_dsi_video_timing_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > unsigned long m, n; > u16 htotal; > int ret; > @@ -575,7 +575,7 @@ static int anx7625_dsi_video_timing_config(struct > anx7625_data *ctx) > static int anx7625_swap_dsi_lane3(struct anx7625_data *ctx) > { > int val; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Swap MIPI-DSI data lane 3 P and N */ > val = anx7625_reg_read(ctx, ctx->i2c.rx_p1_client, MIPI_SWAP); > @@ -592,7 +592,7 @@ static int anx7625_api_dsi_config(struct anx7625_data > *ctx) > > { > int val, ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Swap MIPI-DSI data lane 3 P and N */ > ret = anx7625_swap_dsi_lane3(ctx); > @@ -657,7 +657,7 @@ static int anx7625_api_dsi_config(struct anx7625_data > *ctx) > > static int anx7625_dsi_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int ret; > > DRM_DEV_DEBUG_DRIVER(dev, "config dsi.\n"); > @@ -689,7 +689,7 @@ static int anx7625_dsi_config(struct anx7625_data *ctx) > > static int anx7625_api_dpi_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > u16 freq = ctx->dt.pixelclock.min / 1000; > int ret; > > @@ -720,7 +720,7 @@ static int anx7625_api_dpi_config(struct anx7625_data > *ctx) > > static int anx7625_dpi_config(struct anx7625_data *ctx) > { > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > int ret; > > DRM_DEV_DEBUG_DRIVER(dev, "config dpi\n"); > @@ -765,7 +765,7 @@ static int anx7625_read_flash_status(struct anx7625_data > *ctx) > static int anx7625_hdcp_key_probe(struct anx7625_data *ctx) > { > int ret, val; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > u8 ident[FLASH_BUF_LEN]; > > ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client, > @@ -815,7 +815,7 @@ static int anx7625_hdcp_key_probe(struct anx7625_data > *ctx) > static int anx7625_hdcp_key_load(struct anx7625_data *ctx) > { > int ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > /* Select HDCP 1.4 KEY */ > ret = anx7625_reg_write(ctx, ctx->i2c.rx_p0_client, > @@ -843,7 +843,7 @@ static int anx7625_hdcp_key_load(struct anx7625_data *ctx) > static int anx7625_hdcp_disable(struct anx7625_data *ctx) > { > int ret; > - struct device *dev = &ctx->client->dev; > + struct device *dev = ctx->dev; > > dev_dbg(dev, "disable HDCP 1.4\n"); > > @@ -864,7 +864,7 @@ static int anx7625_hdcp_enable(struct anx7625_data *ctx) > { > u8 bcap; > int ret; > - struct device *dev = &ctx->client->dev; >
Re: [PATCH v12 07/10] drm/bridge: anx7625: Register Type C mode switches
On Tue, Feb 21, 2023 at 05:50:51PM +0800, Pin-yen Lin wrote: > Register USB Type-C mode switches when the "mode-switch" property and > relevant ports are available in Device Tree. Configure the crosspoint > switch based on the entered alternate mode for a specific Type-C > connector. > > Crosspoint switch can also be used for switching the output signal for > different orientations of a single USB Type-C connector, but the > orientation switch is not implemented yet. A TODO is added for this. ... > +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx) > +{ > + int i; unsigned? + Blank line. > + /* Check if both ports available and do nothing to retain the current > one */ > + if (ctx->port_data[0].dp_connected && ctx->port_data[1].dp_connected) > + return; > + > + for (i = 0; i < 2; i++) { > + if (ctx->port_data[i].dp_connected) > + anx7625_set_crosspoint_switch(ctx, > + > ctx->port_data[i].orientation); > + } > +} ... > + ctx->port_data[port->port_num].dp_connected = > + state->alt && state->alt->svid == USB_TYPEC_DP_SID && I would move the first parameter of && to the separate line for slightly better readability. > + state->alt->mode == USB_TYPEC_DP_MODE; ... > + for (i = 0; i < switch_desc->num_typec_switches; i++) { > + struct drm_dp_typec_port_data *port = > &switch_desc->typec_ports[i]; > + struct fwnode_handle *fwnode = port->fwnode; > + > + num_lanes = fwnode_property_count_u32(fwnode, "data-lanes"); > + Redundant blank line. > + if (num_lanes < 0) { > + dev_err(dev, > + "Error on getting data lanes count from %pfwP: > %d\n", > + fwnode, num_lanes); > + ret = num_lanes; Can be written differently: > + goto unregister_mux; > + } ret = ... if (ret < 0) { ... } num_lanes = ret; What if it's 0? > + ret = fwnode_property_read_u32_array(fwnode, "data-lanes", > + dp_lanes, num_lanes); > + if (ret) { > + dev_err(dev, > + "Failed to read the data-lanes variable: %d\n", > + ret); > + goto unregister_mux; > + } > + > + ctx->port_data[i].orientation = (dp_lanes[0] / 2 == 0) ? > + TYPEC_ORIENTATION_REVERSE : TYPEC_ORIENTATION_NORMAL; > + ctx->port_data[i].dp_connected = false; > + } > + complete_all(&ctx->mux_register); > + > + return 0; > + > +unregister_mux: > + complete_all(&ctx->mux_register); > + anx7625_unregister_typec_switches(ctx); > + return ret; > +} -- With Best Regards, Andy Shevchenko
Re: [PATCH v12 01/10] device property: Add remote endpoint to devcon matcher
On Tue, Feb 21, 2023 at 05:50:45PM +0800, Pin-yen Lin wrote: > From: Prashant Malani > > When searching the device graph for device matches, check the > remote-endpoint itself for a match. > > Some drivers register devices for individual endpoints. This allows > the matcher code to evaluate those for a match too, instead > of only looking at the remote parent devices. This is required when a > device supports two mode switches in its endpoints, so we can't simply > register the mode switch with the parent node. ... > * @match: Function to check and convert the connection description > * > * Find a connection with unique identifier @con_id between @fwnode and > another > - * device node. @match will be used to convert the connection description to > - * data the caller is expecting to be returned. > + * device node. For fwnode graph connections, the graph endpoints are also > + * checked. @match will be used to convert the connection description to data > + * the caller is expecting to be returned. > */ Please add a Return: section at the end of the kernel doc. Otherwise it complains that there is none. ... > * @matches_len: Length of @matches > * > * Find up to @matches_len connections with unique identifier @con_id between > - * @fwnode and other device nodes. @match will be used to convert the > - * connection description to data the caller is expecting to be returned > - * through the @matches array. > + * @fwnode and other device nodes. For fwnode graph connections, the graph > + * endpoints are also checked. @match will be used to convert the connection > + * description to data the caller is expecting to be returned through the > + * @matches array. > * If @matches is NULL @matches_len is ignored and the total number of > resolved > * matches is returned. Ditto. -- With Best Regards, Andy Shevchenko
[Bug 217065] New: Linux 6.2.0 - AMDGPU cannot start, ends in a blank screen.
https://bugzilla.kernel.org/show_bug.cgi?id=217065 Bug ID: 217065 Summary: Linux 6.2.0 - AMDGPU cannot start, ends in a blank screen. Product: Drivers Version: 2.5 Kernel Version: 6.2.0 Hardware: All OS: Linux Tree: Mainline Status: NEW Severity: normal Priority: P1 Component: Video(DRI - non Intel) Assignee: drivers_video-...@kernel-bugs.osdl.org Reporter: fredbez...@gmail.com Regression: No Hello. I installed this morning on my Archlinux installation linux 6.2.0. I noticed that boot is stopped when using amdgpu driver. Here is my amdgpu driver info: $ inxi -G Graphics: Device-1: AMD Raven Ridge [Radeon Vega Series / Radeon Mobile Series] driver: amdgpu v: kernel I also reported the issue on archlinux bugtracker: https://bugs.archlinux.org/task/77595 -- 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] drm/fb-helper: Remove drm_fb_helper_unprepare() from drm_fb_helper_fini()
Thomas Zimmermann writes: > Move drm_fb_helper_unprepare() from drm_fb_helper_fini() into the > calling fbdev implementation. Avoids a possible stale mutex with > generic fbdev code. > > As indicated by its name, drm_fb_helper_prepare() prepares struct > drm_fb_helper before setting up the fbdev support with a call to > drm_fb_helper_init(). In legacy fbdev emulation, this happens next > to each other. If successful, drm_fb_helper_fini() later tear down > the fbdev device and also unprepare via drm_fb_helper_unprepare(). > > Generic fbdev emulation prepares struct drm_fb_helper immediately > after allocating the instance. It only calls drm_fb_helper_init() > as part of processing a hotplug event. If the hotplug-handling fails, > it runs drm_fb_helper_fini(). This unprepares the fb-helper instance > and the next hotplug event runs on stale data. > > Solve this by moving drm_fb_helper_unprepare() from drm_fb_helper_fini() > into the fbdev implementations. Call it right before freeing the > fb-helper instance. > > Fixes: 4825797c36da ("drm/fb-helper: Introduce drm_fb_helper_unprepare()") I think this should be Fixes: 032116bbe152 ("drm/fbdev-generic: Minimize client unregistering") instead? Because commit 4825797c36da just added a wrapper function for mutex_destroy(&fb_helper->lock), but it was commit 032116bbe152 that made drm_fbdev_cleanup() to call that helper function. > Cc: Thomas Zimmermann > Cc: Javier Martinez Canillas > Cc: Maarten Lankhorst > Cc: Maxime Ripard > Cc: David Airlie > Cc: Daniel Vetter > Cc: dri-devel@lists.freedesktop.org > > Signed-off-by: Thomas Zimmermann > --- The change itself looks good to me. Reviewed-by: Javier Martinez Canillas -- Best regards, Javier Martinez Canillas Core Platforms Red Hat