Re: [PATCH 07/14] riscv: dts: canaan: fix the k210's memory node
On 6/20/22 08:54, conor.doo...@microchip.com wrote:
> On 20/06/2022 00:38, Damien Le Moal wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
>> content is safe
>>
>> On 6/18/22 21:30, Conor Dooley wrote:
>>> From: Conor Dooley
>>>
>>> The k210 memory node has a compatible string that does not match with
>>> any driver or dt-binding & has several non standard properties.
>>> Replace the reg names with a comment and delete the rest.
>>>
>>> Signed-off-by: Conor Dooley
>>> ---
>>> ---
>>> arch/riscv/boot/dts/canaan/k210.dtsi | 6 --
>>> 1 file changed, 6 deletions(-)
>>>
>>> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi
>>> b/arch/riscv/boot/dts/canaan/k210.dtsi
>>> index 44d338514761..287ea6eebe47 100644
>>> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
>>> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
>>> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>>>
>>> sram: memory@8000 {
>>> device_type = "memory";
>>> - compatible = "canaan,k210-sram";
>>> reg = <0x8000 0x40>,
>>> <0x8040 0x20>,
>>> <0x8060 0x20>;
>>> - reg-names = "sram0", "sram1", "aisram";
>>> - clocks = <&sysclk K210_CLK_SRAM0>,
>>> - <&sysclk K210_CLK_SRAM1>,
>>> - <&sysclk K210_CLK_AI>;
>>> - clock-names = "sram0", "sram1", "aisram";
>>> };
>>
>> These are used by u-boot to setup the memory clocks and initialize the
>> aisram. Sure the kernel actually does not use this, but to be in sync with
>> u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
>> kernel work fine with both u-boot internal DT and the kernel DT.
>
> Right, but unfortunately that desire alone doesn't do anything about
> the dtbs_check complaints.
>
> I guess the alternative approach of actually documenting the compatible
> would be more palatable?
Yes, I think so. That would allow keeping the fields without the DTB build
warnings.
>
> Thanks,
> Conor.
>
>
>
--
Damien Le Moal
Western Digital Research
Re: [PATCH 07/14] riscv: dts: canaan: fix the k210's memory node
On 6/18/22 21:30, Conor Dooley wrote:
> From: Conor Dooley
>
> The k210 memory node has a compatible string that does not match with
> any driver or dt-binding & has several non standard properties.
> Replace the reg names with a comment and delete the rest.
>
> Signed-off-by: Conor Dooley
> ---
> ---
> arch/riscv/boot/dts/canaan/k210.dtsi | 6 --
> 1 file changed, 6 deletions(-)
>
> diff --git a/arch/riscv/boot/dts/canaan/k210.dtsi
> b/arch/riscv/boot/dts/canaan/k210.dtsi
> index 44d338514761..287ea6eebe47 100644
> --- a/arch/riscv/boot/dts/canaan/k210.dtsi
> +++ b/arch/riscv/boot/dts/canaan/k210.dtsi
> @@ -69,15 +69,9 @@ cpu1_intc: interrupt-controller {
>
> sram: memory@8000 {
> device_type = "memory";
> - compatible = "canaan,k210-sram";
> reg = <0x8000 0x40>,
> <0x8040 0x20>,
> <0x8060 0x20>;
> - reg-names = "sram0", "sram1", "aisram";
> - clocks = <&sysclk K210_CLK_SRAM0>,
> - <&sysclk K210_CLK_SRAM1>,
> - <&sysclk K210_CLK_AI>;
> - clock-names = "sram0", "sram1", "aisram";
> };
These are used by u-boot to setup the memory clocks and initialize the
aisram. Sure the kernel actually does not use this, but to be in sync with
u-boot DT, I would prefer keeping this as is. Right now, u-boot *and* the
kernel work fine with both u-boot internal DT and the kernel DT.
--
Damien Le Moal
Western Digital Research
Re: [PATCH, v2] media: mediatek: vcodec: Fix non subdev architecture open power fail
On Fri, Jun 17, 2022 at 3:25 PM Yunfei Dong wrote:
>
> According to subdev_bitmap bit value to open hardware power, need to
> set subdev_bitmap value for non subdev architecture.
>
> Fixes: c05bada35f01 ("media: mtk-vcodec: Add to support multi hardware
> decode")
> Signed-off-by: Yunfei Dong
Reviewed-by: Chen-Yu Tsai
Tested-by: Chen-Yu Tsai
on MT8183 on next-20220617. This makes the hardware operate correctly.
Previously it kept timing out, presumably because the hardware wasn't
properly enabled.
Re: [Intel-gfx] [PATCH v2 1/1] i915/gem: drop wbinvd_on_all_cpus usage
Hi, On Fri, 2022-06-17 at 16:30 -0700, Lucas De Marchi wrote: > On Thu, Apr 14, 2022 at 11:19:23AM -0700, Michael Cheng wrote: > > Previous concern with using drm_clflush_sg was that we don't know > > what the > > sg_table is pointing to, thus the usage of wbinvd_on_all_cpus to > > flush > > everything at once to avoid paranoia. > > humn... and now we know it is backed by struct pages? I'm not sure I > follow what we didn't know before and now we do. > > Thomas / Matthew, could you take another look herer if it seems > correct > to you. > > No, this isn't correct. A dma-buf sg-table may not be backed by struct pages, and AFAIK, there is no way for the importer to tell, whether that's the case or not. If we need to get rid of the wbinvd_on_all_cpus here, we need to use the dma_buf_vmap() function to obtain a virtual address and then use drm_clflush_virt_range() on that address. After that clflush, we can call dma_buf_vunmap(). This should work since x86 uses PIPT caches and a flush to a virtual range should flush all other virtual ranges pointing to the same pages as well. /Thomas > thanks > Lucas De Marchi > > > > To make i915 more architecture-neutral and be less paranoid, lets > > attempt to > > use drm_clflush_sg to flush the pages for when the GPU wants to > > read > > from main memory. > > > > Signed-off-by: Michael Cheng > > --- > > drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c | 9 ++--- > > 1 file changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > index f5062d0c6333..b0a5baaebc43 100644 > > --- a/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c > > @@ -8,6 +8,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -250,16 +251,10 @@ static int > > i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj) > > * DG1 is special here since it still snoops transactions > > even with > > * CACHE_NONE. This is not the case with other HAS_SNOOP > > platforms. We > > * might need to revisit this as we add new discrete > > platforms. > > - * > > - * XXX: Consider doing a vmap flush or something, where > > possible. > > - * Currently we just do a heavy handed wbinvd_on_all_cpus() > > here since > > - * the underlying sg_table might not even point to struct > > pages, so we > > - * can't just call drm_clflush_sg or similar, like we do > > elsewhere in > > - * the driver. > > */ > > if (i915_gem_object_can_bypass_llc(obj) || > > (!HAS_LLC(i915) && !IS_DG1(i915))) > > - wbinvd_on_all_cpus(); > > + drm_clflush_sg(pages); > > > > sg_page_sizes = i915_sg_dma_sizes(pages->sgl); > > __i915_gem_object_set_pages(obj, pages, sg_page_sizes); > > -- > > 2.25.1 > >
[PATCH v2 0/4] HDR aux backlight range calculation
This patch set splits out static hdr metadata backlight range parsing from gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c into gpu/drm/drm-edid.c as a new function. This new function is then used during edid parsing when HDR static metadata block parsing. Calculated values are stored in a new struct drm_luminance_range introduced into display_info. Amdgpu_dm.c and intel_dp_aux_backlight.c are using this new data. v2: Calculate the range during edid parsing and store into display_info Cc: Roman Li Cc: Maarten Lankhorst Cc: Rodrigo Siqueira Cc: Harry Wentland Cc: Lyude Paul Cc: Mika Kahola Cc: Jani Nikula Cc: Manasi Navare Jouni Högander (4): drm/display: Add drm_luminance_range_info drm: New function to get luminance range based on static hdr metadata drm/amdgpu_dm: Rely on split out luminance calculation function drm/i915: Use luminance range calculated during edid parsing .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 ++--- drivers/gpu/drm/drm_edid.c| 50 ++- .../drm/i915/display/intel_dp_aux_backlight.c | 17 +-- include/drm/drm_connector.h | 21 4 files changed, 88 insertions(+), 35 deletions(-) -- 2.25.1
[PATCH v2 1/4] drm/display: Add drm_luminance_range_info
Add new data structure to store luminance range calculated using
data from EDID's static hdr metadata block. Add this new struct
as a part of drm_display_info struct.
Cc: Roman Li
Cc: Rodrigo Siqueira
Cc: Harry Wentland
Cc: Lyude Paul
Cc: Mika Kahola
Cc: Jani Nikula
Cc: Manasi Navare
Signed-off-by: Jouni Högander
---
include/drm/drm_connector.h | 21 +
1 file changed, 21 insertions(+)
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 3ac4bf87f257..7d8eeac6cc68 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -322,6 +322,22 @@ struct drm_monitor_range_info {
u8 max_vfreq;
};
+/**
+ * struct drm_luminance_range_info - Panel's luminance range for
+ * &drm_display_info. Calculated using data in EDID
+ *
+ * This struct is used to store a luminance range supported by panel
+ * as calculated using data from EDID's static hdr metadata.
+ *
+ * @min_luminance: This is the min supported luminance value
+ *
+ * @max_luminance: This is the max supported luminance value
+ */
+struct drm_luminance_range_info {
+ u32 min_luminance;
+ u32 max_luminance;
+};
+
/**
* enum drm_privacy_screen_status - privacy screen status
*
@@ -623,6 +639,11 @@ struct drm_display_info {
*/
struct drm_monitor_range_info monitor_range;
+ /**
+* @luminance_range: Luminance range supported by panel
+*/
+ struct drm_luminance_range_info luminance_range;
+
/**
* @mso_stream_count: eDP Multi-SST Operation (MSO) stream count from
* the DisplayID VESA vendor block. 0 for conventional Single-Stream
--
2.25.1
[PATCH v2 2/4] drm: New function to get luminance range based on static hdr metadata
Split luminance min/max calculation using static hdr metadata from
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:update_connector_ext_caps
into drm/drm_edid.c and use it during edid parsing. Calculated range is
stored into connector->display_info->luminance_range.
v2: Calculate range during edid parsing
Cc: Roman Li
Cc: Rodrigo Siqueira
Cc: Harry Wentland
Cc: Lyude Paul
Cc: Mika Kahola
Cc: Jani Nikula
Cc: Manasi Navare
Signed-off-by: Jouni Högander
---
drivers/gpu/drm/drm_edid.c | 50 +-
1 file changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 2bdaf1e34a9d..3b367100290f 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5014,6 +5014,49 @@ static void fixup_detailed_cea_mode_clock(struct
drm_display_mode *mode)
mode->clock = clock;
}
+static void drm_calculate_luminance_range(struct drm_connector *connector)
+{
+ struct hdr_static_metadata *hdr_metadata =
&connector->hdr_sink_metadata.hdmi_type1;
+ struct drm_luminance_range_info *luminance_range =
+ &connector->display_info.luminance_range;
+ static const u8 pre_computed_values[] = {
+ 50, 51, 52, 53, 55, 56, 57, 58, 59, 61, 62, 63, 65, 66, 68, 69,
+ 71, 72, 74, 75, 77, 79, 81, 82, 84, 86, 88, 90, 92, 94, 96, 98};
+ u32 max_avg, min_cll, max, min, q, r;
+
+ if (!(hdr_metadata->metadata_type & BIT(HDMI_STATIC_METADATA_TYPE1)))
+ return;
+
+ max_avg = hdr_metadata->max_fall;
+ min_cll = hdr_metadata->min_cll;
+
+ /* From the specification (CTA-861-G), for calculating the maximum
+* luminance we need to use:
+* Luminance = 50*2**(CV/32)
+* Where CV is a one-byte value.
+* For calculating this expression we may need float point precision;
+* to avoid this complexity level, we take advantage that CV is divided
+* by a constant. From the Euclids division algorithm, we know that CV
+* can be written as: CV = 32*q + r. Next, we replace CV in the
+* Luminance expression and get 50*(2**q)*(2**(r/32)), hence we just
+* need to pre-compute the value of r/32. For pre-computing the values
+* We just used the following Ruby line:
+* (0...32).each {|cv| puts (50*2**(cv/32.0)).round}
+* The results of the above expressions can be verified at
+* pre_computed_values.
+*/
+ q = max_avg >> 5;
+ r = max_avg % 32;
+ max = (1 << q) * pre_computed_values[r];
+
+ /* min luminance: maxLum * (CV/255)^2 / 100 */
+ q = DIV_ROUND_CLOSEST(min_cll, 255);
+ min = max * DIV_ROUND_CLOSEST((q * q), 100);
+
+ luminance_range->min_luminance = min;
+ luminance_range->max_luminance = max;
+}
+
static uint8_t eotf_supported(const u8 *edid_ext)
{
return edid_ext[2] &
@@ -5045,8 +5088,12 @@ drm_parse_hdr_metadata_block(struct drm_connector
*connector, const u8 *db)
connector->hdr_sink_metadata.hdmi_type1.max_cll = db[4];
if (len >= 5)
connector->hdr_sink_metadata.hdmi_type1.max_fall = db[5];
- if (len >= 6)
+ if (len >= 6) {
connector->hdr_sink_metadata.hdmi_type1.min_cll = db[6];
+
+ /* Calculate only when all values are available */
+ drm_calculate_luminance_range(connector);
+ }
}
static void
@@ -5951,6 +5998,7 @@ drm_reset_display_info(struct drm_connector *connector)
info->non_desktop = 0;
memset(&info->monitor_range, 0, sizeof(info->monitor_range));
+ memset(&info->luminance_range, 0, sizeof(info->luminance_range));
info->mso_stream_count = 0;
info->mso_pixel_overlap = 0;
--
2.25.1
[PATCH v2 3/4] drm/amdgpu_dm: Rely on split out luminance calculation function
Luminance range calculation was split out into drm_edid.c and is now
part of edid parsing. Rely on values calculated during edid parsing and
use these for caps->aux_max_input_signal and caps->aux_min_input_signal.
v2: Use values calculated during edid parsing
Cc: Roman Li
Cc: Rodrigo Siqueira
Cc: Harry Wentland
Cc: Lyude Paul
Cc: Mika Kahola
Cc: Jani Nikula
Cc: Manasi Navare
Signed-off-by: Jouni Högander
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 35 +++
1 file changed, 4 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 428bb041f92c..896a6b5eb2ed 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2813,15 +2813,12 @@ static struct drm_mode_config_helper_funcs
amdgpu_dm_mode_config_helperfuncs = {
static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
{
- u32 max_avg, min_cll, max, min, q, r;
struct amdgpu_dm_backlight_caps *caps;
struct amdgpu_display_manager *dm;
struct drm_connector *conn_base;
struct amdgpu_device *adev;
struct dc_link *link = NULL;
- static const u8 pre_computed_values[] = {
- 50, 51, 52, 53, 55, 56, 57, 58, 59, 61, 62, 63, 65, 66, 68, 69,
- 71, 72, 74, 75, 77, 79, 81, 82, 84, 86, 88, 90, 92, 94, 96, 98};
+ struct drm_luminance_range_info *luminance_range;
int i;
if (!aconnector || !aconnector->dc_link)
@@ -2843,8 +2840,6 @@ static void update_connector_ext_caps(struct
amdgpu_dm_connector *aconnector)
caps = &dm->backlight_caps[i];
caps->ext_caps = &aconnector->dc_link->dpcd_sink_ext_caps;
caps->aux_support = false;
- max_avg = conn_base->hdr_sink_metadata.hdmi_type1.max_fall;
- min_cll = conn_base->hdr_sink_metadata.hdmi_type1.min_cll;
if (caps->ext_caps->bits.oled == 1 /*||
caps->ext_caps->bits.sdr_aux_backlight_control == 1 ||
@@ -2856,31 +2851,9 @@ static void update_connector_ext_caps(struct
amdgpu_dm_connector *aconnector)
else if (amdgpu_backlight == 1)
caps->aux_support = true;
- /* From the specification (CTA-861-G), for calculating the maximum
-* luminance we need to use:
-* Luminance = 50*2**(CV/32)
-* Where CV is a one-byte value.
-* For calculating this expression we may need float point precision;
-* to avoid this complexity level, we take advantage that CV is divided
-* by a constant. From the Euclids division algorithm, we know that CV
-* can be written as: CV = 32*q + r. Next, we replace CV in the
-* Luminance expression and get 50*(2**q)*(2**(r/32)), hence we just
-* need to pre-compute the value of r/32. For pre-computing the values
-* We just used the following Ruby line:
-* (0...32).each {|cv| puts (50*2**(cv/32.0)).round}
-* The results of the above expressions can be verified at
-* pre_computed_values.
-*/
- q = max_avg >> 5;
- r = max_avg % 32;
- max = (1 << q) * pre_computed_values[r];
-
- // min luminance: maxLum * (CV/255)^2 / 100
- q = DIV_ROUND_CLOSEST(min_cll, 255);
- min = max * DIV_ROUND_CLOSEST((q * q), 100);
-
- caps->aux_max_input_signal = max;
- caps->aux_min_input_signal = min;
+ luminance_range = &conn_base->display_info.luminance_range;
+ caps->aux_min_input_signal = luminance_range->min_luminance;
+ caps->aux_max_input_signal = luminance_range->max_luminance;
}
void amdgpu_dm_update_connector_after_detect(
--
2.25.1
[PATCH v2 4/4] drm/i915: Use luminance range calculated during edid parsing
Instead of using fixed 0 - 512 range use luminance range calculated
as a part of edid parsing. As a backup fall back to static 0 - 512.
v2: Use values calculated during edid parsing
Cc: Lyude Paul
Cc: Mika Kahola
Cc: Jani Nikula
Cc: Manasi Navare
Signed-off-by: Jouni Högander
---
.../drm/i915/display/intel_dp_aux_backlight.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
index c92d5bb2326a..b2666bd67701 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c
@@ -276,8 +276,11 @@ intel_dp_aux_hdr_disable_backlight(const struct
drm_connector_state *conn_state,
static int
intel_dp_aux_hdr_setup_backlight(struct intel_connector *connector, enum pipe
pipe)
{
- struct drm_i915_private *i915 = to_i915(connector->base.dev);
+ struct drm_connector *conn_base = &connector->base;
+ struct drm_i915_private *i915 = to_i915(conn_base->dev);
struct intel_panel *panel = &connector->panel;
+ struct drm_luminance_range_info *luminance_range =
+ &conn_base->display_info.luminance_range;
int ret;
if (panel->backlight.edp.intel.sdr_uses_aux) {
@@ -293,8 +296,16 @@ intel_dp_aux_hdr_setup_backlight(struct intel_connector
*connector, enum pipe pi
}
}
- panel->backlight.max = 512;
- panel->backlight.min = 0;
+ if (!luminance_range->max_luminance) {
+ panel->backlight.max = 512;
+ panel->backlight.min = 0;
+ } else {
+ panel->backlight.max = luminance_range->max_luminance;
+ panel->backlight.min = luminance_range->min_luminance;
+ }
+
+ drm_dbg(&i915->drm, "Using range %d..%d\n", panel->backlight.min,
panel->backlight.max);
+
panel->backlight.level = intel_dp_aux_hdr_get_backlight(connector,
pipe);
panel->backlight.enabled = panel->backlight.level != 0;
--
2.25.1
Re: [PATCH 06/14] spi: dt-bindings: dw-apb-ssi: update spi-{r,t}x-bus-width for dwc-ssi
Hi Conor,
On Sat, Jun 18, 2022 at 2:32 PM Conor Dooley wrote:
> From: Conor Dooley
>
> snps,dwc-ssi-1.01a has a single user - the Canaan k210, which uses a
> width of 4 for spi-{r,t}x-bus-width. Update the binding to reflect
> this.
>
> Signed-off-by: Conor Dooley
Thanks for your patch!
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -135,19 +135,41 @@ properties:
>of the designware controller, and the upper limit is also subject to
>controller configuration.
>
> -patternProperties:
> - "^.*@[0-9a-f]+$":
> -type: object
> -properties:
> - reg:
> -minimum: 0
> -maximum: 3
> -
> - spi-rx-bus-width:
> -const: 1
> -
> - spi-tx-bus-width:
> -const: 1
> +if:
> + properties:
> +compatible:
> + contains:
> +const: snps,dwc-ssi-1.01a
> +
> +then:
> + patternProperties:
> +"^.*@[0-9a-f]+$":
> + type: object
> + properties:
> +reg:
> + minimum: 0
> + maximum: 3
> +
> +spi-rx-bus-width:
> + const: 4
> +
> +spi-tx-bus-width:
> + const: 4
These two also depend on the board (SPI device + wiring).
So all of [1, 2, 4] are valid values.
> +
> +else:
> + patternProperties:
> +"^.*@[0-9a-f]+$":
> + type: object
> + properties:
> +reg:
> + minimum: 0
> + maximum: 3
> +
> +spi-rx-bus-width:
> + const: 1
> +
> +spi-tx-bus-width:
> + const: 1
>
> unevaluatedProperties: false
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH, v2] media: mediatek: vcodec: Initialize decoder parameters after getting dec_capability
Il 20/06/22 08:32, Yunfei Dong ha scritto:
Need to get dec_capability from scp first, then initialize decoder
supported format and other parameters according to dec_capability value.
Fixes: fd00d90330d1 ("media: mtk-vcodec: vdec: move stateful ops into their own
file")
Signed-off-by: Yunfei Dong
Tested-by: Chen-Yu Tsai
Reviewed-by: AngeloGioacchino Del Regno
Re: [PATCH, v2] media: mediatek: vcodec: Initialize decoder parameters after getting dec_capability
On Mon, Jun 20, 2022 at 2:32 PM Yunfei Dong wrote:
>
> Need to get dec_capability from scp first, then initialize decoder
> supported format and other parameters according to dec_capability value.
>
> Fixes: fd00d90330d1 ("media: mtk-vcodec: vdec: move stateful ops into their
> own file")
> Signed-off-by: Yunfei Dong
> Tested-by: Chen-Yu Tsai
Reviewed-by: Chen-Yu Tsai
Re: [PATCH v2 1/3] dt-bindings: phy: qcom,hdmi-phy-qmp: add clock-cells and XO clock
On 20/06/2022 03:02, Dmitry Baryshkov wrote: > As the QMP HDMI PHY is a clock provider, add constant #clock-cells > property. For the compatibility with older DTs the property is not > marked as required. Also add the XO clock to the list of the clocks used > by the driver. > > Signed-off-by: Dmitry Baryshkov Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH v2 3/3] arm64: dts: qcom: msm8996: add #clock-cells and XO clock to the HDMI PHY node
On 20/06/2022 03:03, Dmitry Baryshkov wrote: > Add #clock-cells property to the HDMI PHY device node to let other nodes > resolve the hdmipll clock. While we are at it, also add the XO clock to > the device node. > > Signed-off-by: Dmitry Baryshkov Acked-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH v5 01/13] mm: add zone device coherent type memory support
Oded Gabbay writes:
> On Mon, Jun 20, 2022 at 3:33 AM Alistair Popple wrote:
>>
>>
>> Oded Gabbay writes:
>>
>> > On Fri, Jun 17, 2022 at 8:20 PM Sierra Guiza, Alejandro (Alex)
>> > wrote:
>> >>
>> >>
>> >> On 6/17/2022 4:40 AM, David Hildenbrand wrote:
>> >> > On 31.05.22 22:00, Alex Sierra wrote:
>> >> >> Device memory that is cache coherent from device and CPU point of view.
>> >> >> This is used on platforms that have an advanced system bus (like CAPI
>> >> >> or CXL). Any page of a process can be migrated to such memory. However,
>> >> >> no one should be allowed to pin such memory so that it can always be
>> >> >> evicted.
>> >> >>
>> >> >> Signed-off-by: Alex Sierra
>> >> >> Acked-by: Felix Kuehling
>> >> >> Reviewed-by: Alistair Popple
>> >> >> [hch: rebased ontop of the refcount changes,
>> >> >>removed is_dev_private_or_coherent_page]
>> >> >> Signed-off-by: Christoph Hellwig
>> >> >> ---
>> >> >> include/linux/memremap.h | 19 +++
>> >> >> mm/memcontrol.c | 7 ---
>> >> >> mm/memory-failure.c | 8 ++--
>> >> >> mm/memremap.c| 10 ++
>> >> >> mm/migrate_device.c | 16 +++-
>> >> >> mm/rmap.c| 5 +++--
>> >> >> 6 files changed, 49 insertions(+), 16 deletions(-)
>> >> >>
>> >> >> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
>> >> >> index 8af304f6b504..9f752ebed613 100644
>> >> >> --- a/include/linux/memremap.h
>> >> >> +++ b/include/linux/memremap.h
>> >> >> @@ -41,6 +41,13 @@ struct vmem_altmap {
>> >> >>* A more complete discussion of unaddressable memory may be found in
>> >> >>* include/linux/hmm.h and Documentation/vm/hmm.rst.
>> >> >>*
>> >> >> + * MEMORY_DEVICE_COHERENT:
>> >> >> + * Device memory that is cache coherent from device and CPU point of
>> >> >> view. This
>> >> >> + * is used on platforms that have an advanced system bus (like CAPI
>> >> >> or CXL). A
>> >> >> + * driver can hotplug the device memory using ZONE_DEVICE and with
>> >> >> that memory
>> >> >> + * type. Any page of a process can be migrated to such memory.
>> >> >> However no one
>> >> > Any page might not be right, I'm pretty sure. ... just thinking about
>> >> > special pages
>> >> > like vdso, shared zeropage, ... pinned pages ...
>> >>
>> >> Hi David,
>> >>
>> >> Yes, I think you're right. This type does not cover all special pages.
>> >> I need to correct that on the cover letter.
>> >> Pinned pages are allowed as long as they're not long term pinned.
>> >>
>> >> Regards,
>> >> Alex Sierra
>> >
>> > What if I want to hotplug this device's coherent memory, but I do
>> > *not* want the OS
>> > to migrate any page to it ?
>> > I want to fully-control what resides on this memory, as I can consider
>> > this memory
>> > "expensive". i.e. I don't have a lot of it, I want to use it for
>> > specific purposes and
>> > I don't want the OS to start using it when there is some memory pressure in
>> > the system.
>>
>> This is exactly what MEMORY_DEVICE_COHERENT is for. Device coherent
>> pages are only allocated by a device driver and exposed to user-space by
>> a driver migrating pages to them with migrate_vma. The OS can't just
>> start using them due to memory pressure for example.
>>
>> - Alistair
> Thanks for the explanation.
>
> I guess the commit message confused me a bit, especially these two sentences:
>
> "Any page of a process can be migrated to such memory. However no one should
> be
> allowed to pin such memory so that it can always be evicted."
>
> I read them as if the OS is free to choose which pages are migrated to
> this memory,
> and anything is eligible for migration to that memory (and that's why
> we also don't
> allow it to pin memory there).
>
> If we are not allowed to pin anything there, can the device driver
> decide to disable
> any option for oversubscription of this memory area ?
I'm not sure I follow your thinking on how oversubscription would work
here, however all allocations are controlled by the driver. So if a
device's coherent memory is full a driver would be unable to migrate
pages to that device until pages are freed by the OS due to being
unmapped or the driver evicts pages by migrating them back to normal CPU
memory.
Pinning of pages is allowed, and could prevent such migrations. However
this patch series prevents device coherent pages from being pinned
longterm (ie. with FOLL_LONGTERM), so it should always be able to evict
pages eventually.
> Let's assume the user uses this memory area for doing p2p with other
> CXL devices.
> In that case, I wouldn't want the driver/OS to migrate pages in and
> out of that memory...
The OS will not migrate pages in or out (although it may free them if no
longer required), but a driver might choose to. So at the moment it's
really up to the driver to implement what you want in this regards.
> So either I should let the user pin those pages, or prevent him from
> doing (accidently or n
Re: radeon driver warning
Am 17.06.22 um 16:22 schrieb John Garry: Hi Christian, Am 17.06.22 um 14:01 schrieb John Garry: On 17/06/2022 12:57, Christian König wrote: And/Or compile out the warning when "warnings = errors"? That should be doable I think. ok, if something can be done then I would appreciate it. I do much randconfig builds as part of my upstream process and anything breaking is a bit of a pain. I've just double checked the code and we have already wrapped the warning into "#ifndef CONFIG_COMPILE_TEST". Yes So the question is why does your random config not set CONFIG_COMPILE_TEST? My randconfig does not have CONFIG_COMPILE_TEST set - see attached. AFAIK randconfig does not always set CONFIG_COMPILE_TEST. Mhm, we could probably change the ifdef. But a random configuration which doesn't sets CONFIG_COMPILE_TEST sounds like a bug to me as well. Going to provide a patch for changing the ifdef, but not sure when I will have time for that. Regards, Christian. Thanks, John
[PATCH 1/2] drm/amdgpu: Drop CONFIG_BACKLIGHT_CLASS_DEVICE ifdefs
The DRM_AMDGPU Kconfig code contains:
select BACKLIGHT_CLASS_DEVICE
So the condition these ifdefs test for is always true, drop them.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c | 6 --
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 4
drivers/gpu/drm/amd/amdgpu/atombios_encoders.c| 14 --
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 15 +--
4 files changed, 1 insertion(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
index 98ac53ee6bb5..130060834b4e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acpi.c
@@ -66,9 +66,7 @@ struct amdgpu_atif {
struct amdgpu_atif_notifications notifications;
struct amdgpu_atif_functions functions;
struct amdgpu_atif_notification_cfg notification_cfg;
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
struct backlight_device *bd;
-#endif
struct amdgpu_dm_backlight_caps backlight_caps;
};
@@ -436,7 +434,6 @@ static int amdgpu_atif_handler(struct amdgpu_device *adev,
DRM_DEBUG_DRIVER("ATIF: %d pending SBIOS requests\n", count);
if (req.pending & ATIF_PANEL_BRIGHTNESS_CHANGE_REQUEST) {
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
if (atif->bd) {
DRM_DEBUG_DRIVER("Changing brightness to %d\n",
req.backlight_level);
@@ -447,7 +444,6 @@ static int amdgpu_atif_handler(struct amdgpu_device *adev,
*/
backlight_device_set_brightness(atif->bd,
req.backlight_level);
}
-#endif
}
if (req.pending & ATIF_DGPU_DISPLAY_EVENT) {
@@ -849,7 +845,6 @@ int amdgpu_acpi_init(struct amdgpu_device *adev)
{
struct amdgpu_atif *atif = &amdgpu_acpi_priv.atif;
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
if (atif->notifications.brightness_change) {
if (amdgpu_device_has_dc_support(adev)) {
#if defined(CONFIG_DRM_AMD_DC)
@@ -876,7 +871,6 @@ int amdgpu_acpi_init(struct amdgpu_device *adev)
}
}
}
-#endif
adev->acpi_nb.notifier_call = amdgpu_acpi_event;
register_acpi_notifier(&adev->acpi_nb);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index f80b4838cea1..dbe2904e015b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -349,15 +349,11 @@ struct amdgpu_mode_info {
#define AMDGPU_MAX_BL_LEVEL 0xFF
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
-
struct amdgpu_backlight_privdata {
struct amdgpu_encoder *encoder;
uint8_t negative;
};
-#endif
-
struct amdgpu_atom_ss {
uint16_t percentage;
uint16_t percentage_divider;
diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
index 8158677302fe..e2056fbbc750 100644
--- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
+++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
@@ -120,8 +120,6 @@ amdgpu_atombios_encoder_set_backlight_level(struct
amdgpu_encoder *amdgpu_encode
}
}
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
-
static u8 amdgpu_atombios_encoder_backlight_level(struct backlight_device *bd)
{
u8 level;
@@ -263,18 +261,6 @@ amdgpu_atombios_encoder_fini_backlight(struct
amdgpu_encoder *amdgpu_encoder)
}
}
-#else /* !CONFIG_BACKLIGHT_CLASS_DEVICE */
-
-void amdgpu_atombios_encoder_init_backlight(struct amdgpu_encoder *encoder)
-{
-}
-
-void amdgpu_atombios_encoder_fini_backlight(struct amdgpu_encoder *encoder)
-{
-}
-
-#endif
-
bool amdgpu_atombios_encoder_is_digital(struct drm_encoder *encoder)
{
struct amdgpu_encoder *amdgpu_encoder = to_amdgpu_encoder(encoder);
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 6def02dec82d..87c1f7190752 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3868,9 +3868,6 @@ static int amdgpu_dm_mode_config_init(struct
amdgpu_device *adev)
#define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
#define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||\
- defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
-
static void amdgpu_dm_update_backlight_caps(struct amdgpu_display_manager *dm,
int bl_idx)
{
@@ -4081,7 +4078,6 @@ amdgpu_dm_reg
[PATCH 2/2] drm/radeon: Drop CONFIG_BACKLIGHT_CLASS_DEVICE ifdefs
The DRM_RADEON Kconfig code contains:
select BACKLIGHT_CLASS_DEVICE
So the condition these ifdefs test for is always true, drop them.
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/radeon/atombios_encoders.c | 14 --
drivers/gpu/drm/radeon/radeon_acpi.c| 2 --
drivers/gpu/drm/radeon/radeon_legacy_encoders.c | 15 ---
drivers/gpu/drm/radeon/radeon_mode.h| 4
4 files changed, 35 deletions(-)
diff --git a/drivers/gpu/drm/radeon/atombios_encoders.c
b/drivers/gpu/drm/radeon/atombios_encoders.c
index f82577dc25e8..160a309e1048 100644
--- a/drivers/gpu/drm/radeon/atombios_encoders.c
+++ b/drivers/gpu/drm/radeon/atombios_encoders.c
@@ -143,8 +143,6 @@ atombios_set_backlight_level(struct radeon_encoder
*radeon_encoder, u8 level)
}
}
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
-
static u8 radeon_atom_bl_level(struct backlight_device *bd)
{
u8 level;
@@ -293,18 +291,6 @@ static void radeon_atom_backlight_exit(struct
radeon_encoder *radeon_encoder)
}
}
-#else /* !CONFIG_BACKLIGHT_CLASS_DEVICE */
-
-void radeon_atom_backlight_init(struct radeon_encoder *encoder)
-{
-}
-
-static void radeon_atom_backlight_exit(struct radeon_encoder *encoder)
-{
-}
-
-#endif
-
static bool radeon_atom_mode_fixup(struct drm_encoder *encoder,
const struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode)
diff --git a/drivers/gpu/drm/radeon/radeon_acpi.c
b/drivers/gpu/drm/radeon/radeon_acpi.c
index 1baef7b493de..b603c0b77075 100644
--- a/drivers/gpu/drm/radeon/radeon_acpi.c
+++ b/drivers/gpu/drm/radeon/radeon_acpi.c
@@ -391,7 +391,6 @@ static int radeon_atif_handler(struct radeon_device *rdev,
radeon_set_backlight_level(rdev, enc,
req.backlight_level);
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
if (rdev->is_atom_bios) {
struct radeon_encoder_atom_dig *dig =
enc->enc_priv;
backlight_force_update(dig->bl_dev,
@@ -401,7 +400,6 @@ static int radeon_atif_handler(struct radeon_device *rdev,
backlight_force_update(dig->bl_dev,
BACKLIGHT_UPDATE_HOTKEY);
}
-#endif
}
}
if (req.pending & ATIF_DGPU_DISPLAY_EVENT) {
diff --git a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
index d2180f5c80fa..1d207c76f53e 100644
--- a/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
+++ b/drivers/gpu/drm/radeon/radeon_legacy_encoders.c
@@ -320,8 +320,6 @@ radeon_legacy_set_backlight_level(struct radeon_encoder
*radeon_encoder, u8 leve
radeon_legacy_lvds_update(&radeon_encoder->base, dpms_mode);
}
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
-
static uint8_t radeon_legacy_lvds_level(struct backlight_device *bd)
{
struct radeon_backlight_privdata *pdata = bl_get_data(bd);
@@ -495,19 +493,6 @@ static void radeon_legacy_backlight_exit(struct
radeon_encoder *radeon_encoder)
}
}
-#else /* !CONFIG_BACKLIGHT_CLASS_DEVICE */
-
-void radeon_legacy_backlight_init(struct radeon_encoder *encoder)
-{
-}
-
-static void radeon_legacy_backlight_exit(struct radeon_encoder *encoder)
-{
-}
-
-#endif
-
-
static void radeon_lvds_enc_destroy(struct drm_encoder *encoder)
{
struct radeon_encoder *radeon_encoder = to_radeon_encoder(encoder);
diff --git a/drivers/gpu/drm/radeon/radeon_mode.h
b/drivers/gpu/drm/radeon/radeon_mode.h
index 3485e7f142e9..b34cffc162e2 100644
--- a/drivers/gpu/drm/radeon/radeon_mode.h
+++ b/drivers/gpu/drm/radeon/radeon_mode.h
@@ -281,15 +281,11 @@ struct radeon_mode_info {
#define RADEON_MAX_BL_LEVEL 0xFF
-#if defined(CONFIG_BACKLIGHT_CLASS_DEVICE) ||
defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE)
-
struct radeon_backlight_privdata {
struct radeon_encoder *encoder;
uint8_t negative;
};
-#endif
-
#define MAX_H_CODE_TIMING_LEN 32
#define MAX_V_CODE_TIMING_LEN 32
--
2.36.0
[PATCH] drm/nouveau/Kconfig: Drop duplicate "select ACPI_VIDEO"
Before this change nouveau's Kconfig section had 2 "select ACPI_VIDEO" statements: select ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT select ACPI_VIDEO if ACPI && X86 Drop the one with the extra conditions, if the conditions for that one are true then the second statement will always select ACPI_VIDEO already. Signed-off-by: Hans de Goede --- drivers/gpu/drm/nouveau/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig index 34760164c271..03d12caf9e26 100644 --- a/drivers/gpu/drm/nouveau/Kconfig +++ b/drivers/gpu/drm/nouveau/Kconfig @@ -11,7 +11,6 @@ config DRM_NOUVEAU select DRM_TTM select DRM_TTM_HELPER select BACKLIGHT_CLASS_DEVICE if DRM_NOUVEAU_BACKLIGHT - select ACPI_VIDEO if ACPI && X86 && BACKLIGHT_CLASS_DEVICE && INPUT select X86_PLATFORM_DEVICES if ACPI && X86 select ACPI_WMI if ACPI && X86 select MXM_WMI if ACPI && X86 -- 2.36.0
Re: [PATCH v3 00/14] Driver of Intel(R) Gaussian & Neural Accelerator
On Mon, Jun 20, 2022 at 11:49:07AM +0200, maciej.kwapulin...@linux.intel.com wrote: > Please share your thoughts. No code here to share thoughts about :(
Re: DRM FB interface does not sanitize len when mmap'ed
Hi Am 20.06.22 um 01:01 schrieb Nuno Gonçalves: On Fri, Jun 17, 2022 at 10:27 AM Thomas Zimmermann wrote: Could you please try the attached patch? It's similar to your solution, but closer to the original intention of the code. Works fine. Tested-by: Nuno Gonçalves Thanks, I'll post the patch to the list. Thanks, Nuno -- 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 v2] drm/bridge: cdns-dsi: Add support for J721E wrapper
Hi, On 19/06/2022 17:01, Rahul T R wrote: Add support for wrapper settings for DSI bridge on j721e. Also set the DPI input to DPI0 Signed-off-by: Rahul T R --- Nack... This wouldn't work with some other SoC using CDNS DSI. See cdns-mhdp8546 for an example of a bit more generic wrapper support. Tomi
Re: [PATCH] drm/i915/gem: remove unused assignments
On Mon, 20 Jun 2022, zys.zlj...@gmail.com wrote:
> From: katrinzhou
>
> The variable ret is reassigned and the value EINVAL is never used.
> Thus, remove the unused assignments.
It's obviously a bug, but it's not obvious just throwing the code away
is the fix. Maybe there's a missing "else" instead.
BR,
Jani.
>
> Addresses-Coverity: ("Unused value")
> Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create
> parameters (v5)")
> Signed-off-by: katrinzhou
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index ab4c5ab28e4d..d5ef5243673a 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -931,8 +931,6 @@ static int set_proto_ctx_param(struct
> drm_i915_file_private *fpriv,
> break;
>
> case I915_CONTEXT_PARAM_PERSISTENCE:
> - if (args->size)
> - ret = -EINVAL;
> ret = proto_context_set_persistence(fpriv->dev_priv, pc,
> args->value);
> break;
--
Jani Nikula, Intel Open Source Graphics Center
Re: [PATCH] drm/i915/gem: remove unused assignments
On 20/06/2022 11:02, zys.zlj...@gmail.com wrote:
From: katrinzhou
The variable ret is reassigned and the value EINVAL is never used.
Thus, remove the unused assignments.
Addresses-Coverity: ("Unused value")
Fixes: d4433c7600f7 ("drm/i915/gem: Use the proto-context to handle create
parameters (v5)")
Signed-off-by: katrinzhou
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index ab4c5ab28e4d..d5ef5243673a 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -931,8 +931,6 @@ static int set_proto_ctx_param(struct drm_i915_file_private
*fpriv,
break;
case I915_CONTEXT_PARAM_PERSISTENCE:
- if (args->size)
- ret = -EINVAL;
ret = proto_context_set_persistence(fpriv->dev_priv, pc,
args->value);
AFAICT fix should end up with code like this:
if (args->size)
ret = -EINVAL;
else
ret = proto_context_set_persistence(...)
break;
Alternatively move args->size checking into
proto_context_set_persistence to align with set_persistence().
Regards,
Tvrtko
break;
Re: [PATCH 05/64] drm/connector: Mention the cleanup after drm_connector_init
Hi Am 10.06.22 um 11:28 schrieb Maxime Ripard: Unlike encoders and CRTCs, the drm_connector_init() and drm_connector_init_with_ddc() don't mention how the cleanup is supposed to be done. Let's add it. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/drm_connector.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c index 353d83ae09d3..2a78a23836d8 100644 --- a/drivers/gpu/drm/drm_connector.c +++ b/drivers/gpu/drm/drm_connector.c @@ -222,6 +222,11 @@ void drm_connector_free_work_fn(struct work_struct *work) * Initialises a preallocated connector. Connectors should be * subclassed as part of driver connector objects. * + * At driver unload time the driver's &drm_connector_funcs.destroy hook + * should call drm_connector_unregister(), drm_connector_cleanup() and + * kfree() the connector structure. The connector structure should not This sounds odd. Maybe '... to release... the connector structure' ? + * be allocated with devm_kzalloc(). + * * Returns: * Zero on success, error code on failure. */ @@ -345,6 +350,11 @@ EXPORT_SYMBOL(drm_connector_init); * Initialises a preallocated connector. Connectors should be * subclassed as part of driver connector objects. * + * At driver unload time the driver's &drm_connector_funcs.destroy hook + * should call drm_connector_unregister(), drm_connector_cleanup() and + * kfree() the connector structure. The connector structure should not + * be allocated with devm_kzalloc(). + * Same here. Best regards Thomas * Ensures that the ddc field of the connector is correctly set. * * Returns: -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 06/64] drm/connector: Introduce drmm_connector_init
Am 10.06.22 um 11:28 schrieb Maxime Ripard:
Unlike other DRM entities, there's no helper to create a DRM-managed
initialisation of a connector.
Let's create an helper to initialise a connector that would be passed as an
argument, and handle the cleanup through a DRM-managed action.
Signed-off-by: Maxime Ripard
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_connector.c | 108 +---
include/drm/drm_connector.h | 4 ++
2 files changed, 90 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 2a78a23836d8..f150270b519f 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -26,6 +26,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -212,28 +213,10 @@ void drm_connector_free_work_fn(struct work_struct *work)
}
}
-/**
- * drm_connector_init - Init a preallocated connector
- * @dev: DRM device
- * @connector: the connector to init
- * @funcs: callbacks for this connector
- * @connector_type: user visible type of the connector
- *
- * Initialises a preallocated connector. Connectors should be
- * subclassed as part of driver connector objects.
- *
- * At driver unload time the driver's &drm_connector_funcs.destroy hook
- * should call drm_connector_unregister(), drm_connector_cleanup() and
- * kfree() the connector structure. The connector structure should not
- * be allocated with devm_kzalloc().
- *
- * Returns:
- * Zero on success, error code on failure.
- */
-int drm_connector_init(struct drm_device *dev,
- struct drm_connector *connector,
- const struct drm_connector_funcs *funcs,
- int connector_type)
+static int __drm_connector_init(struct drm_device *dev,
+ struct drm_connector *connector,
+ const struct drm_connector_funcs *funcs,
+ int connector_type)
{
struct drm_mode_config *config = &dev->mode_config;
int ret;
@@ -337,6 +320,39 @@ int drm_connector_init(struct drm_device *dev,
return ret;
}
+
+/**
+ * drm_connector_init - Init a preallocated connector
+ * @dev: DRM device
+ * @connector: the connector to init
+ * @funcs: callbacks for this connector
+ * @connector_type: user visible type of the connector
+ *
+ * Initialises a preallocated connector. Connectors should be
+ * subclassed as part of driver connector objects.
+ *
+ * At driver unload time the driver's &drm_connector_funcs.destroy hook
+ * should call drm_connector_unregister(), drm_connector_cleanup() and
+ * kfree() the connector structure. The connector structure should not
+ * be allocated with devm_kzalloc().
+ *
+ * Note: consider using drmm_connector_init() instead of
+ * drm_connector_init() to let the DRM managed resource infrastructure
+ * take care of cleanup and deallocation.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_connector_init(struct drm_device *dev,
+ struct drm_connector *connector,
+ const struct drm_connector_funcs *funcs,
+ int connector_type)
+{
+ if (WARN_ON(!(funcs && funcs->destroy)))
+ return -EINVAL;
+
+ return __drm_connector_init(dev, connector, funcs, connector_type);
+}
EXPORT_SYMBOL(drm_connector_init);
/**
@@ -379,6 +395,54 @@ int drm_connector_init_with_ddc(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_connector_init_with_ddc);
+static void drm_connector_cleanup_action(struct drm_device *dev,
+void *ptr)
+{
+ struct drm_connector *connector = ptr;
+
+ drm_connector_unregister(connector);
+ drm_connector_cleanup(connector);
+}
+
+/**
+ * drmm_connector_init - Init a preallocated connector
+ * @dev: DRM device
+ * @connector: the connector to init
+ * @funcs: callbacks for this connector
+ * @connector_type: user visible type of the connector
+ *
+ * Initialises a preallocated connector. Connectors should be
+ * subclassed as part of driver connector objects. Cleanup is
+ * automatically handled through registering drm_connector_unregister()
+ * and drm_connector_cleanup() with drm_add_action(). The connector
+ * structure should be allocated with drmm_kzalloc().
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drmm_connector_init(struct drm_device *dev,
+ struct drm_connector *connector,
+ const struct drm_connector_funcs *funcs,
+ int connector_type)
+{
+ int ret;
+
+ if (WARN_ON(funcs && funcs->destroy))
+ return -EINVAL;
+
+ ret = __drm_connector_init(dev, connector, funcs, connector_type);
+ if (ret)
+ return ret;
+
+ ret = drmm_add_action_or_reset(dev, drm_connector_cleanup_action,
+
Re: [PATCH v3 00/14] Driver of Intel(R) Gaussian & Neural Accelerator
On Mon, Jun 20, 2022 at 12:08:34PM +0200, Maciej Kwapulinski wrote: > > Greg KH writes: > > > On Mon, Jun 20, 2022 at 11:49:07AM +0200, > > maciej.kwapulin...@linux.intel.com wrote: > >> Please share your thoughts. > > > > No code here to share thoughts about :( > > code will be published in comming weeks to dri-devel ML Saying "we will send patches sometime in the future" is a bit odd when we have thousands of patches each week in our inbox to review now. Why not help us out with that workload? :) thanks, greg k-h
Re: [PATCH 08/64] drm/writeback: Introduce drmm_writeback_connector_init
Hi
Am 10.06.22 um 11:28 schrieb Maxime Ripard:
Let's create a DRM-managed variant of drmm_writeback_connector_init that
will take care of an action of the encoder and connector cleanup.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/drm_writeback.c | 136
include/drm/drm_writeback.h | 5 ++
2 files changed, 108 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c
index dccf4504f1bb..0b00921f3379 100644
--- a/drivers/gpu/drm/drm_writeback.c
+++ b/drivers/gpu/drm/drm_writeback.c
@@ -149,32 +149,27 @@ static const struct drm_encoder_funcs
drm_writeback_encoder_funcs = {
.destroy = drm_encoder_cleanup,
};
-/**
- * drm_writeback_connector_init - Initialize a writeback connector and its
properties
- * @dev: DRM device
- * @wb_connector: Writeback connector to initialize
- * @con_funcs: Connector funcs vtable
- * @enc_helper_funcs: Encoder helper funcs vtable to be used by the internal
encoder
- * @formats: Array of supported pixel formats for the writeback engine
- * @n_formats: Length of the formats array
- *
- * This function creates the writeback-connector-specific properties if they
- * have not been already created, initializes the connector as
- * type DRM_MODE_CONNECTOR_WRITEBACK, and correctly initializes the property
- * values. It will also create an internal encoder associated with the
- * drm_writeback_connector and set it to use the @enc_helper_funcs vtable for
- * the encoder helper.
- *
- * Drivers should always use this function instead of drm_connector_init() to
- * set up writeback connectors.
- *
- * Returns: 0 on success, or a negative error code
- */
-int drm_writeback_connector_init(struct drm_device *dev,
-struct drm_writeback_connector *wb_connector,
-const struct drm_connector_funcs *con_funcs,
-const struct drm_encoder_helper_funcs
*enc_helper_funcs,
-const u32 *formats, int n_formats)
+typedef int (*encoder_init_t)(struct drm_device *dev,
+ struct drm_encoder *encoder,
+ const struct drm_encoder_funcs *funcs,
+ int encoder_type,
+ const char *name, ...);
+
+typedef int (*connector_init_t)(struct drm_device *dev,
+ struct drm_connector *connector,
+ const struct drm_connector_funcs *funcs,
+ int connector_type);
This code has menawhile changed in drm-tip, which makes it harder to
give a good review for such refactoring.
But generally, I'd do the connector and encoder initialization in
drmm_writeback_connector_init() and give the initialized encoders to an
internal helper that does the common init steps. That avoids such
indirections with functions pointers.
Best regards
Thomas
+
+static int writeback_init(struct drm_device *dev,
+ struct drm_writeback_connector *wb_connector,
+ connector_init_t conn_init,
+ void (*conn_destroy)(struct drm_connector *connector),
+ encoder_init_t enc_init,
+ void (*enc_destroy)(struct drm_encoder *encoder),
+ const struct drm_connector_funcs *con_funcs,
+ const struct drm_encoder_funcs *enc_funcs,
+ const struct drm_encoder_helper_funcs
*enc_helper_funcs,
+ const u32 *formats, int n_formats)
{
struct drm_property_blob *blob;
struct drm_connector *connector = &wb_connector->base;
@@ -190,16 +185,16 @@ int drm_writeback_connector_init(struct drm_device *dev,
return PTR_ERR(blob);
drm_encoder_helper_add(&wb_connector->encoder, enc_helper_funcs);
- ret = drm_encoder_init(dev, &wb_connector->encoder,
- &drm_writeback_encoder_funcs,
- DRM_MODE_ENCODER_VIRTUAL, NULL);
+ ret = enc_init(dev, &wb_connector->encoder,
+ enc_funcs,
+ DRM_MODE_ENCODER_VIRTUAL, NULL);
if (ret)
goto fail;
connector->interlace_allowed = 0;
- ret = drm_connector_init(dev, connector, con_funcs,
-DRM_MODE_CONNECTOR_WRITEBACK);
+ ret = conn_init(dev, connector, con_funcs,
+ DRM_MODE_CONNECTOR_WRITEBACK);
if (ret)
goto connector_fail;
@@ -231,15 +226,90 @@ int drm_writeback_connector_init(struct drm_device *dev,
return 0;
attach_fail:
- drm_connector_cleanup(connector);
+ if (conn_destroy)
+ conn_destroy(connector);
connector_fail:
- drm_encoder_cleanup(&wb_connector->encoder);
+ if (enc_destroy)
+
Re: [Intel-gfx] [PATCH v2 1/3] drm/doc/rfc: VM_BIND feature design document
Hi, On 17/06/2022 06:14, Niranjana Vishwanathapura wrote: VM_BIND design document with description of intended use cases. v2: Reduce the scope to simple Mesa use case. since I expressed interest please add me to cc when sending out. How come the direction changed to simplify all of a sudden? I did not spot any discussion to that effect. Was it internal talks? Signed-off-by: Niranjana Vishwanathapura --- Documentation/gpu/rfc/i915_vm_bind.rst | 238 + Documentation/gpu/rfc/index.rst| 4 + 2 files changed, 242 insertions(+) create mode 100644 Documentation/gpu/rfc/i915_vm_bind.rst diff --git a/Documentation/gpu/rfc/i915_vm_bind.rst b/Documentation/gpu/rfc/i915_vm_bind.rst new file mode 100644 index ..4ab590ef11fd --- /dev/null +++ b/Documentation/gpu/rfc/i915_vm_bind.rst @@ -0,0 +1,238 @@ +== +I915 VM_BIND feature design and use cases +== + +VM_BIND feature + +DRM_I915_GEM_VM_BIND/UNBIND ioctls allows UMD to bind/unbind GEM buffer +objects (BOs) or sections of a BOs at specified GPU virtual addresses on a +specified address space (VM). These mappings (also referred to as persistent +mappings) will be persistent across multiple GPU submissions (execbuf calls) +issued by the UMD, without user having to provide a list of all required +mappings during each submission (as required by older execbuf mode). + +The VM_BIND/UNBIND calls allow UMDs to request a timeline fence for signaling +the completion of bind/unbind operation. + +VM_BIND feature is advertised to user via I915_PARAM_HAS_VM_BIND. +User has to opt-in for VM_BIND mode of binding for an address space (VM) +during VM creation time via I915_VM_CREATE_FLAGS_USE_VM_BIND extension. + +Normally, vm_bind/unbind operations will get completed synchronously, To me synchronously, at this point in the text, reads as ioctl will return only when the operation is done. Rest of the paragraph however disagrees (plus existence of out fence). It is not clear to me what is the actual behaviour. Will it be clear to userspace developers reading uapi kerneldoc? If it is async, what are the ordering rules in this version? +but if the object is being moved, the binding will happen once that the +moving is complete and out fence will be signaled after binding is complete. +The bind/unbind operation can get completed out of submission order. + +VM_BIND features include: + +* Multiple Virtual Address (VA) mappings can map to the same physical pages + of an object (aliasing). +* VA mapping can map to a partial section of the BO (partial binding). +* Support capture of persistent mappings in the dump upon GPU error. +* TLB is flushed upon unbind completion. Batching of TLB flushes in some + use cases will be helpful. +* Support for userptr gem objects (no special uapi is required for this). + +Execbuf ioctl in VM_BIND mode +--- +A VM in VM_BIND mode will not support older execbuf mode of binding. +The execbuf ioctl handling in VM_BIND mode differs significantly from the +older execbuf2 ioctl (See struct drm_i915_gem_execbuffer2). +Hence, a new execbuf3 ioctl has been added to support VM_BIND mode. (See +struct drm_i915_gem_execbuffer3). The execbuf3 ioctl will not accept any +execlist. Hence, no support for implicit sync. It is expected that the below +work will be able to support requirements of object dependency setting in all +use cases: + +"dma-buf: Add an API for exporting sync files" +(https://lwn.net/Articles/859290/) What does this mean? If execbuf3 does not know about target objects how can we add a meaningful fence? + +The execbuf3 ioctl directly specifies the batch addresses instead of as +object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not +support many of the older features like in/out/submit fences, fence array, +default gem context and many more (See struct drm_i915_gem_execbuffer3). + +In VM_BIND mode, VA allocation is completely managed by the user instead of +the i915 driver. Hence all VA assignment, eviction are not applicable in +VM_BIND mode. Also, for determining object activeness, VM_BIND mode will not +be using the i915_vma active reference tracking. It will instead use dma-resv +object for that (See `VM_BIND dma_resv usage`_). + +So, a lot of existing code supporting execbuf2 ioctl, like relocations, VA +evictions, vma lookup table, implicit sync, vma active reference tracking etc., +are not applicable for execbuf3 ioctl. Hence, all execbuf3 specific handling +should be in a separate file and only functionalities common to these ioctls +can be the shared code where possible. + +VM_PRIVATE objects +--- +By default, BOs can be mapped on multiple VMs and can also be dma-buf +exported. Hence these BOs are referred to as Shared BOs. +During each execbuf submission, the request fence must be added to the +dma-resv fence list
Re: [PATCH 09/64] drm/simple: Introduce drmm_simple_encoder_init
Hi
Am 10.06.22 um 11:28 schrieb Maxime Ripard:
The DRM-managed function to register an encoder is
drmm_simple_encoder_alloc() and its variants, which will allocate the
underlying structure and initialisation the encoder.
However, we might want to separate the structure creation and the encoder
initialisation, for example if the structure is shared across multiple DRM
entities, for example an encoder and a connector.
Let's create an helper to only initialise an encoder that would be passed
as an argument.
There's nothing wrong with this patch, but I have to admit that adding
drm_simple_encoder_init() et al was a mistake. It would have been better
to add an initializer macro like
#define DRM_STATIC_ENCODER_DEFAULT_FUNCS \
.destroy = drm_encoder_cleanup
It's way more flexible and similarly easy to use. Anyway...
Signed-off-by: Maxime Ripard
Acked-by: Thomas Zimmermann
Best regards
Thomas
---
drivers/gpu/drm/drm_simple_kms_helper.c | 46 +++--
include/drm/drm_simple_kms_helper.h | 3 ++
2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 72989ed1baba..876870dd98e5 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -58,9 +58,10 @@ static const struct drm_encoder_funcs
drm_simple_encoder_funcs_cleanup = {
* stored in the device structure. Free the encoder's memory as part of
* the device release function.
*
- * Note: consider using drmm_simple_encoder_alloc() instead of
- * drm_simple_encoder_init() to let the DRM managed resource infrastructure
- * take care of cleanup and deallocation.
+ * Note: consider using drmm_simple_encoder_alloc() or
+ * drmm_simple_encoder_init() instead of drm_simple_encoder_init() to
+ * let the DRM managed resource infrastructure take care of cleanup and
+ * deallocation.
*
* Returns:
* Zero on success, error code on failure.
@@ -75,6 +76,45 @@ int drm_simple_encoder_init(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_simple_encoder_init);
+static void drmm_simple_encoder_cleanup(struct drm_device *dev, void *ptr)
+{
+ struct drm_encoder *encoder = ptr;
+
+ drm_encoder_cleanup(encoder);
+}
+
+/**
+ * drmm_simple_encoder_init - Initialize a preallocated encoder with
+ *basic functionality.
+ * @dev: drm device
+ * @encoder: the encoder to initialize
+ * @encoder_type: user visible type of the encoder
+ *
+ * Initialises a preallocated encoder that has no further functionality.
+ * Settings for possible CRTC and clones are left to their initial
+ * values. The encoder will be cleaned up automatically using a
+ * DRM-managed action.
+ *
+ * The structure containing the encoder's memory should be allocated
+ * using drmm_kzalloc().
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drmm_simple_encoder_init(struct drm_device *dev,
+struct drm_encoder *encoder,
+int encoder_type)
+{
+ int ret;
+
+ ret = drm_encoder_init(dev, encoder, NULL, encoder_type, NULL);
+ if (ret)
+ return ret;
+
+ return drmm_add_action_or_reset(dev, drmm_simple_encoder_cleanup,
encoder);
+}
+EXPORT_SYMBOL(drmm_simple_encoder_init);
+
void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
size_t offset, int encoder_type)
{
diff --git a/include/drm/drm_simple_kms_helper.h
b/include/drm/drm_simple_kms_helper.h
index 0b3647e614dd..20456f4712f0 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -241,6 +241,9 @@ int drm_simple_display_pipe_init(struct drm_device *dev,
int drm_simple_encoder_init(struct drm_device *dev,
struct drm_encoder *encoder,
int encoder_type);
+int drmm_simple_encoder_init(struct drm_device *dev,
+struct drm_encoder *encoder,
+int encoder_type);
void *__drmm_simple_encoder_alloc(struct drm_device *dev, size_t size,
size_t offset, int encoder_type);
--
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 13/64] drm/vc4: hvs: Protect device resources after removal
Am 10.06.22 um 11:28 schrieb Maxime Ripard:
Whenever the device and driver are unbound, the main device and all the
subdevices will be removed by calling their unbind() method.
However, the DRM device itself will only be freed when the last user will
have closed it.
It means that there is a time window where the device and its resources
aren't there anymore, but the userspace can still call into our driver.
Fortunately, the DRM framework provides the drm_dev_enter() and
drm_dev_exit() functions to make sure our underlying device is still there
for the section protected by those calls. Let's add them to the HVS driver.
Signed-off-by: Maxime Ripard
Reviewed-by: Thomas Zimmermann
---
drivers/gpu/drm/vc4/vc4_drv.h | 1 +
drivers/gpu/drm/vc4/vc4_hvs.c | 106 +++---
2 files changed, 99 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index aa4c5910ea05..080deae55f64 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -317,6 +317,7 @@ struct vc4_v3d {
};
struct vc4_hvs {
+ struct drm_device *dev;
struct platform_device *pdev;
void __iomem *regs;
u32 __iomem *dlist;
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 2a58fc421cf6..483053e7b14f 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -25,6 +25,7 @@
#include
#include
+#include
#include
#include "vc4_drv.h"
@@ -66,11 +67,15 @@ static const struct debugfs_reg32 hvs_regs[] = {
void vc4_hvs_dump_state(struct vc4_hvs *hvs)
{
+ struct drm_device *drm = hvs->dev;
struct drm_printer p = drm_info_printer(&hvs->pdev->dev);
- int i;
+ int idx, i;
drm_print_regset32(&p, &hvs->regset);
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
DRM_INFO("HVS ctx:\n");
for (i = 0; i < 64; i += 4) {
DRM_INFO("0x%08x (%s): 0x%08x 0x%08x 0x%08x 0x%08x\n",
@@ -80,6 +85,8 @@ void vc4_hvs_dump_state(struct vc4_hvs *hvs)
readl((u32 __iomem *)hvs->dlist + i + 2),
readl((u32 __iomem *)hvs->dlist + i + 3));
}
+
+ drm_dev_exit(idx);
}
static int vc4_hvs_debugfs_underrun(struct seq_file *m, void *data)
@@ -132,14 +139,18 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs
*hvs,
struct drm_mm_node *space,
const u32 *kernel)
{
- int ret, i;
+ struct drm_device *drm = hvs->dev;
+ int idx, ret, i;
u32 __iomem *dst_kernel;
+ if (!drm_dev_enter(drm, &idx))
+ return -ENODEV;
+
ret = drm_mm_insert_node(&hvs->dlist_mm, space, VC4_KERNEL_DWORDS);
if (ret) {
DRM_ERROR("Failed to allocate space for filter kernel: %d\n",
ret);
- return ret;
+ goto err_dev_exit;
}
dst_kernel = hvs->dlist + space->start;
@@ -153,16 +164,26 @@ static int vc4_hvs_upload_linear_kernel(struct vc4_hvs
*hvs,
}
}
+ drm_dev_exit(idx);
return 0;
+
+err_dev_exit:
+ drm_dev_exit(idx);
+ return ret;
}
static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
struct vc4_crtc *vc4_crtc)
{
+ struct drm_device *drm = hvs->dev;
struct drm_crtc *crtc = &vc4_crtc->base;
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
+ int idx;
u32 i;
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
/* The LUT memory is laid out with each HVS channel in order,
* each of which takes 256 writes for R, 256 for G, then 256
* for B.
@@ -177,6 +198,8 @@ static void vc4_hvs_lut_load(struct vc4_hvs *hvs,
HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_g[i]);
for (i = 0; i < crtc->gamma_size; i++)
HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
+
+ drm_dev_exit(idx);
}
static void vc4_hvs_update_gamma_lut(struct vc4_hvs *hvs,
@@ -198,7 +221,12 @@ static void vc4_hvs_update_gamma_lut(struct vc4_hvs *hvs,
u8 vc4_hvs_get_fifo_frame_count(struct vc4_hvs *hvs, unsigned int fifo)
{
+ struct drm_device *drm = hvs->dev;
u8 field = 0;
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ return 0;
switch (fifo) {
case 0:
@@ -215,6 +243,7 @@ u8 vc4_hvs_get_fifo_frame_count(struct vc4_hvs *hvs,
unsigned int fifo)
break;
}
+ drm_dev_exit(idx);
return field;
}
@@ -226,6 +255,12 @@ int vc4_hvs_get_fifo_from_output(struct vc4_hvs *hvs, unsigned int output)
if (!hvs->hvs5)
return output;
+ /*
+* NOTE: We should probably use drm_dev_enter()/drm_dev_exit()
+* here, but this function is only used
Re: [PATCH 32/64] drm/vc4: dsi: Fix the driver structure lifetime
Am 10.06.22 um 11:28 schrieb Maxime Ripard:
The vc4_dsi structure is currently allocated through a device-managed
allocation. This can lead to use-after-free issues however in the unbinding
path since the DRM entities will stick around, but the underlying structure
has been freed.
However, we can't just fix it by using a DRM-managed allocation like we did
for the other drivers since the DSI case is a bit more intricate.
Indeed, the structure will be allocated at probe time, when we don't have a
DRM device yet, to be able to register the DSI bus driver. We will then
reuse it at bind time to register our KMS entities in the framework.
In order to work around both constraints, we can use a kref to track the
users of the structure (DSI host, and KMS), and then put our structure when
the DSI host will have been unregistered, and through a DRM-managed action
that will execute once we won't need the KMS entities anymore.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_dsi.c | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 10533a2a41b3..282537f27b8e 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -510,6 +510,8 @@ struct vc4_dsi {
struct vc4_encoder encoder;
struct mipi_dsi_host dsi_host;
+ struct kref kref;
+
struct platform_device *pdev;
struct drm_bridge *bridge;
@@ -1479,6 +1481,15 @@ vc4_dsi_init_phy_clocks(struct vc4_dsi *dsi)
dsi->clk_onecell);
}
+static void vc4_dsi_release(struct kref *kref);
+
+static void vc4_dsi_put(struct drm_device *drm, void *ptr)
+{
+ struct vc4_dsi *dsi = ptr;
+
+ kref_put(&dsi->kref, &vc4_dsi_release);
+}
+
static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -1488,6 +1499,12 @@ static int vc4_dsi_bind(struct device *dev, struct
device *master, void *data)
dma_cap_mask_t dma_mask;
int ret;
+ kref_get(&dsi->kref);
+
+ ret = drmm_add_action_or_reset(drm, vc4_dsi_put, dsi);
+ if (ret)
+ return ret;
+
dsi->variant = of_device_get_match_data(dev);
INIT_LIST_HEAD(&dsi->bridge_chain);
@@ -1642,16 +1659,25 @@ static const struct component_ops vc4_dsi_ops = {
.unbind = vc4_dsi_unbind,
};
+static void vc4_dsi_release(struct kref *kref)
+{
+ struct vc4_dsi *dsi =
+ container_of(kref, struct vc4_dsi, kref);
+
+ kfree(dsi);
+}
+
static int vc4_dsi_dev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct vc4_dsi *dsi;
- dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
+ dsi = kzalloc(sizeof(*dsi), GFP_KERNEL);
if (!dsi)
return -ENOMEM;
dev_set_drvdata(dev, dsi);
+ kref_init(&dsi->kref);
dsi->pdev = pdev;
dsi->dsi_host.ops = &vc4_dsi_host_ops;
dsi->dsi_host.dev = dev;
@@ -1666,6 +1692,7 @@ static int vc4_dsi_dev_remove(struct platform_device
*pdev)
struct vc4_dsi *dsi = dev_get_drvdata(dev);
mipi_dsi_host_unregister(&dsi->dsi_host);
+ kref_put(&dsi->kref, &vc4_dsi_release);
Maybe vc4_dsi_put() ?
return 0;
}
--
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 32/64] drm/vc4: dsi: Fix the driver structure lifetime
Am 20.06.22 um 12:55 schrieb Thomas Zimmermann:
Am 10.06.22 um 11:28 schrieb Maxime Ripard:
The vc4_dsi structure is currently allocated through a device-managed
allocation. This can lead to use-after-free issues however in the
unbinding
path since the DRM entities will stick around, but the underlying
structure
has been freed.
However, we can't just fix it by using a DRM-managed allocation like
we did
for the other drivers since the DSI case is a bit more intricate.
Indeed, the structure will be allocated at probe time, when we don't
have a
DRM device yet, to be able to register the DSI bus driver. We will then
reuse it at bind time to register our KMS entities in the framework.
In order to work around both constraints, we can use a kref to track the
users of the structure (DSI host, and KMS), and then put our structure
when
the DSI host will have been unregistered, and through a DRM-managed
action
that will execute once we won't need the KMS entities anymore.
Signed-off-by: Maxime Ripard
---
drivers/gpu/drm/vc4/vc4_dsi.c | 29 -
1 file changed, 28 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c
b/drivers/gpu/drm/vc4/vc4_dsi.c
index 10533a2a41b3..282537f27b8e 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -510,6 +510,8 @@ struct vc4_dsi {
struct vc4_encoder encoder;
struct mipi_dsi_host dsi_host;
+ struct kref kref;
+
struct platform_device *pdev;
struct drm_bridge *bridge;
@@ -1479,6 +1481,15 @@ vc4_dsi_init_phy_clocks(struct vc4_dsi *dsi)
dsi->clk_onecell);
}
+static void vc4_dsi_release(struct kref *kref);
+
+static void vc4_dsi_put(struct drm_device *drm, void *ptr)
+{
+ struct vc4_dsi *dsi = ptr;
+
+ kref_put(&dsi->kref, &vc4_dsi_release);
+}
+
static int vc4_dsi_bind(struct device *dev, struct device *master,
void *data)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -1488,6 +1499,12 @@ static int vc4_dsi_bind(struct device *dev,
struct device *master, void *data)
dma_cap_mask_t dma_mask;
int ret;
+ kref_get(&dsi->kref);
+
+ ret = drmm_add_action_or_reset(drm, vc4_dsi_put, dsi);
+ if (ret)
+ return ret;
+
dsi->variant = of_device_get_match_data(dev);
INIT_LIST_HEAD(&dsi->bridge_chain);
@@ -1642,16 +1659,25 @@ static const struct component_ops vc4_dsi_ops = {
.unbind = vc4_dsi_unbind,
};
+static void vc4_dsi_release(struct kref *kref)
+{
+ struct vc4_dsi *dsi =
+ container_of(kref, struct vc4_dsi, kref);
+
+ kfree(dsi);
+}
+
static int vc4_dsi_dev_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct vc4_dsi *dsi;
- dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
+ dsi = kzalloc(sizeof(*dsi), GFP_KERNEL);
if (!dsi)
return -ENOMEM;
dev_set_drvdata(dev, dsi);
+ kref_init(&dsi->kref);
dsi->pdev = pdev;
dsi->dsi_host.ops = &vc4_dsi_host_ops;
dsi->dsi_host.dev = dev;
@@ -1666,6 +1692,7 @@ static int vc4_dsi_dev_remove(struct
platform_device *pdev)
struct vc4_dsi *dsi = dev_get_drvdata(dev);
mipi_dsi_host_unregister(&dsi->dsi_host);
+ kref_put(&dsi->kref, &vc4_dsi_release);
Maybe vc4_dsi_put() ?
No, wait. That's the release function.
It's confusing. I'd rename vc4_dsi_put() to vc4_dsi_release_action() and
wrap those kref_get/kref_put calls in small helpers named
vc4_dsi_get/vc4_dsi_put. That's more aligned to the usual naming
conventions, I'd say.
Best regards
Thomas
return 0;
}
--
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 34/64] drm/vc4: hdmi: Switch to drmm_kzalloc
Am 10.06.22 um 11:28 schrieb Maxime Ripard: Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc. This means that this will eventually be freed whenever the device is removed. In our case, the most like source of removal is that the main 'most likely source' device is going to be unbound, and component_unbind_all() is being run. However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free. Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore. Signed-off-by: Maxime Ripard --- drivers/gpu/drm/vc4/vc4_hdmi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 6aadb65eb640..eb8ff7b258d1 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -2833,9 +2833,10 @@ static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data) struct device_node *ddc_node; int ret; - vc4_hdmi = devm_kzalloc(dev, sizeof(*vc4_hdmi), GFP_KERNEL); + vc4_hdmi = drmm_kzalloc(drm, sizeof(*vc4_hdmi), GFP_KERNEL); if (!vc4_hdmi) return -ENOMEM; + mutex_init(&vc4_hdmi->mutex); spin_lock_init(&vc4_hdmi->hw_lock); INIT_DELAYED_WORK(&vc4_hdmi->scrambling_work, vc4_hdmi_scrambling_wq); -- 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 43/64] drm/vc4: hdmi: Protect device resources after removal
Am 10.06.22 um 11:29 schrieb Maxime Ripard:
Whenever the device and driver are unbound, the main device and all the
subdevices will be removed by calling their unbind() method.
However, the DRM device itself will only be freed when the last user will
have closed it.
It means that there is a time window where the device and its resources
aren't there anymore, but the userspace can still call into our driver.
Fortunately, the DRM framework provides the drm_dev_enter() and
drm_dev_exit() functions to make sure our underlying device is still there
for the section protected by those calls. Let's add them to the HDMI driver.
Signed-off-by: Maxime Ripard
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/vc4/vc4_hdmi.c | 286 +++--
1 file changed, 269 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 814517c1fdaa..b4fd581861ea 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -140,17 +141,33 @@ static int vc4_hdmi_debugfs_regs(struct seq_file *m, void
*unused)
{
struct drm_info_node *node = (struct drm_info_node *)m->private;
struct vc4_hdmi *vc4_hdmi = node->info_ent->data;
+ struct drm_device *drm = vc4_hdmi->connector.dev;
struct drm_printer p = drm_seq_file_printer(m);
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ return -ENODEV;
drm_print_regset32(&p, &vc4_hdmi->hdmi_regset);
drm_print_regset32(&p, &vc4_hdmi->hd_regset);
+ drm_dev_exit(idx);
+
return 0;
}
static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
{
+ struct drm_device *drm = vc4_hdmi->connector.dev;
unsigned long flags;
+ int idx;
+
+ /*
+* We can be called by our bind callback, when the
+* connector->dev pointer might not be initialised yet.
+*/
+ if (drm && !drm_dev_enter(drm, &idx))
+ return;
spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
@@ -167,11 +184,23 @@ static void vc4_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
HDMI_WRITE(HDMI_SW_RESET_CONTROL, 0);
spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
+
+ if (drm)
+ drm_dev_exit(idx);
}
static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
{
+ struct drm_device *drm = vc4_hdmi->connector.dev;
unsigned long flags;
+ int idx;
+
+ /*
+* We can be called by our bind callback, when the
+* connector->dev pointer might not be initialised yet.
+*/
+ if (drm && !drm_dev_enter(drm, &idx))
+ return;
reset_control_reset(vc4_hdmi->reset);
@@ -183,15 +212,25 @@ static void vc5_hdmi_reset(struct vc4_hdmi *vc4_hdmi)
HDMI_READ(HDMI_CLOCK_STOP) | VC4_DVP_HT_CLOCK_STOP_PIXEL);
spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
+
+ if (drm)
+ drm_dev_exit(idx);
}
#ifdef CONFIG_DRM_VC4_HDMI_CEC
static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
{
- unsigned long cec_rate = clk_get_rate(vc4_hdmi->cec_clock);
+ struct drm_device *drm = vc4_hdmi->connector.dev;
+ unsigned long cec_rate;
unsigned long flags;
u16 clk_cnt;
u32 value;
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ return;
+
+ cec_rate = clk_get_rate(vc4_hdmi->cec_clock);
spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
@@ -207,6 +246,8 @@ static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi)
HDMI_WRITE(HDMI_CEC_CNTRL_1, value);
spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
+
+ drm_dev_exit(idx);
}
#else
static void vc4_hdmi_cec_update_clk_div(struct vc4_hdmi *vc4_hdmi) {}
@@ -427,25 +468,34 @@ static int vc4_hdmi_stop_packet(struct drm_encoder
*encoder,
bool poll)
{
struct vc4_hdmi *vc4_hdmi = encoder_to_vc4_hdmi(encoder);
+ struct drm_device *drm = vc4_hdmi->connector.dev;
u32 packet_id = type - 0x80;
unsigned long flags;
+ int ret = 0;
+ int idx;
+
+ if (!drm_dev_enter(drm, &idx))
+ return -ENODEV;
spin_lock_irqsave(&vc4_hdmi->hw_lock, flags);
HDMI_WRITE(HDMI_RAM_PACKET_CONFIG,
HDMI_READ(HDMI_RAM_PACKET_CONFIG) & ~BIT(packet_id));
spin_unlock_irqrestore(&vc4_hdmi->hw_lock, flags);
- if (!poll)
- return 0;
+ if (poll) {
+ ret = wait_for(!(HDMI_READ(HDMI_RAM_PACKET_STATUS) &
+BIT(packet_id)), 100);
+ }
- return wait_for(!(HDMI_READ(HDMI_RAM_PACKET_STATUS) &
- BIT(packet_id)), 100);
+ drm_dev_exit(idx);
+ return ret;
}
static void vc4_hdmi_write_infoframe(struct drm_en
Re: [PATCH 45/64] drm/vc4: txp: Remove vc4_dev txp pointer
Am 10.06.22 um 11:29 schrieb Maxime Ripard:
There's no user for that pointer so let's just get rid of it.
Signed-off-by: Maxime Ripard
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/vc4/vc4_drv.h | 1 -
drivers/gpu/drm/vc4/vc4_txp.c | 6 --
2 files changed, 7 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 846f3cda179a..5f2d7640a09d 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -80,7 +80,6 @@ struct vc4_dev {
struct vc4_hvs *hvs;
struct vc4_v3d *v3d;
struct vc4_vec *vec;
- struct vc4_txp *txp;
struct vc4_hang_state *hang_state;
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index f306e05ac5b2..87c896f482fb 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -468,7 +468,6 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
{
struct platform_device *pdev = to_platform_device(dev);
struct drm_device *drm = dev_get_drvdata(master);
- struct vc4_dev *vc4 = to_vc4_dev(drm);
struct vc4_crtc *vc4_crtc;
struct vc4_txp *txp;
struct drm_crtc *crtc;
@@ -521,7 +520,6 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
return ret;
dev_set_drvdata(dev, txp);
- vc4->txp = txp;
vc4_debugfs_add_regset32(drm, "txp_regs", &txp->regset);
@@ -531,13 +529,9 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data)
static void vc4_txp_unbind(struct device *dev, struct device *master,
void *data)
{
- struct drm_device *drm = dev_get_drvdata(master);
- struct vc4_dev *vc4 = to_vc4_dev(drm);
struct vc4_txp *txp = dev_get_drvdata(dev);
vc4_txp_connector_destroy(&txp->connector.base);
-
- vc4->txp = NULL;
}
static const struct component_ops vc4_txp_ops = {
--
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 46/64] drm/vc4: txp: Remove duplicate regset
Am 10.06.22 um 11:29 schrieb Maxime Ripard:
There's already a regset in the vc4_crtc structure so there's no need to
duplicate it in vc4_txp.
Signed-off-by: Maxime Ripard
Acked-by: Thomas Zimmermann
---
drivers/gpu/drm/vc4/vc4_txp.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c
index 87c896f482fb..51ac01838093 100644
--- a/drivers/gpu/drm/vc4/vc4_txp.c
+++ b/drivers/gpu/drm/vc4/vc4_txp.c
@@ -154,7 +154,6 @@ struct vc4_txp {
struct drm_writeback_connector connector;
void __iomem *regs;
- struct debugfs_regset32 regset;
};
static inline struct vc4_txp *encoder_to_vc4_txp(struct drm_encoder *encoder)
@@ -493,9 +492,9 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
txp->regs = vc4_ioremap_regs(pdev, 0);
if (IS_ERR(txp->regs))
return PTR_ERR(txp->regs);
- txp->regset.base = txp->regs;
- txp->regset.regs = txp_regs;
- txp->regset.nregs = ARRAY_SIZE(txp_regs);
+ vc4_crtc->regset.base = txp->regs;
+ vc4_crtc->regset.regs = txp_regs;
+ vc4_crtc->regset.nregs = ARRAY_SIZE(txp_regs);
drm_connector_helper_add(&txp->connector.base,
&vc4_txp_connector_helper_funcs);
@@ -521,7 +520,7 @@ static int vc4_txp_bind(struct device *dev, struct device
*master, void *data)
dev_set_drvdata(dev, txp);
- vc4_debugfs_add_regset32(drm, "txp_regs", &txp->regset);
+ vc4_debugfs_add_regset32(drm, "txp_regs", &vc4_crtc->regset);
return 0;
}
--
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 47/64] drm/vc4: txp: Switch to drmm_kzalloc
Am 10.06.22 um 11:29 schrieb Maxime Ripard: Our internal structure that stores the DRM entities structure is allocated through a device-managed kzalloc. This means that this will eventually be freed whenever the device is removed. In our case, the most like source of removal is that the main device is going to be unbound, and component_unbind_all() is being run. However, it occurs while the DRM device is still registered, which will create dangling pointers, eventually resulting in use-after-free. Switch to a DRM-managed allocation to keep our structure until the DRM driver doesn't need it anymore. Signed-off-by: Maxime Ripard Acked-by: Thomas Zimmermann --- drivers/gpu/drm/vc4/vc4_txp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_txp.c b/drivers/gpu/drm/vc4/vc4_txp.c index 51ac01838093..6a16b2798724 100644 --- a/drivers/gpu/drm/vc4/vc4_txp.c +++ b/drivers/gpu/drm/vc4/vc4_txp.c @@ -477,7 +477,7 @@ static int vc4_txp_bind(struct device *dev, struct device *master, void *data) if (irq < 0) return irq; - txp = devm_kzalloc(dev, sizeof(*txp), GFP_KERNEL); + txp = drmm_kzalloc(drm, sizeof(*txp), GFP_KERNEL); if (!txp) return -ENOMEM; vc4_crtc = &txp->base; -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH] drm/i915/display: Re-add check for low voltage sku for max dp source rate
Hi Jani, Do you plan to merge this revert? Thanks, Jason
Re: [PATCH v2 0/2] Improve vfio-pci primary GPU assignment behavior
On 06/16/22 22:38, Alex Williamson wrote: > When assigning a primary graphics device to VM through vfio-pci device > assignment, users often prevent binding of the native PCI graphics > driver to avoid device initialization conflicts, however firmware > console drivers may still be attached to the device which can often be > cumbersome to manually unbind or exclude via cmdline options. > > This series proposes to move the DRM aperture helpers out to > drivers/video/ to make it more accessible to drivers like vfio-pci, > which have neither dependencies on DRM code nor a struct drm_driver > to present to existing interfaces. vfio-pci can then trivially call > into the aperture helpers to remove conflicting drivers, rather than > open coding it ourselves as was proposed with a new symbol export in > v1 of this series[1]. > > Thanks to Thomas for splitting out the aperture code with new > documentation. > > Thomas had proposed this going through the vfio tree with appropriate > stakeholder acks, that's fine with me, but I'm also open to it going > through the DRM tree given that the vfio-pci-core change is even more > trivial now and the bulk of the changes are DRM/video paths. Thanks, > > Alex > > [1]https://lore.kernel.org/all/165453797543.3592816.6381793341352595461.stgit@omen/ > > --- > > Alex Williamson (1): > vfio/pci: Remove console drivers > > Thomas Zimmermann (1): > drm: Implement DRM aperture helpers under video/ > > > Documentation/driver-api/aperture.rst | 13 + > Documentation/driver-api/index.rst| 1 + > drivers/gpu/drm/drm_aperture.c| 174 + > drivers/gpu/drm/tiny/Kconfig | 1 + > drivers/vfio/pci/vfio_pci_core.c | 5 + > drivers/video/Kconfig | 6 + > drivers/video/Makefile| 2 + > drivers/video/aperture.c | 340 ++ > drivers/video/console/Kconfig | 1 + > drivers/video/fbdev/Kconfig | 7 +- > include/linux/aperture.h | 56 + > 11 files changed, 440 insertions(+), 166 deletions(-) > create mode 100644 Documentation/driver-api/aperture.rst > create mode 100644 drivers/video/aperture.c > create mode 100644 include/linux/aperture.h > series Tested-by: Laszlo Ersek (on top of Fedora's 5.18.5-100.fc35.x86_64) Thanks, Laszlo
[Bug 201957] amdgpu: ring gfx timeout
https://bugzilla.kernel.org/show_bug.cgi?id=201957 --- Comment #72 from Martin von Wittich (martin.von.witt...@iserv.eu) --- I can confirm that adding "amdgpu.dpm=0" to the kernel command line seems to resolve this issue - I enabled that option on 2022-06-12 13:24, and my system didn't crash at all on 2022-06-12 - 2022-06-14 (I was on vacation from 2022-06-15 on and didn't use my computer from then on). I don't use Linux for gaming and therefore can't comment how badly this affects gaming performance, but I did notice mpv could no longer play 1080p x264 video without stuttering when it defaults to --vo=gpu. Using another --vo like sdl seems to be a viable workaround. > Did you try with the latest Linux Kernel? I had a lot of gpu lockups like > this. Also try these kernel parameters : "amdgpu.ppfeaturemask=0xbffb > amdgpu.noretry=0 amdgpu.lockup_timeout=0 amdgpu.gpu_recovery=1 amdgpu.audio=0 > amdgpu.deep_color=1 amd_iommu=on iommu=pt"" ( you might also try with > amdgpu.ppfeaturemask=0xfffd7fff or amdgpu.ppfeaturemask=0x ) I'll try these next. -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[Bug 201957] amdgpu: ring gfx timeout
https://bugzilla.kernel.org/show_bug.cgi?id=201957 --- Comment #73 from Martin von Wittich (martin.von.witt...@iserv.eu) --- Sorry, forgot to mention in my last post and now can't edit: interestingly enough, the attached video "5 second video clip that triggers a crash" still successfully triggers the crash. Seems to me like the root issue isn't actually in the dynamic power management code, but somewhere else, and the DPM is just one of several things that can trigger it? -- 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 05/64] drm/connector: Mention the cleanup after drm_connector_init
Hi Thomas, Thanks for your review On Mon, Jun 20, 2022 at 12:21:33PM +0200, Thomas Zimmermann wrote: > Am 10.06.22 um 11:28 schrieb Maxime Ripard: > > Unlike encoders and CRTCs, the drm_connector_init() and > > drm_connector_init_with_ddc() don't mention how the cleanup is supposed to > > be done. Let's add it. > > > > Signed-off-by: Maxime Ripard > > --- > > drivers/gpu/drm/drm_connector.c | 10 ++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_connector.c > > b/drivers/gpu/drm/drm_connector.c > > index 353d83ae09d3..2a78a23836d8 100644 > > --- a/drivers/gpu/drm/drm_connector.c > > +++ b/drivers/gpu/drm/drm_connector.c > > @@ -222,6 +222,11 @@ void drm_connector_free_work_fn(struct work_struct > > *work) > >* Initialises a preallocated connector. Connectors should be > >* subclassed as part of driver connector objects. > >* > > + * At driver unload time the driver's &drm_connector_funcs.destroy hook > > + * should call drm_connector_unregister(), drm_connector_cleanup() and > > + * kfree() the connector structure. The connector structure should not > > This sounds odd. Maybe > > '... to release... the connector structure' ? I didn't really get what your suggestion was exactly Is it > At driver unload time the driver's &drm_connector_funcs.destroy hook > should call drm_connector_unregister(), drm_connector_cleanup() and > kfree() to release the connector structure. Or something else? Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH 25/64] drm/vc4: dpi: Add action to disable the clock
On Thu, Jun 16, 2022 at 10:47:56AM +0100, Dave Stevenson wrote: > On Thu, 16 Jun 2022 at 09:38, Maxime Ripard wrote: > > > > Hi Dave, > > > > On Tue, Jun 14, 2022 at 05:47:28PM +0100, Dave Stevenson wrote: > > > Hi Maxime. > > > > > > On Fri, 10 Jun 2022 at 10:30, Maxime Ripard wrote: > > > > > > > > Adding a device-managed action will make the error path easier, so let's > > > > create one to disable our clock. > > > > > > The DPI block has two clocks (core and pixel), and this is only > > > affecting one of them (the core clock). > > > > Thanks for the suggestion, I've amended the commit message. > > > > > (I'm actually puzzling over what it's wanting to do with the core > > > clock here as it's only enabling it rather than setting a rate. I > > > think it may be redundant). > > > > Could it be that it a "bus" clock that we need it to access the > > registers? > > No idea. Normally it's the power domain that is required to access > registers. For HDMI at least, the power domain being off will make a read return some bogus value, but the core clock being off will just make the CPU stall. I can only assume it would be the same for the DPI controller? > AFAIK the core clock is never turned off, so the request for it is > just a little odd. Even if the clock driver never shuts it off, I think it still has value to follow the Clock Framework conventions. We might have a different clock policy in the future, and it would throw people reading the DPI driver off. Maxime signature.asc Description: PGP signature
Re: [PATCH v5 01/13] mm: add zone device coherent type memory support
On Mon, Jun 20, 2022 at 11:50 AM Alistair Popple wrote:
>
>
> Oded Gabbay writes:
>
> > On Mon, Jun 20, 2022 at 3:33 AM Alistair Popple wrote:
> >>
> >>
> >> Oded Gabbay writes:
> >>
> >> > On Fri, Jun 17, 2022 at 8:20 PM Sierra Guiza, Alejandro (Alex)
> >> > wrote:
> >> >>
> >> >>
> >> >> On 6/17/2022 4:40 AM, David Hildenbrand wrote:
> >> >> > On 31.05.22 22:00, Alex Sierra wrote:
> >> >> >> Device memory that is cache coherent from device and CPU point of
> >> >> >> view.
> >> >> >> This is used on platforms that have an advanced system bus (like CAPI
> >> >> >> or CXL). Any page of a process can be migrated to such memory.
> >> >> >> However,
> >> >> >> no one should be allowed to pin such memory so that it can always be
> >> >> >> evicted.
> >> >> >>
> >> >> >> Signed-off-by: Alex Sierra
> >> >> >> Acked-by: Felix Kuehling
> >> >> >> Reviewed-by: Alistair Popple
> >> >> >> [hch: rebased ontop of the refcount changes,
> >> >> >>removed is_dev_private_or_coherent_page]
> >> >> >> Signed-off-by: Christoph Hellwig
> >> >> >> ---
> >> >> >> include/linux/memremap.h | 19 +++
> >> >> >> mm/memcontrol.c | 7 ---
> >> >> >> mm/memory-failure.c | 8 ++--
> >> >> >> mm/memremap.c| 10 ++
> >> >> >> mm/migrate_device.c | 16 +++-
> >> >> >> mm/rmap.c| 5 +++--
> >> >> >> 6 files changed, 49 insertions(+), 16 deletions(-)
> >> >> >>
> >> >> >> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> >> >> >> index 8af304f6b504..9f752ebed613 100644
> >> >> >> --- a/include/linux/memremap.h
> >> >> >> +++ b/include/linux/memremap.h
> >> >> >> @@ -41,6 +41,13 @@ struct vmem_altmap {
> >> >> >>* A more complete discussion of unaddressable memory may be found
> >> >> >> in
> >> >> >>* include/linux/hmm.h and Documentation/vm/hmm.rst.
> >> >> >>*
> >> >> >> + * MEMORY_DEVICE_COHERENT:
> >> >> >> + * Device memory that is cache coherent from device and CPU point
> >> >> >> of view. This
> >> >> >> + * is used on platforms that have an advanced system bus (like CAPI
> >> >> >> or CXL). A
> >> >> >> + * driver can hotplug the device memory using ZONE_DEVICE and with
> >> >> >> that memory
> >> >> >> + * type. Any page of a process can be migrated to such memory.
> >> >> >> However no one
> >> >> > Any page might not be right, I'm pretty sure. ... just thinking about
> >> >> > special pages
> >> >> > like vdso, shared zeropage, ... pinned pages ...
> >> >>
> >> >> Hi David,
> >> >>
> >> >> Yes, I think you're right. This type does not cover all special pages.
> >> >> I need to correct that on the cover letter.
> >> >> Pinned pages are allowed as long as they're not long term pinned.
> >> >>
> >> >> Regards,
> >> >> Alex Sierra
> >> >
> >> > What if I want to hotplug this device's coherent memory, but I do
> >> > *not* want the OS
> >> > to migrate any page to it ?
> >> > I want to fully-control what resides on this memory, as I can consider
> >> > this memory
> >> > "expensive". i.e. I don't have a lot of it, I want to use it for
> >> > specific purposes and
> >> > I don't want the OS to start using it when there is some memory pressure
> >> > in
> >> > the system.
> >>
> >> This is exactly what MEMORY_DEVICE_COHERENT is for. Device coherent
> >> pages are only allocated by a device driver and exposed to user-space by
> >> a driver migrating pages to them with migrate_vma. The OS can't just
> >> start using them due to memory pressure for example.
> >>
> >> - Alistair
> > Thanks for the explanation.
> >
> > I guess the commit message confused me a bit, especially these two
> > sentences:
> >
> > "Any page of a process can be migrated to such memory. However no one
> > should be
> > allowed to pin such memory so that it can always be evicted."
> >
> > I read them as if the OS is free to choose which pages are migrated to
> > this memory,
> > and anything is eligible for migration to that memory (and that's why
> > we also don't
> > allow it to pin memory there).
> >
> > If we are not allowed to pin anything there, can the device driver
> > decide to disable
> > any option for oversubscription of this memory area ?
>
> I'm not sure I follow your thinking on how oversubscription would work
> here, however all allocations are controlled by the driver. So if a
> device's coherent memory is full a driver would be unable to migrate
> pages to that device until pages are freed by the OS due to being
> unmapped or the driver evicts pages by migrating them back to normal CPU
> memory.
>
> Pinning of pages is allowed, and could prevent such migrations. However
> this patch series prevents device coherent pages from being pinned
> longterm (ie. with FOLL_LONGTERM), so it should always be able to evict
> pages eventually.
>
> > Let's assume the user uses this memory area for doing p2p with other
> > CXL devices.
> > In that case, I wouldn't want the driver/OS to migrate pages in and
>
Re: [PATCH v12 01/14] dt-bindings: mediatek,dpi: Add DP_INTF compatible
On Mon, Jun 20, 2022 at 08:10:15PM +0800, Bo-Chen Chen wrote: > From: Markus Schneider-Pargmann > > DP_INTF is similar to DPI but does not have the exact same feature set > or register layouts. > > DP_INTF is the sink of the display pipeline that is connected to the > DisplayPort controller and encoder unit. It takes the same clocks as > DPI. > > In this patch, we also do these string replacement: > - s/mediatek/MediaTek/ in title. > - s/Mediatek/MediaTek/ in description. > > Signed-off-by: Markus Schneider-Pargmann > Signed-off-by: Guillaume Ranquet > [Bo-Chen: Modify reviewers' comments.] > Signed-off-by: Bo-Chen Chen > --- > .../bindings/display/mediatek/mediatek,dpi.yaml | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git > a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml > b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml > index 77ee1b923991..d72f74632038 100644 > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml > @@ -4,16 +4,16 @@ > $id: http://devicetree.org/schemas/display/mediatek/mediatek,dpi.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: mediatek DPI Controller Device Tree Bindings > +title: MediaTek DPI and DP_INTF Controller > > maintainers: >- CK Hu >- Jitao shi > > description: | > - The Mediatek DPI function block is a sink of the display subsystem and > - provides 8-bit RGB/YUV444 or 8/10/10-bit YUV422 pixel data on a parallel > - output bus. > + The MediaTek DPI and DP_INTF function blocks are a sink of the display > + subsystem and provides 8-bit RGB/YUV444 or 8/10/10-bit YUV422 pixel data > on a > + parallel output bus. > > properties: >compatible: > @@ -24,6 +24,7 @@ properties: >- mediatek,mt8183-dpi >- mediatek,mt8186-dpi >- mediatek,mt8192-dpi > + - mediatek,mt8195-dp_intf Underscores are frowned upon in the compatibles. See Section 2.3.1 of the device tree spec: > The compatible string should consist only of lowercase letters, digits > and dashes, and should start with a letter. A single comma is > typically only used following a vendor prefix. Underscores should not > be used. Maxime signature.asc Description: PGP signature
Re: [Intel-gfx] [PATCH] drm/i915: Fix vm use-after-free in vma destruction
Hi, Andi
On 5/19/22 23:46, Andi Shyti wrote:
Hi Thomas,
On Thu, May 12, 2022 at 11:40:45AM +0200, Thomas Hellström wrote:
In vma destruction, the following race may occur:
Thread 1: Thread 2:
i915_vma_destroy();
...
list_del_init(vma->vm_link);
...
mutex_unlock(vma->vm->mutex);
__i915_vm_release();
release_references();
And in release_reference() we dereference vma->vm to get to the
vm gt pointer, leadin go a use-after free.
leading to
Thanks, will fix.
[...]
-static void release_references(struct i915_vma *vma, bool vm_ddestroy)
+static void release_references(struct i915_vma *vma, struct intel_gt *gt,
+ bool vm_ddestroy)
{
struct drm_i915_gem_object *obj = vma->obj;
- struct intel_gt *gt = vma->vm->gt;
GEM_BUG_ON(i915_vma_is_active(vma));
but then we have
if (vm_ddestroy)
i915_vm_resv_put(vma->vm);
were we reference to a freed vm, right? Do we need to check it
here, as well?
No, it's not needed, since if vm_ddestroy is true, we keep the vm alive
using the vm
resv_ref until i915_vm_resv_put(). This is for the rare occasions where,
during vm destruction, we fail to grab an object reference and therefore
vma destruction is left for the object destructor. In those cases the
vma needs to keep the vm in memory for it to be able to grab the vm mutex.
/Thomas
Andi
Re: [PATCH] fbdev: Fix syntax errors in comments
On 6/5/22 10:59, Xiang wangx wrote:
> Delete the redundant word 'its'.
>
> Signed-off-by: Xiang wangx
applied to fbdev tree.
Thanks!
Helge
> ---
> drivers/video/fbdev/skeletonfb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/video/fbdev/skeletonfb.c
> b/drivers/video/fbdev/skeletonfb.c
> index bcacfb6934fa..3d4d78362ede 100644
> --- a/drivers/video/fbdev/skeletonfb.c
> +++ b/drivers/video/fbdev/skeletonfb.c
> @@ -96,7 +96,7 @@ static const struct fb_fix_screeninfo xxxfb_fix = {
>
> /*
> * Modern graphical hardware not only supports pipelines but some
> - * also support multiple monitors where each display can have its
> + * also support multiple monitors where each display can have
> * its own unique data. In this case each display could be
> * represented by a separate framebuffer device thus a separate
> * struct fb_info. Now the struct xxx_par represents the graphics
[PATCH v2] drm/i915: Fix vm use-after-free in vma destruction
In vma destruction, the following race may occur:
Thread 1: Thread 2:
i915_vma_destroy();
...
list_del_init(vma->vm_link);
...
mutex_unlock(vma->vm->mutex);
__i915_vm_release();
release_references();
And in release_reference() we dereference vma->vm to get to the
vm gt pointer, leading to a use-after free.
However, __i915_vm_release() grabs the vm->mutex so the vm won't be
destroyed before vma->vm->mutex is released, so extract the gt pointer
under the vm->mutex to avoid the vma->vm dereference in
release_references().
v2: Fix a typo in the commit message (Andi Shyti)
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5944
Fixes: e1a7ab4fca ("drm/i915: Remove the vm open count")
Cc: Niranjana Vishwanathapura
Cc: Matthew Auld
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/i915_vma.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 0bffb70b3c5f..04d12f278f57 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1637,10 +1637,10 @@ static void force_unbind(struct i915_vma *vma)
GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
}
-static void release_references(struct i915_vma *vma, bool vm_ddestroy)
+static void release_references(struct i915_vma *vma, struct intel_gt *gt,
+ bool vm_ddestroy)
{
struct drm_i915_gem_object *obj = vma->obj;
- struct intel_gt *gt = vma->vm->gt;
GEM_BUG_ON(i915_vma_is_active(vma));
@@ -1695,11 +1695,12 @@ void i915_vma_destroy_locked(struct i915_vma *vma)
force_unbind(vma);
list_del_init(&vma->vm_link);
- release_references(vma, false);
+ release_references(vma, vma->vm->gt, false);
}
void i915_vma_destroy(struct i915_vma *vma)
{
+ struct intel_gt *gt;
bool vm_ddestroy;
mutex_lock(&vma->vm->mutex);
@@ -1707,8 +1708,11 @@ void i915_vma_destroy(struct i915_vma *vma)
list_del_init(&vma->vm_link);
vm_ddestroy = vma->vm_ddestroy;
vma->vm_ddestroy = false;
+
+ /* vma->vm may be freed when releasing vma->vm->mutex. */
+ gt = vma->vm->gt;
mutex_unlock(&vma->vm->mutex);
- release_references(vma, vm_ddestroy);
+ release_references(vma, gt, vm_ddestroy);
}
void i915_vma_parked(struct intel_gt *gt)
--
2.34.3
Re: [PATCH 0/2] video: fbdev: Convert from PCI to generic power management
On 6/8/22 20:09, Bjorn Helgaas wrote: > On Wed, Jun 08, 2022 at 06:26:34PM +0200, Daniel Vetter wrote: >> On Tue, Jun 07, 2022 at 06:11:10PM -0500, Bjorn Helgaas wrote: >>> From: Bjorn Helgaas >>> >>> PCI-specific power management (pci_driver.suspend and pci_driver.resume) is >>> deprecated. If drivers implement power management, they should use the >>> generic power management framework, not the PCI-specific hooks. >>> >>> No fbdev drivers actually implement PCI power management, but there are a >>> cirrusfb has some commented-out references to it and skeletonfb has >>> examples of it. Remove these. >> >> Is this holding up some cleanup on your side and so would be easier to >> merge these through the pci tree? If so >> >> Acked-by: Daniel Vetter >> >> for merging through your tree. Otherwise I guess Helge will get around to >> pile them up for 5.20 (or 6.0) eventually. > > No particular rush and nothing depending on these. > > I added your ack to my local branch and if nothing happens for a > while, I'll merge them via my tree. I've been on vacation, but picked them up now. Thanks! Helge
Re: [PATCH 13/64] drm/vc4: hvs: Protect device resources after removal
Hi Dave, On Tue, Jun 14, 2022 at 04:11:20PM +0100, Dave Stevenson wrote: > On Fri, 10 Jun 2022 at 10:30, Maxime Ripard wrote: > > > > Whenever the device and driver are unbound, the main device and all the > > subdevices will be removed by calling their unbind() method. > > > > However, the DRM device itself will only be freed when the last user will > > have closed it. > > > > It means that there is a time window where the device and its resources > > aren't there anymore, but the userspace can still call into our driver. > > > > Fortunately, the DRM framework provides the drm_dev_enter() and > > drm_dev_exit() functions to make sure our underlying device is still there > > for the section protected by those calls. Let's add them to the HVS driver. > > The framework appears to rely on the remove function calling > drm_dev_unplug instead of drm_dev_unregister, but I haven't seen a > patch that makes that change in the vc4 driver. > Have I missed it, or is there some other route to set the unplugged > flag that drm_dev_enter/exit are relying on? You're right, we should have called drm_dev_unplug. I fixed it up (and the fallouts) Thanks! Maxime signature.asc Description: PGP signature
Re: [PATCH 13/64] drm/vc4: hvs: Protect device resources after removal
Hi,
On Tue, Jun 14, 2022 at 05:59:15PM +0100, Dave Stevenson wrote:
> > > @@ -132,14 +139,18 @@ static int vc4_hvs_upload_linear_kernel(struct
> > > vc4_hvs *hvs,
> > > struct drm_mm_node *space,
> > > const u32 *kernel)
> > > {
> > > - int ret, i;
> > > + struct drm_device *drm = hvs->dev;
> > > + int idx, ret, i;
> > > u32 __iomem *dst_kernel;
> > >
> > > + if (!drm_dev_enter(drm, &idx))
> > > + return -ENODEV;
> > > +
>
> vc4_hvs_upload_linear_kernel is only called from vc4_hvs_bind, so
> unless bind and unbind calls can be concurrent, then there's no need
> for protection here.
Indeed. I've removed those changes and added a comment
Thanks!
Maxime
signature.asc
Description: PGP signature
Re: [PATCH v2] drm/i915: Fix vm use-after-free in vma destruction
Acked-by: Nirmoy Das
On 6/20/2022 2:36 PM, Thomas Hellström wrote:
In vma destruction, the following race may occur:
Thread 1: Thread 2:
i915_vma_destroy();
...
list_del_init(vma->vm_link);
...
mutex_unlock(vma->vm->mutex);
__i915_vm_release();
release_references();
And in release_reference() we dereference vma->vm to get to the
vm gt pointer, leading to a use-after free.
However, __i915_vm_release() grabs the vm->mutex so the vm won't be
destroyed before vma->vm->mutex is released, so extract the gt pointer
under the vm->mutex to avoid the vma->vm dereference in
release_references().
v2: Fix a typo in the commit message (Andi Shyti)
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5944
Fixes: e1a7ab4fca ("drm/i915: Remove the vm open count")
Cc: Niranjana Vishwanathapura
Cc: Matthew Auld
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/i915_vma.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 0bffb70b3c5f..04d12f278f57 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1637,10 +1637,10 @@ static void force_unbind(struct i915_vma *vma)
GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
}
-static void release_references(struct i915_vma *vma, bool vm_ddestroy)
+static void release_references(struct i915_vma *vma, struct intel_gt *gt,
+ bool vm_ddestroy)
{
struct drm_i915_gem_object *obj = vma->obj;
- struct intel_gt *gt = vma->vm->gt;
GEM_BUG_ON(i915_vma_is_active(vma));
@@ -1695,11 +1695,12 @@ void i915_vma_destroy_locked(struct i915_vma *vma)
force_unbind(vma);
list_del_init(&vma->vm_link);
- release_references(vma, false);
+ release_references(vma, vma->vm->gt, false);
}
void i915_vma_destroy(struct i915_vma *vma)
{
+ struct intel_gt *gt;
bool vm_ddestroy;
mutex_lock(&vma->vm->mutex);
@@ -1707,8 +1708,11 @@ void i915_vma_destroy(struct i915_vma *vma)
list_del_init(&vma->vm_link);
vm_ddestroy = vma->vm_ddestroy;
vma->vm_ddestroy = false;
+
+ /* vma->vm may be freed when releasing vma->vm->mutex. */
+ gt = vma->vm->gt;
mutex_unlock(&vma->vm->mutex);
- release_references(vma, vm_ddestroy);
+ release_references(vma, gt, vm_ddestroy);
}
void i915_vma_parked(struct intel_gt *gt)
Re: [PATCH 09/64] drm/simple: Introduce drmm_simple_encoder_init
Hi, On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: > Am 10.06.22 um 11:28 schrieb Maxime Ripard: > > The DRM-managed function to register an encoder is > > drmm_simple_encoder_alloc() and its variants, which will allocate the > > underlying structure and initialisation the encoder. > > > > However, we might want to separate the structure creation and the encoder > > initialisation, for example if the structure is shared across multiple DRM > > entities, for example an encoder and a connector. > > > > Let's create an helper to only initialise an encoder that would be passed > > as an argument. > > > > There's nothing wrong with this patch, but I have to admit that adding > drm_simple_encoder_init() et al was a mistake. Why do you think it was a mistake? > It would have been better to add an initializer macro like > > #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ > .destroy = drm_encoder_cleanup > > It's way more flexible and similarly easy to use. Anyway... We can still have this. It would simplify this series so I could definitely squeeze that patch in and add a TODO item / deprecation notice for simple encoders if you think it's needed Maxime signature.asc Description: PGP signature
Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker
On 6/19/22 20:53, Rob Clark wrote:
...
>> +static unsigned long
>> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
>> +struct shrink_control *sc)
>> +{
>> + struct drm_gem_shmem_shrinker *gem_shrinker =
>> to_drm_shrinker(shrinker);
>> + struct drm_gem_shmem_object *shmem;
>> + unsigned long count = 0;
>> +
>> + if (!mutex_trylock(&gem_shrinker->lock))
>> + return 0;
>> +
>> + list_for_each_entry(shmem, &gem_shrinker->lru_evictable, madv_list) {
>> + count += shmem->base.size;
>> +
>> + if (count >= SHRINK_EMPTY)
>> + break;
>> + }
>> +
>> + mutex_unlock(&gem_shrinker->lock);
>
> As I mentioned on other thread, count_objects, being approximate but
> lockless and fast is the important thing. Otherwise when you start
> hitting the shrinker on many threads, you end up serializing them all,
> even if you have no pages to return to the system at that point.
Daniel's point for dropping the lockless variant was that we're already
in trouble if we're hitting shrinker too often and extra optimizations
won't bring much benefits to us.
Alright, I'll add back the lockless variant (or will use yours
drm_gem_lru) in the next revision. The code difference is very small
after all.
...
>> + /* prevent racing with the dma-buf importing/exporting */
>> + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) {
>> + *lock_contention |= true;
>> + goto resv_unlock;
>> + }
>
> I'm not sure this is a good idea to serialize on object_name_lock.
> Purgeable buffers should never be shared (imported or exported). So
> at best you are avoiding evicting and immediately swapping back in, in
> a rare case, at the cost of serializing multiple threads trying to
> reclaim pages in parallel.
The object_name_lock shouldn't cause contention in practice. But objects
are also pinned on attachment, hence maybe this lock is indeed
unnecessary.. I'll re-check it.
--
Best regards,
Dmitry
Re: [PATCH 05/64] drm/connector: Mention the cleanup after drm_connector_init
Hi Am 20.06.22 um 14:18 schrieb Maxime Ripard: + * At driver unload time the driver's &drm_connector_funcs.destroy hook + * should call drm_connector_unregister(), drm_connector_cleanup() and + * kfree() the connector structure. This sentence sounds odd. -- 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 09/64] drm/simple: Introduce drmm_simple_encoder_init
Hi Am 20.06.22 um 15:48 schrieb Maxime Ripard: Hi, On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: Am 10.06.22 um 11:28 schrieb Maxime Ripard: The DRM-managed function to register an encoder is drmm_simple_encoder_alloc() and its variants, which will allocate the underlying structure and initialisation the encoder. However, we might want to separate the structure creation and the encoder initialisation, for example if the structure is shared across multiple DRM entities, for example an encoder and a connector. Let's create an helper to only initialise an encoder that would be passed as an argument. There's nothing wrong with this patch, but I have to admit that adding drm_simple_encoder_init() et al was a mistake. Why do you think it was a mistake? The simple-encoder stuff is an interface that no one really needs. Compared to open-coding the function, it's barely an improvement in LOCs, but nothing else. Back when I added drm_simple_encoder_init(), I wanted to simplify the many drivers that initialized the encoder with a cleanup callback and nothing else. IIRC it was an improvement back then. But now we already have a related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro back then, we could use the regular encoder helpers. It would have been better to add an initializer macro like #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ .destroy = drm_encoder_cleanup It's way more flexible and similarly easy to use. Anyway... We can still have this. It would simplify this series so I could definitely squeeze that patch in and add a TODO item / deprecation notice for simple encoders if you think it's needed Not necessary. It's not super important. Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 09/64] drm/simple: Introduce drmm_simple_encoder_init
On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote: > Hi > > Am 20.06.22 um 15:48 schrieb Maxime Ripard: > > Hi, > > > > On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: > > > Am 10.06.22 um 11:28 schrieb Maxime Ripard: > > > > The DRM-managed function to register an encoder is > > > > drmm_simple_encoder_alloc() and its variants, which will allocate the > > > > underlying structure and initialisation the encoder. > > > > > > > > However, we might want to separate the structure creation and the > > > > encoder > > > > initialisation, for example if the structure is shared across multiple > > > > DRM > > > > entities, for example an encoder and a connector. > > > > > > > > Let's create an helper to only initialise an encoder that would be > > > > passed > > > > as an argument. > > > > > > > > > > There's nothing wrong with this patch, but I have to admit that adding > > > drm_simple_encoder_init() et al was a mistake. > > > > Why do you think it was a mistake? > > The simple-encoder stuff is an interface that no one really needs. Compared > to open-coding the function, it's barely an improvement in LOCs, but nothing > else. > > Back when I added drm_simple_encoder_init(), I wanted to simplify the many > drivers that initialized the encoder with a cleanup callback and nothing > else. IIRC it was an improvement back then. But now we already have a > related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro > back then, we could use the regular encoder helpers. > > > > > > It would have been better to add an initializer macro like > > > > > > #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ > > >.destroy = drm_encoder_cleanup > > > > > > It's way more flexible and similarly easy to use. Anyway... > > > > We can still have this. It would simplify this series so I could > > definitely squeeze that patch in and add a TODO item / deprecation > > notice for simple encoders if you think it's needed > > Not necessary. It's not super important. The corollary is though :) If I understand you right, it means that you'd rather have a destroy callback everywhere instead of calling the _cleanup function through a drm-managed callback, and let drm_dev_unregister do its job? If so, it means that we shouldn't be following the drmm_.*_alloc functions and should drop all the new ones from this series. Maxime signature.asc Description: PGP signature
Re: [PATCH 05/64] drm/connector: Mention the cleanup after drm_connector_init
On Mon, Jun 20, 2022 at 04:19:43PM +0200, Thomas Zimmermann wrote: > Hi > > Am 20.06.22 um 14:18 schrieb Maxime Ripard: > > + * At driver unload time the driver's &drm_connector_funcs.destroy hook > > + * should call drm_connector_unregister(), drm_connector_cleanup() and > > + * kfree() the connector structure. > > This sentence sounds odd. Yeah, I was using kfree as a verb which is probably where the weirdness comes from. Would that be better: At driver unload time the driver's &drm_connector_funcs.destroy hook should call drm_connector_unregister() and free the connector structure. Maxime signature.asc Description: PGP signature
RE: [PATCH v2 3/3] drm/doc/rfc: VM_BIND uapi definition
Thanks,
Oak
> -Original Message-
> From: Vishwanathapura, Niranjana
> Sent: June 17, 2022 1:15 AM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vetter,
> Daniel
> Cc: Hellstrom, Thomas ; Wilson, Chris P
> ; ja...@jlekstrand.net;
> christian.koe...@amd.com; Brost, Matthew ;
> Ursulin, Tvrtko ; Auld, Matthew
> ; Landwerlin, Lionel G
> ; Zanoni, Paulo R
> ; Zeng, Oak
> Subject: [PATCH v2 3/3] drm/doc/rfc: VM_BIND uapi definition
>
> VM_BIND and related uapi definitions
>
> v2: Reduce the scope to simple Mesa use case.
>
> Signed-off-by: Niranjana Vishwanathapura
>
> ---
> Documentation/gpu/rfc/i915_vm_bind.h | 226
> +++
> 1 file changed, 226 insertions(+)
> create mode 100644 Documentation/gpu/rfc/i915_vm_bind.h
>
> diff --git a/Documentation/gpu/rfc/i915_vm_bind.h
> b/Documentation/gpu/rfc/i915_vm_bind.h
> new file mode 100644
> index ..b7540ddb526d
> --- /dev/null
> +++ b/Documentation/gpu/rfc/i915_vm_bind.h
> @@ -0,0 +1,226 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +/**
> + * DOC: I915_PARAM_HAS_VM_BIND
> + *
> + * VM_BIND feature availability.
> + * See typedef drm_i915_getparam_t param.
> + */
> +#define I915_PARAM_HAS_VM_BIND 57
> +
> +/**
> + * DOC: I915_VM_CREATE_FLAGS_USE_VM_BIND
> + *
> + * Flag to opt-in for VM_BIND mode of binding during VM creation.
> + * See struct drm_i915_gem_vm_control flags.
> + *
> + * The older execbuf2 ioctl will not support VM_BIND mode of operation.
> + * For VM_BIND mode, we have new execbuf3 ioctl which will not accept
> any
> + * execlist (See struct drm_i915_gem_execbuffer3 for more details).
> + *
> + */
> +#define I915_VM_CREATE_FLAGS_USE_VM_BIND (1 << 0)
> +
> +/* VM_BIND related ioctls */
> +#define DRM_I915_GEM_VM_BIND 0x3d
> +#define DRM_I915_GEM_VM_UNBIND 0x3e
> +#define DRM_I915_GEM_EXECBUFFER3 0x3f
> +
> +#define DRM_IOCTL_I915_GEM_VM_BIND
> DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_BIND,
> struct drm_i915_gem_vm_bind)
> +#define DRM_IOCTL_I915_GEM_VM_UNBIND
> DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_VM_UNBIND, struct drm_i915_gem_vm_bind)
> +#define DRM_IOCTL_I915_GEM_EXECBUFFER3
> DRM_IOWR(DRM_COMMAND_BASE +
> DRM_I915_GEM_EXECBUFFER3, struct drm_i915_gem_execbuffer3)
> +
> +/**
> + * struct drm_i915_gem_vm_bind_fence - Bind/unbind completion
> notification.
> + *
> + * A timeline out fence for vm_bind/unbind completion notification.
> + */
> +struct drm_i915_gem_vm_bind_fence {
> + /** @handle: User's handle for a drm_syncobj to signal. */
> + __u32 handle;
> +
> + /** @rsvd: Reserved, MBZ */
> + __u32 rsvd;
> +
> + /**
> + * @value: A point in the timeline.
> + * Value must be 0 for a binary drm_syncobj. A Value of 0 for a
> + * timeline drm_syncobj is invalid as it turns a drm_syncobj into a
> + * binary one.
> + */
> + __u64 value;
> +};
> +
> +/**
> + * struct drm_i915_gem_vm_bind - VA to object mapping to bind.
> + *
> + * This structure is passed to VM_BIND ioctl and specifies the mapping of
> GPU
> + * virtual address (VA) range to the section of an object that should be
> bound
> + * in the device page table of the specified address space (VM).
> + * The VA range specified must be unique (ie., not currently bound) and can
> + * be mapped to whole object or a section of the object (partial binding).
> + * Multiple VA mappings can be created to the same section of the object
> + * (aliasing).
> + *
> + * The @start, @offset and @length should be 4K page aligned. However
> the DG2
> + * and XEHPSDV has 64K page size for device local-memory and has compact
> page
> + * table. On those platforms, for binding device local-memory objects, the
> + * @start should be 2M aligned, @offset and @length should be 64K aligned.
> + * Also, on those platforms, it is not allowed to bind an device local-memory
> + * object and a system memory object in a single 2M section of VA range.
> + */
> +struct drm_i915_gem_vm_bind {
> + /** @vm_id: VM (address space) id to bind */
> + __u32 vm_id;
> +
> + /** @handle: Object handle */
> + __u32 handle;
> +
> + /** @start: Virtual Address start to bind */
> + __u64 start;
> +
> + /** @offset: Offset in object to bind */
> + __u64 offset;
> +
> + /** @length: Length of mapping to bind */
> + __u64 length;
> +
> + /**
> + * @flags: Supported flags are:
> + *
> + * I915_GEM_VM_BIND_READONLY:
> + * Mapping is read-only.
> + *
> + * I915_GEM_VM_BIND_CAPTURE:
> + * Capture this mapping in the dump upon GPU error.
> + */
> + __u64 flags;
> +#define I915_GEM_VM_BIND_READONLY(1 << 0)
Should we define another flag for DEVICE_ATOMIC? Without this flag, do you
imply all the mapping support device atomic operation?
HW platform also has an implication to device atomic, i.e., some platform don
Re: [PATCH 05/64] drm/connector: Mention the cleanup after drm_connector_init
Hi Am 20.06.22 um 16:40 schrieb Maxime Ripard: On Mon, Jun 20, 2022 at 04:19:43PM +0200, Thomas Zimmermann wrote: Hi Am 20.06.22 um 14:18 schrieb Maxime Ripard: + * At driver unload time the driver's &drm_connector_funcs.destroy hook + * should call drm_connector_unregister(), drm_connector_cleanup() and + * kfree() the connector structure. This sentence sounds odd. Yeah, I was using kfree as a verb which is probably where the weirdness comes from. Would that be better: At driver unload time the driver's &drm_connector_funcs.destroy hook should call drm_connector_unregister() and free the connector structure. Yes. That's better. Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 09/64] drm/simple: Introduce drmm_simple_encoder_init
Hi Am 20.06.22 um 16:39 schrieb Maxime Ripard: On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote: Hi Am 20.06.22 um 15:48 schrieb Maxime Ripard: Hi, On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: Am 10.06.22 um 11:28 schrieb Maxime Ripard: The DRM-managed function to register an encoder is drmm_simple_encoder_alloc() and its variants, which will allocate the underlying structure and initialisation the encoder. However, we might want to separate the structure creation and the encoder initialisation, for example if the structure is shared across multiple DRM entities, for example an encoder and a connector. Let's create an helper to only initialise an encoder that would be passed as an argument. There's nothing wrong with this patch, but I have to admit that adding drm_simple_encoder_init() et al was a mistake. Why do you think it was a mistake? The simple-encoder stuff is an interface that no one really needs. Compared to open-coding the function, it's barely an improvement in LOCs, but nothing else. Back when I added drm_simple_encoder_init(), I wanted to simplify the many drivers that initialized the encoder with a cleanup callback and nothing else. IIRC it was an improvement back then. But now we already have a related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro back then, we could use the regular encoder helpers. It would have been better to add an initializer macro like #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ .destroy = drm_encoder_cleanup It's way more flexible and similarly easy to use. Anyway... We can still have this. It would simplify this series so I could definitely squeeze that patch in and add a TODO item / deprecation notice for simple encoders if you think it's needed Not necessary. It's not super important. The corollary is though :) If I understand you right, it means that you'd rather have a destroy callback everywhere instead of calling the _cleanup function through a drm-managed callback, and let drm_dev_unregister do its job? If so, it means that we shouldn't be following the drmm_.*_alloc functions and should drop all the new ones from this series. No, no. What I'm saying is that simple-encoder is a pointless mid-layer. There's nothing that couldn't easily be achieved with the regular encoder functions. Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker
On Mon, Jun 20, 2022 at 7:09 AM Dmitry Osipenko
wrote:
>
> On 6/19/22 20:53, Rob Clark wrote:
> ...
> >> +static unsigned long
> >> +drm_gem_shmem_shrinker_count_objects(struct shrinker *shrinker,
> >> +struct shrink_control *sc)
> >> +{
> >> + struct drm_gem_shmem_shrinker *gem_shrinker =
> >> to_drm_shrinker(shrinker);
> >> + struct drm_gem_shmem_object *shmem;
> >> + unsigned long count = 0;
> >> +
> >> + if (!mutex_trylock(&gem_shrinker->lock))
> >> + return 0;
> >> +
> >> + list_for_each_entry(shmem, &gem_shrinker->lru_evictable,
> >> madv_list) {
> >> + count += shmem->base.size;
> >> +
> >> + if (count >= SHRINK_EMPTY)
> >> + break;
> >> + }
> >> +
> >> + mutex_unlock(&gem_shrinker->lock);
> >
> > As I mentioned on other thread, count_objects, being approximate but
> > lockless and fast is the important thing. Otherwise when you start
> > hitting the shrinker on many threads, you end up serializing them all,
> > even if you have no pages to return to the system at that point.
>
> Daniel's point for dropping the lockless variant was that we're already
> in trouble if we're hitting shrinker too often and extra optimizations
> won't bring much benefits to us.
At least with zram swap (which I highly recommend using even if you
are not using a physical swap file/partition), swapin/out is actually
quite fast. And if you are leaning on zram swap to fit 8GB of chrome
browser on a 4GB device, the shrinker gets hit quite a lot. Lower
spec (4GB RAM) chromebooks can be under constant memory pressure and
can quite easily get into a situation where you are hitting the
shrinker on many threads simultaneously. So it is pretty important
for all shrinkers in the system (not just drm driver) to be as
concurrent as possible. As long as you avoid serializing reclaim on
all the threads, performance can still be quite good, but if you don't
performance will fall off a cliff.
jfwiw, we are seeing pretty good results (iirc 40-70% increase in open
tab counts) with the combination of eviction + multigen LRU[1] +
sizing zram swap to be 2x physical RAM
[1] https://lwn.net/Articles/856931/
> Alright, I'll add back the lockless variant (or will use yours
> drm_gem_lru) in the next revision. The code difference is very small
> after all.
>
> ...
> >> + /* prevent racing with the dma-buf importing/exporting */
> >> + if (!mutex_trylock(&gem_shrinker->dev->object_name_lock)) {
> >> + *lock_contention |= true;
> >> + goto resv_unlock;
> >> + }
> >
> > I'm not sure this is a good idea to serialize on object_name_lock.
> > Purgeable buffers should never be shared (imported or exported). So
> > at best you are avoiding evicting and immediately swapping back in, in
> > a rare case, at the cost of serializing multiple threads trying to
> > reclaim pages in parallel.
>
> The object_name_lock shouldn't cause contention in practice. But objects
> are also pinned on attachment, hence maybe this lock is indeed
> unnecessary.. I'll re-check it.
I'm not worried about contention with export/import/etc, but
contention between multiple threads hitting the shrinker in parallel.
I guess since you are using trylock, it won't *block* the other
threads hitting shrinker, but they'll just end up looping in
do_shrink_slab() because they are hitting contention.
I'd have to do some experiments to see how it works out in practice,
but my gut feel is that it isn't a good idea
BR,
-R
> --
> Best regards,
> Dmitry
Re: [PATCH 3/3] drm/msm/dp: Get rid of dp_ctrl_on_stream_phy_test_report()
On 6/17/2022 1:47 PM, Stephen Boyd wrote:
This API isn't really more than a couple lines now that we don't store
the pixel_rate to the struct member. Inline it into the caller.
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
Reviewed-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 40
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index e114521af2e9..d04fddb29fdf 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1582,34 +1582,15 @@ static bool dp_ctrl_send_phy_test_pattern(struct
dp_ctrl_private *ctrl)
return success;
}
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
+static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
{
int ret;
- struct dp_ctrl_private *ctrl;
unsigned long pixel_rate;
- ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
-
- pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
- ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
- if (ret) {
- DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
- return ret;
- }
-
- dp_ctrl_send_phy_test_pattern(ctrl);
-
- return 0;
-}
-
-static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private *ctrl)
-{
- int ret = 0;
-
if (!ctrl->link->phy_params.phy_test_pattern_sel) {
drm_dbg_dp(ctrl->drm_dev,
"no test pattern selected by sink\n");
- return ret;
+ return 0;
}
/*
@@ -1624,12 +1605,21 @@ static int dp_ctrl_process_phy_test_request(struct
dp_ctrl_private *ctrl)
}
ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
- if (!ret)
- ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
- else
+ if (ret) {
DRM_ERROR("failed to enable DP link controller\n");
+ return ret;
+ }
- return ret;
+ pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+ ret = dp_ctrl_enable_stream_clocks(ctrl, pixel_rate);
+ if (ret) {
+ DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
+ return ret;
+ }
+
+ dp_ctrl_send_phy_test_pattern(ctrl);
+
+ return 0;
}
void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
Re: [PATCH 1/3] drm/msm/dp: Reorganize code to avoid forward declaration
On 6/17/2022 3:50 PM, Dmitry Baryshkov wrote:
On 17/06/2022 23:47, Stephen Boyd wrote:
Let's move these functions around to avoid having to forward declare
dp_ctrl_on_stream_phy_test_report(). Also remove
dp_ctrl_reinitialize_mainlink() forward declaration because we're doing
that sort of task.
Cc: Kuogee Hsieh
Signed-off-by: Stephen Boyd
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_ctrl.c | 104 +++
1 file changed, 50 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 703249384e7c..bd445e683cfc 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -1238,8 +1238,6 @@ static int dp_ctrl_link_train_2(struct
dp_ctrl_private *ctrl,
return -ETIMEDOUT;
}
-static int dp_ctrl_reinitialize_mainlink(struct dp_ctrl_private
*ctrl);
-
static int dp_ctrl_link_train(struct dp_ctrl_private *ctrl,
int *training_step)
{
@@ -1534,38 +1532,6 @@ static int dp_ctrl_link_maintenance(struct
dp_ctrl_private *ctrl)
return ret;
}
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl
*dp_ctrl);
-
-static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private
*ctrl)
-{
- int ret = 0;
-
- if (!ctrl->link->phy_params.phy_test_pattern_sel) {
- drm_dbg_dp(ctrl->drm_dev,
- "no test pattern selected by sink\n");
- return ret;
- }
-
- /*
- * The global reset will need DP link related clocks to be
- * running. Add the global reset just before disabling the
- * link clocks and core clocks.
- */
- ret = dp_ctrl_off(&ctrl->dp_ctrl);
- if (ret) {
- DRM_ERROR("failed to disable DP controller\n");
- return ret;
- }
-
- ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
- if (!ret)
- ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
- else
- DRM_ERROR("failed to enable DP link controller\n");
-
- return ret;
-}
-
static bool dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private
*ctrl)
{
bool success = false;
@@ -1618,6 +1584,56 @@ static bool
dp_ctrl_send_phy_test_pattern(struct dp_ctrl_private *ctrl)
return success;
}
+static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
+{
+ int ret;
+ struct dp_ctrl_private *ctrl;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
+
+ ret = dp_ctrl_enable_stream_clocks(ctrl);
+ if (ret) {
+ DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
+ return ret;
+ }
+
+ dp_ctrl_send_phy_test_pattern(ctrl);
+
+ return 0;
+}
+
+static int dp_ctrl_process_phy_test_request(struct dp_ctrl_private
*ctrl)
+{
+ int ret = 0;
+
+ if (!ctrl->link->phy_params.phy_test_pattern_sel) {
+ drm_dbg_dp(ctrl->drm_dev,
+ "no test pattern selected by sink\n");
+ return ret;
+ }
+
+ /*
+ * The global reset will need DP link related clocks to be
+ * running. Add the global reset just before disabling the
+ * link clocks and core clocks.
+ */
+ ret = dp_ctrl_off(&ctrl->dp_ctrl);
+ if (ret) {
+ DRM_ERROR("failed to disable DP controller\n");
+ return ret;
+ }
+
+ ret = dp_ctrl_on_link(&ctrl->dp_ctrl);
+ if (!ret)
+ ret = dp_ctrl_on_stream_phy_test_report(&ctrl->dp_ctrl);
+ else
+ DRM_ERROR("failed to enable DP link controller\n");
+
+ return ret;
+}
+
void dp_ctrl_handle_sink_request(struct dp_ctrl *dp_ctrl)
{
struct dp_ctrl_private *ctrl;
@@ -1815,26 +1831,6 @@ static int dp_ctrl_link_retrain(struct
dp_ctrl_private *ctrl)
return dp_ctrl_setup_main_link(ctrl, &training_step);
}
-static int dp_ctrl_on_stream_phy_test_report(struct dp_ctrl *dp_ctrl)
-{
- int ret;
- struct dp_ctrl_private *ctrl;
-
- ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
-
- ctrl->dp_ctrl.pixel_rate = ctrl->panel->dp_mode.drm_mode.clock;
-
- ret = dp_ctrl_enable_stream_clocks(ctrl);
- if (ret) {
- DRM_ERROR("Failed to start pixel clocks. ret=%d\n", ret);
- return ret;
- }
-
- dp_ctrl_send_phy_test_pattern(ctrl);
-
- return 0;
-}
-
int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl, bool force_link_train)
{
int ret = 0;
Re: [RFT][PATCH v1 6/6] vfio: Replace phys_pfn with phys_page for vfio_pin_pages()
On Sun, Jun 19, 2022 at 11:37:47PM -0700, Christoph Hellwig wrote: > On Sun, Jun 19, 2022 at 10:51:47PM -0700, Christoph Hellwig wrote: > > On Mon, Jun 20, 2022 at 12:00:46AM -0300, Jason Gunthorpe wrote: > > > On Fri, Jun 17, 2022 at 01:54:05AM -0700, Christoph Hellwig wrote: > > > > There is a bunch of code an comments in the iommu type1 code that > > > > suggest we can pin memory that is not page backed. > > > > > > AFAIK you can.. The whole follow_pte() mechanism allows raw PFNs to be > > > loaded into the type1 maps and the pin API will happily return > > > them. This happens in almost every qemu scenario because PCI MMIO BAR > > > memory ends up routed down this path. > > > > Indeed, my read wasn't deep enough. Which means that we can't change > > the ->pin_pages interface to return a struct pages array, as we don't > > have one for those. > > Actually. gvt requires a struct page, and both s390 seem to require > normal non-I/O, non-remapped kernel pointers. So I think for the > vfio_pin_pages we can assume that we only want page backed memory and > remove the follow_fault_pfn case entirely. But we'll probably have > to keep it for the vfio_iommu_replay case that is not tied to > emulated IOMMU drivers. Right, that is my thinking - since all drivers actually need a struct page we should have the API return a working struct page and have the VFIO layer isolate the non-struct page stuff so it never leaks out of this API. This nicely fixes the various problems in all the drivers if io memory comes down this path. It is also why doing too much surgery deeper into type 1 probably isn't too worthwhile as it still needs raw pfns in its data structures for iommu modes. Thanks, Jason
Re: [PATCH v6 17/22] drm/shmem-helper: Add generic memory shrinker
()
On Thu, May 26, 2022 at 4:55 PM Dmitry Osipenko
wrote:
>
> Introduce a common DRM SHMEM shrinker framework that allows to reduce
> code duplication among DRM drivers by replacing theirs custom shrinker
> implementations with the generic shrinker.
>
> In order to start using DRM SHMEM shrinker drivers should:
>
> 1. Implement new evict() shmem object callback.
> 2. Register shrinker using drm_gem_shmem_shrinker_register(drm_device).
> 3. Use drm_gem_shmem_set_purgeable(shmem) and alike API functions to
>activate shrinking of shmem GEMs.
>
> This patch is based on a ideas borrowed from Rob's Clark MSM shrinker,
> Thomas' Zimmermann variant of SHMEM shrinker and Intel's i915 shrinker.
>
> Signed-off-by: Daniel Almeida
> Signed-off-by: Dmitry Osipenko
> ---
> drivers/gpu/drm/drm_gem_shmem_helper.c| 540 --
> .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 9 +-
> drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +
> include/drm/drm_device.h | 4 +
> include/drm/drm_gem_shmem_helper.h| 87 ++-
> 5 files changed, 594 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 555fe212bd98..4cd0b5913492 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -126,6 +126,42 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(struct
> drm_device *dev, size_t
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
>
> +static bool drm_gem_shmem_is_evictable(struct drm_gem_shmem_object *shmem)
> +{
> + return (shmem->madv >= 0) && shmem->evict &&
> + shmem->eviction_enabled && shmem->pages_use_count &&
> + !shmem->pages_pin_count && !shmem->base.dma_buf &&
> + !shmem->base.import_attach && shmem->sgt && !shmem->evicted;
> +}
> +
> +static void
> +drm_gem_shmem_update_pages_state(struct drm_gem_shmem_object *shmem)
> +{
> + struct drm_gem_object *obj = &shmem->base;
> + struct drm_gem_shmem_shrinker *gem_shrinker =
> obj->dev->shmem_shrinker;
> +
> + dma_resv_assert_held(shmem->base.resv);
> +
> + if (!gem_shrinker || obj->import_attach)
> + return;
> +
> + mutex_lock(&gem_shrinker->lock);
> +
> + if (drm_gem_shmem_is_evictable(shmem) ||
> + drm_gem_shmem_is_purgeable(shmem))
> + list_move_tail(&shmem->madv_list,
> &gem_shrinker->lru_evictable);
> + else if (shmem->madv < 0)
> + list_del_init(&shmem->madv_list);
> + else if (shmem->evicted)
> + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_evicted);
> + else if (!shmem->pages)
> + list_del_init(&shmem->madv_list);
> + else
> + list_move_tail(&shmem->madv_list, &gem_shrinker->lru_pinned);
> +
> + mutex_unlock(&gem_shrinker->lock);
> +}
> +
> /**
> * drm_gem_shmem_free - Free resources associated with a shmem GEM object
> * @shmem: shmem GEM object to free
> @@ -142,6 +178,9 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> } else {
> dma_resv_lock(shmem->base.resv, NULL);
>
> + /* take out shmem GEM object from the memory shrinker */
> + drm_gem_shmem_madvise(shmem, -1);
> +
> WARN_ON(shmem->vmap_use_count);
>
> if (shmem->sgt) {
> @@ -150,7 +189,7 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> sg_free_table(shmem->sgt);
> kfree(shmem->sgt);
> }
> - if (shmem->pages)
> + if (shmem->pages_use_count)
> drm_gem_shmem_put_pages(shmem);
>
> WARN_ON(shmem->pages_use_count);
> @@ -163,18 +202,82 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object
> *shmem)
> }
> EXPORT_SYMBOL_GPL(drm_gem_shmem_free);
>
> -static int drm_gem_shmem_get_pages(struct drm_gem_shmem_object *shmem)
> +/**
> + * drm_gem_shmem_set_evictable() - Make GEM evictable by memory shrinker
> + * @shmem: shmem GEM object
> + *
> + * Tell memory shrinker that this GEM can be evicted. Initially eviction is
> + * disabled for all GEMs. If GEM was purged, then -ENOMEM is returned.
> + *
> + * Returns:
> + * 0 on success or a negative error code on failure.
> + */
> +int drm_gem_shmem_set_evictable(struct drm_gem_shmem_object *shmem)
> +{
> + dma_resv_lock(shmem->base.resv, NULL);
> +
> + if (shmem->madv < 0)
> + return -ENOMEM;
> +
> + shmem->eviction_enabled = true;
> +
> + dma_resv_unlock(shmem->base.resv);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(drm_gem_shmem_set_evictable);
> +
> +/**
> + * drm_gem_shmem_set_purgeable() - Make GEM purgeable by memory shrinker
> + * @shmem: shmem GEM object
> + *
> + * Tell memory shrinker that this GEM can be purged. Initially purging is
> + * disabled for al
Re: [RFT][PATCH v1 5/6] vfio/ccw: Add kmap_local_page() for memcpy
On Sun, Jun 19, 2022 at 11:32:07PM -0700, Christoph Hellwig wrote: > > This helps because we now block io memory from ever getting into these > > call paths. I'm pretty sure this is a serious security bug, but would > > let the IBM folks remark as I don't know it all that well.. > > Prevent as in crash when trying to convert it to a page? That or when calling memcpy() on an IO memory PFN that the guest passed into the dma s/g list the ccw driver is processing. Jason
Re: [PATCH v2 3/3] drm/doc/rfc: VM_BIND uapi definition
On Mon, Jun 20, 2022 at 07:42:25AM -0700, Zeng, Oak wrote:
Thanks,
Oak
-Original Message-
From: Vishwanathapura, Niranjana
Sent: June 17, 2022 1:15 AM
To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vetter,
Daniel
Cc: Hellstrom, Thomas ; Wilson, Chris P
; ja...@jlekstrand.net;
christian.koe...@amd.com; Brost, Matthew ;
Ursulin, Tvrtko ; Auld, Matthew
; Landwerlin, Lionel G
; Zanoni, Paulo R
; Zeng, Oak
Subject: [PATCH v2 3/3] drm/doc/rfc: VM_BIND uapi definition
VM_BIND and related uapi definitions
v2: Reduce the scope to simple Mesa use case.
Signed-off-by: Niranjana Vishwanathapura
---
Documentation/gpu/rfc/i915_vm_bind.h | 226
+++
1 file changed, 226 insertions(+)
create mode 100644 Documentation/gpu/rfc/i915_vm_bind.h
diff --git a/Documentation/gpu/rfc/i915_vm_bind.h
b/Documentation/gpu/rfc/i915_vm_bind.h
new file mode 100644
index ..b7540ddb526d
--- /dev/null
+++ b/Documentation/gpu/rfc/i915_vm_bind.h
@@ -0,0 +1,226 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+/**
+ * DOC: I915_PARAM_HAS_VM_BIND
+ *
+ * VM_BIND feature availability.
+ * See typedef drm_i915_getparam_t param.
+ */
+#define I915_PARAM_HAS_VM_BIND 57
+
+/**
+ * DOC: I915_VM_CREATE_FLAGS_USE_VM_BIND
+ *
+ * Flag to opt-in for VM_BIND mode of binding during VM creation.
+ * See struct drm_i915_gem_vm_control flags.
+ *
+ * The older execbuf2 ioctl will not support VM_BIND mode of operation.
+ * For VM_BIND mode, we have new execbuf3 ioctl which will not accept
any
+ * execlist (See struct drm_i915_gem_execbuffer3 for more details).
+ *
+ */
+#define I915_VM_CREATE_FLAGS_USE_VM_BIND (1 << 0)
+
+/* VM_BIND related ioctls */
+#define DRM_I915_GEM_VM_BIND 0x3d
+#define DRM_I915_GEM_VM_UNBIND 0x3e
+#define DRM_I915_GEM_EXECBUFFER3 0x3f
+
+#define DRM_IOCTL_I915_GEM_VM_BIND
DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_BIND,
struct drm_i915_gem_vm_bind)
+#define DRM_IOCTL_I915_GEM_VM_UNBIND
DRM_IOWR(DRM_COMMAND_BASE +
DRM_I915_GEM_VM_UNBIND, struct drm_i915_gem_vm_bind)
+#define DRM_IOCTL_I915_GEM_EXECBUFFER3
DRM_IOWR(DRM_COMMAND_BASE +
DRM_I915_GEM_EXECBUFFER3, struct drm_i915_gem_execbuffer3)
+
+/**
+ * struct drm_i915_gem_vm_bind_fence - Bind/unbind completion
notification.
+ *
+ * A timeline out fence for vm_bind/unbind completion notification.
+ */
+struct drm_i915_gem_vm_bind_fence {
+ /** @handle: User's handle for a drm_syncobj to signal. */
+ __u32 handle;
+
+ /** @rsvd: Reserved, MBZ */
+ __u32 rsvd;
+
+ /**
+ * @value: A point in the timeline.
+ * Value must be 0 for a binary drm_syncobj. A Value of 0 for a
+ * timeline drm_syncobj is invalid as it turns a drm_syncobj into a
+ * binary one.
+ */
+ __u64 value;
+};
+
+/**
+ * struct drm_i915_gem_vm_bind - VA to object mapping to bind.
+ *
+ * This structure is passed to VM_BIND ioctl and specifies the mapping of
GPU
+ * virtual address (VA) range to the section of an object that should be
bound
+ * in the device page table of the specified address space (VM).
+ * The VA range specified must be unique (ie., not currently bound) and can
+ * be mapped to whole object or a section of the object (partial binding).
+ * Multiple VA mappings can be created to the same section of the object
+ * (aliasing).
+ *
+ * The @start, @offset and @length should be 4K page aligned. However
the DG2
+ * and XEHPSDV has 64K page size for device local-memory and has compact
page
+ * table. On those platforms, for binding device local-memory objects, the
+ * @start should be 2M aligned, @offset and @length should be 64K aligned.
+ * Also, on those platforms, it is not allowed to bind an device local-memory
+ * object and a system memory object in a single 2M section of VA range.
+ */
+struct drm_i915_gem_vm_bind {
+ /** @vm_id: VM (address space) id to bind */
+ __u32 vm_id;
+
+ /** @handle: Object handle */
+ __u32 handle;
+
+ /** @start: Virtual Address start to bind */
+ __u64 start;
+
+ /** @offset: Offset in object to bind */
+ __u64 offset;
+
+ /** @length: Length of mapping to bind */
+ __u64 length;
+
+ /**
+ * @flags: Supported flags are:
+ *
+ * I915_GEM_VM_BIND_READONLY:
+ * Mapping is read-only.
+ *
+ * I915_GEM_VM_BIND_CAPTURE:
+ * Capture this mapping in the dump upon GPU error.
+ */
+ __u64 flags;
+#define I915_GEM_VM_BIND_READONLY(1 << 0)
Should we define another flag for DEVICE_ATOMIC? Without this flag, do you
imply all the mapping support device atomic operation?
HW platform also has an implication to device atomic, i.e., some platform don't
support device atomics to system memory.
Thanks Oak, we can always add required flags later when we want to add the
support.
Niranjana
Regards,
Oak
+#define I915_GEM_VM_BIND_CAPTURE (1 << 1)
+
+ /** @fence: Ti
Re: [Intel-gfx] [PATCH v2] drm/i915: Fix vm use-after-free in vma destruction
On 20.06.2022 14:36, Thomas Hellström wrote:
In vma destruction, the following race may occur:
Thread 1: Thread 2:
i915_vma_destroy();
...
list_del_init(vma->vm_link);
...
mutex_unlock(vma->vm->mutex);
__i915_vm_release();
release_references();
And in release_reference() we dereference vma->vm to get to the
vm gt pointer, leading to a use-after free.
However, __i915_vm_release() grabs the vm->mutex so the vm won't be
destroyed before vma->vm->mutex is released, so extract the gt pointer
under the vm->mutex to avoid the vma->vm dereference in
release_references().
v2: Fix a typo in the commit message (Andi Shyti)
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/5944
Fixes: e1a7ab4fca ("drm/i915: Remove the vm open count")
Cc: Niranjana Vishwanathapura
Cc: Matthew Auld
Signed-off-by: Thomas Hellström
---
drivers/gpu/drm/i915/i915_vma.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
index 0bffb70b3c5f..04d12f278f57 100644
--- a/drivers/gpu/drm/i915/i915_vma.c
+++ b/drivers/gpu/drm/i915/i915_vma.c
@@ -1637,10 +1637,10 @@ static void force_unbind(struct i915_vma *vma)
GEM_BUG_ON(drm_mm_node_allocated(&vma->node));
}
-static void release_references(struct i915_vma *vma, bool vm_ddestroy)
+static void release_references(struct i915_vma *vma, struct intel_gt *gt,
+ bool vm_ddestroy)
{
struct drm_i915_gem_object *obj = vma->obj;
- struct intel_gt *gt = vma->vm->gt;
GEM_BUG_ON(i915_vma_is_active(vma));
@@ -1695,11 +1695,12 @@ void i915_vma_destroy_locked(struct i915_vma *vma)
force_unbind(vma);
list_del_init(&vma->vm_link);
- release_references(vma, false);
+ release_references(vma, vma->vm->gt, false);
}
void i915_vma_destroy(struct i915_vma *vma)
{
+ struct intel_gt *gt;
bool vm_ddestroy;
mutex_lock(&vma->vm->mutex);
@@ -1707,8 +1708,11 @@ void i915_vma_destroy(struct i915_vma *vma)
list_del_init(&vma->vm_link);
vm_ddestroy = vma->vm_ddestroy;
vma->vm_ddestroy = false;
+
+ /* vma->vm may be freed when releasing vma->vm->mutex. */
+ gt = vma->vm->gt;
mutex_unlock(&vma->vm->mutex);
- release_references(vma, vm_ddestroy);
+ release_references(vma, gt, vm_ddestroy);
Reviewed-by: Andrzej Hajda
Regards
Andrzej
}
void i915_vma_parked(struct intel_gt *gt)
[PATCH v4 0/3] KUnit tests for drm_format_helper
Hello everyone, Following the style used in the selftest to KUnit series [1] and the AMD series [2], the tests were moved to the "tests" folder. In addition, to be consistent naming functions, I renamed the kunit_suite and the test cases to use underscores as suggested in [3]. It is not clear yet whether we want to have one or multiple Kconfig symbols and select which test should be built. However, refactoring from one approach to the other is quite simple, so I think we should be fine choosing the simpler option now and refactoring if required. Thanks a lot, José Expósito [1] https://lore.kernel.org/dri-devel/20220615135824.15522-1-maira.ca...@usp.br/T/ [2] https://lore.kernel.org/dri-devel/20220608010709.272962-1-maira.ca...@usp.br/ [3] https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html RFC -> v1: https://lore.kernel.org/dri-devel/20220530102017.471865-1-jose.exposit...@gmail.com/T/ - Add .kunitconfig (Maxime Ripard) - Fix memory leak (Daniel Latypov) - Make config option generic (Javier Martinez Canillas): DRM_FORMAR_HELPER_TEST -> DRM_KUNIT_TEST - Remove DISABLE_STRUCTLEAK_PLUGIN (Daniel Latypov) v1 -> v2: https://lore.kernel.org/dri-devel/20220606095516.938934-1-jose.exposit...@gmail.com/T/ Thomas Zimmermann: - Add DRM_RECT_INIT() macro - Move tests to drivers/gpu/drm/kunit - Improve test documentation v2 -> v3: https://lore.kernel.org/dri-devel/20220612161248.271590-1-jose.exposit...@gmail.com/T/ - Use designated initializer in DRM_RECT_INIT (Jani Nikula) - Simplify the "conversion_buf_size" helper v3 -> v4: https://lore.kernel.org/dri-devel/20220616183852.GA12343@elementary/T/ - Move the source to the "tests" folder - Use "_" in kunit_suite and cases: https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html - Reviewed-by and Acked-by tags José Expósito (3): drm/rect: Add DRM_RECT_INIT() macro drm/format-helper: Add KUnit tests for drm_fb_xrgb_to_rgb332() drm/doc: Add KUnit documentation Documentation/gpu/drm-internals.rst | 32 drivers/gpu/drm/Kconfig | 16 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/tests/.kunitconfig| 3 + drivers/gpu/drm/tests/Makefile| 3 + .../gpu/drm/tests/drm_format_helper_test.c| 161 ++ include/drm/drm_rect.h| 16 ++ 7 files changed, 232 insertions(+) create mode 100644 drivers/gpu/drm/tests/.kunitconfig create mode 100644 drivers/gpu/drm/tests/Makefile create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c -- 2.25.1
[PATCH v4 1/3] drm/rect: Add DRM_RECT_INIT() macro
Add a helper macro to initialize a rectangle from x, y, width and
height information.
Reviewed-by: Jani Nikula
Acked-by: Thomas Zimmermann
Signed-off-by: José Expósito
---
include/drm/drm_rect.h | 16
1 file changed, 16 insertions(+)
diff --git a/include/drm/drm_rect.h b/include/drm/drm_rect.h
index 6f6e19bd4dac..e8d94fca2703 100644
--- a/include/drm/drm_rect.h
+++ b/include/drm/drm_rect.h
@@ -47,6 +47,22 @@ struct drm_rect {
int x1, y1, x2, y2;
};
+/**
+ * DRM_RECT_INIT - initialize a rectangle from x/y/w/h
+ * @x: x coordinate
+ * @y: y coordinate
+ * @w: width
+ * @h: height
+ *
+ * RETURNS:
+ * A new rectangle of the specified size.
+ */
+#define DRM_RECT_INIT(x, y, w, h) ((struct drm_rect){ \
+ .x1 = (x), \
+ .y1 = (y), \
+ .x2 = (x) + (w), \
+ .y2 = (y) + (h) })
+
/**
* DRM_RECT_FMT - printf string for &struct drm_rect
*/
--
2.25.1
[PATCH v4 2/3] drm/format-helper: Add KUnit tests for drm_fb_xrgb8888_to_rgb332()
Test the conversion from XRGB to RGB332.
What is tested?
- Different values for the X in XRGB to make sure it is ignored
- Different clip values: Single pixel and full and partial buffer
- Well known colors: White, black, red, green, blue, magenta, yellow
and cyan
- Other colors: Randomly picked
- Destination pitch
How to run the tests?
$ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \
--kconfig_add CONFIG_VIRTIO_UML=y \
--kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y
Suggested-by: Javier Martinez Canillas
Reviewed-by: Javier Martinez Canillas
Acked-by: Thomas Zimmermann
Signed-off-by: José Expósito
---
drivers/gpu/drm/Kconfig | 16 ++
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/tests/.kunitconfig| 3 +
drivers/gpu/drm/tests/Makefile| 3 +
.../gpu/drm/tests/drm_format_helper_test.c| 161 ++
5 files changed, 184 insertions(+)
create mode 100644 drivers/gpu/drm/tests/.kunitconfig
create mode 100644 drivers/gpu/drm/tests/Makefile
create mode 100644 drivers/gpu/drm/tests/drm_format_helper_test.c
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 22e7fa48d693..6c2256e8474b 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -70,6 +70,22 @@ config DRM_DEBUG_SELFTEST
If in doubt, say "N".
+config DRM_KUNIT_TEST
+ tristate "KUnit tests for DRM" if !KUNIT_ALL_TESTS
+ depends on DRM && KUNIT=y
+ select DRM_KMS_HELPER
+ default KUNIT_ALL_TESTS
+ help
+ This builds unit tests for DRM. This option is not useful for
+ distributions or general kernels, but only for kernel
+ developers working on DRM and associated drivers.
+
+ For more information on KUnit and unit tests in general,
+ please refer to the KUnit documentation in
+ Documentation/dev-tools/kunit/.
+
+ If in doubt, say "N".
+
config DRM_KMS_HELPER
tristate
depends on DRM
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 13ef240b3d2b..db8ffcf4e048 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o
#
obj-$(CONFIG_DRM_DEBUG_SELFTEST) += selftests/
+obj-$(CONFIG_DRM_KUNIT_TEST) += tests/
obj-$(CONFIG_DRM_MIPI_DBI) += drm_mipi_dbi.o
obj-$(CONFIG_DRM_MIPI_DSI) += drm_mipi_dsi.o
diff --git a/drivers/gpu/drm/tests/.kunitconfig
b/drivers/gpu/drm/tests/.kunitconfig
new file mode 100644
index ..6ec04b4c979d
--- /dev/null
+++ b/drivers/gpu/drm/tests/.kunitconfig
@@ -0,0 +1,3 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_KUNIT_TEST=y
diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
new file mode 100644
index ..2c8273796d9d
--- /dev/null
+++ b/drivers/gpu/drm/tests/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_DRM_KUNIT_TEST) += drm_format_helper_test.o
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c
b/drivers/gpu/drm/tests/drm_format_helper_test.c
new file mode 100644
index ..98583bf56044
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "../drm_crtc_internal.h"
+
+#define TEST_BUF_SIZE 50
+
+struct xrgb_to_rgb332_case {
+ const char *name;
+ unsigned int pitch;
+ unsigned int dst_pitch;
+ struct drm_rect clip;
+ const u32 xrgb[TEST_BUF_SIZE];
+ const u8 expected[4 * TEST_BUF_SIZE];
+};
+
+static struct xrgb_to_rgb332_case xrgb_to_rgb332_cases[] = {
+ {
+ .name = "single_pixel_source_buffer",
+ .pitch = 1 * 4,
+ .dst_pitch = 0,
+ .clip = DRM_RECT_INIT(0, 0, 1, 1),
+ .xrgb = { 0x01FF },
+ .expected = { 0xE0 },
+ },
+ {
+ .name = "single_pixel_clip_rectangle",
+ .pitch = 2 * 4,
+ .dst_pitch = 0,
+ .clip = DRM_RECT_INIT(1, 1, 1, 1),
+ .xrgb = {
+ 0x, 0x,
+ 0x, 0x10FF,
+ },
+ .expected = { 0xE0 },
+ },
+ {
+ /* Well known colors: White, black, red, green, blue, magenta,
+* yellow and cyan. Different values for the X in XRGB to
+* make sure it is ignored. Partial clip area.
+*/
+ .name = "well_known_colors",
+ .pitch = 4 * 4,
+ .dst_pitch = 0,
+ .clip = DRM_RECT_INIT(1, 1, 2, 4),
+ .xrgb = {
+ 0x, 0x, 0x, 0x,
+
[PATCH v4 3/3] drm/doc: Add KUnit documentation
Explain how to run the KUnit tests present in the DRM subsystem and clarify why the UML-only options were not added to the configuration file present in drivers/gpu/drm/.kunitconfig [1] [2]. [1] https://lore.kernel.org/dri-devel/CABVgOSn8i=lo5p7830h2xu1jgg0krn0qtnxkomhf1otgxja...@mail.gmail.com/ [2] https://lore.kernel.org/dri-devel/CAGS_qxqpiCim_sy1LDK7PLwVgWf-LKW+uNFTGM=t7ydk-dy...@mail.gmail.com/ Reviewed-by: Maxime Ripard Reviewed-by: Javier Martinez Canillas Acked-by: Thomas Zimmermann Signed-off-by: José Expósito --- Documentation/gpu/drm-internals.rst | 32 + 1 file changed, 32 insertions(+) diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst index 38afed24a75c..5fd20a306718 100644 --- a/Documentation/gpu/drm-internals.rst +++ b/Documentation/gpu/drm-internals.rst @@ -207,6 +207,38 @@ Utilities :internal: +Unit testing + + +KUnit +- + +KUnit (Kernel unit testing framework) provides a common framework for unit tests +within the Linux kernel. + +This section covers the specifics for the DRM subsystem. For general information +about KUnit, please refer to Documentation/dev-tools/kunit/start.rst. + +How to run the tests? +~ + +In order to facilitate running the test suite, a configuration file is present +in ``drivers/gpu/drm/tests/.kunitconfig``. It can be used by ``kunit.py`` as +follows: + +.. code-block:: bash + + $ ./tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/tests \ + --kconfig_add CONFIG_VIRTIO_UML=y \ + --kconfig_add CONFIG_UML_PCI_OVER_VIRTIO=y + +.. note:: + The configuration included in ``.kunitconfig`` should be as generic as + possible. + ``CONFIG_VIRTIO_UML`` and ``CONFIG_UML_PCI_OVER_VIRTIO`` are not + included in it because they are only required for User Mode Linux. + + Legacy Support Code === -- 2.25.1
Re: [Intel-gfx] [PATCH] drm/i915/display: Re-add check for low voltage sku for max dp source rate
On Mon, 20 Jun 2022, "Jason A. Donenfeld" wrote: > Hi Jani, > > Do you plan to merge this revert? Yes, I've done that now, thanks for the bisection and the patch. Ankit, Imre, we need to figure out what to do with [1] now. BR, Jani. [1] https://gitlab.freedesktop.org/drm/intel/-/issues/5272 -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915/display: Re-add check for low voltage sku for max dp source rate
Hi Jani, On Mon, Jun 20, 2022 at 07:10:30PM +0300, Jani Nikula wrote: > On Mon, 20 Jun 2022, "Jason A. Donenfeld" wrote: > > Hi Jani, > > > > Do you plan to merge this revert? > > Yes, I've done that now, thanks for the bisection and the patch. Thanks! I see that this went into `drm-intel-next`, but shouldn't it go into `drm-intel-fixes`, so that it makes it into 5.19-rc4? Jason
Re: [Intel-gfx] [PATCH] drm/i915/display: Re-add check for low voltage sku for max dp source rate
On Mon, 20 Jun 2022, "Jason A. Donenfeld" wrote: > Hi Jani, > > On Mon, Jun 20, 2022 at 07:10:30PM +0300, Jani Nikula wrote: >> On Mon, 20 Jun 2022, "Jason A. Donenfeld" wrote: >> > Hi Jani, >> > >> > Do you plan to merge this revert? >> >> Yes, I've done that now, thanks for the bisection and the patch. > > Thanks! > > I see that this went into `drm-intel-next`, but shouldn't it go into > `drm-intel-fixes`, so that it makes it into 5.19-rc4? All of our commits go to drm-intel-next first. I'll pick it up for fixes later. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH v2 1/3] drm/doc/rfc: VM_BIND feature design document
On Mon, Jun 20, 2022 at 11:43:10AM +0100, Tvrtko Ursulin wrote: Hi, On 17/06/2022 06:14, Niranjana Vishwanathapura wrote: VM_BIND design document with description of intended use cases. v2: Reduce the scope to simple Mesa use case. since I expressed interest please add me to cc when sending out. Hi Tvrtko, I did include you in the cc list with git send-email, but looks like some patches in this series has the full cc list, but some don't (you are on cc list of this patch though). I am not sure why. How come the direction changed to simplify all of a sudden? I did not spot any discussion to that effect. Was it internal talks? Yah, some of us had offline discussion involving the Mesa team. I did update the thread (previous version of this patch series) about that. Plan was to align our roadmap to focus on the deliverables at this point without further complicating the uapi. Signed-off-by: Niranjana Vishwanathapura --- Documentation/gpu/rfc/i915_vm_bind.rst | 238 + Documentation/gpu/rfc/index.rst| 4 + 2 files changed, 242 insertions(+) create mode 100644 Documentation/gpu/rfc/i915_vm_bind.rst diff --git a/Documentation/gpu/rfc/i915_vm_bind.rst b/Documentation/gpu/rfc/i915_vm_bind.rst new file mode 100644 index ..4ab590ef11fd --- /dev/null +++ b/Documentation/gpu/rfc/i915_vm_bind.rst @@ -0,0 +1,238 @@ +== +I915 VM_BIND feature design and use cases +== + +VM_BIND feature + +DRM_I915_GEM_VM_BIND/UNBIND ioctls allows UMD to bind/unbind GEM buffer +objects (BOs) or sections of a BOs at specified GPU virtual addresses on a +specified address space (VM). These mappings (also referred to as persistent +mappings) will be persistent across multiple GPU submissions (execbuf calls) +issued by the UMD, without user having to provide a list of all required +mappings during each submission (as required by older execbuf mode). + +The VM_BIND/UNBIND calls allow UMDs to request a timeline fence for signaling +the completion of bind/unbind operation. + +VM_BIND feature is advertised to user via I915_PARAM_HAS_VM_BIND. +User has to opt-in for VM_BIND mode of binding for an address space (VM) +during VM creation time via I915_VM_CREATE_FLAGS_USE_VM_BIND extension. + +Normally, vm_bind/unbind operations will get completed synchronously, To me synchronously, at this point in the text, reads as ioctl will return only when the operation is done. Rest of the paragraph however disagrees (plus existence of out fence). It is not clear to me what is the actual behaviour. Will it be clear to userspace developers reading uapi kerneldoc? If it is async, what are the ordering rules in this version? Yah, here I am simply stating the i915_vma_pin_ww() behavior which mostly does the binding synchronously unless there is a moving fence associated with the object in which case, binding will complete later once that fence is signaled (hence the out fence). +but if the object is being moved, the binding will happen once that the +moving is complete and out fence will be signaled after binding is complete. +The bind/unbind operation can get completed out of submission order. + +VM_BIND features include: + +* Multiple Virtual Address (VA) mappings can map to the same physical pages + of an object (aliasing). +* VA mapping can map to a partial section of the BO (partial binding). +* Support capture of persistent mappings in the dump upon GPU error. +* TLB is flushed upon unbind completion. Batching of TLB flushes in some + use cases will be helpful. +* Support for userptr gem objects (no special uapi is required for this). + +Execbuf ioctl in VM_BIND mode +--- +A VM in VM_BIND mode will not support older execbuf mode of binding. +The execbuf ioctl handling in VM_BIND mode differs significantly from the +older execbuf2 ioctl (See struct drm_i915_gem_execbuffer2). +Hence, a new execbuf3 ioctl has been added to support VM_BIND mode. (See +struct drm_i915_gem_execbuffer3). The execbuf3 ioctl will not accept any +execlist. Hence, no support for implicit sync. It is expected that the below +work will be able to support requirements of object dependency setting in all +use cases: + +"dma-buf: Add an API for exporting sync files" +(https://lwn.net/Articles/859290/) What does this mean? If execbuf3 does not know about target objects how can we add a meaningful fence? Execbuf3 does know about the target objects. It is all the objects bound to that VM via vm_bind call. + +The execbuf3 ioctl directly specifies the batch addresses instead of as +object handles as in execbuf2 ioctl. The execbuf3 ioctl will also not +support many of the older features like in/out/submit fences, fence array, +default gem context and many more (See struct drm_i915_gem_execbuffer3). + +In VM_BIND mode, VA allocation is completely managed by the user instead of +the i9
[pull] drm/msm: drm-msm-fixes-2022-06-20 for v5.19-rc4
Hi Dave, Here are fixes for v5.19, summary below (and in tag msg) The following changes since commit 24df12013853ac59c52cc726e9cbe51e38d09eda: MAINTAINERS: Add Dmitry as MSM DRM driver co-maintainer (2022-05-07 12:02:29 -0700) are available in the Git repository at: https://gitlab.freedesktop.org/drm/msm.git tags/drm-msm-fixes-2022-06-20 for you to fetch changes up to a6e2af64a79afa7f1b29375b5231e840a84bb845: drm/msm/dp: force link training for display resolution change (2022-06-18 09:14:06 -0700) Fixes for v5.19-rc4 - Workaround for parade DSI bridge power sequencing - Fix for multi-planar YUV format offsets - Limiting WB modes to max sspp linewidth - Fixing the supported rotations to add 180 back for IGT - Fix to handle pm_runtime_get_sync() errors to avoid unclocked access in the bind() path for dpu driver - Fix the irq_free() without request issue which was being hit frequently in CI. - Fix to add minimum ICC vote in the msm_mdss pm_resume path to address bootup splats - Fix to avoid dereferencing without checking in WB encoder - Fix to avoid crash during suspend in DP driver by ensuring interrupt mask bits are updated - Remove unused code from dpu_encoder_virt_atomic_check() - Fix to remove redundant init of dsc variable - Fix to ensure mmap offset is initialized to avoid memory corruption from unpin/evict - Fix double runpm disable in probe-defer path - VMA fenced-unpin fixes - Fix for WB max-width - Fix for rare dp resolution change issue Abhinav Kumar (4): drm/msm/dpu: limit writeback modes according to max_linewidth drm/msm/dpu: add DRM_MODE_ROTATE_180 back to supported rotations drm/msm/dpu: handle pm_runtime_get_sync() errors in bind path drm/msm/dpu: limit wb modes based on max_mixer_width Dmitry Baryshkov (1): drm/msm: don't free the IRQ if it was not requested Douglas Anderson (2): drm/msm/dsi: don't powerup at modeset time for parade-ps8640 drm/msm/dpu: Move min BW request and full BW disable back to mdss Hangyu Hua (1): drm: msm: fix possible memory leak in mdp5_crtc_cursor_set() Haowen Bai (1): drm/msm/dpu: Fix pointer dereferenced before checking Jiapeng Chong (1): drm/msm/dpu: Remove unused code Jonathan Marek (1): drm/msm: use for_each_sgtable_sg to iterate over scatterlist Kuogee Hsieh (3): drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl() drm/msm/dp: check core_initialized before disable interrupts at dp_display_unbind() drm/msm/dp: force link training for display resolution change Maximilian Luz (1): drm/msm: Fix double pm_runtime_disable() call Miaoqian Lin (2): drm/msm/a6xx: Fix refcount leak in a6xx_gpu_init drm/msm/mdp4: Fix refcount leak in mdp4_modeset_init_intf Rob Clark (9): drm/msm: Fix fb plane offset calculation Merge tag 'msm-next-5.19-fixes' of https://gitlab.freedesktop.org/abhinavk/msm into msm-fixes-staging Merge tag 'msm-next-5.19-fixes-06-01' of https://gitlab.freedesktop.org/abhinavk/msm into msm-fixes-staging drm/msm: Ensure mmap offset is initialized drm/msm: Switch ordering of runpm put vs devfreq_idle drm/msm/gem: Separate object and vma unpin drm/msm/gem: Drop early returns in close/purge vma drm/msm: Drop update_fences() drm/msm: Don't overwrite hw fence in hw_init Vinod Koul (1): drm/msm/disp/dpu1: remove superfluous init drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 1 + drivers/gpu/drm/msm/adreno/adreno_gpu.c| 14 -- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c| 3 -- .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_wb.c| 4 +- drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c| 12 ++--- drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 2 +- drivers/gpu/drm/msm/disp/dpu1/dpu_writeback.c | 13 - drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c | 2 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 4 +- drivers/gpu/drm/msm/dp/dp_ctrl.c | 42 drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +- drivers/gpu/drm/msm/dp/dp_display.c| 16 +++--- drivers/gpu/drm/msm/dsi/dsi_manager.c | 32 +++- drivers/gpu/drm/msm/msm_drv.c | 9 +++- drivers/gpu/drm/msm/msm_drv.h | 1 + drivers/gpu/drm/msm/msm_fb.c | 2 +- drivers/gpu/drm/msm/msm_fence.c| 8 +-- drivers/gpu/drm/msm/msm_gem.c | 7 ++- drivers/gpu/drm/msm/msm_gem.h | 11 +++-- drivers/gpu/drm/msm/msm_gem_prime.c| 15 ++ drivers/gpu/drm/msm/msm_gem_submit.c | 18 --- drivers/gpu/drm/msm/msm_gem_vma.c | 6 +-- drivers/gpu/drm/msm/msm_gpu.c
[PATCH v2] drm/sun4i: Add DMA mask and segment size
Kernel occasionally complains that there is mismatch in segment size
when trying to render HW decoded videos and rendering them directly with
sun4i DRM driver. Following message can be observed on H6 SoC:
[ 184.298308] [ cut here ]
[ 184.298326] DMA-API: sun4i-drm display-engine: mapping sg segment longer
than device claims to support [len=6144000] [max=65536]
[ 184.298364] WARNING: CPU: 1 PID: 382 at kernel/dma/debug.c:1162
debug_dma_map_sg+0x2b0/0x350
[ 184.322997] CPU: 1 PID: 382 Comm: ffmpeg Not tainted 5.19.0-rc1+ #1331
[ 184.329533] Hardware name: Tanix TX6 (DT)
[ 184.333544] pstate: 6005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 184.340512] pc : debug_dma_map_sg+0x2b0/0x350
[ 184.344882] lr : debug_dma_map_sg+0x2b0/0x350
[ 184.349250] sp : 89f33a50
[ 184.352567] x29: 89f33a50 x28: 0001 x27: 01b86c00
[ 184.359725] x26: x25: 05d8cc80 x24:
[ 184.366879] x23: 8939ab18 x22: 0001 x21: 0001
[ 184.374031] x20: x19: 018a7410 x18:
[ 184.381186] x17: x16: x15:
[ 184.388338] x14: 0001 x13: 89534e86 x12: 6f70707573206f74
[ 184.395493] x11: 20736d69616c6320 x10: 000a x9 : 0001
[ 184.402647] x8 : 893b6d40 x7 : 89f33850 x6 : 000c
[ 184.409800] x5 : bf997940 x4 : x3 : 0027
[ 184.416953] x2 : x1 : x0 : 03960e80
[ 184.424106] Call trace:
[ 184.426556] debug_dma_map_sg+0x2b0/0x350
[ 184.430580] __dma_map_sg_attrs+0xa0/0x110
[ 184.434687] dma_map_sgtable+0x28/0x4c
[ 184.438447] vb2_dc_dmabuf_ops_map+0x60/0xcc
[ 184.442729] __map_dma_buf+0x2c/0xd4
[ 184.446321] dma_buf_map_attachment+0xa0/0x130
[ 184.450777] drm_gem_prime_import_dev+0x7c/0x18c
[ 184.455410] drm_gem_prime_fd_to_handle+0x1b8/0x214
[ 184.460300] drm_prime_fd_to_handle_ioctl+0x2c/0x40
[ 184.465190] drm_ioctl_kernel+0xc4/0x174
[ 184.469123] drm_ioctl+0x204/0x420
[ 184.472534] __arm64_sys_ioctl+0xac/0xf0
[ 184.476474] invoke_syscall+0x48/0x114
[ 184.480240] el0_svc_common.constprop.0+0x44/0xec
[ 184.484956] do_el0_svc+0x2c/0xc0
[ 184.488283] el0_svc+0x2c/0x84
[ 184.491354] el0t_64_sync_handler+0x11c/0x150
[ 184.495723] el0t_64_sync+0x18c/0x190
[ 184.499397] ---[ end trace ]---
Fix that by setting DMA mask and segment size.
Signed-off-by: Jernej Skrabec
---
Changes from v1:
- added comment
- updated commit message with kernel report
drivers/gpu/drm/sun4i/sun4i_drv.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c
b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 275f7e4a03ae..f135a6b3cadb 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -7,6 +7,7 @@
*/
#include
+#include
#include
#include
#include
@@ -367,6 +368,13 @@ static int sun4i_drv_probe(struct platform_device *pdev)
INIT_KFIFO(list.fifo);
+ /*
+* DE2 and DE3 cores actually supports 40-bit addresses, but
+* driver does not.
+*/
+ dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+ dma_set_max_seg_size(&pdev->dev, DMA_BIT_MASK(32));
+
for (i = 0;; i++) {
struct device_node *pipeline = of_parse_phandle(np,
"allwinner,pipelines",
--
2.36.1
Re: [PATCH 1/4] video: fbdev: offb: Include missing linux/platform_device.h
On 6/11/22 18:50, Christophe Leroy wrote:
> A lot of drivers were getting platform and of headers
> indirectly via headers like asm/pci.h or asm/prom.h
>
> Most of them were fixed during 5.19 cycle but a newissue was
> introduced by commit 52b1b46c39ae ("of: Create platform devices
> for OF framebuffers")
>
> Include missing platform_device.h to allow cleaning asm/pci.h
>
> Cc: Thomas Zimmermann
> Fixes: 52b1b46c39ae ("of: Create platform devices for OF framebuffers")
> Signed-off-by: Christophe Leroy
Acked-by: Helge Deller
I assume you take this through the linuxppc git tree?
Helge
> ---
> drivers/video/fbdev/offb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
> index b1acb1ebebe9..91001990e351 100644
> --- a/drivers/video/fbdev/offb.c
> +++ b/drivers/video/fbdev/offb.c
> @@ -26,6 +26,7 @@
> #include
> #include
> #include
> +#include
> #include
>
> #ifdef CONFIG_PPC32
Re: [PATCH] video: fbdev: au1100fb: Check before some function not needed
On 6/8/22 13:43, Yihao Han wrote: > clk_disable() already checks the clk ptr using IS_ERR_OR_NULL(clk) > and clk_enable() checks the clk ptr using !clk, so there is no > need to check clk ptr again before calling them. > > Signed-off-by: Yihao Han applied to fbdev tree. Thanks! Helge > --- > drivers/video/fbdev/au1100fb.c | 6 ++ > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/fbdev/au1100fb.c b/drivers/video/fbdev/au1100fb.c > index 52f731a61482..519313b8bb00 100644 > --- a/drivers/video/fbdev/au1100fb.c > +++ b/drivers/video/fbdev/au1100fb.c > @@ -560,8 +560,7 @@ int au1100fb_drv_suspend(struct platform_device *dev, > pm_message_t state) > /* Blank the LCD */ > au1100fb_fb_blank(VESA_POWERDOWN, &fbdev->info); > > - if (fbdev->lcdclk) > - clk_disable(fbdev->lcdclk); > + clk_disable(fbdev->lcdclk); > > memcpy(&fbregs, fbdev->regs, sizeof(struct au1100fb_regs)); > > @@ -577,8 +576,7 @@ int au1100fb_drv_resume(struct platform_device *dev) > > memcpy(fbdev->regs, &fbregs, sizeof(struct au1100fb_regs)); > > - if (fbdev->lcdclk) > - clk_enable(fbdev->lcdclk); > + clk_enable(fbdev->lcdclk); > > /* Unblank the LCD */ > au1100fb_fb_blank(VESA_NO_BLANKING, &fbdev->info);
Re: [PATCH] fbdev: simplefb: Check before clk_put() not needed
On 6/2/22 12:50, Hans de Goede wrote:
> Hi,
>
> On 6/2/22 11:42, Yihao Han wrote:
>> clk_put() already checks the clk ptr using !clk and IS_ERR()
>> so there is no need to check it again before calling it.
>>
>> Signed-off-by: Yihao Han
>> ---
>> drivers/video/fbdev/simplefb.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c
>> index 2c198561c338..f96ce8801be4 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -237,8 +237,7 @@ static int simplefb_clocks_get(struct simplefb_par *par,
>> if (IS_ERR(clock)) {
>> if (PTR_ERR(clock) == -EPROBE_DEFER) {
>> while (--i >= 0) {
>> -if (par->clks[i])
>> -clk_put(par->clks[i]);
>> +clk_put(par->clks[i]);
>> }
>> kfree(par->clks);
>> return -EPROBE_DEFER;
>
> Thanks, patch looks good to me:
>
> Reviewed-by: Hans de Goede
applied to fbdev tree.
Thanks!
Helge
Re: [PATCH v2 07/15] Documentation: ABI: testing: mt6370: Add ADC sysfs guideline
On Mon, 20 Jun 2022 14:00:43 +0800 ChiaEn Wu wrote: > Hi Jonathan, > > Thanks for your helpful comments, and I have some questions want to > ask you below. > > Jonathan Cameron 於 2022年6月18日 週六 晚上11:39寫道： > > > > On Mon, 13 Jun 2022 19:11:38 +0800 > > ChiaEn Wu wrote: > > > > > From: ChiaEn Wu > > > > > > Add ABI documentation for mt6370 non-standard ADC sysfs interfaces. > > > > > > Signed-off-by: ChiaEn Wu > > > --- > > > .../ABI/testing/sysfs-bus-iio-adc-mt6370 | 36 +++ > > > 1 file changed, 36 insertions(+) > > > create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-mt6370 > > > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6370 > > > b/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6370 > > > new file mode 100644 > > > index ..039b3381176a > > > --- /dev/null > > > +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-mt6370 > > > @@ -0,0 +1,36 @@ > > > +What:/sys/bus/iio/devices/iio:deviceX/in_voltage0_raw > > > > Unfortunately the kernel documentation build scripts do no support > > duplicating > > standard ABI for particular devices so as to provide more information. > > Hence you can't have anything in this file. > > > > I want to confirm with you again, > because my ABI file duplicates with standard sysfs-bus-iio (voltage, > current, and temperature channels), > Should I just remove this ABI file and modify the code of mt6370-adc > to meet your expectations?? yes. > > > > > > +KernelVersion: 5.18 > > > +Contact: chiaen...@richtek.com > > > +Description: > > > + Indicated MT6370 VBUS ADC with lower accuracy(+-75mA) > > Curious though, voltage with a mA accuracy range? > > Yes, this description is based on the data sheet. Weird :) > > > This scale should be presented directly to userspace anyway so no need > > for this doc. > > > > > + higher measure range(1~22V) > > > + Calculating with scale returns voltage in uV > > > > No. All channels return in mV. That's the ABI requirement as > > in sysfs-bus-iio and we cannot vary if for particular drivers. If we did > > no generic tooling would work. > > Ok, I got it! > > > > > > + > > > +What:/sys/bus/iio/devices/iio:deviceX/in_voltage1_raw > > > +KernelVersion: 5.18 > > > +Contact: chiaen...@richtek.com > > > +Description: > > > + Indicated MT6370 VBUS ADC with higher accuracy(+-30mA) > > > + lower measure range(1~9.76V) > > > + Calculating with scale offset returns voltage in uV > > > + > > > +What:/sys/bus/iio/devices/iio:deviceX/in_voltage4_raw > > > +KernelVersion: 5.18 > > > +Contact: chiaen...@richtek.com > > > +Description: > > > + Indicated MT6370 TS_BAT ADC > > > + Calculating with scale returns voltage in uV > > > + > > > +What:/sys/bus/iio/devices/iio:deviceX/in_voltage7_raw > > > +KernelVersion: 5.18 > > > +Contact: chiaen...@richtek.com > > > +Description: > > > + Indicated MT6370 CHG_VDDP ADC > > > + Calculating with scale returns voltage in mV > > > + > > > +What:/sys/bus/iio/devices/iio:deviceX/in_temp8_raw > > > +KernelVersion: 5.18 > > > +Contact: chiaen...@richtek.com > > > +Description: > > > + Indicated MT6370 IC junction temperature > > > + Calculating with scale and offset returns temperature in > > > degree > > Shall I modify the scale of temperature to milli degrees in > mt6370-adc.c and remove this item?? yes. Thanks, Jonathan > > > > > Best regards, > ChiaEn Wu
[PATCH v2 0/2] per-process page tables for qcom 8250
This enable per-process page tables on the Qualcomm RB5 boards I'm setting up for Mesa CI. Has survived a full deqp-vk run. v2: moved qcom,adreno-smmu compatible earlier Emma Anholt (2): iommu: arm-smmu-impl: Add 8250 display compatible to the client list. arm64: dts: qcom: sm8250: Enable per-process page tables. arch/arm64/boot/dts/qcom/sm8250.dtsi | 2 +- drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) -- 2.36.1
[PATCH v2 1/2] iommu: arm-smmu-impl: Add 8250 display compatible to the client list.
Required for turning on per-process page tables for the GPU.
Signed-off-by: Emma Anholt
Reviewed-by: Konrad Dybcio
Reviewed-by: Dmitry Baryshkov
---
drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index d8e1ef83c01b..bb9220937068 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -233,6 +233,7 @@ static const struct of_device_id
qcom_smmu_client_of_match[] __maybe_unused = {
{ .compatible = "qcom,sc7280-mdss" },
{ .compatible = "qcom,sc7280-mss-pil" },
{ .compatible = "qcom,sc8180x-mdss" },
+ { .compatible = "qcom,sm8250-mdss" },
{ .compatible = "qcom,sdm845-mdss" },
{ .compatible = "qcom,sdm845-mss-pil" },
{ }
--
2.36.1
[PATCH v2 2/2] arm64: dts: qcom: sm8250: Enable per-process page tables.
This is an SMMU for the adreno gpu, and adding this compatible lets
the driver use per-fd page tables, which are required for security
between GPU clients.
Signed-off-by: Emma Anholt
Reviewed-by: Dmitry Baryshkov
---
v2: moved qcom,adreno-smmu earlier
arch/arm64/boot/dts/qcom/sm8250.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi
b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index a92230bec1dd..aae7b841b81a 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -2513,7 +2513,7 @@ gpucc: clock-controller@3d9 {
};
adreno_smmu: iommu@3da {
- compatible = "qcom,sm8250-smmu-500", "arm,mmu-500";
+ compatible = "qcom,sm8250-smmu-500",
"qcom,adreno-smmu", "arm,mmu-500";
reg = <0 0x03da 0 0x1>;
#iommu-cells = <2>;
#global-interrupts = <2>;
--
2.36.1
Re: [PATCH 09/64] drm/simple: Introduce drmm_simple_encoder_init
Hi Am 20.06.22 um 16:45 schrieb Thomas Zimmermann: Hi Am 20.06.22 um 16:39 schrieb Maxime Ripard: On Mon, Jun 20, 2022 at 04:25:38PM +0200, Thomas Zimmermann wrote: Hi Am 20.06.22 um 15:48 schrieb Maxime Ripard: Hi, On Mon, Jun 20, 2022 at 12:44:24PM +0200, Thomas Zimmermann wrote: Am 10.06.22 um 11:28 schrieb Maxime Ripard: The DRM-managed function to register an encoder is drmm_simple_encoder_alloc() and its variants, which will allocate the underlying structure and initialisation the encoder. However, we might want to separate the structure creation and the encoder initialisation, for example if the structure is shared across multiple DRM entities, for example an encoder and a connector. Let's create an helper to only initialise an encoder that would be passed as an argument. There's nothing wrong with this patch, but I have to admit that adding drm_simple_encoder_init() et al was a mistake. Why do you think it was a mistake? The simple-encoder stuff is an interface that no one really needs. Compared to open-coding the function, it's barely an improvement in LOCs, but nothing else. Back when I added drm_simple_encoder_init(), I wanted to simplify the many drivers that initialized the encoder with a cleanup callback and nothing else. IIRC it was an improvement back then. But now we already have a related drmm_ helper and a drmm_alloc_ helper. If I had only added the macro back then, we could use the regular encoder helpers. It would have been better to add an initializer macro like #define DRM_STATIC_ENCODER_DEFAULT_FUNCS \ .destroy = drm_encoder_cleanup It's way more flexible and similarly easy to use. Anyway... We can still have this. It would simplify this series so I could definitely squeeze that patch in and add a TODO item / deprecation notice for simple encoders if you think it's needed Not necessary. It's not super important. The corollary is though :) If I understand you right, it means that you'd rather have a destroy callback everywhere instead of calling the _cleanup function through a drm-managed callback, and let drm_dev_unregister do its job? If so, it means that we shouldn't be following the drmm_.*_alloc functions and should drop all the new ones from this series. No, no. What I'm saying is that simple-encoder is a pointless mid-layer. There's nothing that couldn't easily be achieved with the regular encoder functions. I guess that if you want to change something here, you could add drmm_encoder_init() instead and convert vc4. That function might be more useful for other drivers in the long run. Best regards Thomas Best regards Thomas Maxime -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Ivo Totev OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v2 4/4] drm/bridge: anx7625: Use DPI bus type
On Fri, 17 Jun 2022 at 12:32, Chen-Yu Tsai wrote:
>
> Hi,
>
> On Mon, May 23, 2022 at 4:37 PM Robert Foss wrote:
> >
> > On Mon, 23 May 2022 at 09:18, Chen-Yu Tsai wrote:
> > >
> > > On Mon, May 23, 2022 at 11:13 AM Xin Ji wrote:
> > > >
> > > > On Sat, May 21, 2022 at 06:28:42PM +0200, Daniel Vetter wrote:
> > > > > On Sat, 21 May 2022 at 18:07, Daniel Vetter wrote:
> > > > > >
> > > > > > On Tue, 17 May 2022 at 18:09, Robert Foss
> > > > > > wrote:
> > > > > > >
> > > > > > > On Mon, 25 Apr 2022 at 11:14, Xin Ji
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Mon, Apr 25, 2022 at 04:24:50PM +0800, Chen-Yu Tsai wrote:
> > > > > > > > > On Fri, Apr 22, 2022 at 10:13 PM Robert Foss
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > On Fri, 22 Apr 2022 at 16:01, Robert Foss
> > > > > > > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Fri, 22 Apr 2022 at 10:49, Xin Ji
> > > > > > > > > > > wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > As V4L2_FWNODE_BUS_TYPE_PARALLEL not properly descript
> > > > > > > > > > > > for DPI
> > > > > > > > > > > > interface, this patch use new defined
> > > > > > > > > > > > V4L2_FWNODE_BUS_TYPE_DPI for it.
> > > > > > > > > > > >
> > > > > > > > > > > > Fixes: fd0310b6fe7d ("drm/bridge: anx7625: add MIPI DPI
> > > > > > > > > > > > input feature")
> > > > > > > > > > > > Signed-off-by: Xin Ji
> > > > > > > > > > > > ---
> > > > > > > > > > > > drivers/gpu/drm/bridge/analogix/anx7625.c | 8
> > > > > > > > > > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > > > > > > > > > >
> > > > > > > > > > > > diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > > > > > b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > > > > > index 376da01243a3..71df977e8f53 100644
> > > > > > > > > > > > --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > > > > > +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> > > > > > > > > > > > @@ -1623,14 +1623,14 @@ static int
> > > > > > > > > > > > anx7625_parse_dt(struct device *dev,
> > > > > > > > > > > >
> > > > > > > > > > > > anx7625_get_swing_setting(dev, pdata);
> > > > > > > > > > > >
> > > > > > > > > > > > - pdata->is_dpi = 1; /* default dpi mode */
> > > > > > > > > > > > + pdata->is_dpi = 0; /* default dsi mode */
> > > > > > > > > > > > pdata->mipi_host_node =
> > > > > > > > > > > > of_graph_get_remote_node(np, 0, 0);
> > > > > > > > > > > > if (!pdata->mipi_host_node) {
> > > > > > > > > > > > DRM_DEV_ERROR(dev, "fail to get
> > > > > > > > > > > > internal panel.\n");
> > > > > > > > > > > > return -ENODEV;
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > - bus_type = V4L2_FWNODE_BUS_TYPE_PARALLEL;
> > > > > > > > > > > > + bus_type = 0;
> > > > > > > > > > > > mipi_lanes = MAX_LANES_SUPPORT;
> > > > > > > > > > > > ep0 = of_graph_get_endpoint_by_regs(np, 0, 0);
> > > > > > > > > > > > if (ep0) {
> > > > > > > > > > > > @@ -1640,8 +1640,8 @@ static int
> > > > > > > > > > > > anx7625_parse_dt(struct device *dev,
> > > > > > > > > > > > mipi_lanes =
> > > > > > > > > > > > of_property_count_u32_elems(ep0, "data-lanes");
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > - if (bus_type == V4L2_FWNODE_BUS_TYPE_PARALLEL)
> > > > > > > > > > > > /* bus type is Parallel(DSI) */
> > > > > > > > > > > > - pdata->is_dpi = 0;
> > > > > > > > > > > > + if (bus_type == V4L2_FWNODE_BUS_TYPE_DPI) /*
> > > > > > > > > > > > bus type is DPI */
> > > > > > > > > > > > + pdata->is_dpi = 1;
> > > > > > > > > > > >
> > > > > > > > > > > > pdata->mipi_lanes = mipi_lanes;
> > > > > > > > > > > > if (pdata->mipi_lanes > MAX_LANES_SUPPORT ||
> > > > > > > > > > > > pdata->mipi_lanes <= 0)
> > > > > > > > > > >
> > > > > > > > > > > Reviewed-by: Robert Foss
> > > > > > > > > >
> > > > > > > > > > Acked-by: Robert Foss
> > > > > > > > >
> > > > > > > > > Tested-by: Chen-Yu Tsai
> > > > > > > > >
> > > > > > > > > Confirmed this fixes the display on Juniper (Acer Chromebook
> > > > > > > > > Spin 311) on
> > > > > > > > > mainline (next-20220422).
> > > > > > > > >
> > > > > > > > > Xin, in the future, please send the whole series to all
> > > > > > > > > recipients of
> > > > > > > > > all patches listed by get_maintainers.pl, not just the
> > > > > > > > > recipients of
> > > > > > > > > each patch. In the case of this series, they should have been
> > > > > > > > > sent
> > > > > > > > > to all of the mailing lists (media, devicetree, dri-devel) so
> > > > > > > > > that
> > > > > > > > > everyone has the same, full view of the patches.
> > > > > > > > Hi ChenYu, OK, I'll send to all media, devicetree, dri-devel
> > > > > > > > next time.
> > > > > > > > Thanks,
> > > > > > > > Xin
> > > > > > >
Re: [PATCH v9 00/14] Add some DRM bridge drivers support for i.MX8qm/qxp SoCs
On Thu, 16 Jun 2022 at 13:18, Sakari Ailus wrote: > > On Sat, Jun 11, 2022 at 10:14:07PM +0800, Liu Ying wrote: > > Patch 1/14 and 2/14 add bus formats used by pixel combiner. > > Thanks! > > For these: > > Acked-by: Sakari Ailus Applied to drm-misc-next.
Re: (subset) [PATCH 1/2] dt-bindings: leds: qcom-wled: fix number of addresses
On Thu, 5 May 2022 17:47:01 +0200, Krzysztof Kozlowski wrote: > On PM660L, PMI8994 and PMI8998, the WLED has two address spaces. This > also fixes dtbs_check warnings like: > > arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dtb: leds@d800: reg: > [[55296], [2]] is too long > > Applied, thanks! [1/2] dt-bindings: leds: qcom-wled: fix number of addresses https://git.kernel.org/krzk/linux-dt/c/ba52039325826b3f2bddd00972f3f61fbe7d9f0e Best regards, -- Krzysztof Kozlowski
Re: [PATCH 0/2] Fixes for TC358775 DSI to LVDS bridge
On Thu, 16 Jun 2022 at 00:25, Jiri Vanek wrote: > > This patchset fixes two bugs in the driver for TC358775 DSI to LVDS bridge. > > Jiri Vanek (2): > drm/bridge/tc358775: Return before displaying inappropriate error > message > drm/bridge/tc358775: Fix DSI clock division for vsync delay > calculation > > drivers/gpu/drm/bridge/tc358775.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > Applied to drm-misc-next.
Re: [PATCH v13 0/3] eDP/DP Phy vdda realted function
On 6/16/2022 5:02 PM, Vinod Koul wrote: On 25-05-22, 14:02, Kuogee Hsieh wrote: 1) add regulator_set_load() to eDP phy 2) add regulator_set_load() to DP phy 3) remove vdda related function out of eDP/DP controller Kuogee Hsieh (3): phy: qcom-edp: add regulator_set_load to edp phy phy: qcom-qmp: add regulator_set_load to dp phy drm/msm/dp: delete vdda regulator related functions from eDP/DP controller drivers/gpu/drm/msm/dp/dp_parser.c | 14 -- drivers/gpu/drm/msm/dp/dp_parser.h | 8 drivers/gpu/drm/msm/dp/dp_power.c | 95 + drivers/phy/qualcomm/phy-qcom-edp.c | 12 + drivers/phy/qualcomm/phy-qcom-qmp.c | 40 Please rebase this to phy-next and apply to specific qmp phy driver... I will rebase to ==> https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git
Re: [PATCH v2 1/2] drm/bridge: ti-sn65dsi83: add more dev_err_probe
On Tue, 14 Jun 2022 at 11:58, Alexander Stein wrote: > > Add more warning/debug messages during probe. E.g. a single -EPROBE_DEFER > might have several causes, these messages help finding the origin. > > Signed-off-by: Alexander Stein > --- > * New in v2 > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > index b27c0d7c451a..a306150a8027 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > @@ -677,7 +677,7 @@ static int sn65dsi83_probe(struct i2c_client *client, > ctx->enable_gpio = devm_gpiod_get_optional(ctx->dev, "enable", >GPIOD_OUT_LOW); > if (IS_ERR(ctx->enable_gpio)) > - return PTR_ERR(ctx->enable_gpio); > + return dev_err_probe(dev, PTR_ERR(ctx->enable_gpio), "failed > to get enable GPIO\n"); > > usleep_range(1, 11000); > > @@ -687,7 +687,7 @@ static int sn65dsi83_probe(struct i2c_client *client, > > ctx->regmap = devm_regmap_init_i2c(client, &sn65dsi83_regmap_config); > if (IS_ERR(ctx->regmap)) > - return PTR_ERR(ctx->regmap); > + return dev_err_probe(dev, PTR_ERR(ctx->regmap), "failed to > get regmap\n"); > > dev_set_drvdata(dev, ctx); > i2c_set_clientdata(client, ctx); > -- > 2.25.1 > Reviewed & applied series to drm-misc-next. Reviewed-by: Robert Foss
[PATCH v14 0/3] eDP/DP Phy vdda realted function
0) rebase on https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git tree 1) add regulator_set_load() to eDP phy 2) add regulator_set_load() to DP phy 3) remove vdda related function out of eDP/DP controller Kuogee Hsieh (3): phy: qcom-edp: add regulator_set_load to edp phy phy: qcom-qmp: add regulator_set_load to dp phy drm/msm/dp: delete vdda regulator related functions from eDP/DP controller drivers/gpu/drm/msm/dp/dp_parser.c | 14 -- drivers/gpu/drm/msm/dp/dp_parser.h | 8 drivers/gpu/drm/msm/dp/dp_power.c | 95 + drivers/phy/qualcomm/phy-qcom-edp.c | 12 + drivers/phy/qualcomm/phy-qcom-qmp.c | 40 5 files changed, 45 insertions(+), 124 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v14 1/3] phy: qcom-edp: add regulator_set_load to edp phy
This patch add regulator_set_load() before enable regulator at
eDP phy driver.
Signed-off-by: Kuogee Hsieh
Reviewed-by: Douglas Anderson
---
drivers/phy/qualcomm/phy-qcom-edp.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c
b/drivers/phy/qualcomm/phy-qcom-edp.c
index cacd32f..7e357078 100644
--- a/drivers/phy/qualcomm/phy-qcom-edp.c
+++ b/drivers/phy/qualcomm/phy-qcom-edp.c
@@ -639,6 +639,18 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
if (ret)
return ret;
+ ret = regulator_set_load(edp->supplies[0].consumer, 21800); /* 1.2 V
vdda-phy */
+ if (ret) {
+ dev_err(dev, "failed to set load at %s\n",
edp->supplies[0].supply);
+ return ret;
+ }
+
+ ret = regulator_set_load(edp->supplies[1].consumer, 36000); /* 0.9 V
vdda-pll */
+ if (ret) {
+ dev_err(dev, "failed to set load at %s\n",
edp->supplies[1].supply);
+ return ret;
+ }
+
ret = qcom_edp_clks_register(edp, pdev->dev.of_node);
if (ret)
return ret;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v14 2/3] phy: qcom-qmp: add regulator_set_load to dp phy
This patch add regulator_set_load() before enable regulator at
DP phy driver.
Signed-off-by: Kuogee Hsieh
Reviewed-by: Stephen Boyd
Reviewed-by: Douglas Anderson
---
drivers/phy/qualcomm/phy-qcom-qmp.c | 40 -
1 file changed, 31 insertions(+), 9 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
b/drivers/phy/qualcomm/phy-qcom-qmp.c
index c7309e981..e5836ea 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -3119,6 +3119,17 @@ static const struct qmp_phy_init_tbl
sm8450_qmp_gen4x2_pcie_pcs_misc_tbl[] = {
QMP_PHY_INIT_CFG(QPHY_V5_20_PCS_PCIE_G4_PRE_GAIN, 0x2e),
};
+/* list of regulators */
+struct qmp_regulator_data {
+ const char *name;
+ unsigned int enable_load;
+};
+
+struct qmp_regulator_data qmp_phy_vreg_l[] = {
+ { .name = "vdda-phy", .enable_load = 21800 },
+ { .name = "vdda-pll", .enable_load = 36000 },
+};
+
struct qmp_phy;
/* struct qmp_phy_cfg - per-PHY initialization config */
@@ -3173,7 +3184,7 @@ struct qmp_phy_cfg {
const char * const *reset_list;
int num_resets;
/* regulators to be requested */
- const char * const *vreg_list;
+ const struct qmp_regulator_data *vreg_list;
int num_vregs;
/* array of registers with different offsets */
@@ -3385,11 +3396,6 @@ static const char * const sdm845_pciephy_reset_l[] = {
"phy",
};
-/* list of regulators */
-static const char * const qmp_phy_vreg_l[] = {
- "vdda-phy", "vdda-pll",
-};
-
static const struct qmp_phy_cfg ipq8074_usb3phy_cfg = {
.type = PHY_TYPE_USB3,
.nlanes = 1,
@@ -5561,16 +5567,32 @@ static int qcom_qmp_phy_vreg_init(struct device *dev,
const struct qmp_phy_cfg *
{
struct qcom_qmp *qmp = dev_get_drvdata(dev);
int num = cfg->num_vregs;
- int i;
+ int ret, i;
qmp->vregs = devm_kcalloc(dev, num, sizeof(*qmp->vregs), GFP_KERNEL);
if (!qmp->vregs)
return -ENOMEM;
for (i = 0; i < num; i++)
- qmp->vregs[i].supply = cfg->vreg_list[i];
+ qmp->vregs[i].supply = cfg->vreg_list[i].name;
- return devm_regulator_bulk_get(dev, num, qmp->vregs);
+ ret = devm_regulator_bulk_get(dev, num, qmp->vregs);
+ if (ret) {
+ dev_err(dev, "failed at devm_regulator_bulk_get\n");
+ return ret;
+ }
+
+ for (i = 0; i < num; i++) {
+ ret = regulator_set_load(qmp->vregs[i].consumer,
+cfg->vreg_list[i].enable_load);
+ if (ret) {
+ dev_err(dev, "failed to set load at %s\n",
+ qmp->vregs[i].supply);
+ return ret;
+ }
+ }
+
+ return 0;
}
static int qcom_qmp_phy_reset_init(struct device *dev, const struct
qmp_phy_cfg *cfg)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v14 3/3] drm/msm/dp: delete vdda regulator related functions from eDP/DP controller
Vdda regulators are related to both eDP and DP phy so that it should be
managed at eDP and DP phy driver instead of controller. This patch removes
vdda regulators related functions out of eDP/DP controller.
Signed-off-by: Kuogee Hsieh
Reviewed-by: Stephen Boyd
Reviewed-by: Dmitry Baryshkov
Reviewed-by: Douglas Anderson
---
drivers/gpu/drm/msm/dp/dp_parser.c | 14 --
drivers/gpu/drm/msm/dp/dp_parser.h | 8
drivers/gpu/drm/msm/dp/dp_power.c | 95 +-
3 files changed, 2 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c
b/drivers/gpu/drm/msm/dp/dp_parser.c
index 8f9fed9..4ef2130 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -22,14 +22,6 @@
#define DP_DEFAULT_P0_OFFSET 0x1000
#define DP_DEFAULT_P0_SIZE 0x0400
-static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
- .num = 2,
- .regs = {
- {"vdda-1p2", 21800, 4 },/* 1.2 V */
- {"vdda-0p9", 36000, 32 }, /* 0.9 V */
- },
-};
-
static void __iomem *dp_ioremap(struct platform_device *pdev, int idx, size_t
*len)
{
struct resource *res;
@@ -298,12 +290,6 @@ static int dp_parser_parse(struct dp_parser *parser)
if (rc)
return rc;
- /* Map the corresponding regulator information according to
-* version. Currently, since we only have one supported platform,
-* mapping the regulator directly.
-*/
- parser->regulator_cfg = &sdm845_dp_reg_cfg;
-
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h
b/drivers/gpu/drm/msm/dp/dp_parser.h
index 3a4d797..47430e3 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -92,8 +92,6 @@ struct dp_pinctrl {
struct pinctrl_state *state_suspend;
};
-#define DP_DEV_REGULATOR_MAX 4
-
/* Regulators for DP devices */
struct dp_reg_entry {
char name[32];
@@ -101,11 +99,6 @@ struct dp_reg_entry {
int disable_load;
};
-struct dp_regulator_cfg {
- int num;
- struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
-};
-
/**
* struct dp_parser - DP parser's data exposed to clients
*
@@ -121,7 +114,6 @@ struct dp_parser {
struct dp_pinctrl pinctrl;
struct dp_io io;
struct dp_display_data disp_data;
- const struct dp_regulator_cfg *regulator_cfg;
u32 max_dp_lanes;
struct drm_bridge *next_bridge;
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c
b/drivers/gpu/drm/msm/dp/dp_power.c
index d9e0117..b52ac1d 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -20,82 +20,10 @@ struct dp_power_private {
struct clk *link_clk_src;
struct clk *pixel_provider;
struct clk *link_provider;
- struct regulator_bulk_data supplies[DP_DEV_REGULATOR_MAX];
struct dp_power dp_power;
};
-static void dp_power_regulator_disable(struct dp_power_private *power)
-{
- struct regulator_bulk_data *s = power->supplies;
- const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
- int num = power->parser->regulator_cfg->num;
- int i;
-
- DBG("");
- for (i = num - 1; i >= 0; i--)
- if (regs[i].disable_load >= 0)
- regulator_set_load(s[i].consumer,
- regs[i].disable_load);
-
- regulator_bulk_disable(num, s);
-}
-
-static int dp_power_regulator_enable(struct dp_power_private *power)
-{
- struct regulator_bulk_data *s = power->supplies;
- const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
- int num = power->parser->regulator_cfg->num;
- int ret, i;
-
- DBG("");
- for (i = 0; i < num; i++) {
- if (regs[i].enable_load >= 0) {
- ret = regulator_set_load(s[i].consumer,
-regs[i].enable_load);
- if (ret < 0) {
- pr_err("regulator %d set op mode failed, %d\n",
- i, ret);
- goto fail;
- }
- }
- }
-
- ret = regulator_bulk_enable(num, s);
- if (ret < 0) {
- pr_err("regulator enable failed, %d\n", ret);
- goto fail;
- }
-
- return 0;
-
-fail:
- for (i--; i >= 0; i--)
- regulator_set_load(s[i].consumer, regs[i].disable_load);
- return ret;
-}
-
-static int dp_power_regulator_init(struct dp_power_private *power)
-{
- struct regulator_bulk_data *s = power->supplies;
- const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
- struct platform_device *pdev = power->pdev;
- int num = power->parser->regulator_cfg->num;
- int i, ret;
-
- for (i = 0; i < num; i++)
-
Re: [PATCH v14 1/3] phy: qcom-edp: add regulator_set_load to edp phy
On 20/06/2022 23:12, Kuogee Hsieh wrote: This patch add regulator_set_load() before enable regulator at eDP phy driver. Signed-off-by: Kuogee Hsieh Reviewed-by: Douglas Anderson Reviewed-by: Dmitry Baryshkov -- With best wishes Dmitry
Re: [PATCH v14 2/3] phy: qcom-qmp: add regulator_set_load to dp phy
On 20/06/2022 23:12, Kuogee Hsieh wrote: This patch add regulator_set_load() before enable regulator at DP phy driver. Signed-off-by: Kuogee Hsieh Reviewed-by: Stephen Boyd Reviewed-by: Douglas Anderson --- drivers/phy/qualcomm/phy-qcom-qmp.c | 40 - This was not rebased. There is no phy-qcom-qmp.c in phy-next. -- With best wishes Dmitry
