[Bug 207383] [Regression] 5.7 amdgpu/polaris11 gpf: amdgpu_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=207383 --- Comment #60 from Stratos Zolotas (str...@gmail.com) --- (In reply to Chan Cuan from comment #59) > I didn't have this issue prior to installing the 5.7.7 kernel though... To make things looks more strange... I have a non-explicable development with this issue. When it appeared to me I was in the middle of upgrading some components on my system. I replaced my AMD FX-8350 with one AMD Ryzen 5 3600X and my Gigabyte GA-970a-ds3p motherboard with one Gigabyte X570 UD (along with new RAM dimms from 16GB to 32GB). RX580 stayed the same and also OS is the same (disks moved to the new motherboard, no re-install). Guess what... running with 5.7.7 for 48 hours now without issues problem has disappeared. I suspect a very rare combination of things maybe even not in the amdgpu driver itself... With 5.7.7 on my "old" configuration, I had the crash almost immediately after login like in the above comment. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 207383] [Regression] 5.7 amdgpu/polaris11 gpf: amdgpu_atomic_commit_tail
https://bugzilla.kernel.org/show_bug.cgi?id=207383 --- Comment #59 from chancua...@gmail.com --- (In reply to Paul Menzel from comment #54) > (In reply to Stratos Zolotas from comment #53) > > > Don't know if it helps. I'm getting a similar issue on Opensuse Tumbleweed > > with kernel 5.7.7. Reverting to kernel 5.7.5 makes things stable for me. My > > GPU is RX580. > > […] > > Thank you for your report. How quickly can you reproduce it? If you could > bisect the issue to pinpoint the culprit commit between 5.7.5 and 5.7.7, > that’d be great. Maybe open even a separate bug report, in case they are > unrelated. They can always be marked as duplicates later. I am running the same setup as the comment. RX 580, Tumbleweed, have both kernels 5.7.5 and 5.7.7. On 5.7.7, it happens almost immediately after login. However, reverting to 5.7.5 does NOT stabilise, and the same problem arises somewhere between 1 to 10 minutes. I didn't have this issue prior to installing the 5.7.7 kernel though... -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/panel: panel-simple: validate panel description
Hi Sam,
(CC'ing Daniel)
Thank you for the patch.
On Sat, Jul 11, 2020 at 11:47:26AM +0200, Sam Ravnborg wrote:
> Warn is we detect a panel with missing descriptions.
s/is/if/
> This is inpsired by a similar patch by Laurent that introduced checks
> for LVDS panels - this extends the checks to the reminaing type of
s/reminaing type/remaining types/
> connectors.
>
> This is known to fail for some of the existing panels but added
> despite this as we need help from people using the panels to
> add the missing info.
> The checks are not complete but will catch the most common mistakes.
> The checks at the same time serves as documentation for the minimum
> required description for a panel.
>
> Signed-off-by: Sam Ravnborg
> Cc: Laurent Pinchart
> Cc: Thierry Reding
> Cc: Sam Ravnborg
> ---
>
> This is my attempt on the validation described in the previous mail.
> The assignment of default connector_type will then be a follow-up patch
> to this.
>
> Sam
>
> drivers/gpu/drm/panel/panel-simple.c | 32 ++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index 2aff93accad5..025a7ccdfcb3 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const
> struct panel_desc *desc)
> panel_simple_parse_panel_timing_node(dev, panel, );
> }
>
> - if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
> - /* Catch common mistakes for LVDS panels. */
> + /* Catch common mistakes for panels. */
> + switch (desc->connector_type) {
> + case 0:
> + WARN(desc->connector_type == 0, "specify missing
> connector_type\n");
> + break;
> + case DRM_MODE_CONNECTOR_LVDS:
> WARN_ON(desc->bus_flags &
> ~(DRM_BUS_FLAG_DE_LOW |
> DRM_BUS_FLAG_DE_HIGH |
> @@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const
> struct panel_desc *desc)
> WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
>desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA)
> &&
> desc->bpc != 8);
> + break;
> + case DRM_MODE_CONNECTOR_eDP:
> + WARN_ON(desc->bus_format == 0);
> + WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> + break;
> + case DRM_MODE_CONNECTOR_DSI:
> + WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> + break;
> + case DRM_MODE_CONNECTOR_DPI:
> + WARN_ON(desc->bus_flags &
> + ~(DRM_BUS_FLAG_DE_LOW |
> + DRM_BUS_FLAG_DE_HIGH |
> + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> + DRM_BUS_FLAG_DATA_MSB_TO_LSB |
> + DRM_BUS_FLAG_DATA_LSB_TO_MSB |
> + DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
> + DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE));
> + WARN_ON(desc->bus_format == 0);
> + WARN_ON(desc->bpc != 6 && desc->bpc != 8);
> + break;
> + default:
> + WARN(true, "panel has unknown connector_type: %d\n",
> desc->connector_type);
> + break;
> }
The checks look sane to me. For LVDS we've added the WARN_ON after
checking all LVDS panels [1], so the warning will only get displayed for
new panel drivers. For other types of panel, this will cause lots of
WARN_ON to trigger. On one hand it gets the issues noticed, which should
help fixing them, but on the other hand it will also scare lots of users
and developers. I'm not sure if we should downgrade that to a dev_warn()
for some time until we get at least the majority of the issues fixed.
Daniel, any opinion ?
[1] Actually not quite, I've just sent "[PATCH] drm: panel: simple: Fix
bpc for LG LB070WV8 panel" to fix one bpc issue.
> drm_panel_init(>base, dev, _simple_funcs,
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: panel: simple: Fix bpc for LG LB070WV8 panel
The LG LB070WV8 panel incorrectly reports a 16 bits per component value,
while the panel uses 8 bits per component. Fix it.
Fixes: dd0150026901 ("drm/panel: simple: Add support for LG LB070WV8 800x480 7"
panel")
Signed-off-by: Laurent Pinchart
---
drivers/gpu/drm/panel/panel-simple.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index 3a35f74d6cb7..dfb62821932a 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -2365,7 +2365,7 @@ static const struct drm_display_mode lg_lb070wv8_mode = {
static const struct panel_desc lg_lb070wv8 = {
.modes = _lb070wv8_mode,
.num_modes = 1,
- .bpc = 16,
+ .bpc = 8,
.size = {
.width = 151,
.height = 91,
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/4] drm: core: Convert logging to drm_* functions.
On Sat, 2020-07-11 at 20:41 +0530, Suraj Upadhyay wrote: > On Fri, Jul 10, 2020 at 07:56:43PM +0200, Sam Ravnborg wrote: > > Hi Suraj. > > > > On Tue, Jul 07, 2020 at 10:04:14PM +0530, Suraj Upadhyay wrote: > > > This patchset converts logging to drm_* functions in drm core. > > > > > > The following functions have been converted to their respective > > > DRM alternatives : > > > dev_info() --> drm_info() > > > dev_err() --> drm_err() > > > dev_warn() --> drm_warn() > > > dev_err_once() --> drm_err_once(). > > > > I would prefer that DRM_* logging in the same files are converted in the > > same patch. So we have one logging conversion patch for each file you > > touches and that we do not need to re-vist the files later to change > > another set of logging functions. > > Agreed. > > > If possible WARN_* should also be converted to drm_WARN_* > > If patch is too large, then split them up but again lets have all > > logging updated when we touch a file. > > > > Care to take a look at this approach? > > > > Hii, > The problem with WARN_* macros is that they are used without any > drm device context. For example [this use > here](https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_edid.c#n1667) > in drm_edid.c, > doesn't have a drm device context and only has one argument (namely > !raw_edid). > There are many more such use cases. > > And also there were cases where dev_* logging functions didn't have any > drm_device context. Perhaps change the __drm_printk macro to not dereference the drm argument when NULL. A trivial but perhaps inefficient way might be used like: drm_(NULL, fmt, ...) --- include/drm/drm_print.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index 1c9417430d08..9323a8f46b3c 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -395,8 +395,8 @@ void drm_dev_dbg(const struct device *dev, enum drm_debug_category category, /* Helper for struct drm_device based logging. */ #define __drm_printk(drm, level, type, fmt, ...) \ - dev_##level##type((drm)->dev, "[drm] " fmt, ##__VA_ARGS__) - + dev_##level##type((drm) ? (drm)->dev : NULL, "[drm] " fmt, \ + ##__VA_ARGS__) #define drm_info(drm, fmt, ...)\ __drm_printk((drm), info,, fmt, ##__VA_ARGS__) ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[radeon-alex:amd-staging-drm-next 677/1015] drivers/gpu/drm/amd/amdgpu/../powerplay/sienna_cichlid_ppt.c:1734:5: warning: no previous prototype for function 'sienna_cichlid_set_soft_freq_limited_range
tree: git://people.freedesktop.org/~agd5f/linux.git amd-staging-drm-next
head: 3c831e196bd7543977d4acd506064636809f1dcf
commit: ac7413ecad5e406065529cda0adaa29c353cc557 [677/1015] drm/amd/amdgpu:
disable gfxoff to retrieve gfxclk
config: powerpc64-randconfig-r004-20200710 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project
02946de3802d3bc65bc9f2eb9b8d4969b5a7add8)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install powerpc64 cross compiling tool for clang build
# apt-get install binutils-powerpc64-linux-gnu
git checkout ac7413ecad5e406065529cda0adaa29c353cc557
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross
ARCH=powerpc64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
drivers/gpu/drm/amd/amdgpu/../powerplay/sienna_cichlid_ppt.c:1718:5:
warning: no previous prototype for function
'sienna_cichlid_get_dpm_ultimate_freq' [-Wmissing-prototypes]
int sienna_cichlid_get_dpm_ultimate_freq(struct smu_context *smu,
^
drivers/gpu/drm/amd/amdgpu/../powerplay/sienna_cichlid_ppt.c:1718:1: note:
declare 'static' if the function is not intended to be used outside of this
translation unit
int sienna_cichlid_get_dpm_ultimate_freq(struct smu_context *smu,
^
static
>> drivers/gpu/drm/amd/amdgpu/../powerplay/sienna_cichlid_ppt.c:1734:5:
>> warning: no previous prototype for function
>> 'sienna_cichlid_set_soft_freq_limited_range' [-Wmissing-prototypes]
int sienna_cichlid_set_soft_freq_limited_range(struct smu_context *smu,
^
drivers/gpu/drm/amd/amdgpu/../powerplay/sienna_cichlid_ppt.c:1734:1: note:
declare 'static' if the function is not intended to be used outside of this
translation unit
int sienna_cichlid_set_soft_freq_limited_range(struct smu_context *smu,
^
static
2 warnings generated.
vim +/sienna_cichlid_set_soft_freq_limited_range +1734
drivers/gpu/drm/amd/amdgpu/../powerplay/sienna_cichlid_ppt.c
1717
> 1718 int sienna_cichlid_get_dpm_ultimate_freq(struct smu_context *smu,
1719 enum smu_clk_type clk_type,
1720 uint32_t *min, uint32_t *max)
1721 {
1722 struct amdgpu_device *adev = smu->adev;
1723 int ret;
1724
1725 if (clk_type == SMU_GFXCLK)
1726 amdgpu_gfx_off_ctrl(adev, false);
1727 ret = smu_v11_0_get_dpm_ultimate_freq(smu, clk_type, min, max);
1728 if (clk_type == SMU_GFXCLK)
1729 amdgpu_gfx_off_ctrl(adev, true);
1730
1731 return ret;
1732 }
1733
> 1734 int sienna_cichlid_set_soft_freq_limited_range(struct smu_context *smu,
1735enum smu_clk_type clk_type,
1736uint32_t min, uint32_t max)
1737 {
1738 struct amdgpu_device *adev = smu->adev;
1739 int ret;
1740
1741 if (clk_type == SMU_GFXCLK)
1742 amdgpu_gfx_off_ctrl(adev, false);
1743 ret = smu_v11_0_set_soft_freq_limited_range(smu, clk_type, min,
max);
1744 if (clk_type == SMU_GFXCLK)
1745 amdgpu_gfx_off_ctrl(adev, true);
1746
1747 return ret;
1748 }
1749
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[radeon-alex:amd-staging-drm-next 628/1015] drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10_3.c:268:17: sparse: sparse: cast removes address space '' of expression
tree: git://people.freedesktop.org/~agd5f/linux.git amd-staging-drm-next
head: 3c831e196bd7543977d4acd506064636809f1dcf
commit: 10be8791067fc672e44fcaa7afb886390909a0c0 [628/1015] drm/amdkfd: Support
Sienna_Cichlid KFD v4
config: x86_64-randconfig-s022-20200710 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-37-gc9676a3b-dirty
git checkout 10be8791067fc672e44fcaa7afb886390909a0c0
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10_3.c:268:17: sparse: sparse:
>> cast removes address space '' of expression
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10_3.c:270:17: sparse: sparse:
cast removes address space '' of expression
vim +268 drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gfx_v10_3.c
192
193 static int hqd_load_v10_3(struct kgd_dev *kgd, void *mqd, uint32_t
pipe_id,
194 uint32_t queue_id, uint32_t __user *wptr,
195 uint32_t wptr_shift, uint32_t wptr_mask,
196 struct mm_struct *mm)
197 {
198 struct amdgpu_device *adev = get_amdgpu_device(kgd);
199 struct v10_compute_mqd *m;
200 uint32_t *mqd_hqd;
201 uint32_t reg, hqd_base, data;
202
203 m = get_mqd(mqd);
204
205 pr_debug("Load hqd of pipe %d queue %d\n", pipe_id, queue_id);
206 acquire_queue(kgd, pipe_id, queue_id);
207
208 /* HIQ is set during driver init period with vmid set to 0*/
209 if (m->cp_hqd_vmid == 0) {
210 uint32_t value, mec, pipe;
211
212 mec = (pipe_id / adev->gfx.mec.num_pipe_per_mec) + 1;
213 pipe = (pipe_id % adev->gfx.mec.num_pipe_per_mec);
214
215 pr_debug("kfd: set HIQ, mec:%d, pipe:%d, queue:%d.\n",
216 mec, pipe, queue_id);
217 value = RREG32(SOC15_REG_OFFSET(GC, 0,
mmRLC_CP_SCHEDULERS));
218 value = REG_SET_FIELD(value, RLC_CP_SCHEDULERS,
scheduler1,
219 ((mec << 5) | (pipe << 3) | queue_id | 0x80));
220 WREG32(SOC15_REG_OFFSET(GC, 0, mmRLC_CP_SCHEDULERS),
value);
221 }
222
223 /* HQD registers extend from CP_MQD_BASE_ADDR to
CP_HQD_EOP_WPTR_MEM. */
224 mqd_hqd = >cp_mqd_base_addr_lo;
225 hqd_base = SOC15_REG_OFFSET(GC, 0, mmCP_MQD_BASE_ADDR);
226
227 for (reg = hqd_base;
228 reg <= SOC15_REG_OFFSET(GC, 0, mmCP_HQD_PQ_WPTR_HI); reg++)
229 WREG32(reg, mqd_hqd[reg - hqd_base]);
230
231
232 /* Activate doorbell logic before triggering WPTR poll. */
233 data = REG_SET_FIELD(m->cp_hqd_pq_doorbell_control,
234 CP_HQD_PQ_DOORBELL_CONTROL, DOORBELL_EN,
1);
235 WREG32(SOC15_REG_OFFSET(GC, 0, mmCP_HQD_PQ_DOORBELL_CONTROL),
data);
236
237 if (wptr) {
238 /* Don't read wptr with get_user because the user
239 * context may not be accessible (if this function
240 * runs in a work queue). Instead trigger a one-shot
241 * polling read from memory in the CP. This assumes
242 * that wptr is GPU-accessible in the queue's VMID via
243 * ATC or SVM. WPTR==RPTR before starting the poll so
244 * the CP starts fetching new commands from the right
245 * place.
246 *
247 * Guessing a 64-bit WPTR from a 32-bit RPTR is a bit
248 * tricky. Assume that the queue didn't overflow. The
249 * number of valid bits in the 32-bit RPTR depends on
250 * the queue size. The remaining bits are taken from
251 * the saved 64-bit WPTR. If the WPTR wrapped, add the
252 * queue size.
253 */
254 uint32_t queue_size =
255 2 << REG_GET_FIELD(m->cp_hqd_pq_control,
256 CP_HQD_PQ_CONTROL,
QUEUE_SIZE);
257 uint64_t guessed_wptr = m->cp_hqd_pq_rptr & (queue_size
- 1);
258
259 if ((m->cp_hqd_pq_wptr_lo & (queue_size - 1)) <
guessed_wptr)
260 guessed_wptr += queue_size;
261 guessed_wptr += m->cp_hqd_pq_wptr_lo & ~(queue_size -
1);
262 guessed_wptr
Re: [PATCH 1/2] drm/msm: sync generated headers
On Sat, Jul 11, 2020 at 4:49 AM Linus Walleij wrote: > > On Tue, Jul 7, 2020 at 10:36 PM Rob Clark wrote: > > > From: Rob Clark > > > > We haven't sync'd for a while.. pull in updates to get definitions for > > some fields in pkt7 payloads. > > > > Signed-off-by: Rob Clark > > Out of curiosity : where are the syncs coming from? Mesa? sometimes indirectly.. but they are generated from: https://github.com/freedreno/envytools/tree/master/rnndb The cmdstream and devcoredump decoding tools (which also use the xml) are in the envytools tree as well. We have a copy of the gpu side xml in mesa, where we generate the headers at build time, but I guess doing that on the kernel side would introduce some build time dependencies that others wouldn't appreciate: https://gitlab.freedesktop.org/mesa/mesa/-/tree/master/src/freedreno/registers Mesa already depends a lot on py generated headers, tables, etc. BR, -R > Yours, > Linus Walleij ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0
Hi,
On 7/11/20 8:11 AM, Uwe Kleine-König wrote:
On Thu, Jul 09, 2020 at 05:47:59PM +0200, Hans de Goede wrote:
Hi,
On 7/9/20 4:50 PM, Andy Shevchenko wrote:
On Wed, Jul 08, 2020 at 11:14:22PM +0200, Hans de Goede wrote:
The datasheet specifies that programming the base_unit part of the
ctrl register to 0 results in a contineous low signal.
Adjust the get_state method to reflect this by setting pwm_state.period
to 1 and duty_cycle to 0.
...
+ if (freq == 0) {
+ /* In this case the PWM outputs a continous low signal */
+ state->period = 1;
I guess this should be something like half of the range (so base unit calc
will give 128). Because with period = 1 (too small) it will give too small
base unit (if apply) and as a result we get high frequency pulses.
You are right, that if after this the user only changes the duty-cycle
things will work very poorly, we will end up with a base_unit value of
e.g 65535 and then have almost no duty-cycle resolution at all.
Is this a problem of the consumer that we don't need to solve? Are there
known consumers running into this problem?
AFAICT we never ever actually see freq == 0 here, this is just a code-path
to avoid a divide by 0 in case we somehow mysteriously do get freq == 0
here.
On boot the PWM controller is either not used and then the default freq =
input-clock / 256, or it is used and programmed to same sane value.
pwm_lpss_prepare() is buggy here, a request for a too low period should be
refused.
So instead of clamping as is done in an earlier patch, we should return
-EINVAL ? Only for too low periods, or also for too high periods ?
I must say this does worry me a bit, the VBT may request 200Hz output
frequency and some revisions of the PWM controller can do 283Hz as
lowest output freq. ATM we just give the i915 code the 283 Hz if it
request 200, that seems more sane then to give it -EINVAL, since -EINVAL
would require the i915 driver to know the exact limits of each PWM
controller and then to clamp the VBT value before passing it to the
PWM driver, that means moving knowledge out of the PWM driver into
the i915 code.
I believe that without first amending the PWM API too allow a consumer
to query the period min/max values, returning -EINVAL is not the right
thing to do here.
Regards,
Hans
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller
Hi,
On 7/11/20 8:32 AM, Uwe Kleine-König wrote:
On Wed, Jul 08, 2020 at 11:14:32PM +0200, Hans de Goede wrote:
Now that the PWM drivers which we use have been converted to the atomic
PWM API, we can move the i915 panel code over to using the atomic PWM API.
The removes a long standing FIXME and this removes a flicker where
the backlight brightness would jump to 100% when i915 loads even if
using the fastset path.
Note that this commit also simplifies pwm_disable_backlight(), by dropping
the intel_panel_actually_set_backlight(..., 0) call. This call sets the
PWM to 0% duty-cycle. I believe that this call was only present as a
workaround for a bug in the pwm-crc.c driver where it failed to clear the
PWM_OUTPUT_ENABLE bit. This is fixed by an earlier patch in this series.
After the dropping of this workaround, the usleep call, which seems
unnecessary to begin with, has no useful effect anymore, so drop that too.
Acked-by: Jani Nikula
Signed-off-by: Hans de Goede
---
Changes in v4:
- Add a note to the commit message about the dropping of the
intel_panel_actually_set_backlight() and usleep() calls from
pwm_disable_backlight()
- Use the pwm_set/get_relative_duty_cycle() helpers instead of using DIY code
for this
---
.../drm/i915/display/intel_display_types.h| 3 +-
drivers/gpu/drm/i915/display/intel_panel.c| 71 +--
2 files changed, 34 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
b/drivers/gpu/drm/i915/display/intel_display_types.h
index de32f9efb120..4bd9981e70a1 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -28,6 +28,7 @@
#include
#include
+#include
#include
#include
@@ -223,7 +224,7 @@ struct intel_panel {
bool util_pin_active_low; /* bxt+ */
u8 controller; /* bxt+ only */
struct pwm_device *pwm;
- int pwm_period_ns;
+ struct pwm_state pwm_state;
/* DPCD backlight */
u8 pwmgen_bit_count;
diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
b/drivers/gpu/drm/i915/display/intel_panel.c
index cb28b9908ca4..3d97267c8238 100644
--- a/drivers/gpu/drm/i915/display/intel_panel.c
+++ b/drivers/gpu/drm/i915/display/intel_panel.c
@@ -592,10 +592,10 @@ static u32 bxt_get_backlight(struct intel_connector
*connector)
static u32 pwm_get_backlight(struct intel_connector *connector)
{
struct intel_panel *panel = >panel;
- int duty_ns;
+ struct pwm_state state;
- duty_ns = pwm_get_duty_cycle(panel->backlight.pwm);
- return DIV_ROUND_UP(duty_ns * 100, panel->backlight.pwm_period_ns);
+ pwm_get_state(panel->backlight.pwm, );
+ return pwm_get_relative_duty_cycle(, 100);
Here you introduce a slight difference: pwm_get_relative_duty_cycle uses
round-closest while you replace a round-up. Is this relevant?
Yes I'm aware of the change in rounding and I do not believe that it is
relevant. One of the advantages of switching to the helpers is not having
to worry about the rounding and letting the helpers figure that out :)
}
static void lpt_set_backlight(const struct drm_connector_state *conn_state, u32 level)
@@ -669,10 +669,9 @@ static void bxt_set_backlight(const struct
drm_connector_state *conn_state, u32
static void pwm_set_backlight(const struct drm_connector_state *conn_state,
u32 level)
{
struct intel_panel *panel =
_intel_connector(conn_state->connector)->panel;
- int duty_ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
- pwm_config(panel->backlight.pwm, duty_ns,
- panel->backlight.pwm_period_ns);
+ pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100);
+ pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
Similar here: The function used to use round-up but
pwm_set_relative_duty_cycle() used round-closest.
Idem.
}
static void
@@ -841,10 +840,8 @@ static void pwm_disable_backlight(const struct
drm_connector_state *old_conn_sta
struct intel_connector *connector =
to_intel_connector(old_conn_state->connector);
struct intel_panel *panel = >panel;
- /* Disable the backlight */
- intel_panel_actually_set_backlight(old_conn_state, 0);
- usleep_range(2000, 3000);
- pwm_disable(panel->backlight.pwm);
+ panel->backlight.pwm_state.enabled = false;
+ pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
}
void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state)
@@ -1176,9 +1173,12 @@ static void pwm_enable_backlight(const struct
intel_crtc_state *crtc_state,
{
struct intel_connector *connector =
to_intel_connector(conn_state->connector);
struct intel_panel *panel = >panel;
+ int level = panel->backlight.level;
- pwm_enable(panel->backlight.pwm);
Re: [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
Hi, On 7/11/20 8:19 AM, Uwe Kleine-König wrote: Hi Hans, On Thu, Jul 09, 2020 at 04:40:56PM +0200, Hans de Goede wrote: On 7/9/20 4:14 PM, Sam Ravnborg wrote: On Wed, Jul 08, 2020 at 11:14:16PM +0200, Hans de Goede wrote: Here is v4 of my patch series converting the i915 driver's code for controlling the panel's backlight with an external PWM controller to use the atomic PWM API. See below for the changelog. Why is it that i915 cannot use the pwm_bl driver for backlight? I have not studied the code - just wondering. The intel_panel.c code deals with 7 different types of PWM controllers which are built into the GPU + support for external PWM controllers through the kernel's PWM subsystem. pwm_bl will work for the external PWM controller case, but not for the others. On top of that the intel_panel code integrates which the video BIOS, getting things like frequency, minimum value and if the range is inverted (0% duty == backlight brightness max). I'm not even sure if pwm_bl supports all of this, but even if it does the intel_panel code handles this in a unified manner for all supported PWM controllers, including the ones which are an integral part of the GPU. pwm_bl handles inverted PWM just fine. I'm unsure what "integrates which the video BIOS" means, Integrating with the video BIOS means reading the VBT (Video BIOS Tables) and extracting info about which PWM controller to use, what frequency to program the output at, minimum allowed duty-cycle and if the scale is inverted. but I don't see how "handling 7 different types of PWM controllers explicitly and others using the PWM API" can be seen as "unified manner" compared to "provide a pwm driver for whatever might be in the GPU and then use generic code (PWM API, pwm_bl) to drive it". Part of this is historical, the main x86 GPU drivers have always treated backlight control as integral part of the display pipeline and in some cases it really is, e.g. for eDP panels in some cases the backlight is controlled through the DP aux channel, there is no PWM controller (visible to the kernel involved). So the intel_panel.c code really is a backlight-control de-multiplexer, picking the right "plugin" to control the backlight, which may also be the eDP backlight control code. Using a PWM controller supported by the PWM-core/class is just one of the many supported "plugins". Also the GPU currently is treated as a single device, not as a MFD device, so we cannot have an isolated PWM driver. We could have code inside the GPU driver which exports a PWM-controller to the PWM-core, to then get a reference to the exported PWM-controller but that would be very roundabout. The devices which are using an external PWM controller are actually the exception here, 99.9% of all devices use the GPU integrated PWM controller. Anyways changing over the other PWM-like controllers support by the intel-panel code falls way outside of the scope of this patch-set. Regards, Hans ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[radeon-alex:amd-staging-drm-next 468/1015] drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:172:15: sparse: sparse: incorrect type in argument 1 (different address spaces)
tree: git://people.freedesktop.org/~agd5f/linux.git amd-staging-drm-next
head: 3c831e196bd7543977d4acd506064636809f1dcf
commit: 3a2b9affb4c366dac8a088156c644cf329701816 [468/1015] drm/amdkfd: Track
SDMA utilization per process
config: x86_64-randconfig-s022-20200710 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-37-gc9676a3b-dirty
git checkout 3a2b9affb4c366dac8a088156c644cf329701816
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
sparse warnings: (new ones prefixed by >>)
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:140:6:
sparse: sparse: symbol 'increment_queue_count' was not declared. Should it be
static?
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:148:6:
sparse: sparse: symbol 'decrement_queue_count' was not declared. Should it be
static?
>> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:172:15:
>> sparse: sparse: incorrect type in argument 1 (different address spaces) @@
>> expected void const volatile [noderef] * @@ got unsigned long
>> long [usertype] * @@
>> drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:172:15:
>> sparse: expected void const volatile [noderef] *
drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c:172:15:
sparse: got unsigned long long [usertype] *
vim +172 drivers/gpu/drm/amd/amdgpu/../amdkfd/kfd_device_queue_manager.c
139
> 140 void increment_queue_count(struct device_queue_manager *dqm,
141 enum kfd_queue_type type)
142 {
143 dqm->active_queue_count++;
144 if (type == KFD_QUEUE_TYPE_COMPUTE || type ==
KFD_QUEUE_TYPE_DIQ)
145 dqm->active_cp_queue_count++;
146 }
147
148 void decrement_queue_count(struct device_queue_manager *dqm,
149 enum kfd_queue_type type)
150 {
151 dqm->active_queue_count--;
152 if (type == KFD_QUEUE_TYPE_COMPUTE || type ==
KFD_QUEUE_TYPE_DIQ)
153 dqm->active_cp_queue_count--;
154 }
155
156 int read_sdma_queue_counter(struct queue *q, uint64_t *val)
157 {
158 int ret;
159 uint64_t tmp = 0;
160
161 if (!q || !val)
162 return -EINVAL;
163 /*
164 * SDMA activity counter is stored at queue's RPTR + 0x8
location.
165 */
166 if (!access_ok((const void __user
*)((uint64_t)q->properties.read_ptr +
167 sizeof(uint64_t)),
sizeof(uint64_t))) {
168 pr_err("Can't access sdma queue activity counter\n");
169 return -EFAULT;
170 }
171
> 172 ret = get_user(tmp, (uint64_t
> *)((uint64_t)(q->properties.read_ptr) +
173 sizeof(uint64_t)));
174 if (!ret) {
175 *val = tmp;
176 }
177
178 return ret;
179 }
180
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/msm: sync generated headers
On Tue, Jul 7, 2020 at 10:36 PM Rob Clark wrote: > From: Rob Clark > > We haven't sync'd for a while.. pull in updates to get definitions for > some fields in pkt7 payloads. > > Signed-off-by: Rob Clark Out of curiosity : where are the syncs coming from? Mesa? Yours, Linus Walleij ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/panel: panel-simple: validate panel description
Warn is we detect a panel with missing descriptions.
This is inpsired by a similar patch by Laurent that introduced checks
for LVDS panels - this extends the checks to the reminaing type of
connectors.
This is known to fail for some of the existing panels but added
despite this as we need help from people using the panels to
add the missing info.
The checks are not complete but will catch the most common mistakes.
The checks at the same time serves as documentation for the minimum
required description for a panel.
Signed-off-by: Sam Ravnborg
Cc: Laurent Pinchart
Cc: Thierry Reding
Cc: Sam Ravnborg
---
This is my attempt on the validation described in the previous mail.
The assignment of default connector_type will then be a follow-up patch
to this.
Sam
drivers/gpu/drm/panel/panel-simple.c | 32 ++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c
b/drivers/gpu/drm/panel/panel-simple.c
index 2aff93accad5..025a7ccdfcb3 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const
struct panel_desc *desc)
panel_simple_parse_panel_timing_node(dev, panel, );
}
- if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) {
- /* Catch common mistakes for LVDS panels. */
+ /* Catch common mistakes for panels. */
+ switch (desc->connector_type) {
+ case 0:
+ WARN(desc->connector_type == 0, "specify missing
connector_type\n");
+ break;
+ case DRM_MODE_CONNECTOR_LVDS:
WARN_ON(desc->bus_flags &
~(DRM_BUS_FLAG_DE_LOW |
DRM_BUS_FLAG_DE_HIGH |
@@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const
struct panel_desc *desc)
WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG ||
desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA)
&&
desc->bpc != 8);
+ break;
+ case DRM_MODE_CONNECTOR_eDP:
+ WARN_ON(desc->bus_format == 0);
+ WARN_ON(desc->bpc != 6 && desc->bpc != 8);
+ break;
+ case DRM_MODE_CONNECTOR_DSI:
+ WARN_ON(desc->bpc != 6 && desc->bpc != 8);
+ break;
+ case DRM_MODE_CONNECTOR_DPI:
+ WARN_ON(desc->bus_flags &
+ ~(DRM_BUS_FLAG_DE_LOW |
+ DRM_BUS_FLAG_DE_HIGH |
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
+ DRM_BUS_FLAG_DATA_MSB_TO_LSB |
+ DRM_BUS_FLAG_DATA_LSB_TO_MSB |
+ DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE |
+ DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE));
+ WARN_ON(desc->bus_format == 0);
+ WARN_ON(desc->bpc != 6 && desc->bpc != 8);
+ break;
+ default:
+ WARN(true, "panel has unknown connector_type: %d\n",
desc->connector_type);
+ break;
}
drm_panel_init(>base, dev, _simple_funcs,
--
2.25.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 11/20] Documentation: leds/ledtrig-transient: eliminate duplicated word
On Tue 2020-07-07 11:04:05, Randy Dunlap wrote: > Drop the doubled word "for". > > Signed-off-by: Randy Dunlap > Cc: Jonathan Corbet > Cc: linux-...@vger.kernel.org > Cc: Jacek Anaszewski Acked-by: Pavel Machek (I expect documentation people take this, not me). -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html signature.asc Description: Digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/atomic_helper: add a flag for duplicating drm_private_obj state
On Sat, Jun 27, 2020 at 7:53 PM Shawn Guo wrote: > > From: Shawn Guo > > The msm/mdp5 driver uses state of drm_private_obj as its global atomic > state, which keeps the assignment of hwpipe to plane. With > drm_private_obj missing from duplicate state call in context of atomic > suspend/resume helpers, mdp5 suspend works with no problem only for the > very first time. Any subsequent suspend will hit the following warning, > because hwpipe assignment doesn't get duplicated for suspend state. > > $ echo mem > /sys/power/state > [ 38.44] PM: suspend entry (deep) > [ 38.85] PM: Syncing filesystems ... done. > [ 38.114630] Freezing user space processes ... (elapsed 0.001 seconds) done. > [ 38.115912] OOM killer disabled. > [ 38.115914] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) > done. > [ 38.122170] [ cut here ] > [ 38.122212] WARNING: CPU: 0 PID: 1747 at > drivers/gpu/drm/msm/disp/mdp5/mdp5_pipe.c:145 mdp5_pipe_release+0x90/0xc0 > [ 38.122215] Modules linked in: > [ 38.12] CPU: 0 PID: 1747 Comm: sh Not tainted > 4.19.107-00515-g9d5e4d7a33ed-dirty #323 > [ 38.14] Hardware name: Square, Inc. T2 Devkit (DT) > [ 38.18] pstate: 4005 (nZcv daif -PAN -UAO) > [ 38.122230] pc : mdp5_pipe_release+0x90/0xc0 > [ 38.122233] lr : mdp5_pipe_release+0x90/0xc0 > [ 38.122235] sp : 0d13b7f0 > [ 38.122236] x29: 0d13b7f0 x28: > [ 38.122240] x27: 0002 x26: 800079adce00 > [ 38.122243] x25: 800079405200 x24: > [ 38.122246] x23: 80007a78cc08 x22: 80007b1cc018 > [ 38.122249] x21: 80007b1cc000 x20: 80007b317080 > [ 38.122252] x19: 80007a78ce80 x18: 0002 > [ 38.122255] x17: x16: > [ 38.122258] x15: fff0 x14: 08c3fb48 > [ 38.122261] x13: 08cdac4a x12: 08c3f000 > [ 38.122264] x11: x10: 08cda000 > [ 38.122267] x9 : x8 : 08ce4a40 > [ 38.122269] x7 : x6 : 39ea41a9 > [ 38.122272] x5 : x4 : > [ 38.122275] x3 : x2 : c7580c109cae4500 > [ 38.122278] x1 : x0 : 0024 > [ 38.122281] Call trace: > [ 38.122285] mdp5_pipe_release+0x90/0xc0 > [ 38.122288] mdp5_plane_atomic_check+0x2c0/0x448 > [ 38.122294] drm_atomic_helper_check_planes+0xd0/0x208 > [ 38.122298] drm_atomic_helper_check+0x38/0xa8 > [ 38.122302] drm_atomic_check_only+0x3e8/0x630 > [ 38.122305] drm_atomic_commit+0x18/0x58 > [ 38.122309] __drm_atomic_helper_disable_all.isra.12+0x15c/0x1a8 > [ 38.122312] drm_atomic_helper_suspend+0x80/0xf0 > [ 38.122316] msm_pm_suspend+0x4c/0x70 > [ 38.122320] dpm_run_callback.isra.6+0x20/0x68 > [ 38.122323] __device_suspend+0x110/0x308 > [ 38.122326] dpm_suspend+0x100/0x1f0 > [ 38.122329] dpm_suspend_start+0x64/0x70 > [ 38.122334] suspend_devices_and_enter+0x110/0x500 > [ 38.122336] pm_suspend+0x268/0x2c0 > [ 38.122339] state_store+0x88/0x110 > [ 38.122345] kobj_attr_store+0x14/0x28 > [ 38.122352] sysfs_kf_write+0x3c/0x50 > [ 38.122355] kernfs_fop_write+0x118/0x1e0 > [ 38.122360] __vfs_write+0x30/0x168 > [ 38.122363] vfs_write+0xa4/0x1a8 > [ 38.122366] ksys_write+0x64/0xe8 > [ 38.122368] __arm64_sys_write+0x18/0x20 > [ 38.122374] el0_svc_common+0x6c/0x178 > [ 38.122377] el0_svc_compat_handler+0x1c/0x28 > [ 38.122381] el0_svc_compat+0x8/0x18 > [ 38.122383] ---[ end trace 24145b7d8545345b ]--- > [ 38.491552] Disabling non-boot CPUs ... > > Let's add a flag for duplicating the state of drm_private_obj and set > the flag from msm/mdp5 driver, so that the problem can be fixed while > other drivers using drm_private_obj stay unaffected. > > Signed-off-by: Shawn Guo > --- > Changes for v2: > - Add a flag to duplicate the state of drm_private_obj conditionally, >so that other drivers using drm_private_obj stay unaffected. Hi Daniel, Are you okay with this version? Shawn ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 02/21] drm/panel: panel-simple: add default connector_type
Hi Laurent. On Sat, Jul 11, 2020 at 01:11:24AM +0300, Laurent Pinchart wrote: > Hi Sam, > > Thank you for the patch. > > On Fri, Jul 03, 2020 at 09:23:58PM +0200, Sam Ravnborg wrote: > > All panels shall report a connector type. > > panel-simple has a lot of panels with no connector_type, > > and for these fall back to DPI as the default. > > > > Signed-off-by: Sam Ravnborg > > Cc: Thierry Reding > > Cc: Sam Ravnborg > > --- > > drivers/gpu/drm/panel/panel-simple.c | 10 -- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/panel/panel-simple.c > > b/drivers/gpu/drm/panel/panel-simple.c > > index 7b5d212215e0..b3ec965721b0 100644 > > --- a/drivers/gpu/drm/panel/panel-simple.c > > +++ b/drivers/gpu/drm/panel/panel-simple.c > > @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const > > struct panel_desc *desc) > > struct panel_simple *panel; > > struct display_timing dt; > > struct device_node *ddc; > > + int connector_type; > > int err; > > > > panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); > > @@ -566,8 +567,13 @@ static int panel_simple_probe(struct device *dev, > > const struct panel_desc *desc) > > desc->bpc != 8); > > } > > > > - drm_panel_init(>base, dev, _simple_funcs, > > - desc->connector_type); > > + /* Default DRM_MODE_CONNECTOR_DPI if no connector_type is set */ > > + if (desc->connector_type != 0) > > + connector_type = desc->connector_type; > > + else > > + connector_type = DRM_MODE_CONNECTOR_DPI; > > This avoids a WARN_ON(), which is good, but should we add a dev_warn() > to get this fixed ? We need a proper check for all relevant properties for DPI and the other connectors. Like you already did for LVDS. The idea is we shall introduce the checks in one series, so users when they fix the warnings they are all good. And then we will have to catch less during review which is good. It is on my TODO list, but wanted to have some other stuff done first. So in other words, good to go with this patch or do we need the proper checks in place first? Sam > > > + > > + drm_panel_init(>base, dev, _simple_funcs, connector_type); > > > > err = drm_panel_of_backlight(>base); > > if (err) > > -- > Regards, > > Laurent Pinchart > ___ > 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 v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller
On Wed, Jul 08, 2020 at 11:14:32PM +0200, Hans de Goede wrote:
> Now that the PWM drivers which we use have been converted to the atomic
> PWM API, we can move the i915 panel code over to using the atomic PWM API.
>
> The removes a long standing FIXME and this removes a flicker where
> the backlight brightness would jump to 100% when i915 loads even if
> using the fastset path.
>
> Note that this commit also simplifies pwm_disable_backlight(), by dropping
> the intel_panel_actually_set_backlight(..., 0) call. This call sets the
> PWM to 0% duty-cycle. I believe that this call was only present as a
> workaround for a bug in the pwm-crc.c driver where it failed to clear the
> PWM_OUTPUT_ENABLE bit. This is fixed by an earlier patch in this series.
>
> After the dropping of this workaround, the usleep call, which seems
> unnecessary to begin with, has no useful effect anymore, so drop that too.
>
> Acked-by: Jani Nikula
> Signed-off-by: Hans de Goede
> ---
> Changes in v4:
> - Add a note to the commit message about the dropping of the
> intel_panel_actually_set_backlight() and usleep() calls from
> pwm_disable_backlight()
> - Use the pwm_set/get_relative_duty_cycle() helpers instead of using DIY code
> for this
> ---
> .../drm/i915/display/intel_display_types.h| 3 +-
> drivers/gpu/drm/i915/display/intel_panel.c| 71 +--
> 2 files changed, 34 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index de32f9efb120..4bd9981e70a1 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -28,6 +28,7 @@
>
> #include
> #include
> +#include
> #include
>
> #include
> @@ -223,7 +224,7 @@ struct intel_panel {
> bool util_pin_active_low; /* bxt+ */
> u8 controller; /* bxt+ only */
> struct pwm_device *pwm;
> - int pwm_period_ns;
> + struct pwm_state pwm_state;
>
> /* DPCD backlight */
> u8 pwmgen_bit_count;
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c
> b/drivers/gpu/drm/i915/display/intel_panel.c
> index cb28b9908ca4..3d97267c8238 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -592,10 +592,10 @@ static u32 bxt_get_backlight(struct intel_connector
> *connector)
> static u32 pwm_get_backlight(struct intel_connector *connector)
> {
> struct intel_panel *panel = >panel;
> - int duty_ns;
> + struct pwm_state state;
>
> - duty_ns = pwm_get_duty_cycle(panel->backlight.pwm);
> - return DIV_ROUND_UP(duty_ns * 100, panel->backlight.pwm_period_ns);
> + pwm_get_state(panel->backlight.pwm, );
> + return pwm_get_relative_duty_cycle(, 100);
Here you introduce a slight difference: pwm_get_relative_duty_cycle uses
round-closest while you replace a round-up. Is this relevant?
> }
>
> static void lpt_set_backlight(const struct drm_connector_state *conn_state,
> u32 level)
> @@ -669,10 +669,9 @@ static void bxt_set_backlight(const struct
> drm_connector_state *conn_state, u32
> static void pwm_set_backlight(const struct drm_connector_state *conn_state,
> u32 level)
> {
> struct intel_panel *panel =
> _intel_connector(conn_state->connector)->panel;
> - int duty_ns = DIV_ROUND_UP(level * panel->backlight.pwm_period_ns, 100);
>
> - pwm_config(panel->backlight.pwm, duty_ns,
> -panel->backlight.pwm_period_ns);
> + pwm_set_relative_duty_cycle(>backlight.pwm_state, level, 100);
> + pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
Similar here: The function used to use round-up but
pwm_set_relative_duty_cycle() used round-closest.
> }
>
> static void
> @@ -841,10 +840,8 @@ static void pwm_disable_backlight(const struct
> drm_connector_state *old_conn_sta
> struct intel_connector *connector =
> to_intel_connector(old_conn_state->connector);
> struct intel_panel *panel = >panel;
>
> - /* Disable the backlight */
> - intel_panel_actually_set_backlight(old_conn_state, 0);
> - usleep_range(2000, 3000);
> - pwm_disable(panel->backlight.pwm);
> + panel->backlight.pwm_state.enabled = false;
> + pwm_apply_state(panel->backlight.pwm, >backlight.pwm_state);
> }
>
> void intel_panel_disable_backlight(const struct drm_connector_state
> *old_conn_state)
> @@ -1176,9 +1173,12 @@ static void pwm_enable_backlight(const struct
> intel_crtc_state *crtc_state,
> {
> struct intel_connector *connector =
> to_intel_connector(conn_state->connector);
> struct intel_panel *panel = >panel;
> + int level = panel->backlight.level;
>
> - pwm_enable(panel->backlight.pwm);
> - intel_panel_actually_set_backlight(conn_state, panel->backlight.level);
> + level =
Re: [PATCH v4 00/15] acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API
Hi Hans, On Thu, Jul 09, 2020 at 04:40:56PM +0200, Hans de Goede wrote: > On 7/9/20 4:14 PM, Sam Ravnborg wrote: > > On Wed, Jul 08, 2020 at 11:14:16PM +0200, Hans de Goede wrote: > > > Here is v4 of my patch series converting the i915 driver's code for > > > controlling the panel's backlight with an external PWM controller to > > > use the atomic PWM API. See below for the changelog. > > > > Why is it that i915 cannot use the pwm_bl driver for backlight? > > I have not studied the code - just wondering. > > The intel_panel.c code deals with 7 different types of PWM controllers > which are built into the GPU + support for external PWM controllers > through the kernel's PWM subsystem. > > pwm_bl will work for the external PWM controller case, but not for > the others. On top of that the intel_panel code integrates which > the video BIOS, getting things like frequency, minimum value > and if the range is inverted (0% duty == backlight brightness max). > I'm not even sure if pwm_bl supports all of this, but even if it > does the intel_panel code handles this in a unified manner for > all supported PWM controllers, including the ones which are > an integral part of the GPU. pwm_bl handles inverted PWM just fine. I'm unsure what "integrates which the video BIOS" means, but I don't see how "handling 7 different types of PWM controllers explicitly and others using the PWM API" can be seen as "unified manner" compared to "provide a pwm driver for whatever might be in the GPU and then use generic code (PWM API, pwm_bl) to drive it". Maybe I'm not understanding some involved complexity here? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0
On Thu, Jul 09, 2020 at 05:47:59PM +0200, Hans de Goede wrote:
> Hi,
>
> On 7/9/20 4:50 PM, Andy Shevchenko wrote:
> > On Wed, Jul 08, 2020 at 11:14:22PM +0200, Hans de Goede wrote:
> > > The datasheet specifies that programming the base_unit part of the
> > > ctrl register to 0 results in a contineous low signal.
> > >
> > > Adjust the get_state method to reflect this by setting pwm_state.period
> > > to 1 and duty_cycle to 0.
> >
> > ...
> >
> > > + if (freq == 0) {
> > > + /* In this case the PWM outputs a continous low signal */
> >
> > > + state->period = 1;
> >
> > I guess this should be something like half of the range (so base unit calc
> > will give 128). Because with period = 1 (too small) it will give too small
> > base unit (if apply) and as a result we get high frequency pulses.
>
> You are right, that if after this the user only changes the duty-cycle
> things will work very poorly, we will end up with a base_unit value of
> e.g 65535 and then have almost no duty-cycle resolution at all.
Is this a problem of the consumer that we don't need to solve? Are there
known consumers running into this problem?
pwm_lpss_prepare() is buggy here, a request for a too low period should be
refused.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König|
Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
