Re: [PATCH 0/3] drm/amd/display/dc/dce: remove some not used variables
Hi Zheng, Harry's previous comment about the superfluous REG_READ()s is still unaddressed. Once that's fixed, I can give my r-b. Thanks, Leo On 2019-10-28 5:32 a.m., zhengbin (A) wrote: > ping > > On 2019/10/9 14:25, zhengbin wrote: >> zhengbin (3): >> drm/amd/display: Remove set but not used variables >> 'bl_pwm_cntl','pwm_period_cntl' >> drm/amd/display: Remove set but not used variable 'value0' >> drm/amd/display: Remove set but not used variables 'regval','speakers' >> >> drivers/gpu/drm/amd/display/dc/dce/dce_abm.c| 8 >> drivers/gpu/drm/amd/display/dc/dce/dce_link_encoder.c | 3 +-- >> drivers/gpu/drm/amd/display/dc/dce/dce_stream_encoder.c | 5 + >> 3 files changed, 6 insertions(+), 10 deletions(-) >> >> -- >> 2.7.4 >> >> >> . >> > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/amdgpu: Add DC feature mask to disable fractional pwm
On 2019-10-23 10:19 a.m., Lukáš Krejčí wrote: > On Mon, 2019-10-21 at 15:44 -0400, sunpeng...@amd.com wrote: >> From: Leo Li >> >> [Why] >> >> Some LED panel drivers might not like fractional PWM. In such cases, >> backlight flickering may be observed. >> >> [How] >> >> Add a DC feature mask to disable fractional PWM, and associate it with >> the preexisting dc_config flag. >> >> The flag is only plumbed through the dmcu firmware, so plumb it through >> the driver path as well. >> >> To disable, add the following to the linux cmdline: >> amdgpu.dcfeaturemask=0x4 >> >> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204957 >> Signed-off-by: Leo Li >> --- >> >> v2: Add bugzilla link >> >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 3 +++ >> drivers/gpu/drm/amd/display/dc/dce/dce_abm.c | 4 >> drivers/gpu/drm/amd/include/amd_shared.h | 1 + >> 3 files changed, 8 insertions(+) > > Tested on Ubuntu 19.04, vanilla v5.3.7 kernel with the patch applied > and amdgpu.dcfeaturemask=0x4 added to the kernel command line, no > issues after 8 reboots. (The issue could be reproduced without > amdgpu.dcfeaturemask=0x4 on first boot.) > > Tested-by: Lukáš Krejčí Applied, thanks! Leo ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu/display: fix compile error
This has already been fixed here: https://patchwork.freedesktop.org/patch/336195/ Should be mirrored on Alex's tree soon. Thanks, Leo On 2019-10-17 2:19 a.m., Chen Wandun wrote: > From: Chenwandun > > drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_resource.c:1913:48: > error: struct dc_crtc_timing_flags has no member named DSC >if (res_ctx->pipe_ctx[i].stream->timing.flags.DSC) > ^ > drivers/gpu/drm/amd/amdgpu/../display/dc/dcn20/dcn20_resource.c:1914:73: > error: struct dc_crtc_timing has no member named dsc_cfg >pipes[pipe_cnt].dout.output_bpp = > res_ctx->pipe_ctx[i].stream->timing.dsc_cfg.bits_per_pixel / 16.0; > ^ > Signed-off-by: Chenwandun > --- > drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > index 914e378..4f03318 100644 > --- a/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > +++ b/drivers/gpu/drm/amd/display/dc/dcn20/dcn20_resource.c > @@ -1910,8 +1910,10 @@ int dcn20_populate_dml_pipes_from_context( > pipes[pipe_cnt].dout.output_bpp = output_bpc * 3; > } > > +#ifdef CONFIG_DRM_AMD_DC_DSC_SUPPORT > if (res_ctx->pipe_ctx[i].stream->timing.flags.DSC) > pipes[pipe_cnt].dout.output_bpp = > res_ctx->pipe_ctx[i].stream->timing.dsc_cfg.bits_per_pixel / 16.0; > +#endif > > /* todo: default max for now, until there is logic reflecting > this in dc*/ > pipes[pipe_cnt].dout.output_bpc = 12; > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH RESEND] drm/amd/display: Add back missing backlight level rounding
Thanks for the detailed notes! See reply inline.
On 2019-10-15 4:03 p.m., Lukáš Krejčí wrote:
> [Why]
> Having the rounding of the backlight value restored to the way it was
> seemingly gets rid of backlight flickering on certain Stoney Ridge
> laptops.
>
> [How]
> Rescale the backlight level between min and max input signal value and
> round it to a number between 0x0 and 0xFF. Then, use the rounding mode
> that was previously in driver_set_backlight_level() and
> dmcu_set_backlight_level(), i.e. rescale the backlight level between 0x0
> and 0x1000 by multiplying it by 0x101 and use the most significant bit
> of the fraction (or in this case the 8th bit of the value because it's
> the same thing, e.g. C3 * 0x101 = 0xC3C3 and C3 * 0x10101 = 0xC3C3C3) to
> round it.
>
> Fixes: 262485a50fd4 ("drm/amd/display: Expand dc to use 16.16 bit backlight")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204957
> Signed-off-by: Lukáš Krejčí
> ---
>
> Notes:
> Bug:
> - Can be reproduced on HP 15-rb020nc (Stoney Ridge R2 E2-9000e APU) and
> Acer Aspire 3 A315-21G-91JL (Stoney Ridge R3 A4-9120 APU).
>
> - For me, the bug is inconsistent - it does not happen on every boot,
> but if it does happen, it's usually within three minutes after bootup.
>
> - It concerns the backlight. That means it can be reproduced on the
> framebuffer console.
>
> - Reproduces on Arch Linux (custom built) live CD, linux kernel v5.3.2
> and Ubuntu 19.04, kernel-ppa/mainline v5.0.0-rc1, v5.0.0, v5.3.2,
> v5.3.4,
> v5.4-rc2.
>
> - Can not be reproduced on kernel v5.3.4 with this patch applied or on
> v4.20.0, 4.20.17, 4.19.75 (this bug is a regression).
>
> - The other person that reproduced the issue (see the Bugzilla link
> above) confirmed that the patch works for them too.
>
> Patch:
> - Is the comment modified by this commit correct? Both
> driver_set_backlight_level() and dmcu_set_backlight_level() check the
> 17th bit of `brightness` aka `backlight_pwm_u16_16`, but
> 262485a50fd4532a8d71165190adc7a0a19bcc9e ("drm/amd/display: Expand dc
> to use 16.16 bit backlight") specifically mentions 0x as the max
> backlight value>
> - use_smooth_brightness is false (no DMCU firmware available) on my
> laptop, so the other code path (dmcu_set_backlight_level()) is
> untested.
>
> - I'm not sure why the rounding fixes the issue and whether this
> function is the right place to add back the rounding (and whether it
> even is the right way to solve the issue), so that's why this patch is
> RFC.
We've seen some similar issues when fractional duty cycles are enabled for
backlight pwm.
I attached a hack to the bugzilla ticket that disables it, please give that
patch a shot
first. I'd rather not change this calculation for all panels if the flickering
issue is
only a quirk for some.
Thanks,
Leo
>
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index a52f0b13a2c8..af9a5f46b671 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2083,17 +2083,22 @@ static int amdgpu_dm_backlight_update_status(struct
> backlight_device *bd)
>* The brightness input is in the range 0-255
>* It needs to be rescaled to be between the
>* requested min and max input signal
> - *
> - * It also needs to be scaled up by 0x101 to
> - * match the DC interface which has a range of
> - * 0 to 0x
>*/
> brightness =
> brightness
> - * 0x101
> + * 0x100
> * (caps.max_input_signal - caps.min_input_signal)
> / AMDGPU_MAX_BL_LEVEL
> - + caps.min_input_signal * 0x101;
> + + caps.min_input_signal * 0x100;
> +
> + brightness = (brightness >> 8) + ((brightness >> 7) & 1);
> + /*
> + * It also needs to be scaled up by 0x101 and
> + * rounded off to match the DC interface which
> + * has a range of 0 to 0x1
> + */
> + brightness *= 0x101;
> + brightness += (brightness >> 7) & 1;
>
> if (dc_link_set_backlight_level(dm->backlight_link,
> brightness, 0))
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 09/14] drm/dp-mst: Export symbols for dpcd read/write
On 2019-08-20 5:02 p.m., Lyude Paul wrote:
> [Added Leo Li here, since they did the auxdev work that introduced these
> functions]
>
> Since it seems we'll actually be doing remote DPCD read/writes in DRM drivers
> and not just from auxdev, maybe we should combine drm_dp_dpcd_read() with
> drm_dp_mst_dpcd_read() and do the same with the _write() variants? Based on
> previous discussions with Leo Li I remember that we can't just combine the
> dp_aux_dev->transfer() callbacks in struct drm_dp_aux_dev, but I don't see a
> reason we can't just teach drm_dp_dpcd_read/write() to work with MST aux so
> that we don't need two seperate functions. This should be pretty easy to do,
> just:
>
> /* Add a check like this at the start of drm_dp_dpcd_read(): */
> ssize_t drm_dp_dpcd_read(...) {
> if (aux->is_remote)
> return drm_dp_mst_dpcd_read(...);
>
> /* ... */
> }
>
> /* And in drm_dp_dpcd_write(): */
> ssize_t drm_dp_dpcd_write(...) {
> if (aux->is_remote)
> return drm_dp_mst_dpcd_write(...);
>
> /* ... */
> }
>
> Then just replace the manual calls to drm_dp_mst_dpcd_write() in
> drivers/gpu/drm/drm_dp_aux_dev.c with normal
> drm_dp_dpcd_read()/drm_dp_dpcd_write() calls. Thoughts?
I think this would work well.
drm_dp_mst_dpcd_read/write will eventually call drm_dp_dpcd_write, so
doing so would cause a recursive call. That should be safe though. The
mst manager's aux will be used, and that should always be a real
(is_remote == false) aux, fwict.
Essentially, it's just moving that conditional from
auxdev_read/write_iter to drm_dp_dpcd_read/write.
Leo
>
> On Tue, 2019-08-20 at 15:11 -0400, David Francis wrote:
>> To use these functions in drm driver directories, they must be
>> exported
>>
>> Signed-off-by: David Francis
>> ---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index b40d975aec76..5d5bd42da17c 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -1512,6 +1512,7 @@ ssize_t drm_dp_mst_dpcd_read(struct drm_dp_aux *aux,
>> return drm_dp_send_dpcd_read(port->mgr, port,
>> offset, size, buffer);
>> }
>> +EXPORT_SYMBOL(drm_dp_mst_dpcd_read);
>>
>> /**
>> * drm_dp_mst_dpcd_write() - write a series of bytes to the DPCD via
>> sideband
>> @@ -1535,6 +1536,7 @@ ssize_t drm_dp_mst_dpcd_write(struct drm_dp_aux *aux,
>> return drm_dp_send_dpcd_write(port->mgr, port,
>>offset, size, buffer);
>> }
>> +EXPORT_SYMBOL(drm_dp_mst_dpcd_write);
>>
>> static void drm_dp_check_mstb_guid(struct drm_dp_mst_branch *mstb, u8
>> *guid)
>> {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [RFC][PATCH 0/2] drm: PATH prop for all connectors?
On 2019-08-01 5:51 a.m., Pekka Paalanen wrote: > On Tue, 16 Jul 2019 14:59:58 + > "Li, Sun peng (Leo)" wrote: > >> On 2019-07-11 3:29 a.m., Pekka Paalanen wrote: >>> Wait, one can write udev rules for connectors and stuff? >>> How? What can they do? >> >> I was using it to generate user-friendly device names for the mst aux >> implementation: >> https://patchwork.freedesktop.org/patch/315900/?series=63237=2 > > Hi, > > what is that device node used for? > > Are the "by-path" symlinks to help a display server associate the > right device node with the right DRM KMS connector resource? I intended it to be something more descriptive than the '/dev/drm_dp_aux0, drm_dp_aux1, drm_dp_aux2, ...' names, to help users identify the connector they're addressing in the mst topology. I guess it could also be used for the purpose you mention as well. Of course, we'd need more reliable and persistent PATH props first. The patch was dropped until this happens. Leo > > The patch commit message did not explain what the names are > actually used for. > > > Thanks, > pq > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/7] drm/amd/display: Use proper enum conversion functions
On 2019-07-25 12:14 p.m., Kazlauskas, Nicholas wrote:
> On 7/25/19 12:00 PM, Li, Sun peng (Leo) wrote:
>>
>>
>> On 2019-07-18 11:16 p.m., Nathan Chancellor wrote:
>>> On Wed, Jul 03, 2019 at 10:52:16PM -0700, Nathan Chancellor wrote:
>>>> clang warns:
>>>>
>>>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_pp_smu.c:336:8:
>>>> warning: implicit conversion from enumeration type 'enum smu_clk_type'
>>>> to different enumeration type 'enum amd_pp_clock_type'
>>>> [-Wenum-conversion]
>>>> dc_to_smu_clock_type(clk_type),
>>>> ^~~
>>>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_pp_smu.c:421:14:
>>>> warning: implicit conversion from enumeration type 'enum
>>>> amd_pp_clock_type' to different enumeration type 'enum smu_clk_type'
>>>> [-Wenum-conversion]
>>>> dc_to_pp_clock_type(clk_type),
>>>> ^~
>>>>
>>>> There are functions to properly convert between all of these types, use
>>>> them so there are no longer any warnings.
>>>>
>>>> Fixes: a43913ea50a5 ("drm/amd/powerplay: add function
>>>> get_clock_by_type_with_latency for navi10")
>>>> Fixes: e5e4e22391c2 ("drm/amd/powerplay: add interface to get clock by
>>>> type with latency for display (v2)")
>>>> Link: https://github.com/ClangBuiltLinux/linux/issues/586
>>>> Signed-off-by: Nathan Chancellor
>>>> ---
>>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>>>> index eac09bfe3be2..0f76cfff9d9b 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>>>> @@ -333,7 +333,7 @@ bool dm_pp_get_clock_levels_by_type(
>>>>}
>>>>} else if (adev->smu.funcs && adev->smu.funcs->get_clock_by_type) {
>>>>if (smu_get_clock_by_type(>smu,
>>>> -dc_to_smu_clock_type(clk_type),
>>>> +dc_to_pp_clock_type(clk_type),
>>
>> smu_funcs->get_clock_by_type doesn't seem to be hooked up anywhere,
>> so this looks to be the most correct.
>>
>> Although it makes more sense to use smu_clk_types here rather than
>> amd_pp_clock_type - any reason why this isn't the case?
>>
>>>> _clks)) {
>>>>get_default_clock_levels(clk_type, dc_clks);
>>>>return true;
>>>> @@ -418,7 +418,7 @@ bool dm_pp_get_clock_levels_by_type_with_latency(
>>>>return false;
>>>>} else if (adev->smu.ppt_funcs &&
>>>> adev->smu.ppt_funcs->get_clock_by_type_with_latency) {
>>>>if (smu_get_clock_by_type_with_latency(>smu,
>>>> -
>>>> dc_to_pp_clock_type(clk_type),
>>>> +
>>>> dc_to_smu_clock_type(clk_type),
>>
>> This is slightly concerning. The functions are doing the right thing,
>> but amd_pp_clock_type doesn't map 1-1 to smu_clk_type... In any case,
>> this looks good to me.
>>
>> Reviewed-by: Leo Li
>
> Looks mostly like the table just needs to be sized properly:
>
> static int dc_clk_type_map[] = {
> ->
> static int dc_clk_type_map[DM_PP_CLOCK_TYPE_NUM_TYPES] = {
>
> where DM_PP_CLOCK_TYPE_NUM_TYPES would be added to enum dm_pp_clock_type.
>
> Or it could just use a switch table instead, like the other function does.
>
> Nicholas Kazlauskas
Good catch, I'll spin up something quick.
Leo
>
>
>>
>>>> _clks))
>>>>return false;
>>>>}
>>>> --
>>>> 2.22.0
>>>>
>>>
>>> Gentle ping for review, this is the last remaining warning that I see
>>> from amdgpu on next-20190718.
>>>
>>> Cheers,
>>> Nathan
>>>
>> ___
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 5/7] drm/amd/display: Use proper enum conversion functions
On 2019-07-18 11:16 p.m., Nathan Chancellor wrote:
> On Wed, Jul 03, 2019 at 10:52:16PM -0700, Nathan Chancellor wrote:
>> clang warns:
>>
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_pp_smu.c:336:8:
>> warning: implicit conversion from enumeration type 'enum smu_clk_type'
>> to different enumeration type 'enum amd_pp_clock_type'
>> [-Wenum-conversion]
>> dc_to_smu_clock_type(clk_type),
>> ^~~
>> drivers/gpu/drm/amd/amdgpu/../display/amdgpu_dm/amdgpu_dm_pp_smu.c:421:14:
>> warning: implicit conversion from enumeration type 'enum
>> amd_pp_clock_type' to different enumeration type 'enum smu_clk_type'
>> [-Wenum-conversion]
>> dc_to_pp_clock_type(clk_type),
>> ^~
>>
>> There are functions to properly convert between all of these types, use
>> them so there are no longer any warnings.
>>
>> Fixes: a43913ea50a5 ("drm/amd/powerplay: add function
>> get_clock_by_type_with_latency for navi10")
>> Fixes: e5e4e22391c2 ("drm/amd/powerplay: add interface to get clock by type
>> with latency for display (v2)")
>> Link: https://github.com/ClangBuiltLinux/linux/issues/586
>> Signed-off-by: Nathan Chancellor
>> ---
>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>> index eac09bfe3be2..0f76cfff9d9b 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_pp_smu.c
>> @@ -333,7 +333,7 @@ bool dm_pp_get_clock_levels_by_type(
>> }
>> } else if (adev->smu.funcs && adev->smu.funcs->get_clock_by_type) {
>> if (smu_get_clock_by_type(>smu,
>> - dc_to_smu_clock_type(clk_type),
>> + dc_to_pp_clock_type(clk_type),
smu_funcs->get_clock_by_type doesn't seem to be hooked up anywhere,
so this looks to be the most correct.
Although it makes more sense to use smu_clk_types here rather than
amd_pp_clock_type - any reason why this isn't the case?
>>_clks)) {
>> get_default_clock_levels(clk_type, dc_clks);
>> return true;
>> @@ -418,7 +418,7 @@ bool dm_pp_get_clock_levels_by_type_with_latency(
>> return false;
>> } else if (adev->smu.ppt_funcs &&
>> adev->smu.ppt_funcs->get_clock_by_type_with_latency) {
>> if (smu_get_clock_by_type_with_latency(>smu,
>> -
>> dc_to_pp_clock_type(clk_type),
>> +
>> dc_to_smu_clock_type(clk_type),
This is slightly concerning. The functions are doing the right thing,
but amd_pp_clock_type doesn't map 1-1 to smu_clk_type... In any case,
this looks good to me.
Reviewed-by: Leo Li
>> _clks))
>> return false;
>> }
>> --
>> 2.22.0
>>
>
> Gentle ping for review, this is the last remaining warning that I see
> from amdgpu on next-20190718.
>
> Cheers,
> Nathan
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 9/9] drm/amd/display: Implement MST Aux device registration
On 2019-07-24 4:05 p.m., Kazlauskas, Nicholas wrote:
> On 7/23/19 7:28 PM, sunpeng...@amd.com wrote:
>> From: Leo Li
>>
>> Implement late_register and early_unregister hooks for MST connectors.
>> Call drm helpers for MST connector registration, which registers the
>> AUX devices.
>>
>> Cc: Jerry Zuo
>> Cc: Nicholas Kazlauskas
>> Cc: Harry Wentland
>> Signed-off-by: Leo Li
>
> Reviewed-by: Nicholas Kazlauskas
>
> BTW: I already reviewed patch 5, feel free to add my R-B.
>
> Nicholas Kazlauskas
Sorry, will do.
Thanks!
Leo
>
>> ---
>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 24 ++-
>> 1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> index 53d2cfe62e13..16218a202b59 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_mst_types.c
>> @@ -156,6 +156,26 @@ dm_dp_mst_connector_destroy(struct drm_connector
>> *connector)
>> kfree(amdgpu_dm_connector);
>> }
>>
>> +static int
>> +amdgpu_dm_mst_connector_late_register(struct drm_connector *connector)
>> +{
>> +struct amdgpu_dm_connector *amdgpu_dm_connector =
>> +to_amdgpu_dm_connector(connector);
>> +struct drm_dp_mst_port *port = amdgpu_dm_connector->port;
>> +
>> +return drm_dp_mst_connector_late_register(connector, port);
>> +}
>> +
>> +static void
>> +amdgpu_dm_mst_connector_early_unregister(struct drm_connector *connector)
>> +{
>> +struct amdgpu_dm_connector *amdgpu_dm_connector =
>> +to_amdgpu_dm_connector(connector);
>> +struct drm_dp_mst_port *port = amdgpu_dm_connector->port;
>> +
>> +drm_dp_mst_connector_early_unregister(connector, port);
>> +}
>> +
>> static const struct drm_connector_funcs dm_dp_mst_connector_funcs = {
>> .detect = dm_dp_mst_detect,
>> .fill_modes = drm_helper_probe_single_connector_modes,
>> @@ -164,7 +184,9 @@ static const struct drm_connector_funcs
>> dm_dp_mst_connector_funcs = {
>> .atomic_duplicate_state = amdgpu_dm_connector_atomic_duplicate_state,
>> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> .atomic_set_property = amdgpu_dm_connector_atomic_set_property,
>> -.atomic_get_property = amdgpu_dm_connector_atomic_get_property
>> +.atomic_get_property = amdgpu_dm_connector_atomic_get_property,
>> +.late_register = amdgpu_dm_mst_connector_late_register,
>> +.early_unregister = amdgpu_dm_mst_connector_early_unregister,
>> };
>>
>> static int dm_dp_mst_get_modes(struct drm_connector *connector)
>>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 07/10] drm/i915: Implement MST Aux device registration
On 2019-07-12 4:15 p.m., Ville Syrjälä wrote:
> On Fri, Jul 12, 2019 at 11:05:59PM +0300, Ville Syrjälä wrote:
>> On Fri, Jul 12, 2019 at 03:48:53PM -0400, Lyude Paul wrote:
>>> BTW, I just tried these patches on my T450s (using i915) and I'm seeing some
>>> kernel warnings with them when adding DP aux devices after connecting a new
>>> MST topology to the system:
>>>
>>> [ 367.742571] WARNING: CPU: 2 PID: 442 at
>>> drivers/gpu/drm/drm_mode_object.c:45 __drm_mode_object_add+0xaa/0xb0 [drm]
>>
>> Looks like Daniel added that particular WARN in
>> commit 4f5368b5541a ("drm/kms: Catch mode_object lifetime errors").
>
> And I'm the one who added the max_bpc prop to the mst connectors, which
> is a per-connector property (ie. a new one gets created for every
> connecotor). So that could be a problem I suppose. I guess we may need
> to create just one of these for MST and reuse it for every connector.
> Could just point at the prop of the corresponding SST connector I
> suppose...
>
> + /* Reuse the prop because blah */
> + connector->max_bpc_property =
> + intel_dp->attached_connector->base.max_bpc_property;
> drm_connector_attach_max_bpc_property(connector, 6, 12);
I'd prefer to address this separately, if that's alright.
It isn't related to this series, and no new warnings are introduced by
this either.
Leo
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/sysfs: Add mstpath attribute to connector devices
On 2019-07-10 6:50 p.m., Lyude Paul wrote:
> gah. So, I was originally going to ask "why didn't we add the connector name
> into this?", but then I realized we're doing the right thing already and just
> doing card%d-%s % (card_number, path_prop). Which means we probably really
> don't
> want to add a property called mstpath, since it's hardly different from path
> (whoops!).
>
> Additionally, after some thinking I realized I may have made a mistake as I'm
> not entirely sure if we would need to specify the DRM card in the path prop
> for
> udev, considering that's specified in the sysfs path all ready. Even if I'm
> wrong on that though, I think it might be better not to add an mstpath
> property
> and just go the route of just adding a new path_v2 property that we could use
> for both MST and non-MST connector paths. (I cc'd you on the email thread
> about
> this, so you can read more about this there.
Funny enough, I was originally trying to make this work for SST devices.
It didn't make sense to have by-name and by-path, but only have SST
exist in the by-name symlinks. The question there was "what to use for
sst paths?" Eventually I settled with keeping this purely for user
friendliness. But since discussion is already underway for a better
'path', it makes sense to delay this.
>
> So, I would actually suggest we just drop this patch entirely for now. We
> should
> be fine without it, even though the dp_aux_dev paths will be kind of ugly for
> a
> little while. I'd rather the rest of this series get upstream first, and try
> to
> do the path prop stuff separately.>
Sounds fair, going to spin up v3.
Thanks!
Leo
>
> On Fri, 2019-07-05 at 10:32 -0400, sunpeng...@amd.com wrote:
>> From: Leo Li
>>
>> This can be used to create more descriptive symlinks for MST aux
>> devices. Consider the following udev rule:
>>
>> SUBSYSTEM=="drm_dp_aux_dev", SUBSYSTEMS=="drm", ATTRS{mstpath}=="?*",
>> SYMLINK+="drm_dp_aux/by-path/$attr{mstpath}"
>>
>> The following symlinks will be created (depending on your MST topology):
>>
>> $ ls /dev/drm_dp_aux/by-path/
>> card0-mst:0-1 card0-mst:0-1-1 card0-mst:0-1-8 card0-mst:0-8
>>
>> v2: remove unnecessary locking of mode_config.mutex
>>
>> Signed-off-by: Leo Li
>> ---
>> drivers/gpu/drm/drm_sysfs.c | 20
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index ad10810bc972..7d483ab684a0 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -236,16 +236,36 @@ static ssize_t modes_show(struct device *device,
>> return written;
>> }
>>
>> +static ssize_t mstpath_show(struct device *device,
>> +struct device_attribute *attr,
>> +char *buf)
>> +{
>> +struct drm_connector *connector = to_drm_connector(device);
>> +ssize_t ret = 0;
>> +char *path;
>> +
>> +if (!connector->path_blob_ptr)
>> +return ret;
>> +
>> +path = connector->path_blob_ptr->data;
>> +ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n",
>> + connector->dev->primary->index, path);
>> +
>> +return ret;
>> +}
>> +
>> static DEVICE_ATTR_RW(status);
>> static DEVICE_ATTR_RO(enabled);
>> static DEVICE_ATTR_RO(dpms);
>> static DEVICE_ATTR_RO(modes);
>> +static DEVICE_ATTR_RO(mstpath);
>>
>> static struct attribute *connector_dev_attrs[] = {
>> _attr_status.attr,
>> _attr_enabled.attr,
>> _attr_dpms.attr,
>> _attr_modes.attr,
>> +_attr_mstpath.attr,
>> NULL
>> };
>>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [RFC][PATCH 0/2] drm: PATH prop for all connectors?
On 2019-07-11 3:29 a.m., Pekka Paalanen wrote: > Wait, one can write udev rules for connectors and stuff? > How? What can they do? I was using it to generate user-friendly device names for the mst aux implementation: https://patchwork.freedesktop.org/patch/315900/?series=63237=2 Leo ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/10] Enable MST Aux devices (v2)
Hi Lyude, sorry - just realized I forgot to CC you on this series! Let me know if I should resend them. Adding some additional reviewers as well. Thanks, Leo On 2019-07-04 3:05 p.m., sunpeng...@amd.com wrote: > From: Leo Li > > Hi all, > > Here's the second revision of patches to enable mst aux devices. > > v2 fixes an aux device unregistration issue during driver unload. See > patch 2/10 for details. Consequently, drivers supporting mst are > modified to use the new mst connector late register and early unregister > helpers. > > Thanks, > Leo > > Leo Li (9): > drm/dp: Use non-cyclic idr > drm/sysfs: Add mstpath attribute to connector devices > drm/nouveau: Use connector kdev as aux device parent > drm/bridge/analogix-anx78xx: Use connector kdev as aux device parent > drm/amd/display: Use connector kdev as aux device parent > drm/i915: Implement MST Aux device registration > drm/nouveau/kms/nv50: Implement MST Aux device registration > drm/radeon: Implement MST Aux device registration > drm/amd/display: Implement MST Aux device registration > > Ville Syrjälä (1): > drm/dp_mst: Enable registration of AUX devices for MST ports > > .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 26 +++- > drivers/gpu/drm/bridge/analogix-anx78xx.c | 22 +-- > drivers/gpu/drm/drm_dp_aux_dev.c | 19 ++- > drivers/gpu/drm/drm_dp_mst_topology.c | 137 -- > drivers/gpu/drm/drm_sysfs.c | 23 +++ > drivers/gpu/drm/i915/display/intel_dp_mst.c | 29 +++- > drivers/gpu/drm/nouveau/dispnv50/disp.c | 20 +++ > drivers/gpu/drm/nouveau/nouveau_connector.c | 2 +- > drivers/gpu/drm/radeon/radeon_dp_mst.c| 22 +++ > include/drm/drm_dp_helper.h | 4 + > include/drm/drm_dp_mst_helper.h | 11 ++ > 11 files changed, 285 insertions(+), 30 deletions(-) > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 03/10] drm/sysfs: Add mstpath attribute to connector devices
On 2019-07-04 3:33 p.m., Ville Syrjälä wrote:
> On Thu, Jul 04, 2019 at 03:05:12PM -0400, sunpeng...@amd.com wrote:
>> From: Leo Li
>>
>> This can be used to create more descriptive symlinks for MST aux
>> devices. Consider the following udev rule:
>>
>> SUBSYSTEM=="drm_dp_aux_dev", SUBSYSTEMS=="drm", ATTRS{mstpath}=="?*",
>> SYMLINK+="drm_dp_aux/by-path/$attr{mstpath}"
>>
>> The following symlinks will be created (depending on your MST topology):
>>
>> $ ls /dev/drm_dp_aux/by-path/
>> card0-mst:0-1 card0-mst:0-1-1 card0-mst:0-1-8 card0-mst:0-8
>>
>> Signed-off-by: Leo Li
>> ---
>> drivers/gpu/drm/drm_sysfs.c | 23 +++
>> 1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index ad10810bc972..53fd1f71e900 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -236,16 +236,39 @@ static ssize_t modes_show(struct device *device,
>> return written;
>> }
>>
>> +static ssize_t mstpath_show(struct device *device,
>> +struct device_attribute *attr,
>> +char *buf)
>> +{
>> +struct drm_connector *connector = to_drm_connector(device);
>> +ssize_t ret = 0;
>> +char *path;
>> +
>> +mutex_lock(>dev->mode_config.mutex);
>
> The blob is populated when the connector is created so I don't think
> this lock is actually doing anything for us.
Right, will drop this.
>
> One would also hope that device_unregister() protects us from racing
> with the removal of the attribute. Eg. if you hold a file descriptor
> open to the sysfs file does device_unregister() block until the fd is
> closed?
For dpcd transactions, as long as the aux device is unregistered through
drm_dp_aux_unregister_devnode(), we should be good. There's an atomic_t
use_count that syncs against reads and writes.
Although I'm not sure what would happen if the device was ripped out
from underneath us, say, if the parent connector device is removed
before calling aux_unregister_devnode(). If this does happen, I think
it's more of an order-of-operations issue from the driver. V2 of patch 2
(and 7-10) actually addresses a specific occurence of this during driver
unload.
Regarding sysfs attrs, the kernfs underneath does seem to guarantee that
any r/w is completed before removal. See kernfs_drain(), called as part
of kernfs_remove().
Leo
>
>> +if (!connector->path_blob_ptr)
>> +goto unlock;
>> +
>> +path = connector->path_blob_ptr->data;
>> +ret = snprintf(buf, PAGE_SIZE, "card%d-%s\n",
>> + connector->dev->primary->index, path);
>> +
>> +unlock:
>> +mutex_unlock(>dev->mode_config.mutex);
>> +return ret;
>> +}
>> +
>> static DEVICE_ATTR_RW(status);
>> static DEVICE_ATTR_RO(enabled);
>> static DEVICE_ATTR_RO(dpms);
>> static DEVICE_ATTR_RO(modes);
>> +static DEVICE_ATTR_RO(mstpath);
>>
>> static struct attribute *connector_dev_attrs[] = {
>> _attr_status.attr,
>> _attr_enabled.attr,
>> _attr_dpms.attr,
>> _attr_modes.attr,
>> +_attr_mstpath.attr,
>> NULL
>> };
>>
>> --
>> 2.22.0
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
Sorry for the late response! just jumping back on this now.
On 2019-05-16 5:40 p.m., Lyude Paul wrote:
> [CAUTION: External Email]
>
> So a couple of things:
>
> On Thu, 2019-05-16 at 11:17 -0400, sunpeng...@amd.com wrote:
>> From: Ville Syrjälä
>>
>> All available downstream ports - physical and logical - are exposed for
>> each MST device. They are listed in /dev/, following the same naming
>> scheme as SST devices by appending an incremental ID.
>>
>> Although all downstream ports are exposed, only some will work as
>> expected. Consider the following topology:
>>
>> +-+
>> | ASIC |
>> +-+
>>Conn-0|
>> |
>> +v+
>>+| MST HUB |+
>>|+-+|
>>| |
>>|Port-1 Port-2|
>> +-v-+ +-v-+
>> | MST | | SST |
>> | Display | | Display |
>> +---+ +---+
>>|Port-1
>>x
>>
>> MST Path | MST Device
>> --+--
>> sst:0 | MST Hub
>> mst:0-1 | MST Display
>> mst:0-1-1 | MST Display's disconnected DP out
>> mst:0-1-8 | MST Display's internal sink
>> mst:0-2 | SST Display
>>
>> On certain MST displays, the upstream physical port will ACK DPCD reads.
>> However, reads on the local logical port to the internal sink will
>> *NAK*. i.e. reading mst:0-1 ACKs, but mst:0-1-8 NAKs.
>>
>> There may also be duplicates. Some displays will return the same GUID
>> when reading DPCD from both mst:0-1 and mst:0-1-8.
>>
>> There are some device-dependent behavior as well. The MST hub used
>> during testing will actually *ACK* read requests on a disconnected
>> physical port, whereas the MST displays will NAK.
>>
>> In light of these discrepancies, it's simpler to expose all downstream
>> ports - both physical and logical - and let the user decide what to use.
>>
>> Cc: Lyude Paul
>> Signed-off-by: Ville Syrjälä
>> Signed-off-by: Leo Li
>> ---
>> drivers/gpu/drm/drm_dp_aux_dev.c | 14 -
>> drivers/gpu/drm/drm_dp_mst_topology.c | 103 +
>> -
>> include/drm/drm_dp_helper.h | 4 ++
>> include/drm/drm_dp_mst_helper.h | 6 ++
>> 4 files changed, 112 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
>> b/drivers/gpu/drm/drm_dp_aux_dev.c
>> index 6d84611..01c02b9 100644
>> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
>> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
>> @@ -34,6 +34,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -114,6 +115,7 @@ static ssize_t name_show(struct device *dev,
>>
>>return res;
>> }
>> +
>> static DEVICE_ATTR_RO(name);
>>
>> static struct attribute *drm_dp_aux_attrs[] = {
>> @@ -160,7 +162,11 @@ static ssize_t auxdev_read_iter(struct kiocb *iocb,
>> struct iov_iter *to)
>>break;
>>}
>>
>> - res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
>> + if (aux_dev->aux->is_remote)
>> + res = drm_dp_mst_dpcd_read(aux_dev->aux, pos, buf,
>> todo);
>> + else
>> + res = drm_dp_dpcd_read(aux_dev->aux, pos, buf, todo);
>> +
>
> It's still not at all clear to me why we're trying to avoid specifying actual
> callbacks for the aux device. We should remove this part entirely, remove the
> is_remote entry from struct drm_dp_aux, and then just specify our own hook in
> struct drm_dp_aux->transfer().
>
I'm not sure if this would work well. The existing policy does retries
around the ->transfer() call. Using the same hook will cause a nested
retry - once when calling remote_aux->transfer, and another when calling
real_aux->transfer. The difference is the scope of the retry. The former
replays the entire aux transaction, while the latter replays just the
failed sideband packet. I think having the retry at the packet level
makes more sense. Either way, it shouldn't happen in a nested manner.
In general, we need a way to determine whether a message needs to be
sent via sideband. I'm not sure if there's a better way than setting a
'is_remote' flag?
Leo
>>if (res <= 0)
>>break;
>>
>> @@ -207,7 +213,11 @@ static ssize_t auxdev_write_iter(struct kiocb *iocb,
>> struct iov_iter *from)
>>break;
>>}
>>
>> - res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
>> + if (aux_dev->aux->is_remote)
>> + res = drm_dp_mst_dpcd_write(aux_dev->aux, pos, buf,
>> todo);
>> + else
>> + res = drm_dp_dpcd_write(aux_dev->aux, pos, buf, todo);
>> +
>>if (res <= 0)
>>break;
>>
>> diff --git
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
On 2019-06-03 3:28 p.m., Lyude Paul wrote: >> I'm reproducing this just by reloading i915 on a machine with some MST >> displays connected. I uploaded a copy of the script that I use to do this >> here: >> >> https://people.freedesktop.org/~lyudess/archive/06-03-2019/unloadgpumod.sh > oops-almost forgot to mention. The argument you pass to make it reload instead > of just unloading is --reload > Thanks for the script! So, the warning has to do with: 1. Having the aux device as a child of connector device, and 2. During driver unload, drm_connector_unregister() is called before drm_dp_mst_topology_put_port() Which means that connector_unregister() will recursively remove the child aux device, before put_port() can properly unregister it. Any further attempts to remove after the first will throw a "not found" warning. Call-stacks for reference: *drm_connector_unregister*+0x37/0x60 [drm] drm_connector_unregister_all+0x30/0x60 [drm] drm_modeset_unregister_all+0xe/0x30 [drm] drm_dev_unregister+0x9c/0xb0 [drm] i915_driver_unload+0x73/0x120 [i915] drm_dp_aux_unregister_devnode+0xf5/0x180 [drm_kms_helper] *drm_dp_mst_topology_put_port*+0x4e/0xf0 [drm_kms_helper] drm_dp_mst_topology_put_mstb+0x91/0x160 [drm_kms_helper] drm_dp_mst_topology_mgr_set_mst+0x12b/0x2b0 [drm_kms_helper] ? __finish_swait+0x10/0x40 drm_dp_mst_topology_mgr_destroy+0x11/0xa0 [drm_kms_helper] intel_dp_encoder_flush_work+0x32/0xb0 [i915] intel_ddi_encoder_destroy+0x32/0x60 [i915] drm_mode_config_cleanup+0x51/0x2e0 [drm] intel_modeset_cleanup+0xc8/0x140 [i915] i915_driver_unload+0xa0/0x120 [i915] A solution is to unregister the aux device immediately before the connector device is unregistered - if we are to keep the aux device as a child. Following current scheme with SST, it looks like drm_connector_funcs->early_unregister() is the right place to do so. To keep the balance, aux registration will then happen in drm_connector_funcs->late_register(). This will leave the SDP transaction handling part in DRM still, but pass the responsibility of creating and removing remote (fake) aux devices to the driver. I have a WIP patch here for you to take a look. It should apply on top of the existing patchset: https://pastebin.com/1YJZhL4C I'd like to hear your thoughts, before I go and modify other drivers :) Thanks, Leo ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/7] drm/dp_mst: Register AUX devices for MST ports
Hey, sorry for my late response!
On 2019-05-16 5:40 p.m., Lyude Paul wrote:
>>if (old_pdt != port->pdt && !port->input) {
>> @@ -1220,6 +1268,8 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
>> *mstb,
>>drm_connector_set_tile_property(port->connector);
>>}
>>(*mstb->mgr->cbs->register_connector)(port->connector);
> This is wrong: we always want to setup everything in the connector first
> before trying to register it, not after. Otherwise, things explode like so:
**snip**
> [ 452.424461] [ cut here ]
> [ 452.424464] sysfs group 'power' not found for kobject 'drm_dp_aux5'
> [ 452.424471] WARNING: CPU: 3 PID: 1887 at fs/sysfs/group.c:256
> sysfs_remove_group+0x76/0x80
> [ 452.424473] Modules linked in: vfat fat elan_i2c i915(O) intel_rapl
> x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel iTCO_wdt kvm mei_wdt
> irqbypass iTCO_vendor_support crct10dif_pclmul wmi_bmof intel_wmi_thunderbolt
> crc32_pclmul i2c_algo_bit ghash_clmulni_intel intel_cstate drm_kms_helper(O)
> intel_uncore intel_rapl_perf btusb btrtl btbcm syscopyarea btintel
> sysfillrect sysimgblt fb_sys_fops bluetooth drm(O) joydev mei_me idma64
> ucsi_acpi thunderbolt ecdh_generic i2c_i801 intel_xhci_usb_role_switch
> processor_thermal_device typec_ucsi intel_lpss_pci intel_soc_dts_iosf mei
> roles intel_lpss typec intel_pch_thermal wmi thinkpad_acpi ledtrig_audio
> rfkill int3403_thermal int340x_thermal_zone int3400_thermal acpi_thermal_rel
> acpi_pad video pcc_cpufreq crc32c_intel nvme serio_raw uas e1000e nvme_core
> usb_storage i2c_dev
> [ 452.424492] CPU: 3 PID: 1887 Comm: unloadgpumod Tainted: G O
> 5.1.0Lyude-Test+ #1
> [ 452.424494] Hardware name: LENOVO 20L8S2N800/20L8S2N800, BIOS N22ET35W
> (1.12 ) 04/09/2018
> [ 452.424496] RIP: 0010:sysfs_remove_group+0x76/0x80
> [ 452.424498] Code: 48 89 df 5b 5d 41 5c e9 f8 bc ff ff 48 89 df e8 d0 bc ff
> ff eb cb 49 8b 14 24 48 8b 75 00 48 c7 c7 08 a5 0c aa e8 44 00 d6 ff <0f> 0b
> 5b 5d 41 5c c3 0f 1f 00 0f 1f 44 00 00 48 85 f6 74 31 41 54
> [ 452.424500] RSP: 0018:a8bb81b5fb28 EFLAGS: 00010282
> [ 452.424501] RAX: RBX: RCX:
> 0006
> [ 452.424502] RDX: 0007 RSI: 0086 RDI:
> 981fde2d5a00
> [ 452.424503] RBP: a9ea12e0 R08: 0792 R09:
> 0046
> [ 452.424504] R10: 0727 R11: a8bb81b5f9d0 R12:
> 981fd5f77010
> [ 452.424505] R13: 981fd6ebbedc R14: dead0200 R15:
> dead0100
> [ 452.424507] FS: 7f8cc1d8c740() GS:981fde2c()
> knlGS:
> [ 452.424508] CS: 0010 DS: ES: CR0: 80050033
> [ 452.424509] CR2: 55b19d079a08 CR3: 00043b2a0002 CR4:
> 003606e0
> [ 452.424510] DR0: DR1: DR2:
>
> [ 452.424511] DR3: DR6: fffe0ff0 DR7:
> 0400
> [ 452.424512] Call Trace:
> [ 452.424516] device_del+0x75/0x360
> [ 452.424518] ? class_find_device+0x96/0xf0
> [ 452.424520] device_unregister+0x16/0x60
> [ 452.424521] device_destroy+0x3a/0x40
> [ 452.424525] drm_dp_aux_unregister_devnode+0xea/0x180 [drm_kms_helper]
> [ 452.424534] ? drm_dbg+0x87/0x90 [drm]
> [ 452.424538] drm_dp_mst_topology_put_port+0x5b/0x110 [drm_kms_helper]
> [ 452.424543] drm_dp_mst_topology_put_mstb+0xb6/0x180 [drm_kms_helper]
> [ 452.424547] drm_dp_mst_topology_mgr_set_mst+0x233/0x2b0 [drm_kms_helper]
> [ 452.424551] drm_dp_mst_topology_mgr_destroy+0x18/0xa0 [drm_kms_helper]
> [ 452.424571] intel_dp_encoder_flush_work+0x32/0xb0 [i915]
> [ 452.424592] intel_ddi_encoder_destroy+0x32/0x70 [i915]
> [ 452.424600] drm_mode_config_cleanup+0x51/0x2e0 [drm]
> [ 452.424621] intel_modeset_cleanup+0xc8/0x140 [i915]
> [ 452.424633] i915_driver_unload+0xa8/0x130 [i915]
> [ 452.424645] i915_pci_remove+0x1e/0x40 [i915]
> [ 452.424647] pci_device_remove+0x3b/0xc0
> [ 452.424649] device_release_driver_internal+0xe4/0x1d0
> [ 452.424651] pci_stop_bus_device+0x69/0x90
> [ 452.424653] pci_stop_and_remove_bus_device_locked+0x16/0x30
> [ 452.424655] remove_store+0x75/0x90
> [ 452.424656] kernfs_fop_write+0x116/0x190
> [ 452.424658] vfs_write+0xa5/0x1a0
> [ 452.424660] ksys_write+0x57/0xd0
> [ 452.424663] do_syscall_64+0x55/0x150
> [ 452.424665] entry_SYSCALL_64_after_hwframe+0x44/0xa9
> [ 452.424667] RIP: 0033:0x7f8cc1e7d038
> [ 452.424668] Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 f3
> 0f 1e fa 48 8d 05 e5 76 0d 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05 <48> 3d
> 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
> [ 452.424670] RSP: 002b:7ffce4321218 EFLAGS: 0246 ORIG_RAX:
> 0001
> [ 452.424672] RAX: ffda RBX: 0002 RCX:
> 7f8cc1e7d038
> [ 452.424673] RDX: 0002 RSI:
Re: [PATCH 0/7] Add DP MST AUX devices
On 2019-05-16 3:54 p.m., Lyude Paul wrote: > [CAUTION: External Email] > > Hi, could we (and for future versions of this series and others) get a respin > of this that's actually rebased against drm-tip? That is the defacto standard > branch to do development on, and otherwise anyone trying to test these patches > has to resolve merge conflicts (along with maintainers). The branch this > appears to be based off of doesn't even have the new kref scheme for branch > devices and ports. > Sorry, this was laziness on my part :) Rebasing this now. Leo > On Thu, 2019-05-16 at 11:17 -0400, sunpeng...@amd.com wrote: >> From: Leo Li >> >> This series adds support for MST AUX devices. >> >> A more descriptive 'mstpath' attribute is also added to MST connector >> devices. In addition, the DP aux device is made to be a child of the >> corresponding connector's device where possible (*). This allows udev >> rules to provide descriptive symlinks to the AUX devices. >> >> (*) This can only be done on drivers that register their connector and >> aux devices via drm_connector_register() and drm_dp_aux_register() >> respectively. The connector also needs to be registered before the aux >> device. >> >> Leo Li (6): >>drm/dp: Use non-cyclic idr >>drm/dp-mst: Use connector kdev as aux device parent >>drm/sysfs: Add mstpath attribute to connector devices >>drm/amd/display: Use connector kdev as aux device parent >>drm/bridge/analogix-anx78xx: Use connector kdev as aux device parent >>drm/nouveau: Use connector kdev as aux device parent >> >> Ville Syrjälä (1): >>drm/dp_mst: Register AUX devices for MST ports >> >> .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c| 2 +- >> drivers/gpu/drm/bridge/analogix-anx78xx.c | 22 ++--- >> drivers/gpu/drm/drm_dp_aux_dev.c | 17 +++- >> drivers/gpu/drm/drm_dp_mst_topology.c | 106 >> ++--- >> drivers/gpu/drm/drm_sysfs.c| 23 + >> drivers/gpu/drm/nouveau/nouveau_connector.c| 2 +- >> include/drm/drm_dp_helper.h| 4 + >> include/drm/drm_dp_mst_helper.h| 6 ++ >> 8 files changed, 152 insertions(+), 30 deletions(-) >> > -- > Cheers, > Lyude Paul > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/3] drm/dp_mst: Expose build_mst_prop_path()
On 2019-04-24 1:26 p.m., Lyude Paul wrote:
> Closer, but are we sure we want to use the MST prop path for this? Why not add
> a sysfs attribute with the corresponding DRM connector name instead since the
> connector itself will have a path property. That way we can associate aux
> devices for eDP and DP devices with their corresponding connectors as well
I thought about that as well, but I hit a wall when trying to get the
SST connector from the aux device. Perhaps there's a simpler way that
I'm overlooking?
It's easier for MST, since the mst_port can be obtained via container_of
dp_aux. port->connector would then give what we want.
For SST though, each driver calls drm_aux_register() with an aux struct
that they've initialized. I'm not sure how I can reliably get the
drm_connector from that.
Maybe drm_dp_aux should have a back reference to the connector? FWICT
all drivers using drm_aux_register() should be able to provide the
associated connector when calling it.
Leo
>
> On Mon, 2019-04-22 at 19:56 -0400, sunpeng...@amd.com wrote:
>> From: Leo Li
>>
>> To give identifiable attributes to MST DP aux devices, we can use the
>> MST relative address. Expose this function for later use.
>>
>> Signed-off-by: Leo Li
>> ---
>> drivers/gpu/drm/drm_dp_mst_topology.c | 4 ++--
>> include/drm/drm_dp_mst_helper.h | 4
>> 2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c
>> b/drivers/gpu/drm/drm_dp_mst_topology.c
>> index 2ab16c9..86ff8e2 100644
>> --- a/drivers/gpu/drm/drm_dp_mst_topology.c
>> +++ b/drivers/gpu/drm/drm_dp_mst_topology.c
>> @@ -1120,7 +1120,7 @@ static void drm_dp_check_mstb_guid(struct
>> drm_dp_mst_branch *mstb, u8 *guid)
>> }
>> }
>>
>> -static void build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>> int pnum,
>> char *proppath,
>> size_t proppath_size)
>> @@ -1202,7 +1202,7 @@ static void drm_dp_add_port(struct drm_dp_mst_branch
>> *mstb,
>> if (created && !port->input) {
>> char proppath[255];
>>
>> -build_mst_prop_path(mstb, port->port_num, proppath,
>> sizeof(proppath));
>> +drm_dp_build_mst_prop_path(mstb, port->port_num, proppath,
>> sizeof(proppath));
>> port->connector = (*mstb->mgr->cbs->add_connector)(mstb->mgr,
>> port, proppath);
>> if (!port->connector) {
>> /* remove it from the port list */
>> diff --git a/include/drm/drm_dp_mst_helper.h
>> b/include/drm/drm_dp_mst_helper.h
>> index 371cc28..81c8d79 100644
>> --- a/include/drm/drm_dp_mst_helper.h
>> +++ b/include/drm/drm_dp_mst_helper.h
>> @@ -602,6 +602,10 @@ void drm_dp_mst_deallocate_vcpi(struct
>> drm_dp_mst_topology_mgr *mgr,
>> int drm_dp_find_vcpi_slots(struct drm_dp_mst_topology_mgr *mgr,
>> int pbn);
>>
>> +void drm_dp_build_mst_prop_path(const struct drm_dp_mst_branch *mstb,
>> + int pnum,
>> + char *proppath,
>> + size_t proppath_size);
>>
>> int drm_dp_update_payload_part1(struct drm_dp_mst_topology_mgr *mgr);
>>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/dp_aux: Use non-cyclic idr, add suffix option for aux device
On 2019-04-16 6:16 p.m., Lyude Paul wrote:
> Sorry for the slow response, I've been really busy ;_;
No worries :)
>
> On Fri, 2019-04-12 at 12:05 -0400, sunpeng...@amd.com wrote:
>> From: Leo Li
>>
>> In preparation for adding aux devices for DP MST:
>>
>> 1. A non-cyclic idr is used for the device minor version. That way,
>> hotplug cycling MST devices won't needlessly increment the minor
>> version index.
>
> I really like this btw, cyclic idrs are really annoying for drm_dp_aux_dev,
> even outside of MST (try reloading a drm driver a couple of times and you'll
> understand ;). I think we should split this into another commit though
>
>> 2. A suffix option is added to the aux device file name. It can be used
>> to identify the corresponding MST device.
>
> I like this idea, but I think there may be a better way that we can do this.
> Using /dev/nvme* as an example, we have the standard dev paths (/dev/nvme0,
> /dev/nvme0n1, etc.). But we can also access them through /dev/disk/by-
> {id,label,partlabel,partuuid,path,uuid}.
>
> So what if we added something similar for aux devices? We start off with the
> standard /dev/drm_dp_aux*, then provide more descriptive symlinks and
> directories:
>
> (feel free to come up with better naming then this if you can)
>
> /dev/drm_dp_aux/card0-DP-1 -> /dev/drm_dp_aux1
> /dev/drm_dp_aux/card0-DP-2 -> /dev/drm_dp_aux2
> etc.
That does sound neater - although FWICT, this will have to be done with
udev rules?
I took a brief look at how that's done for storage devices. It looks
like they have rules defined in
/lib/udev/rules.d/60-persistent-storage.rules that manages the "by-x"
symlinks.
To make this happen for aux devices, what we could do is attach sysfs
attributes to the device. It can then be parsed by udev in a similar
fashion. Currently, only 'name' attribute is attached, as seen in
drm_dp_aux_dev.c (after name_show()).
Thanks,
Leo
>
>>
>> Signed-off-by: Leo Li
>> ---
>> drivers/gpu/drm/drm_crtc_helper_internal.h | 5 +++--
>> drivers/gpu/drm/drm_dp_aux_dev.c | 8
>> drivers/gpu/drm/drm_dp_helper.c| 2 +-
>> 3 files changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc_helper_internal.h
>> b/drivers/gpu/drm/drm_crtc_helper_internal.h
>> index b5ac158..9f0907b 100644
>> --- a/drivers/gpu/drm/drm_crtc_helper_internal.h
>> +++ b/drivers/gpu/drm/drm_crtc_helper_internal.h
>> @@ -46,7 +46,7 @@ static inline int drm_fb_helper_modinit(void)
>> #ifdef CONFIG_DRM_DP_AUX_CHARDEV
>> int drm_dp_aux_dev_init(void);
>> void drm_dp_aux_dev_exit(void);
>> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux);
>> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char
>> *suffix);
>> void drm_dp_aux_unregister_devnode(struct drm_dp_aux *aux);
>> #else
>> static inline int drm_dp_aux_dev_init(void)
>> @@ -58,7 +58,8 @@ static inline void drm_dp_aux_dev_exit(void)
>> {
>> }
>>
>> -static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>> +static inline int drm_dp_aux_register_devnode(struct drm_dp_aux *aux,
>> + const char *suffix)
>> {
>> return 0;
>> }
>> diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c
>> b/drivers/gpu/drm/drm_dp_aux_dev.c
>> index 0e4f25d..2310a67 100644
>> --- a/drivers/gpu/drm/drm_dp_aux_dev.c
>> +++ b/drivers/gpu/drm/drm_dp_aux_dev.c
>> @@ -80,8 +80,7 @@ static struct drm_dp_aux_dev *alloc_drm_dp_aux_dev(struct
>> drm_dp_aux *aux)
>> kref_init(_dev->refcount);
>>
>> mutex_lock(_idr_mutex);
>> -index = idr_alloc_cyclic(_idr, aux_dev, 0, DRM_AUX_MINORS,
>> - GFP_KERNEL);
>> +index = idr_alloc(_idr, aux_dev, 0, DRM_AUX_MINORS, GFP_KERNEL);
>> mutex_unlock(_idr_mutex);
>> if (index < 0) {
>> kfree(aux_dev);
>> @@ -290,7 +289,7 @@ void drm_dp_aux_unregister_devnode(struct drm_dp_aux
>> *aux)
>> kref_put(_dev->refcount, release_drm_dp_aux_dev);
>> }
>>
>> -int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>> +int drm_dp_aux_register_devnode(struct drm_dp_aux *aux, const char *suffix)
>> {
>> struct drm_dp_aux_dev *aux_dev;
>> int res;
>> @@ -301,7 +300,8 @@ int drm_dp_aux_register_devnode(struct drm_dp_aux *aux)
>>
>> aux_dev->dev = device_create(drm_dp_aux_dev_class, aux->dev,
>> MKDEV(drm_dev_major, aux_dev->index),
>> NULL,
>> - "drm_dp_aux%d", aux_dev->index);
>> + "drm_dp_aux%d%s", aux_dev->index,
>> + suffix ? suffix : "");
>> if (IS_ERR(aux_dev->dev)) {
>> res = PTR_ERR(aux_dev->dev);
>> aux_dev->dev = NULL;
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c
>> b/drivers/gpu/drm/drm_dp_helper.c
>> index e2266ac..13f1005 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>>
Re: [RFC 0/2] Add AUX device entries for DP MST devices
>> Hmm. My MST-foo is admittedly weak so I'm not sure. A quick trawl through >> the spec didn't provide any solid explanations either :( However eg. >> "Figure 2-83: Example Multi-function MST Branch-Sink Device Enumeration" >> in the DP 1.4 spec does appear to show kind of virtual DPCD thing behind >> a logical port. But I'm not really sure what than means. > > Good point, I'm gonna dig more into that. It sounds like we should be > able to access that with the relative addressing as defined by the spec > (2.11.5). I'll have to see why that's currently getting nacked though. > It looks like DPCD reads on logical ports work on some devices, and not others... I swapped my MST display out with a different one, and it read just fine. More specifically - in a daisy chain setup with 2 MST displays - with the one that works at the end: # auxrw.py read /dev/drm_dp_aux4_mst\:0-1-8 0x30-0x3f -> ACK # auxrw.py read /dev/drm_dp_aux6_mst\:0-1 0x30-0x3f -> ACK (The GUIDs returned are identical) With the one that doesn't at the end: # auxrw.py read /dev/drm_dp_aux4_mst\:0-1-8 0x30-0x3f -> *NAK* # auxrw.py read /dev/drm_dp_aux6_mst\:0-1 0x30-0x3f -> ACK So it seems there's some device dependent behavior here. I'm not sure if there's a better way of handling this besides exposing all the downstream ports: If it works, great. If not, just don't use it? Leo ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amd/display: Fix reference counting for struct dc_sink.
On 2019-02-20 12:24 a.m., Mathias Fröhlich wrote: > Hi, > > ping? > ... to the dc folks? > > best > Mathias Hi Mathias, Sorry for the wait, change looks good to me. Reviewed-by: Leo Li ...and merged. Thanks for cleaning this up. Leo > > On Wednesday, 13 February 2019 21:38:03 CET Alex Deucher wrote: >> Add amd-gfx and some DC people. >> >> Alex >> >> On Sun, Feb 10, 2019 at 5:13 AM wrote: >>> >>> From: Mathias Fröhlich >>> >>> Reference counting in amdgpu_dm_connector for amdgpu_dm_connector::dc_sink >>> and amdgpu_dm_connector::dc_em_sink as well as in dc_link::local_sink seems >>> to be out of shape. Thus make reference counting consistent for these >>> members and just plain increment the reference count when the variable >>> gets assigned and decrement when the pointer is set to zero or replaced. >>> Also simplify reference counting in selected function sopes to be sure the >>> reference is released in any case. In some cases add NULL pointer check >>> before dereferencing. >>> At a hand full of places a comment is placed to stat that the reference >>> increment happened already somewhere else. >>> >>> This actually fixes the following kernel bug on my system when enabling >>> display core in amdgpu. There are some more similar bug reports around, >>> so it probably helps at more places. >>> >>> kernel BUG at mm/slub.c:294! >>> invalid opcode: [#1] SMP PTI >>> CPU: 9 PID: 1180 Comm: Xorg Not tainted 5.0.0-rc1+ #2 >>> Hardware name: Supermicro X10DAi/X10DAI, BIOS 3.0a 02/05/2018 >>> RIP: 0010:__slab_free+0x1e2/0x3d0 >>> Code: 8b 54 24 30 48 89 4c 24 28 e8 da fb ff ff 4c 8b 54 24 28 85 c0 0f >>> 85 67 fe ff ff 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 <0f> 0b 49 3b >>> 5c 24 28 75 ab 48 8b 44 24 30 49 89 4c 24 28 49 89 44 >>> RSP: 0018:b0978589fa90 EFLAGS: 00010246 >>> RAX: 92f12806c400 RBX: 80200019 RCX: 92f12806c400 >>> RDX: 92f12806c400 RSI: dd6421a01a00 RDI: 92ed2f406e80 >>> RBP: b0978589fb40 R08: 0001 R09: c0ee4748 >>> R10: 92f12806c400 R11: 0001 R12: dd6421a01a00 >>> R13: 92f12806c400 R14: 92ed2f406e80 R15: dd6421a01a20 >>> FS: 7f4170be0ac0() GS:92ed2fb4() >>> knlGS: >>> CS: 0010 DS: ES: CR0: 80050033 >>> CR2: 562818aaa000 CR3: 00045745a002 CR4: 003606e0 >>> DR0: DR1: DR2: >>> DR3: DR6: fffe0ff0 DR7: 0400 >>> Call Trace: >>> ? drm_dbg+0x87/0x90 [drm] >>> dc_stream_release+0x28/0x50 [amdgpu] >>> amdgpu_dm_connector_mode_valid+0xb4/0x1f0 [amdgpu] >>> drm_helper_probe_single_connector_modes+0x492/0x6b0 [drm_kms_helper] >>> drm_mode_getconnector+0x457/0x490 [drm] >>> ? drm_connector_property_set_ioctl+0x60/0x60 [drm] >>> drm_ioctl_kernel+0xa9/0xf0 [drm] >>> drm_ioctl+0x201/0x3a0 [drm] >>> ? drm_connector_property_set_ioctl+0x60/0x60 [drm] >>> amdgpu_drm_ioctl+0x49/0x80 [amdgpu] >>> do_vfs_ioctl+0xa4/0x630 >>> ? __sys_recvmsg+0x83/0xa0 >>> ksys_ioctl+0x60/0x90 >>> __x64_sys_ioctl+0x16/0x20 >>> do_syscall_64+0x5b/0x160 >>> entry_SYSCALL_64_after_hwframe+0x44/0xa9 >>> RIP: 0033:0x7f417110809b >>> Code: 0f 1e fa 48 8b 05 ed bd 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff >>> ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 >>> ff ff 73 01 c3 48 8b 0d bd bd 0c 00 f7 d8 64 89 01 48 >>> RSP: 002b:7ffdd8d1c268 EFLAGS: 0246 ORIG_RAX: 0010 >>> RAX: ffda RBX: 562818a8ebc0 RCX: 7f417110809b >>> RDX: 7ffdd8d1c2a0 RSI: c05064a7 RDI: 0012 >>> RBP: 7ffdd8d1c2a0 R08: 562819012280 R09: 0007 >>> R10: R11: 0246 R12: c05064a7 >>> R13: 0012 R14: 0012 R15: 7ffdd8d1c2a0 >>> Modules linked in: nfsv4 dns_resolver nfs lockd grace fscache fuse vfat >>> fat amdgpu intel_rapl sb_edac x86_pkg_temp_thermal intel_powerclamp >>> coretemp kvm_intel kvm irqbypass crct10dif_pclmul chash gpu_sched >>> crc32_pclmul snd_hda_codec_realtek ghash_clmulni_intel amd_iommu_v2 >>> iTCO_wdt iTCO_vendor_support ttm snd_hda_codec_generic snd_hda_codec_hdmi >>> ledtrig_audio snd_hda_intel drm_kms_helper snd_hda_codec intel_cstate >>> snd_hda_core drm snd_hwdep snd_seq snd_seq_device intel_uncore snd_pcm >>> intel_rapl_perf snd_timer snd soundcore ioatdma pcspkr >>> intel_wmi_thunderbolt mxm_wmi i2c_i801 lpc_ich pcc_cpufreq auth_rpcgss >>> sunrpc igb crc32c_intel i2c_algo_bit dca wmi hid_cherry analog gameport >>> joydev >>> >>> This patch is based on agd5f/drm-next-5.1-wip. This patch does not require >>> all of that, but agd5f/drm-next-5.1-wip contains at least one more dc_sink >>> counting fix that I could spot. >>>
Re: [PATCH][drm-next] drm/amd/display: fix dereference of pointer fs_params before it is null checked
On 2018-11-20 12:17 p.m., Colin King wrote:
> From: Colin Ian King
>
> Currently there are several instances of pointer fs_params being
> dereferenced before fs_params is being null checked. Fix this by
> only dereferencing fs_params after the null check.
>
> Detected by CoverityScan, CID#1475565 ("Dereference before null check")
>
> Fixes: e1e8a020c6b8 ("drm/amd/display: Add support for Freesync 2 HDR and
> Content to Display Mapping")
> Signed-off-by: Colin Ian King
Reviewed-by: Leo Li
Thanks!
> ---
> .../drm/amd/display/modules/color/color_gamma.c | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
> b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
> index 7480f072c375..bbecbaefb741 100644
> --- a/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
> +++ b/drivers/gpu/drm/amd/display/modules/color/color_gamma.c
> @@ -813,20 +813,26 @@ static bool build_freesync_hdr(struct pwl_float_data_ex
> *rgb_regamma,
> const struct hw_x_point *coord_x = coordinate_x;
> struct fixed31_32 scaledX = dc_fixpt_zero;
> struct fixed31_32 scaledX1 = dc_fixpt_zero;
> - struct fixed31_32 max_display =
> dc_fixpt_from_int(fs_params->max_display);
> - struct fixed31_32 min_display =
> dc_fixpt_from_fraction(fs_params->min_display, 1);
> - struct fixed31_32 max_content =
> dc_fixpt_from_int(fs_params->max_content);
> - struct fixed31_32 min_content =
> dc_fixpt_from_fraction(fs_params->min_content, 1);
> + struct fixed31_32 max_display;
> + struct fixed31_32 min_display;
> + struct fixed31_32 max_content;
> + struct fixed31_32 min_content;
> struct fixed31_32 clip = dc_fixpt_one;
> struct fixed31_32 output;
> bool use_eetf = false;
> bool is_clipped = false;
> - struct fixed31_32 sdr_white_level =
> dc_fixpt_from_int(fs_params->sdr_white_level);
> + struct fixed31_32 sdr_white_level;
>
> if (fs_params == NULL || fs_params->max_content == 0 ||
> fs_params->max_display == 0)
> return false;
>
> + max_display = dc_fixpt_from_int(fs_params->max_display);
> + min_display = dc_fixpt_from_fraction(fs_params->min_display, 1);
> + max_content = dc_fixpt_from_int(fs_params->max_content);
> + min_content = dc_fixpt_from_fraction(fs_params->min_content, 1);
> + sdr_white_level = dc_fixpt_from_int(fs_params->sdr_white_level);
> +
> if (fs_params->min_display > 1000) // cap at 0.1 at the bottom
> min_display = dc_fixpt_from_fraction(1, 10);
> if (fs_params->max_display < 100) // cap at 100 at the top
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/atomic: Create and use __drm_atomic_helper_crtc_reset() everywhere
On 2018-11-12 10:01 AM, Maarten Lankhorst wrote:
> We already have __drm_atomic_helper_connector_reset() and
> __drm_atomic_helper_plane_reset(), extend this to crtc as well.
>
> Most drivers already have a gpu reset hook, correct it.
> Nouveau already implemented its own __drm_atomic_helper_crtc_reset(),
> convert it to the common one.
>
> Signed-off-by: Maarten Lankhorst
> Cc: Harry Wentland
> Cc: Leo Li
> Cc: Alex Deucher
> Cc: "Christian König"
> Cc: "David (ChunMing) Zhou"
> Cc: David Airlie
> Cc: Liviu Dudau
> Cc: Brian Starkey
> Cc: Mali DP Maintainers
> Cc: Boris Brezillon
> Cc: Nicolas Ferre
> Cc: Alexandre Belloni
> Cc: Ludovic Desroches
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Sean Paul
> Cc: Jani Nikula
> Cc: Joonas Lahtinen
> Cc: Rodrigo Vivi
> Cc: Philipp Zabel
> Cc: CK Hu
> Cc: Matthias Brugger
> Cc: Rob Clark
> Cc: Ben Skeggs
> Cc: Tomi Valkeinen
> Cc: Laurent Pinchart
> Cc: Kieran Bingham
> Cc: Sandy Huang
> Cc: "Heiko Stübner"
> Cc: Thierry Reding
> Cc: Jonathan Hunter
> Cc: Eric Anholt
> Cc: VMware Graphics
> Cc: Sinclair Yeh
> Cc: Thomas Hellstrom
> Cc: Tony Cheng
> Cc: Shirish S
> Cc: Mikita Lipski
> Cc: Bhawanpreet Lakha
> Cc: David Francis
> Cc: Anthony Koo
> Cc: Jeykumar Sankaran
> Cc: Jordan Crouse
> Cc: Bruce Wang
> Cc: Sravanthi Kollukuduru
> Cc: Archit Taneja
> Cc: Steve Kowalik
> Cc: Carsten Behling
> Cc: Haneen Mohammed
> Cc: Daniel Vetter
> Cc: Rodrigo Siqueira
> Cc: Mahesh Kumar
> Cc: amd-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-ker...@vger.kernel.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: intel-...@lists.freedesktop.org
> Cc: linux-media...@lists.infradead.org
> Cc: linux-arm-...@vger.kernel.org
> Cc: freedr...@lists.freedesktop.org
> Cc: nouv...@lists.freedesktop.org
> Cc: linux-renesas-...@vger.kernel.org
> Cc: linux-rockc...@lists.infradead.org
> Cc: linux-te...@vger.kernel.org
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +--
> drivers/gpu/drm/arm/malidp_crtc.c | 5 +--
> .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c| 5 +--
> drivers/gpu/drm/drm_atomic_state_helper.c | 31 ---
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/imx/ipuv3-crtc.c | 5 +--
> drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 5 +--
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 12 ++-
> drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 6 +---
> drivers/gpu/drm/nouveau/dispnv50/head.c | 13 ++--
> drivers/gpu/drm/omapdrm/omap_crtc.c | 7 ++---
> drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 4 +--
> drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 7 +++--
> drivers/gpu/drm/tegra/dc.c| 5 +--
> drivers/gpu/drm/vc4/vc4_crtc.c| 8 ++---
> drivers/gpu/drm/vkms/vkms_crtc.c | 7 +
> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 9 +-
> include/drm/drm_atomic_state_helper.h | 2 ++
> 18 files changed, 56 insertions(+), 81 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 5064768642f3..770a71726cd1 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2802,9 +2802,7 @@ static void dm_crtc_reset_state(struct drm_crtc *crtc)
> if (WARN_ON(!state))
> return;
>
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> -
> + __drm_atomic_helper_crtc_reset(crtc, >base);
> }
>
> static struct drm_crtc_state *
> diff --git a/drivers/gpu/drm/arm/malidp_crtc.c
> b/drivers/gpu/drm/arm/malidp_crtc.c
> index e1b72782848c..9a924ff27148 100644
> --- a/drivers/gpu/drm/arm/malidp_crtc.c
> +++ b/drivers/gpu/drm/arm/malidp_crtc.c
> @@ -474,10 +474,7 @@ static void malidp_crtc_reset(struct drm_crtc *crtc)
>
> kfree(state);
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> - }
> + __drm_atomic_helper_crtc_reset(crtc, >base);
> }
>
> static void malidp_crtc_destroy_state(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> index 96f4082671fe..8084d549c7d1 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c
> @@ -412,10 +412,7 @@ static void atmel_hlcdc_crtc_reset(struct drm_crtc *crtc)
> }
>
> state = kzalloc(sizeof(*state), GFP_KERNEL);
> - if (state) {
> - crtc->state = >base;
> - crtc->state->crtc = crtc;
> - }
> + __drm_atomic_helper_crtc_reset(crtc, >base);
> }
>
> static struct drm_crtc_state *
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
>
Re: [PATCH] drm/amd/display: Fix misleading buffer information
+amdgfx, amdgpu specific patches should go here
On 2018-11-05 05:33 AM, Shaokun Zhang wrote:
> RETIMER_REDRIVER_INFO shows the buffer as a decimal value with a '0x'
> prefix, which is somewhat misleading.
>
> Fix it to print hexadecimal, as was intended.
>
> Fixes: 2f14bc89("drm/amd/display: add retimer log for HWQ tuning use.")
> Cc: Charlene Liu
> Cc: Dmytro Laktyushkin
> Cc: Leo Li
> Signed-off-by: Shaokun Zhang
Reviewed-by: Leo Li
Thanks!
Leo
> ---
> drivers/gpu/drm/amd/display/dc/core/dc_link.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index fb04a4a..5da2186 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -1722,7 +1722,7 @@ static void write_i2c_retimer_setting(
> i2c_success = i2c_write(pipe_ctx, slave_address,
> buffer, sizeof(buffer));
> RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\
> - offset = 0x%d, reg_val = 0x%d, i2c_success = %d\n",
> + offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n",
> slave_address, buffer[0], buffer[1], i2c_success?1:0);
> if (!i2c_success)
> /* Write failure */
> @@ -1734,7 +1734,7 @@ static void write_i2c_retimer_setting(
> i2c_success = i2c_write(pipe_ctx, slave_address,
> buffer, sizeof(buffer));
> RETIMER_REDRIVER_INFO("retimer write to slave_address = 0x%x,\
> - offset = 0x%d, reg_val = 0x%d, i2c_success = %d\n",
> + offset = 0x%x, reg_val = 0x%x, i2c_success = %d\n",
> slave_address, buffer[0], buffer[1], i2c_success?1:0);
> if (!i2c_success)
> /* Write failure */
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm: Get ref on CRTC commit object when waiting for flip_done
On 2018-10-16 08:33 AM, Daniel Vetter wrote:
> On Mon, Oct 15, 2018 at 09:46:40AM -0400, sunpeng...@amd.com wrote:
>> From: Leo Li
>>
>> This fixes a general protection fault, caused by accessing the contents
>> of a flip_done completion object that has already been freed. It occurs
>> due to the preemption of a non-blocking commit worker thread W by
>> another commit thread X. X continues to clear its atomic state at the
>> end, destroying the CRTC commit object that W still needs. Switching
>> back to W and accessing the commit objects then leads to bad results.
>>
>> Worker W becomes preemptable when waiting for flip_done to complete. At
>> this point, a frequently occurring commit thread X can take over. Here's
>> an example where W is a worker thread that flips on both CRTCs, and X
>> does a legacy cursor update on both CRTCs:
>>
>> ...
>> 1. W does flip work
>> 2. W runs commit_hw_done()
>> 3. W waits for flip_done on CRTC 1
>> 4. > flip_done for CRTC 1 completes
>> 5. W finishes waiting for CRTC 1
>> 6. W waits for flip_done on CRTC 2
>>
>> 7. > Preempted by X
>> 8. > flip_done for CRTC 2 completes
>> 9. X atomic_check: hw_done and flip_done are complete on all CRTCs
>> 10. X updates cursor on both CRTCs
>> 11. X destroys atomic state
>> 12. X done
>>
>> 13. > Switch back to W
>> 14. W waits for flip_done on CRTC 2
>> 15. W raises general protection fault
>>
>> The error looks like so:
>>
>> general protection fault: [#1] PREEMPT SMP PTI
>> **snip**
>> Call Trace:
>> lock_acquire+0xa2/0x1b0
>> _raw_spin_lock_irq+0x39/0x70
>> wait_for_completion_timeout+0x31/0x130
>> drm_atomic_helper_wait_for_flip_done+0x64/0x90 [drm_kms_helper]
>> amdgpu_dm_atomic_commit_tail+0xcae/0xdd0 [amdgpu]
>> commit_tail+0x3d/0x70 [drm_kms_helper]
>> process_one_work+0x212/0x650
>> worker_thread+0x49/0x420
>> kthread+0xfb/0x130
>> ret_from_fork+0x3a/0x50
>> Modules linked in: x86_pkg_temp_thermal amdgpu(O) chash(O)
>> gpu_sched(O) drm_kms_helper(O) syscopyarea sysfillrect sysimgblt
>> fb_sys_fops ttm(O) drm(O)
>>
>> Note that i915 has this issue masked, since hw_done is signaled after
>> waiting for flip_done. Doing so will block the cursor update from
>> happening until hw_done is signaled, preventing the cursor commit from
>> destroying the state.
>>
>> v2: The reference on the commit object needs to be obtained before
>> hw_done() is signaled, since that's the point where another commit
>> is allowed to modify the state. Assuming that the
>> new_crtc_state->commit object still exists within flip_done() is
>> incorrect.
>>
>> Fix by getting a reference in setup_commit(), and releasing it
>> during default_clear().
>>
>> Signed-off-by: Leo Li
>> ---
>>
>> Sending again, this time to the correct mailing list :)
>>
>> Leo
>
> Reviewed-by: Daniel Vetter
> Cc: sta...@vger.kernel.org
>
> I think it'd be really good if you could get intel-gfx-ci to test this
> patch. Simply submit it to intel-...@lists.freedesktop.org. I'll leave
> applying to one of the amd drm-misc committers, once it's passed CI.
>
> Cheers, Daniel
Thanks for the rb!
On the topic of CI, is it possible to write a test (maybe one already
exists) for this issue? I've attempted to do one here:
https://patchwork.freedesktop.org/patch/256319/
The problem is finding a surefire way to trigger the sequence, I'm not
sure how that can be done. If you have any ideas, I would love to hear them.
Leo
>
>>
>> drivers/gpu/drm/drm_atomic.c| 5 +
>> drivers/gpu/drm/drm_atomic_helper.c | 12
>> include/drm/drm_atomic.h| 11 +++
>> 3 files changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 3eb061e..12ae917 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -174,6 +174,11 @@ void drm_atomic_state_default_clear(struct
>> drm_atomic_state *state)
>> state->crtcs[i].state = NULL;
>> state->crtcs[i].old_state = NULL;
>> state->crtcs[i].new_state = NULL;
>> +
>> +if (state->crtcs[i].commit) {
>> +drm_crtc_commit_put(state->crtcs[i].commit);
>> +state->crtcs[i].commit = NULL;
>> +}
>> }
>>
>> for (i = 0; i < config->num_total_plane; i++) {
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c
>> index 80be74d..1bb4c31 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1408,15 +1408,16 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>> void drm_atomic_helper_wait_for_flip_done(struct drm_device *dev,
>>struct drm_atomic_state *old_state)
>>
