Re: [patch 15/30] pinctrl: nomadik: Use irq_has_action()
On Thu, Dec 10, 2020 at 8:42 PM Thomas Gleixner wrote: > Let the core code do the fiddling with irq_desc. > > Signed-off-by: Thomas Gleixner > Cc: Linus Walleij > Cc: linux-arm-ker...@lists.infradead.org > Cc: linux-g...@vger.kernel.org Reviewed-by: Linus Walleij I suppose you will funnel this directly to Torvalds, else tell me and I'll apply it to my tree. Yours, Linus Walleij ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/4] dma-buf: begin/end_cpu might lock the dma_resv lock
Hi Daniel, I love your patch! Yet something to improve: [auto build test ERROR on next-20201211] [also build test ERROR on v5.10-rc7] [cannot apply to tegra-drm/drm/tegra/for-next linus/master v5.10-rc7 v5.10-rc6 v5.10-rc5] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Daniel-Vetter/dma-buf-Remove-kmap-kerneldoc-vestiges/20201212-40 base:3cc2bd440f2171f093b3a8480a4b54d8c270ed38 config: powerpc64-randconfig-r035-20201210 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 5ff35356f1af2bb92785b38c657463924d9ec386) 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 # https://github.com/0day-ci/linux/commit/24d4fcf0e09c80974556c7639cb630f86a544378 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Daniel-Vetter/dma-buf-Remove-kmap-kerneldoc-vestiges/20201212-40 git checkout 24d4fcf0e09c80974556c7639cb630f86a544378 # 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 errors (new ones prefixed by >>): >> drivers/dma-buf/dma-buf.c:1121:14: error: use of undeclared identifier >> 'dma_buf' might_lock(&dma_buf->resv.lock); ^ >> drivers/dma-buf/dma-buf.c:1121:14: error: use of undeclared identifier >> 'dma_buf' >> drivers/dma-buf/dma-buf.c:1121:14: error: use of undeclared identifier >> 'dma_buf' drivers/dma-buf/dma-buf.c:1156:14: error: use of undeclared identifier 'dma_buf' might_lock(&dma_buf->resv.lock); ^ drivers/dma-buf/dma-buf.c:1156:14: error: use of undeclared identifier 'dma_buf' drivers/dma-buf/dma-buf.c:1156:14: error: use of undeclared identifier 'dma_buf' 6 errors generated. vim +/dma_buf +1121 drivers/dma-buf/dma-buf.c 1093 1094 /** 1095 * dma_buf_begin_cpu_access - Must be called before accessing a dma_buf from the 1096 * cpu in the kernel context. Calls begin_cpu_access to allow exporter-specific 1097 * preparations. Coherency is only guaranteed in the specified range for the 1098 * specified access direction. 1099 * @dmabuf: [in]buffer to prepare cpu access for. 1100 * @direction: [in]length of range for cpu access. 1101 * 1102 * After the cpu access is complete the caller should call 1103 * dma_buf_end_cpu_access(). Only when cpu access is braketed by both calls is 1104 * it guaranteed to be coherent with other DMA access. 1105 * 1106 * This function will also wait for any DMA transactions tracked through 1107 * implicit synchronization in &dma_buf.resv. For DMA transactions with explicit 1108 * synchronization this function will only ensure cache coherency, callers must 1109 * ensure synchronization with such DMA transactions on their own. 1110 * * Can return negative error values, returns 0 on success. 1112 */ 1113 int dma_buf_begin_cpu_access(struct dma_buf *dmabuf, 1114 enum dma_data_direction direction) 1115 { 1116 int ret = 0; 1117 1118 if (WARN_ON(!dmabuf)) 1119 return -EINVAL; 1120 > 1121 might_lock(&dma_buf->resv.lock); 1122 1123 if (dmabuf->ops->begin_cpu_access) 1124 ret = dmabuf->ops->begin_cpu_access(dmabuf, direction); 1125 1126 /* Ensure that all fences are waited upon - but we first allow 1127 * the native handler the chance to do so more efficiently if it 1128 * chooses. A double invocation here will be reasonably cheap no-op. 1129 */ 1130 if (ret == 0) 1131 ret = __dma_buf_begin_cpu_access(dmabuf, direction); 1132 1133 return ret; 1134 } 1135 EXPORT_SYMBOL_GPL(dma_buf_begin_cpu_access); 1136 --- 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/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-11 4:44 p.m., Luben Tuikov wrote: > If however, you never decide to send it to the hardware and simply > abandon it (in hopes that the DRM or the Application Client will > reissue it), then you should send it back to DRM with status "COMPLETE". Correction: "... decide to *not* send it to ..." Regards, Luben ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-10 4:46 a.m., Steven Price wrote: > On 10/12/2020 02:14, Luben Tuikov wrote: >> This patch does not change current behaviour. >> >> The driver's job timeout handler now returns >> status indicating back to the DRM layer whether >> the task (job) was successfully aborted or whether >> more time should be given to the task to complete. > > I find the definitions given a little confusing, see below. > >> Default behaviour as of this patch, is preserved, >> except in obvious-by-comment case in the Panfrost >> driver, as documented below. >> >> All drivers which make use of the >> drm_sched_backend_ops' .timedout_job() callback >> have been accordingly renamed and return the >> would've-been default value of >> DRM_TASK_STATUS_ALIVE to restart the task's >> timeout timer--this is the old behaviour, and >> is preserved by this patch. >> >> In the case of the Panfrost driver, its timedout >> callback correctly first checks if the job had >> completed in due time and if so, it now returns >> DRM_TASK_STATUS_COMPLETE to notify the DRM layer >> that the task can be moved to the done list, to be >> freed later. In the other two subsequent checks, >> the value of DRM_TASK_STATUS_ALIVE is returned, as >> per the default behaviour. >> >> A more involved driver's solutions can be had >> in subequent patches. > > NIT: ^ subsequent Thank you--no idea how my spellcheck and checkpatch.pl missed that. > >> >> v2: Use enum as the status of a driver's job >> timeout callback method. >> >> Cc: Alexander Deucher >> Cc: Andrey Grodzovsky >> Cc: Christian König >> Cc: Daniel Vetter >> Cc: Lucas Stach >> Cc: Russell King >> Cc: Christian Gmeiner >> Cc: Qiang Yu >> Cc: Rob Herring >> Cc: Tomeu Vizoso >> Cc: Steven Price >> Cc: Alyssa Rosenzweig >> Cc: Eric Anholt >> Reported-by: kernel test robot > > This reported-by seems a little odd for this patch. I got this because I wasn't (originally) changing all drivers which were using the task timeout callback. Should I remove it? > >> Signed-off-by: Luben Tuikov >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 +++-- >> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 10 +++- >> drivers/gpu/drm/lima/lima_sched.c | 4 +++- >> drivers/gpu/drm/panfrost/panfrost_job.c | 9 --- >> drivers/gpu/drm/scheduler/sched_main.c | 4 +--- >> drivers/gpu/drm/v3d/v3d_sched.c | 32 + >> include/drm/gpu_scheduler.h | 20 +--- >> 7 files changed, 57 insertions(+), 28 deletions(-) >> > > [] > >> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h >> index 2e0c368e19f6..cedfc5394e52 100644 >> --- a/include/drm/gpu_scheduler.h >> +++ b/include/drm/gpu_scheduler.h >> @@ -206,6 +206,11 @@ static inline bool drm_sched_invalidate_job(struct >> drm_sched_job *s_job, >> return s_job && atomic_inc_return(&s_job->karma) > threshold; >> } >> >> +enum drm_task_status { >> +DRM_TASK_STATUS_COMPLETE, >> +DRM_TASK_STATUS_ALIVE >> +}; >> + >> /** >>* struct drm_sched_backend_ops >>* >> @@ -230,10 +235,19 @@ struct drm_sched_backend_ops { >> struct dma_fence *(*run_job)(struct drm_sched_job *sched_job); >> >> /** >> - * @timedout_job: Called when a job has taken too long to execute, >> - * to trigger GPU recovery. >> + * @timedout_job: Called when a job has taken too long to execute, >> + * to trigger GPU recovery. >> + * >> + * Return DRM_TASK_STATUS_ALIVE, if the task (job) is healthy >> + * and executing in the hardware, i.e. it needs more time. > > So 'alive' means the job (was) alive, and GPU recovery is happening. > I.e. it's the job just takes too long. Panfrost will trigger a GPU reset > (killing the job) in this case while returning DRM_TASK_STATUS_ALIVE. "ALIVE" means "at this moment the task is in the hardware executing, using memories, DMA engines, compute units, the whole thing." You, can also call it active, executing, busy, etc. "ALIVE" makes no assumptions about how the task got there. Maybe this was original submission, and the task is taking its sweet time. Maybe the driver aborted it and reissued it, all in the timer timeout callback, etc. It merely tells the DRM layer that the task is actively executing in the hardware, so no assumptions can be made about freeing memories, decrementing krefs, etc. IOW, it's executing, check back later. > >> + * >> + * Return DRM_TASK_STATUS_COMPLETE, if the task (job) has >> + * been aborted or is unknown to the hardware, i.e. if >> + * the task is out of the hardware, and maybe it is now >> + * in the done list, or it was completed long ago, or >> + * if it is unknown to the hardware. > > Where 'complete' seems to mean a variety of things: > > * The job completed successfully (i.e. the timeout raced), this is the > situation that Panfrost detects. In this case (and only this case) the > GPU reset will *n
Re: [PATCH v3 4/5] dt-bindings: phy: mixel: mipi-dsi-phy: Add Mixel combo PHY support for i.MX8qxp
On Fri, 11 Dec 2020 09:46:21 +0800, Liu Ying wrote: > Add support for Mixel MIPI DPHY + LVDS PHY combo IP > as found on Freescale i.MX8qxp SoC. > > Cc: Guido Günther > Cc: Kishon Vijay Abraham I > Cc: Vinod Koul > Cc: Rob Herring > Cc: NXP Linux Team > Signed-off-by: Liu Ying > --- > v2->v3: > * No change. > > v1->v2: > * Add the binding for i.MX8qxp Mixel combo PHY based on the converted binding. > (Guido) > > .../bindings/phy/mixel,mipi-dsi-phy.yaml | 41 > -- > 1 file changed, 38 insertions(+), 3 deletions(-) > Reviewed-by: Rob Herring ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/5] dt-bindings: phy: Convert mixel, mipi-dsi-phy to json-schema
On Fri, 11 Dec 2020 09:46:20 +0800, Liu Ying wrote: > This patch converts the mixel,mipi-dsi-phy binding to > DT schema format using json-schema. > > Comparing to the plain text version, the new binding adds > the 'assigned-clocks', 'assigned-clock-parents' and > 'assigned-clock-rates' properites, otherwise 'make dtbs_check' > would complain that there are mis-matches. Also, the new > binding requires the 'power-domains' property since all potential > SoCs that embed this PHY would provide a power domain for it. > The example of the new binding takes reference to the latest > dphy node in imx8mq.dtsi. > > Cc: Guido Günther > Cc: Kishon Vijay Abraham I > Cc: Vinod Koul > Cc: Rob Herring > Cc: NXP Linux Team > Signed-off-by: Liu Ying > --- > v2->v3: > * Improve the 'clock-names' property by dropping 'items:'. > > v1->v2: > * Newly introduced in v2. (Guido) > > .../devicetree/bindings/phy/mixel,mipi-dsi-phy.txt | 29 - > .../bindings/phy/mixel,mipi-dsi-phy.yaml | 72 > ++ > 2 files changed, 72 insertions(+), 29 deletions(-) > delete mode 100644 > Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.txt > create mode 100644 > Documentation/devicetree/bindings/phy/mixel,mipi-dsi-phy.yaml > Reviewed-by: Rob Herring ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-10 4:41 a.m., Christian König wrote: > Am 10.12.20 um 10:31 schrieb Lucas Stach: >> Hi Luben, >> >> Am Mittwoch, den 09.12.2020, 21:14 -0500 schrieb Luben Tuikov: >>> [SNIP] >>> -static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job) >>> +static enum drm_task_status etnaviv_sched_timedout_job(struct drm_sched_job >>> + *sched_job) >>> { >>> struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job); >>> struct etnaviv_gpu *gpu = submit->gpu; >>> @@ -120,9 +121,16 @@ static void etnaviv_sched_timedout_job(struct >>> drm_sched_job *sched_job) >>> >>> drm_sched_resubmit_jobs(&gpu->sched); >>> >>> + /* Tell the DRM scheduler that this task needs >>> +* more time. >>> +*/ >> This comment doesn't match the kernel coding style, but it's also moot >> as the whole added code block isn't needed. The code just below is >> identical, so letting execution continue here instead of returning >> would be the right thing to do, but maybe you mean to return >> DRM_TASK_STATUS_COMPLETE? It's a bit confusing that aborted and job >> successfully finished should be signaled with the same return code. > > Yes and no. As I tried to describe in my previous mail the naming of the > enum values is also not very good. I tried to make the naming as minimal as possible: COMPLETE: the task is out of the hardware. ALIVE: the task is in the hardware. > > See even when the job has completed we need to restart the timer for the > potential next job. Sure, yes. But this is something which the DRM decides--why should drivers know of the internals of the DRM? (i.e. that it restarts the timer or that there is a timer, etc.) Return minimal value and let the DRM decide what to do next. > > Only when the device is completely gone and unrecoverable we should not > restart the timer. Yes, agreed. > > I suggest to either make this an int and return -ENODEV when that > happens or rename the enum to something like DRM_SCHED_NODEV. It was an int, but you suggested that it'd be a macro, so I made it an enum so that the complier can check the values against the macros returned. I suggested, DRM_TASK_SCHED_ENODEV, but let Andrey add it when he adds his patches on top of my patch here, because his work adds hotplug/unplug support. Also, note that if the pending list is freed, while the DRM had been blocked, i.e. notified that the device is gone, then returning DRM_TASK_SCHED_ENODEV would be moot, as there are no tasks in the pending list. This patch here is good as it is, since it is minimal and doesn't make assumptions on DRM behaviour. Regards, Luben > > Regards, > Christian. > >> >>> + drm_sched_start(&gpu->sched, true); >>> + return DRM_TASK_STATUS_ALIVE; >>> + >>> out_no_timeout: >>> /* restart scheduler after GPU is usable again */ >>> drm_sched_start(&gpu->sched, true); >>> + return DRM_TASK_STATUS_ALIVE; >>> } >> Regards, >> Lucas >> > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 27/30] xen/events: Only force affinity mask for percpu interrupts
On 12/11/20 7:37 AM, Thomas Gleixner wrote: > On Fri, Dec 11 2020 at 13:10, Jürgen Groß wrote: >> On 11.12.20 00:20, boris.ostrov...@oracle.com wrote: >>> On 12/10/20 2:26 PM, Thomas Gleixner wrote: All event channel setups bind the interrupt on CPU0 or the target CPU for percpu interrupts and overwrite the affinity mask with the corresponding cpumask. That does not make sense. The XEN implementation of irqchip::irq_set_affinity() already picks a single target CPU out of the affinity mask and the actual target is stored in the effective CPU mask, so destroying the user chosen affinity mask which might contain more than one CPU is wrong. Change the implementation so that the channel is bound to CPU0 at the XEN level and leave the affinity mask alone. At startup of the interrupt affinity will be assigned out of the affinity mask and the XEN binding will be updated. >>> >>> If that's the case then I wonder whether we need this call at all and >>> instead bind at startup time. >> After some discussion with Thomas on IRC and xen-devel archaeology the >> result is: this will be needed especially for systems running on a >> single vcpu (e.g. small guests), as the .irq_set_affinity() callback >> won't be called in this case when starting the irq. On UP are we not then going to end up with an empty affinity mask? Or are we guaranteed to have it set to 1 by interrupt generic code? This is actually why I brought this up in the first place --- a potential mismatch between the affinity mask and Xen-specific data (e.g. info->cpu and then protocol-specific data in event channel code). Even if they are re-synchronized later, at startup time (for SMP). I don't see anything that would cause a problem right now but I worry that this inconsistency may come up at some point. -boris > That's right, but not limited to ARM. The same problem exists on x86 UP. > So yes, the call makes sense, but the changelog is not really useful. > Let me add a comment to this. > > Thanks, > > tglx > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)
On 2020-12-10 4:31 a.m., Lucas Stach wrote: > Hi Luben, > > Am Mittwoch, den 09.12.2020, 21:14 -0500 schrieb Luben Tuikov: >> This patch does not change current behaviour. >> >> The driver's job timeout handler now returns >> status indicating back to the DRM layer whether >> the task (job) was successfully aborted or whether >> more time should be given to the task to complete. >> >> Default behaviour as of this patch, is preserved, >> except in obvious-by-comment case in the Panfrost >> driver, as documented below. >> >> All drivers which make use of the >> drm_sched_backend_ops' .timedout_job() callback >> have been accordingly renamed and return the >> would've-been default value of >> DRM_TASK_STATUS_ALIVE to restart the task's >> timeout timer--this is the old behaviour, and >> is preserved by this patch. >> >> In the case of the Panfrost driver, its timedout >> callback correctly first checks if the job had >> completed in due time and if so, it now returns >> DRM_TASK_STATUS_COMPLETE to notify the DRM layer >> that the task can be moved to the done list, to be >> freed later. In the other two subsequent checks, >> the value of DRM_TASK_STATUS_ALIVE is returned, as >> per the default behaviour. >> >> A more involved driver's solutions can be had >> in subequent patches. >> >> v2: Use enum as the status of a driver's job >> timeout callback method. >> >> Cc: Alexander Deucher >> Cc: Andrey Grodzovsky >> Cc: Christian König >> Cc: Daniel Vetter >> Cc: Lucas Stach >> Cc: Russell King >> Cc: Christian Gmeiner >> Cc: Qiang Yu >> Cc: Rob Herring >> Cc: Tomeu Vizoso >> Cc: Steven Price >> Cc: Alyssa Rosenzweig >> Cc: Eric Anholt >> Reported-by: kernel test robot >> Signed-off-by: Luben Tuikov >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 6 +++-- >> drivers/gpu/drm/etnaviv/etnaviv_sched.c | 10 +++- >> drivers/gpu/drm/lima/lima_sched.c | 4 +++- >> drivers/gpu/drm/panfrost/panfrost_job.c | 9 --- >> drivers/gpu/drm/scheduler/sched_main.c | 4 +--- >> drivers/gpu/drm/v3d/v3d_sched.c | 32 + >> include/drm/gpu_scheduler.h | 20 +--- >> 7 files changed, 57 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> index ff48101bab55..a111326cbdde 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c >> @@ -28,7 +28,7 @@ >> #include "amdgpu.h" >> #include "amdgpu_trace.h" >> >> >> >> >> -static void amdgpu_job_timedout(struct drm_sched_job *s_job) >> +static enum drm_task_status amdgpu_job_timedout(struct drm_sched_job *s_job) >> { >> struct amdgpu_ring *ring = to_amdgpu_ring(s_job->sched); >> struct amdgpu_job *job = to_amdgpu_job(s_job); >> @@ -41,7 +41,7 @@ static void amdgpu_job_timedout(struct drm_sched_job >> *s_job) >> amdgpu_ring_soft_recovery(ring, job->vmid, s_job->s_fence->parent)) >> { >> DRM_ERROR("ring %s timeout, but soft recovered\n", >> s_job->sched->name); >> -return; >> +return DRM_TASK_STATUS_ALIVE; >> } >> >> >> >> >> amdgpu_vm_get_task_info(ring->adev, job->pasid, &ti); >> @@ -53,10 +53,12 @@ static void amdgpu_job_timedout(struct drm_sched_job >> *s_job) >> >> >> >> >> if (amdgpu_device_should_recover_gpu(ring->adev)) { >> amdgpu_device_gpu_recover(ring->adev, job); >> +return DRM_TASK_STATUS_ALIVE; >> } else { >> drm_sched_suspend_timeout(&ring->sched); >> if (amdgpu_sriov_vf(adev)) >> adev->virt.tdr_debug = true; >> +return DRM_TASK_STATUS_ALIVE; >> } >> } >> >> >> >> >> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c >> b/drivers/gpu/drm/etnaviv/etnaviv_sched.c >> index cd46c882269c..c49516942328 100644 >> --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c >> +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c >> @@ -82,7 +82,8 @@ static struct dma_fence *etnaviv_sched_run_job(struct >> drm_sched_job *sched_job) >> return fence; >> } >> >> >> >> >> -static void etnaviv_sched_timedout_job(struct drm_sched_job *sched_job) >> +static enum drm_task_status etnaviv_sched_timedout_job(struct drm_sched_job >> + *sched_job) >> { >> struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job); >> struct etnaviv_gpu *gpu = submit->gpu; >> @@ -120,9 +121,16 @@ static void etnaviv_sched_timedout_job(struct >> drm_sched_job *sched_job) >> >> drm_sched_resubmit_jobs(&gpu->sched); >> >> +/* Tell the DRM scheduler that this task needs >> + * more time. >> + */ > > This comment doesn't match the kernel coding style, but it's also moot > as the whole added code block isn't needed. The code just below is > identical, so letting execution continue here instead of returning > would be the
Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
On Thu, Dec 10, 2020 at 5:10 AM Daniel Vetter wrote: > On Thu, Dec 10, 2020 at 1:06 PM Greg KH wrote: > > On Thu, Dec 10, 2020 at 12:26:01PM +0100, Daniel Vetter wrote: > > > On Thu, Dec 10, 2020 at 11:55 AM Greg KH > > > wrote: > > > > On Thu, Dec 10, 2020 at 11:27:27AM +0100, Daniel Vetter wrote: > > > > > This only shows shared memory, so it does smell a lot like > > > > > $specific_issue > > > > > and we're designing a narrow solution for that and then have to carry > > > > > it > > > > > forever. > > > > > > > > I think the "issue" is that this was a feature from ion that people > > > > "missed" in the dmabuf move. Taking away the ability to see what kind > > > > of allocations were being made didn't make a lot of debugging tools > > > > happy :( > > > > > > If this is just for dma-heaps then why don't we add the stuff back > > > over there? It reinforces more that the android gpu stack and the > > > non-android gpu stack on linux are fairly different in fundamental > > > ways, but that's not really new. > > > > Back "over where"? > > > > dma-bufs are not only used for the graphics stack on android from what I > > can tell, so this shouldn't be a gpu-specific issue. > > dma-buf heaps exist because android, mostly because google mandates > it. So, I don't think that's fair. dma-buf heaps and ION before exist because it solves a problem they have for allocating shared buffers for multiple complicated multi-device pipelines where the various devices have constraints. It's not strictly required[1], as your next point makes clear (along with ChromeOS's Android not using it). > There's not a whole lot (meaning zero) of actually open gpu stacks > around that run on android and use dma-buf heaps like approved google > systems, largely because the gralloc implementation in mesa just > doesnt. So yes, db845c currently uses the gbm_gralloc, which doesn't use dmabuf heaps or ION. That said, the resulting system still uses quite a number of dmabufs, as Hridya's patch shows: ==> /sys/kernel/dmabuf/28435/exporter_name <== drm ==> /sys/kernel/dmabuf/28435/dev_map_info <== ==> /sys/kernel/dmabuf/28435/size <== 16384 ==> /sys/kernel/dmabuf/28161/exporter_name <== drm ==> /sys/kernel/dmabuf/28161/dev_map_info <== ==> /sys/kernel/dmabuf/28161/size <== 524288 ==> /sys/kernel/dmabuf/30924/exporter_name <== drm ==> /sys/kernel/dmabuf/30924/dev_map_info <== ==> /sys/kernel/dmabuf/30924/size <== 8192 ==> /sys/kernel/dmabuf/26880/exporter_name <== drm ==> /sys/kernel/dmabuf/26880/dev_map_info <== ==> /sys/kernel/dmabuf/26880/size <== 262144 ... So even when devices are not using dma-buf heaps (which I get, you have an axe to grind with :), having some way to collect useful stats for dmabufs in use can be valuable. (Also one might note, the db845c also doesn't have many constrained devices, and we've not yet enabled hw codec support or camera pipelines, so it avoids much of the complexity that ION/dma-buf heaps was created to solve) > So if android needs some quick debug output in sysfs, we can just add > that in dma-buf heaps, for android only, problem solved. And much less > annoying review to make sure it actually fits into the wider ecosystem > because as-is (and I'm not seeing that chance anytime soon), dma-buf > heaps is for android only. dma-buf at large isn't, so merging a debug > output sysfs api that's just for android but misses a ton of the more > generic features and semantics of dma-buf is not great. The intent behind this patch is *not* to create more Android-specific logic, but to provide useful information generically. Indeed, Android does use dmabufs heavily for passing buffers around, and your point that not all systems handle graphics buffers that way is valid, and it's important we don't bake any Android-isms into the interface. But being able to collect data about the active dmabufs in a system is useful, regardless of how regardless of how the dma-buf was allocated. So I'd much rather see your feedback on how we expose other aspects of dmabufs (dma_resv, exporter devices, attachment links) integrated, rather then trying to ghettoize it as android-only and limit it to the dmabuf heaps, where I don't think it makes as much sense to add. thanks -john [1] Out of the box, the codec2 code added a few years back does directly call to ION (and now dmabuf heaps) for system buffers, but it can be configured differently as it's used in ChromeOS's Android too. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 0/9] Xilinx AI engine kernel driver
Hi all On Fri, Dec 11, 2020 at 8:03 PM Alex Deucher wrote: > > On Mon, Nov 30, 2020 at 3:25 AM Wendy Liang wrote: > > > > AI engine is the acceleration engine provided by Xilinx. These engines > > provide high compute density for vector-based algorithms, and flexible > > custom compute and data movement. It has core tiles for compute and > > shim tiles to interface the FPGA fabric. > > > > You can check the AI engine architecture document for more hardware details: > > https://www.xilinx.com/support/documentation/architecture-manuals/am009-versal-ai-engine.pdf > > > > This patch series adds a Linux kernel driver to manage the Xilinx AI > > engine array device and AI engine partitions (groups of AI engine tiles > > dedicated to an application). > > Hi Wendy, > > I think it would be good to provide an overview of how your stack > works in general. That would give reviewers a better handle on how > all of this fits together. I'd suggest including an overview in the > cover letter and also in the commit message and/or as a comment in the > code in one of the patches. I'm not really an expert when it comes to > FPGAs, but this basically looks like a pretty low level interface to > set up the data fabric for a kernel that will run on the soft logic or > maybe the microcontroller on the board. It doesn't have to be super > detailed, just a nice flow for how you might use this. E.g., > > Userspace uses ioctls X, Y, Z to configure the data fabric for the > FPGA kernel. The kernels can run on... . DMA access to system memory > for data sets can be allocated using ioctl A. DMA access is limited > by... . The user can then load the FPGA kernel on to one of the > engines using ioctl B and finally they can kick off the whole thing > using ioctl C. FPGA kernels are compiled using YYY toolchain and use > use the following runtime (link to runtime) to configure the data > fabric using ioctls X, Y, Z. At least for drm drivers we ideally have that as a .rst file in Documentation/. With that you can even do full svg graphs, or just dot graphs, of the overall stack if you really want to go overboard :-) > It would also be good to go over the security implications of the > design. E.g., can the FPGA kernel(s) access the DMA engine directly, > or is it limited to just the DMA regions set up by the ioctls? Also, > does the hardware and software design allow for multiple users? If > so, how does that work? I've also seen indications that there's some on-chip or on-card memory. How that's planned to be used and whether we want to manage this (maybe even with something like ttm) would be good to understand. All excellent questions from Alex, just figured I add some more. Cheers, Daniel > Thanks, > > Alex > > > > > > v3: > > * unlock AIE dev mutex after failed to gain the partition lock in > > errors handing > > * replace pointer with __u64 and enum with __u32 in ioctl > > > > v2: > > * Fix dtschema check errors > > * Fix test bot warning on interrupt implementation. Removed set but > > unused varaible. > > * Fix compilation unused function warning of firmware change in case > > ZynqMP firmware is not configured > > * There are other warning on ZynqMP firmware reported from testbot > > which is not introduced by this patch set. > > "[PATCH] firmware: xlnx-zynqmp: fix compilation warning" is submitted > > for those fixes. > > > > > > Izhar Ameer Shaikh (1): > > firmware: xilinx: Add IOCTL support for AIE ISR Clear > > > > Nishad Saraf (2): > > misc: xilinx-ai-engine: Add support to request device management > > services > > misc: xilinx-ai-engine: Add support for servicing error interrupts > > > > Wendy Liang (6): > > dt-binding: soc: xilinx: ai-engine: Add AI engine binding > > misc: Add Xilinx AI engine device driver > > misc: xilinx-ai-engine: Implement AI engine cleanup sequence > > misc: xilinx-ai-engine: expose AI engine tile memories to userspace > > misc: xilinx-ai-engine: add setting shim dma bd operation > > misc: xilinx-ai-engine: add request and release tiles > > > > .../bindings/soc/xilinx/xlnx,ai-engine.yaml| 126 > > MAINTAINERS| 8 + > > drivers/firmware/xilinx/zynqmp.c | 14 + > > drivers/misc/Kconfig | 12 + > > drivers/misc/Makefile | 1 + > > drivers/misc/xilinx-ai-engine/Makefile | 16 + > > drivers/misc/xilinx-ai-engine/ai-engine-aie.c | 608 > > +++ > > drivers/misc/xilinx-ai-engine/ai-engine-clock.c| 245 > > drivers/misc/xilinx-ai-engine/ai-engine-dev.c | 496 > > drivers/misc/xilinx-ai-engine/ai-engine-dma.c | 481 +++ > > drivers/misc/xilinx-ai-engine/ai-engine-internal.h | 519 > > .../misc/xilinx-ai-engine/ai-engine-interrupt.c| 659 > > + > > drivers/misc/xilinx-ai-engine/ai-engine-mem.c
Re: [PATCH v3 0/9] Xilinx AI engine kernel driver
On Mon, Nov 30, 2020 at 3:25 AM Wendy Liang wrote: > > AI engine is the acceleration engine provided by Xilinx. These engines > provide high compute density for vector-based algorithms, and flexible > custom compute and data movement. It has core tiles for compute and > shim tiles to interface the FPGA fabric. > > You can check the AI engine architecture document for more hardware details: > https://www.xilinx.com/support/documentation/architecture-manuals/am009-versal-ai-engine.pdf > > This patch series adds a Linux kernel driver to manage the Xilinx AI > engine array device and AI engine partitions (groups of AI engine tiles > dedicated to an application). Hi Wendy, I think it would be good to provide an overview of how your stack works in general. That would give reviewers a better handle on how all of this fits together. I'd suggest including an overview in the cover letter and also in the commit message and/or as a comment in the code in one of the patches. I'm not really an expert when it comes to FPGAs, but this basically looks like a pretty low level interface to set up the data fabric for a kernel that will run on the soft logic or maybe the microcontroller on the board. It doesn't have to be super detailed, just a nice flow for how you might use this. E.g., Userspace uses ioctls X, Y, Z to configure the data fabric for the FPGA kernel. The kernels can run on... . DMA access to system memory for data sets can be allocated using ioctl A. DMA access is limited by... . The user can then load the FPGA kernel on to one of the engines using ioctl B and finally they can kick off the whole thing using ioctl C. FPGA kernels are compiled using YYY toolchain and use use the following runtime (link to runtime) to configure the data fabric using ioctls X, Y, Z. It would also be good to go over the security implications of the design. E.g., can the FPGA kernel(s) access the DMA engine directly, or is it limited to just the DMA regions set up by the ioctls? Also, does the hardware and software design allow for multiple users? If so, how does that work? Thanks, Alex > > v3: > * unlock AIE dev mutex after failed to gain the partition lock in > errors handing > * replace pointer with __u64 and enum with __u32 in ioctl > > v2: > * Fix dtschema check errors > * Fix test bot warning on interrupt implementation. Removed set but > unused varaible. > * Fix compilation unused function warning of firmware change in case > ZynqMP firmware is not configured > * There are other warning on ZynqMP firmware reported from testbot > which is not introduced by this patch set. > "[PATCH] firmware: xlnx-zynqmp: fix compilation warning" is submitted > for those fixes. > > > Izhar Ameer Shaikh (1): > firmware: xilinx: Add IOCTL support for AIE ISR Clear > > Nishad Saraf (2): > misc: xilinx-ai-engine: Add support to request device management > services > misc: xilinx-ai-engine: Add support for servicing error interrupts > > Wendy Liang (6): > dt-binding: soc: xilinx: ai-engine: Add AI engine binding > misc: Add Xilinx AI engine device driver > misc: xilinx-ai-engine: Implement AI engine cleanup sequence > misc: xilinx-ai-engine: expose AI engine tile memories to userspace > misc: xilinx-ai-engine: add setting shim dma bd operation > misc: xilinx-ai-engine: add request and release tiles > > .../bindings/soc/xilinx/xlnx,ai-engine.yaml| 126 > MAINTAINERS| 8 + > drivers/firmware/xilinx/zynqmp.c | 14 + > drivers/misc/Kconfig | 12 + > drivers/misc/Makefile | 1 + > drivers/misc/xilinx-ai-engine/Makefile | 16 + > drivers/misc/xilinx-ai-engine/ai-engine-aie.c | 608 +++ > drivers/misc/xilinx-ai-engine/ai-engine-clock.c| 245 > drivers/misc/xilinx-ai-engine/ai-engine-dev.c | 496 > drivers/misc/xilinx-ai-engine/ai-engine-dma.c | 481 +++ > drivers/misc/xilinx-ai-engine/ai-engine-internal.h | 519 > .../misc/xilinx-ai-engine/ai-engine-interrupt.c| 659 > + > drivers/misc/xilinx-ai-engine/ai-engine-mem.c | 275 + > drivers/misc/xilinx-ai-engine/ai-engine-part.c | 635 > drivers/misc/xilinx-ai-engine/ai-engine-res.c | 219 +++ > drivers/misc/xilinx-ai-engine/ai-engine-reset.c| 159 + > include/linux/firmware/xlnx-zynqmp.h | 8 + > include/uapi/linux/xlnx-ai-engine.h| 238 > 18 files changed, 4719 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/soc/xilinx/xlnx,ai-engine.yaml > create mode 100644 drivers/misc/xilinx-ai-engine/Makefile > create mode 100644 drivers/misc/xilinx-ai-engine/ai-engine-aie.c > create mode 100644 drivers/misc/xilinx-ai-engine/ai-engine-clock.c > create mode 100644 drivers/misc/xilinx
[PATCH v4 3/4] drm: require a non_NULL drm_crtc.primary
If a CRTC is missing a legacy primary plane pointer, a lot of things will be broken for user-space: fbdev stops working and the entire legacy uAPI stops working. Require all drivers to populate drm_crtc.primary to prevent these issues. Warn if it's NULL. Signed-off-by: Simon Ser Reviewed-by: Daniel Vetter Acked-by: Pekka Paalanen --- drivers/gpu/drm/drm_mode_config.c | 3 +++ drivers/gpu/drm/drm_plane.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 1628c8b60d9a..1e5da83fd2a8 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -639,6 +639,9 @@ void drm_mode_config_validate(struct drm_device *dev) } drm_for_each_crtc(crtc, dev) { + WARN(!crtc->primary, "Missing primary plane on [CRTC:%d:%s]\n", +crtc->base.id, crtc->name); + if (crtc->primary) { WARN(!(crtc->primary->possible_crtcs & drm_crtc_mask(crtc)), "Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 5d33ca9f0032..49b0a8b9ac02 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -57,7 +57,7 @@ * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core * relies on the driver to set the primary and optionally the cursor plane used * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All - * drivers should provide one primary plane per CRTC to avoid surprising legacy + * drivers must provide one primary plane per CRTC to avoid surprising legacy * userspace too much. */ -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 2/4] drm: validate possible_crtcs for primary and cursor planes
If a primary or cursor plane is not compatible with a CRTC it's attached to via the legacy primary/cursor field, things will be broken for legacy user-space. v4: use drm_crtc_mask instead of BIT (Ville) Signed-off-by: Simon Ser Reviewed-by: Daniel Vetter Acked-by: Pekka Paalanen Cc: Ville Syrjala --- drivers/gpu/drm/drm_mode_config.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..1628c8b60d9a 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -625,6 +625,7 @@ static void validate_encoder_possible_crtcs(struct drm_encoder *encoder) void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder; + struct drm_crtc *crtc; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return; @@ -636,4 +637,19 @@ void drm_mode_config_validate(struct drm_device *dev) validate_encoder_possible_clones(encoder); validate_encoder_possible_crtcs(encoder); } + + drm_for_each_crtc(crtc, dev) { + if (crtc->primary) { + WARN(!(crtc->primary->possible_crtcs & drm_crtc_mask(crtc)), +"Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", +crtc->primary->base.id, crtc->primary->name, +crtc->base.id, crtc->name); + } + if (crtc->cursor) { + WARN(!(crtc->cursor->possible_crtcs & drm_crtc_mask(crtc)), +"Bogus cursor plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", +crtc->cursor->base.id, crtc->cursor->name, +crtc->base.id, crtc->name); + } + } } -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/4] drm: rework description of primary and cursor planes
The previous wording could be understood by user-space evelopers as "a primary/cursor plane is only compatible with a single CRTC" [1]. Reword the planes description to make it clear the DRM-internal drm_crtc.primary and drm_crtc.cursor planes are for legacy uAPI. [1]: https://github.com/swaywm/wlroots/pull/2333#discussion_r456788057 Signed-off-by: Simon Ser Reviewed-by: Daniel Vetter Acked-by: Pekka Paalanen --- drivers/gpu/drm/drm_crtc.c | 3 +++ drivers/gpu/drm/drm_plane.c | 16 +--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 8d19d258547f..a6336c7154d6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -256,6 +256,9 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc) * planes). For really simple hardware which has only 1 plane look at * drm_simple_display_pipe_init() instead. * + * The @primary and @cursor planes are only relevant for legacy uAPI, see + * &drm_crtc.primary and &drm_crtc.cursor. + * * Returns: * Zero on success, error code on failure. */ diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 385801dd21f9..5d33ca9f0032 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -49,14 +49,16 @@ * &struct drm_plane (possibly as part of a larger structure) and registers it * with a call to drm_universal_plane_init(). * - * Cursor and overlay planes are optional. All drivers should provide one - * primary plane per CRTC to avoid surprising userspace too much. See enum - * drm_plane_type for a more in-depth discussion of these special uapi-relevant - * plane types. Special planes are associated with their CRTC by calling - * drm_crtc_init_with_planes(). - * * The type of a plane is exposed in the immutable "type" enumeration property, - * which has one of the following values: "Overlay", "Primary", "Cursor". + * which has one of the following values: "Overlay", "Primary", "Cursor" (see + * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see + * &drm_plane.possible_crtcs. + * + * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core + * relies on the driver to set the primary and optionally the cursor plane used + * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All + * drivers should provide one primary plane per CRTC to avoid surprising legacy + * userspace too much. */ static unsigned int drm_num_planes(struct drm_device *dev) -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 4/4] drm: require each CRTC to have a unique primary plane
User-space expects to be able to pick a primary plane for each CRTC exposed by the driver. Make sure this assumption holds in drm_mode_config_validate. Use the legacy drm_crtc.primary field to check this, because it's simpler and we require drivers to set it anyways. Accumulate a set of primary planes which are already used for a CRTC in a bitmask. Error out if a primary plane is re-used. v2: new patch v3: - Use u64 instead of __u64 (Jani) - Use `unsigned int` instead of `unsigned` (Jani) v4: - Use u32 instead of u64 for plane mask (Ville) - Use drm_plane_mask instead of BIT (Ville) - Fix typos (Ville) Signed-off-by: Simon Ser Reviewed-by: Daniel Vetter Cc: Pekka Paalanen Cc: Jani Nikula Cc: Ville Syrjala --- drivers/gpu/drm/drm_mode_config.c | 21 + drivers/gpu/drm/drm_plane.c | 6 ++ 2 files changed, 27 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 1e5da83fd2a8..7ffc0d6a8e2c 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -626,6 +626,9 @@ void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder; struct drm_crtc *crtc; + struct drm_plane *plane; + u32 primary_with_crtc = 0, cursor_with_crtc = 0; + unsigned int num_primary = 0; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return; @@ -647,12 +650,30 @@ void drm_mode_config_validate(struct drm_device *dev) "Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", crtc->primary->base.id, crtc->primary->name, crtc->base.id, crtc->name); + WARN(primary_with_crtc & drm_plane_mask(crtc->primary), +"Primary plane [PLANE:%d:%s] used for multiple CRTCs", +crtc->primary->base.id, crtc->primary->name); + primary_with_crtc |= drm_plane_mask(crtc->primary); } if (crtc->cursor) { WARN(!(crtc->cursor->possible_crtcs & drm_crtc_mask(crtc)), "Bogus cursor plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", crtc->cursor->base.id, crtc->cursor->name, crtc->base.id, crtc->name); + WARN(cursor_with_crtc & drm_plane_mask(crtc->cursor), +"Cursor plane [PLANE:%d:%s] used for multiple CRTCs", +crtc->cursor->base.id, crtc->cursor->name); + cursor_with_crtc |= drm_plane_mask(crtc->cursor); } } + + drm_for_each_plane(plane, dev) { + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { + num_primary++; + } + } + + WARN(num_primary != dev->mode_config.num_crtc, +"Must have as many primary planes as there are CRTCs, but have %u primary planes and %u CRTCs", +num_primary, dev->mode_config.num_crtc); } diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 49b0a8b9ac02..a1f4510efa83 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -54,6 +54,12 @@ * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see * &drm_plane.possible_crtcs. * + * Each CRTC must have a unique primary plane userspace can attach to enable + * the CRTC. In other words, userspace must be able to attach a different + * primary plane to each CRTC at the same time. Primary planes can still be + * compatible with multiple CRTCs. There must be exactly as many primary planes + * as there are CRTCs. + * * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core * relies on the driver to set the primary and optionally the cursor plane used * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/ast: Fixed CVE for DP501
Hi KuoHsiang, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on drm-exynos/exynos-drm-next] [also build test WARNING on drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc7 next-20201211] [cannot apply to drm/drm-next] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352 base: https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next config: x86_64-randconfig-s022-20201210 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-179-ga00755aa-dirty # https://github.com/0day-ci/linux/commit/75af180bfa7bc2227224653381d743b9396b41c2 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352 git checkout 75af180bfa7bc2227224653381d743b9396b41c2 # 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/ast/ast_dp501.c:357:39: sparse: sparse: incorrect type in >> argument 2 (different address spaces) @@ expected void volatile >> [noderef] __iomem *addr @@ got unsigned int [usertype] * @@ drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse: expected void volatile [noderef] __iomem *addr drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse: got unsigned int [usertype] * drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@ expected void volatile [noderef] __iomem *addr @@ got unsigned int [usertype] * @@ drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse: expected void volatile [noderef] __iomem *addr drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse: got unsigned int [usertype] * vim +357 drivers/gpu/drm/ast/ast_dp501.c 332 333 bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata) 334 { 335 struct ast_private *ast = to_ast_private(dev); 336 u32 i, boot_address, offset, data; 337 338 if (ast->config_mode == ast_use_p2a) { 339 boot_address = get_fw_base(ast); 340 341 /* validate FW version */ 342 offset = AST_DP501_GBL_VERSION; 343 data = ast_mindwm(ast, boot_address + offset); 344 if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) 345 return false; 346 347 /* validate PnP Monitor */ 348 offset = AST_DP501_PNPMONITOR; 349 data = ast_mindwm(ast, boot_address + offset); 350 if (!(data & AST_DP501_PNP_CONNECTED)) 351 return false; 352 353 /* Read EDID */ 354 offset = AST_DP501_EDID_DATA; 355 for (i = 0; i < 128; i += 4) { 356 data = ast_mindwm(ast, boot_address + offset + i); > 357 writel(data, (u32 *)(ediddata + i)); 358 } 359 } else { 360 if (!ast->dp501_fw_buf) 361 return false; 362 363 /* dummy read */ 364 offset = 0x; 365 data = readl(ast->dp501_fw_buf + offset); 366 367 /* validate FW version */ 368 offset = AST_DP501_GBL_VERSION; 369 data = readl(ast->dp501_fw_buf + offset); 370 if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) 371 return false; 372 373 /* validate PnP Monitor */ 374 offset = AST_DP501_PNPMONITOR; 375 data = readl(ast->dp501_fw_buf + offset); 376 if (!(data & AST_DP501_PNP_CONNECTED)) 377 return false; 378 379 /* Read EDID */ 380 offset = AST_DP501_EDID_DATA; 381 for (i = 0; i < 128; i += 4) { 382 data = readl(ast->dp501_fw_buf + offset + i); 383 writel(data, (u32 *)(ediddata + i)); 384 } 385 } 386 387 return true; 388 }
Re: [git pull] drm fixes for 5.10 final
The pull request you sent on Fri, 11 Dec 2020 11:03:01 +1000: > git://anongit.freedesktop.org/drm/drm tags/drm-fixes-2020-12-11 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/059fe8296e0fb4b89d997ea0aa75996911b8f3aa Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 16/30] mfd: ab8500-debugfs: Remove the racy fiddling with irq_desc
On Thu, Dec 10, 2020 at 9:57 PM Thomas Gleixner wrote: > > First of all drivers have absolutely no business to dig into the internals > of an irq descriptor. That's core code and subject to change. All of this > information is readily available to /proc/interrupts in a safe and race > free way. > > Remove the inspection code which is a blatant violation of subsystem > boundaries and racy against concurrent modifications of the interrupt > descriptor. > > Print the irq line instead so the information can be looked up in a sane > way in /proc/interrupts. ... > - seq_printf(s, "%3i: %6i %4i", > + seq_printf(s, "%3i: %6i %4i %4i\n", Seems different specifiers, I think the intention was something like seq_printf(s, "%3i: %4i %6i %4i\n", >line, > + line + irq_first, >num_interrupts[line], >num_wake_interrupts[line]); -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 09/30] ARM: smp: Use irq_desc_kstat_cpu() in show_ipi_list()
On Thu, 10 Dec 2020 19:25:45 +, Thomas Gleixner wrote: > > The irq descriptor is already there, no need to look it up again. > > Signed-off-by: Thomas Gleixner > Cc: Marc Zyngier > Cc: Russell King > Cc: linux-arm-ker...@lists.infradead.org > --- > arch/arm/kernel/smp.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- a/arch/arm/kernel/smp.c > +++ b/arch/arm/kernel/smp.c > @@ -550,7 +550,7 @@ void show_ipi_list(struct seq_file *p, i > seq_printf(p, "%*s%u: ", prec - 1, "IPI", i); > > for_each_online_cpu(cpu) > - seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu)); > + seq_printf(p, "%10u ", irq_desc_kstat_cpu(ipi_desc[i], > cpu)); > > seq_printf(p, " %s\n", ipi_types[i]); > } > > Acked-by: Marc Zyngier -- Without deviation from the norm, progress is not possible. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 10/30] arm64/smp: Use irq_desc_kstat_cpu() in arch_show_interrupts()
On Thu, 10 Dec 2020 19:25:46 +, Thomas Gleixner wrote: > > The irq descriptor is already there, no need to look it up again. > > Signed-off-by: Thomas Gleixner > Cc: Mark Rutland > Cc: Catalin Marinas > Cc: Will Deacon > Cc: Marc Zyngier > Cc: linux-arm-ker...@lists.infradead.org > --- > arch/arm64/kernel/smp.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > --- a/arch/arm64/kernel/smp.c > +++ b/arch/arm64/kernel/smp.c > @@ -809,7 +809,7 @@ int arch_show_interrupts(struct seq_file > seq_printf(p, "%*s%u:%s", prec - 1, "IPI", i, > prec >= 4 ? " " : ""); > for_each_online_cpu(cpu) > - seq_printf(p, "%10u ", kstat_irqs_cpu(irq, cpu)); > + seq_printf(p, "%10u ", irq_desc_kstat_cpu(ipi_desc[i], > cpu)); > seq_printf(p, " %s\n", ipi_types[i]); > } > > > Acked-by: Marc Zyngier -- Without deviation from the norm, progress is not possible. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 03/30] genirq: Move irq_set_lockdep_class() to core
On Thu, Dec 10, 2020 at 10:14 PM Thomas Gleixner wrote: > > irq_set_lockdep_class() is used from modules and requires irq_to_desc() to > be exported. Move it into the core code which lifts another requirement for > the export. ... > + if (IS_ENABLED(CONFIG_LOCKDEP)) > + __irq_set_lockdep_class(irq, lock_class, request_class); Maybe I missed something, but even if the compiler does not warn the use of if IS_ENABLED() with complimentary #ifdef seems inconsistent. > +#ifdef CONFIG_LOCKDEP ... > +EXPORT_SYMBOL_GPL(irq_set_lockdep_class); > +#endif -- With Best Regards, Andy Shevchenko ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 28/65] drm/ttm: WARN_ON non-empty lru when disabling a resource manager
On Fri, Oct 23, 2020 at 04:56:20PM +0200, Daniel Vetter wrote: > On Fri, Oct 23, 2020 at 4:54 PM Christian König > wrote: > > > > Am 23.10.20 um 14:21 schrieb Daniel Vetter: > > > ttm_resource_manager->use_type is only used for runtime changes by > > > vmwgfx. I think ideally we'd push this functionality into drivers - > > > ttm itself does not provide any locking to guarantee this is safe, so > > > the only way this can work at runtime is if the driver does provide > > > additional guarantees. vwmgfx does that through the > > > vmw_private->reservation_sem. Therefore supporting this feature in > > > shared code feels a bit misplaced. > > > > > > As a first step add a WARN_ON to make sure the resource manager is > > > empty. This is just to make sure I actually understand correctly what > > > vmwgfx is doing, and to make sure an eventual subsequent refactor > > > doesn't break anything. > > > > > > This check should also be useful for other drivers, to make sure they > > > haven't leaked anything. > > > > > > Signed-off-by: Daniel Vetter > > > Cc: Christian Koenig > > > Cc: Huang Rui > > > > I'm pretty sure that this will trigger for vmwgfx. But that's what it is > > supposed to do, isn't it? > > Yeah, this is an accidental dump of my wip pile, and it's not done yet > at all. Please disregard (at least for now). > -Daniel > > > Reviewed-by: Christian König Ok decided to submit these 3 patches finally, including the 2 vmwgfx fixes which should avoid the splat. I included your r-b, pls complain if that's not ok anymore. Thanks, Daniel > > > > > --- > > > include/drm/ttm/ttm_resource.h | 4 > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/include/drm/ttm/ttm_resource.h > > > b/include/drm/ttm/ttm_resource.h > > > index f48a70d39ac5..789ec477b607 100644 > > > --- a/include/drm/ttm/ttm_resource.h > > > +++ b/include/drm/ttm/ttm_resource.h > > > @@ -191,6 +191,10 @@ struct ttm_resource { > > > static inline void > > > ttm_resource_manager_set_used(struct ttm_resource_manager *man, bool > > > used) > > > { > > > + int i; > > > + > > > + for (i = 0; i < TTM_MAX_BO_PRIORITY; i++) > > > + WARN_ON(!list_empty(&man->lru[i])); > > > man->use_type = used; > > > } > > > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/ttm: WARN_ON non-empty lru when disabling a resource manager
ttm_resource_manager->use_type is only used for runtime changes by vmwgfx. I think ideally we'd push this functionality into drivers - ttm itself does not provide any locking to guarantee this is safe, so the only way this can work at runtime is if the driver does provide additional guarantees. vwmgfx does that through the vmw_private->reservation_sem. Therefore supporting this feature in shared code feels a bit misplaced. As a first step add a WARN_ON to make sure the resource manager is empty. This is just to make sure I actually understand correctly what vmwgfx is doing, and to make sure an eventual subsequent refactor doesn't break anything. This check should also be useful for other drivers, to make sure they haven't leaked anything. Reviewed-by: Christian König Signed-off-by: Daniel Vetter Cc: Christian Koenig Cc: Huang Rui --- include/drm/ttm/ttm_resource.h | 4 1 file changed, 4 insertions(+) diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h index f48a70d39ac5..789ec477b607 100644 --- a/include/drm/ttm/ttm_resource.h +++ b/include/drm/ttm/ttm_resource.h @@ -191,6 +191,10 @@ struct ttm_resource { static inline void ttm_resource_manager_set_used(struct ttm_resource_manager *man, bool used) { + int i; + + for (i = 0; i < TTM_MAX_BO_PRIORITY; i++) + WARN_ON(!list_empty(&man->lru[i])); man->use_type = used; } -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/vmwgfx: Always evict vram _before_ disabling it
Other way round is a bit inconsistent (but not buggy in any kind). This is prep work so that ttm_resource_manager_set_used can assert that the resource manager is empty. Signed-off-by: Daniel Vetter Cc: VMware Graphics Cc: Roland Scheidegger --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 204f7a1830f0..0d074c432152 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -1250,9 +1250,9 @@ void vmw_svga_disable(struct vmw_private *dev_priv) vmw_kms_lost_device(dev_priv->dev); ttm_write_lock(&dev_priv->reservation_sem, false); if (ttm_resource_manager_used(man)) { - ttm_resource_manager_set_used(man, false); if (ttm_resource_manager_evict_all(&dev_priv->bdev, man)) DRM_ERROR("Failed evicting VRAM buffers.\n"); + ttm_resource_manager_set_used(man, false); vmw_write(dev_priv, SVGA_REG_ENABLE, SVGA_REG_ENABLE_HIDE | SVGA_REG_ENABLE_ENABLE); -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/vmwgfx: Drop svga_lock
This isn't actually protecting anything becuase: - when running, ttm_resource_manager->use_type is protected through vmw_private->reservation_semaphore against concurrent execbuf or well anything else that might evict or reserve buffers - during suspend/resume there's nothing else running, hence vmw_pm_freeze and vmw_pm_restore do not need to take the same lock. - this also holds for the SVGA_REG_ENABLE register write Hence it is safe to just remove that spinlock. Signed-off-by: Daniel Vetter Cc: VMware Graphics Cc: Roland Scheidegger --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 10 +- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 1 - 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 0008be02d31c..204f7a1830f0 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -672,7 +672,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned long chipset) spin_lock_init(&dev_priv->hw_lock); spin_lock_init(&dev_priv->waiter_lock); spin_lock_init(&dev_priv->cap_lock); - spin_lock_init(&dev_priv->svga_lock); spin_lock_init(&dev_priv->cursor_lock); for (i = vmw_res_context; i < vmw_res_max; ++i) { @@ -1189,12 +1188,10 @@ static void __vmw_svga_enable(struct vmw_private *dev_priv) { struct ttm_resource_manager *man = ttm_manager_type(&dev_priv->bdev, TTM_PL_VRAM); - spin_lock(&dev_priv->svga_lock); if (!ttm_resource_manager_used(man)) { vmw_write(dev_priv, SVGA_REG_ENABLE, SVGA_REG_ENABLE); ttm_resource_manager_set_used(man, true); } - spin_unlock(&dev_priv->svga_lock); } /** @@ -1220,14 +1217,12 @@ static void __vmw_svga_disable(struct vmw_private *dev_priv) { struct ttm_resource_manager *man = ttm_manager_type(&dev_priv->bdev, TTM_PL_VRAM); - spin_lock(&dev_priv->svga_lock); if (ttm_resource_manager_used(man)) { ttm_resource_manager_set_used(man, false); vmw_write(dev_priv, SVGA_REG_ENABLE, SVGA_REG_ENABLE_HIDE | SVGA_REG_ENABLE_ENABLE); } - spin_unlock(&dev_priv->svga_lock); } /** @@ -1254,17 +1249,14 @@ void vmw_svga_disable(struct vmw_private *dev_priv) */ vmw_kms_lost_device(dev_priv->dev); ttm_write_lock(&dev_priv->reservation_sem, false); - spin_lock(&dev_priv->svga_lock); if (ttm_resource_manager_used(man)) { ttm_resource_manager_set_used(man, false); - spin_unlock(&dev_priv->svga_lock); if (ttm_resource_manager_evict_all(&dev_priv->bdev, man)) DRM_ERROR("Failed evicting VRAM buffers.\n"); vmw_write(dev_priv, SVGA_REG_ENABLE, SVGA_REG_ENABLE_HIDE | SVGA_REG_ENABLE_ENABLE); - } else - spin_unlock(&dev_priv->svga_lock); + } ttm_write_unlock(&dev_priv->reservation_sem); } diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h index 5b9a28157dd3..715f2bfee08a 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h @@ -596,7 +596,6 @@ struct vmw_private { bool stealth; bool enable_fb; - spinlock_t svga_lock; /** * PM management. -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs
On Thu 2020-12-10 16:08:05, Andy Shevchenko wrote: > On Thu, Dec 10, 2020 at 03:55:27PM +0200, Sakari Ailus wrote: > > On Thu, Dec 10, 2020 at 03:05:02PM +0200, Andy Shevchenko wrote: > > > My concerns are: > > > - not so standard format of representation (why not to use > > > string_escape_mem() helper?) or is it? > > > > The format string may contain spaces that are not meant to be printed. > > Other unprintable chacaters should not be present (at least not in V4L2 > > pixelformats). The hexadecimal representation is there to convey the > > numerical value and that originally came from DRM, not V4L2. > > Yes, but I mean that we usually anticipate the escaped characters in a form of > '\xNN' (hex) or '\NNN' (octal). The format '(NN)' is quite unusual to me. It is true that I have been a bit confused when I saw it. > > > - no compatibility with generic 4cc > > > (I would rather have an additional specifier here for v4l2 cases. > > > > What do you mean by "generic 4cc"? There are two users of 4cc codes in the > > kernel that I know of: V4L2 and DRM. Something that does not refer to > > in-memory pixel formats? > > Of course. Everything else. 4cc is a generic term to describe something which > is of 4 characters long [1]. It's not limited by media file formats. And > moreover some (chip) vendors are using it as well (Synopsys). > Microsoft uses 4cc in CSRT ACPI table for vendor field and so on... Honestly, I do not even know where exactly it is going to be used. I did not have strong opinion. So I just followed the long discussions about previous revisions. Some people loved it from the beginning. Some people were concerned. Anyway, there were discussed only implementation details in the last two revisions, so I assumed that the idea was more or less accepted. Would it help to send another revision with some existing DRM and V4L2 code converted to use it? It would help me/us to see how much different it is from the current output. Also it will require ack from the affected subsystem maintainers and developers. Best Regqrds, Petr ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/vkms: Unset preferred_depth
On Fri, Dec 11, 2020 at 06:21:40PM +0200, Ville Syrjälä wrote: > On Fri, Dec 11, 2020 at 05:11:12PM +0100, Daniel Vetter wrote: > > There's a confusion between the preferred_depth uapi and the generic > > fbdev helpers. Former wants depth, latter wants bpp, and for XRGB > > they don't match. Which hit me with vkms, which wants that. > > > > All other drivers setting this and using the generic fbdev helpers use > > 16, where both numbers match, for RGB565. > > > > Since fixing this is a bit involved (I think for atomic drivers we > > should just compute this all internally from the format list of the > > first primary plane) paper over the issue in vkms by using defaults > > everywhere. Then userspace will pick XRGB, and fbdev helpers will > > do the same, and we have what we want. > > I think I had a patch ages ago that tried to improve the fb_helper > pixel format stuff a bit. This one I think: > https://patchwork.freedesktop.org/patch/203189/ > > Haven't checked how much of that would still be relevant though. Uh this looks useful, my attempts have only tried to clean up the driver side in what they're setting for preferred_depth and how they set up the fbdev emulation. I haven't looked at the fbdev code itself, and especially your mapping table should be useful. Care to resubmit that one again? -Daniel > > > > > Reported-by: Simon Ser > > Reviewed-by: Simon Ser > > Cc: Simon Ser > > Signed-off-by: Daniel Vetter > > Cc: Rodrigo Siqueira > > Cc: Melissa Wen > > Cc: Haneen Mohammed > > Cc: Daniel Vetter > > --- > > drivers/gpu/drm/vkms/vkms_drv.c | 5 - > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c > > b/drivers/gpu/drm/vkms/vkms_drv.c > > index d4d39227f2ed..aef29393b811 100644 > > --- a/drivers/gpu/drm/vkms/vkms_drv.c > > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > > @@ -113,7 +113,10 @@ static int vkms_modeset_init(struct vkms_device > > *vkmsdev) > > dev->mode_config.max_height = YRES_MAX; > > dev->mode_config.cursor_width = 512; > > dev->mode_config.cursor_height = 512; > > - dev->mode_config.preferred_depth = 32; > > + /* FIXME: There's a confusion between bpp and depth between this and > > +* fbdev helpers. We have to go with 0, meaning "pick the default", > > +* which ix XRGB in all cases. */ > > + dev->mode_config.preferred_depth = 0; > > dev->mode_config.helper_private = &vkms_mode_config_helpers; > > > > return vkms_output_init(vkmsdev, 0); > > -- > > 2.29.2 > > > > ___ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrjälä > Intel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/vkms: Unset preferred_depth
On Fri, Dec 11, 2020 at 05:11:12PM +0100, Daniel Vetter wrote: > There's a confusion between the preferred_depth uapi and the generic > fbdev helpers. Former wants depth, latter wants bpp, and for XRGB > they don't match. Which hit me with vkms, which wants that. > > All other drivers setting this and using the generic fbdev helpers use > 16, where both numbers match, for RGB565. > > Since fixing this is a bit involved (I think for atomic drivers we > should just compute this all internally from the format list of the > first primary plane) paper over the issue in vkms by using defaults > everywhere. Then userspace will pick XRGB, and fbdev helpers will > do the same, and we have what we want. I think I had a patch ages ago that tried to improve the fb_helper pixel format stuff a bit. This one I think: https://patchwork.freedesktop.org/patch/203189/ Haven't checked how much of that would still be relevant though. > > Reported-by: Simon Ser > Reviewed-by: Simon Ser > Cc: Simon Ser > Signed-off-by: Daniel Vetter > Cc: Rodrigo Siqueira > Cc: Melissa Wen > Cc: Haneen Mohammed > Cc: Daniel Vetter > --- > drivers/gpu/drm/vkms/vkms_drv.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c > index d4d39227f2ed..aef29393b811 100644 > --- a/drivers/gpu/drm/vkms/vkms_drv.c > +++ b/drivers/gpu/drm/vkms/vkms_drv.c > @@ -113,7 +113,10 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev) > dev->mode_config.max_height = YRES_MAX; > dev->mode_config.cursor_width = 512; > dev->mode_config.cursor_height = 512; > - dev->mode_config.preferred_depth = 32; > + /* FIXME: There's a confusion between bpp and depth between this and > + * fbdev helpers. We have to go with 0, meaning "pick the default", > + * which ix XRGB in all cases. */ > + dev->mode_config.preferred_depth = 0; > dev->mode_config.helper_private = &vkms_mode_config_helpers; > > return vkms_output_init(vkmsdev, 0); > -- > 2.29.2 > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 27/30] xen/events: Only force affinity mask for percpu interrupts
On 11.12.20 00:20, boris.ostrov...@oracle.com wrote: On 12/10/20 2:26 PM, Thomas Gleixner wrote: All event channel setups bind the interrupt on CPU0 or the target CPU for percpu interrupts and overwrite the affinity mask with the corresponding cpumask. That does not make sense. The XEN implementation of irqchip::irq_set_affinity() already picks a single target CPU out of the affinity mask and the actual target is stored in the effective CPU mask, so destroying the user chosen affinity mask which might contain more than one CPU is wrong. Change the implementation so that the channel is bound to CPU0 at the XEN level and leave the affinity mask alone. At startup of the interrupt affinity will be assigned out of the affinity mask and the XEN binding will be updated. If that's the case then I wonder whether we need this call at all and instead bind at startup time. After some discussion with Thomas on IRC and xen-devel archaeology the result is: this will be needed especially for systems running on a single vcpu (e.g. small guests), as the .irq_set_affinity() callback won't be called in this case when starting the irq. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/vkms: Unset preferred_depth
There's a confusion between the preferred_depth uapi and the generic fbdev helpers. Former wants depth, latter wants bpp, and for XRGB they don't match. Which hit me with vkms, which wants that. All other drivers setting this and using the generic fbdev helpers use 16, where both numbers match, for RGB565. Since fixing this is a bit involved (I think for atomic drivers we should just compute this all internally from the format list of the first primary plane) paper over the issue in vkms by using defaults everywhere. Then userspace will pick XRGB, and fbdev helpers will do the same, and we have what we want. Reported-by: Simon Ser Reviewed-by: Simon Ser Cc: Simon Ser Signed-off-by: Daniel Vetter Cc: Rodrigo Siqueira Cc: Melissa Wen Cc: Haneen Mohammed Cc: Daniel Vetter --- drivers/gpu/drm/vkms/vkms_drv.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c index d4d39227f2ed..aef29393b811 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.c +++ b/drivers/gpu/drm/vkms/vkms_drv.c @@ -113,7 +113,10 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev) dev->mode_config.max_height = YRES_MAX; dev->mode_config.cursor_width = 512; dev->mode_config.cursor_height = 512; - dev->mode_config.preferred_depth = 32; + /* FIXME: There's a confusion between bpp and depth between this and +* fbdev helpers. We have to go with 0, meaning "pick the default", +* which ix XRGB in all cases. */ + dev->mode_config.preferred_depth = 0; dev->mode_config.helper_private = &vkms_mode_config_helpers; return vkms_output_init(vkmsdev, 0); -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/fb-helper: Add a FIXME that generic_setup is very confusing
I tried to fix this for real, but it's very sprawling and lots of drivers get this mildly wrong one way or the other. Signed-off-by: Daniel Vetter Cc: Maarten Lankhorst Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: David Airlie Cc: Daniel Vetter --- drivers/gpu/drm/drm_fb_helper.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index 596255edf023..27deed4af015 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -2494,6 +2494,11 @@ void drm_fbdev_generic_setup(struct drm_device *dev, return; } + /* +* FIXME: This mixes up depth with bpp, which results in a glorious +* mess, resulting in some drivers picking wrong fbdev defaults and +* others wrong preferred_depth defaults. +*/ if (!preferred_bpp) preferred_bpp = dev->mode_config.preferred_depth; if (!preferred_bpp) -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/panel: khadas: Fix error code in khadas_ts050_panel_add()
Hi All, On 11/12/2020 12:01, Sam Ravnborg wrote: > Hi Dan, > > I assume your nice tooling found this buggy. > Nice! > > On Fri, Dec 11, 2020 at 01:05:50PM +0300, Dan Carpenter wrote: >> There is a copy and paste bug so it didn't return the correct error >> code. >> >> Fixes: b215212117f7 ("drm: panel: add Khadas TS050 panel driver") >> Signed-off-by: Dan Carpenter Thanks for the fix ! > > Reviewed-by: Sam Ravnborg > > Neil, I assume you will take this via next-fixes, which I think is the > right tree at the moment. Sure, but seems drm-misc-next-fixes is out of sync with drm-misc-next right now, I'll apply it when it's sync'ed back. Neil > > Sam > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/4] dma-buf: begin/end_cpu might lock the dma_resv lock
At least amdgpu and i915 do, so lets just document this as the rule. Signed-off-by: Daniel Vetter Cc: Thomas Zimmermann Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org --- drivers/dma-buf/dma-buf.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index e1fa6c6f02c4..00d5afe904cc 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1118,6 +1118,8 @@ int dma_buf_begin_cpu_access(struct dma_buf *dmabuf, if (WARN_ON(!dmabuf)) return -EINVAL; + might_lock(&dma_buf->resv.lock); + if (dmabuf->ops->begin_cpu_access) ret = dmabuf->ops->begin_cpu_access(dmabuf, direction); @@ -1151,6 +1153,8 @@ int dma_buf_end_cpu_access(struct dma_buf *dmabuf, WARN_ON(!dmabuf); + might_lock(&dma_buf->resv.lock); + if (dmabuf->ops->end_cpu_access) ret = dmabuf->ops->end_cpu_access(dmabuf, direction); -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/4] dma-buf: doc polish for pin/unpin
Motivated by a discussion with Christian and Thomas: Try to untangle a bit what's relevant for importers and what's relevant for exporters. Also add an assert that really only dynamic importers use the api function, anything else doesn't make sense. Signed-off-by: Daniel Vetter Cc: Thomas Zimmermann Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org --- drivers/dma-buf/dma-buf.c | 19 --- include/linux/dma-buf.h | 8 +--- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 00d5afe904cc..1065545e9ca1 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -809,9 +809,15 @@ EXPORT_SYMBOL_GPL(dma_buf_detach); /** * dma_buf_pin - Lock down the DMA-buf - * * @attach:[in]attachment which should be pinned * + * Only dynamic importers (who set up @attach with dma_buf_dynamic_attach()) may + * call this, and only for limited use cases like scanout and not for temporary + * pin operations. It is not permitted to allow userspace to pin arbitrary + * amounts of buffers through this interface. + * + * Buffers must be unpinned by calling dma_buf_unpin(). + * * Returns: * 0 on success, negative error code on failure. */ @@ -820,6 +826,8 @@ int dma_buf_pin(struct dma_buf_attachment *attach) struct dma_buf *dmabuf = attach->dmabuf; int ret = 0; + WARN_ON(!dma_buf_attachment_is_dynamic(attach)); + dma_resv_assert_held(dmabuf->resv); if (dmabuf->ops->pin) @@ -830,14 +838,19 @@ int dma_buf_pin(struct dma_buf_attachment *attach) EXPORT_SYMBOL_GPL(dma_buf_pin); /** - * dma_buf_unpin - Remove lock from DMA-buf - * + * dma_buf_unpin - Unpin a DMA-buf * @attach:[in]attachment which should be unpinned + * + * This unpins a buffer pinned by dma_buf_pin() and allows the exporter to move + * any mapping of @attach again and inform the importer through + * &dma_buf_attach_ops.move_notify. */ void dma_buf_unpin(struct dma_buf_attachment *attach) { struct dma_buf *dmabuf = attach->dmabuf; + WARN_ON(!dma_buf_attachment_is_dynamic(attach)); + dma_resv_assert_held(dmabuf->resv); if (dmabuf->ops->unpin) diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 43802a31b25d..628681bf6c99 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -86,13 +86,15 @@ struct dma_buf_ops { * @pin: * * This is called by dma_buf_pin() and lets the exporter know that the -* DMA-buf can't be moved any more. +* DMA-buf can't be moved any more. The exporter should pin the buffer +* into system memory to make sure it is generally accessible by other +* devices. * * This is called with the &dmabuf.resv object locked and is mutual * exclusive with @cache_sgt_mapping. * -* This callback is optional and should only be used in limited use -* cases like scanout and not for temporary pin operations. +* This is called automatically for non-dynamic importers from +* dma_buf_attach(). * * Returns: * -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/4] dma-buf: some kerneldoc formatting fixes
Noticed while reviewing the output. Adds a bunch more links and fixes the function interface quoting. Signed-off-by: Daniel Vetter Cc: Thomas Zimmermann Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org --- drivers/dma-buf/dma-buf.c | 31 ++- include/linux/dma-buf.h | 6 +++--- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index a12fdffa130f..e1fa6c6f02c4 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -480,7 +480,7 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) * * 4. Once a driver is done with a shared buffer it needs to call *dma_buf_detach() (after cleaning up any mappings) and then release the - *reference acquired with dma_buf_get by calling dma_buf_put(). + *reference acquired with dma_buf_get() by calling dma_buf_put(). * * For the detailed semantics exporters are expected to implement see * &dma_buf_ops. @@ -496,9 +496,10 @@ static struct file *dma_buf_getfile(struct dma_buf *dmabuf, int flags) * by the exporter. see &struct dma_buf_export_info * for further details. * - * Returns, on success, a newly created dma_buf object, which wraps the - * supplied private data and operations for dma_buf_ops. On either missing - * ops, or error in allocating struct dma_buf, will return negative error. + * Returns, on success, a newly created struct dma_buf object, which wraps the + * supplied private data and operations for struct dma_buf_ops. On either + * missing ops, or error in allocating struct dma_buf, will return negative + * error. * * For most cases the easiest way to create @exp_info is through the * %DEFINE_DMA_BUF_EXPORT_INFO macro. @@ -584,7 +585,7 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) EXPORT_SYMBOL_GPL(dma_buf_export); /** - * dma_buf_fd - returns a file descriptor for the given dma_buf + * dma_buf_fd - returns a file descriptor for the given struct dma_buf * @dmabuf:[in]pointer to dma_buf for which fd is required. * @flags: [in]flags to give to fd * @@ -608,10 +609,10 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags) EXPORT_SYMBOL_GPL(dma_buf_fd); /** - * dma_buf_get - returns the dma_buf structure related to an fd - * @fd:[in]fd associated with the dma_buf to be returned + * dma_buf_get - returns the struct dma_buf related to an fd + * @fd:[in]fd associated with the struct dma_buf to be returned * - * On success, returns the dma_buf structure associated with an fd; uses + * On success, returns the struct dma_buf associated with an fd; uses * file's refcounting done by fget to increase refcount. returns ERR_PTR * otherwise. */ @@ -653,8 +654,7 @@ void dma_buf_put(struct dma_buf *dmabuf) EXPORT_SYMBOL_GPL(dma_buf_put); /** - * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list; optionally, - * calls attach() of dma_buf_ops to allow device-specific attach functionality + * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list * @dmabuf:[in]buffer to attach device to. * @dev: [in]device to be attached. * @importer_ops: [in]importer operations for the attachment @@ -663,6 +663,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put); * Returns struct dma_buf_attachment pointer for this attachment. Attachments * must be cleaned up by calling dma_buf_detach(). * + * Optionally this calls &dma_buf_ops.attach to allow device-specific attach + * functionality. + * * Returns: * * A pointer to newly created &dma_buf_attachment on success, or a negative @@ -769,12 +772,13 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, EXPORT_SYMBOL_GPL(dma_buf_attach); /** - * dma_buf_detach - Remove the given attachment from dmabuf's attachments list; - * optionally calls detach() of dma_buf_ops for device-specific detach + * dma_buf_detach - Remove the given attachment from dmabuf's attachments list * @dmabuf:[in]buffer to detach from. * @attach:[in]attachment to be detached; is free'd after this call. * * Clean up a device attachment obtained by calling dma_buf_attach(). + * + * Optionally this calls &dma_buf_ops.detach for device-specific detach. */ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) { @@ -1061,11 +1065,12 @@ EXPORT_SYMBOL_GPL(dma_buf_move_notify); * shootdowns would increase the complexity quite a bit. * * Interface:: + * * int dma_buf_mmap(struct dma_buf \*, struct vm_area_struct \*, *unsigned long); * * If the importing subsystem simply provides a special-purpose mmap call to - * set up a mapping in userspace, calling do_mmap with dma_buf->file will + * set up a mapping in userspace, calling do_m
[PATCH 1/4] dma-buf: Remove kmap kerneldoc vestiges
Also try to clarify a bit when dma_buf_begin/end_cpu_access should be called. Signed-off-by: Daniel Vetter Cc: Thomas Zimmermann Cc: Sumit Semwal Cc: "Christian König" Cc: linux-me...@vger.kernel.org Cc: linaro-mm-...@lists.linaro.org --- drivers/dma-buf/dma-buf.c | 20 ++-- include/linux/dma-buf.h | 25 + 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index e63684d4cd90..a12fdffa130f 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1001,15 +1001,15 @@ EXPORT_SYMBOL_GPL(dma_buf_move_notify); * vmalloc space might be limited and result in vmap calls failing. * * Interfaces:: + * * void \*dma_buf_vmap(struct dma_buf \*dmabuf) * void dma_buf_vunmap(struct dma_buf \*dmabuf, void \*vaddr) * * The vmap call can fail if there is no vmap support in the exporter, or if - * it runs out of vmalloc space. Fallback to kmap should be implemented. Note - * that the dma-buf layer keeps a reference count for all vmap access and - * calls down into the exporter's vmap function only when no vmapping exists, - * and only unmaps it once. Protection against concurrent vmap/vunmap calls is - * provided by taking the dma_buf->lock mutex. + * it runs out of vmalloc space. Note that the dma-buf layer keeps a reference + * count for all vmap access and calls down into the exporter's vmap function + * only when no vmapping exists, and only unmaps it once. Protection against + * concurrent vmap/vunmap calls is provided by taking the &dma_buf.lock mutex. * * - For full compatibility on the importer side with existing userspace * interfaces, which might already support mmap'ing buffers. This is needed in @@ -1098,6 +1098,11 @@ static int __dma_buf_begin_cpu_access(struct dma_buf *dmabuf, * dma_buf_end_cpu_access(). Only when cpu access is braketed by both calls is * it guaranteed to be coherent with other DMA access. * + * This function will also wait for any DMA transactions tracked through + * implicit synchronization in &dma_buf.resv. For DMA transactions with explicit + * synchronization this function will only ensure cache coherency, callers must + * ensure synchronization with such DMA transactions on their own. + * * Can return negative error values, returns 0 on success. */ int dma_buf_begin_cpu_access(struct dma_buf *dmabuf, @@ -1199,7 +1204,10 @@ EXPORT_SYMBOL_GPL(dma_buf_mmap); * This call may fail due to lack of virtual mapping address space. * These calls are optional in drivers. The intended use for them * is for mapping objects linear in kernel space for high use objects. - * Please attempt to use kmap/kunmap before thinking about these interfaces. + * + * To ensure coherency users must call dma_buf_begin_cpu_access() and + * dma_buf_end_cpu_access() around any cpu access performed through this + * mapping. * * Returns 0 on success, or a negative errno code otherwise. */ diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index cf72699cb2bc..7eca37c8b10c 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -183,24 +183,19 @@ struct dma_buf_ops { * @begin_cpu_access: * * This is called from dma_buf_begin_cpu_access() and allows the -* exporter to ensure that the memory is actually available for cpu -* access - the exporter might need to allocate or swap-in and pin the -* backing storage. The exporter also needs to ensure that cpu access is -* coherent for the access direction. The direction can be used by the -* exporter to optimize the cache flushing, i.e. access with a different +* exporter to ensure that the memory is actually coherent for cpu +* access. The exporter also needs to ensure that cpu access is coherent +* for the access direction. The direction can be used by the exporter +* to optimize the cache flushing, i.e. access with a different * direction (read instead of write) might return stale or even bogus * data (e.g. when the exporter needs to copy the data to temporary * storage). * -* This callback is optional. +* Note that this is both called through the DMA_BUF_IOCTL_SYNC IOCTL +* command for userspace mappings established through @mmap, and also +* for kernel mappings established with @vmap. * -* FIXME: This is both called through the DMA_BUF_IOCTL_SYNC command -* from userspace (where storage shouldn't be pinned to avoid handing -* de-factor mlock rights to userspace) and for the kernel-internal -* users of the various kmap interfaces, where the backing storage must -* be pinned to guarantee that the atomic kmap calls can succeed. Since -* there's no in-kernel users of the kmap interfaces yet this isn't a -* real problem.
[PATCH v4 1/2] drm: automatic legacy gamma support
To support legacy gamma ioctls the drivers need to set drm_crtc_funcs.gamma_set either to a custom implementation or to drm_atomic_helper_legacy_gamma_set. Most of the atomic drivers do the latter. We can simplify this by making the core handle it automatically. Move the drm_atomic_helper_legacy_gamma_set() functionality into drm_color_mgmt.c to make drm_mode_gamma_set_ioctl() use drm_crtc_funcs.gamma_set if set or GAMMA_LUT property if not. Signed-off-by: Tomi Valkeinen --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 - .../gpu/drm/arm/display/komeda/komeda_crtc.c | 1 - drivers/gpu/drm/arm/malidp_crtc.c | 1 - drivers/gpu/drm/armada/armada_crtc.c | 1 - drivers/gpu/drm/ast/ast_mode.c| 1 - .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c| 1 - drivers/gpu/drm/drm_atomic_helper.c | 70 --- drivers/gpu/drm/drm_color_mgmt.c | 111 -- drivers/gpu/drm/i915/display/intel_display.c | 1 - drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 - drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 - drivers/gpu/drm/nouveau/dispnv50/head.c | 2 - drivers/gpu/drm/omapdrm/omap_crtc.c | 1 - drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 1 - drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 1 - drivers/gpu/drm/stm/ltdc.c| 1 - drivers/gpu/drm/vc4/vc4_crtc.c| 1 - drivers/gpu/drm/vc4/vc4_txp.c | 1 - include/drm/drm_atomic_helper.h | 4 - 19 files changed, 102 insertions(+), 101 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 2855bb918535..848b06c51b0e 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -5396,7 +5396,6 @@ static void dm_disable_vblank(struct drm_crtc *crtc) static const struct drm_crtc_funcs amdgpu_dm_crtc_funcs = { .reset = dm_crtc_reset_state, .destroy = amdgpu_dm_crtc_destroy, - .gamma_set = drm_atomic_helper_legacy_gamma_set, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, .atomic_duplicate_state = dm_crtc_duplicate_state, diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c index 4b485eb512e2..59172acb9738 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_crtc.c @@ -550,7 +550,6 @@ static void komeda_crtc_vblank_disable(struct drm_crtc *crtc) } static const struct drm_crtc_funcs komeda_crtc_funcs = { - .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy= drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, diff --git a/drivers/gpu/drm/arm/malidp_crtc.c b/drivers/gpu/drm/arm/malidp_crtc.c index 108e7a31bd26..494075ddbef6 100644 --- a/drivers/gpu/drm/arm/malidp_crtc.c +++ b/drivers/gpu/drm/arm/malidp_crtc.c @@ -510,7 +510,6 @@ static void malidp_crtc_disable_vblank(struct drm_crtc *crtc) } static const struct drm_crtc_funcs malidp_crtc_funcs = { - .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index 3ebcf5a52c8b..b7bb90ae787f 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -820,7 +820,6 @@ static const struct drm_crtc_funcs armada_crtc_funcs = { .cursor_set = armada_drm_crtc_cursor_set, .cursor_move= armada_drm_crtc_cursor_move, .destroy= armada_drm_crtc_destroy, - .gamma_set = drm_atomic_helper_legacy_gamma_set, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c index 9db371f4054f..5b0ec785c516 100644 --- a/drivers/gpu/drm/ast/ast_mode.c +++ b/drivers/gpu/drm/ast/ast_mode.c @@ -903,7 +903,6 @@ static void ast_crtc_atomic_destroy_state(struct drm_crtc *crtc, static const struct drm_crtc_funcs ast_crtc_funcs = { .reset = ast_crtc_reset, - .gamma_set = drm_atomic_helper_legacy_gamma_set, .destroy = drm_crtc_cleanup, .set_config = drm_atomic_helper_set_config, .page_flip = drm_atomic_helper_page_flip, diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index c58fa00b4848..05ad75d155e8 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers
[PATCH v4 2/2] drm: add legacy support for using degamma for gamma
The DRM core handles legacy gamma-set ioctl by setting GAMMA_LUT and clearing CTM and DEGAMMA_LUT. This works fine on HW where we have either: degamma -> ctm -> gamma -> out or ctm -> gamma -> out However, if the HW has gamma table before ctm, the atomic property should be DEGAMMA_LUT, and thus we have: degamma -> ctm -> out This is fine for userspace which sets gamma table using the properties, as the userspace can check for the existence of gamma & degamma, but the legacy gamma-set ioctl does not work. Change the DRM core to use DEGAMMA_LUT instead of GAMMA_LUT when the latter is unavailable. Signed-off-by: Tomi Valkeinen --- drivers/gpu/drm/drm_color_mgmt.c | 22 ++ drivers/gpu/drm/drm_fb_helper.c | 5 + 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 65eb36dc92bf..9100aac1570c 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -91,7 +91,7 @@ * * There is also support for a legacy gamma table, which is set up by calling * drm_mode_crtc_set_gamma_size(). The DRM core will then alias the legacy gamma - * ramp with "GAMMA_LUT". + * ramp with "GAMMA_LUT" or, if that is unavailable, "DEGAMMA_LUT". * * Support for different non RGB color encodings is controlled through * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties. They @@ -238,6 +238,7 @@ EXPORT_SYMBOL(drm_mode_crtc_set_gamma_size); static bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc) { uint32_t gamma_id = crtc->dev->mode_config.gamma_lut_property->base.id; + uint32_t degamma_id = crtc->dev->mode_config.degamma_lut_property->base.id; if (!crtc->gamma_size) return false; @@ -245,7 +246,8 @@ static bool drm_crtc_supports_legacy_gamma(struct drm_crtc *crtc) if (crtc->funcs->gamma_set) return true; - return !!drm_mode_obj_find_prop_id(&crtc->base, gamma_id); + return !!(drm_mode_obj_find_prop_id(&crtc->base, gamma_id) || + drm_mode_obj_find_prop_id(&crtc->base, degamma_id)); } /** @@ -276,12 +278,22 @@ static int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state; struct drm_property_blob *blob; struct drm_color_lut *blob_data; + uint32_t gamma_id = dev->mode_config.gamma_lut_property->base.id; + uint32_t degamma_id = dev->mode_config.degamma_lut_property->base.id; + bool use_gamma_lut; int i, ret = 0; bool replaced; if (crtc->funcs->gamma_set) return crtc->funcs->gamma_set(crtc, red, green, blue, size, ctx); + if (drm_mode_obj_find_prop_id(&crtc->base, gamma_id)) + use_gamma_lut = true; + else if (drm_mode_obj_find_prop_id(&crtc->base, degamma_id)) + use_gamma_lut = false; + else + return -ENODEV; + state = drm_atomic_state_alloc(crtc->dev); if (!state) return -ENOMEM; @@ -311,9 +323,11 @@ static int drm_crtc_legacy_gamma_set(struct drm_crtc *crtc, } /* Set GAMMA_LUT and reset DEGAMMA_LUT and CTM */ - replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); + replaced = drm_property_replace_blob(&crtc_state->degamma_lut, +use_gamma_lut ? NULL : blob); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); - replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, blob); + replaced |= drm_property_replace_blob(&crtc_state->gamma_lut, + use_gamma_lut ? blob : NULL); crtc_state->color_mgmt_changed |= replaced; ret = drm_atomic_commit(state); diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index e82db0f4e771..5ad19785daee 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -1059,6 +1059,11 @@ static int setcmap_atomic(struct fb_cmap *cmap, struct fb_info *info) goto out_state; } + /* +* FIXME: This always uses gamma_lut. Some HW have only +* degamma_lut, in which case we should reset gamma_lut and set +* degamma_lut. See drm_crtc_legacy_gamma_set(). +*/ replaced = drm_property_replace_blob(&crtc_state->degamma_lut, NULL); replaced |= drm_property_replace_blob(&crtc_state->ctm, NULL); -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 0/2] drm: automatic legacy gamma support
Hi, Yet another try. This time I don't touch the drm_fb_helper.c at all, except adding a FIXME comment. Now everything is inside drm_color_mgmt.c. Tomi Tomi Valkeinen (2): drm: automatic legacy gamma support drm: add legacy support for using degamma for gamma .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 1 - .../gpu/drm/arm/display/komeda/komeda_crtc.c | 1 - drivers/gpu/drm/arm/malidp_crtc.c | 1 - drivers/gpu/drm/armada/armada_crtc.c | 1 - drivers/gpu/drm/ast/ast_mode.c| 1 - .../gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c| 1 - drivers/gpu/drm/drm_atomic_helper.c | 70 -- drivers/gpu/drm/drm_color_mgmt.c | 125 -- drivers/gpu/drm/drm_fb_helper.c | 5 + drivers/gpu/drm/i915/display/intel_display.c | 1 - drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 2 - drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 1 - drivers/gpu/drm/nouveau/dispnv50/head.c | 2 - drivers/gpu/drm/omapdrm/omap_crtc.c | 1 - drivers/gpu/drm/rcar-du/rcar_du_crtc.c| 1 - drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 1 - drivers/gpu/drm/stm/ltdc.c| 1 - drivers/gpu/drm/vc4/vc4_crtc.c| 1 - drivers/gpu/drm/vc4/vc4_txp.c | 1 - include/drm/drm_atomic_helper.h | 4 - 20 files changed, 121 insertions(+), 101 deletions(-) -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH 1/2] drm: RFC add choice to use dynamic debug in drm-debug
On Thu, Dec 03, 2020 at 08:53:17PM -0700, Jim Cromie wrote: > drm's debug system uses distinct categories of debug messages, mapped > to bits in drm.debug. Currently, code does a lot of unlikely bit-mask > checks on drm.debug (in drm_debug_enabled), we can use dynamic debug > instead, and get all that jump_label goodness. > > RFC: dynamic debug has no concept of category, but we can do without > one if we can prepend a class-prefix to each printk format. Then we > can use "format ^prefix" to select the whole category with one query. > This is a log-facing and user visible change, but it seems unlikely to > cause trouble for log watchers; they're not relying on the absence of > class prefix strings. > > This conversion yields ~2100 new callsites on my i7 laptop: > > dyndbg: 195 debug prints in module drm_kms_helper > dyndbg: 298 debug prints in module drm > dyndbg: 1630 debug prints in module i915 > > Since this change has wide-ranging effects (many drm drivers, with > many callsites, and kernel image growth), and most vendors don't > enable DYNAMIC_DEBUG, we supplement the existing mechanism, adding > CONFIG_DRM_USE_DYNAMIC_DEBUG to enable the new one. > > The indirection/switchover has a few parts: > > 1 a new callback on drm.debug which calls dynamic_debug_exec_queries > to map those bits to specific query/commands > dynamic_debug_exec_queries("format ^drm:kms: +p", "drm*"); > > 2 a "converted" or "classy" DRM_UT_* map > similar to DRM_UT_* ( symbol => bit-mask ) > named it cDRM_UT_* ( symbol => format-class-prefix-string ) > > cDRM_UT_* is either ( CONFIG_DRM_USE_DYNAMIC_DEBUG ) > legacy: cDRM_UT_* <-- DRM_UT_* > enabled: > +#define cDRM_UT_KMS"drm:kms: " > +#define cDRM_UT_PRIME "drm:prime: " > +#define cDRM_UT_ATOMIC "drm:atomic: " > > these are similar to "gvt: cmd:" in i915/gvt > and effectively a replacement for DRM_NAME > please bikeshed on keys, values. latter are log-facing. > > 3 drm_dev_dbg & drm_debug are renamed (prefixed with '_') > old names are now macros, which are ifdefd > legacy: -> to renamed fn > enabled: -> dev_dbg & pr_debug, after prepending prefix to format. > > 4 names in (2) are called from DRM_DEBUG_ and drm_dbg_. > all these get "converted" to use cDRM_UT_*, to get right token type. > > RFC: for dynamic debug, category is a source-facing addition; > something like pr_debug_cat(cat, ...) would do it, iff cat is a > compile-time const. Note that cat isn't needed in the printing, it > would be saved into a new field in struct _ddebug, and used only for > callsite selection, activation and control. > > Signed-off-by: Jim Cromie > --- > drivers/gpu/drm/Kconfig | 13 ++ > drivers/gpu/drm/drm_print.c | 75 -- > include/drm/drm_print.h | 92 +++-- > 3 files changed, 153 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 147d61b9674e..854bc1ad21fb 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -54,6 +54,19 @@ config DRM_DEBUG_MM > > If in doubt, say "N". > > +config DRM_USE_DYNAMIC_DEBUG > + bool "use dynamic debug to implement drm.debug" > + default n > + depends on DRM > + depends on DYNAMIC_DEBUG > + depends on JUMP_LABEL > + help > + The drm debug category facility does a lot of unlikely bit-field > + tests at runtime; while cheap individually, the cost accumulates. > + This option uses dynamic debug facility (if configured and > + using jump_label) to avoid those runtime checks, patching > + the kernel when those debugs are desired. > + > config DRM_DEBUG_SELFTEST > tristate "kselftests for DRM" > depends on DRM > diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c > index 111b932cf2a9..e2acdfc7088b 100644 > --- a/drivers/gpu/drm/drm_print.c > +++ b/drivers/gpu/drm/drm_print.c > @@ -52,8 +52,75 @@ MODULE_PARM_DESC(debug, "Enable debug output, where each > bit enables a debug cat > "\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n" > "\t\tBit 7 (0x80) will enable LEASE messages (leasing code)\n" > "\t\tBit 8 (0x100) will enable DP messages (displayport code)"); > + > +#ifndef CONFIG_DRM_USE_DYNAMIC_DEBUG > module_param_named(debug, __drm_debug, int, 0600); > > +#else > +static char *format_class_prefixes[] = { > + cDRM_UT_CORE, > + cDRM_UT_DRIVER, > + cDRM_UT_KMS, > + cDRM_UT_PRIME, > + cDRM_UT_ATOMIC, > + cDRM_UT_VBL, > + cDRM_UT_STATE, > + cDRM_UT_LEASE, > + cDRM_UT_DP, > + cDRM_UT_DRMRES > +}; > + > +#define OUR_QUERY_SIZE 64 /* > strlen "format '^%s' %cp" + longest prefix */ > + > +static int param_set_dyndbg(const char *instr, const struct kernel_param *kp) > +{ > + unsigned int val; > + unsigned long changes, result; > + int rc, chgct = 0, totct = 0, bitpos; > + char query[OUR_QUERY_SIZE]
Re: [PATCH v2 2/2] drm: automatic legacy gamma support
On 10/12/2020 20:06, kernel test robot wrote: > Hi Tomi, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on drm-intel/for-linux-next] > [also build test WARNING on linus/master v5.10-rc7] > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: > https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917 > base: git://anongit.freedesktop.org/drm-intel for-linux-next > config: i386-randconfig-m021-20201209 (attached as .config) > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > > New smatch warnings: > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: > potential null dereference 'blob'. (drm_property_create_blob returns null) I don't see how this could happen. There's no code path I see where drm_property_create_blob could return null... Tomi -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 4/5] drm/dp: Extract i915's eDP backlight code into DRM helpers
On Wed, 09 Dec 2020, Lyude Paul wrote: > Since we're about to implement eDP backlight support in nouveau using the > standard protocol from VESA, we might as well just take the code that's > already written for this and move it into a set of shared DRM helpers. > > Note that these helpers are intended to handle DPCD related backlight > control bits such as setting the brightness level over AUX, probing the > backlight's TCON, enabling/disabling the backlight over AUX if supported, > etc. Any PWM-related portions of backlight control are explicitly left up > to the driver, as these will vary from platform to platform. > > The only exception to this is the calculation of the PWM frequency > pre-divider value. This is because the only platform-specific information > required for this is the PWM frequency of the panel, which the driver is > expected to provide if available. The actual algorithm for calculating this > value is standard and is defined in the eDP specification from VESA. > > Note that these helpers do not yet implement the full range of features > the VESA backlight interface provides, and only provide the following > functionality (all of which was already present in i915's DPCD backlight > support): > > * Basic control of brightness levels > * Basic probing of backlight capabilities > * Helpers for enabling and disabling the backlight Overall I like where this is going. Again, not a full review yet, just a few notes below. > > Signed-off-by: Lyude Paul > Cc: Jani Nikula > Cc: Dave Airlie > Cc: greg.depo...@gmail.com > --- > drivers/gpu/drm/drm_dp_helper.c | 332 ++ > .../drm/i915/display/intel_display_types.h| 5 +- > .../drm/i915/display/intel_dp_aux_backlight.c | 304 ++-- > include/drm/drm_dp_helper.h | 48 +++ > 4 files changed, 419 insertions(+), 270 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index 5bd0934004e3..06fdddf44e54 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -2596,3 +2596,335 @@ void drm_dp_vsc_sdp_log(const char *level, struct > device *dev, > #undef DP_SDP_LOG > } > EXPORT_SYMBOL(drm_dp_vsc_sdp_log); > + > +/** > + * drm_edp_backlight_set_level() - Set the backlight level of an eDP panel > via AUX > + * @aux: The DP AUX channel to use > + * @bl: Backlight capability info from drm_edp_backlight_init() > + * @level: The brightness level to set > + * > + * Sets the brightness level of an eDP panel's backlight. Note that the > panel's backlight must > + * already have been enabled by the driver by calling > drm_edp_backlight_enable(). > + * > + * Returns: %0 on success, negative error code on failure > + */ > +int drm_edp_backlight_set_level(struct drm_dp_aux *aux, const struct > drm_edp_backlight_info *bl, > + u16 level) I think I'd go for s/backlight/brightness/g function naming thoughout, to account for OLED. "Backlight" unnecessarily underlines the technology. > +{ > + int ret; > + u8 buf[2] = { 0 }; > + > + if (bl->lsb_reg_used) { > + buf[0] = (level & 0xFF00) >> 8; > + buf[1] = (level & 0x00FF); > + } else { > + buf[0] = level; > + } > + > + ret = drm_dp_dpcd_write(aux, DP_EDP_BACKLIGHT_BRIGHTNESS_MSB, buf, > sizeof(buf)); > + if (ret != sizeof(buf)) { > + DRM_ERROR("%s: Failed to write aux backlight level: %d\n", > aux->name, ret); I'd really like to have a way to get from struct drm_dp_aux to struct drm_device to retain the device specific logging here. It'd be useful in the lower level dpcd access functions too. > + return ret < 0 ? ret : -EIO; > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_edp_backlight_set_level); > + > +static int > +drm_edp_backlight_set_enable(struct drm_dp_aux *aux, const struct > drm_edp_backlight_info *bl, > + bool enable) > +{ > + int ret; > + u8 buf; > + > + /* The panel uses something other then DPCD for enabling it's backlight > */ > + if (!bl->aux_enable) > + return 0; > + > + ret = drm_dp_dpcd_readb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, &buf); > + if (ret != 1) { > + DRM_ERROR("%s: Failed to read eDP display control register: > %d\n", aux->name, ret); > + return ret < 0 ? ret : -EIO; > + } > + if (enable) > + buf |= DP_EDP_BACKLIGHT_ENABLE; > + else > + buf &= ~DP_EDP_BACKLIGHT_ENABLE; > + > + ret = drm_dp_dpcd_writeb(aux, DP_EDP_DISPLAY_CONTROL_REGISTER, buf); > + if (ret != 1) { > + DRM_ERROR("%s: Failed to write eDP display control register: > %d\n", aux->name, ret); > + return ret < 0 ? ret : -EIO; > + } > + > + return 0; > +} > + > +/** > + * drm_edp_backlight_enable() - Enable an eDP panel's backlight using DPCD > + * @aux: The DP AUX channel to use > + * @bl: Ba
Re: [PATCH v8, 3/6] soc: mediatek: mmsys: add mt8183 function call for setting the routing registers
Hi, Yongqiang: Yongqiang Niu 於 2020年12月10日 週四 下午5:08寫道: > > add mt8183 function call for setting the routing registers I think you should move this patch to the series "soc: mediatek: Prepare MMSYS for DDP routing using function call" [1]. Without this patch, that series has no strong reason to separate function call and create mmsys folder. And this patch would go to soc tree same as that series, so I think this patch should be moved to that series. [1] https://patchwork.kernel.org/project/linux-mediatek/list/?series=398819 Regards, Chun-Kuang. > > Signed-off-by: Yongqiang Niu > --- > drivers/soc/mediatek/mmsys/Makefile | 1 + > drivers/soc/mediatek/mmsys/mt8183-mmsys.c | 90 > +++ > drivers/soc/mediatek/mmsys/mtk-mmsys.c| 1 + > include/linux/soc/mediatek/mtk-mmsys.h| 1 + > 4 files changed, 93 insertions(+) > create mode 100644 drivers/soc/mediatek/mmsys/mt8183-mmsys.c > > diff --git a/drivers/soc/mediatek/mmsys/Makefile > b/drivers/soc/mediatek/mmsys/Makefile > index ac03025..25eeb9e5 100644 > --- a/drivers/soc/mediatek/mmsys/Makefile > +++ b/drivers/soc/mediatek/mmsys/Makefile > @@ -1,3 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > obj-$(CONFIG_MTK_MMSYS) += mt2701-mmsys.o > +obj-$(CONFIG_MTK_MMSYS) += mt8183-mmsys.o > obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o > diff --git a/drivers/soc/mediatek/mmsys/mt8183-mmsys.c > b/drivers/soc/mediatek/mmsys/mt8183-mmsys.c > new file mode 100644 > index 000..192b4ab > --- /dev/null > +++ b/drivers/soc/mediatek/mmsys/mt8183-mmsys.c > @@ -0,0 +1,90 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// > +// Copyright (c) 2020 MediaTek Inc. > + > +#include > +#include > +#include > +#include > +#include > + > +#define DISP_OVL0_MOUT_EN 0xf00 > +#define DISP_OVL0_2L_MOUT_EN 0xf04 > +#define DISP_OVL1_2L_MOUT_EN 0xf08 > +#define DISP_DITHER0_MOUT_EN 0xf0c > +#define DISP_PATH0_SEL_IN 0xf24 > +#define DISP_DSI0_SEL_IN 0xf2c > +#define DISP_DPI0_SEL_IN 0xf30 > +#define DISP_RDMA0_SOUT_SEL_IN 0xf50 > +#define DISP_RDMA1_SOUT_SEL_IN 0xf54 > + > +#define OVL0_MOUT_EN_OVL0_2L BIT(4) > +#define OVL0_2L_MOUT_EN_DISP_PATH0 BIT(0) > +#define OVL1_2L_MOUT_EN_RDMA1 BIT(4) > +#define DITHER0_MOUT_IN_DSI0 BIT(0) > +#define DISP_PATH0_SEL_IN_OVL0_2L 0x1 > +#define DSI0_SEL_IN_RDMA0 0x1 > +#define DSI0_SEL_IN_RDMA1 0x3 > +#define DPI0_SEL_IN_RDMA0 0x1 > +#define DPI0_SEL_IN_RDMA1 0x2 > +#define RDMA0_SOUT_COLOR0 0x1 > +#define RDMA1_SOUT_DSI00x1 > + > +static unsigned int mtk_mmsys_ddp_mout_en(enum mtk_ddp_comp_id cur, > + enum mtk_ddp_comp_id next, > + unsigned int *addr) > +{ > + unsigned int value; > + > + if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_OVL_2L0) { > + *addr = DISP_OVL0_MOUT_EN; > + value = OVL0_MOUT_EN_OVL0_2L; > + } else if (cur == DDP_COMPONENT_OVL_2L0 && next == > DDP_COMPONENT_RDMA0) { > + *addr = DISP_OVL0_2L_MOUT_EN; > + value = OVL0_2L_MOUT_EN_DISP_PATH0; > + } else if (cur == DDP_COMPONENT_OVL_2L1 && next == > DDP_COMPONENT_RDMA1) { > + *addr = DISP_OVL1_2L_MOUT_EN; > + value = OVL1_2L_MOUT_EN_RDMA1; > + } else if (cur == DDP_COMPONENT_DITHER && next == DDP_COMPONENT_DSI0) > { > + *addr = DISP_DITHER0_MOUT_EN; > + value = DITHER0_MOUT_IN_DSI0; > + } else { > + value = 0; > + } > + > + return value; > +} > + > +static unsigned int mtk_mmsys_ddp_sel_in(enum mtk_ddp_comp_id cur, > +enum mtk_ddp_comp_id next, > +unsigned int *addr) > +{ > + unsigned int value; > + > + if (cur == DDP_COMPONENT_OVL_2L0 && next == DDP_COMPONENT_RDMA0) { > + *addr = DISP_PATH0_SEL_IN; > + value = DISP_PATH0_SEL_IN_OVL0_2L; > + } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) { > + *addr = DISP_DPI0_SEL_IN; > + value = DPI0_SEL_IN_RDMA1; > + } else { > + value = 0; > + } > + > + return value; > +} > + > +static void mtk_mmsys_ddp_sout_sel(void __iomem *config_regs, > + enum mtk_ddp_comp_id cur, > + enum mtk_ddp_comp_id next) > +{ > + if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_COLOR0) { > + writel_relaxed(RDMA0_SOUT_COLOR0, config_regs + > DISP_RDMA0_SOUT_SEL_IN); > + } > +} > + > +struct mtk_mmsys_conn_funcs mt8183_mmsys_funcs = { > + .m
Re: [PATCH v8, 1/6] dt-bindings: mediatek: add rdma_fifo_size description for mt8183 display
Hi, Yongqiang: Yongqiang Niu 於 2020年12月11日 週五 上午8:43寫道: > > On Thu, 2020-12-10 at 23:40 +0800, Chun-Kuang Hu wrote: > > Hi, Yongqiang: > > > > Yongqiang Niu 於 2020年12月10日 週四 下午5:22寫道: > > > > > > rdma fifo size may be different even in same SOC, add this > > > property to the corresponding rdma > > > > > > Signed-off-by: Yongqiang Niu > > > --- > > > .../bindings/display/mediatek/mediatek,disp.txt | 16 > > > > > > 1 file changed, 16 insertions(+) > > > > > > diff --git > > > a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt > > > b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt > > > index 1212207..64c64ee 100644 > > > --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt > > > +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt > > > @@ -66,6 +66,13 @@ Required properties (DMA function blocks): > > >argument, see > > > Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > > >for details. > > > > > > +Optional properties (RDMA function blocks): > > > +- mediatek,rdma_fifo_size: rdma fifo size may be different even in same > > > SOC, add this > > > + property to the corresponding rdma > > > + the value is the Max value which defined in hardware data sheet. > > > + rdma_fifo_size of rdma0 in mt8183 is 5120 > > > + rdma_fifo_size of rdma1 in mt8183 is 2048 > > > + > > > Examples: > > > > > > mmsys: clock-controller@1400 { > > > @@ -207,3 +214,12 @@ od@14023000 { > > > power-domains = <&scpsys MT8173_POWER_DOMAIN_MM>; > > > clocks = <&mmsys CLK_MM_DISP_OD>; > > > }; > > > + > > > +rdma1: rdma@1400c000 { > > > + compatible = "mediatek,mt8183-disp-rdma"; > > > + reg = <0 0x1400c000 0 0x1000>; > > > + interrupts = ; > > > + power-domains = <&scpsys MT8183_POWER_DOMAIN_DISP>; > > > + clocks = <&mmsys CLK_MM_DISP_RDMA1>; > > > + mediatek,rdma_fifo_size = <2048>; > > > +}; > > > > In [1], Rob has suggest that not add example of rdma1, it's better to > > add mediatek,rdma_fifo_size in rdma0 for example. > > > > [1] > > https://patchwork.kernel.org/project/linux-mediatek/patch/1596855231-5782-2-git-send-email-yongqiang@mediatek.com/ > > > > Regards, > > Chun-Kuang. > > the description of rdma0 is mt8173, and mt8173 rdma driver set the > correspond fifo size already ok like this: > static const struct mtk_disp_rdma_data mt8173_rdma_driver_data = { > .fifo_size = SZ_8K, > }; > > please double confirm shall we add this information into rdma0 > description. > Device tree is used to describe hardware. That means device tree description should not consider your driver's implementation. mediatek,rdma-fifo-size of mt8173-rdma0 is 8K, so I could write this information in device node because this hardware is. Regards, Chun-Kuang. > > > > > > -- > > > 1.8.1.1.dirty > > > ___ > > > Linux-mediatek mailing list > > > linux-media...@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] drm: require each CRTC to have a unique primary plane
On Fri, Dec 11, 2020 at 01:08:26PM +, Simon Ser wrote: > User-space expects to be able to pick a primary plane for each CRTC > exposed by the driver. Make sure this assumption holds in > drm_mode_config_validate. > > Use the legacy drm_crtc.primary field to check this, because it's > simpler and we require drivers to set it anyways. Accumulate a set of > primary planes which are already used for a CRTC in a bitmask. Error out > if a primary plane is re-used. > > v2: new patch > > v3: > - Use u64 instead of __u64 (Jani) > - Use `unsigned int` instead of `unsigned` (Jani) > > Signed-off-by: Simon Ser > Cc: Daniel Vetter > Cc: Pekka Paalanen > Cc: Jani Nikula > --- > drivers/gpu/drm/drm_mode_config.c | 21 + > drivers/gpu/drm/drm_plane.c | 6 ++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index fbe680035129..c5cf5624c106 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -626,6 +626,9 @@ void drm_mode_config_validate(struct drm_device *dev) > { > struct drm_encoder *encoder; > struct drm_crtc *crtc; > + struct drm_plane *plane; > + u64 primary_with_crtc = 0, cursor_with_crtc = 0; u32 > + unsigned int num_primary = 0; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return; > @@ -647,12 +650,30 @@ void drm_mode_config_validate(struct drm_device *dev) >"Bogus primary plane possible_crtcs: [PLANE:%d:%s] > must be compatible with [CRTC:%d:%s]\n", >crtc->primary->base.id, crtc->primary->name, >crtc->base.id, crtc->name); > + WARN(primary_with_crtc & BIT(crtc->primary->index), drm_plane_mask() all over > + "Primary plane [PLANE:%d:%s] used for two CRTCs", s/two/multiple/ ? > + crtc->primary->base.id, crtc->primary->name); > + primary_with_crtc |= BIT(crtc->primary->index); > } > if (crtc->cursor) { > WARN(!(crtc->cursor->possible_crtcs & BIT(crtc->index)), >"Bogus cursor plane possible_crtcs: [PLANE:%d:%s] > must be compatible with [CRTC:%d:%s]\n", >crtc->cursor->base.id, crtc->cursor->name, >crtc->base.id, crtc->name); > + WARN(cursor_with_crtc & BIT(crtc->cursor->index), > + "Primary plane [PLANE:%d:%s] used for two CRTCs", s/Primary/Cursor/ > + crtc->cursor->base.id, crtc->cursor->name); > + cursor_with_crtc |= BIT(crtc->cursor->index); > } > } > + > + drm_for_each_plane(plane, dev) { > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { > + num_primary++; > + } > + } > + > + WARN(num_primary != dev->mode_config.num_crtc, > + "Must have as many primary planes as there are CRTCs, but have %u > primary planes and %u CRTCs", > + num_primary, dev->mode_config.num_crtc); > } > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 49b0a8b9ac02..a1f4510efa83 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -54,6 +54,12 @@ > * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see > * &drm_plane.possible_crtcs. > * > + * Each CRTC must have a unique primary plane userspace can attach to enable > + * the CRTC. In other words, userspace must be able to attach a different > + * primary plane to each CRTC at the same time. Primary planes can still be > + * compatible with multiple CRTCs. There must be exactly as many primary > planes > + * as there are CRTCs. > + * > * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM > core > * relies on the driver to set the primary and optionally the cursor plane > used > * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). > All > -- > 2.29.2 > > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC 3/5] drm/i915/dp: Remove redundant AUX backlight frequency calculations
On Wed, 09 Dec 2020, Lyude Paul wrote: > Noticed this while moving all of the VESA backlight code in i915 over to > DRM helpers: it would appear that we calculate the frequency value we want > to write to DP_EDP_BACKLIGHT_FREQ_SET twice even though this value never > actually changes during runtime. So, let's simplify things by just caching > this value in intel_panel.backlight, and re-writing it as-needed. This isn't a full review, just something I spotted so far. Please see inline. BR, Jani. > > Signed-off-by: Lyude Paul > Cc: Jani Nikula > Cc: Dave Airlie > Cc: greg.depo...@gmail.com > --- > .../drm/i915/display/intel_display_types.h| 1 + > .../drm/i915/display/intel_dp_aux_backlight.c | 64 ++- > 2 files changed, 19 insertions(+), 46 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index 5bc5bfbc4551..133c9cb742a7 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -259,6 +259,7 @@ struct intel_panel { > > /* DPCD backlight */ > u8 pwmgen_bit_count; > + u8 pwm_freq_pre_divider; > > struct backlight_device *device; > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > index 4fd536801b14..94ce5ca1affa 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_aux_backlight.c > @@ -129,50 +129,6 @@ intel_dp_aux_set_backlight(const struct > drm_connector_state *conn_state, u32 lev > } > } > > -/* > - * Set PWM Frequency divider to match desired frequency in vbt. > - * The PWM Frequency is calculated as 27Mhz / (F x P). > - * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the > - * EDP_BACKLIGHT_FREQ_SET register (DPCD Address 00728h) > - * - Where P = 2^Pn, where Pn is the value programmed by field 4:0 of the > - * EDP_PWMGEN_BIT_COUNT register (DPCD Address 00724h) > - */ > -static bool intel_dp_aux_set_pwm_freq(struct intel_connector *connector) > -{ > - struct drm_i915_private *dev_priv = to_i915(connector->base.dev); > - struct intel_dp *intel_dp = intel_attached_dp(connector); > - const u8 pn = connector->panel.backlight.pwmgen_bit_count; > - int freq, fxp, f, fxp_actual, fxp_min, fxp_max; > - > - freq = dev_priv->vbt.backlight.pwm_freq_hz; > - if (!freq) { > - drm_dbg_kms(&dev_priv->drm, > - "Use panel default backlight frequency\n"); > - return false; > - } > - > - fxp = DIV_ROUND_CLOSEST(KHz(DP_EDP_BACKLIGHT_FREQ_BASE_KHZ), freq); > - f = clamp(DIV_ROUND_CLOSEST(fxp, 1 << pn), 1, 255); > - fxp_actual = f << pn; > - > - /* Ensure frequency is within 25% of desired value */ > - fxp_min = DIV_ROUND_CLOSEST(fxp * 3, 4); > - fxp_max = DIV_ROUND_CLOSEST(fxp * 5, 4); > - > - if (fxp_min > fxp_actual || fxp_actual > fxp_max) { > - drm_dbg_kms(&dev_priv->drm, "Actual frequency out of range\n"); > - return false; > - } > - > - if (drm_dp_dpcd_writeb(&intel_dp->aux, > -DP_EDP_BACKLIGHT_FREQ_SET, (u8) f) < 0) { > - drm_dbg_kms(&dev_priv->drm, > - "Failed to write aux backlight freq\n"); > - return false; > - } > - return true; > -} > - > static void intel_dp_aux_enable_backlight(const struct intel_crtc_state > *crtc_state, > const struct drm_connector_state > *conn_state) > { > @@ -213,9 +169,13 @@ static void intel_dp_aux_enable_backlight(const struct > intel_crtc_state *crtc_st > break; > } > > - if (intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_FREQ_AUX_SET_CAP) > - if (intel_dp_aux_set_pwm_freq(connector)) > + if (panel->backlight.pwm_freq_pre_divider) { > + if (drm_dp_dpcd_writeb(&intel_dp->aux, > DP_EDP_BACKLIGHT_FREQ_SET, > +panel->backlight.pwm_freq_pre_divider) > == 1) > new_dpcd_buf |= DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE; > + else > + drm_dbg_kms(&i915->drm, "Failed to write aux backlight > frequency\n"); > + } > > if (new_dpcd_buf != dpcd_buf) { > if (drm_dp_dpcd_writeb(&intel_dp->aux, > @@ -236,6 +196,14 @@ static void intel_dp_aux_disable_backlight(const struct > drm_connector_state *old >false); > } > > +/* > + * Compute PWM frequency divider value based off the frequency provided to > us by the vbt. > + * The PWM Frequency is calculated as 27Mhz / (F x P). > + * - Where F = PWM Frequency Pre-Divider value programmed by field 7:0 of the > + * EDP_BACKLIGHT_FREQ_SET register (DPCD
Re: [PATCH v2 2/2] drm: automatic legacy gamma support
On Fri, Dec 11, 2020 at 01:24:49PM +0200, Tomi Valkeinen wrote: > On 10/12/2020 20:06, kernel test robot wrote: > > Hi Tomi, > > > > I love your patch! Perhaps something to improve: > > > > [auto build test WARNING on drm-intel/for-linux-next] > > [also build test WARNING on linus/master v5.10-rc7] > > [cannot apply to drm-tip/drm-tip anholt/for-next next-20201210] > > [If your patch is applied to the wrong git tree, kindly drop us a note. > > And when submitting patch, we suggest to use '--base' as documented in > > https://git-scm.com/docs/git-format-patch] > > > > url: > > https://github.com/0day-ci/linux/commits/Tomi-Valkeinen/drm-fix-and-cleanup-legacy-gamma-support/20201208-215917 > > base: git://anongit.freedesktop.org/drm-intel for-linux-next > > config: i386-randconfig-m021-20201209 (attached as .config) > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 > > > > If you fix the issue, kindly add following tag as appropriate > > Reported-by: kernel test robot > > > > New smatch warnings: > > drivers/gpu/drm/drm_color_mgmt.c:307 drm_crtc_legacy_gamma_set() error: > > potential null dereference 'blob'. (drm_property_create_blob returns null) > > I don't see how this could happen. There's no code path I see where > drm_property_create_blob could > return null... IIRC we've received multiple similar nonsense reports from lkp, but no explanation why it thinks it could ever be null. Hmm, maybe there is a codepath somewhere that has a null check on the return value? -- Ville Syrjälä Intel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] drm: require each CRTC to have a unique primary plane
On Friday, December 11th, 2020 at 2:50 PM, Pekka Paalanen wrote: > is there a reason why one cannot have more primary planes than CRTCs in > existence? > > Daniel implied that in <20201209003637.GK401619@phenom.ffwll.local>, > but I didn't get the reason for it yet. > > E.g. if all your planes are interchangeable in the sense that you can > turn on a CRTC with any one of them, would one not then expose all the > planes as "Primary"? I'm thinking of primary as a hint for simple user-space: "you can likely light up a CRTC if you attach this plane and don't do anything crazy". For anything more complicated, user-space uses atomic commits and can completely ignore whether a plane is primary, cursor or overlay. > If the planes have other differences, like supported formats or > scaling, then marking them all "Primary" would let userspace know that > it can pick any plane with the suitable properties and expect to turn > on the CRTC with it. That's interesting, but I'd bet no user-space does that. If new user-space wants to, it's better to rely on test-only commits instead. > Or does marking a plane as "Primary" imply something else too, like > "cannot scale"? I think Weston does make this assumption in an attempt > to hit fewer causes for failure. No, AFAIK "Primary" doesn't imply something else, e.g. on amdgpu you can do scaling on the primary plane. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [patch 27/30] xen/events: Only force affinity mask for percpu interrupts
On 11.12.20 11:13, Thomas Gleixner wrote: On Fri, Dec 11 2020 at 07:17, Jürgen Groß wrote: On 11.12.20 00:20, boris.ostrov...@oracle.com wrote: On 12/10/20 2:26 PM, Thomas Gleixner wrote: All event channel setups bind the interrupt on CPU0 or the target CPU for percpu interrupts and overwrite the affinity mask with the corresponding cpumask. That does not make sense. The XEN implementation of irqchip::irq_set_affinity() already picks a single target CPU out of the affinity mask and the actual target is stored in the effective CPU mask, so destroying the user chosen affinity mask which might contain more than one CPU is wrong. Change the implementation so that the channel is bound to CPU0 at the XEN level and leave the affinity mask alone. At startup of the interrupt affinity will be assigned out of the affinity mask and the XEN binding will be updated. If that's the case then I wonder whether we need this call at all and instead bind at startup time. This binding to cpu0 was introduced with commit 97253eeeb792d61ed2 and I have no reason to believe the underlying problem has been eliminated. "The kernel-side VCPU binding was not being correctly set for newly allocated or bound interdomain events. In ARM guests where 2-level events were used, this would result in no interdomain events being handled because the kernel-side VCPU masks would all be clear. x86 guests would work because the irq affinity was set during irq setup and this would set the correct kernel-side VCPU binding." I'm not convinced that this is really correctly analyzed because affinity setting is done at irq startup. switch (__irq_startup_managed(desc, aff, force)) { case IRQ_STARTUP_NORMAL: ret = __irq_startup(desc); irq_setup_affinity(desc); break; which is completely architecture agnostic. So why should this magically work on x86 and not on ARM if both are using the same XEN irqchip with the same irqchip callbacks. I think this might be related to _initial_ cpu binding of events and changing the binding later. This might be handled differently in the hypervisor. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/8] dma-buf: Add vmap_local and vnumap_local operations
On Wed, Dec 09, 2020 at 03:25:22PM +0100, Thomas Zimmermann wrote: > The existing dma-buf calls dma_buf_vmap() and dma_buf_vunmap() are > allowed to pin the buffer or acquire the buffer's reservation object > lock. > > This is a problem for callers that only require a short-term mapping > of the buffer without the pinning, or callers that have special locking > requirements. These may suffer from unnecessary overhead or interfere > with regular pin operations. > > The new interfaces dma_buf_vmap_local(), dma_buf_vunmapo_local(), and > their rsp callbacks in struct dma_buf_ops provide an alternative without > pinning or reservation locking. Callers are responsible for these > operations. > > Signed-off-by: Thomas Zimmermann > --- > drivers/dma-buf/dma-buf.c | 80 +++ > include/linux/dma-buf.h | 34 + > 2 files changed, 114 insertions(+) > > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > index e63684d4cd90..be9f80190a66 100644 > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c > @@ -1265,6 +1265,86 @@ void dma_buf_vunmap(struct dma_buf *dmabuf, struct > dma_buf_map *map) > } > EXPORT_SYMBOL_GPL(dma_buf_vunmap); > > +/** > + * dma_buf_vmap_local - Create virtual mapping for the buffer object into > kernel > + * address space. > + * @dmabuf: [in]buffer to vmap > + * @map: [out] returns the vmap pointer > + * > + * This call may fail due to lack of virtual mapping address space. > + * These calls are optional in drivers. The intended use for them > + * is for mapping objects linear in kernel space for high use objects. > + * Please attempt to use kmap/kunmap before thinking about these interfaces. We also need to specify whether callers need to call dma_buf_begin/end_cpu access around these or not. For current implementations it doesn't matter, but if you want to convert udl/gm12u320, it will. I think requiring an explicit call would be good, for more consistency with how normal vmap works. -Daniel > + * > + * Returns: > + * 0 on success, or a negative errno code otherwise. > + */ > +int dma_buf_vmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map) > +{ > + struct dma_buf_map ptr; > + int ret = 0; > + > + dma_buf_map_clear(map); > + > + if (WARN_ON(!dmabuf)) > + return -EINVAL; > + > + dma_resv_assert_held(dmabuf->resv); > + > + if (!dmabuf->ops->vmap_local) > + return -EINVAL; > + > + mutex_lock(&dmabuf->lock); > + if (dmabuf->vmapping_counter) { > + dmabuf->vmapping_counter++; > + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); > + *map = dmabuf->vmap_ptr; > + goto out_unlock; > + } > + > + BUG_ON(dma_buf_map_is_set(&dmabuf->vmap_ptr)); > + > + ret = dmabuf->ops->vmap_local(dmabuf, &ptr); > + if (WARN_ON_ONCE(ret)) > + goto out_unlock; > + > + dmabuf->vmap_ptr = ptr; > + dmabuf->vmapping_counter = 1; > + > + *map = dmabuf->vmap_ptr; > + > +out_unlock: > + mutex_unlock(&dmabuf->lock); > + return ret; > +} > +EXPORT_SYMBOL_GPL(dma_buf_vmap_local); > + > +/** > + * dma_buf_vunmap_local - Unmap a vmap obtained by dma_buf_vmap_local. > + * @dmabuf: [in]buffer to vunmap > + * @map: [in]vmap pointer to vunmap > + */ > +void dma_buf_vunmap_local(struct dma_buf *dmabuf, struct dma_buf_map *map) > +{ > + if (WARN_ON(!dmabuf)) > + return; > + > + dma_resv_assert_held(dmabuf->resv); > + > + BUG_ON(dma_buf_map_is_null(&dmabuf->vmap_ptr)); > + BUG_ON(dmabuf->vmapping_counter == 0); > + BUG_ON(!dma_buf_map_is_equal(&dmabuf->vmap_ptr, map)); > + > + mutex_lock(&dmabuf->lock); > + if (--dmabuf->vmapping_counter == 0) { > + if (dmabuf->ops->vunmap_local) > + dmabuf->ops->vunmap_local(dmabuf, map); > + dma_buf_map_clear(&dmabuf->vmap_ptr); > + } > + mutex_unlock(&dmabuf->lock); > +} > +EXPORT_SYMBOL_GPL(dma_buf_vunmap_local); > + > #ifdef CONFIG_DEBUG_FS > static int dma_buf_debug_show(struct seq_file *s, void *unused) > { > diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h > index cf72699cb2bc..f66580d23a9b 100644 > --- a/include/linux/dma-buf.h > +++ b/include/linux/dma-buf.h > @@ -269,6 +269,38 @@ struct dma_buf_ops { > > int (*vmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > void (*vunmap)(struct dma_buf *dmabuf, struct dma_buf_map *map); > + > + /** > + * @vmap_local: > + * > + * Creates a virtual mapping for the buffer into kernel address space. > + * > + * This callback establishes short-term mappings for situations where > + * callers only use the buffer for a bounded amount of time; such as > + * updates to the framebuffer or reading back contained information. > + * In contrast to the regular @vmap callback, vmap_local does never pin > + * the buffer to a s
Re: [PATCH] drm/panel: khadas: Fix error code in khadas_ts050_panel_add()
On Fri, Dec 11, 2020 at 12:01:57PM +0100, Sam Ravnborg wrote: > Hi Dan, > > I assume your nice tooling found this buggy. Yeah. Passing a valid pointer to PTR_ERR(). regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/mediatek: mtk_dpi: Create connector for bridges
Hi, Hsin-Yi: Hsin-Yi Wang 於 2020年12月3日 週四 下午4:24寫道: > > Similar to a9d9fea74be2 ("drm/mediatek: mtk_dsi: Create connector for > bridges"): > > Use the drm_bridge_connector helper to create a connector for pipelines > that use drm_bridge. This allows splitting connector operations across > multiple bridges when necessary, instead of having the last bridge in > the chain creating the connector and handling all connector operations > internally. Reviewed-by: Chun-Kuang Hu > > Signed-off-by: Hsin-Yi Wang > --- > drivers/gpu/drm/mediatek/mtk_dpi.c | 13 - > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c > b/drivers/gpu/drm/mediatek/mtk_dpi.c > index 52f11a63a330..189377e342fa 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c > @@ -20,6 +20,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -66,6 +67,7 @@ struct mtk_dpi { > struct drm_encoder encoder; > struct drm_bridge bridge; > struct drm_bridge *next_bridge; > + struct drm_connector *connector; > void __iomem *regs; > struct device *dev; > struct clk *engine_clk; > @@ -603,12 +605,21 @@ static int mtk_dpi_bind(struct device *dev, struct > device *master, void *data) > > dpi->encoder.possible_crtcs = > mtk_drm_find_possible_crtc_by_comp(drm_dev, dpi->ddp_comp); > > - ret = drm_bridge_attach(&dpi->encoder, &dpi->bridge, NULL, 0); > + ret = drm_bridge_attach(&dpi->encoder, &dpi->bridge, NULL, > + DRM_BRIDGE_ATTACH_NO_CONNECTOR); > if (ret) { > dev_err(dev, "Failed to attach bridge: %d\n", ret); > goto err_cleanup; > } > > + dpi->connector = drm_bridge_connector_init(drm_dev, &dpi->encoder); > + if (IS_ERR(dpi->connector)) { > + dev_err(dev, "Unable to create bridge connector\n"); > + ret = PTR_ERR(dpi->connector); > + goto err_cleanup; > + } > + drm_connector_attach_encoder(dpi->connector, &dpi->encoder); > + > dpi->bit_num = MTK_DPI_OUT_BIT_NUM_8BITS; > dpi->channel_swap = MTK_DPI_OUT_CHANNEL_SWAP_RGB; > dpi->yc_map = MTK_DPI_OUT_YC_MAP_RGB; > -- > 2.29.2.576.ga3fc446d84-goog > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
Am 11.12.20 um 14:58 schrieb Michel Dänzer: On 2020-12-14 9:57 p.m., Christian König wrote: Am 11.12.20 um 13:20 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: I think the general idea we settled on is that we specify an earliest display time for each frame and give feedback to the application when a frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. X11 already supports that with the DRI3 extension. Also see VDPAU present and the Vulkan extension for reference application API implementations, so that stuff is already present. I suspect you mean the Present extension, specifically PresentOptionUST. There is no working implementation of this yet (the xserver tree has never had any code which would even advertise PresentCapabilityUST, let alone do anything with a timestamp passed in by the client). Good point, what I wanted to say is that this is already specified in those extensions. What we are missing is somehow wiring it all together and see how it works. Christian. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 8/8] drm/fb-helper: Move BO locking from DRM client to fbdev damage worker
On Fri, Dec 11, 2020 at 11:16:25AM +0100, Thomas Zimmermann wrote: > Hi > > Am 11.12.20 um 11:01 schrieb Daniel Vetter: > > On Wed, Dec 09, 2020 at 03:25:27PM +0100, Thomas Zimmermann wrote: > > > Fbdev emulation has to lock the BO into place while flushing the shadow > > > buffer into the BO's memory. Remove any interference with pinning by > > > using vmap_local functionality (instead of full vmap). This requires > > > BO reservation locking in fbdev's damage worker. > > > > > > The new DRM client functions for locking and vmap_local functionality > > > are added for consistency with the existing style. > > > > > > Signed-off-by: Thomas Zimmermann > > > > The old vmap/vunmap in the client library aren't used anymore, please > > delete. That will also make it clearer in the diff what's going on and > > that it makes sense to have the client and fb-helper part in one patch. > > They are still around for perma-mapped BOs where HW supports it (really only > CMA-based drivers). See drm_fb_helper_generic_probe() and > drm_fbdev_cleanup(). Ah right I didn't grep this carefully enough. I guess in that case splitting this into the drm_client patch and fb-helper patch would be good I think. Also some kerneldoc commment addition for normal vmap that vmap_local is preferred for short-term mappings, and for vmap_local that _vmap() gives you the long term mapping. Just for more links and stuff in docs. With that: Reviewed-by: Daniel Vetter -Daniel > > Best regards > Thomas > > > > > With that: Reviewed-by: Daniel Vetter > > > > > --- > > > drivers/gpu/drm/drm_client.c| 91 + > > > drivers/gpu/drm/drm_fb_helper.c | 41 +++ > > > include/drm/drm_client.h| 4 ++ > > > 3 files changed, 116 insertions(+), 20 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > > > index ce45e380f4a2..795f5cb052ba 100644 > > > --- a/drivers/gpu/drm/drm_client.c > > > +++ b/drivers/gpu/drm/drm_client.c > > > @@ -288,6 +288,37 @@ drm_client_buffer_create(struct drm_client_dev > > > *client, u32 width, u32 height, u > > > return ERR_PTR(ret); > > > } > > > +/** > > > + * drm_client_buffer_lock - Locks the DRM client buffer > > > + * @buffer: DRM client buffer > > > + * > > > + * This function locks the client buffer by acquiring the buffer > > > + * object's reservation lock. > > > + * > > > + * Unlock the buffer with drm_client_buffer_unlock(). > > > + * > > > + * Returns: > > > + * 0 on success, or a negative errno code otherwise. > > > + */ > > > +int > > > +drm_client_buffer_lock(struct drm_client_buffer *buffer) > > > +{ > > > + return dma_resv_lock(buffer->gem->resv, NULL); > > > +} > > > +EXPORT_SYMBOL(drm_client_buffer_lock); > > > + > > > +/** > > > + * drm_client_buffer_unlock - Unlock DRM client buffer > > > + * @buffer: DRM client buffer > > > + * > > > + * Unlocks a client buffer. See drm_client_buffer_lock(). > > > + */ > > > +void drm_client_buffer_unlock(struct drm_client_buffer *buffer) > > > +{ > > > + dma_resv_unlock(buffer->gem->resv); > > > +} > > > +EXPORT_SYMBOL(drm_client_buffer_unlock); > > > + > > > /** > > >* drm_client_buffer_vmap - Map DRM client buffer into address space > > >* @buffer: DRM client buffer > > > @@ -348,6 +379,66 @@ void drm_client_buffer_vunmap(struct > > > drm_client_buffer *buffer) > > > } > > > EXPORT_SYMBOL(drm_client_buffer_vunmap); > > > +/** > > > + * drm_client_buffer_vmap_local - Map DRM client buffer into address > > > space > > > + * @buffer: DRM client buffer > > > + * @map_copy: Returns the mapped memory's address > > > + * > > > + * This function maps a client buffer into kernel address space. If the > > > + * buffer is already mapped, it returns the existing mapping's address. > > > + * > > > + * Client buffer mappings are not ref'counted. Each call to > > > + * drm_client_buffer_vmap_local() should be followed by a call to > > > + * drm_client_buffer_vunmap_local(); or the client buffer should be > > > mapped > > > + * throughout its lifetime. > > > + * > > > + * The returned address is a copy of the internal value. In contrast to > > > + * other vmap interfaces, you don't need it for the client's vunmap > > > + * function. So you can modify it at will during blit and draw > > > operations. > > > + * > > > + * Returns: > > > + * 0 on success, or a negative errno code otherwise. > > > + */ > > > +int > > > +drm_client_buffer_vmap_local(struct drm_client_buffer *buffer, struct > > > dma_buf_map *map_copy) > > > +{ > > > + struct dma_buf_map *map = &buffer->map; > > > + int ret; > > > + > > > + /* > > > + * FIXME: The dependency on GEM here isn't required, we could > > > + * convert the driver handle to a dma-buf instead and use the > > > + * backend-agnostic dma-buf vmap_local support instead. This would > > > + * require that the handle2fd prime ioctl is reworked to pull the > > > + * fd_install step out of the
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
On 2020-12-14 9:57 p.m., Christian König wrote: Am 11.12.20 um 13:20 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: I think the general idea we settled on is that we specify an earliest display time for each frame and give feedback to the application when a frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. X11 already supports that with the DRI3 extension. Also see VDPAU present and the Vulkan extension for reference application API implementations, so that stuff is already present. I suspect you mean the Present extension, specifically PresentOptionUST. There is no working implementation of this yet (the xserver tree has never had any code which would even advertise PresentCapabilityUST, let alone do anything with a timestamp passed in by the client). -- Earthling Michel Dänzer | https://redhat.com Libre software enthusiast | Mesa and X developer ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 19/19] drm/i915/hdcp: Enable HDCP 2.2 MST support
Enable HDCP 2.2 MST support till Gen12. Cc: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_hdcp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 768a6218b9c4..20c8d8f63566 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -2233,7 +2233,7 @@ int intel_hdcp_init(struct intel_connector *connector, if (!shim) return -EINVAL; - if (is_hdcp2_supported(dev_priv) && !connector->mst_port) + if (is_hdcp2_supported(dev_priv)) intel_hdcp2_init(connector, dig_port, shim); ret = -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 18/19] drm/i915/hdcp: Configure HDCP2.2 MST steram encryption status
Authenticate and enable port encryption only once for an active HDCP 2.2 session, once port is authenticated and encrypted enable encryption for each stream that requires encryption on this port. Similarly disable the stream encryption for each encrypted stream, once all encrypted stream encryption is disabled, disable the port HDCP encryption and deauthenticate the port. v2: - Add connector details in drm_err. [Ram] - 's/port_auth/hdcp_auth_status'. [Ram] - Added a debug print for stream enc. v3: - uniformity for connector detail in DMESG. [Ram] Cc: Ramalingam C Reviewed-by: Uma Shankar Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_hdcp.c | 51 ++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 2fd8b0453b1d..768a6218b9c4 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -1700,6 +1700,36 @@ static int hdcp2_authenticate_sink(struct intel_connector *connector) return ret; } +static int hdcp2_enable_stream_encryption(struct intel_connector *connector) +{ + struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct drm_i915_private *dev_priv = to_i915(connector->base.dev); + struct intel_hdcp *hdcp = &connector->hdcp; + enum transcoder cpu_transcoder = hdcp->cpu_transcoder; + enum port port = dig_port->base.port; + int ret = 0; + + if (!(intel_de_read(dev_priv, HDCP2_STATUS(dev_priv, cpu_transcoder, port)) & + LINK_ENCRYPTION_STATUS)) { + drm_err(&dev_priv->drm, "[%s:%d] HDCP 2.2 Link is not encrypted\n", + connector->base.name, connector->base.base.id); + return -EPERM; + } + + if (hdcp->shim->stream_2_2_encryption) { + ret = hdcp->shim->stream_2_2_encryption(connector, true); + if (ret) { + drm_err(&dev_priv->drm, "[%s:%d] Failed to enable HDCP 2.2 stream enc\n", + connector->base.name, connector->base.base.id); + return ret; + } + drm_dbg_kms(&dev_priv->drm, "HDCP 2.2 transcoder: %s stream encrypted\n", + transcoder_name(hdcp->stream_transcoder)); + } + + return ret; +} + static int hdcp2_enable_encryption(struct intel_connector *connector) { struct intel_digital_port *dig_port = intel_attached_dig_port(connector); @@ -1838,7 +1868,7 @@ static int hdcp2_authenticate_and_encrypt(struct intel_connector *connector) drm_dbg_kms(&i915->drm, "Port deauth failed.\n"); } - if (!ret) { + if (!ret && !dig_port->hdcp_auth_status) { /* * Ensuring the required 200mSec min time interval between * Session Key Exchange and encryption. @@ -1853,6 +1883,8 @@ static int hdcp2_authenticate_and_encrypt(struct intel_connector *connector) } } + ret = hdcp2_enable_stream_encryption(connector); + return ret; } @@ -1898,11 +1930,26 @@ static int _intel_hdcp2_disable(struct intel_connector *connector) struct intel_digital_port *dig_port = intel_attached_dig_port(connector); struct drm_i915_private *i915 = to_i915(connector->base.dev); struct hdcp_port_data *data = &dig_port->hdcp_port_data; + struct intel_hdcp *hdcp = &connector->hdcp; int ret; drm_dbg_kms(&i915->drm, "[%s:%d] HDCP2.2 is being Disabled\n", connector->base.name, connector->base.base.id); + if (hdcp->shim->stream_2_2_encryption) { + ret = hdcp->shim->stream_2_2_encryption(connector, false); + if (ret) { + drm_err(&i915->drm, "[%s:%d] Failed to disable HDCP 2.2 stream enc\n", + connector->base.name, connector->base.base.id); + return ret; + } + drm_dbg_kms(&i915->drm, "HDCP 2.2 transcoder: %s stream encryption disabled\n", + transcoder_name(hdcp->stream_transcoder)); + } + + if (dig_port->num_hdcp_streams > 0) + return ret; + ret = hdcp2_disable_encryption(connector); if (hdcp2_deauthenticate_port(connector) < 0) @@ -1926,6 +1973,7 @@ static int intel_hdcp2_check_link(struct intel_connector *connector) int ret = 0; mutex_lock(&hdcp->mutex); + mutex_lock(&dig_port->hdcp_mutex); cpu_transcoder = hdcp->cpu_transcoder; /* hdcp2_check_link is expected only when HDCP2.2 is Enabled */ @@ -2003,6 +2051,7 @@ static int intel_hdcp2_check_link(struct intel_connector *connector) } out: + mutex_unlock(&dig_port->hdcp_mutex);
[PATCH v8 17/19] drm/i915/hdcp: Support for HDCP 2.2 MST shim callbacks
Add support for HDCP 2.2 DP MST shim callback. This adds existing DP HDCP shim callback for Link Authentication and Encryption and HDCP 2.2 stream encryption callback. v2: - Added a WARN_ON() instead of drm_err. [Uma] - Cosmetic changes. [Uma] v3: - 's/port_data/hdcp_port_data' [Ram] - skip redundant link check. [Ram] v4: - use pipe instead of port to access HDCP2_STREAM_STATUS Cc: Ramalingam C Reviewed-by: Uma Shankar Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- .../drm/i915/display/intel_display_types.h| 4 + drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 89 +-- 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 63de25b40eff..da91e3f4ff27 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -378,6 +378,10 @@ struct intel_hdcp_shim { int (*config_stream_type)(struct intel_digital_port *dig_port, bool is_repeater, u8 type); + /* Enable/Disable HDCP 2.2 stream encryption on DP MST Transport Link */ + int (*stream_2_2_encryption)(struct intel_connector *connector, +bool enable); + /* HDCP2.2 Link Integrity Check */ int (*check_2_2_link)(struct intel_digital_port *dig_port, struct intel_connector *connector); diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c index 9ade1ad3a80c..f372e25edab4 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c @@ -698,18 +698,14 @@ intel_dp_mst_hdcp_stream_encryption(struct intel_connector *connector, return 0; } -static -bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port, - struct intel_connector *connector) +static bool intel_dp_mst_get_qses_status(struct intel_digital_port *dig_port, +struct intel_connector *connector) { struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); - struct intel_dp *intel_dp = &dig_port->dp; struct drm_dp_query_stream_enc_status_ack_reply reply; + struct intel_dp *intel_dp = &dig_port->dp; int ret; - if (!intel_dp_hdcp_check_link(dig_port, connector)) - return false; - ret = drm_dp_send_query_stream_enc_status(&intel_dp->mst_mgr, connector->port, &reply); if (ret) { @@ -726,6 +722,78 @@ bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port, return reply.auth_completed && reply.encryption_enabled; } +static +bool intel_dp_mst_hdcp_check_link(struct intel_digital_port *dig_port, + struct intel_connector *connector) +{ + if (!intel_dp_hdcp_check_link(dig_port, connector)) + return false; + + return intel_dp_mst_get_qses_status(dig_port, connector); +} + +static int +intel_dp_mst_hdcp2_stream_encryption(struct intel_connector *connector, +bool enable) +{ + struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct drm_i915_private *i915 = to_i915(connector->base.dev); + struct hdcp_port_data *data = &dig_port->hdcp_port_data; + struct intel_hdcp *hdcp = &connector->hdcp; + enum transcoder cpu_transcoder = hdcp->stream_transcoder; + enum pipe pipe = (enum pipe)cpu_transcoder; + enum port port = dig_port->base.port; + int ret; + + drm_WARN_ON(&i915->drm, enable && + !!(intel_de_read(i915, HDCP2_AUTH_STREAM(i915, cpu_transcoder, port)) + & AUTH_STREAM_TYPE) != data->streams[0].stream_type); + + ret = intel_dp_mst_toggle_hdcp_stream_select(connector, enable); + if (ret) + return ret; + + /* Wait for encryption confirmation */ + if (intel_de_wait_for_register(i915, + HDCP2_STREAM_STATUS(i915, cpu_transcoder, pipe), + STREAM_ENCRYPTION_STATUS, + enable ? STREAM_ENCRYPTION_STATUS : 0, + HDCP_ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) { + drm_err(&i915->drm, "Timed out waiting for transcoder: %s stream encryption %s\n", + transcoder_name(cpu_transcoder), enable ? "enabled" : "disabled"); + return -ETIMEDOUT; + } + + return 0; +} + +/* + * DP v2.0 I.3.3 ignore the stream signature L' in QSES reply msg reply. + * I.3.5 MST source device may use a QSES msg to query downstream status + * for a particular stream. + */ +static +int intel_dp_mst_hdcp2_check_link(struct intel_digita
[PATCH v8 16/19] drm/i915/hdcp: Add HDCP 2.2 stream register
Add HDCP 2.2 DP MST HDCP2_STREAM_STATUS and HDCP2_AUTH_STREAM register in i915_reg header. B.Spec: 21780 B.Spec: 14410 B.Spec: 50573 v2 - Modified naming convention of HDCP2_STREAM_STATUS for pre-gen12 platforms inline with B.Spec. Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/i915_reg.h | 39 + 1 file changed, 39 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b448e507d41e..cade0a7a97b2 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -9873,6 +9873,7 @@ enum skl_power_gate { _PORTD_HDCP2_BASE, \ _PORTE_HDCP2_BASE, \ _PORTF_HDCP2_BASE) + (x)) + #define PORT_HDCP2_AUTH(port) _PORT_HDCP2_BASE(port, 0x98) #define _TRANSA_HDCP2_AUTH 0x66498 #define _TRANSB_HDCP2_AUTH 0x66598 @@ -9912,6 +9913,44 @@ enum skl_power_gate { TRANS_HDCP2_STATUS(trans) : \ PORT_HDCP2_STATUS(port)) +#define _PIPEA_HDCP2_STREAM_STATUS 0x668C0 +#define _PIPEB_HDCP2_STREAM_STATUS 0x665C0 +#define _PIPEC_HDCP2_STREAM_STATUS 0x666C0 +#define _PIPED_HDCP2_STREAM_STATUS 0x667C0 +#define PIPE_HDCP2_STREAM_STATUS(pipe) _MMIO(_PICK((pipe), \ + _PIPEA_HDCP2_STREAM_STATUS, \ + _PIPEB_HDCP2_STREAM_STATUS, \ + _PIPEC_HDCP2_STREAM_STATUS, \ + _PIPED_HDCP2_STREAM_STATUS)) + +#define _TRANSA_HDCP2_STREAM_STATUS0x664C0 +#define _TRANSB_HDCP2_STREAM_STATUS0x665C0 +#define TRANS_HDCP2_STREAM_STATUS(trans) _MMIO_TRANS(trans, \ + _TRANSA_HDCP2_STREAM_STATUS, \ + _TRANSB_HDCP2_STREAM_STATUS) +#define STREAM_ENCRYPTION_STATUS BIT(31) +#define STREAM_TYPE_STATUS BIT(30) +#define HDCP2_STREAM_STATUS(dev_priv, trans, port) \ + (INTEL_GEN(dev_priv) >= 12 ? \ +TRANS_HDCP2_STREAM_STATUS(trans) : \ +PIPE_HDCP2_STREAM_STATUS(pipe)) + +#define _PORTA_HDCP2_AUTH_STREAM 0x66F00 +#define _PORTB_HDCP2_AUTH_STREAM 0x66F04 +#define PORT_HDCP2_AUTH_STREAM(port) _MMIO_PORT(port, \ + _PORTA_HDCP2_AUTH_STREAM, \ + _PORTB_HDCP2_AUTH_STREAM) +#define _TRANSA_HDCP2_AUTH_STREAM 0x66F00 +#define _TRANSB_HDCP2_AUTH_STREAM 0x66F04 +#define TRANS_HDCP2_AUTH_STREAM(trans) _MMIO_TRANS(trans, \ + _TRANSA_HDCP2_AUTH_STREAM, \ + _TRANSB_HDCP2_AUTH_STREAM) +#define AUTH_STREAM_TYPE BIT(31) +#define HDCP2_AUTH_STREAM(dev_priv, trans, port) \ + (INTEL_GEN(dev_priv) >= 12 ? \ +TRANS_HDCP2_AUTH_STREAM(trans) : \ +PORT_HDCP2_AUTH_STREAM(port)) + /* Per-pipe DDI Function Control */ #define _TRANS_DDI_FUNC_CTL_A 0x60400 #define _TRANS_DDI_FUNC_CTL_B 0x61400 -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 15/19] drm/i915/hdcp: Pass connector to check_2_2_link
This requires for HDCP 2.2 MST check link. As for DP/HDMI shims check_2_2_link retrieves the connector from dig_port, this is not sufficient or DP MST connector, there can be multiple DP MST topology connector associated with same dig_port. Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_display_types.h | 3 ++- drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 3 ++- drivers/gpu/drm/i915/display/intel_hdcp.c | 2 +- drivers/gpu/drm/i915/display/intel_hdmi.c | 3 ++- 4 files changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index b37a02a73de6..63de25b40eff 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -379,7 +379,8 @@ struct intel_hdcp_shim { bool is_repeater, u8 type); /* HDCP2.2 Link Integrity Check */ - int (*check_2_2_link)(struct intel_digital_port *dig_port); + int (*check_2_2_link)(struct intel_digital_port *dig_port, + struct intel_connector *connector); }; struct intel_hdcp { diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c index 3f23f8b53dcd..9ade1ad3a80c 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c @@ -585,7 +585,8 @@ int intel_dp_hdcp2_config_stream_type(struct intel_digital_port *dig_port, } static -int intel_dp_hdcp2_check_link(struct intel_digital_port *dig_port) +int intel_dp_hdcp2_check_link(struct intel_digital_port *dig_port, + struct intel_connector *connector) { u8 rx_status; int ret; diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index e26a63f0c189..2fd8b0453b1d 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -1947,7 +1947,7 @@ static int intel_hdcp2_check_link(struct intel_connector *connector) goto out; } - ret = hdcp->shim->check_2_2_link(dig_port); + ret = hdcp->shim->check_2_2_link(dig_port, connector); if (ret == HDCP_LINK_PROTECTED) { if (hdcp->value != DRM_MODE_CONTENT_PROTECTION_UNDESIRED) { intel_hdcp_update_value(connector, diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 25d76460f8f9..977e6b6c35c7 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1733,7 +1733,8 @@ int intel_hdmi_hdcp2_read_msg(struct intel_digital_port *dig_port, } static -int intel_hdmi_hdcp2_check_link(struct intel_digital_port *dig_port) +int intel_hdmi_hdcp2_check_link(struct intel_digital_port *dig_port, + struct intel_connector *connector) { u8 rx_status[HDCP_2_2_HDMI_RXSTATUS_LEN]; int ret; -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 13/19] drm/hdcp: Max MST content streams
Let's define Maximum MST content streams up to four generically which can be supported by modern display controllers. Cc: Sean Paul Cc: Ramalingam C Acked-by: Maarten Lankhorst Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- include/drm/drm_hdcp.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/drm/drm_hdcp.h b/include/drm/drm_hdcp.h index fe58dbb46962..ac22c246542a 100644 --- a/include/drm/drm_hdcp.h +++ b/include/drm/drm_hdcp.h @@ -101,11 +101,11 @@ /* Following Macros take a byte at a time for bit(s) masking */ /* - * TODO: This has to be changed for DP MST, as multiple stream on - * same port is possible. - * For HDCP2.2 on HDMI and DP SST this value is always 1. + * TODO: HDCP_2_2_MAX_CONTENT_STREAMS_CNT is based upon actual + * H/W MST streams capacity. + * This required to be moved out to platform specific header. */ -#define HDCP_2_2_MAX_CONTENT_STREAMS_CNT 1 +#define HDCP_2_2_MAX_CONTENT_STREAMS_CNT 4 #define HDCP_2_2_TXCAP_MASK_LEN2 #define HDCP_2_2_RXCAPS_LEN3 #define HDCP_2_2_RX_REPEATER(x)((x) & BIT(0)) -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 14/19] drm/i915/hdcp: MST streams support in hdcp port_data
Add support for multiple mst stream in hdcp port data which will be used by RepeaterAuthStreamManage msg and HDCP 2.2 security f/w for m' validation. Security f/w doesn't have any provision to mark the stream_type for each stream separately, it just take single input of stream_type while authenticating the port and applies the same stream_type to all streams. So driver mark each stream_type with common highest supported content type for all streams in DP MST Topology. Security f/w supports RepeaterAuthStreamManage msg and m' validation only once during port authentication and encryption. Though it is not compulsory, security fw should support dynamic update of content_type and should support RepeaterAuthStreamManage msg and m' validation whenever required. v2: - Init the hdcp port data k for HDMI/DP SST stream. v3: - Cosmetic changes. [Uma] v4: - 's/port_auth/hdcp_port_auth'. [Ram] - Commit log improvement. v5: - Comment and commit log improvement. [Ram] Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- .../drm/i915/display/intel_display_types.h| 4 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 113 +++--- 2 files changed, 102 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index b74c10c8b01c..b37a02a73de6 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -1502,10 +1502,12 @@ struct intel_digital_port { enum phy_fia tc_phy_fia; u8 tc_phy_fia_idx; - /* protects num_hdcp_streams reference count, hdcp_port_data */ + /* protects num_hdcp_streams reference count, hdcp_port_data and hdcp_auth_status */ struct mutex hdcp_mutex; /* the number of pipes using HDCP signalling out of this port */ unsigned int num_hdcp_streams; + /* port HDCP auth status */ + bool hdcp_auth_status; /* HDCP port data need to pass to security f/w */ struct hdcp_port_data hdcp_port_data; diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 2bec26123a05..e26a63f0c189 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -26,6 +26,74 @@ #define KEY_LOAD_TRIES 5 #define HDCP2_LC_RETRY_CNT 3 +static int intel_conn_to_vcpi(struct intel_connector *connector) +{ + /* For HDMI this is forced to be 0x0. For DP SST also this is 0x0. */ + return connector->port ? connector->port->vcpi.vcpi : 0; +} + +/* + * intel_hdcp_required_content_stream selects the most highest common possible HDCP + * content_type for all streams in DP MST topology because security f/w doesn't + * have any provision to mark content_type for each stream separately, it marks + * all available streams with the content_type proivided at the time of port + * authentication. This may prohibit the userspace to use type1 content on + * HDCP 2.2 capable sink because of other sink are not capable of HDCP 2.2 in + * DP MST topology. Though it is not compulsory, security fw should change its + * policy to mark different content_types for different streams. + */ +static int +intel_hdcp_required_content_stream(struct intel_digital_port *dig_port) +{ + struct drm_connector_list_iter conn_iter; + struct intel_digital_port *conn_dig_port; + struct intel_connector *connector; + struct drm_i915_private *i915 = to_i915(dig_port->base.base.dev); + struct hdcp_port_data *data = &dig_port->hdcp_port_data; + bool enforce_type0 = false; + int k; + + if (dig_port->hdcp_auth_status) + return 0; + + drm_connector_list_iter_begin(&i915->drm, &conn_iter); + for_each_intel_connector_iter(connector, &conn_iter) { + if (!intel_encoder_is_mst(intel_attached_encoder(connector))) + continue; + + conn_dig_port = intel_attached_dig_port(connector); + if (conn_dig_port != dig_port) + continue; + + if (connector->base.status == connector_status_disconnected) + continue; + + if (!enforce_type0 && !intel_hdcp2_capable(connector)) + enforce_type0 = true; + + data->streams[data->k].stream_id = intel_conn_to_vcpi(connector); + data->k++; + + /* if there is only one active stream */ + if (dig_port->dp.active_mst_links <= 1) + break; + } + drm_connector_list_iter_end(&conn_iter); + + if (drm_WARN_ON(&i915->drm, data->k > INTEL_NUM_PIPES(i915) || data->k == 0)) + return -EINVAL; + + /* +* Apply common protection level across all streams in DP MST Topology. +* Use
Re: [PATCH v3 2/8] drm/ast: Only map cursor BOs during updates
On Fri, Dec 11, 2020 at 11:49:48AM +0100, Thomas Zimmermann wrote: > > > Am 11.12.20 um 11:18 schrieb Daniel Vetter: > > On Wed, Dec 09, 2020 at 03:25:21PM +0100, Thomas Zimmermann wrote: > > > The HW cursor's BO used to be mapped permanently into the kernel's > > > address space. GEM's vmap operation will be protected by locks, and > > > we don't want to lock the BO's for an indefinate period of time. > > > > > > Change the cursor code to map the HW BOs only during updates. The > > > vmap operation in VRAM helpers is cheap, as a once estabished mapping > > > is being reused until the BO actually moves. As the HW cursor BOs are > > > permanently pinned, they never move at all. > > > > > > v2: > > > * fix typos in commit description > > > > > > Signed-off-by: Thomas Zimmermann > > > Acked-by: Christian König > > > > Acked-by: Daniel Vetter > > > > Now there's a pretty big issue here though: We can't take dma_resv_lock in > > commit_tail, because of possible deadlocks on at least gpus that do real > > async rendering because of the dma_fences. Unfortunately my annotations > > patches got stuck a bit, I need to refresh them. > > > > Rules are you can pin and unpin stuff in prepare/cleanup_plane, and also > > take dma_resv_lock there, but not in commit_tail in-between. So I think > > our vmap_local needs to loose the unconditional assert_locked and require > > either that or a pin count. > > I guess my commit description is misleading when it speaks of updates. > ast_cursor_blit() is actually called from the cursor plane's prepare_fb > function. [1] The vmap code in ast_cursor_show() could be moved into blit() > as well, I think. Oh I failed to check this properly. Even better. > I guess the clean solution is to integrate the cursor code with the > modesetting code in ast_mode. From there, locks and mappings can be > established in prepare_fb and the HW state can be updated in atomic_commit. Yup. I'll still refresh my series with lockdep annotations, keeps paranoid me at peace :-) -Daniel > > Best regards > Thomas > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/ast/ast_mode.c#n646 > > > -Daniel > > > > > --- > > > drivers/gpu/drm/ast/ast_cursor.c | 51 ++-- > > > drivers/gpu/drm/ast/ast_drv.h| 2 -- > > > 2 files changed, 28 insertions(+), 25 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/ast/ast_cursor.c > > > b/drivers/gpu/drm/ast/ast_cursor.c > > > index 68bf3d33f1ed..fac1ee79c372 100644 > > > --- a/drivers/gpu/drm/ast/ast_cursor.c > > > +++ b/drivers/gpu/drm/ast/ast_cursor.c > > > @@ -39,7 +39,6 @@ static void ast_cursor_fini(struct ast_private *ast) > > > for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) { > > > gbo = ast->cursor.gbo[i]; > > > - drm_gem_vram_vunmap(gbo, &ast->cursor.map[i]); > > > drm_gem_vram_unpin(gbo); > > > drm_gem_vram_put(gbo); > > > } > > > @@ -53,14 +52,13 @@ static void ast_cursor_release(struct drm_device > > > *dev, void *ptr) > > > } > > > /* > > > - * Allocate cursor BOs and pins them at the end of VRAM. > > > + * Allocate cursor BOs and pin them at the end of VRAM. > > >*/ > > > int ast_cursor_init(struct ast_private *ast) > > > { > > > struct drm_device *dev = &ast->base; > > > size_t size, i; > > > struct drm_gem_vram_object *gbo; > > > - struct dma_buf_map map; > > > int ret; > > > size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, > > > PAGE_SIZE); > > > @@ -77,15 +75,7 @@ int ast_cursor_init(struct ast_private *ast) > > > drm_gem_vram_put(gbo); > > > goto err_drm_gem_vram_put; > > > } > > > - ret = drm_gem_vram_vmap(gbo, &map); > > > - if (ret) { > > > - drm_gem_vram_unpin(gbo); > > > - drm_gem_vram_put(gbo); > > > - goto err_drm_gem_vram_put; > > > - } > > > - > > > ast->cursor.gbo[i] = gbo; > > > - ast->cursor.map[i] = map; > > > } > > > return drmm_add_action_or_reset(dev, ast_cursor_release, NULL); > > > @@ -94,7 +84,6 @@ int ast_cursor_init(struct ast_private *ast) > > > while (i) { > > > --i; > > > gbo = ast->cursor.gbo[i]; > > > - drm_gem_vram_vunmap(gbo, &ast->cursor.map[i]); > > > drm_gem_vram_unpin(gbo); > > > drm_gem_vram_put(gbo); > > > } > > > @@ -168,31 +157,38 @@ static void update_cursor_image(u8 __iomem *dst, > > > const u8 *src, int width, int h > > > int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb) > > > { > > > struct drm_device *dev = &ast->base; > > > - struct drm_gem_vram_object *gbo; > > > - struct dma_buf_map map; > > > - int ret; > > > - void *src; > > > + stru
[PATCH v8 12/19] misc/mei/hdcp: Fix AUTH_STREAM_REQ cmd buffer len
Fix the size of WIRED_REPEATER_AUTH_STREAM_REQ cmd buffer size. It is based upon the actual number of MST streams and size of wired_cmd_repeater_auth_stream_req_in. Excluding the size of hdcp_cmd_header. v2: - hdcp_cmd_header size annotation nitpick. [Tomas] Cc: Tomas Winkler Cc: Ramalingam C Acked-by: Tomas Winkler Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/misc/mei/hdcp/mei_hdcp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/misc/mei/hdcp/mei_hdcp.c b/drivers/misc/mei/hdcp/mei_hdcp.c index 9ae9669e46ea..3506a3534294 100644 --- a/drivers/misc/mei/hdcp/mei_hdcp.c +++ b/drivers/misc/mei/hdcp/mei_hdcp.c @@ -569,8 +569,7 @@ static int mei_hdcp_verify_mprime(struct device *dev, verify_mprime_in->header.api_version = HDCP_API_VERSION; verify_mprime_in->header.command_id = WIRED_REPEATER_AUTH_STREAM_REQ; verify_mprime_in->header.status = ME_HDCP_STATUS_SUCCESS; - verify_mprime_in->header.buffer_len = - WIRED_CMD_BUF_LEN_REPEATER_AUTH_STREAM_REQ_MIN_IN; + verify_mprime_in->header.buffer_len = cmd_size - sizeof(verify_mprime_in->header); verify_mprime_in->port.integrated_port_type = data->port_type; verify_mprime_in->port.physical_port = (u8)data->fw_ddi; -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 11/19] drm/i915/hdcp: Encapsulate hdcp_port_data to dig_port
hdcp_port_data is specific to a port on which HDCP encryption is getting enabled, so encapsulate it to intel_digital_port. This will be required to enable HDCP 2.2 stream encryption. v2: - 's/port_data/hdcp_port_data'. [Ram] Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_ddi.c | 2 + .../drm/i915/display/intel_display_types.h| 5 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 56 +++ 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 046c49931b98..54418a6cc3d4 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -4965,6 +4965,8 @@ static void intel_ddi_encoder_destroy(struct drm_encoder *encoder) intel_dp_encoder_flush_work(encoder); drm_encoder_cleanup(encoder); + if (dig_port) + kfree(dig_port->hdcp_port_data.streams); kfree(dig_port); } diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index f0aeba9a222a..b74c10c8b01c 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -406,7 +406,6 @@ struct intel_hdcp { * content can flow only through a link protected by HDCP2.2. */ u8 content_type; - struct hdcp_port_data port_data; bool is_paired; bool is_repeater; @@ -1503,10 +1502,12 @@ struct intel_digital_port { enum phy_fia tc_phy_fia; u8 tc_phy_fia_idx; - /* protects num_hdcp_streams reference count */ + /* protects num_hdcp_streams reference count, hdcp_port_data */ struct mutex hdcp_mutex; /* the number of pipes using HDCP signalling out of this port */ unsigned int num_hdcp_streams; + /* HDCP port data need to pass to security f/w */ + struct hdcp_port_data hdcp_port_data; void (*write_infoframe)(struct intel_encoder *encoder, const struct intel_crtc_state *crtc_state, diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 4ad086e7ec3c..2bec26123a05 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -15,6 +15,7 @@ #include #include +#include "i915_drv.h" #include "i915_reg.h" #include "intel_display_power.h" #include "intel_display_types.h" @@ -1025,7 +1026,8 @@ static int hdcp2_prepare_ake_init(struct intel_connector *connector, struct hdcp2_ake_init *ake_data) { - struct hdcp_port_data *data = &connector->hdcp.port_data; + struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct hdcp_port_data *data = &dig_port->hdcp_port_data; struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct i915_hdcp_comp_master *comp; int ret; @@ -1054,7 +1056,8 @@ hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector, struct hdcp2_ake_no_stored_km *ek_pub_km, size_t *msg_sz) { - struct hdcp_port_data *data = &connector->hdcp.port_data; + struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct hdcp_port_data *data = &dig_port->hdcp_port_data; struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct i915_hdcp_comp_master *comp; int ret; @@ -1081,7 +1084,8 @@ hdcp2_verify_rx_cert_prepare_km(struct intel_connector *connector, static int hdcp2_verify_hprime(struct intel_connector *connector, struct hdcp2_ake_send_hprime *rx_hprime) { - struct hdcp_port_data *data = &connector->hdcp.port_data; + struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct hdcp_port_data *data = &dig_port->hdcp_port_data; struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct i915_hdcp_comp_master *comp; int ret; @@ -1106,7 +1110,8 @@ static int hdcp2_store_pairing_info(struct intel_connector *connector, struct hdcp2_ake_send_pairing_info *pairing_info) { - struct hdcp_port_data *data = &connector->hdcp.port_data; + struct intel_digital_port *dig_port = intel_attached_dig_port(connector); + struct hdcp_port_data *data = &dig_port->hdcp_port_data; struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct i915_hdcp_comp_master *comp; int ret; @@ -1132,7 +1137,8 @@ static int hdcp2_prepare_lc_init(struct intel_connector *connector, struct hdcp2_lc_init *lc_init) { - struct hdcp_port_data *data = &conn
[PATCH v8 10/19] drm/i915/hdcp: Pass dig_port to intel_hdcp_init
Pass dig_port as an argument to intel_hdcp_init() and intel_hdcp2_init(). This will be required for HDCP 2.2 stream encryption. Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 4 ++-- drivers/gpu/drm/i915/display/intel_hdcp.c| 12 +++- drivers/gpu/drm/i915/display/intel_hdcp.h| 4 +++- drivers/gpu/drm/i915/display/intel_hdmi.c| 2 +- 4 files changed, 13 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c index 94c5462fa037..3f23f8b53dcd 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c @@ -755,10 +755,10 @@ int intel_dp_init_hdcp(struct intel_digital_port *dig_port, return 0; if (intel_connector->mst_port) - return intel_hdcp_init(intel_connector, port, + return intel_hdcp_init(intel_connector, dig_port, &intel_dp_mst_hdcp_shim); else if (!intel_dp_is_edp(intel_dp)) - return intel_hdcp_init(intel_connector, port, + return intel_hdcp_init(intel_connector, dig_port, &intel_dp_hdcp_shim); return 0; diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index fce444d69521..4ad086e7ec3c 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -1979,12 +1979,13 @@ static enum mei_fw_tc intel_get_mei_fw_tc(enum transcoder cpu_transcoder) } static int initialize_hdcp_port_data(struct intel_connector *connector, -enum port port, +struct intel_digital_port *dig_port, const struct intel_hdcp_shim *shim) { struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct intel_hdcp *hdcp = &connector->hdcp; struct hdcp_port_data *data = &hdcp->port_data; + enum port port = dig_port->base.port; if (INTEL_GEN(dev_priv) < 12) data->fw_ddi = intel_get_mei_fw_ddi_index(port); @@ -2057,14 +2058,15 @@ void intel_hdcp_component_init(struct drm_i915_private *dev_priv) } } -static void intel_hdcp2_init(struct intel_connector *connector, enum port port, +static void intel_hdcp2_init(struct intel_connector *connector, +struct intel_digital_port *dig_port, const struct intel_hdcp_shim *shim) { struct drm_i915_private *i915 = to_i915(connector->base.dev); struct intel_hdcp *hdcp = &connector->hdcp; int ret; - ret = initialize_hdcp_port_data(connector, port, shim); + ret = initialize_hdcp_port_data(connector, dig_port, shim); if (ret) { drm_dbg_kms(&i915->drm, "Mei hdcp data init failed\n"); return; @@ -2074,7 +2076,7 @@ static void intel_hdcp2_init(struct intel_connector *connector, enum port port, } int intel_hdcp_init(struct intel_connector *connector, - enum port port, + struct intel_digital_port *dig_port, const struct intel_hdcp_shim *shim) { struct drm_i915_private *dev_priv = to_i915(connector->base.dev); @@ -2085,7 +2087,7 @@ int intel_hdcp_init(struct intel_connector *connector, return -EINVAL; if (is_hdcp2_supported(dev_priv) && !connector->mst_port) - intel_hdcp2_init(connector, port, shim); + intel_hdcp2_init(connector, dig_port, shim); ret = drm_connector_attach_content_protection_property(&connector->base, diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h index b912a3a0f5b8..8f53b0c7fe5c 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.h +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h @@ -18,13 +18,15 @@ struct intel_connector; struct intel_crtc_state; struct intel_encoder; struct intel_hdcp_shim; +struct intel_digital_port; enum port; enum transcoder; void intel_hdcp_atomic_check(struct drm_connector *connector, struct drm_connector_state *old_state, struct drm_connector_state *new_state); -int intel_hdcp_init(struct intel_connector *connector, enum port port, +int intel_hdcp_init(struct intel_connector *connector, + struct intel_digital_port *dig_port, const struct intel_hdcp_shim *hdcp_shim); int intel_hdcp_enable(struct intel_connector *connector, const struct intel_crtc_state *pipe_config, u8 content_type); diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index e072
[PATCH v8 09/19] drm/i915/hdcp: Enable Gen12 HDCP 1.4 DP MST support
Enable HDCP 1.4 over DP MST for Gen12. v2: - Enable HDCP for <= Gen12 platforms. [Ram] v3: - Connector detials in debug msg. [Ram] Cc: Ramalingam C Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_dp_mst.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 47beb442094f..f76e2c2a83b8 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -829,12 +829,11 @@ static struct drm_connector *intel_dp_add_mst_connector(struct drm_dp_mst_topolo intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector); - - /* TODO: Figure out how to make HDCP work on GEN12+ */ - if (INTEL_GEN(dev_priv) < 12) { + if (INTEL_GEN(dev_priv) <= 12) { ret = intel_dp_init_hdcp(dig_port, intel_connector); if (ret) - DRM_DEBUG_KMS("HDCP init failed, skipping.\n"); + drm_dbg_kms(&dev_priv->drm, "[%s:%d] HDCP MST init failed, skipping.\n", + connector->name, connector->base.id); } /* -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 08/19] drm/i915/hdcp: Configure HDCP1.4 MST steram encryption status
Enable HDCP 1.4 DP MST stream encryption. Enable stream encryption once encryption is enabled on the DP transport driving the link for each stream which has requested encryption. Disable stream encryption for each stream that no longer requires encryption before disabling HDCP encryption on the link. v2: - Added debug print for stream encryption. - Disable the hdcp on port after disabling last stream encryption. v3: - Cosmetic change, removed the value less comment. [Uma] v4: - Split the Gen12 HDCP enablement patch. [Ram] - Add connector details in drm_err. v5: - uniformity for connector detail in DMESG. [Ram] - comments improvement. [Ram] Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_hdcp.c | 38 +++ 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 6e6465b4ecfa..fce444d69521 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -766,10 +766,17 @@ static int intel_hdcp_auth(struct intel_connector *connector) return -ETIMEDOUT; } - /* -* XXX: If we have MST-connected devices, we need to enable encryption -* on those as well. -*/ + /* DP MST Auth Part 1 Step 2.a and Step 2.b */ + if (shim->stream_encryption) { + ret = shim->stream_encryption(connector, true); + if (ret) { + drm_err(&dev_priv->drm, "[%s:%d] Failed to enable HDCP 1.4 stream enc\n", + connector->base.name, connector->base.base.id); + return ret; + } + drm_dbg_kms(&dev_priv->drm, "HDCP 1.4 transcoder: %s stream encrypted\n", + transcoder_name(hdcp->stream_transcoder)); + } if (repeater_present) return intel_hdcp_auth_downstream(connector); @@ -791,18 +798,23 @@ static int _intel_hdcp_disable(struct intel_connector *connector) drm_dbg_kms(&dev_priv->drm, "[%s:%d] HDCP is being disabled...\n", connector->base.name, connector->base.base.id); + if (hdcp->shim->stream_encryption) { + ret = hdcp->shim->stream_encryption(connector, false); + if (ret) { + drm_err(&dev_priv->drm, "[%s:%d] Failed to disable HDCP 1.4 stream enc\n", + connector->base.name, connector->base.base.id); + return ret; + } + drm_dbg_kms(&dev_priv->drm, "HDCP 1.4 transcoder: %s stream encryption disabled\n", + transcoder_name(hdcp->stream_transcoder)); + } + /* -* If there are other connectors on this port using HDCP, don't disable -* it. Instead, toggle the HDCP signalling off on that particular -* connector/pipe and exit. +* If there are other connectors on this port using HDCP, don't disable it +* until it disabled HDCP encryption for all connectors in MST topology. */ - if (dig_port->num_hdcp_streams > 0) { - ret = hdcp->shim->toggle_signalling(dig_port, - cpu_transcoder, false); - if (ret) - DRM_ERROR("Failed to disable HDCP signalling\n"); + if (dig_port->num_hdcp_streams > 0) return ret; - } hdcp->hdcp_encrypted = false; intel_de_write(dev_priv, HDCP_CONF(dev_priv, cpu_transcoder, port), 0); -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 07/19] drm/i915/hdcp: HDCP stream encryption support
Both HDCP_{1.x,2.x} requires to select/deselect Multistream HDCP bit in TRANS_DDI_FUNC_CTL in order to enable/disable stream HDCP encryption over DP MST Transport Link. HDCP 1.4 stream encryption requires to validate the stream encryption status in HDCP_STATUS_{TRANSCODER,PORT} register driving that link in order to enable/disable the stream encryption. Both of above requirement are same for all Gen with respect to B.Spec Documentation. v2: - Cosmetic changes function name, error msg print and stream typo fixes. [Uma] v3: - uniformity for connector detail in DMESG. [Ram] Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_ddi.c | 10 +-- drivers/gpu/drm/i915/display/intel_ddi.h | 6 +- .../drm/i915/display/intel_display_types.h| 4 + drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 90 --- drivers/gpu/drm/i915/display/intel_hdmi.c | 14 +-- drivers/gpu/drm/i915/i915_reg.h | 1 + 6 files changed, 97 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 14c2c0a15464..046c49931b98 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -2029,9 +2029,9 @@ void intel_ddi_disable_transcoder_func(const struct intel_crtc_state *crtc_state } } -int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder, -enum transcoder cpu_transcoder, -bool enable) +int intel_ddi_toggle_hdcp_bits(struct intel_encoder *intel_encoder, + enum transcoder cpu_transcoder, + bool enable, u32 hdcp_mask) { struct drm_device *dev = intel_encoder->base.dev; struct drm_i915_private *dev_priv = to_i915(dev); @@ -2046,9 +2046,9 @@ int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder, tmp = intel_de_read(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder)); if (enable) - tmp |= TRANS_DDI_HDCP_SIGNALLING; + tmp |= hdcp_mask; else - tmp &= ~TRANS_DDI_HDCP_SIGNALLING; + tmp &= ~hdcp_mask; intel_de_write(dev_priv, TRANS_DDI_FUNC_CTL(cpu_transcoder), tmp); intel_display_power_put(dev_priv, intel_encoder->power_domain, wakeref); return ret; diff --git a/drivers/gpu/drm/i915/display/intel_ddi.h b/drivers/gpu/drm/i915/display/intel_ddi.h index dcc711cfe4fe..a4dd815c 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.h +++ b/drivers/gpu/drm/i915/display/intel_ddi.h @@ -50,9 +50,9 @@ u32 bxt_signal_levels(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state); u32 ddi_signal_levels(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state); -int intel_ddi_toggle_hdcp_signalling(struct intel_encoder *intel_encoder, -enum transcoder cpu_transcoder, -bool enable); +int intel_ddi_toggle_hdcp_bits(struct intel_encoder *intel_encoder, + enum transcoder cpu_transcoder, + bool enable, u32 hdcp_mask); void icl_sanitize_encoder_pll_mapping(struct intel_encoder *encoder); #endif /* __INTEL_DDI_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 729a9792051f..f0aeba9a222a 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -343,6 +343,10 @@ struct intel_hdcp_shim { enum transcoder cpu_transcoder, bool enable); + /* Enable/Disable stream encryption on DP MST Transport Link */ + int (*stream_encryption)(struct intel_connector *connector, +bool enable); + /* Ensures the link is still protected */ bool (*check_link)(struct intel_digital_port *dig_port, struct intel_connector *connector); diff --git a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c index 03424d20e9f7..94c5462fa037 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_dp_hdcp.c @@ -16,6 +16,30 @@ #include "intel_dp.h" #include "intel_hdcp.h" +static unsigned int transcoder_to_stream_enc_status(enum transcoder cpu_transcoder) +{ + u32 stream_enc_mask; + + switch (cpu_transcoder) { + case TRANSCODER_A: + stream_enc_mask = HDCP_STATUS_STREAM_A_ENC; + break; + case TRANSCODER_B: + stream_enc_mask = HDCP_STATUS_STREAM_B_ENC; + break; + case TRAN
[PATCH v8 06/19] drm/i915/hdcp: Move HDCP enc status timeout to header
DP MST stream encryption status requires time of a link frame in order to change its status, but as there were some HDCP encryption timeout observed earlier, it is safer to use ENCRYPT_STATUS_CHANGE_TIMEOUT_MS timeout for stream status too, it requires to move the macro to a header. It will be used by both HDCP{1.x,2.x} stream status timeout. Related: 'commit 7e90e8d0c0ea ("drm/i915: Increase timeout for Encrypt status change")' Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_hdcp.c | 9 - drivers/gpu/drm/i915/display/intel_hdcp.h | 2 ++ 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 293f72d1d215..6e6465b4ecfa 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -23,7 +23,6 @@ #include "intel_connector.h" #define KEY_LOAD_TRIES 5 -#define ENCRYPT_STATUS_CHANGE_TIMEOUT_MS 50 #define HDCP2_LC_RETRY_CNT 3 static @@ -762,7 +761,7 @@ static int intel_hdcp_auth(struct intel_connector *connector) if (intel_de_wait_for_set(dev_priv, HDCP_STATUS(dev_priv, cpu_transcoder, port), HDCP_STATUS_ENC, - ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) { + HDCP_ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) { drm_err(&dev_priv->drm, "Timed out waiting for encryption\n"); return -ETIMEDOUT; } @@ -809,7 +808,7 @@ static int _intel_hdcp_disable(struct intel_connector *connector) intel_de_write(dev_priv, HDCP_CONF(dev_priv, cpu_transcoder, port), 0); if (intel_de_wait_for_clear(dev_priv, HDCP_STATUS(dev_priv, cpu_transcoder, port), - ~0, ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) { + ~0, HDCP_ENCRYPT_STATUS_CHANGE_TIMEOUT_MS)) { drm_err(&dev_priv->drm, "Failed to disable HDCP, timeout clearing status\n"); return -ETIMEDOUT; @@ -1641,7 +1640,7 @@ static int hdcp2_enable_encryption(struct intel_connector *connector) HDCP2_STATUS(dev_priv, cpu_transcoder, port), LINK_ENCRYPTION_STATUS, - ENCRYPT_STATUS_CHANGE_TIMEOUT_MS); + HDCP_ENCRYPT_STATUS_CHANGE_TIMEOUT_MS); return ret; } @@ -1665,7 +1664,7 @@ static int hdcp2_disable_encryption(struct intel_connector *connector) HDCP2_STATUS(dev_priv, cpu_transcoder, port), LINK_ENCRYPTION_STATUS, - ENCRYPT_STATUS_CHANGE_TIMEOUT_MS); + HDCP_ENCRYPT_STATUS_CHANGE_TIMEOUT_MS); if (ret == -ETIMEDOUT) drm_dbg_kms(&dev_priv->drm, "Disable Encryption Timedout"); diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h index bc51c1e9b481..b912a3a0f5b8 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.h +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h @@ -8,6 +8,8 @@ #include +#define HDCP_ENCRYPT_STATUS_CHANGE_TIMEOUT_MS 50 + struct drm_connector; struct drm_connector_state; struct drm_i915_private; -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 05/19] drm/i915/hdcp: DP MST transcoder for link and stream
Gen12 has H/W delta with respect to HDCP{1.x,2.x} display engine instances lies in Transcoder instead of DDI as in Gen11. This requires hdcp driver to use mst_master_transcoder for link authentication and stream transcoder for stream encryption separately. This will be used for both HDCP 1.4 and HDCP 2.2 over DP MST on Gen12. Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_ddi.c | 2 +- .../gpu/drm/i915/display/intel_display_types.h| 2 ++ drivers/gpu/drm/i915/display/intel_dp_mst.c | 2 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 15 +++ drivers/gpu/drm/i915/display/intel_hdcp.h | 2 +- 5 files changed, 16 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 6863236df1d0..14c2c0a15464 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -4252,7 +4252,7 @@ static void intel_enable_ddi(struct intel_atomic_state *state, if (conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED) intel_hdcp_enable(to_intel_connector(conn_state->connector), - crtc_state->cpu_transcoder, + crtc_state, (u8)conn_state->hdcp_content_type); } diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 5bc5bfbc4551..729a9792051f 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -436,6 +436,8 @@ struct intel_hdcp { * Hence caching the transcoder here. */ enum transcoder cpu_transcoder; + /* Only used for DP MST stream encryption */ + enum transcoder stream_transcoder; }; struct intel_connector { diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index 27f04aed8764..47beb442094f 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -569,7 +569,7 @@ static void intel_mst_enable_dp(struct intel_atomic_state *state, if (conn_state->content_protection == DRM_MODE_CONTENT_PROTECTION_DESIRED) intel_hdcp_enable(to_intel_connector(conn_state->connector), - pipe_config->cpu_transcoder, + pipe_config, (u8)conn_state->hdcp_content_type); } diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index 7d63e9495956..293f72d1d215 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -2095,7 +2095,7 @@ int intel_hdcp_init(struct intel_connector *connector, } int intel_hdcp_enable(struct intel_connector *connector, - enum transcoder cpu_transcoder, u8 content_type) + const struct intel_crtc_state *pipe_config, u8 content_type) { struct drm_i915_private *dev_priv = to_i915(connector->base.dev); struct intel_digital_port *dig_port = intel_attached_dig_port(connector); @@ -2117,10 +2117,17 @@ int intel_hdcp_enable(struct intel_connector *connector, drm_WARN_ON(&dev_priv->drm, hdcp->value == DRM_MODE_CONTENT_PROTECTION_ENABLED); hdcp->content_type = content_type; - hdcp->cpu_transcoder = cpu_transcoder; + + if (intel_crtc_has_type(pipe_config, INTEL_OUTPUT_DP_MST)) { + hdcp->cpu_transcoder = pipe_config->mst_master_transcoder; + hdcp->stream_transcoder = pipe_config->cpu_transcoder; + } else { + hdcp->cpu_transcoder = pipe_config->cpu_transcoder; + hdcp->stream_transcoder = INVALID_TRANSCODER; + } if (INTEL_GEN(dev_priv) >= 12) - hdcp->port_data.fw_tc = intel_get_mei_fw_tc(cpu_transcoder); + hdcp->port_data.fw_tc = intel_get_mei_fw_tc(hdcp->cpu_transcoder); /* * Considering that HDCP2.2 is more secure than HDCP1.4, If the setup @@ -2240,7 +2247,7 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state, if (desired_and_not_enabled || content_protection_type_changed) intel_hdcp_enable(connector, - crtc_state->cpu_transcoder, + crtc_state, (u8)conn_state->hdcp_content_type); } diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.h b/drivers/gpu/drm/i915/display/intel_hdcp.h index 1bbf5b67ed0a..bc51c1e9b481 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.h +++ b/drivers/gpu/drm/i915/display/intel_hdcp.h @@ -25,7 +25,7 @@
[PATCH v8 04/19] drm/i915/hdcp: No HDCP when encoder is't initialized
There can be situation when DP MST connector is created without mst modeset being done, in those cases connector->encoder will be NULL. MST connector->encoder initializes after modeset. Don't enable HDCP in such cases to prevent any crash. Cc: Ramalingam C Cc: Juston Li Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_hdcp.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index b9d8825e2bb1..7d63e9495956 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -2106,6 +2106,12 @@ int intel_hdcp_enable(struct intel_connector *connector, if (!hdcp->shim) return -ENOENT; + if (!connector->encoder) { + drm_err(&dev_priv->drm, "[%s:%d] encoder is not initialized\n", + connector->base.name, connector->base.base.id); + return -ENODEV; + } + mutex_lock(&hdcp->mutex); mutex_lock(&dig_port->hdcp_mutex); drm_WARN_ON(&dev_priv->drm, -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 03/19] drm/i915/hotplug: Handle CP_IRQ for DP-MST
Handle CP_IRQ in DEVICE_SERVICE_IRQ_VECTOR_ESI0 It requires to call intel_hdcp_handle_cp_irq() in case of CP_IRQ is triggered by a sink in DP-MST topology. Cc: "Ville Syrjälä" Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_dp.c | 14 +- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index b2bc0c8c39c7..501b9a8a2f45 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -5783,6 +5783,17 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) "Could not write test response to sink\n"); } +static void +intel_dp_mst_hpd_irq(struct intel_dp *intel_dp, u8 *esi, bool *handled) +{ + drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, handled); + + if (esi[1] & DP_CP_IRQ) { + intel_hdcp_handle_cp_irq(intel_dp->attached_connector); + *handled = true; + } +} + /** * intel_dp_check_mst_status - service any pending MST interrupts, check link status * @intel_dp: Intel DP struct @@ -5827,7 +5838,8 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp) drm_dbg_kms(&i915->drm, "got esi %3ph\n", esi); - drm_dp_mst_hpd_irq(&intel_dp->mst_mgr, esi, &handled); + intel_dp_mst_hpd_irq(intel_dp, esi, &handled); + if (!handled) break; -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 02/19] drm/i915/hdcp: Get conn while content_type changed
Get DRM connector reference count while scheduling a prop work to avoid any possible destroy of DRM connector when it is in DRM_CONNECTOR_REGISTERED state. Fixes: a6597faa2d59 ("drm/i915: Protect workers against disappearing connectors") Cc: Sean Paul Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_hdcp.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index eee8263405b9..b9d8825e2bb1 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -2210,6 +2210,7 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state, if (content_protection_type_changed) { mutex_lock(&hdcp->mutex); hdcp->value = DRM_MODE_CONTENT_PROTECTION_DESIRED; + drm_connector_get(&connector->base); schedule_work(&hdcp->prop_work); mutex_unlock(&hdcp->mutex); } -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 01/19] drm/i915/hdcp: Update CP property in update_pipe
When crtc state need_modeset is true it is not necessary it is going to be a real modeset, it can turns to be a fastset instead of modeset. This turns content protection property to be DESIRED and hdcp update_pipe left with property to be in DESIRED state but actual hdcp->value was ENABLED. This issue is caught with DP MST setup, where we have multiple connector in same DP_MST topology. When disabling HDCP on one of DP MST connector leads to set the crtc state need_modeset to true for all other crtc driving the other DP-MST topology connectors. This turns up other DP MST connectors CP property to be DESIRED despite the actual hdcp->value is ENABLED. Above scenario fails the DP MST HDCP IGT test, disabling HDCP on one MST stream should not cause to disable HDCP on another MST stream on same DP MST topology. v2: - Fixed connector->base.registration_state == DRM_CONNECTOR_REGISTERED WARN_ON. v3: - Commit log improvement. [Uma] - Added a comment before scheduling prop_work. [Uma] Fixes: 33f9a623bfc6 ("drm/i915/hdcp: Update CP as per the kernel internal state") Cc: Ramalingam C Reviewed-by: Uma Shankar Reviewed-by: Ramalingam C Tested-by: Karthik B S Signed-off-by: Anshuman Gupta --- drivers/gpu/drm/i915/display/intel_hdcp.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/i915/display/intel_hdcp.c b/drivers/gpu/drm/i915/display/intel_hdcp.c index b2a4bbcfdcd2..eee8263405b9 100644 --- a/drivers/gpu/drm/i915/display/intel_hdcp.c +++ b/drivers/gpu/drm/i915/display/intel_hdcp.c @@ -2221,6 +2221,14 @@ void intel_hdcp_update_pipe(struct intel_atomic_state *state, desired_and_not_enabled = hdcp->value != DRM_MODE_CONTENT_PROTECTION_ENABLED; mutex_unlock(&hdcp->mutex); + /* +* If HDCP already ENABLED and CP property is DESIRED, schedule +* prop_work to update correct CP property to user space. +*/ + if (!desired_and_not_enabled && !content_protection_type_changed) { + drm_connector_get(&connector->base); + schedule_work(&hdcp->prop_work); + } } if (desired_and_not_enabled || content_protection_type_changed) -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8 00/19] HDCP 2.2 and HDCP 1.4 Gen12 DP MST support
This v8 version has fixed the cosmetics eview comment from ram. No functional change. It has been tested manually with below IGT series on TGL and ICL. https://patchwork.freedesktop.org/series/82987/ [PATCH v8 12/19] misc/mei/hdcp: Fix AUTH_STREAM_REQ cmd buffer len has an Ack from Tomas to merge it via drm-intel. [PATCH v8 13/19] drm/hdcp: Max MST content streams has an Ack from drm-misc maintainer to merge it via drm-intel. Test-with: 20201126050320.2434-2-karthik@intel.com Anshuman Gupta (19): drm/i915/hdcp: Update CP property in update_pipe drm/i915/hdcp: Get conn while content_type changed drm/i915/hotplug: Handle CP_IRQ for DP-MST drm/i915/hdcp: No HDCP when encoder is't initialized drm/i915/hdcp: DP MST transcoder for link and stream drm/i915/hdcp: Move HDCP enc status timeout to header drm/i915/hdcp: HDCP stream encryption support drm/i915/hdcp: Configure HDCP1.4 MST steram encryption status drm/i915/hdcp: Enable Gen12 HDCP 1.4 DP MST support drm/i915/hdcp: Pass dig_port to intel_hdcp_init drm/i915/hdcp: Encapsulate hdcp_port_data to dig_port misc/mei/hdcp: Fix AUTH_STREAM_REQ cmd buffer len drm/hdcp: Max MST content streams drm/i915/hdcp: MST streams support in hdcp port_data drm/i915/hdcp: Pass connector to check_2_2_link drm/i915/hdcp: Add HDCP 2.2 stream register drm/i915/hdcp: Support for HDCP 2.2 MST shim callbacks drm/i915/hdcp: Configure HDCP2.2 MST steram encryption status drm/i915/hdcp: Enable HDCP 2.2 MST support drivers/gpu/drm/i915/display/intel_ddi.c | 14 +- drivers/gpu/drm/i915/display/intel_ddi.h | 6 +- .../drm/i915/display/intel_display_types.h| 20 +- drivers/gpu/drm/i915/display/intel_dp.c | 14 +- drivers/gpu/drm/i915/display/intel_dp_hdcp.c | 186 +-- drivers/gpu/drm/i915/display/intel_dp_mst.c | 9 +- drivers/gpu/drm/i915/display/intel_hdcp.c | 303 ++ drivers/gpu/drm/i915/display/intel_hdcp.h | 8 +- drivers/gpu/drm/i915/display/intel_hdmi.c | 19 +- drivers/gpu/drm/i915/i915_reg.h | 40 +++ drivers/misc/mei/hdcp/mei_hdcp.c | 3 +- include/drm/drm_hdcp.h| 8 +- 12 files changed, 510 insertions(+), 120 deletions(-) -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 4/4] drm: require each CRTC to have a unique primary plane
On Fri, 11 Dec 2020 13:08:26 + Simon Ser wrote: > User-space expects to be able to pick a primary plane for each CRTC > exposed by the driver. Make sure this assumption holds in > drm_mode_config_validate. > > Use the legacy drm_crtc.primary field to check this, because it's > simpler and we require drivers to set it anyways. Accumulate a set of > primary planes which are already used for a CRTC in a bitmask. Error out > if a primary plane is re-used. > > v2: new patch > > v3: > - Use u64 instead of __u64 (Jani) > - Use `unsigned int` instead of `unsigned` (Jani) > > Signed-off-by: Simon Ser > Cc: Daniel Vetter > Cc: Pekka Paalanen > Cc: Jani Nikula > --- > drivers/gpu/drm/drm_mode_config.c | 21 + > drivers/gpu/drm/drm_plane.c | 6 ++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index fbe680035129..c5cf5624c106 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c ... > + > + drm_for_each_plane(plane, dev) { > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { > + num_primary++; > + } > + } > + > + WARN(num_primary != dev->mode_config.num_crtc, > + "Must have as many primary planes as there are CRTCs, but have %u > primary planes and %u CRTCs", > + num_primary, dev->mode_config.num_crtc); > } > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 49b0a8b9ac02..a1f4510efa83 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -54,6 +54,12 @@ > * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see > * &drm_plane.possible_crtcs. > * > + * Each CRTC must have a unique primary plane userspace can attach to enable > + * the CRTC. In other words, userspace must be able to attach a different > + * primary plane to each CRTC at the same time. Primary planes can still be > + * compatible with multiple CRTCs. There must be exactly as many primary > planes > + * as there are CRTCs. > + * > * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM > core > * relies on the driver to set the primary and optionally the cursor plane > used > * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). > All Hi, is there a reason why one cannot have more primary planes than CRTCs in existence? Daniel implied that in <20201209003637.GK401619@phenom.ffwll.local>, but I didn't get the reason for it yet. E.g. if all your planes are interchangeable in the sense that you can turn on a CRTC with any one of them, would one not then expose all the planes as "Primary"? If the planes have other differences, like supported formats or scaling, then marking them all "Primary" would let userspace know that it can pick any plane with the suitable properties and expect to turn on the CRTC with it. Or does marking a plane as "Primary" imply something else too, like "cannot scale"? I think Weston does make this assumption in an attempt to hit fewer causes for failure. Thanks, pq pgpWvRtEkAc0C.pgp Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 3/4] drm: require a non_NULL drm_crtc.primary
On Fri, 11 Dec 2020 13:06:17 + Simon Ser wrote: > If a CRTC is missing a legacy primary plane pointer, a lot of things > will be broken for user-space: fbdev stops working and the entire legacy > uAPI stops working. > > Require all drivers to populate drm_crtc.primary to prevent these > issues. Warn if it's NULL. > > Signed-off-by: Simon Ser > Reviewed-by: Daniel Vetter > Cc: Pekka Paalanen Acked-by: Pekka Paalanen > --- > drivers/gpu/drm/drm_mode_config.c | 3 +++ > drivers/gpu/drm/drm_plane.c | 2 +- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index 2c73a60e8765..fbe680035129 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -639,6 +639,9 @@ void drm_mode_config_validate(struct drm_device *dev) > } > > drm_for_each_crtc(crtc, dev) { > + WARN(!crtc->primary, "Missing primary plane on [CRTC:%d:%s]\n", > + crtc->base.id, crtc->name); > + > if (crtc->primary) { > WARN(!(crtc->primary->possible_crtcs & > BIT(crtc->index)), >"Bogus primary plane possible_crtcs: [PLANE:%d:%s] > must be compatible with [CRTC:%d:%s]\n", > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 5d33ca9f0032..49b0a8b9ac02 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -57,7 +57,7 @@ > * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM > core > * relies on the driver to set the primary and optionally the cursor plane > used > * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). > All > - * drivers should provide one primary plane per CRTC to avoid surprising > legacy > + * drivers must provide one primary plane per CRTC to avoid surprising legacy > * userspace too much. > */ > pgpJi3Edy8G4I.pgp Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/4] drm: validate possible_crtcs for primary and cursor planes
On Fri, 11 Dec 2020 13:06:14 + Simon Ser wrote: > If a primary or cursor plane is not compatible with a CRTC it's attached > to via the legacy primary/cursor field, things will be broken for legacy > user-space. > > Signed-off-by: Simon Ser > Reviewed-by: Daniel Vetter > Cc: Pekka Paalanen Acked-by: Pekka Paalanen > --- > drivers/gpu/drm/drm_mode_config.c | 16 > 1 file changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index f1affc1bb679..2c73a60e8765 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -625,6 +625,7 @@ static void validate_encoder_possible_crtcs(struct > drm_encoder *encoder) > void drm_mode_config_validate(struct drm_device *dev) > { > struct drm_encoder *encoder; > + struct drm_crtc *crtc; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return; > @@ -636,4 +637,19 @@ void drm_mode_config_validate(struct drm_device *dev) > validate_encoder_possible_clones(encoder); > validate_encoder_possible_crtcs(encoder); > } > + > + drm_for_each_crtc(crtc, dev) { > + if (crtc->primary) { > + WARN(!(crtc->primary->possible_crtcs & > BIT(crtc->index)), > + "Bogus primary plane possible_crtcs: [PLANE:%d:%s] > must be compatible with [CRTC:%d:%s]\n", > + crtc->primary->base.id, crtc->primary->name, > + crtc->base.id, crtc->name); > + } > + if (crtc->cursor) { > + WARN(!(crtc->cursor->possible_crtcs & BIT(crtc->index)), > + "Bogus cursor plane possible_crtcs: [PLANE:%d:%s] > must be compatible with [CRTC:%d:%s]\n", > + crtc->cursor->base.id, crtc->cursor->name, > + crtc->base.id, crtc->name); > + } > + } > } pgpoRlw0vtTGR.pgp Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/4] drm: rework description of primary and cursor planes
On Fri, 11 Dec 2020 13:06:10 + Simon Ser wrote: > The previous wording could be understood by user-space evelopers as "a > primary/cursor plane is only compatible with a single CRTC" [1]. > > Reword the planes description to make it clear the DRM-internal > drm_crtc.primary and drm_crtc.cursor planes are for legacy uAPI. > > [1]: https://github.com/swaywm/wlroots/pull/2333#discussion_r456788057 > > Signed-off-by: Simon Ser > Reviewed-by: Daniel Vetter > Cc: Pekka Paalanen Acked-by: Pekka Paalanen > --- > drivers/gpu/drm/drm_crtc.c | 3 +++ > drivers/gpu/drm/drm_plane.c | 16 +--- > 2 files changed, 12 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > index 8d19d258547f..a6336c7154d6 100644 > --- a/drivers/gpu/drm/drm_crtc.c > +++ b/drivers/gpu/drm/drm_crtc.c > @@ -256,6 +256,9 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc > *crtc) > * planes). For really simple hardware which has only 1 plane look at > * drm_simple_display_pipe_init() instead. > * > + * The @primary and @cursor planes are only relevant for legacy uAPI, see > + * &drm_crtc.primary and &drm_crtc.cursor. > + * > * Returns: > * Zero on success, error code on failure. > */ > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 385801dd21f9..5d33ca9f0032 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -49,14 +49,16 @@ > * &struct drm_plane (possibly as part of a larger structure) and registers > it > * with a call to drm_universal_plane_init(). > * > - * Cursor and overlay planes are optional. All drivers should provide one > - * primary plane per CRTC to avoid surprising userspace too much. See enum > - * drm_plane_type for a more in-depth discussion of these special > uapi-relevant > - * plane types. Special planes are associated with their CRTC by calling > - * drm_crtc_init_with_planes(). > - * > * The type of a plane is exposed in the immutable "type" enumeration > property, > - * which has one of the following values: "Overlay", "Primary", "Cursor". > + * which has one of the following values: "Overlay", "Primary", "Cursor" (see > + * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see > + * &drm_plane.possible_crtcs. > + * > + * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM > core > + * relies on the driver to set the primary and optionally the cursor plane > used > + * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). > All > + * drivers should provide one primary plane per CRTC to avoid surprising > legacy > + * userspace too much. > */ > > static unsigned int drm_num_planes(struct drm_device *dev) pgpkRWnHYZUjQ.pgp Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
On Mon, 14 Dec 2020 21:57:25 +0100 Christian König wrote: > Am 11.12.20 um 13:20 schrieb Pekka Paalanen: > > On Fri, 11 Dec 2020 11:28:36 +0100 > > Christian König wrote: > > > >> Am 11.12.20 um 10:55 schrieb Pekka Paalanen: > >>> On Fri, 11 Dec 2020 09:56:07 +0530 > >>> Shashank Sharma wrote: > >>> > Hello Simon, > > Hope you are doing well, > > I was helping out Aurabindo and the team with the design, so I have > taken the liberty of adding some comments on behalf of the team, > Inline. > > On 11/12/20 3:31 am, Simon Ser wrote: > > Hi, > > > > (CC dri-devel, Pekka and Martin who might be interested in this as > > well.) > >>> Thanks for the Cc! This is very interesting to me, and because it was > >>> not Cc'd to dri-devel@ originally, I would have missed this otherwise. > >>> > > On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai > > wrote: > > > >> This patchset enables freesync video mode usecase where the userspace > >> can request a freesync compatible video mode such that switching to > >> this > >> mode does not trigger blanking. > >> > >> This feature is guarded by a module parameter which is disabled by > >> default. Enabling this paramters adds additional modes to the driver > >> modelist, and also enables the optimization to skip modeset when using > >> one of these modes. > > Thanks for working on this, it's an interesting feature! However I'd > > like to > > take some time to think about the user-space API for this. > > > > As I understand it, some new synthetic modes are added, and user-space > > can > > perform a test-only atomic *without* ALLOW_MODESET to figure out > > whether it can > > switch to a mode without blanking the screen. > The implementation is in those lines, but a bit different. The idea > is to: > > - check if the monitor supports VRR, > > - If it does, add some new modes which are in the VRR tolerance > range, as new video modes in the list (with driver flag). > > - when you get modeset on any of these modes, skip the full modeset, > and just adjust the front_porch timing > > so they are not test-only as such, for any user-space these modes > will be as real as any other probed modes of the list. > >>> But is it worth to allow a modeset to be glitch-free if the userspace > >>> does not know they are glitch-free? I think if this is going in, it > >>> would be really useful to give the guarantees to userspace explicitly, > >>> and not leave this feature at an "accidentally no glitch sometimes" > >>> level. > >>> > >>> > >>> I have been expecting and hoping for the ability to change video mode > >>> timings without a modeset ever since I learnt that VRR is about > >>> front-porch adjustment, quite a while ago. > >>> > >>> This is how I envision userspace making use of it: > >>> > >>> Let us have a Wayland compositor, which uses fixed-frequency video > >>> modes, because it wants predictable vblank cycles. IOW, it will not > >>> enable VRR as such. > >> Well in general please keep in mind that this is just a short term > >> solution for X11 applications. > > I guess someone should have mentioned that. :-) > > > > Do we really want to add more Xorg-only features in the kernel? > > Well, my preferred solution was to add the mode in userspace instead :) > > > It feels like "it's a short term solution for X11" could be almost used > > as an excuse to avoid proper uAPI design process. However, with uAPI > > there is no "short term". Once it's in, it's there for decades. So why > > does it matter if you design it for Xorg foremost? Are others forbidden > > to make use of it? Or do you deliberately want to design it so that > > it's not generally useful and it will indeed end up being a short term > > feature? Planned obsolescence from the start? > > Yes exactly. We need something which works for now and can be removed > again later on when we have a better solution. Adding some extra modes > is not considered UAPI since both displays and drivers are doing this > for a couple of different purposes already. > > Another main reason is that this approach works with existing media > players like mpv and kodi without changing them because we do pretty > much the same thing in the kernel which TVs do in their EDID. > > E.g. when starting to play a video kodi will automatically try to switch > to a mode which has the same fps as the video. Aha! That is very valuable information to put this proposal into perspective. I'll leave you to it, then. :-) Thanks, pq pgpTRXG0_9guM.pgp Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [Intel-gfx] [PATCH] drm/i915/display/tc: Only WARN once for bogus tc port flag
On Wed, 09 Dec 2020, Rodrigo Vivi wrote: > On Wed, Dec 09, 2020 at 04:16:36PM -0500, Sean Paul wrote: >> From: Sean Paul >> >> No need to spam syslog/console when we can ignore/fix the flag. > > besides that we are calling from multiple places anyway.. > >> >> Signed-off-by: Sean Paul > > > Reviewed-by: Rodrigo Vivi Thanks, pushed to din. BR, Jani. > > > >> --- >> drivers/gpu/drm/i915/display/intel_tc.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_tc.c >> b/drivers/gpu/drm/i915/display/intel_tc.c >> index 4346bc1a747a..27dc2dad6809 100644 >> --- a/drivers/gpu/drm/i915/display/intel_tc.c >> +++ b/drivers/gpu/drm/i915/display/intel_tc.c >> @@ -262,7 +262,7 @@ static u32 tc_port_live_status_mask(struct >> intel_digital_port *dig_port) >> mask |= BIT(TC_PORT_LEGACY); >> >> /* The sink can be connected only in a single mode. */ >> -if (!drm_WARN_ON(&i915->drm, hweight32(mask) > 1)) >> +if (!drm_WARN_ON_ONCE(&i915->drm, hweight32(mask) > 1)) >> tc_port_fixup_legacy_flag(dig_port, mask); >> >> return mask; >> -- >> Sean Paul, Software Engineer, Google / Chromium OS >> >> ___ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > ___ > Intel-gfx mailing list > intel-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Graphics Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/3] drm: Add support for the LogiCVC display controller
Hi, On Mon 07 Dec 20, 11:42, Maxime Ripard wrote: > On Wed, Dec 02, 2020 at 05:06:40PM +0100, Paul Kocialkowski wrote: > > > > +static void logicvc_crtc_atomic_begin(struct drm_crtc *drm_crtc, > > > > + struct drm_atomic_state *state) > > > > +{ > > > > + struct logicvc_crtc *crtc = logicvc_crtc(drm_crtc); > > > > + struct drm_crtc_state *crtc_state = > > > > + drm_atomic_get_old_crtc_state(state, drm_crtc); > > > > + struct drm_device *drm_dev = drm_crtc->dev; > > > > + unsigned long flags; > > > > + > > > > + /* Register pending event, only if vblank is already on. */ > > > > + if (drm_crtc->state->event && crtc_state->active) { > > > > + spin_lock_irqsave(&drm_dev->event_lock, flags); > > > > + WARN_ON(drm_crtc_vblank_get(drm_crtc) != 0); > > > > + > > > > + crtc->event = drm_crtc->state->event; > > > > + drm_crtc->state->event = NULL; > > > > + > > > > + spin_unlock_irqrestore(&drm_dev->event_lock, flags); > > > > + } > > > > +} > > > > > > That's unusual to do it in atomic_begin, why do you need it? > > > > This is to cover the case where we need to send a page flip event but the > > crtc is already on. In that case, neither atomic_enable nor atomic_disable > > will be called so we need to rely on atomic_begin to grab that event. > > This happens for example when a single plane is updated. > > > > The same thing is done in e.g. sun4i-drm. > > Yeah, but I'm not sure why that's needed in the first place on sun4i-drm > either. This looks to me as either something that should be handled by > the helpers, or isn't needed at all. Just like the other times you > fiddle with the vblank in your driver. I didn't really question myself about whether this could be done in helpers, but it looks like the philosophy now is that the driver grabs the page flip done event when it can and serves the event in the IRQ routine. So nothing unusual about this driver in particular. > I looked around and the only drivers that have that logic seem to be ARM > HDLCD, Atmel HCLDC, Meson, Tegra. This looks like it might be some cargo > cult. > > Daniel, do you know why that would be needed? As far as I understand, this could work just as well with a helper in my case (and sun4i-drm's case as well). But in any case, what this patch implements is the current philosophy and I guess that reworking it through helpers is way out of the scope of this series ;) > > > > +static void logicvc_version_print(struct logicvc_drm *logicvc) > > > > +{ > > > > + u32 version; > > > > + > > > > + regmap_read(logicvc->regmap, LOGICVC_IP_VERSION_REG, &version); > > > > + > > > > + DRM_INFO("LogiCVC version %d.%02d.%c\n", > > > > +(int)LOGICVC_FIELD_GET(LOGICVC_IP_VERSION_MAJOR, > > > > version), > > > > +(int)LOGICVC_FIELD_GET(LOGICVC_IP_VERSION_MINOR, > > > > version), > > > > +(char)LOGICVC_FIELD_GET(LOGICVC_IP_VERSION_LEVEL, > > > > version) + > > > > +'a'); > > > > > > DRM_DEV_INFO? > > > > Okay but now according to Sam, "DRM_DEV_ERROR() and friends are deprecated" > > so I wonder which is the right one to use at this point. > > AFAIU, it's drm_info / drm_err Thanks! > > > > +static void logicvc_encoder_enable(struct drm_encoder *drm_encoder) > > > > +{ > > > > + struct logicvc_drm *logicvc = logicvc_drm(drm_encoder->dev); > > > > + struct logicvc_interface *interface = > > > > + logicvc_interface_from_drm_encoder(drm_encoder); > > > > + > > > > + regmap_update_bits(logicvc->regmap, LOGICVC_POWER_CTRL_REG, > > > > + LOGICVC_POWER_CTRL_VIDEO_ENABLE, > > > > + LOGICVC_POWER_CTRL_VIDEO_ENABLE); > > > > + > > > > + if (interface->drm_panel) { > > > > + drm_panel_prepare(interface->drm_panel); > > > > + > > > > + /* Encoder enable is too early to enable the panel and > > > > a white > > > > +* screen will be seen if the panel gets enabled before > > > > the > > > > +* first page flip is done (and no other framebuffer > > > > +* configuration remains from the boot software). */ > > > > + interface->drm_panel_enabled = false; > > > > + } > > > > +} > > > > > > That's fishy (and the similar stuff in commit_tail). Is it because you > > > need to have the CRTC powered before the encoder? > > > > > > If so, you should try the commit_tail_rpm variant, it makes sure the > > > CRTC is powered on before making a commit. > > > > No, this is unrelated to CRTC vs encoder enable order. Instead, it's about > > panel enable order: I don't want to enable the panel before a buffer was > > flipped on the CRTC otherwise a blank/white/garbage screen will be shown. > > Well, since the encoder will enable the panel, it's kind of re
Re: [PATCH v3 4/4] drm: require each CRTC to have a unique primary plane
On Fri, Dec 11, 2020 at 2:08 PM Simon Ser wrote: > > User-space expects to be able to pick a primary plane for each CRTC > exposed by the driver. Make sure this assumption holds in > drm_mode_config_validate. > > Use the legacy drm_crtc.primary field to check this, because it's > simpler and we require drivers to set it anyways. Accumulate a set of > primary planes which are already used for a CRTC in a bitmask. Error out > if a primary plane is re-used. > > v2: new patch > > v3: > - Use u64 instead of __u64 (Jani) > - Use `unsigned int` instead of `unsigned` (Jani) > > Signed-off-by: Simon Ser > Cc: Daniel Vetter > Cc: Pekka Paalanen > Cc: Jani Nikula Yeah makes sense to also check this. Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/drm_mode_config.c | 21 + > drivers/gpu/drm/drm_plane.c | 6 ++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index fbe680035129..c5cf5624c106 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -626,6 +626,9 @@ void drm_mode_config_validate(struct drm_device *dev) > { > struct drm_encoder *encoder; > struct drm_crtc *crtc; > + struct drm_plane *plane; > + u64 primary_with_crtc = 0, cursor_with_crtc = 0; > + unsigned int num_primary = 0; > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return; > @@ -647,12 +650,30 @@ void drm_mode_config_validate(struct drm_device *dev) > "Bogus primary plane possible_crtcs: > [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", > crtc->primary->base.id, crtc->primary->name, > crtc->base.id, crtc->name); > + WARN(primary_with_crtc & BIT(crtc->primary->index), > +"Primary plane [PLANE:%d:%s] used for two CRTCs", > +crtc->primary->base.id, crtc->primary->name); > + primary_with_crtc |= BIT(crtc->primary->index); > } > if (crtc->cursor) { > WARN(!(crtc->cursor->possible_crtcs & > BIT(crtc->index)), > "Bogus cursor plane possible_crtcs: > [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", > crtc->cursor->base.id, crtc->cursor->name, > crtc->base.id, crtc->name); > + WARN(cursor_with_crtc & BIT(crtc->cursor->index), > +"Primary plane [PLANE:%d:%s] used for two CRTCs", > +crtc->cursor->base.id, crtc->cursor->name); > + cursor_with_crtc |= BIT(crtc->cursor->index); > } > } > + > + drm_for_each_plane(plane, dev) { > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { > + num_primary++; > + } > + } > + > + WARN(num_primary != dev->mode_config.num_crtc, > +"Must have as many primary planes as there are CRTCs, but have > %u primary planes and %u CRTCs", > +num_primary, dev->mode_config.num_crtc); > } > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 49b0a8b9ac02..a1f4510efa83 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -54,6 +54,12 @@ > * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see > * &drm_plane.possible_crtcs. > * > + * Each CRTC must have a unique primary plane userspace can attach to enable > + * the CRTC. In other words, userspace must be able to attach a different > + * primary plane to each CRTC at the same time. Primary planes can still be > + * compatible with multiple CRTCs. There must be exactly as many primary > planes > + * as there are CRTCs. > + * > * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM > core > * relies on the driver to set the primary and optionally the cursor plane > used > * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). > All > -- > 2.29.2 > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 4/4] drm: require each CRTC to have a unique primary plane
User-space expects to be able to pick a primary plane for each CRTC exposed by the driver. Make sure this assumption holds in drm_mode_config_validate. Use the legacy drm_crtc.primary field to check this, because it's simpler and we require drivers to set it anyways. Accumulate a set of primary planes which are already used for a CRTC in a bitmask. Error out if a primary plane is re-used. v2: new patch v3: - Use u64 instead of __u64 (Jani) - Use `unsigned int` instead of `unsigned` (Jani) Signed-off-by: Simon Ser Cc: Daniel Vetter Cc: Pekka Paalanen Cc: Jani Nikula --- drivers/gpu/drm/drm_mode_config.c | 21 + drivers/gpu/drm/drm_plane.c | 6 ++ 2 files changed, 27 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index fbe680035129..c5cf5624c106 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -626,6 +626,9 @@ void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder; struct drm_crtc *crtc; + struct drm_plane *plane; + u64 primary_with_crtc = 0, cursor_with_crtc = 0; + unsigned int num_primary = 0; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return; @@ -647,12 +650,30 @@ void drm_mode_config_validate(struct drm_device *dev) "Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", crtc->primary->base.id, crtc->primary->name, crtc->base.id, crtc->name); + WARN(primary_with_crtc & BIT(crtc->primary->index), +"Primary plane [PLANE:%d:%s] used for two CRTCs", +crtc->primary->base.id, crtc->primary->name); + primary_with_crtc |= BIT(crtc->primary->index); } if (crtc->cursor) { WARN(!(crtc->cursor->possible_crtcs & BIT(crtc->index)), "Bogus cursor plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", crtc->cursor->base.id, crtc->cursor->name, crtc->base.id, crtc->name); + WARN(cursor_with_crtc & BIT(crtc->cursor->index), +"Primary plane [PLANE:%d:%s] used for two CRTCs", +crtc->cursor->base.id, crtc->cursor->name); + cursor_with_crtc |= BIT(crtc->cursor->index); } } + + drm_for_each_plane(plane, dev) { + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { + num_primary++; + } + } + + WARN(num_primary != dev->mode_config.num_crtc, +"Must have as many primary planes as there are CRTCs, but have %u primary planes and %u CRTCs", +num_primary, dev->mode_config.num_crtc); } diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 49b0a8b9ac02..a1f4510efa83 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -54,6 +54,12 @@ * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see * &drm_plane.possible_crtcs. * + * Each CRTC must have a unique primary plane userspace can attach to enable + * the CRTC. In other words, userspace must be able to attach a different + * primary plane to each CRTC at the same time. Primary planes can still be + * compatible with multiple CRTCs. There must be exactly as many primary planes + * as there are CRTCs. + * * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core * relies on the driver to set the primary and optionally the cursor plane used * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/4] drm: validate possible_crtcs for primary and cursor planes
If a primary or cursor plane is not compatible with a CRTC it's attached to via the legacy primary/cursor field, things will be broken for legacy user-space. Signed-off-by: Simon Ser Reviewed-by: Daniel Vetter Cc: Pekka Paalanen --- drivers/gpu/drm/drm_mode_config.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index f1affc1bb679..2c73a60e8765 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -625,6 +625,7 @@ static void validate_encoder_possible_crtcs(struct drm_encoder *encoder) void drm_mode_config_validate(struct drm_device *dev) { struct drm_encoder *encoder; + struct drm_crtc *crtc; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return; @@ -636,4 +637,19 @@ void drm_mode_config_validate(struct drm_device *dev) validate_encoder_possible_clones(encoder); validate_encoder_possible_crtcs(encoder); } + + drm_for_each_crtc(crtc, dev) { + if (crtc->primary) { + WARN(!(crtc->primary->possible_crtcs & BIT(crtc->index)), +"Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", +crtc->primary->base.id, crtc->primary->name, +crtc->base.id, crtc->name); + } + if (crtc->cursor) { + WARN(!(crtc->cursor->possible_crtcs & BIT(crtc->index)), +"Bogus cursor plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", +crtc->cursor->base.id, crtc->cursor->name, +crtc->base.id, crtc->name); + } + } } -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 3/4] drm: require a non_NULL drm_crtc.primary
If a CRTC is missing a legacy primary plane pointer, a lot of things will be broken for user-space: fbdev stops working and the entire legacy uAPI stops working. Require all drivers to populate drm_crtc.primary to prevent these issues. Warn if it's NULL. Signed-off-by: Simon Ser Reviewed-by: Daniel Vetter Cc: Pekka Paalanen --- drivers/gpu/drm/drm_mode_config.c | 3 +++ drivers/gpu/drm/drm_plane.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 2c73a60e8765..fbe680035129 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -639,6 +639,9 @@ void drm_mode_config_validate(struct drm_device *dev) } drm_for_each_crtc(crtc, dev) { + WARN(!crtc->primary, "Missing primary plane on [CRTC:%d:%s]\n", +crtc->base.id, crtc->name); + if (crtc->primary) { WARN(!(crtc->primary->possible_crtcs & BIT(crtc->index)), "Bogus primary plane possible_crtcs: [PLANE:%d:%s] must be compatible with [CRTC:%d:%s]\n", diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 5d33ca9f0032..49b0a8b9ac02 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -57,7 +57,7 @@ * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core * relies on the driver to set the primary and optionally the cursor plane used * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All - * drivers should provide one primary plane per CRTC to avoid surprising legacy + * drivers must provide one primary plane per CRTC to avoid surprising legacy * userspace too much. */ -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 1/4] drm: rework description of primary and cursor planes
The previous wording could be understood by user-space evelopers as "a primary/cursor plane is only compatible with a single CRTC" [1]. Reword the planes description to make it clear the DRM-internal drm_crtc.primary and drm_crtc.cursor planes are for legacy uAPI. [1]: https://github.com/swaywm/wlroots/pull/2333#discussion_r456788057 Signed-off-by: Simon Ser Reviewed-by: Daniel Vetter Cc: Pekka Paalanen --- drivers/gpu/drm/drm_crtc.c | 3 +++ drivers/gpu/drm/drm_plane.c | 16 +--- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 8d19d258547f..a6336c7154d6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -256,6 +256,9 @@ struct dma_fence *drm_crtc_create_fence(struct drm_crtc *crtc) * planes). For really simple hardware which has only 1 plane look at * drm_simple_display_pipe_init() instead. * + * The @primary and @cursor planes are only relevant for legacy uAPI, see + * &drm_crtc.primary and &drm_crtc.cursor. + * * Returns: * Zero on success, error code on failure. */ diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 385801dd21f9..5d33ca9f0032 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -49,14 +49,16 @@ * &struct drm_plane (possibly as part of a larger structure) and registers it * with a call to drm_universal_plane_init(). * - * Cursor and overlay planes are optional. All drivers should provide one - * primary plane per CRTC to avoid surprising userspace too much. See enum - * drm_plane_type for a more in-depth discussion of these special uapi-relevant - * plane types. Special planes are associated with their CRTC by calling - * drm_crtc_init_with_planes(). - * * The type of a plane is exposed in the immutable "type" enumeration property, - * which has one of the following values: "Overlay", "Primary", "Cursor". + * which has one of the following values: "Overlay", "Primary", "Cursor" (see + * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see + * &drm_plane.possible_crtcs. + * + * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM core + * relies on the driver to set the primary and optionally the cursor plane used + * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). All + * drivers should provide one primary plane per CRTC to avoid surprising legacy + * userspace too much. */ static unsigned int drm_num_planes(struct drm_device *dev) -- 2.29.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
Am 11.12.20 um 13:20 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: Am 11.12.20 um 10:55 schrieb Pekka Paalanen: On Fri, 11 Dec 2020 09:56:07 +0530 Shashank Sharma wrote: Hello Simon, Hope you are doing well, I was helping out Aurabindo and the team with the design, so I have taken the liberty of adding some comments on behalf of the team, Inline. On 11/12/20 3:31 am, Simon Ser wrote: Hi, (CC dri-devel, Pekka and Martin who might be interested in this as well.) Thanks for the Cc! This is very interesting to me, and because it was not Cc'd to dri-devel@ originally, I would have missed this otherwise. On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai wrote: This patchset enables freesync video mode usecase where the userspace can request a freesync compatible video mode such that switching to this mode does not trigger blanking. This feature is guarded by a module parameter which is disabled by default. Enabling this paramters adds additional modes to the driver modelist, and also enables the optimization to skip modeset when using one of these modes. Thanks for working on this, it's an interesting feature! However I'd like to take some time to think about the user-space API for this. As I understand it, some new synthetic modes are added, and user-space can perform a test-only atomic *without* ALLOW_MODESET to figure out whether it can switch to a mode without blanking the screen. The implementation is in those lines, but a bit different. The idea is to: - check if the monitor supports VRR, - If it does, add some new modes which are in the VRR tolerance range, as new video modes in the list (with driver flag). - when you get modeset on any of these modes, skip the full modeset, and just adjust the front_porch timing so they are not test-only as such, for any user-space these modes will be as real as any other probed modes of the list. But is it worth to allow a modeset to be glitch-free if the userspace does not know they are glitch-free? I think if this is going in, it would be really useful to give the guarantees to userspace explicitly, and not leave this feature at an "accidentally no glitch sometimes" level. I have been expecting and hoping for the ability to change video mode timings without a modeset ever since I learnt that VRR is about front-porch adjustment, quite a while ago. This is how I envision userspace making use of it: Let us have a Wayland compositor, which uses fixed-frequency video modes, because it wants predictable vblank cycles. IOW, it will not enable VRR as such. Well in general please keep in mind that this is just a short term solution for X11 applications. I guess someone should have mentioned that. :-) Do we really want to add more Xorg-only features in the kernel? Well, my preferred solution was to add the mode in userspace instead :) It feels like "it's a short term solution for X11" could be almost used as an excuse to avoid proper uAPI design process. However, with uAPI there is no "short term". Once it's in, it's there for decades. So why does it matter if you design it for Xorg foremost? Are others forbidden to make use of it? Or do you deliberately want to design it so that it's not generally useful and it will indeed end up being a short term feature? Planned obsolescence from the start? Yes exactly. We need something which works for now and can be removed again later on when we have a better solution. Adding some extra modes is not considered UAPI since both displays and drivers are doing this for a couple of different purposes already. Another main reason is that this approach works with existing media players like mpv and kodi without changing them because we do pretty much the same thing in the kernel which TVs do in their EDID. E.g. when starting to play a video kodi will automatically try to switch to a mode which has the same fps as the video. For things like Wayland we probably want to approach this from a completely different vector. When the Wayland compositor starts, it will choose *some* video mode for an output. It may or may not be what a KMS driver calls "preferred mode", because it depends on things like user preferences. The compositor makes the initial modeset to this mode. I think the general idea we settled on is that we specify an earliest display time for each frame and give feedback to the application when a frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. X11 already supports that with the DRI3 extension. Also see VDPAU present and the Vulkan extension for reference application API implementations, so that stuff is already present. It's just not wired up correctly with KMS and we don't have anything similar in Wayland as far as I know.
Re: [PATCH v2 4/4] drm: require each CRTC to have a unique primary plane
On Fri, 11 Dec 2020, Simon Ser wrote: > User-space expects to be able to pick a primary plane for each CRTC > exposed by the driver. Make sure this assumption holds in > drm_mode_config_validate. > > Use the legacy drm_crtc.primary field to check this, because it's > simpler and we require drivers to set it anyways. Accumulate a set of > primary planes which are already used for a CRTC in a bitmask. Error out > if a primary plane is re-used. > > Signed-off-by: Simon Ser > Cc: Daniel Vetter > Cc: Pekka Paalanen > --- > drivers/gpu/drm/drm_mode_config.c | 21 + > drivers/gpu/drm/drm_plane.c | 6 ++ > 2 files changed, 27 insertions(+) > > diff --git a/drivers/gpu/drm/drm_mode_config.c > b/drivers/gpu/drm/drm_mode_config.c > index fbe680035129..f143f56f0820 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -626,6 +626,9 @@ void drm_mode_config_validate(struct drm_device *dev) > { > struct drm_encoder *encoder; > struct drm_crtc *crtc; > + struct drm_plane *plane; > + __u64 primary_with_crtc = 0, cursor_with_crtc = 0; Why __u64 and not u64? > + unsigned num_primary = 0; I think the preference is to spell out the full type instead of using just "unsigned". BR, Jani. > > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return; > @@ -647,12 +650,30 @@ void drm_mode_config_validate(struct drm_device *dev) >"Bogus primary plane possible_crtcs: [PLANE:%d:%s] > must be compatible with [CRTC:%d:%s]\n", >crtc->primary->base.id, crtc->primary->name, >crtc->base.id, crtc->name); > + WARN(primary_with_crtc & BIT(crtc->primary->index), > + "Primary plane [PLANE:%d:%s] used for two CRTCs", > + crtc->primary->base.id, crtc->primary->name); > + primary_with_crtc |= BIT(crtc->primary->index); > } > if (crtc->cursor) { > WARN(!(crtc->cursor->possible_crtcs & BIT(crtc->index)), >"Bogus cursor plane possible_crtcs: [PLANE:%d:%s] > must be compatible with [CRTC:%d:%s]\n", >crtc->cursor->base.id, crtc->cursor->name, >crtc->base.id, crtc->name); > + WARN(cursor_with_crtc & BIT(crtc->cursor->index), > + "Primary plane [PLANE:%d:%s] used for two CRTCs", > + crtc->cursor->base.id, crtc->cursor->name); > + cursor_with_crtc |= BIT(crtc->cursor->index); > } > } > + > + drm_for_each_plane(plane, dev) { > + if (plane->type == DRM_PLANE_TYPE_PRIMARY) { > + num_primary++; > + } > + } > + > + WARN(num_primary != dev->mode_config.num_crtc, > + "Must have as many primary planes as there are CRTCs, but have %u > primary planes and %u CRTCs", > + num_primary, dev->mode_config.num_crtc); > } > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 49b0a8b9ac02..a1f4510efa83 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -54,6 +54,12 @@ > * enum drm_plane_type). A plane can be compatible with multiple CRTCs, see > * &drm_plane.possible_crtcs. > * > + * Each CRTC must have a unique primary plane userspace can attach to enable > + * the CRTC. In other words, userspace must be able to attach a different > + * primary plane to each CRTC at the same time. Primary planes can still be > + * compatible with multiple CRTCs. There must be exactly as many primary > planes > + * as there are CRTCs. > + * > * Legacy uAPI doesn't expose the primary and cursor planes directly. DRM > core > * relies on the driver to set the primary and optionally the cursor plane > used > * for legacy IOCTLs. This is done by calling drm_crtc_init_with_planes(). > All -- Jani Nikula, Intel Open Source Graphics Center ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/8] drm/vc4: dsi: Correct DSI register definition
Hi, On Tue, Dec 08, 2020 at 09:34:05AM +0100, Frieder Schrempf wrote: > On 03.12.20 14:25, Maxime Ripard wrote: > > From: Dave Stevenson > > > > The DSI1_PHY_AFEC0_PD_DLANE1 and DSI1_PHY_AFEC0_PD_DLANE3 register > > definitions were swapped, so trying to use more than a single data > > lane failed as lane 1 would get powered down. > > (In theory a 4 lane device would work as all lanes would remain > > powered). > > > > Correct the definitions. > > > > Signed-off-by: Dave Stevenson > > Signed-off-by: Maxime Ripard > > Wouldn't this deserve a "Fixes: ..." and "Cc: sta...@vger.kernel.org" tag, > as this bug is present in all stable releases since this driver was > introduced? I think it would be really helpful to have it backported. Sorry I forgot it. The patch is applied however and drm-misc-next doesn't get rebased, so I can't add it now. We can always send it for stable by hand though once it's in Linus' tree Maxime signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH -next] gpu: drm: i915: convert comma to semicolon
Replace a comma between expression statements by a semicolon. Signed-off-by: Zheng Yongjun --- drivers/gpu/drm/i915/display/intel_hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 3f2008d845c2..9737a8604fc7 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -1323,8 +1323,8 @@ static int intel_hdmi_hdcp_write(struct intel_digital_port *dig_port, memcpy(&write_buf[1], buffer, size); msg.addr = DRM_HDCP_DDC_ADDR; - msg.flags = 0, - msg.len = size + 1, + msg.flags = 0; + msg.len = size + 1; msg.buf = write_buf; ret = i2c_transfer(adapter, &msg, 1); -- 2.22.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH -next] gpu: drm: imx: convert comma to semicolon
Replace a comma between expression statements by a semicolon. Signed-off-by: Zheng Yongjun --- drivers/gpu/drm/imx/parallel-display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index 2eb8df4697df..c4dab79a9385 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -74,7 +74,7 @@ static int imx_pd_connector_get_modes(struct drm_connector *connector) return ret; drm_mode_copy(mode, &imxpd->mode); - mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, + mode->type |= DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; drm_mode_probed_add(connector, mode); num_modes++; } -- 2.22.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH -next] gpu: drm: vmwgfx: convert comma to semicolon
Replace a comma between expression statements by a semicolon. Signed-off-by: Zheng Yongjun --- drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c index e67e2e8f6e6f..537c48eff197 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c @@ -2509,7 +2509,7 @@ static int vmw_cmd_dx_set_so_targets(struct vmw_private *dev_priv, binding.bi.ctx = ctx_node->ctx; binding.bi.res = res; - binding.bi.bt = vmw_ctx_binding_so_target, + binding.bi.bt = vmw_ctx_binding_so_target; binding.offset = cmd->targets[i].offset; binding.size = cmd->targets[i].sizeInBytes; binding.slot = i; -- 2.22.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 0/3] Experimental freesync video mode optimization
On Fri, 11 Dec 2020 11:28:36 +0100 Christian König wrote: > Am 11.12.20 um 10:55 schrieb Pekka Paalanen: > > On Fri, 11 Dec 2020 09:56:07 +0530 > > Shashank Sharma wrote: > > > >> Hello Simon, > >> > >> Hope you are doing well, > >> > >> I was helping out Aurabindo and the team with the design, so I have > >> taken the liberty of adding some comments on behalf of the team, > >> Inline. > >> > >> On 11/12/20 3:31 am, Simon Ser wrote: > >>> Hi, > >>> > >>> (CC dri-devel, Pekka and Martin who might be interested in this as > >>> well.) > > Thanks for the Cc! This is very interesting to me, and because it was > > not Cc'd to dri-devel@ originally, I would have missed this otherwise. > > > >>> On Thursday, December 10th, 2020 at 7:48 PM, Aurabindo Pillai > >>> wrote: > >>> > This patchset enables freesync video mode usecase where the userspace > can request a freesync compatible video mode such that switching to this > mode does not trigger blanking. > > This feature is guarded by a module parameter which is disabled by > default. Enabling this paramters adds additional modes to the driver > modelist, and also enables the optimization to skip modeset when using > one of these modes. > >>> Thanks for working on this, it's an interesting feature! However I'd like > >>> to > >>> take some time to think about the user-space API for this. > >>> > >>> As I understand it, some new synthetic modes are added, and user-space can > >>> perform a test-only atomic *without* ALLOW_MODESET to figure out whether > >>> it can > >>> switch to a mode without blanking the screen. > >> The implementation is in those lines, but a bit different. The idea > >> is to: > >> > >> - check if the monitor supports VRR, > >> > >> - If it does, add some new modes which are in the VRR tolerance > >> range, as new video modes in the list (with driver flag). > >> > >> - when you get modeset on any of these modes, skip the full modeset, > >> and just adjust the front_porch timing > >> > >> so they are not test-only as such, for any user-space these modes > >> will be as real as any other probed modes of the list. > > But is it worth to allow a modeset to be glitch-free if the userspace > > does not know they are glitch-free? I think if this is going in, it > > would be really useful to give the guarantees to userspace explicitly, > > and not leave this feature at an "accidentally no glitch sometimes" > > level. > > > > > > I have been expecting and hoping for the ability to change video mode > > timings without a modeset ever since I learnt that VRR is about > > front-porch adjustment, quite a while ago. > > > > This is how I envision userspace making use of it: > > > > Let us have a Wayland compositor, which uses fixed-frequency video > > modes, because it wants predictable vblank cycles. IOW, it will not > > enable VRR as such. > > Well in general please keep in mind that this is just a short term > solution for X11 applications. I guess someone should have mentioned that. :-) Do we really want to add more Xorg-only features in the kernel? It feels like "it's a short term solution for X11" could be almost used as an excuse to avoid proper uAPI design process. However, with uAPI there is no "short term". Once it's in, it's there for decades. So why does it matter if you design it for Xorg foremost? Are others forbidden to make use of it? Or do you deliberately want to design it so that it's not generally useful and it will indeed end up being a short term feature? Planned obsolescence from the start? > For things like Wayland we probably want to approach this from a > completely different vector. > > > When the Wayland compositor starts, it will choose *some* video mode > > for an output. It may or may not be what a KMS driver calls "preferred > > mode", because it depends on things like user preferences. The > > compositor makes the initial modeset to this mode. > > I think the general idea we settled on is that we specify an earliest > display time for each frame and give feedback to the application when a > frame was actually displayed. That is indeed something completely different, and feels like it would be several years in the future, while the proposed scheme or the min/max properties could be done fairly quickly. But I'm not in a hurry. Setting the earliest display time for a flip does not fully cover what video mode timing adjustment or min/max frame time or refresh rate property would offer: prediction of when the next flip can happen. Choosing display times requires knowing the possible display times, so something more is needed. The min/max properties would fit in. > This approach should also be able to handle multiple applications with > different refresh rates. E.g. just think of a video playback with 25 and > another one with 30 Hz in two windows when the max refresh rate is > something like 120Hz. But that's just an extension to what I already
Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
Thank you for the reviews Greg, Christian and Daniel! On Thu, Dec 10, 2020 at 1:59 AM Christian König wrote: > > In general a good idea, but I have a few concern/comments here. > > Am 10.12.20 um 05:43 schrieb Hridya Valsaraju: > > This patch allows statistics to be enabled for each DMA-BUF in > > sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > > > > The following stats will be exposed by the interface: > > > > /sys/kernel/dmabuf//exporter_name > > /sys/kernel/dmabuf//size > > /sys/kernel/dmabuf//dev_map_info > > > > The inode_number is unique for each DMA-BUF and was added earlier [1] > > in order to allow userspace to track DMA-BUF usage across different > > processes. > > > > Currently, this information is exposed in > > /sys/kernel/debug/dma_buf/bufinfo. > > However, since debugfs is considered unsafe to be mounted in production, > > it is being duplicated in sysfs. > > Mhm, this makes it part of the UAPI. What is the justification for this? > > In other words do we really need those debug information in a production > environment? Yes, we currently collect this information on production devices as well. > > > > > This information is intended to help with root-causing > > low-memory kills and the debugging/analysis of other memory-related issues. > > > > It will also be used to derive DMA-BUF > > per-exporter stats and per-device usage stats for Android Bug reports. > > > > [1]: > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1088791%2F&data=04%7C01%7Cchristian.koenig%40amd.com%7C30a0e015502b4d20e18208d89cc63f1a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431722574983797%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=RdGMvj5VsFUwJcVOuSPaLuAr4eI3CR1YOaznupmpTqg%3D&reserved=0 > > > > Signed-off-by: Hridya Valsaraju > > --- > > Documentation/ABI/testing/sysfs-kernel-dmabuf | 32 > > drivers/dma-buf/Kconfig | 11 ++ > > drivers/dma-buf/Makefile | 1 + > > drivers/dma-buf/dma-buf-sysfs-stats.c | 162 ++ > > drivers/dma-buf/dma-buf-sysfs-stats.h | 37 > > drivers/dma-buf/dma-buf.c | 29 > > include/linux/dma-buf.h | 13 ++ > > 7 files changed, 285 insertions(+) > > create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c > > create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h > > > > diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf > > b/Documentation/ABI/testing/sysfs-kernel-dmabuf > > new file mode 100644 > > index ..02d407d57aaa > > --- /dev/null > > +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf > > @@ -0,0 +1,32 @@ > > +What:/sys/kernel/dmabuf > > +Date:November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: The /sys/kernel/dmabuf directory contains a > > + snapshot of the internal state of every DMA-BUF. > > + /sys/kernel/dmabuf/ will contain the > > + statistics for the DMA-BUF with the unique inode number > > + > > +Users: kernel memory tuning/debugging tools > > + > > +What:/sys/kernel/dmabuf//exporter_name > > +Date:November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: This file is read-only and contains the name of the exporter > > of > > + the DMA-BUF. > > + > > +What:/sys/kernel/dmabuf//size > > +Dat: November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: This file is read-only and specifies the size of the DMA-BUF > > in > > + bytes. > > + > > +What:/sys/kernel/dmabuf//dev_map_info > > +Dat: November 2020 > > +KernelVersion: v5.11 > > +Contact: Hridya Valsaraju > > +Description: This file is read-only and lists the name of devices currently > > + mapping the DMA-BUF in a space-separated format. > > + > > diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig > > index 4f8224a6ac95..2fed26f14548 100644 > > --- a/drivers/dma-buf/Kconfig > > +++ b/drivers/dma-buf/Kconfig > > @@ -64,6 +64,17 @@ menuconfig DMABUF_HEAPS > > allows userspace to allocate dma-bufs that can be shared > > between drivers. > > > > +menuconfig DMABUF_SYSFS_STATS > > + bool "DMA-BUF sysfs statistics" > > + select DMA_SHARED_BUFFER > > + help > > +Choose this option to enable DMA-BUF sysfs statistics > > +in location /sys/kernel/dmabuf. > > + > > +/sys/kernel/dmabuf/ will contain > > +statistics for the DMA-BUF with the unique inode number > > +. > > + > > source "drivers/dma-buf/heaps/Kconfig" > > > > endmen
Re: [PATCH] drm/amdgpu: make DRM_AMD_DC x86-only again
On Tue, Dec 8, 2020 at 2:18 PM Arnd Bergmann wrote: > > On Tue, Dec 8, 2020 at 7:21 PM 'Nick Desaulniers' via Clang Built > Linux wrote: > > > > On Tue, Dec 8, 2020 at 6:26 AM Arnd Bergmann wrote: > > > > > > On Mon, Dec 7, 2020 at 11:28 PM 'Nick Desaulniers' via Clang Built > > > Linux wrote: > > Hmm...no warnings for me with that config on next-20201208: > > $ make LLVM=1 -j71 olddefconfig > > $ make LLVM=1 -j71 > > ... > > $ clang --version > > clang version 12.0.0 (g...@github.com:llvm/llvm-project.git > > 1c98f984105e552daa83ed8e92c61fba0e401410) > > (ie. near ToT LLVM) > > > > I see CONFIG_KASAN=y in the .config, so that's a recurring theme with > > clang; excessive stack usage. It does seem a shame to disable a > > driver for a bunch of archs just due to KASAN stack usage. > > $ grep KASAN=y 0x9077925C_defconfig > > CONFIG_HAVE_ARCH_KASAN=y > > CONFIG_KASAN=y > > > > Is there a different branch of a different tree that I should be > > testing on instead? Otherwise, can you send the object file? > > I was on a slightly older linux-next, and the latest version contains > the patch "ubsan: enable for all*config builds" in linux-next, > which enables CONFIG_UBSAN_SANITIZE_ALL. It took me > an hour to figure out, but after turning that option off, the warning > is back. > > I'll send you the object for reference in a private mail, it's kind > of large. Got it, thanks. $ llvm-objdump -Dr --disassemble-symbols=dml30_ModeSupportAndSystemConfigurationFull display_mode_vba_30.o ... 1584f: 48 81 ec a0 08 00 00 subq$2208, %rsp ... $ python3 /android0/frame-larger-than/frame_larger_than.py display_mode_vba_30.o dml30_ModeSupportAndSystemConfigurationFull No dwarf info found in display_mode_vba_30.o $ llvm-readelf -S display_mode_vba_30.o | grep debug_info $ echo $? 1 Can you rebuild+resend with CONFIG_DEBUG_INFO enabled? frame_larger_than.py relies on the DWARF debug info to know what local variables occupy how much stack space. -- Thanks, ~Nick Desaulniers ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dmabuf: Add the capability to expose DMA-BUF stats in sysfs
Thanks again for the reviews! On Thu, Dec 10, 2020 at 3:03 AM Christian König wrote: > > Am 10.12.20 um 11:56 schrieb Greg KH: > > On Thu, Dec 10, 2020 at 11:27:27AM +0100, Daniel Vetter wrote: > >> On Thu, Dec 10, 2020 at 11:10:45AM +0100, Greg KH wrote: > >>> On Thu, Dec 10, 2020 at 10:58:50AM +0100, Christian König wrote: > In general a good idea, but I have a few concern/comments here. > > Am 10.12.20 um 05:43 schrieb Hridya Valsaraju: > > This patch allows statistics to be enabled for each DMA-BUF in > > sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. > > > > The following stats will be exposed by the interface: > > > > /sys/kernel/dmabuf//exporter_name > > /sys/kernel/dmabuf//size > > /sys/kernel/dmabuf//dev_map_info > > > > The inode_number is unique for each DMA-BUF and was added earlier [1] > > in order to allow userspace to track DMA-BUF usage across different > > processes. > > > > Currently, this information is exposed in > > /sys/kernel/debug/dma_buf/bufinfo. > > However, since debugfs is considered unsafe to be mounted in production, > > it is being duplicated in sysfs. > Mhm, this makes it part of the UAPI. What is the justification for this? > > In other words do we really need those debug information in a production > environment? > >>> Production environments seem to want to know who is using up memory :) > >> This only shows shared memory, so it does smell a lot like $specific_issue > >> and we're designing a narrow solution for that and then have to carry it > >> forever. > > I think the "issue" is that this was a feature from ion that people > > "missed" in the dmabuf move. Taking away the ability to see what kind > > of allocations were being made didn't make a lot of debugging tools > > happy :( > > Yeah, that is certainly a very valid concern. > > > But Hridya knows more, she's been dealing with the transition for a long > > time now. Currently, telemetry tools capture this information(along with other memory metrics) periodically as well as on important events like a foreground app kill (which might have been triggered by an LMK). We would also like to get a snapshot of the system memory usage on other events such as OOM kills and ANRs. > > > >> E.g. why is the list of attachments not a sysfs link? That's how we > >> usually expose struct device * pointers in sysfs to userspace, not as a > >> list of things. > > These aren't struct devices, so I don't understand the objection here. > > Where else could these go in sysfs? > > Sure they are! Just take a look at an attachment: > > struct dma_buf_attachment { > struct dma_buf *dmabuf; > struct device *dev; > > This is the struct device which is importing the buffer and the patch in > discussion is just printing the name of this device into sysfs. I actually did not know that this is not ok to do. I will change it in the next version of the patch to be sysfs links instead. > > >> Furthermore we don't have the exporter device covered anywhere, how is > >> that tracked? Yes Android just uses ion for all shared buffers, but that's > >> not how all of linux userspace works. > > Do we have the exporter device link in the dmabuf interface? If so, > > great, let's use that, but for some reason I didn't think it was there. > > Correct, since we don't really need a device as an exporter (it can just > be a system heap as well) we only have a const char* as name for the > exporter. Yes, the file exporter_name prints out this information. > > >> Then I guess there's the mmaps, you can fish them out of procfs. A tool > >> which collects all that information might be useful, just as demonstration > >> of how this is all supposed to be used. > > There's a script somewhere that does this today, again, Hridya knows > > more. That is correct, we do have a tool in AOSP that gathers the per-process DMA-BUF map stats from procfs. https://android.googlesource.com/platform/system/memory/libmeminfo/+/refs/heads/master/libdmabufinfo/tools/dmabuf_dump.cpp When I send the next revision of the patch, I will also include links to AOSP CLs that show the usage for the sysfs files. > > > >> There's also some things to make sure we're at least having thought about > >> how other things fit in here. E.d. dma_resv attached to the dma-buf > >> matters in general a lot. It doesn't matter on Android because > >> everything's pinned all the time anyway. I see your point Daniel! I will make the interface extendable in the next version of the patch. > >> > >> Also I thought sysfs was one value one file, dumping an entire list into > >> dev_info_map with properties we'll need to extend (once you care about > >> dma_resv you also want to know which attachments are dynamic) does not > >> smell like sysfs design at all. > > sysfs is one value per file, what is being exported that is larger than > > that here? Did I miss something on r
Re: [PATCH v1] drm/bridge: lt9611: Fix handling of 4k panels
Thanks. Confirmed that this fixes display output for me on a 4K monitor. On Mon, Nov 23, 2020 at 2:46 AM Robert Foss wrote: > > 4k requires two dsi pipes, so don't report MODE_OK when only a > single pipe is configured. But rather report MODE_PANEL to > signal that requirements of the panel are not being met. > > Reported-by: Peter Collingbourne > Suggested-by: Peter Collingbourne > Signed-off-by: Robert Foss > Tested-by: John Stultz Tested-by: Peter Collingbourne > --- > drivers/gpu/drm/bridge/lontium-lt9611.c | 8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c > b/drivers/gpu/drm/bridge/lontium-lt9611.c > index d734d9402c35..e8eb8deb444b 100644 > --- a/drivers/gpu/drm/bridge/lontium-lt9611.c > +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c > @@ -867,8 +867,14 @@ static enum drm_mode_status > lt9611_bridge_mode_valid(struct drm_bridge *bridge, > const struct > drm_display_mode *mode) > { > struct lt9611_mode *lt9611_mode = lt9611_find_mode(mode); > + struct lt9611 *lt9611 = bridge_to_lt9611(bridge); > > - return lt9611_mode ? MODE_OK : MODE_BAD; > + if (!lt9611_mode) > + return MODE_BAD; > + else if (lt9611_mode->intfs > 1 && !lt9611->dsi1) > + return MODE_PANEL; > + else > + return MODE_OK; > } > > static void lt9611_bridge_pre_enable(struct drm_bridge *bridge) > -- > 2.27.0 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel