date:20200313

On Fri, Mar 13, 2020 at 01:07:27PM -0500, Rob Herring wrote:
> Extra dtc warnings (roughly what W=1 enables) are now enabled by default
> when building the binding examples. These were fixed treewide in
> 5.6-rc5, but some new display bindings have been added with new
> warnings:
> 
> Documentation/devicetree/bindings/display/ti/ti,am65x-dss.example.dts:21.27-49.11:
>  Warning (unit_address_format): /example-0/dss@04a0: unit name should not 
> have leading 0s
> Documentation/devicetree/bindings/display/ti/ti,j721e-dss.example.dts:21.27-72.11:
>  Warning (unit_address_format): /example-0/dss@04a0: unit name should not 
> have leading 0s
> Documentation/devicetree/bindings/display/ti/ti,k2g-dss.example.dts:20.27-42.11:
>  Warning (unit_address_format): /example-0/dss@0254: unit name should not 
> have leading 0s
> 
> Cc: Jyri Sarha 
> Cc: Tomi Valkeinen 
> Signed-off-by: Rob Herring 
Reviewed-by: Sam Ravnborg 

> ---
> v2:
>  - Drop panel fixes as there's another patch fixing the 3 panels plus
>others.
Will revisit the panel/ fixes patch and apply tomorrow.

Sam

> ---
>  Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml | 2 +-
>  Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml | 2 +-
>  Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml   | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml 
> b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> index cac61a998203..aa5543a64526 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
> @@ -121,7 +121,7 @@ examples:
>  #include 
>  #include 
>  
> -dss: dss@04a0 {
> +dss: dss@4a0 {
>  compatible = "ti,am65x-dss";
>  reg =   <0x0 0x04a0 0x0 0x1000>, /* common */
>  <0x0 0x04a02000 0x0 0x1000>, /* vidl1 */
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml 
> b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> index ade9b2f513f5..6d47cd7206c2 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
> @@ -154,7 +154,7 @@ examples:
>  #include 
>  #include 
>  
> -dss: dss@04a0 {
> +dss: dss@4a0 {
>  compatible = "ti,j721e-dss";
>  reg =   <0x00 0x04a0 0x00 0x1>, /* common_m */
>  <0x00 0x04a1 0x00 0x1>, /* common_s0*/
> diff --git a/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml 
> b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
> index 385bd060ccf9..7cb37053e95b 100644
> --- a/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
> +++ b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
> @@ -81,7 +81,7 @@ examples:
>  #include 
>  #include 
>  
> -dss: dss@0254 {
> +dss: dss@254 {
>  compatible = "ti,k2g-dss";
>  reg =   <0x0254 0x400>,
>  <0x0255 0x1000>,
> -- 
> 2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/display: Fix pageflip event race condition for DCN. (v2)

2020-03-13 Thread Mario Kleiner

On Fri, Mar 13, 2020 at 5:02 PM Michel Dänzer  wrote:

> On 2020-03-13 1:35 p.m., Kazlauskas, Nicholas wrote:
> > On 2020-03-12 10:32 a.m., Alex Deucher wrote:
> >> On Thu, Mar 5, 2020 at 4:21 PM Mario Kleiner
> >>  wrote:
> >>>
> >>> Commit '16f17eda8bad ("drm/amd/display: Send vblank and user
> >>> events at vsartup for DCN")' introduces a new way of pageflip
> >>> completion handling for DCN, and some trouble.
> >>>
> >>> The current implementation introduces a race condition, which
> >>> can cause pageflip completion events to be sent out one vblank
> >>> too early, thereby confusing userspace and causing flicker:
> >>>
> >>> prepare_flip_isr():
> >>>
> >>> 1. Pageflip programming takes the ddev->event_lock.
> >>> 2. Sets acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED
> >>> 3. Releases ddev->event_lock.
> >>>
> >>> --> Deadline for surface address regs double-buffering passes on
> >>>  target pipe.
> >>>
> >>> 4. dc_commit_updates_for_stream() MMIO programs the new pageflip
> >>> into hw, but too late for current vblank.
> >>>
> >>> => pflip_status == AMDGPU_FLIP_SUBMITTED, but flip won't complete
> >>> in current vblank due to missing the double-buffering deadline
> >>> by a tiny bit.
> >>>
> >>> 5. VSTARTUP trigger point in vblank is reached, VSTARTUP irq fires,
> >>> dm_dcn_crtc_high_irq() gets called.
> >>>
> >>> 6. Detects pflip_status == AMDGPU_FLIP_SUBMITTED and assumes the
> >>> pageflip has been completed/will complete in this vblank and
> >>> sends out pageflip completion event to userspace and resets
> >>> pflip_status = AMDGPU_FLIP_NONE.
> >>>
> >>> => Flip completion event sent out one vblank too early.
> >>>
> >>> This behaviour has been observed during my testing with measurement
> >>> hardware a couple of time.
> >>>
> >>> The commit message says that the extra flip event code was added to
> >>> dm_dcn_crtc_high_irq() to prevent missing to send out pageflip events
> >>> in case the pflip irq doesn't fire, because the "DCH HUBP" component
> >>> is clock gated and doesn't fire pflip irqs in that state. Also that
> >>> this clock gating may happen if no planes are active. According to
> >>> Nicholas, the clock gating can also happen if psr is active, and the
> >>> gating is controlled independently by the hardware, so difficult to
> >>> detect if and when the completion code in above commit is needed.
> >>>
> >>> This patch tries the following solution: It only executes the extra
> >>> pflip
> >>> completion code in dm_dcn_crtc_high_irq() iff the hardware reports
> >>> that there aren't any surface updated pending in the double-buffered
> >>> surface scanout address registers. Otherwise it leaves pflip completion
> >>> to the pflip irq handler, for a more race-free experience.
> >>>
> >>> This would only guard against the order of events mentioned above.
> >>> If Step 5 (VSTARTUP trigger) happens before step 4 then this won't help
> >>> at all, because 1-3 + 5 might happen even without the hw being
> >>> programmed
> >>> at all, ie. no surface update pending because none yet programmed
> >>> into hw.
> >>>
> >>> Therefore this patch also changes locking in amdgpu_dm_commit_planes(),
> >>> so that prepare_flip_isr() and dc_commit_updates_for_stream() are done
> >>> under event_lock protection within the same critical section.
> >>>
> >>> v2: Take Nicholas comments into account, try a different solution.
> >>>
> >>> Lightly tested on Polaris (locking) and Raven (the whole DCN stuff).
> >>> Seems to work without causing obvious new trouble.
> >>
> >> Nick, any comments on this?  Can we get this committed or do you think
> >> it needs additional rework?
> >>
> >> Thanks,
> >>
> >> Alex
> >
> > Hi Alex, Mario,
> >
> > This might be a little strange, but if we want to get this in as a fix
> > for regressions caused by the original vblank and user events at
> > vstartup patch then I'm actually going to give my reviewed by on the
> > *v1* of this patch (but not this v2):
> >
> > Reviewed-by: Nicholas Kazlauskas 
> >
> > You can feel free to apply that one.
> >
> > Reason 1: After having thought about it some more I don't think we
> > enable anything today that has hubp powered down at the same time we
> > expect to be waiting for a flip - eg. DMCU powering down HUBP during PSR
> > entry. Static screen interrupt should happen after that flip finishes I
> > think.
> >
> > The CRTC can still be powered on with zero planes, and I don't think any
> > userspace explicitly asks for vblank events in this case but it doesn't
> > hurt to have the check.
> >
>

Ok, so the original commit that causes the races currently solves a
non-existing - but potential future - problem?

I guess then my v1 patch makes sense for the moment and fixes the immediate
problem for Linux 5.6-rc.

Maybe there's a way to ask the hw if hubp is clock-gated and so if that
code needs to run or not in the future?

As mentioned before, it would be helpful at least for me to get a better
overview about

Re: [RFC PATCH 0/8] * Per context fencing *

2020-03-13 Thread Chia-I Wu

On Thu, Mar 12, 2020 at 4:08 PM Gurchetan Singh
 wrote:
>
>
>
> On Thu, Mar 12, 2020 at 2:29 AM Gerd Hoffmann  wrote:
>>
>> On Wed, Mar 11, 2020 at 04:36:16PM -0700, Gurchetan Singh wrote:
>> > On Wed, Mar 11, 2020 at 3:36 AM Gerd Hoffmann  wrote:
>> >
>> > >   Hi,
>> > >
>> > > > I should've been more clear -- this is an internal cleanup/preparation
>> > > and
>> > > > the per-context changes are invisible to host userspace.
>> > >
>> > > Ok, it wasn't clear that you don't flip the switch yet.  In general the
>> > > commit messages could be a bit more verbose ...
>> > >
>> > > I'm wondering though why we need the new fence_id in the first place.
>> > > Isn't it enough to have per-context (instead of global) last_seq?
>> > >
>> >
>> > Heh, that was to leave open the possibility of multiple timelines per
>> > context.  Roughly speaking,
>> >
>> > V2 -- multiple processes
>> > V3 -- multiple processes and multiple threads (due to VK multi-threaded
>> > command buffers)
>> >
>> > I think we all agree on V2.  It seems we still have to discuss V3
>> > (multi-queue, thread pools, a fence context associated with each thread) a
>> > bit more before we start landing pieces.
>>
>> While thinking about the whole thing a bit more ...
>> Do we need virtio_gpu_ctrl_hdr->fence_id at all?
>
>
> A fence ID could be useful for sharing fences across virtio devices.  Think 
> FENCE_ASSIGN_UUID, akin to  RESOURCE_ASSIGN_UUID (+dstevens@).
>
>>
>> At virtio level it is pretty simple:  The host completes the SUBMIT_3D
>> virtio command when it finished rendering, period.
>>
>>
>> On the guest side we don't need the fence_id.  The completion callback
>> gets passed the virtio_gpu_vbuffer, so it can figure which command did
>> actually complete without looking at virtio_gpu_ctrl_hdr->fence_id.
>>
>> On the host side we depend on the fence_id right now, but only because
>> that is the way the virgl_renderer_callbacks->write_fence interface is
>> designed.  We have to change that anyway for per-context (or whatever)
>> fences, so it should not be a problem to drop the fence_id dependency
>> too and just pass around an opaque pointer instead.

I am still catching up, but IIUC, indeed I don't think the host needs
to depend on fence_id.  We should be able to repurpose fence_id.  On
the other hand, the VIRTIO_GPU_FLAG_FENCE flag is interesting, and it
indicates that the vbuf is on the host GPU timeline instead of the
host CPU timeline.

>
>
> For multiple GPU timelines per context, the (process local) sync object 
> handle looks interesting:
>
> https://patchwork.kernel.org/patch/9758565/
>
> Some have extended EXECBUFFER to support this flow:
>
> https://patchwork.freedesktop.org/patch/msgid/1499289202-25441-1-git-send-email-jason.ekstr...@intel.com

I think this only affects the kernel/userspace interface?  I know
there were works being done to support VK_KHR_timeline semaphore,
which is something we definitely want.  I don't know if it is the only
way for the userspace to gain the extension support.  I need to do my
homework...



>
>>
>> cheers,
>>   Gerd
>>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1 1/3] drm: drm_encoder_init() => drm_encoder_init_funcs()

Many callers of drm_encoder_init use a drm_encoder_funcs
that contains only a drm_encoder_cleanup() callback.

drm_simple_encoder_init() was introduced to cover the
common case where an encoder with only basic functionality
was needed. But it really do not belong in drm_simple_*

Rename drm_encoder_init() to drm_encoder_funcs(),
so we can then in a subsequent patch rename
drm_simple_encoder_init() to drm_encoder_init().
And then move the functionality to drm_encoder where it bleongs.

checkpatch complains about long lines in amd and radeon.
Surrounding lines are too long too, so warnings were ignored.

Signed-off-by: Sam Ravnborg 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Thomas Zimmermann 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: NXP Linux Team 
Cc: Laurent Pinchart 
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Cc: linux-media...@lists.infradead.org
Cc: linux-amlo...@lists.infradead.org
Cc: amd-...@lists.freedesktop.org
Cc: linux-renesas-...@vger.kernel.org
Cc: linux-rockc...@lists.infradead.org
Cc: linux-st...@st-md-mailman.stormreply.com
Cc: linux-te...@vger.kernel.org
Cc: virtualizat...@lists.linux-foundation.org
---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c| 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c| 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_virtual.c  |  4 +-
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 10 ++---
 .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 10 ++---
 drivers/gpu/drm/arc/arcpgu_hdmi.c |  4 +-
 drivers/gpu/drm/arc/arcpgu_sim.c  |  4 +-
 .../gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c  |  6 +--
 drivers/gpu/drm/drm_encoder.c | 14 +++
 drivers/gpu/drm/drm_encoder_slave.c   |  2 +-
 drivers/gpu/drm/drm_simple_kms_helper.c   |  6 +--
 drivers/gpu/drm/drm_writeback.c   |  6 +--
 drivers/gpu/drm/exynos/exynos_dp.c|  4 +-
 drivers/gpu/drm/exynos/exynos_drm_dpi.c   |  4 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c   |  4 +-
 drivers/gpu/drm/exynos/exynos_drm_vidi.c  |  4 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c  |  4 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c |  4 +-
 drivers/gpu/drm/gma500/cdv_intel_crt.c|  5 ++-
 drivers/gpu/drm/gma500/cdv_intel_dp.c |  4 +-
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c   |  4 +-
 drivers/gpu/drm/gma500/cdv_intel_lvds.c   |  6 +--
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.c|  7 ++--
 drivers/gpu/drm/gma500/oaktrail_hdmi.c|  6 +--
 drivers/gpu/drm/gma500/oaktrail_lvds.c|  4 +-
 drivers/gpu/drm/gma500/psb_intel_lvds.c   |  6 +--
 drivers/gpu/drm/gma500/psb_intel_sdvo.c   |  4 +-
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c  |  4 +-
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c  |  4 +-
 drivers/gpu/drm/i2c/tda998x_drv.c |  5 ++-
 drivers/gpu/drm/i915/display/icl_dsi.c|  4 +-
 drivers/gpu/drm/i915/display/intel_crt.c  |  5 ++-
 drivers/gpu/drm/i915/display/intel_ddi.c  |  6 ++-
 drivers/gpu/drm/i915/display/intel_dp.c   |  6 +--
 drivers/gpu/drm/i915/display/intel_dp_mst.c   |  6 ++-
 drivers/gpu/drm/i915/display/intel_dvo.c  |  6 +--
 drivers/gpu/drm/i915/display/intel_hdmi.c |  6 +--
 drivers/gpu/drm/i915/display/intel_lvds.c |  4 +-
 drivers/gpu/drm/i915/display/intel_sdvo.c |  6 +--
 drivers/gpu/drm/i915/display/intel_tv.c   |  4 +-
 drivers/gpu/drm/i915/display/vlv_dsi.c|  5 ++-
 drivers/gpu/drm/imx/dw_hdmi-imx.c |  4 +-
 drivers/gpu/drm/imx/imx-ldb.c |  4 +-
 drivers/gpu/drm/imx/imx-tve.c |  4 +-
 drivers/gpu/drm/imx/parallel-display.c|  4 +-
 drivers/gpu/drm/ingenic/ingenic-drm.c |  5 ++-
 drivers/gpu/drm/mediatek/mtk_dpi.c|  5 ++-
 drivers/gpu/drm/mediatek/mtk_dsi.c|  4 +-
 drivers/gpu/drm/meson/meson_dw_hdmi.c |  5 ++-
 drivers/gpu/drm/meson/meson_venc_cvbs.c   |  5 ++-
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c   |  4 +-
 .../gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c  |  4 +-
 .../gpu/drm/msm/disp/mdp4/mdp4_dtv_encoder.c  |  4 +-
 .../gpu/drm/msm/disp/mdp4/mdp4_lcdc_encoder.c |  4 +-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_encoder.c  |  3 +-
 drivers/gpu/drm/nouveau/dispnv04/dac.c|  4 +-
 drivers/gpu/drm/nouveau/dispnv04/dfp.c|  3 +-
 drivers/gpu/drm/nouveau/dispnv04/tvnv04.c |  4 +-
 drivers/gpu/drm/nouveau/dispnv04/tvnv17.c |  4 +-
 drivers/gpu/drm/nouveau/dispnv50/disp.c   | 16 
 drivers/gpu/drm/omapdrm/omap_encoder.c|  4 +-
 drivers/gpu/drm/radeon/atombios_encoders.c| 40 +--
 drivers/gpu/drm/radeon/radeon_dp_mst.c|  4 +-
 .../gpu/drm/radeon/radeon_legacy_encoders.c   | 20 +-
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c |  4 +-

[PATCH v1 0/3] drm: drm_encoder_init() => drm_encoder_init_funcs()

Thomas Zimmermann had made a nice patch-set that introduced
drm_simple_encoder_init() which is already present in drm-misc-next.

While looking at this it was suddenly obvious to me that
this was functionalty that really should be included in drm_encoder.c
The case where the core could handle the callback is pretty
common and not part of the simple pipe line.

So after some dialog on dri-devel the conclusion was to go for
a change like this:

drm_encoder_init_funcs() for all users that specified a
drm_encoder_funcs to extend the functionality.

drm_encoder_init() for all users that did not
need to extend the basic functionality with
drm_encoder_funcs.

A similar approach with a _funcs() prefix is used elsewhere in drm/

This required a rename of the existing users, and
a follow-up patch that moves drm_simple_encoder_init()
to drm_encoder.c

Patches 3 in this set demonstrate the use of drm_encoder_init().
There are many more drivers that can be converted as Thomas
has already demonstrated.

This is all based on work done by Thomas Zimmermann,
I just wanted to implement my suggestion so
we could select the best way forward.

Note: Daniel Vetter has hinted the approach implemented
here smelled like middle-layer.
IMO this is not so, it is just a way to handle cleanup
for the simple cases.

Sam


Sam Ravnborg (3):
  drm: drm_encoder_init() => drm_encoder_init_funcs()
  drm: drm_simple_encoder_init() => drm_encoder_init()
  drm/atmel-hlcdc: Use drm_encoder_init()

 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v6_0.c  | 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c  | 28 ++---
 drivers/gpu/drm/amd/amdgpu/dce_virtual.c   |  4 +-
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c  | 10 ++---
 .../amd/display/amdgpu_dm/amdgpu_dm_mst_types.c| 10 ++---
 drivers/gpu/drm/arc/arcpgu_hdmi.c  |  4 +-
 drivers/gpu/drm/arc/arcpgu_sim.c   |  4 +-
 drivers/gpu/drm/ast/ast_mode.c |  3 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c   |  8 +---
 drivers/gpu/drm/drm_encoder.c  | 49 +++---
 drivers/gpu/drm/drm_encoder_slave.c|  2 +-
 drivers/gpu/drm/drm_simple_kms_helper.c| 45 +---
 drivers/gpu/drm/drm_writeback.c|  6 +--
 drivers/gpu/drm/exynos/exynos_dp.c |  4 +-
 drivers/gpu/drm/exynos/exynos_drm_dpi.c|  4 +-
 drivers/gpu/drm/exynos/exynos_drm_dsi.c|  4 +-
 drivers/gpu/drm/exynos/exynos_drm_vidi.c   |  4 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c   |  4 +-
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_rgb.c  |  4 +-
 drivers/gpu/drm/gma500/cdv_intel_crt.c |  5 ++-
 drivers/gpu/drm/gma500/cdv_intel_dp.c  |  4 +-
 drivers/gpu/drm/gma500/cdv_intel_hdmi.c|  4 +-
 drivers/gpu/drm/gma500/cdv_intel_lvds.c|  6 +--
 drivers/gpu/drm/gma500/mdfld_dsi_dpi.c |  7 ++--
 drivers/gpu/drm/gma500/oaktrail_hdmi.c |  6 +--
 drivers/gpu/drm/gma500/oaktrail_lvds.c |  4 +-
 drivers/gpu/drm/gma500/psb_intel_lvds.c|  6 +--
 drivers/gpu/drm/gma500/psb_intel_sdvo.c|  4 +-
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c   |  4 +-
 drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c   |  4 +-
 drivers/gpu/drm/i2c/tda998x_drv.c  |  5 ++-
 drivers/gpu/drm/i915/display/icl_dsi.c |  4 +-
 drivers/gpu/drm/i915/display/intel_crt.c   |  5 ++-
 drivers/gpu/drm/i915/display/intel_ddi.c   |  6 ++-
 drivers/gpu/drm/i915/display/intel_dp.c|  6 +--
 drivers/gpu/drm/i915/display/intel_dp_mst.c|  6 ++-
 drivers/gpu/drm/i915/display/intel_dvo.c   |  6 +--
 drivers/gpu/drm/i915/display/intel_hdmi.c  |  6 +--
 drivers/gpu/drm/i915/display/intel_lvds.c  |  4 +-
 drivers/gpu/drm/i915/display/intel_sdvo.c  |  6 +--
 drivers/gpu/drm/i915/display/intel_tv.c|  4 +-
 drivers/gpu/drm/i915/display/vlv_dsi.c |  5 ++-
 drivers/gpu/drm/imx/dw_hdmi-imx.c  |  4 +-
 drivers/gpu/drm/imx/imx-ldb.c  |  4 +-
 drivers/gpu/drm/imx/imx-tve.c  |  4 +-
 drivers/gpu/drm/imx/parallel-display.c |  4 +-
 drivers/gpu/drm/ingenic/ingenic-drm.c  |  5 ++-
 drivers/gpu/drm/mediatek/mtk_dpi.c |  5 ++-
 drivers/gpu/drm/mediatek/mtk_dsi.c |  4 +-
 drivers/gpu/drm/meson/meson_dw_hdmi.c  |  5 ++-
 drivers/gpu/drm/meson/meson_venc_cvbs.c|  5 ++-
 drivers/gpu/drm/mgag200/mgag200_mode.c |  7 +---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c|  4 +-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_dsi_encoder.c   |  4 +-

[PATCH v1 2/3] drm: drm_simple_encoder_init() => drm_encoder_init()

A lot of drivers requires only a basic encoder with no need
to extend the functionality.
This was previously implemented in drm_simple_kms_helper.c
but encoders are not necessarily simple despite no
need for a drm_encoder_funcs for adding functionality.

Move the init function to drm_encoder.c to reflect this.
And adjust the name to drm_encoder_init().

Drop include of drm_simple_kms_helper.h in the touched
drivers as it is no logner required.

Signed-off-by: Sam Ravnborg 
Cc: Dave Airlie 
Cc: Maarten Lankhorst 
Cc: Maxime Ripard 
Cc: Thomas Zimmermann 
Cc: Laurent Pinchart 
Cc: David Airlie 
Cc: Daniel Vetter 
Cc: Gerd Hoffmann 
Cc: Sam Ravnborg 
Cc: Emil Velikov 
Cc: Andrzej Pietrasiewicz 
Cc: "José Roberto de Souza" 
---
 drivers/gpu/drm/ast/ast_mode.c  |  3 +-
 drivers/gpu/drm/drm_encoder.c   | 37 
 drivers/gpu/drm/drm_simple_kms_helper.c | 45 +
 drivers/gpu/drm/mgag200/mgag200_mode.c  |  7 ++--
 drivers/gpu/drm/qxl/qxl_display.c   |  7 ++--
 include/drm/drm_encoder.h   |  3 ++
 include/drm/drm_simple_kms_helper.h |  4 ---
 7 files changed, 46 insertions(+), 60 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cdd6c46d6557..4f6ace1afaf0 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -40,7 +40,6 @@
 #include 
 #include 
 #include 
-#include 
 
 #include "ast_drv.h"
 #include "ast_tables.h"
@@ -964,7 +963,7 @@ static int ast_encoder_init(struct drm_device *dev)
struct drm_encoder *encoder = >encoder;
int ret;
 
-   ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+   ret = drm_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
if (ret)
return ret;
 
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index a76a5f04ab39..e1e90652094c 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -152,6 +152,43 @@ int drm_encoder_init_funcs(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_encoder_init_funcs);
 
+static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
+   .destroy = drm_encoder_cleanup,
+};
+
+/**
+ * drm_simple_init - Initialize a preallocated encoder with basic 
functionality.
+ * @dev: drm device
+ * @encoder: the encoder to initialize
+ * @encoder_type: user visible type of the encoder
+ *
+ * Initialises a preallocated encoder that has no further functionality.
+ * Settings for possible CRTC and clones are left to their initial values.
+ * The encoder will be cleaned up automatically as part of the mode-setting
+ * cleanup.
+ *
+ * The caller of drm_encoder_init() is responsible for freeing
+ * the encoder's memory after the encoder has been cleaned up. At the
+ * moment this only works reliably if the encoder data structure is
+ * stored in the device structure. Free the encoder's memory as part of
+ * the device release function.
+ *
+ * FIXME: Later improvements to DRM's resource management may allow for
+ *an automated kfree() of the encoder's memory.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_simple_init(struct drm_device *dev,
+   struct drm_encoder *encoder,
+   int encoder_type)
+{
+   return drm_encoder_init_funcs(dev, encoder,
+ _simple_encoder_funcs_cleanup,
+ encoder_type, NULL);
+}
+EXPORT_SYMBOL(drm_encoder_init);
+
 /**
  * drm_encoder_cleanup - cleans up an initialised encoder
  * @encoder: encoder to cleanup
diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 24d4433c347b..d70170980839 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -26,51 +26,8 @@
  * entity. Some flexibility for code reuse is provided through a separately
  * allocated _connector object and supporting optional _bridge
  * encoder drivers.
- *
- * Many drivers require only a very simple encoder that fulfills the minimum
- * requirements of the display pipeline and does not add additional
- * functionality. The function drm_simple_encoder_init() provides an
- * implementation of such an encoder.
  */
 
-static const struct drm_encoder_funcs drm_simple_encoder_funcs_cleanup = {
-   .destroy = drm_encoder_cleanup,
-};
-
-/**
- * drm_simple_encoder_init - Initialize a preallocated encoder with
- *   basic functionality.
- * @dev: drm device
- * @encoder: the encoder to initialize
- * @encoder_type: user visible type of the encoder
- *
- * Initialises a preallocated encoder that has no further functionality.
- * Settings for possible CRTC and clones are left to their initial values.
- * The encoder will be cleaned up automatically as part of the mode-setting
- * cleanup.
- *
- * The caller of drm_simple_encoder_init() is responsible for freeing

[PATCH v1 3/3] drm/atmel-hlcdc: Use drm_encoder_init()

atmel-hlcdc has no need to extend the functionality of the
encoder, so use drm_encoder_init().

Signed-off-by: Sam Ravnborg 
Cc: Sam Ravnborg 
Cc: Boris Brezillon 
Cc: Nicolas Ferre 
Cc: Alexandre Belloni 
Cc: Ludovic Desroches 
Cc: Thomas Zimmermann 
Cc: Laurent Pinchart 
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c | 8 +---
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c 
b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
index a845d587c315..96e0d85748d2 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_output.c
@@ -22,10 +22,6 @@ struct atmel_hlcdc_rgb_output {
int bus_fmt;
 };
 
-static const struct drm_encoder_funcs atmel_hlcdc_panel_encoder_funcs = {
-   .destroy = drm_encoder_cleanup,
-};
-
 static struct atmel_hlcdc_rgb_output *
 atmel_hlcdc_encoder_to_rgb_output(struct drm_encoder *encoder)
 {
@@ -98,9 +94,7 @@ static int atmel_hlcdc_attach_endpoint(struct drm_device 
*dev, int endpoint)
return -EINVAL;
}
 
-   ret = drm_encoder_init_funcs(dev, >encoder,
-_hlcdc_panel_encoder_funcs,
-DRM_MODE_ENCODER_NONE, NULL);
+   ret = drm_encoder_init(dev, >encoder, DRM_MODE_ENCODER_NONE);
if (ret)
return ret;
 
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/9] drm: Constify topology id

2020-03-13 Thread Alex Deucher

On Fri, Mar 13, 2020 at 12:21 PM Ville Syrjala
 wrote:
>
> From: Ville Syrjälä 
>
> Make the topology id const since we don't want to change it.
>
> Signed-off-by: Ville Syrjälä 

Series is:
Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/drm_connector.c | 4 ++--
>  include/drm/drm_connector.h | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 644f0ad10671..462d8caa6e72 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -2392,7 +2392,7 @@ EXPORT_SYMBOL(drm_mode_put_tile_group);
>   * tile group or NULL if not found.
>   */
>  struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev,
> -  char topology[8])
> +  const char topology[8])
>  {
> struct drm_tile_group *tg;
> int id;
> @@ -2422,7 +2422,7 @@ EXPORT_SYMBOL(drm_mode_get_tile_group);
>   * new tile group or NULL.
>   */
>  struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
> - char topology[8])
> + const char topology[8])
>  {
> struct drm_tile_group *tg;
> int ret;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index 19ae6bb5c85b..fd543d1db9b2 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -1617,9 +1617,9 @@ struct drm_tile_group {
>  };
>
>  struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
> - char topology[8]);
> + const char topology[8]);
>  struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev,
> -  char topology[8]);
> +  const char topology[8]);
>  void drm_mode_put_tile_group(struct drm_device *dev,
>  struct drm_tile_group *tg);
>
> --
> 2.24.1
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor based integer scaling support

2020-03-13 Thread Ville Syrjälä

On Fri, Mar 13, 2020 at 08:45:35AM +, Laxminarayan Bharadiya, Pankaj wrote:
> 
> 
> > -Original Message-
> > From: Ville Syrjälä 
> > Sent: 12 March 2020 19:25
> > To: Laxminarayan Bharadiya, Pankaj
> > 
> > Cc: jani.nik...@linux.intel.com; dan...@ffwll.ch; intel-
> > g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
> > airl...@linux.ie;
> > maarten.lankho...@linux.intel.com; tzimmerm...@suse.de;
> > mrip...@kernel.org; mihail.atanas...@arm.com; Joonas Lahtinen
> > ; Vivi, Rodrigo ;
> > Chris Wilson ; Souza, Jose ;
> > De Marchi, Lucas ; Roper, Matthew D
> > ; Deak, Imre ; Shankar,
> > Uma ; linux-ker...@vger.kernel.org; Nautiyal, Ankit K
> > 
> > Subject: Re: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor based
> > integer scaling support
> > 
> > On Thu, Mar 12, 2020 at 09:13:24AM +, Laxminarayan Bharadiya, Pankaj
> > wrote:
> > >
> > >
> > > > -Original Message-
> > > > From: Ville Syrjälä 
> > > > Sent: 10 March 2020 21:47
> > > > To: Laxminarayan Bharadiya, Pankaj
> > > > 
> > > > Cc: jani.nik...@linux.intel.com; dan...@ffwll.ch; intel-
> > > > g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> > > > airl...@linux.ie; maarten.lankho...@linux.intel.com;
> > > > tzimmerm...@suse.de; mrip...@kernel.org; mihail.atanas...@arm.com;
> > > > Joonas Lahtinen ; Vivi, Rodrigo
> > > > ; Chris Wilson ;
> > > > Souza, Jose ; De Marchi, Lucas
> > > > ; Roper, Matthew D
> > > > ; Deak, Imre ;
> > > > Shankar, Uma ; linux- ker...@vger.kernel.org;
> > > > Nautiyal, Ankit K 
> > > > Subject: Re: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor
> > > > based integer scaling support
> > > >
> > > > On Tue, Feb 25, 2020 at 12:35:45PM +0530, Pankaj Bharadiya wrote:
> > > > > Integer scaling (IS) is a nearest-neighbor upscaling technique
> > > > > that simply scales up the existing pixels by an integer (i.e.,
> > > > > whole
> > > > > number) multiplier.Nearest-neighbor (NN) interpolation works by
> > > > > filling in the missing color values in the upscaled image with
> > > > > that of the coordinate-mapped nearest source pixel value.
> > > > >
> > > > > Both IS and NN preserve the clarity of the original image. Integer
> > > > > scaling is particularly useful for pixel art games that rely on
> > > > > sharp, blocky images to deliver their distinctive look.
> > > > >
> > > > > Program the scaler filter coefficients to enable the NN filter if
> > > > > scaling filter property is set to
> > > > > DRM_SCALING_FILTER_NEAREST_NEIGHBOR
> > > > > and enable integer scaling.
> > > > >
> > > > > Bspec: 49247
> > > > >
> > > > > Signed-off-by: Pankaj Bharadiya
> > > > > 
> > > > > Signed-off-by: Ankit Nautiyal 
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 83
> > > > > +++-  drivers/gpu/drm/i915/display/intel_display.h
> > > > > +++|
> > > > > 2 +  drivers/gpu/drm/i915/display/intel_sprite.c  | 20 +++--
> > > > >  3 files changed, 97 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index b5903ef3c5a0..6d5f59203258 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -6237,6 +6237,73 @@ void skl_scaler_disable(const struct
> > > > intel_crtc_state *old_crtc_state)
> > > > >   skl_detach_scaler(crtc, i);
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + *  Theory behind setting nearest-neighbor integer scaling:
> > > > > + *
> > > > > + *  17 phase of 7 taps requires 119 coefficients in 60 dwords per 
> > > > > set.
> > > > > + *  The letter represents the filter tap (D is the center tap)
> > > > > +and the number
> > > > > + *  represents the coefficient set for a phase (0-16).
> > > > > + *
> > > > > + * 
> > > > > ++++
> > > > > + * |Index value | Data value coeffient 1 | Data value 
> > > > > coeffient 2 |
> > > > > + * 
> > > > > ++++
> > > > > + * |   00h  |  B0|  A0   
> > > > >  |
> > > > > + * 
> > > > > ++++
> > > > > + * |   01h  |  D0|  C0   
> > > > >  |
> > > > > + * 
> > > > > ++++
> > > > > + * |   02h  |  F0|  E0   
> > > > >  |
> > > > > + * 
> > > > > ++++
> > > > > + * |   03h  |  A1|  G0   
> > > > >  |
> > > > > + * 
> > > > > ++++
> > > > > + * |   04h  |

Re: [PATCH v3] drm/bochs: downgrade pci_request_region failure from error to warning

On Fri, Mar 13, 2020 at 03:35:30PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> > > + if (pci_request_region(pdev, 0, "bochs-drm") != 0)
> > > + DRM_WARN("Cannot request framebuffer, boot fb still active?\n");
> > So you could use drm_WARN() which is what is preferred these days.
> 
> Nope, this isn't yet in -fixes.
Ups, did not see this was for -fixes.
My ack stands without ths change then.

Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2] dt-bindings: display: ti: Fix dtc unit-address warnings in examples

2020-03-13 Thread Rob Herring

Extra dtc warnings (roughly what W=1 enables) are now enabled by default
when building the binding examples. These were fixed treewide in
5.6-rc5, but some new display bindings have been added with new
warnings:

Documentation/devicetree/bindings/display/ti/ti,am65x-dss.example.dts:21.27-49.11:
 Warning (unit_address_format): /example-0/dss@04a0: unit name should not 
have leading 0s
Documentation/devicetree/bindings/display/ti/ti,j721e-dss.example.dts:21.27-72.11:
 Warning (unit_address_format): /example-0/dss@04a0: unit name should not 
have leading 0s
Documentation/devicetree/bindings/display/ti/ti,k2g-dss.example.dts:20.27-42.11:
 Warning (unit_address_format): /example-0/dss@0254: unit name should not 
have leading 0s

Cc: Jyri Sarha 
Cc: Tomi Valkeinen 
Signed-off-by: Rob Herring 
---
v2:
 - Drop panel fixes as there's another patch fixing the 3 panels plus
   others.
---
 Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml | 2 +-
 Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml | 2 +-
 Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml 
b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
index cac61a998203..aa5543a64526 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,am65x-dss.yaml
@@ -121,7 +121,7 @@ examples:
 #include 
 #include 
 
-dss: dss@04a0 {
+dss: dss@4a0 {
 compatible = "ti,am65x-dss";
 reg =   <0x0 0x04a0 0x0 0x1000>, /* common */
 <0x0 0x04a02000 0x0 0x1000>, /* vidl1 */
diff --git a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml 
b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
index ade9b2f513f5..6d47cd7206c2 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,j721e-dss.yaml
@@ -154,7 +154,7 @@ examples:
 #include 
 #include 
 
-dss: dss@04a0 {
+dss: dss@4a0 {
 compatible = "ti,j721e-dss";
 reg =   <0x00 0x04a0 0x00 0x1>, /* common_m */
 <0x00 0x04a1 0x00 0x1>, /* common_s0*/
diff --git a/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml 
b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
index 385bd060ccf9..7cb37053e95b 100644
--- a/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
+++ b/Documentation/devicetree/bindings/display/ti/ti,k2g-dss.yaml
@@ -81,7 +81,7 @@ examples:
 #include 
 #include 
 
-dss: dss@0254 {
+dss: dss@254 {
 compatible = "ti,k2g-dss";
 reg =   <0x0254 0x400>,
 <0x0255 0x1000>,
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [v2] dma-buf: heaps: bugfix for selftest failure

2020-03-13 Thread shuah


On 3/7/20 7:02 AM, Leon He wrote:

From: Leon He 

There are two errors in the dmabuf-heap selftest:
1. The 'char name[5]' was not initialized to zero, which will cause
strcmp(name, "vgem") failed in check_vgem().
2. The return value of test_alloc_errors() should be reversed, other-
wise the while loop in main() will be broken.

Signed-off-by: Leon He 
---
  tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c 
b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
index cd5e1f6..836b185 100644
--- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
+++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
@@ -22,7 +22,7 @@
  static int check_vgem(int fd)
  {
drm_version_t version = { 0 };
-   char name[5];
+   char name[5] = { 0 };
int ret;
  
  	version.name_len = 4;


Please see my comment on v1 for this.


@@ -357,7 +357,7 @@ static int test_alloc_errors(char *heap_name)
if (heap_fd >= 0)
close(heap_fd);
  
-	return ret;

+   return !ret;


This change doesn't make sense. Initializing ret to 0 is a better
way to go.

thanks,
-- Shuah
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[GIT PULL] drm/tegra: Changes for v5.7-rc1

2020-03-13 Thread Thierry Reding

Hi Dave,

The following changes since commit bb6d3fb354c5ee8d6bde2d576eb7220ea09862b9:

  Linux 5.6-rc1 (2020-02-09 16:08:48 -0800)

are available in the Git repository at:

  git://anongit.freedesktop.org/tegra/linux tags/drm/tegra/for-5.7-rc1

for you to fetch changes up to e32c8c2a5fbe1514e4ae9063a49e76ecf486ffb8:

  drm/tegra: hdmi: Silence deferred-probe error (2020-03-13 18:03:06 +0100)

Thanks,
Thierry


drm/tegra: Changes for v5.7-rc1

This contains some minor cleanups, nothing too exciting.


Dmitry Osipenko (4):
  drm/tegra: dc: Use devm_platform_ioremap_resource
  drm/tegra: dc: Release PM and RGB output when client's registration fails
  drm/tegra: dc: Silence RGB output deferred-probe error
  drm/tegra: hdmi: Silence deferred-probe error

 drivers/gpu/drm/tegra/dc.c   | 20 +++-
 drivers/gpu/drm/tegra/hdmi.c | 34 +-
 2 files changed, 40 insertions(+), 14 deletions(-)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/scheduler: fix inconsistent locking of job_list_lock

2020-03-13 Thread Alex Deucher

Done.  There was a bit of fuzz when applying it.  Please double check it:
https://cgit.freedesktop.org/~agd5f/linux/commit/?h=drm-next=a7fbb630c5485f5095146df46f04c2ca1a24c299

Thanks,

Alex


Alex

On Fri, Mar 13, 2020 at 10:51 AM Lucas Stach  wrote:
>
> Hi Alex,
>
> since you seem to be picking up scheduler patches, can I ask you to
> take this one through your tree?
>
> Regards,
> Lucas
>
> On Mo, 2020-01-20 at 11:59 +0100, Christian König wrote:
> > Am 20.01.20 um 11:51 schrieb Lucas Stach:
> > > 1db8c142b6c5 (drm/scheduler: Add drm_sched_suspend/resume_timeout()) made
> > > the job_list_lock IRQ safe in as the suspend/resume calls were expected to
> > > be called from IRQ context. This usage never materialized in upstream.
> > > Instead amdgpu started locking the job_list_lock in an IRQ unsafe way in
> > > amdgpu_ib_preempt_mark_partial_job() and amdgpu_ib_preempt_job_recovery(),
> > > which leads to potential deadlock if one would actually start to call the
> > > drm_sched_suspend/resume_timeout functions from IRQ context.
> > >
> > > As no current user needs the locking to be IRQ safe, the local IRQ
> > > disable/enable is pure overhead. Fix the inconsistent locking by changing
> > > all uses of job_list_lock to use the IRQ unsafe locking primitives.
> > >
> > > Signed-off-by: Lucas Stach 
> >
> > Reviewed-by: Christian König , do you want to
> > merge through drm-misc-next or should I push it to amd-staging-drm-next?
> >
> > Regards,
> > Christian.
> >
> > > ---
> > >   drivers/gpu/drm/scheduler/sched_main.c | 38 ++
> > >   1 file changed, 15 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > index 3c57e84222ca..d1b2dd99 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -220,8 +220,7 @@ EXPORT_SYMBOL(drm_sched_fault);
> > >*
> > >* Suspend the delayed work timeout for the scheduler. This is done by
> > >* modifying the delayed work timeout to an arbitrary large value,
> > > - * MAX_SCHEDULE_TIMEOUT in this case. Note that this function can be
> > > - * called from an IRQ context.
> > > + * MAX_SCHEDULE_TIMEOUT in this case.
> > >*
> > >* Returns the timeout remaining
> > >*
> > > @@ -250,43 +249,39 @@ EXPORT_SYMBOL(drm_sched_suspend_timeout);
> > >* @sched: scheduler instance for which to resume the timeout
> > >* @remaining: remaining timeout
> > >*
> > > - * Resume the delayed work timeout for the scheduler. Note that
> > > - * this function can be called from an IRQ context.
> > > + * Resume the delayed work timeout for the scheduler.
> > >*/
> > >   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
> > > unsigned long remaining)
> > >   {
> > > -   unsigned long flags;
> > > -
> > > -   spin_lock_irqsave(>job_list_lock, flags);
> > > +   spin_lock(>job_list_lock);
> > >
> > > if (list_empty(>ring_mirror_list))
> > > cancel_delayed_work(>work_tdr);
> > > else
> > > mod_delayed_work(system_wq, >work_tdr, remaining);
> > >
> > > -   spin_unlock_irqrestore(>job_list_lock, flags);
> > > +   spin_unlock(>job_list_lock);
> > >   }
> > >   EXPORT_SYMBOL(drm_sched_resume_timeout);
> > >
> > >   static void drm_sched_job_begin(struct drm_sched_job *s_job)
> > >   {
> > > struct drm_gpu_scheduler *sched = s_job->sched;
> > > -   unsigned long flags;
> > >
> > > -   spin_lock_irqsave(>job_list_lock, flags);
> > > +   spin_lock(>job_list_lock);
> > > list_add_tail(_job->node, >ring_mirror_list);
> > > drm_sched_start_timeout(sched);
> > > -   spin_unlock_irqrestore(>job_list_lock, flags);
> > > +   spin_unlock(>job_list_lock);
> > >   }
> > >
> > >   static void drm_sched_job_timedout(struct work_struct *work)
> > >   {
> > > struct drm_gpu_scheduler *sched;
> > > struct drm_sched_job *job;
> > > -   unsigned long flags;
> > >
> > > sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> > > +
> > > job = list_first_entry_or_null(>ring_mirror_list,
> > >struct drm_sched_job, node);
> > >
> > > @@ -303,9 +298,9 @@ static void drm_sched_job_timedout(struct work_struct 
> > > *work)
> > > }
> > > }
> > >
> > > -   spin_lock_irqsave(>job_list_lock, flags);
> > > +   spin_lock(>job_list_lock);
> > > drm_sched_start_timeout(sched);
> > > -   spin_unlock_irqrestore(>job_list_lock, flags);
> > > +   spin_unlock(>job_list_lock);
> > >   }
> > >
> > >/**
> > > @@ -368,7 +363,6 @@ EXPORT_SYMBOL(drm_sched_increase_karma);
> > >   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct 
> > > drm_sched_job *bad)
> > >   {
> > > struct drm_sched_job *s_job, *tmp;
> > > -   unsigned long flags;
> > >
> > > kthread_park(sched->thread);
> > >
> > > @@ -388,9 +382,9 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
> >

[PATCH 1/9] drm: Constify topology id

From: Ville Syrjälä 

Make the topology id const since we don't want to change it.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_connector.c | 4 ++--
 include/drm/drm_connector.h | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 644f0ad10671..462d8caa6e72 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -2392,7 +2392,7 @@ EXPORT_SYMBOL(drm_mode_put_tile_group);
  * tile group or NULL if not found.
  */
 struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev,
-  char topology[8])
+  const char topology[8])
 {
struct drm_tile_group *tg;
int id;
@@ -2422,7 +2422,7 @@ EXPORT_SYMBOL(drm_mode_get_tile_group);
  * new tile group or NULL.
  */
 struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
- char topology[8])
+ const char topology[8])
 {
struct drm_tile_group *tg;
int ret;
diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
index 19ae6bb5c85b..fd543d1db9b2 100644
--- a/include/drm/drm_connector.h
+++ b/include/drm/drm_connector.h
@@ -1617,9 +1617,9 @@ struct drm_tile_group {
 };
 
 struct drm_tile_group *drm_mode_create_tile_group(struct drm_device *dev,
- char topology[8]);
+ const char topology[8]);
 struct drm_tile_group *drm_mode_get_tile_group(struct drm_device *dev,
-  char topology[8]);
+  const char topology[8]);
 void drm_mode_put_tile_group(struct drm_device *dev,
 struct drm_tile_group *tg);
 
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/9] drm/edid: Swap some operands in for_each_displayid_db()

From: Ville Syrjälä 

A+B on the previous line, B+A on the next line. Brain hurts.

Signed-off-by: Ville Syrjälä 
---
 include/drm/drm_displayid.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_displayid.h b/include/drm/drm_displayid.h
index 9d3b745c3107..27bdd273fc4e 100644
--- a/include/drm/drm_displayid.h
+++ b/include/drm/drm_displayid.h
@@ -97,7 +97,7 @@ struct displayid_detailed_timing_block {
 (idx) + sizeof(struct displayid_block) <= (length) && \
 (idx) + sizeof(struct displayid_block) + (block)->num_bytes <= 
(length) && \
 (block)->num_bytes > 0; \
-(idx) += (block)->num_bytes + sizeof(struct displayid_block), \
+(idx) += sizeof(struct displayid_block) + (block)->num_bytes, \
 (block) = (struct displayid_block *)&(displayid)[idx])
 
 #endif
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 7/9] drm/edid: Don't include ext block csum in DispID size

From: Ville Syrjälä 

The EDID extension block checksum byte is not part of the
actual DispID data, so don't use it in validate_displayid().

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3067be710e5b..f1ba06396c0a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3222,7 +3222,8 @@ static u8 *drm_find_displayid_extension(const struct edid 
*edid,
if (!displayid)
return NULL;
 
-   *length = EDID_LENGTH;
+   /* EDID extensions block checksum isn't for us */
+   *length = EDID_LENGTH - 1;
*idx = 1;
 
ret = validate_displayid(displayid, *length, *idx);
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 9/9] drm/edid: Fix DispID tile parsing for override EDID

From: Ville Syrjälä 

Currently the DispID tile block gets parsed in drm_get_edid(), which
is an odd place for it considering we parse nothing else there. Also
this doesn't work for override EDIDs since
drm_connector_update_edid_property() refuses to do its job twice
in such cases. Thus we never update the tile property with results
of the DispID tile block parsing during drm_get_edid().

To fix this let's just move the tile block parsing to happen during
drm_connector_update_edid_property(), which is where we parse a bunch
of other stuff as well (and where we update both the EDID and tile
properties).

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_connector.c |  2 ++
 drivers/gpu/drm/drm_crtc_internal.h |  1 +
 drivers/gpu/drm/drm_edid.c  | 33 +
 3 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 462d8caa6e72..b1099e1251a2 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1970,6 +1970,8 @@ int drm_connector_update_edid_property(struct 
drm_connector *connector,
else
drm_reset_display_info(connector);
 
+   drm_update_tile_info(connector, edid);
+
drm_object_property_set_value(>base,
  dev->mode_config.non_desktop_property,
  connector->display_info.non_desktop);
diff --git a/drivers/gpu/drm/drm_crtc_internal.h 
b/drivers/gpu/drm/drm_crtc_internal.h
index 16f2413403aa..feba683c12a6 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -278,3 +278,4 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 void drm_mode_fixup_1366x768(struct drm_display_mode *mode);
 void drm_reset_display_info(struct drm_connector *connector);
 u32 drm_add_display_info(struct drm_connector *connector, const struct edid 
*edid);
+void drm_update_tile_info(struct drm_connector *connector, const struct edid 
*edid);
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 185027f751f6..8239fd5a335f 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1583,8 +1583,6 @@ module_param_named(edid_fixup, edid_fixup, int, 0400);
 MODULE_PARM_DESC(edid_fixup,
 "Minimum number of valid EDID header bytes (0-8, default 6)");
 
-static void drm_get_displayid(struct drm_connector *connector,
- struct edid *edid);
 static int validate_displayid(u8 *displayid, int length, int idx);
 
 static int drm_edid_block_checksum(const u8 *raw_edid)
@@ -2018,18 +2016,13 @@ EXPORT_SYMBOL(drm_probe_ddc);
 struct edid *drm_get_edid(struct drm_connector *connector,
  struct i2c_adapter *adapter)
 {
-   struct edid *edid;
-
if (connector->force == DRM_FORCE_OFF)
return NULL;
 
if (connector->force == DRM_FORCE_UNSPECIFIED && 
!drm_probe_ddc(adapter))
return NULL;
 
-   edid = drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
-   if (edid)
-   drm_get_displayid(connector, edid);
-   return edid;
+   return drm_do_get_edid(connector, drm_do_probe_ddc_edid, adapter);
 }
 EXPORT_SYMBOL(drm_get_edid);
 
@@ -5793,9 +5786,9 @@ drm_hdmi_vendor_infoframe_from_display_mode(struct 
hdmi_vendor_infoframe *frame,
 EXPORT_SYMBOL(drm_hdmi_vendor_infoframe_from_display_mode);
 
 static int drm_parse_tiled_block(struct drm_connector *connector,
-struct displayid_block *block)
+const struct displayid_block *block)
 {
-   struct displayid_tiled_block *tile = (struct displayid_tiled_block 
*)block;
+   const struct displayid_tiled_block *tile = (struct 
displayid_tiled_block *)block;
u16 w, h;
u8 tile_v_loc, tile_h_loc;
u8 num_v_tile, num_h_tile;
@@ -5846,10 +5839,10 @@ static int drm_parse_tiled_block(struct drm_connector 
*connector,
return 0;
 }
 
-static int drm_parse_display_id(struct drm_connector *connector,
-   u8 *displayid, int length, int idx)
+static int drm_displayid_parse_tiled(struct drm_connector *connector,
+const u8 *displayid, int length, int idx)
 {
-   struct displayid_block *block;
+   const struct displayid_block *block;
int ret;
 
idx += sizeof(struct displayid_hdr);
@@ -5863,12 +5856,6 @@ static int drm_parse_display_id(struct drm_connector 
*connector,
if (ret)
return ret;
break;
-   case DATA_BLOCK_TYPE_1_DETAILED_TIMING:
-   /* handled in mode gathering code. */
-   break;
-   case DATA_BLOCK_CTA:
-   /* handled in the cea parser code. */
-   break;

[PATCH 5/9] drm/edid: Move validate_displayid() drm_find_displayid_extension()

From: Ville Syrjälä 

Instead of everyone having to call validate_displayid() let's just
have drm_find_displayid_extension() do it for them.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 19 +--
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 18c55f3b20de..dbd2e8474c2a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3216,6 +3216,7 @@ static u8 *drm_find_displayid_extension(const struct edid 
*edid,
int *length, int *idx)
 {
u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
+   int ret;
 
if (!displayid)
return NULL;
@@ -3223,12 +3224,15 @@ static u8 *drm_find_displayid_extension(const struct 
edid *edid,
*length = EDID_LENGTH;
*idx = 1;
 
+   ret = validate_displayid(displayid, *length, *idx);
+   if (ret)
+   return NULL;
+
return displayid;
 }
 
 static u8 *drm_find_cea_extension(const struct edid *edid)
 {
-   int ret;
int length, idx;
struct displayid_block *block;
u8 *cea;
@@ -3244,10 +3248,6 @@ static u8 *drm_find_cea_extension(const struct edid 
*edid)
if (!displayid)
return NULL;
 
-   ret = validate_displayid(displayid, length, idx);
-   if (ret)
-   return NULL;
-
idx += sizeof(struct displayid_hdr);
for_each_displayid_db(displayid, block, idx, length) {
if (block->tag == DATA_BLOCK_CTA) {
@@ -5189,7 +5189,6 @@ static int add_displayid_detailed_modes(struct 
drm_connector *connector,
struct edid *edid)
 {
u8 *displayid;
-   int ret;
int length, idx;
struct displayid_block *block;
int num_modes = 0;
@@ -5198,10 +5197,6 @@ static int add_displayid_detailed_modes(struct 
drm_connector *connector,
if (!displayid)
return 0;
 
-   ret = validate_displayid(displayid, length, idx);
-   if (ret)
-   return 0;
-
idx += sizeof(struct displayid_hdr);
for_each_displayid_db(displayid, block, idx, length) {
switch (block->tag) {
@@ -5849,10 +5844,6 @@ static int drm_parse_display_id(struct drm_connector 
*connector,
struct displayid_block *block;
int ret;
 
-   ret = validate_displayid(displayid, length, idx);
-   if (ret)
-   return ret;
-
idx += sizeof(struct displayid_hdr);
for_each_displayid_db(displayid, block, idx, length) {
DRM_DEBUG_KMS("block id 0x%x, rev %d, len %d\n",
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/9] drm/edid: Remove idx==1 assumptions from all over the DispID parsing

From: Ville Syrjälä 

The fact that the DispID starts at byte offset 1 is due to
the DispID coming from and EDID extension block (the first byte
being the extesion block tag). Instead of hadrdocoding that idx==1
assumptions all over let's just have drm_find_displayid_extension()
return it since it actually knows what it's talking about.

If at some point someone comes across a DispID which is not embedded
inside an EDID the function that returns the new type of DispID
can return it's own byte offset without having to updated all the
code.

TODO: should probably just get rid of that idx thing altogether
  and just return the thing we want directly.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 33 ++---
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index b269cd7f7679..3f9e659199af 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3212,15 +3212,22 @@ static u8 *drm_find_edid_extension(const struct edid 
*edid, int ext_id)
 }
 
 
-static u8 *drm_find_displayid_extension(const struct edid *edid)
+static u8 *drm_find_displayid_extension(const struct edid *edid, int *idx)
 {
-   return drm_find_edid_extension(edid, DISPLAYID_EXT);
+   u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
+
+   if (!displayid)
+   return NULL;
+
+   *idx = 1;
+
+   return displayid;
 }
 
 static u8 *drm_find_cea_extension(const struct edid *edid)
 {
int ret;
-   int idx = 1;
+   int idx;
int length = EDID_LENGTH;
struct displayid_block *block;
u8 *cea;
@@ -3232,7 +3239,7 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
return cea;
 
/* CEA blocks can also be found embedded in a DisplayID block */
-   displayid = drm_find_displayid_extension(edid);
+   displayid = drm_find_displayid_extension(edid, );
if (!displayid)
return NULL;
 
@@ -5182,12 +5189,12 @@ static int add_displayid_detailed_modes(struct 
drm_connector *connector,
 {
u8 *displayid;
int ret;
-   int idx = 1;
+   int idx;
int length = EDID_LENGTH;
struct displayid_block *block;
int num_modes = 0;
 
-   displayid = drm_find_displayid_extension(edid);
+   displayid = drm_find_displayid_extension(edid, );
if (!displayid)
return 0;
 
@@ -5837,17 +5844,11 @@ static int drm_parse_tiled_block(struct drm_connector 
*connector,
 }
 
 static int drm_parse_display_id(struct drm_connector *connector,
-   u8 *displayid, int length,
-   bool is_edid_extension)
+   u8 *displayid, int length, int idx)
 {
-   /* if this is an EDID extension the first byte will be 0x70 */
-   int idx = 0;
struct displayid_block *block;
int ret;
 
-   if (is_edid_extension)
-   idx = 1;
-
ret = validate_displayid(displayid, length, idx);
if (ret)
return ret;
@@ -5881,15 +5882,17 @@ static void drm_get_displayid(struct drm_connector 
*connector,
  struct edid *edid)
 {
void *displayid = NULL;
+   int idx;
int ret;
+
connector->has_tile = false;
-   displayid = drm_find_displayid_extension(edid);
+   displayid = drm_find_displayid_extension(edid, );
if (!displayid) {
/* drop reference to any tile group we had */
goto out_drop_ref;
}
 
-   ret = drm_parse_display_id(connector, displayid, EDID_LENGTH, true);
+   ret = drm_parse_display_id(connector, displayid, EDID_LENGTH, idx);
if (ret < 0)
goto out_drop_ref;
if (!connector->has_tile)
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/9] drm/edid: DisplayID parser fixes

From: Ville Syrjälä 

I was playing around with the tile stuff a bit and noticed a bunch of
issues in the DisplayID parser. This series aims to fix what I found.

Ville Syrjälä (9):
  drm: Constify topology id
  drm/edid: Swap some operands in for_each_displayid_db()
  drm/edid: Remove idx==1 assumptions from all over the DispID parsing
  drm/edid: Return DispID length from drm_find_displayid_extension()
  drm/edid: Move validate_displayid() drm_find_displayid_extension()
  drm/edid: Don't parse garbage as DispID blocks
  drm/edid: Don't include ext block csum in DispID size
  drm/edid: Clarify validate_displayid()
  drm/edid: Fix DispID tile parsing for override EDID

 drivers/gpu/drm/drm_connector.c |   6 +-
 drivers/gpu/drm/drm_crtc_internal.h |   1 +
 drivers/gpu/drm/drm_edid.c  | 103 +---
 include/drm/drm_connector.h |   4 +-
 include/drm/drm_displayid.h |   2 +-
 5 files changed, 54 insertions(+), 62 deletions(-)

-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/9] drm/edid: Return DispID length from drm_find_displayid_extension()

From: Ville Syrjälä 

As with the byte offset (idx) drm_find_displayid_extension() is
the only one who actually knows how much data the resulting DispID
block can contain. So return the length from therein instead of
assuming it's the EDID block length all over.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 3f9e659199af..18c55f3b20de 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3212,13 +3212,15 @@ static u8 *drm_find_edid_extension(const struct edid 
*edid, int ext_id)
 }
 
 
-static u8 *drm_find_displayid_extension(const struct edid *edid, int *idx)
+static u8 *drm_find_displayid_extension(const struct edid *edid,
+   int *length, int *idx)
 {
u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
 
if (!displayid)
return NULL;
 
+   *length = EDID_LENGTH;
*idx = 1;
 
return displayid;
@@ -3227,8 +3229,7 @@ static u8 *drm_find_displayid_extension(const struct edid 
*edid, int *idx)
 static u8 *drm_find_cea_extension(const struct edid *edid)
 {
int ret;
-   int idx;
-   int length = EDID_LENGTH;
+   int length, idx;
struct displayid_block *block;
u8 *cea;
u8 *displayid;
@@ -3239,7 +3240,7 @@ static u8 *drm_find_cea_extension(const struct edid *edid)
return cea;
 
/* CEA blocks can also be found embedded in a DisplayID block */
-   displayid = drm_find_displayid_extension(edid, );
+   displayid = drm_find_displayid_extension(edid, , );
if (!displayid)
return NULL;
 
@@ -5189,12 +5190,11 @@ static int add_displayid_detailed_modes(struct 
drm_connector *connector,
 {
u8 *displayid;
int ret;
-   int idx;
-   int length = EDID_LENGTH;
+   int length, idx;
struct displayid_block *block;
int num_modes = 0;
 
-   displayid = drm_find_displayid_extension(edid, );
+   displayid = drm_find_displayid_extension(edid, , );
if (!displayid)
return 0;
 
@@ -5882,17 +5882,17 @@ static void drm_get_displayid(struct drm_connector 
*connector,
  struct edid *edid)
 {
void *displayid = NULL;
-   int idx;
+   int length, idx;
int ret;
 
connector->has_tile = false;
-   displayid = drm_find_displayid_extension(edid, );
+   displayid = drm_find_displayid_extension(edid, , );
if (!displayid) {
/* drop reference to any tile group we had */
goto out_drop_ref;
}
 
-   ret = drm_parse_display_id(connector, displayid, EDID_LENGTH, idx);
+   ret = drm_parse_display_id(connector, displayid, length, idx);
if (ret < 0)
goto out_drop_ref;
if (!connector->has_tile)
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 8/9] drm/edid: Clarify validate_displayid()

From: Ville Syrjälä 

Throw out the magic '5' from validate_displayid() and replace with
the actual thing we mean sizeof(header)+checksum. Also rewrite the
checksum loop to be less hard to parse for mere mortals.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index f1ba06396c0a..185027f751f6 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -5098,7 +5098,7 @@ u32 drm_add_display_info(struct drm_connector *connector, 
const struct edid *edi
 
 static int validate_displayid(u8 *displayid, int length, int idx)
 {
-   int i;
+   int i, dispid_length;
u8 csum = 0;
struct displayid_hdr *base;
 
@@ -5107,15 +5107,18 @@ static int validate_displayid(u8 *displayid, int 
length, int idx)
DRM_DEBUG_KMS("base revision 0x%x, length %d, %d %d\n",
  base->rev, base->bytes, base->prod_id, base->ext_count);
 
-   if (base->bytes + 5 > length - idx)
+   /* +1 for DispID checksum */
+   dispid_length = sizeof(*base) + base->bytes + 1;
+   if (dispid_length > length - idx)
return -EINVAL;
-   for (i = idx; i <= base->bytes + 5; i++) {
-   csum += displayid[i];
-   }
+
+   for (i = 0; i < dispid_length; i++)
+   csum += displayid[idx + i];
if (csum) {
DRM_NOTE("DisplayID checksum invalid, remainder is %d\n", csum);
return -EINVAL;
}
+
return 0;
 }
 
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/9] drm/edid: Don't parse garbage as DispID blocks

From: Ville Syrjälä 

Currently the code assumes that the entire EDID extesion block
can be taken up by the DispID blocks. That is not true. There
is at least always the DispID checksum, and potentially fill
bytes if the extension block uses the interior fill scheme
to pad out to fill EDID block size.

So let's not parse the checksum or the fill bytes as DispID
blocks by having drm_find_displayid_extension() return the
actual length of the DispID data to the caller.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/drm_edid.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index dbd2e8474c2a..3067be710e5b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -3216,6 +3216,7 @@ static u8 *drm_find_displayid_extension(const struct edid 
*edid,
int *length, int *idx)
 {
u8 *displayid = drm_find_edid_extension(edid, DISPLAYID_EXT);
+   struct displayid_hdr *base;
int ret;
 
if (!displayid)
@@ -3228,6 +3229,9 @@ static u8 *drm_find_displayid_extension(const struct edid 
*edid,
if (ret)
return NULL;
 
+   base = (struct displayid_hdr *)[*idx];
+   *length = *idx + sizeof(*base) + base->bytes;
+
return displayid;
 }
 
-- 
2.24.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] dma-buf: heaps: bugfix for selftest failure

2020-03-13 Thread shuah


On 3/3/20 10:34 PM, Leon He wrote:

If the 'name' array in check_vgem() was not initialized to null, the
value of name[4] may be random. Which will cause strcmp(name, "vgem")
failed.


Nit: "to fail" instead of "failed"




Signed-off-by: Leon He 
---
  tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c 
b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
index cd5e1f6..21f3d19 100644
--- a/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
+++ b/tools/testing/selftests/dmabuf-heaps/dmabuf-heap.c
@@ -22,7 +22,7 @@
  static int check_vgem(int fd)
  {
drm_version_t version = { 0 };
-   char name[5];
+   char name[5] = { 0 };
int ret;
  
  	version.name_len = 4;




return !strcmp(name, "vgem");

While you are at it, why not change strcmp() to strncmp()?

thanks,
-- Shuah


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings

2020-03-13 Thread Neil Armstrong

On 13/03/2020 15:17, Laurent Pinchart wrote:
> Hi Neil,
> 
> On Fri, Mar 13, 2020 at 03:12:13PM +0100, Neil Armstrong wrote:
>> On 13/03/2020 14:40, Laurent Pinchart wrote:
>>> On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
 Add the ITE bridge HDMI it66121 bindings.

 Signed-off-by: Phong LE 
 ---
  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++
  1 file changed, 98 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml


[...]

 +
 +  pclk-dual-edge:
 +maxItems: 1
 +description: enable pclk dual edge mode.
>>>
>>> I'm having a bit of trouble understanding how this operates. Looking at
>>> the driver code the property is only taken into account to calculate the
>>> maximum allowed frequency. How is the IT66121 configured for single vs.
>>> dual pixel clock edge mode ?
>>
>> Dual edge mode is Dual-Data-Rate mode, the normal mode is 
>> MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
>> MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on 
>> a single clock period.
>>
>> This should be negociated at runtime, but the bus width should be specified 
>> somewhere to select
>> one of the modes.
> 
> How about replacing this property by bus-width to report the connected
> bus width ? It should then become an endpoint property.

It was my thought.

The mediatek dpi also uses this property, which is also very wrong in the same 
way.

Neil

[...]
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/scheduler: fix inconsistent locking of job_list_lock

2020-03-13 Thread Lucas Stach

Hi Alex,

since you seem to be picking up scheduler patches, can I ask you to
take this one through your tree?

Regards,
Lucas

On Mo, 2020-01-20 at 11:59 +0100, Christian König wrote:
> Am 20.01.20 um 11:51 schrieb Lucas Stach:
> > 1db8c142b6c5 (drm/scheduler: Add drm_sched_suspend/resume_timeout()) made
> > the job_list_lock IRQ safe in as the suspend/resume calls were expected to
> > be called from IRQ context. This usage never materialized in upstream.
> > Instead amdgpu started locking the job_list_lock in an IRQ unsafe way in
> > amdgpu_ib_preempt_mark_partial_job() and amdgpu_ib_preempt_job_recovery(),
> > which leads to potential deadlock if one would actually start to call the
> > drm_sched_suspend/resume_timeout functions from IRQ context.
> > 
> > As no current user needs the locking to be IRQ safe, the local IRQ
> > disable/enable is pure overhead. Fix the inconsistent locking by changing
> > all uses of job_list_lock to use the IRQ unsafe locking primitives.
> > 
> > Signed-off-by: Lucas Stach 
> 
> Reviewed-by: Christian König , do you want to 
> merge through drm-misc-next or should I push it to amd-staging-drm-next?
> 
> Regards,
> Christian.
> 
> > ---
> >   drivers/gpu/drm/scheduler/sched_main.c | 38 ++
> >   1 file changed, 15 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index 3c57e84222ca..d1b2dd99 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -220,8 +220,7 @@ EXPORT_SYMBOL(drm_sched_fault);
> >*
> >* Suspend the delayed work timeout for the scheduler. This is done by
> >* modifying the delayed work timeout to an arbitrary large value,
> > - * MAX_SCHEDULE_TIMEOUT in this case. Note that this function can be
> > - * called from an IRQ context.
> > + * MAX_SCHEDULE_TIMEOUT in this case.
> >*
> >* Returns the timeout remaining
> >*
> > @@ -250,43 +249,39 @@ EXPORT_SYMBOL(drm_sched_suspend_timeout);
> >* @sched: scheduler instance for which to resume the timeout
> >* @remaining: remaining timeout
> >*
> > - * Resume the delayed work timeout for the scheduler. Note that
> > - * this function can be called from an IRQ context.
> > + * Resume the delayed work timeout for the scheduler.
> >*/
> >   void drm_sched_resume_timeout(struct drm_gpu_scheduler *sched,
> > unsigned long remaining)
> >   {
> > -   unsigned long flags;
> > -
> > -   spin_lock_irqsave(>job_list_lock, flags);
> > +   spin_lock(>job_list_lock);
> >   
> > if (list_empty(>ring_mirror_list))
> > cancel_delayed_work(>work_tdr);
> > else
> > mod_delayed_work(system_wq, >work_tdr, remaining);
> >   
> > -   spin_unlock_irqrestore(>job_list_lock, flags);
> > +   spin_unlock(>job_list_lock);
> >   }
> >   EXPORT_SYMBOL(drm_sched_resume_timeout);
> >   
> >   static void drm_sched_job_begin(struct drm_sched_job *s_job)
> >   {
> > struct drm_gpu_scheduler *sched = s_job->sched;
> > -   unsigned long flags;
> >   
> > -   spin_lock_irqsave(>job_list_lock, flags);
> > +   spin_lock(>job_list_lock);
> > list_add_tail(_job->node, >ring_mirror_list);
> > drm_sched_start_timeout(sched);
> > -   spin_unlock_irqrestore(>job_list_lock, flags);
> > +   spin_unlock(>job_list_lock);
> >   }
> >   
> >   static void drm_sched_job_timedout(struct work_struct *work)
> >   {
> > struct drm_gpu_scheduler *sched;
> > struct drm_sched_job *job;
> > -   unsigned long flags;
> >   
> > sched = container_of(work, struct drm_gpu_scheduler, work_tdr.work);
> > +
> > job = list_first_entry_or_null(>ring_mirror_list,
> >struct drm_sched_job, node);
> >   
> > @@ -303,9 +298,9 @@ static void drm_sched_job_timedout(struct work_struct 
> > *work)
> > }
> > }
> >   
> > -   spin_lock_irqsave(>job_list_lock, flags);
> > +   spin_lock(>job_list_lock);
> > drm_sched_start_timeout(sched);
> > -   spin_unlock_irqrestore(>job_list_lock, flags);
> > +   spin_unlock(>job_list_lock);
> >   }
> >   
> >/**
> > @@ -368,7 +363,6 @@ EXPORT_SYMBOL(drm_sched_increase_karma);
> >   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job 
> > *bad)
> >   {
> > struct drm_sched_job *s_job, *tmp;
> > -   unsigned long flags;
> >   
> > kthread_park(sched->thread);
> >   
> > @@ -388,9 +382,9 @@ void drm_sched_stop(struct drm_gpu_scheduler *sched, 
> > struct drm_sched_job *bad)
> >  * remove job from ring_mirror_list.
> >  * Locking here is for concurrent resume timeout
> >  */
> > -   spin_lock_irqsave(>job_list_lock, flags);
> > +   spin_lock(>job_list_lock);
> > list_del_init(_job->node);
> > -   spin_unlock_irqrestore(>job_list_lock, flags);
> > +

Re: [PATCH libdrm] modetest: call drmModeCrtcSetGamma() only if add_property_optional returns true

2020-03-13 Thread Ilia Mirkin

It's to restore the gamma ramp to be the default linear one. Let's say
that the driver doesn't have the GAMMA_LUT property support, and then
you want to modeset with C8 (indexed) format. That means that modeset
has to set the LUT to make the indexed lookups work (which it does, it
all works, you celebrate). Then you want to run modeset with e.g.
XR24, and the colors are all black! The LUT is persistent across
modesets, so it has to reset the ramp to linear.

You could condition calling set_gamma on crtc->gamma_size > 0, I think
-- it didn't occur to me that there would be drivers that didn't
support a LUT.

On Fri, Mar 13, 2020 at 6:08 AM Rohit Visavalia  wrote:
>
> Hi Ilia Mirkin,
>
>
>
> But it should not go for setting gamma(drmModeCrtcSetGamma) as user has not 
> asked to do so in just simple mode set command(modetest -M  -s 
> 42:3840x2160@RG16).
>
>
>
> What is the requirement for setting gamma drmModeCrtcSetGamma() if user has 
> not asked ?
>
>
>
> Thanks
> Rohit
>
>
>
> From: Ilia Mirkin [mailto:imir...@alum.mit.edu]
> Sent: Thursday, March 12, 2020 3:49 PM
> To: Rohit Visavalia 
> Cc: Devarsh Thakkar ; dri-devel 
> ; emil.veli...@collabora.com; Ville Syrjälä 
> ; Hyun Kwon ; Ranganathan Sk 
> ; Dhaval Rajeshbhai Shah ; Varunkumar 
> Allagadapa 
> Subject: Re: [PATCH libdrm] modetest: call drmModeCrtcSetGamma() only if 
> add_property_optional returns true
>
>
>
> CAUTION: This message has originated from an External Source. Please use 
> proper judgment and caution when opening attachments, clicking links, or 
> responding to this email.
>
>
>
> Hm. I'm not sure offhand how to check if drmModeCrtcSetGamma is supported. I 
> guess you could check if gamma size > 0 or something?
>
>
>
> On Thu, Mar 12, 2020, 02:39 Rohit Visavalia  wrote:
>
> Hi Ilia Mirkin,
>
> Thanks for the review.
>
> By old-fashioned way you mean to say using drmModeCrtcSetGamma()? If yes 
> then, it shows error as "failed to set gamma: Function no implemented" if any 
> platform specific drm has no gamma property implemented.
>
> Current code shows error while running modetest for Xilinx drm as it doesn't 
> supports gamma property and ideally it should not show error as gamma is 
> optional property, so it doesn't serve the purpose of optional property.
>
> Please correct me if I am missing anything.
>
> Thanks
> Rohit
>
> > -Original Message-
> > From: Ilia Mirkin [mailto:imir...@alum.mit.edu]
> > Sent: Tuesday, March 3, 2020 7:08 PM
> > To: Devarsh Thakkar 
> > Cc: Rohit Visavalia ; dri-devel@lists.freedesktop.org;
> > emil.veli...@collabora.com; Ville Syrjälä ; 
> > Hyun
> > Kwon ; Ranganathan Sk ; Dhaval
> > Rajeshbhai Shah ; Varunkumar Allagadapa
> > 
> > Subject: Re: [PATCH libdrm] modetest: call drmModeCrtcSetGamma() only if
> > add_property_optional returns true
> >
> > EXTERNAL EMAIL
> >
> > Pretty sure the current code is right. If the GAMMA_LUT property can't be 
> > set,
> > it tries to set gamma the old-fashioned way.
> >
> > On Tue, Mar 3, 2020 at 8:12 AM Devarsh Thakkar 
> > wrote:
> > >
> > > Hi Rohit,
> > >
> > > This makes sense to me as gamma was implemented as optional property.
> > > Reviewed-By: "Devarsh Thakkar "
> > >
> > > @emil.veli...@collabora.com, @imir...@alum.mit.edu, @Ville Syrjälä,
> > Could you please ack and help merge this patch if it also look good to you ?
> > >
> > > Regards,
> > > Devarsh
> > >
> > > > -Original Message-
> > > > From: Rohit Visavalia
> > > > Sent: 27 February 2020 00:40
> > > > To: Rohit Visavalia ;
> > > > dri-devel@lists.freedesktop.org; imir...@alum.mit.edu;
> > > > emil.veli...@collabora.com
> > > > Cc: Hyun Kwon ; Ranganathan Sk ;
> > > > Dhaval Rajeshbhai Shah ; Varunkumar Allagadapa
> > > > ; Devarsh Thakkar 
> > > > Subject: RE: [PATCH libdrm] modetest: call drmModeCrtcSetGamma()
> > > > only if add_property_optional returns true
> > > >
> > > > Gentle reminder.
> > > >
> > > > + Ilia Mirkin, +Emil Velikov.
> > > >
> > > > Thanks & Regards,
> > > > Rohit
> > > >
> > > > > -Original Message-
> > > > > From: Rohit Visavalia [mailto:rohit.visava...@xilinx.com]
> > > > > Sent: Tuesday, February 25, 2020 3:08 PM
> > > > > To: dri-devel@lists.freedesktop.org
> > > > > Cc: Hyun Kwon ; Ranganathan Sk ;
> > > > > Dhaval Rajeshbhai Shah ; Varunkumar Allagadapa
> > > > > ; Devarsh Thakkar ;
> > > > > Rohit Visavalia 
> > > > > Subject: [PATCH libdrm] modetest: call drmModeCrtcSetGamma() only
> > > > > if add_property_optional returns true
> > > > >
> > > > > gamma is a optional property then also it prints error message, so
> > > > > set gamma only if add_property_optional() returns true.
> > > > >
> > > > > Signed-off-by: Rohit Visavalia 
> > > > > ---
> > > > >  tests/modetest/modetest.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > > > > index b907ab3..379b9ea 100644
> > > > > --- a/tests/modetest/modetest.c
> > > > > +++

Re: [PATCH] drm/amd/display: Fix pageflip event race condition for DCN. (v2)

2020-03-13 Thread Michel Dänzer

On 2020-03-13 1:35 p.m., Kazlauskas, Nicholas wrote:
> On 2020-03-12 10:32 a.m., Alex Deucher wrote:
>> On Thu, Mar 5, 2020 at 4:21 PM Mario Kleiner
>>  wrote:
>>>
>>> Commit '16f17eda8bad ("drm/amd/display: Send vblank and user
>>> events at vsartup for DCN")' introduces a new way of pageflip
>>> completion handling for DCN, and some trouble.
>>>
>>> The current implementation introduces a race condition, which
>>> can cause pageflip completion events to be sent out one vblank
>>> too early, thereby confusing userspace and causing flicker:
>>>
>>> prepare_flip_isr():
>>>
>>> 1. Pageflip programming takes the ddev->event_lock.
>>> 2. Sets acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED
>>> 3. Releases ddev->event_lock.
>>>
>>> --> Deadline for surface address regs double-buffering passes on
>>>  target pipe.
>>>
>>> 4. dc_commit_updates_for_stream() MMIO programs the new pageflip
>>>     into hw, but too late for current vblank.
>>>
>>> => pflip_status == AMDGPU_FLIP_SUBMITTED, but flip won't complete
>>>     in current vblank due to missing the double-buffering deadline
>>>     by a tiny bit.
>>>
>>> 5. VSTARTUP trigger point in vblank is reached, VSTARTUP irq fires,
>>>     dm_dcn_crtc_high_irq() gets called.
>>>
>>> 6. Detects pflip_status == AMDGPU_FLIP_SUBMITTED and assumes the
>>>     pageflip has been completed/will complete in this vblank and
>>>     sends out pageflip completion event to userspace and resets
>>>     pflip_status = AMDGPU_FLIP_NONE.
>>>
>>> => Flip completion event sent out one vblank too early.
>>>
>>> This behaviour has been observed during my testing with measurement
>>> hardware a couple of time.
>>>
>>> The commit message says that the extra flip event code was added to
>>> dm_dcn_crtc_high_irq() to prevent missing to send out pageflip events
>>> in case the pflip irq doesn't fire, because the "DCH HUBP" component
>>> is clock gated and doesn't fire pflip irqs in that state. Also that
>>> this clock gating may happen if no planes are active. According to
>>> Nicholas, the clock gating can also happen if psr is active, and the
>>> gating is controlled independently by the hardware, so difficult to
>>> detect if and when the completion code in above commit is needed.
>>>
>>> This patch tries the following solution: It only executes the extra
>>> pflip
>>> completion code in dm_dcn_crtc_high_irq() iff the hardware reports
>>> that there aren't any surface updated pending in the double-buffered
>>> surface scanout address registers. Otherwise it leaves pflip completion
>>> to the pflip irq handler, for a more race-free experience.
>>>
>>> This would only guard against the order of events mentioned above.
>>> If Step 5 (VSTARTUP trigger) happens before step 4 then this won't help
>>> at all, because 1-3 + 5 might happen even without the hw being
>>> programmed
>>> at all, ie. no surface update pending because none yet programmed
>>> into hw.
>>>
>>> Therefore this patch also changes locking in amdgpu_dm_commit_planes(),
>>> so that prepare_flip_isr() and dc_commit_updates_for_stream() are done
>>> under event_lock protection within the same critical section.
>>>
>>> v2: Take Nicholas comments into account, try a different solution.
>>>
>>> Lightly tested on Polaris (locking) and Raven (the whole DCN stuff).
>>> Seems to work without causing obvious new trouble.
>>
>> Nick, any comments on this?  Can we get this committed or do you think
>> it needs additional rework?
>>
>> Thanks,
>>
>> Alex
> 
> Hi Alex, Mario,
> 
> This might be a little strange, but if we want to get this in as a fix
> for regressions caused by the original vblank and user events at
> vstartup patch then I'm actually going to give my reviewed by on the
> *v1* of this patch (but not this v2):
> 
> Reviewed-by: Nicholas Kazlauskas 
> 
> You can feel free to apply that one.
> 
> Reason 1: After having thought about it some more I don't think we
> enable anything today that has hubp powered down at the same time we
> expect to be waiting for a flip - eg. DMCU powering down HUBP during PSR
> entry. Static screen interrupt should happen after that flip finishes I
> think.
> 
> The CRTC can still be powered on with zero planes, and I don't think any
> userspace explicitly asks for vblank events in this case but it doesn't
> hurt to have the check.
> 
> Reason 2: This new patch will need much more thorough testing from side
> to fully understand the consequences of locking the entire DC commit
> sequence. For just a page flip that sounds fine, but for anything more
> than (eg. full updates, modesets, etc) I don't think we want to be
> disabling interrupts for potentially many milliseconds.

Ah! I was wondering where the attached splat comes from, but I think
this explains it: With this patch amdgpu_dm_commit_planes keeps the
pcrtc->dev->event_lock spinlock locked while calling
dc_commit_updates_for_stream, which ends up calling
smu_set_display_count, which tries to lock a mutex.


-- 
Earthling Michel Dänzer

Re: [PATCH v3] drm/bochs: downgrade pci_request_region failure from error to warning

2020-03-13 Thread Gerd Hoffmann

  Hi,

> > +   if (pci_request_region(pdev, 0, "bochs-drm") != 0)
> > +   DRM_WARN("Cannot request framebuffer, boot fb still active?\n");
> So you could use drm_WARN() which is what is preferred these days.

Nope, this isn't yet in -fixes.

cheers,
  Gerd

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/sched: add run job trace

2020-03-13 Thread Alex Deucher

On Fri, Mar 13, 2020 at 6:18 AM Christian König
 wrote:
>
> Am 13.03.20 um 10:56 schrieb Lucas Stach:
> > From: Robert Beckett 
> >
> > Add a new trace event to show when jobs are run on the HW.
> >
> > Signed-off-by: Robert Beckett 
> > Signed-off-by: Lucas Stach 
>
> There is also the scheduled fence we could used for this, but this trace
> point adds a few extra fields which might be useful.
>
> Acked-by: Christian König 

Applied.  thanks!

Alex

>
> > ---
> >   .../gpu/drm/scheduler/gpu_scheduler_trace.h   | 27 +++
> >   drivers/gpu/drm/scheduler/sched_main.c|  1 +
> >   2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h 
> > b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> > index d79086498aff..877ce9b127f1 100644
> > --- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> > @@ -59,6 +59,33 @@ TRACE_EVENT(drm_sched_job,
> > __entry->job_count, __entry->hw_job_count)
> >   );
> >
> > +TRACE_EVENT(drm_run_job,
> > + TP_PROTO(struct drm_sched_job *sched_job, struct drm_sched_entity 
> > *entity),
> > + TP_ARGS(sched_job, entity),
> > + TP_STRUCT__entry(
> > +  __field(struct drm_sched_entity *, entity)
> > +  __field(struct dma_fence *, fence)
> > +  __field(const char *, name)
> > +  __field(uint64_t, id)
> > +  __field(u32, job_count)
> > +  __field(int, hw_job_count)
> > +  ),
> > +
> > + TP_fast_assign(
> > +__entry->entity = entity;
> > +__entry->id = sched_job->id;
> > +__entry->fence = _job->s_fence->finished;
> > +__entry->name = sched_job->sched->name;
> > +__entry->job_count = 
> > spsc_queue_count(>job_queue);
> > +__entry->hw_job_count = atomic_read(
> > +_job->sched->hw_rq_count);
> > +),
> > + TP_printk("entity=%p, id=%llu, fence=%p, ring=%s, job count:%u, 
> > hw job count:%d",
> > +   __entry->entity, __entry->id,
> > +   __entry->fence, __entry->name,
> > +   __entry->job_count, __entry->hw_job_count)
> > +);
> > +
> >   TRACE_EVENT(drm_sched_process_job,
> >   TP_PROTO(struct drm_sched_fence *fence),
> >   TP_ARGS(fence),
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index 71ce6215956f..34231b7163cc 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -773,6 +773,7 @@ static int drm_sched_main(void *param)
> >   atomic_inc(>hw_rq_count);
> >   drm_sched_job_begin(sched_job);
> >
> > + trace_drm_run_job(sched_job, entity);
> >   fence = sched->ops->run_job(sched_job);
> >   drm_sched_fence_scheduled(s_fence);
> >
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings

Hi Neil,

On Fri, Mar 13, 2020 at 03:12:13PM +0100, Neil Armstrong wrote:
> On 13/03/2020 14:40, Laurent Pinchart wrote:
> > On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> >> Add the ITE bridge HDMI it66121 bindings.
> >>
> >> Signed-off-by: Phong LE 
> >> ---
> >>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++
> >>  1 file changed, 98 insertions(+)
> >>  create mode 100644 
> >> Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >>
> >> diff --git 
> >> a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml 
> >> b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >> new file mode 100644
> >> index ..1717e880d130
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> >> @@ -0,0 +1,98 @@
> >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: ITE it66121 HDMI bridge Device Tree Bindings
> >> +
> >> +maintainers:
> >> +  - Phong LE 
> >> +  - Neil Armstrong 
> >> +
> >> +description: |
> >> +  The IT66121 is a high-performance and low-power single channel HDMI
> >> +  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward 
> >> compatible
> >> +  to DVI 1.0 specifications.
> >> +
> >> +properties:
> >> +  compatible:
> >> +const: ite,it66121
> >> +
> >> +  reg:
> >> +maxItems: 1
> >> +description: base I2C address of the device
> >> +
> >> +  reset-gpios:
> >> +maxItems: 1
> >> +description: GPIO connected to active low reset
> >> +
> >> +  vrf12-supply:
> >> +maxItems: 1
> >> +description: Regulator for 1.2V analog core power.
> >> +
> >> +  vcn33-supply:
> >> +maxItems: 1
> >> +description: Regulator for 3.3V digital core power.
> >> +
> >> +  vcn18-supply:
> >> +maxItems: 1
> >> +description: Regulator for 1.8V IO core power.
> >> +
> >> +  interrupts:
> >> +maxItems: 1
> >> +
> >> +  pclk-dual-edge:
> >> +maxItems: 1
> >> +description: enable pclk dual edge mode.
> > 
> > I'm having a bit of trouble understanding how this operates. Looking at
> > the driver code the property is only taken into account to calculate the
> > maximum allowed frequency. How is the IT66121 configured for single vs.
> > dual pixel clock edge mode ?
> 
> Dual edge mode is Dual-Data-Rate mode, the normal mode is 
> MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
> MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a 
> single clock period.
> 
> This should be negociated at runtime, but the bus width should be specified 
> somewhere to select
> one of the modes.

How about replacing this property by bus-width to report the connected
bus width ? It should then become an endpoint property.

> >> +
> >> +  port:
> >> +type: object
> >> +
> >> +properties:
> >> +  endpoint:
> >> +type: object
> >> +description: |
> >> +  Input endpoints of the bridge.
> >> +
> >> +required:
> >> +  - endpoint
> > 
> > You should have two ports, one for the bridge input, and one for the
> > bridge output.
> > 
> >> +
> >> +required:
> >> +  - compatible
> >> +  - reg
> >> +  - reset-gpios
> >> +  - vrf12-supply
> >> +  - vcn33-supply
> >> +  - vcn18-supply
> >> +  - interrupts
> >> +  - port
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +i2c6 {
> >> +  #address-cells = <1>;
> >> +  #size-cells = <0>;
> >> +
> >> +  it66121hdmitx: it66121hdmitx@4c {
> >> +compatible = "ite,it66121";
> >> +pinctrl-names = "default";
> >> +pinctrl-0 = <_pins_default>;
> >> +vcn33-supply = <_vcn33_wifi_reg>;
> >> +vcn18-supply = <_vcn18_reg>;
> >> +vrf12-supply = <_vrf12_reg>;
> >> +reset-gpios = < 160 1 /* GPIO_ACTIVE_LOW */>;
> >> +interrupt-parent = <>;
> >> +interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> >> +reg = <0x4c>;
> >> +pclk-dual-edge;
> >> +
> >> +port {
> >> +  it66121_in: endpoint {
> >> +remote-endpoint = <_out>;
> >> +  };
> >> +};
> >> +  };
> >> +};

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] backlight: lp855x: Ensure regulators are disabled on probe failure

2020-03-13 Thread Jon Hunter

Hi Lee, Daniel,

On 24/02/2020 14:37, Daniel Thompson wrote:
> On Mon, Feb 24, 2020 at 02:07:48PM +, Jon Hunter wrote:
>> If probing the LP885x backlight fails after the regulators have been
>> enabled, then the following warning is seen when releasing the
>> regulators ...
>>
>>  WARNING: CPU: 1 PID: 289 at drivers/regulator/core.c:2051 
>> _regulator_put.part.28+0x158/0x160
>>  Modules linked in: tegra_xudc lp855x_bl(+) host1x pwm_tegra ip_tables 
>> x_tables ipv6 nf_defrag_ipv6
>>  CPU: 1 PID: 289 Comm: systemd-udevd Not tainted 5.6.0-rc2-next-20200224 #1
>>  Hardware name: NVIDIA Jetson TX1 Developer Kit (DT)
>>
>>  ...
>>
>>  Call trace:
>>   _regulator_put.part.28+0x158/0x160
>>   regulator_put+0x34/0x50
>>   devm_regulator_release+0x10/0x18
>>   release_nodes+0x12c/0x230
>>   devres_release_all+0x34/0x50
>>   really_probe+0x1c0/0x370
>>   driver_probe_device+0x58/0x100
>>   device_driver_attach+0x6c/0x78
>>   __driver_attach+0xb0/0xf0
>>   bus_for_each_dev+0x68/0xc8
>>   driver_attach+0x20/0x28
>>   bus_add_driver+0x160/0x1f0
>>   driver_register+0x60/0x110
>>   i2c_register_driver+0x40/0x80
>>   lp855x_driver_init+0x20/0x1000 [lp855x_bl]
>>   do_one_initcall+0x58/0x1a0
>>   do_init_module+0x54/0x1d0
>>   load_module+0x1d80/0x21c8
>>   __do_sys_finit_module+0xe8/0x100
>>   __arm64_sys_finit_module+0x18/0x20
>>   el0_svc_common.constprop.3+0xb0/0x168
>>   do_el0_svc+0x20/0x98
>>   el0_sync_handler+0xf4/0x1b0
>>   el0_sync+0x140/0x180
>>
>> Fix this by ensuring that the regulators are disabled, if enabled, on
>> probe failure.
>>
>> Finally, ensure that the vddio regulator is disabled in the driver
>> remove handler.
>>
>> Signed-off-by: Jon Hunter 
> 
> Reviewed-by: Daniel Thompson 
I received a bounce from Milo's email and so I am not sure that his
email address is still valid.

Can either of you pick this up?

Not sure if we should update the MAINTAINERS as well?

Jon

-- 
nvpublic
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] omapfb: Remove unused writeback code

2020-03-13 Thread Tomi Valkeinen

On 13/03/2020 15:38, Laurent Pinchart wrote:

Hi Tomi,

On Fri, Mar 13, 2020 at 03:30:15PM +0200, Tomi Valkeinen wrote:

On 13/03/2020 15:22, Laurent Pinchart wrote:

On Fri, Mar 13, 2020 at 02:24:10PM +0200, Tomi Valkeinen wrote:

Remove unused writeback code. This code will never be used, as omapfb is
being deprecated.

I'm fine with that, but out of curiosity, is there any particular reason
to remove that code now instead of letting omapfb bitrot slowly ?

It conflicts with tidss's writeback code in TI kernel, when compiling tidss and
omapfb into the
kernel. I thought this is the easiest way to resolve that, as all the removed
code is dead code,
instead of trying to invent new names in tidss for a lot of functions.

Probably the functions in tidss still could use some renaming in the future,
but I didn't want to be
forced to do that because of omapfb's dead code...

Could you do both ? :-) It's not good using too generic names in tidss.
You can just prefix the functions with tidss_. There's a risk of
conflict with omapdrm too if the names are too generic.

I will, but I didn't want to start doing that right now. tidss has just been merged to mainline, and
tidss is still under active development, so doing such a change is an invitation to conflicts. I'd
like things to settle down a bit first.

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: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings

2020-03-13 Thread Neil Armstrong

On 13/03/2020 14:40, Laurent Pinchart wrote:
> Hi Phong,
> 
> Thank you for the patch.
> 
> On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
>> Add the ITE bridge HDMI it66121 bindings.
>>
>> Signed-off-by: Phong LE 
>> ---
>>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++
>>  1 file changed, 98 insertions(+)
>>  create mode 100644 
>> Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml 
>> b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>> new file mode 100644
>> index ..1717e880d130
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
>> @@ -0,0 +1,98 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: ITE it66121 HDMI bridge Device Tree Bindings
>> +
>> +maintainers:
>> +  - Phong LE 
>> +  - Neil Armstrong 
>> +
>> +description: |
>> +  The IT66121 is a high-performance and low-power single channel HDMI
>> +  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward 
>> compatible
>> +  to DVI 1.0 specifications.
>> +
>> +properties:
>> +  compatible:
>> +const: ite,it66121
>> +
>> +  reg:
>> +maxItems: 1
>> +description: base I2C address of the device
>> +
>> +  reset-gpios:
>> +maxItems: 1
>> +description: GPIO connected to active low reset
>> +
>> +  vrf12-supply:
>> +maxItems: 1
>> +description: Regulator for 1.2V analog core power.
>> +
>> +  vcn33-supply:
>> +maxItems: 1
>> +description: Regulator for 3.3V digital core power.
>> +
>> +  vcn18-supply:
>> +maxItems: 1
>> +description: Regulator for 1.8V IO core power.
>> +
>> +  interrupts:
>> +maxItems: 1
>> +
>> +  pclk-dual-edge:
>> +maxItems: 1
>> +description: enable pclk dual edge mode.
> 
> I'm having a bit of trouble understanding how this operates. Looking at
> the driver code the property is only taken into account to calculate the
> maximum allowed frequency. How is the IT66121 configured for single vs.
> dual pixel clock edge mode ?


Dual edge mode is Dual-Data-Rate mode, the normal mode is 
MEDIA_BUS_FMT_RGB888_1X24 and dual edge is
MEDIA_BUS_FMT_RGB888_2X12_LE (or MEDIA_BUS_FMT_RGB888_2X12_BE, not sure) on a 
single clock period.

This should be negociated at runtime, but the bus width should be specified 
somewhere to select
one of the modes.

Neil

> 
>> +
>> +  port:
>> +type: object
>> +
>> +properties:
>> +  endpoint:
>> +type: object
>> +description: |
>> +  Input endpoints of the bridge.
>> +
>> +required:
>> +  - endpoint
> 
> You should have two ports, one for the bridge input, and one for the
> bridge output.
> 
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reset-gpios
>> +  - vrf12-supply
>> +  - vcn33-supply
>> +  - vcn18-supply
>> +  - interrupts
>> +  - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +i2c6 {
>> +  #address-cells = <1>;
>> +  #size-cells = <0>;
>> +
>> +  it66121hdmitx: it66121hdmitx@4c {
>> +compatible = "ite,it66121";
>> +pinctrl-names = "default";
>> +pinctrl-0 = <_pins_default>;
>> +vcn33-supply = <_vcn33_wifi_reg>;
>> +vcn18-supply = <_vcn18_reg>;
>> +vrf12-supply = <_vrf12_reg>;
>> +reset-gpios = < 160 1 /* GPIO_ACTIVE_LOW */>;
>> +interrupt-parent = <>;
>> +interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
>> +reg = <0x4c>;
>> +pclk-dual-edge;
>> +
>> +port {
>> +  it66121_in: endpoint {
>> +remote-endpoint = <_out>;
>> +  };
>> +};
>> +  };
>> +};
> 

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/4] drm: bridge: add it66121 driver

Hi Phong,

Thank you for the patch.

On Wed, Mar 11, 2020 at 01:51:34PM +0100, Phong LE wrote:
> This commit is a simple driver for bridge HMDI it66121.
> The input format is RBG and there is no color conversion.
> Audio, HDCP and CEC are not supported yet.
> 
> Signed-off-by: Phong LE 
> ---
>  drivers/gpu/drm/bridge/Kconfig   |   8 +
>  drivers/gpu/drm/bridge/Makefile  |   1 +
>  drivers/gpu/drm/bridge/ite-it66121.c | 997 +++
>  3 files changed, 1006 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ite-it66121.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index aaed2347ace9..ae57b83f47c8 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -38,6 +38,14 @@ config DRM_DISPLAY_CONNECTOR
> on ARM-based platforms. Saying Y here when this driver is not needed
> will not cause any issue.
>  
> +config DRM_ITE_IT66121
> + tristate "ITE IT66121 HDMI bridge"
> + depends on OF
> + select DRM_KMS_HELPER
> + select REGMAP_I2C
> + help
> +   Support for ITE IT66121 HDMI bridge.
> +
>  config DRM_LVDS_CODEC
>   tristate "Transparent LVDS encoders and decoders support"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 6fb062b5b0f0..9fe80e820f64 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  obj-$(CONFIG_DRM_CDNS_DSI) += cdns-dsi.o
>  obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
> +obj-$(CONFIG_DRM_ITE_IT66121) += ite-it66121.o
>  obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o
>  obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += 
> megachips-stdp-ge-b850v3-fw.o
>  obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c 
> b/drivers/gpu/drm/bridge/ite-it66121.c
> new file mode 100644
> index ..7e1a90319a6a
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -0,0 +1,997 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2020 BayLibre, SAS
> + * Author: Phong LE 
> + * Copyright (C) 2018-2019, Artem Mygaiev
> + * Copyright (C) 2017, Fresco Logic, Incorporated.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

This header is not needed, you should include  instead.

> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define IT66121_MASTER_SEL_REG   0x10
> +#define IT66121_MASTER_SEL_HOST  BIT(0)
> +
> +#define IT66121_AFE_DRV_REG  0x61
> +#define IT66121_AFE_DRV_RST  BIT(4)
> +#define IT66121_AFE_DRV_PWD  BIT(5)
> +
> +#define IT66121_INPUT_MODE_REG   0x70
> +#define IT66121_INPUT_MODE_RGB   (0 << 6)
> +#define IT66121_INPUT_MODE_YUV422BIT(6)
> +#define IT66121_INPUT_MODE_YUV444(2 << 6)
> +#define IT66121_INPUT_MODE_CCIR656   BIT(4)
> +#define IT66121_INPUT_MODE_SYNCEMB   BIT(3)
> +#define IT66121_INPUT_MODE_DDR   BIT(2)
> +
> +#define IT66121_INPUT_CSC_REG0x72
> +#define IT66121_INPUT_CSC_ENDITHER   BIT(7)
> +#define IT66121_INPUT_CSC_ENUDFILTER BIT(6)
> +#define IT66121_INPUT_CSC_DNFREE_GO  BIT(5)
> +#define IT66121_INPUT_CSC_RGB_TO_YUV 0x02
> +#define IT66121_INPUT_CSC_YUV_TO_RGB 0x03
> +#define IT66121_INPUT_CSC_NO_CONV0x00
> +
> +#define IT66121_AFE_XP_REG   0x62
> +#define IT66121_AFE_XP_GAINBIT   BIT(7)
> +#define IT66121_AFE_XP_PWDPLLBIT(6)
> +#define IT66121_AFE_XP_ENI   BIT(5)
> +#define IT66121_AFE_XP_ENO   BIT(4)
> +#define IT66121_AFE_XP_RESETBBIT(3)
> +#define IT66121_AFE_XP_PWDI  BIT(2)
> +
> +#define IT66121_AFE_IP_REG   0x64
> +#define IT66121_AFE_IP_GAINBIT   BIT(7)
> +#define IT66121_AFE_IP_PWDPLLBIT(6)
> +#define IT66121_AFE_IP_CKSEL_05  (0 << 4)
> +#define IT66121_AFE_IP_CKSEL_1   BIT(4)
> +#define IT66121_AFE_IP_CKSEL_2   (2 << 4)
> +#define IT66121_AFE_IP_CKSEL_2OR4(3 << 4)
> +#define IT66121_AFE_IP_ER0   BIT(3)
> +#define IT66121_AFE_IP_RESETBBIT(2)
> +#define IT66121_AFE_IP_ENC   BIT(1)
> +#define IT66121_AFE_IP_EC1   BIT(0)
> +
> +#define IT66121_AFE_XP_EC1_REG   0x68
> +#define IT66121_AFE_XP_EC1_LOWCLKBIT(4)
> +
> +#define IT66121_SW_RST_REG   0x04
> +#define IT66121_SW_RST_REF

Re: [PATCH] omapfb: Remove unused writeback code

2020-03-13 Thread Tomi Valkeinen


On 13/03/2020 15:22, Laurent Pinchart wrote:

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 02:24:10PM +0200, Tomi Valkeinen wrote:

Remove unused writeback code. This code will never be used, as omapfb is
being deprecated.


I'm fine with that, but out of curiosity, is there any particular reason
to remove that code now instead of letting omapfb bitrot slowly ?


It conflicts with tidss's writeback code in TI kernel, when compiling tidss and omapfb into the 
kernel. I thought this is the easiest way to resolve that, as all the removed code is dead code, 
instead of trying to invent new names in tidss for a lot of functions.


Probably the functions in tidss still could use some renaming in the future, but I didn't want to be 
forced to do that because of omapfb's dead code...


So, not a super good reason, but on the other hand, removing dead code is 
always a good thing.

 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: [PATCH v2 2/4] dt-bindings: display: bridge: add it66121 bindings

Hi Phong,

Thank you for the patch.

On Wed, Mar 11, 2020 at 01:51:33PM +0100, Phong LE wrote:
> Add the ITE bridge HDMI it66121 bindings.
> 
> Signed-off-by: Phong LE 
> ---
>  .../bindings/display/bridge/ite,it66121.yaml  | 98 +++
>  1 file changed, 98 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml 
> b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> new file mode 100644
> index ..1717e880d130
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ite,it66121.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/ite,it66121.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ITE it66121 HDMI bridge Device Tree Bindings
> +
> +maintainers:
> +  - Phong LE 
> +  - Neil Armstrong 
> +
> +description: |
> +  The IT66121 is a high-performance and low-power single channel HDMI
> +  transmitter, fully compliant with HDMI 1.3a, HDCP 1.2 and backward 
> compatible
> +  to DVI 1.0 specifications.
> +
> +properties:
> +  compatible:
> +const: ite,it66121
> +
> +  reg:
> +maxItems: 1
> +description: base I2C address of the device
> +
> +  reset-gpios:
> +maxItems: 1
> +description: GPIO connected to active low reset
> +
> +  vrf12-supply:
> +maxItems: 1
> +description: Regulator for 1.2V analog core power.
> +
> +  vcn33-supply:
> +maxItems: 1
> +description: Regulator for 3.3V digital core power.
> +
> +  vcn18-supply:
> +maxItems: 1
> +description: Regulator for 1.8V IO core power.
> +
> +  interrupts:
> +maxItems: 1
> +
> +  pclk-dual-edge:
> +maxItems: 1
> +description: enable pclk dual edge mode.

I'm having a bit of trouble understanding how this operates. Looking at
the driver code the property is only taken into account to calculate the
maximum allowed frequency. How is the IT66121 configured for single vs.
dual pixel clock edge mode ?

> +
> +  port:
> +type: object
> +
> +properties:
> +  endpoint:
> +type: object
> +description: |
> +  Input endpoints of the bridge.
> +
> +required:
> +  - endpoint

You should have two ports, one for the bridge input, and one for the
bridge output.

> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +  - vrf12-supply
> +  - vcn33-supply
> +  - vcn18-supply
> +  - interrupts
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +i2c6 {
> +  #address-cells = <1>;
> +  #size-cells = <0>;
> +
> +  it66121hdmitx: it66121hdmitx@4c {
> +compatible = "ite,it66121";
> +pinctrl-names = "default";
> +pinctrl-0 = <_pins_default>;
> +vcn33-supply = <_vcn33_wifi_reg>;
> +vcn18-supply = <_vcn18_reg>;
> +vrf12-supply = <_vrf12_reg>;
> +reset-gpios = < 160 1 /* GPIO_ACTIVE_LOW */>;
> +interrupt-parent = <>;
> +interrupts = <4 8 /* IRQ_TYPE_LEVEL_LOW */>;
> +reg = <0x4c>;
> +pclk-dual-edge;
> +
> +port {
> +  it66121_in: endpoint {
> +remote-endpoint = <_out>;
> +  };
> +};
> +  };
> +};

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] omapfb: Remove unused writeback code

Hi Tomi,

On Fri, Mar 13, 2020 at 03:30:15PM +0200, Tomi Valkeinen wrote:
> On 13/03/2020 15:22, Laurent Pinchart wrote:
> > On Fri, Mar 13, 2020 at 02:24:10PM +0200, Tomi Valkeinen wrote:
> >> Remove unused writeback code. This code will never be used, as omapfb is
> >> being deprecated.
> > 
> > I'm fine with that, but out of curiosity, is there any particular reason
> > to remove that code now instead of letting omapfb bitrot slowly ?
> 
> It conflicts with tidss's writeback code in TI kernel, when compiling tidss 
> and omapfb into the 
> kernel. I thought this is the easiest way to resolve that, as all the removed 
> code is dead code, 
> instead of trying to invent new names in tidss for a lot of functions.
>
> Probably the functions in tidss still could use some renaming in the future, 
> but I didn't want to be 
> forced to do that because of omapfb's dead code...

Could you do both ? :-) It's not good using too generic names in tidss.
You can just prefix the functions with tidss_. There's a risk of
conflict with omapdrm too if the names are too generic.

> So, not a super good reason, but on the other hand, removing dead code is 
> always a good thing.

-- 
Regards,

Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] lib/scatterlist: add sg_set_dma_addr() function

2020-03-13 Thread Christian König


Am 13.03.20 um 12:21 schrieb Christoph Hellwig:

On Thu, Mar 12, 2020 at 11:19:28AM -0300, Jason Gunthorpe wrote:

The non-page scatterlist is also a big concern for RDMA as we have
drivers that want the page list, so even if we did as this series
contemplates I'd have still have to split the drivers and create the
notion of a dma-only SGL.

The drivers I looked at want a list of IOVA address, aligned to the
device "page size".  What other data do drivers want?


Well for GPUs I have the requirement that those IOVA addresses allow 
random access.


That's the reason why we currently convert the sg_table into a linear 
arrays of addresses and pages. To solve that keeping the length in 
separate optional array would be ideal for us.


But this is so a special use case that I'm not sure if we want to 
support this in the common framework or not.



Execept for the software protocol stack drivers, which of couse need pages for 
the
stack futher down.


Yes completely agree.

For the GPUs I will propose a patch to stop copying the page from the 
sg_table over into our linear arrays and see if anybody starts to scream.


I don't think so, but probably better to double check.

Thanks,
Christian.




I haven't used bio_vecs before, do they support chaining like SGL so
they can be very big? RDMA dma maps gigabytes of memory

bio_vecs itself don't have the chaining, but the bios build around them
do.  But each entry can map a huge pile.  If needed we could use the
same chaining scheme we use for scatterlists for bio_vecs as well, but
lets see if we really end up needing that.


So I'm guessing the path forward is something like

  - Add some generic dma_sg data structure and helper
  - Add dma mapping code to go from pages to dma_sg

That has been on my todo list for a while.  All the DMA consolidatation
is to prepare for that and we're finally getting close.


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] omapfb: Remove unused writeback code

Hi Tomi,

Thank you for the patch.

On Fri, Mar 13, 2020 at 02:24:10PM +0200, Tomi Valkeinen wrote:
> Remove unused writeback code. This code will never be used, as omapfb is
> being deprecated.

I'm fine with that, but out of curiosity, is there any particular reason
to remove that code now instead of letting omapfb bitrot slowly ?

> Signed-off-by: Tomi Valkeinen 
> ---
>  drivers/video/fbdev/omap2/omapfb/dss/dispc.c | 114 ---
>  drivers/video/fbdev/omap2/omapfb/dss/dss.h   |  20 
>  2 files changed, 134 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c 
> b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> index ce37da85cc45..4a16798b2ecd 100644
> --- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> +++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
> @@ -557,11 +557,6 @@ u32 dispc_mgr_get_sync_lost_irq(enum omap_channel 
> channel)
>  }
>  EXPORT_SYMBOL(dispc_mgr_get_sync_lost_irq);
>  
> -u32 dispc_wb_get_framedone_irq(void)
> -{
> - return DISPC_IRQ_FRAMEDONEWB;
> -}
> -
>  bool dispc_mgr_go_busy(enum omap_channel channel)
>  {
>   return mgr_fld_read(channel, DISPC_MGR_FLD_GO) == 1;
> @@ -579,30 +574,6 @@ void dispc_mgr_go(enum omap_channel channel)
>  }
>  EXPORT_SYMBOL(dispc_mgr_go);
>  
> -bool dispc_wb_go_busy(void)
> -{
> - return REG_GET(DISPC_CONTROL2, 6, 6) == 1;
> -}
> -
> -void dispc_wb_go(void)
> -{
> - enum omap_plane plane = OMAP_DSS_WB;
> - bool enable, go;
> -
> - enable = REG_GET(DISPC_OVL_ATTRIBUTES(plane), 0, 0) == 1;
> -
> - if (!enable)
> - return;
> -
> - go = REG_GET(DISPC_CONTROL2, 6, 6) == 1;
> - if (go) {
> - DSSERR("GO bit not down for WB\n");
> - return;
> - }
> -
> - REG_FLD_MOD(DISPC_CONTROL2, 1, 6, 6);
> -}
> -
>  static void dispc_ovl_write_firh_reg(enum omap_plane plane, int reg, u32 
> value)
>  {
>   dispc_write_reg(DISPC_OVL_FIR_COEF_H(plane, reg), value);
> @@ -1028,13 +999,6 @@ static enum omap_channel dispc_ovl_get_channel_out(enum 
> omap_plane plane)
>   }
>  }
>  
> -void dispc_wb_set_channel_in(enum dss_writeback_channel channel)
> -{
> - enum omap_plane plane = OMAP_DSS_WB;
> -
> - REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), channel, 18, 16);
> -}
> -
>  static void dispc_ovl_set_burst_size(enum omap_plane plane,
>   enum omap_burst_size burst_size)
>  {
> @@ -2805,74 +2769,6 @@ int dispc_ovl_setup(enum omap_plane plane, const 
> struct omap_overlay_info *oi,
>  }
>  EXPORT_SYMBOL(dispc_ovl_setup);
>  
> -int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
> - bool mem_to_mem, const struct omap_video_timings *mgr_timings)
> -{
> - int r;
> - u32 l;
> - enum omap_plane plane = OMAP_DSS_WB;
> - const int pos_x = 0, pos_y = 0;
> - const u8 zorder = 0, global_alpha = 0;
> - const bool replication = false;
> - bool truncation;
> - int in_width = mgr_timings->x_res;
> - int in_height = mgr_timings->y_res;
> - enum omap_overlay_caps caps =
> - OMAP_DSS_OVL_CAP_SCALE | OMAP_DSS_OVL_CAP_PRE_MULT_ALPHA;
> -
> - DSSDBG("dispc_wb_setup, pa %x, pa_uv %x, %d,%d -> %dx%d, cmode %x, "
> - "rot %d, mir %d\n", wi->paddr, wi->p_uv_addr, in_width,
> - in_height, wi->width, wi->height, wi->color_mode, wi->rotation,
> - wi->mirror);
> -
> - r = dispc_ovl_setup_common(plane, caps, wi->paddr, wi->p_uv_addr,
> - wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
> - wi->height, wi->color_mode, wi->rotation, wi->mirror, zorder,
> - wi->pre_mult_alpha, global_alpha, wi->rotation_type,
> - replication, mgr_timings, mem_to_mem);
> -
> - switch (wi->color_mode) {
> - case OMAP_DSS_COLOR_RGB16:
> - case OMAP_DSS_COLOR_RGB24P:
> - case OMAP_DSS_COLOR_ARGB16:
> - case OMAP_DSS_COLOR_RGBA16:
> - case OMAP_DSS_COLOR_RGB12U:
> - case OMAP_DSS_COLOR_ARGB16_1555:
> - case OMAP_DSS_COLOR_XRGB16_1555:
> - case OMAP_DSS_COLOR_RGBX16:
> - truncation = true;
> - break;
> - default:
> - truncation = false;
> - break;
> - }
> -
> - /* setup extra DISPC_WB_ATTRIBUTES */
> - l = dispc_read_reg(DISPC_OVL_ATTRIBUTES(plane));
> - l = FLD_MOD(l, truncation, 10, 10); /* TRUNCATIONENABLE */
> - l = FLD_MOD(l, mem_to_mem, 19, 19); /* WRITEBACKMODE */
> - if (mem_to_mem)
> - l = FLD_MOD(l, 1, 26, 24);  /* CAPTUREMODE */
> - else
> - l = FLD_MOD(l, 0, 26, 24);  /* CAPTUREMODE */
> - dispc_write_reg(DISPC_OVL_ATTRIBUTES(plane), l);
> -
> - if (mem_to_mem) {
> - /* WBDELAYCOUNT */
> - REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane), 0, 7, 0);
> - } else {
> - int wbdelay;
> -
> - wbdelay = min(mgr_timings->vfp + mgr_timings->vsw +
> - mgr_timings->vbp, 255);
> -
> -

[PATCH] omapfb: Remove unused writeback code

2020-03-13 Thread Tomi Valkeinen

Remove unused writeback code. This code will never be used, as omapfb is
being deprecated.

Signed-off-by: Tomi Valkeinen 
---
 drivers/video/fbdev/omap2/omapfb/dss/dispc.c | 114 ---
 drivers/video/fbdev/omap2/omapfb/dss/dss.h   |  20 
 2 files changed, 134 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c 
b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
index ce37da85cc45..4a16798b2ecd 100644
--- a/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
+++ b/drivers/video/fbdev/omap2/omapfb/dss/dispc.c
@@ -557,11 +557,6 @@ u32 dispc_mgr_get_sync_lost_irq(enum omap_channel channel)
 }
 EXPORT_SYMBOL(dispc_mgr_get_sync_lost_irq);
 
-u32 dispc_wb_get_framedone_irq(void)
-{
-   return DISPC_IRQ_FRAMEDONEWB;
-}
-
 bool dispc_mgr_go_busy(enum omap_channel channel)
 {
return mgr_fld_read(channel, DISPC_MGR_FLD_GO) == 1;
@@ -579,30 +574,6 @@ void dispc_mgr_go(enum omap_channel channel)
 }
 EXPORT_SYMBOL(dispc_mgr_go);
 
-bool dispc_wb_go_busy(void)
-{
-   return REG_GET(DISPC_CONTROL2, 6, 6) == 1;
-}
-
-void dispc_wb_go(void)
-{
-   enum omap_plane plane = OMAP_DSS_WB;
-   bool enable, go;
-
-   enable = REG_GET(DISPC_OVL_ATTRIBUTES(plane), 0, 0) == 1;
-
-   if (!enable)
-   return;
-
-   go = REG_GET(DISPC_CONTROL2, 6, 6) == 1;
-   if (go) {
-   DSSERR("GO bit not down for WB\n");
-   return;
-   }
-
-   REG_FLD_MOD(DISPC_CONTROL2, 1, 6, 6);
-}
-
 static void dispc_ovl_write_firh_reg(enum omap_plane plane, int reg, u32 value)
 {
dispc_write_reg(DISPC_OVL_FIR_COEF_H(plane, reg), value);
@@ -1028,13 +999,6 @@ static enum omap_channel dispc_ovl_get_channel_out(enum 
omap_plane plane)
}
 }
 
-void dispc_wb_set_channel_in(enum dss_writeback_channel channel)
-{
-   enum omap_plane plane = OMAP_DSS_WB;
-
-   REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), channel, 18, 16);
-}
-
 static void dispc_ovl_set_burst_size(enum omap_plane plane,
enum omap_burst_size burst_size)
 {
@@ -2805,74 +2769,6 @@ int dispc_ovl_setup(enum omap_plane plane, const struct 
omap_overlay_info *oi,
 }
 EXPORT_SYMBOL(dispc_ovl_setup);
 
-int dispc_wb_setup(const struct omap_dss_writeback_info *wi,
-   bool mem_to_mem, const struct omap_video_timings *mgr_timings)
-{
-   int r;
-   u32 l;
-   enum omap_plane plane = OMAP_DSS_WB;
-   const int pos_x = 0, pos_y = 0;
-   const u8 zorder = 0, global_alpha = 0;
-   const bool replication = false;
-   bool truncation;
-   int in_width = mgr_timings->x_res;
-   int in_height = mgr_timings->y_res;
-   enum omap_overlay_caps caps =
-   OMAP_DSS_OVL_CAP_SCALE | OMAP_DSS_OVL_CAP_PRE_MULT_ALPHA;
-
-   DSSDBG("dispc_wb_setup, pa %x, pa_uv %x, %d,%d -> %dx%d, cmode %x, "
-   "rot %d, mir %d\n", wi->paddr, wi->p_uv_addr, in_width,
-   in_height, wi->width, wi->height, wi->color_mode, wi->rotation,
-   wi->mirror);
-
-   r = dispc_ovl_setup_common(plane, caps, wi->paddr, wi->p_uv_addr,
-   wi->buf_width, pos_x, pos_y, in_width, in_height, wi->width,
-   wi->height, wi->color_mode, wi->rotation, wi->mirror, zorder,
-   wi->pre_mult_alpha, global_alpha, wi->rotation_type,
-   replication, mgr_timings, mem_to_mem);
-
-   switch (wi->color_mode) {
-   case OMAP_DSS_COLOR_RGB16:
-   case OMAP_DSS_COLOR_RGB24P:
-   case OMAP_DSS_COLOR_ARGB16:
-   case OMAP_DSS_COLOR_RGBA16:
-   case OMAP_DSS_COLOR_RGB12U:
-   case OMAP_DSS_COLOR_ARGB16_1555:
-   case OMAP_DSS_COLOR_XRGB16_1555:
-   case OMAP_DSS_COLOR_RGBX16:
-   truncation = true;
-   break;
-   default:
-   truncation = false;
-   break;
-   }
-
-   /* setup extra DISPC_WB_ATTRIBUTES */
-   l = dispc_read_reg(DISPC_OVL_ATTRIBUTES(plane));
-   l = FLD_MOD(l, truncation, 10, 10); /* TRUNCATIONENABLE */
-   l = FLD_MOD(l, mem_to_mem, 19, 19); /* WRITEBACKMODE */
-   if (mem_to_mem)
-   l = FLD_MOD(l, 1, 26, 24);  /* CAPTUREMODE */
-   else
-   l = FLD_MOD(l, 0, 26, 24);  /* CAPTUREMODE */
-   dispc_write_reg(DISPC_OVL_ATTRIBUTES(plane), l);
-
-   if (mem_to_mem) {
-   /* WBDELAYCOUNT */
-   REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane), 0, 7, 0);
-   } else {
-   int wbdelay;
-
-   wbdelay = min(mgr_timings->vfp + mgr_timings->vsw +
-   mgr_timings->vbp, 255);
-
-   /* WBDELAYCOUNT */
-   REG_FLD_MOD(DISPC_OVL_ATTRIBUTES2(plane), wbdelay, 7, 0);
-   }
-
-   return r;
-}
-
 int dispc_ovl_enable(enum omap_plane plane, bool enable)
 {
DSSDBG("dispc_enable_plane %d, %d\n", plane, enable);
@@ -2903,16 +2799,6 @@ bool dispc_mgr_is_enabled(enum omap_channel channel)
 }
 EXPORT_SYMBOL(dispc_mgr_is_enabled);

Re: [PATCH] drm/amd/display: Fix pageflip event race condition for DCN. (v2)

2020-03-13 Thread Kazlauskas, Nicholas


On 2020-03-12 10:32 a.m., Alex Deucher wrote:

On Thu, Mar 5, 2020 at 4:21 PM Mario Kleiner  wrote:


Commit '16f17eda8bad ("drm/amd/display: Send vblank and user
events at vsartup for DCN")' introduces a new way of pageflip
completion handling for DCN, and some trouble.

The current implementation introduces a race condition, which
can cause pageflip completion events to be sent out one vblank
too early, thereby confusing userspace and causing flicker:

prepare_flip_isr():

1. Pageflip programming takes the ddev->event_lock.
2. Sets acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED
3. Releases ddev->event_lock.

--> Deadline for surface address regs double-buffering passes on
 target pipe.

4. dc_commit_updates_for_stream() MMIO programs the new pageflip
into hw, but too late for current vblank.

=> pflip_status == AMDGPU_FLIP_SUBMITTED, but flip won't complete
in current vblank due to missing the double-buffering deadline
by a tiny bit.

5. VSTARTUP trigger point in vblank is reached, VSTARTUP irq fires,
dm_dcn_crtc_high_irq() gets called.

6. Detects pflip_status == AMDGPU_FLIP_SUBMITTED and assumes the
pageflip has been completed/will complete in this vblank and
sends out pageflip completion event to userspace and resets
pflip_status = AMDGPU_FLIP_NONE.

=> Flip completion event sent out one vblank too early.

This behaviour has been observed during my testing with measurement
hardware a couple of time.

The commit message says that the extra flip event code was added to
dm_dcn_crtc_high_irq() to prevent missing to send out pageflip events
in case the pflip irq doesn't fire, because the "DCH HUBP" component
is clock gated and doesn't fire pflip irqs in that state. Also that
this clock gating may happen if no planes are active. According to
Nicholas, the clock gating can also happen if psr is active, and the
gating is controlled independently by the hardware, so difficult to
detect if and when the completion code in above commit is needed.

This patch tries the following solution: It only executes the extra pflip
completion code in dm_dcn_crtc_high_irq() iff the hardware reports
that there aren't any surface updated pending in the double-buffered
surface scanout address registers. Otherwise it leaves pflip completion
to the pflip irq handler, for a more race-free experience.

This would only guard against the order of events mentioned above.
If Step 5 (VSTARTUP trigger) happens before step 4 then this won't help
at all, because 1-3 + 5 might happen even without the hw being programmed
at all, ie. no surface update pending because none yet programmed into hw.

Therefore this patch also changes locking in amdgpu_dm_commit_planes(),
so that prepare_flip_isr() and dc_commit_updates_for_stream() are done
under event_lock protection within the same critical section.

v2: Take Nicholas comments into account, try a different solution.

Lightly tested on Polaris (locking) and Raven (the whole DCN stuff).
Seems to work without causing obvious new trouble.


Nick, any comments on this?  Can we get this committed or do you think
it needs additional rework?

Thanks,

Alex


Hi Alex, Mario,

This might be a little strange, but if we want to get this in as a fix 
for regressions caused by the original vblank and user events at 
vstartup patch then I'm actually going to give my reviewed by on the 
*v1* of this patch (but not this v2):


Reviewed-by: Nicholas Kazlauskas 

You can feel free to apply that one.

Reason 1: After having thought about it some more I don't think we 
enable anything today that has hubp powered down at the same time we 
expect to be waiting for a flip - eg. DMCU powering down HUBP during PSR 
entry. Static screen interrupt should happen after that flip finishes I 
think.


The CRTC can still be powered on with zero planes, and I don't think any 
userspace explicitly asks for vblank events in this case but it doesn't 
hurt to have the check.


Reason 2: This new patch will need much more thorough testing from side 
to fully understand the consequences of locking the entire DC commit 
sequence. For just a page flip that sounds fine, but for anything more 
than (eg. full updates, modesets, etc) I don't think we want to be 
disabling interrupts for potentially many milliseconds.


Maybe we could just use a lock-free queue here for the events. It's 
single producer/single consumer per CRTC.


Regards,
Nicholas Kazlauskas





Fixes: 16f17eda8bad ("drm/amd/display: Send vblank and user events at vsartup for 
DCN")
Signed-off-by: Mario Kleiner 
---
  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 80 ---
  1 file changed, 67 insertions(+), 13 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 d7df1a85e72f..aa4e941b276f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -287,6 +287,28 @@ static inline bool

Re: [RFC][PATCH 0/5] Introduce drm scaling filter property

2020-03-13 Thread Pekka Paalanen

On Thu, 12 Mar 2020 18:01:12 +0200
Ville Syrjälä  wrote:

> On Thu, Mar 12, 2020 at 03:37:03PM +, Laxminarayan Bharadiya, Pankaj 
> wrote:
> > 
> >   
> > > -Original Message-
> > > From: Ville Syrjälä 
> > > Sent: 12 March 2020 19:35
> > > To: Laxminarayan Bharadiya, Pankaj
> > > 
> > > Cc: jani.nik...@linux.intel.com; dan...@ffwll.ch; intel-
> > > g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
> > > airl...@linux.ie;
> > > maarten.lankho...@linux.intel.com; tzimmerm...@suse.de;
> > > mrip...@kernel.org; mihail.atanas...@arm.com; linux-
> > > ker...@vger.kernel.org; Nautiyal, Ankit K 
> > > Subject: Re: [RFC][PATCH 0/5] Introduce drm scaling filter property
> > > 
> > > On Tue, Feb 25, 2020 at 12:35:40PM +0530, Pankaj Bharadiya wrote:  
> > > > Integer scaling (IS) is a nearest-neighbor upscaling technique that
> > > > simply scales up the existing pixels by an integer (i.e., whole
> > > > number) multiplier. Nearest-neighbor (NN) interpolation works by
> > > > filling in the missing color values in the upscaled image with that of
> > > > the coordinate-mapped nearest source pixel value.
> > > >
> > > > Both IS and NN preserve the clarity of the original image. In
> > > > contrast, traditional upscaling algorithms, such as bilinear or
> > > > bicubic interpolation, result in blurry upscaled images because they
> > > > employ interpolation techniques that smooth out the transition from
> > > > one pixel to another.  Therefore, integer scaling is particularly
> > > > useful for pixel art games that rely on sharp, blocky images to
> > > > deliver their distinctive look.
> > > >
> > > > Many gaming communities have been asking for integer-mode scaling
> > > > support, some links and background:
> > > >
> > > > https://software.intel.com/en-us/articles/integer-scaling-support-on-i
> > > > ntel-graphics http://tanalin.com/en/articles/lossless-scaling/
> > > > https://community.amd.com/thread/209107
> > > > https://www.nvidia.com/en-us/geforce/forums/game-ready-drivers/13/1002
> > > > /feature-request-nonblurry-upscaling-at-integer-rat/
> > > >
> > > > This patch series -
> > > >   - Introduces new scaling filter property to allow userspace to
> > > > select  the driver's default scaling filter or Nearest-neighbor(NN)
> > > > filter for scaling operations on crtc/plane.
> > > >   - Implements and enable integer scaling for i915
> > > >
> > > > Userspace patch series link: TBD.  
> > > 
> > > That needs to be done or this will go nowhere.  
> > 
> > Yes, Sameer is working on enabling this feature in Kodi. 
> > Sameer, please share link here once you post patches.  
> 
> And who is doing it for other stuff? I think this would be most useful
> for games/emulators and such so IMO we should find a way to get it to
> the hands of users doing those things.
> 

Hi,

FWIW, being able to tell KMS to use nearest-neighbor filtering could be
useful for
https://gitlab.freedesktop.org/wayland/weston/-/merge_requests/394
as a follow-up.


Thanks,
pq


pgprRai7qCCC5.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] RFC: dma-buf: Add an API for importing and exporting sync files (v4)

2020-03-13 Thread Christian König


Am 12.03.20 um 16:57 schrieb Jason Ekstrand:

On Wed, Mar 11, 2020 at 8:18 AM Christian König
 wrote:

Am 11.03.20 um 04:43 schrieb Jason Ekstrand:

Explicit synchronization is the future.  At least, that seems to be what
most userspace APIs are agreeing on at this point.  However, most of our
Linux APIs (both userspace and kernel UAPI) are currently built around
implicit synchronization with dma-buf.  While work is ongoing to change
many of the userspace APIs and protocols to an explicit synchronization
model, switching over piecemeal is difficult due to the number of
potential components involved.  On the kernel side, many drivers use
dma-buf including GPU (3D/compute), display, v4l, and others.  In
userspace, we have X11, several Wayland compositors, 3D drivers, compute
drivers (OpenCL etc.), media encode/decode, and the list goes on.

This patch provides a path forward by allowing userspace to manually
manage the fences attached to a dma-buf.  Alternatively, one can think
of this as making dma-buf's implicit synchronization simply a carrier
for an explicit fence.  This is accomplished by adding two IOCTLs to
dma-buf for importing and exporting a sync file to/from the dma-buf.
This way a userspace component which is uses explicit synchronization,
such as a Vulkan driver, can manually set the write fence on a buffer
before handing it off to an implicitly synchronized component such as a
Wayland compositor or video encoder.  In this way, each of the different
components can be upgraded to an explicit synchronization model one at a
time as long as the userspace pieces connecting them are aware of it and
import/export fences at the right times.

There is a potential race condition with this API if userspace is not
careful.  A typical use case for implicit synchronization is to wait for
the dma-buf to be ready, use it, and then signal it for some other
component.  Because a sync_file cannot be created until it is guaranteed
to complete in finite time, userspace can only signal the dma-buf after
it has already submitted the work which uses it to the kernel and has
received a sync_file back.  There is no way to atomically submit a
wait-use-signal operation.  This is not, however, really a problem with
this API so much as it is a problem with explicit synchronization
itself.  The way this is typically handled is to have very explicit
ownership transfer points in the API or protocol which ensure that only
one component is using it at any given time.  Both X11 (via the PRESENT
extension) and Wayland provide such ownership transfer points via
explicit present and idle messages.

The decision was intentionally made in this patch to make the import and
export operations IOCTLs on the dma-buf itself rather than as a DRM
IOCTL.  This makes it the import/export operation universal across all
components which use dma-buf including GPU, display, v4l, and others.
It also means that a userspace component can do the import/export
without access to the DRM fd which may be tricky to get in cases where
the client communicates with DRM via a userspace API such as OpenGL or
Vulkan.  At a future date we may choose to add direct import/export APIs
to components such as drm_syncobj to avoid allocating a file descriptor
and going through two ioctls.  However, that seems to be something of a
micro-optimization as import/export operations are likely to happen at a
rate of a few per frame of rendered or decoded video.

v2 (Jason Ekstrand):
   - Use a wrapper dma_fence_array of all fences including the new one
 when importing an exclusive fence.

v3 (Jason Ekstrand):
   - Lock around setting shared fences as well as exclusive
   - Mark SIGNAL_SYNC_FILE as a read-write ioctl.
   - Initialize ret to 0 in dma_buf_wait_sync_file

v4 (Jason Ekstrand):
   - Use the new dma_resv_get_singleton helper

Signed-off-by: Jason Ekstrand 
---
   drivers/dma-buf/dma-buf.c| 96 
   include/uapi/linux/dma-buf.h | 13 -
   2 files changed, 107 insertions(+), 2 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index d4097856c86b..09973c689866 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -20,6 +20,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   #include 
@@ -348,6 +349,95 @@ static long dma_buf_set_name(struct dma_buf *dmabuf, const 
char __user *buf)
   return ret;
   }

+static long dma_buf_wait_sync_file(struct dma_buf *dmabuf,
+const void __user *user_data)
+{
+ struct dma_buf_sync_file arg;
+ struct dma_fence *fence;
+ int ret = 0;
+
+ if (copy_from_user(, user_data, sizeof(arg)))
+ return -EFAULT;
+
+ if (arg.flags != 0 && arg.flags != DMA_BUF_SYNC_FILE_SYNC_WRITE)
+ return -EINVAL;
+
+ fence = sync_file_get_fence(arg.fd);
+ if (!fence)
+ return -EINVAL;
+
+ dma_resv_lock(dmabuf->resv, NULL);
+
+ if

Re: [PATCH] drm/sched: add run job trace

2020-03-13 Thread Christian König


Am 13.03.20 um 10:56 schrieb Lucas Stach:

From: Robert Beckett 

Add a new trace event to show when jobs are run on the HW.

Signed-off-by: Robert Beckett 
Signed-off-by: Lucas Stach 


There is also the scheduled fence we could used for this, but this trace 
point adds a few extra fields which might be useful.


Acked-by: Christian König 


---
  .../gpu/drm/scheduler/gpu_scheduler_trace.h   | 27 +++
  drivers/gpu/drm/scheduler/sched_main.c|  1 +
  2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h 
b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
index d79086498aff..877ce9b127f1 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
@@ -59,6 +59,33 @@ TRACE_EVENT(drm_sched_job,
  __entry->job_count, __entry->hw_job_count)
  );
  
+TRACE_EVENT(drm_run_job,

+   TP_PROTO(struct drm_sched_job *sched_job, struct drm_sched_entity 
*entity),
+   TP_ARGS(sched_job, entity),
+   TP_STRUCT__entry(
+__field(struct drm_sched_entity *, entity)
+__field(struct dma_fence *, fence)
+__field(const char *, name)
+__field(uint64_t, id)
+__field(u32, job_count)
+__field(int, hw_job_count)
+),
+
+   TP_fast_assign(
+  __entry->entity = entity;
+  __entry->id = sched_job->id;
+  __entry->fence = _job->s_fence->finished;
+  __entry->name = sched_job->sched->name;
+  __entry->job_count = 
spsc_queue_count(>job_queue);
+  __entry->hw_job_count = atomic_read(
+  _job->sched->hw_rq_count);
+  ),
+   TP_printk("entity=%p, id=%llu, fence=%p, ring=%s, job count:%u, hw job 
count:%d",
+ __entry->entity, __entry->id,
+ __entry->fence, __entry->name,
+ __entry->job_count, __entry->hw_job_count)
+);
+
  TRACE_EVENT(drm_sched_process_job,
TP_PROTO(struct drm_sched_fence *fence),
TP_ARGS(fence),
diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 71ce6215956f..34231b7163cc 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -773,6 +773,7 @@ static int drm_sched_main(void *param)
atomic_inc(>hw_rq_count);
drm_sched_job_begin(sched_job);
  
+		trace_drm_run_job(sched_job, entity);

fence = sched->ops->run_job(sched_job);
drm_sched_fence_scheduled(s_fence);
  


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm/sched: add run job trace

2020-03-13 Thread Lucas Stach

From: Robert Beckett 

Add a new trace event to show when jobs are run on the HW.

Signed-off-by: Robert Beckett 
Signed-off-by: Lucas Stach 
---
 .../gpu/drm/scheduler/gpu_scheduler_trace.h   | 27 +++
 drivers/gpu/drm/scheduler/sched_main.c|  1 +
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h 
b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
index d79086498aff..877ce9b127f1 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
@@ -59,6 +59,33 @@ TRACE_EVENT(drm_sched_job,
  __entry->job_count, __entry->hw_job_count)
 );
 
+TRACE_EVENT(drm_run_job,
+   TP_PROTO(struct drm_sched_job *sched_job, struct drm_sched_entity 
*entity),
+   TP_ARGS(sched_job, entity),
+   TP_STRUCT__entry(
+__field(struct drm_sched_entity *, entity)
+__field(struct dma_fence *, fence)
+__field(const char *, name)
+__field(uint64_t, id)
+__field(u32, job_count)
+__field(int, hw_job_count)
+),
+
+   TP_fast_assign(
+  __entry->entity = entity;
+  __entry->id = sched_job->id;
+  __entry->fence = _job->s_fence->finished;
+  __entry->name = sched_job->sched->name;
+  __entry->job_count = 
spsc_queue_count(>job_queue);
+  __entry->hw_job_count = atomic_read(
+  _job->sched->hw_rq_count);
+  ),
+   TP_printk("entity=%p, id=%llu, fence=%p, ring=%s, job count:%u, hw 
job count:%d",
+ __entry->entity, __entry->id,
+ __entry->fence, __entry->name,
+ __entry->job_count, __entry->hw_job_count)
+);
+
 TRACE_EVENT(drm_sched_process_job,
TP_PROTO(struct drm_sched_fence *fence),
TP_ARGS(fence),
diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
b/drivers/gpu/drm/scheduler/sched_main.c
index 71ce6215956f..34231b7163cc 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -773,6 +773,7 @@ static int drm_sched_main(void *param)
atomic_inc(>hw_rq_count);
drm_sched_job_begin(sched_job);
 
+   trace_drm_run_job(sched_job, entity);
fence = sched->ops->run_job(sched_job);
drm_sched_fence_scheduled(s_fence);
 
-- 
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3] drm/bochs: downgrade pci_request_region failure from error to warning

Hi Gred.

On Fri, Mar 13, 2020 at 09:41:52AM +0100, Gerd Hoffmann wrote:
> Shutdown of firmware framebuffer has a bunch of problems.  Because
> of this the framebuffer region might still be reserved even after
> drm_fb_helper_remove_conflicting_pci_framebuffers() returned.
> 
> Don't consider pci_request_region() failure for the framebuffer
> region as fatal error to workaround this issue.
> 
> Reported-by: Marek Marczykowski-Górecki 
> Signed-off-by: Gerd Hoffmann 
> ---
>  drivers/gpu/drm/bochs/bochs_hw.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs_hw.c 
> b/drivers/gpu/drm/bochs/bochs_hw.c
> index 952199cc0462..dce4672e3fc8 100644
> --- a/drivers/gpu/drm/bochs/bochs_hw.c
> +++ b/drivers/gpu/drm/bochs/bochs_hw.c
> @@ -157,10 +157,8 @@ int bochs_hw_init(struct drm_device *dev)

There is a drm_device avilable.

>   size = min(size, mem);
>   }
>  
> - if (pci_request_region(pdev, 0, "bochs-drm") != 0) {
> - DRM_ERROR("Cannot request framebuffer\n");
> - return -EBUSY;
> - }
> + if (pci_request_region(pdev, 0, "bochs-drm") != 0)
> + DRM_WARN("Cannot request framebuffer, boot fb still active?\n");
So you could use drm_WARN() which is what is preferred these days.
With this fixed:
Acked-by: Sam Ravnborg 

Sam
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor based integer scaling support

2020-03-13 Thread Laxminarayan Bharadiya, Pankaj




> -Original Message-
> From: Ville Syrjälä 
> Sent: 12 March 2020 19:25
> To: Laxminarayan Bharadiya, Pankaj
> 
> Cc: jani.nik...@linux.intel.com; dan...@ffwll.ch; intel-
> g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; airl...@linux.ie;
> maarten.lankho...@linux.intel.com; tzimmerm...@suse.de;
> mrip...@kernel.org; mihail.atanas...@arm.com; Joonas Lahtinen
> ; Vivi, Rodrigo ;
> Chris Wilson ; Souza, Jose ;
> De Marchi, Lucas ; Roper, Matthew D
> ; Deak, Imre ; Shankar,
> Uma ; linux-ker...@vger.kernel.org; Nautiyal, Ankit K
> 
> Subject: Re: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor based
> integer scaling support
> 
> On Thu, Mar 12, 2020 at 09:13:24AM +, Laxminarayan Bharadiya, Pankaj
> wrote:
> >
> >
> > > -Original Message-
> > > From: Ville Syrjälä 
> > > Sent: 10 March 2020 21:47
> > > To: Laxminarayan Bharadiya, Pankaj
> > > 
> > > Cc: jani.nik...@linux.intel.com; dan...@ffwll.ch; intel-
> > > g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> > > airl...@linux.ie; maarten.lankho...@linux.intel.com;
> > > tzimmerm...@suse.de; mrip...@kernel.org; mihail.atanas...@arm.com;
> > > Joonas Lahtinen ; Vivi, Rodrigo
> > > ; Chris Wilson ;
> > > Souza, Jose ; De Marchi, Lucas
> > > ; Roper, Matthew D
> > > ; Deak, Imre ;
> > > Shankar, Uma ; linux- ker...@vger.kernel.org;
> > > Nautiyal, Ankit K 
> > > Subject: Re: [RFC][PATCH 5/5] drm/i915/display: Add Nearest-neighbor
> > > based integer scaling support
> > >
> > > On Tue, Feb 25, 2020 at 12:35:45PM +0530, Pankaj Bharadiya wrote:
> > > > Integer scaling (IS) is a nearest-neighbor upscaling technique
> > > > that simply scales up the existing pixels by an integer (i.e.,
> > > > whole
> > > > number) multiplier.Nearest-neighbor (NN) interpolation works by
> > > > filling in the missing color values in the upscaled image with
> > > > that of the coordinate-mapped nearest source pixel value.
> > > >
> > > > Both IS and NN preserve the clarity of the original image. Integer
> > > > scaling is particularly useful for pixel art games that rely on
> > > > sharp, blocky images to deliver their distinctive look.
> > > >
> > > > Program the scaler filter coefficients to enable the NN filter if
> > > > scaling filter property is set to
> > > > DRM_SCALING_FILTER_NEAREST_NEIGHBOR
> > > > and enable integer scaling.
> > > >
> > > > Bspec: 49247
> > > >
> > > > Signed-off-by: Pankaj Bharadiya
> > > > 
> > > > Signed-off-by: Ankit Nautiyal 
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_display.c | 83
> > > > +++-  drivers/gpu/drm/i915/display/intel_display.h
> > > > +++|
> > > > 2 +  drivers/gpu/drm/i915/display/intel_sprite.c  | 20 +++--
> > > >  3 files changed, 97 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > index b5903ef3c5a0..6d5f59203258 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > @@ -6237,6 +6237,73 @@ void skl_scaler_disable(const struct
> > > intel_crtc_state *old_crtc_state)
> > > > skl_detach_scaler(crtc, i);
> > > >  }
> > > >
> > > > +/**
> > > > + *  Theory behind setting nearest-neighbor integer scaling:
> > > > + *
> > > > + *  17 phase of 7 taps requires 119 coefficients in 60 dwords per set.
> > > > + *  The letter represents the filter tap (D is the center tap)
> > > > +and the number
> > > > + *  represents the coefficient set for a phase (0-16).
> > > > + *
> > > > + * 
> > > > ++++
> > > > + * |Index value | Data value coeffient 1 | Data value 
> > > > coeffient 2 |
> > > > + * 
> > > > ++++
> > > > + * |   00h  |  B0|  A0 
> > > >|
> > > > + * 
> > > > ++++
> > > > + * |   01h  |  D0|  C0 
> > > >|
> > > > + * 
> > > > ++++
> > > > + * |   02h  |  F0|  E0 
> > > >|
> > > > + * 
> > > > ++++
> > > > + * |   03h  |  A1|  G0 
> > > >|
> > > > + * 
> > > > ++++
> > > > + * |   04h  |  C1|  B1 
> > > >|
> > > > + * 
> > > > ++++
> > > > + * |   ...  |  ...   |  ...
> > > >|
> > > > + * 
> > > >

[PATCH v3] drm/bochs: downgrade pci_request_region failure from error to warning

2020-03-13 Thread Gerd Hoffmann

Shutdown of firmware framebuffer has a bunch of problems.  Because
of this the framebuffer region might still be reserved even after
drm_fb_helper_remove_conflicting_pci_framebuffers() returned.

Don't consider pci_request_region() failure for the framebuffer
region as fatal error to workaround this issue.

Reported-by: Marek Marczykowski-Górecki 
Signed-off-by: Gerd Hoffmann 
---
 drivers/gpu/drm/bochs/bochs_hw.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index 952199cc0462..dce4672e3fc8 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -157,10 +157,8 @@ int bochs_hw_init(struct drm_device *dev)
size = min(size, mem);
}
 
-   if (pci_request_region(pdev, 0, "bochs-drm") != 0) {
-   DRM_ERROR("Cannot request framebuffer\n");
-   return -EBUSY;
-   }
+   if (pci_request_region(pdev, 0, "bochs-drm") != 0)
+   DRM_WARN("Cannot request framebuffer, boot fb still active?\n");
 
bochs->fb_map = ioremap(addr, size);
if (bochs->fb_map == NULL) {
-- 
2.18.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v9 4/5] drm/i915: Add helper code for ACPI privacy screen

2020-03-13 Thread Rajat Jain

Add helper functions that can allow i915 to detect and control
an integrated privacy screen via ACPI methods. These shall be used
in the next patch.

Signed-off-by: Rajat Jain 
---
v9: same as v8
v8: Initial version. formed by refactoring the previous patch 4.
print the connector name in the debug messages.

 drivers/gpu/drm/i915/Makefile |   3 +-
 .../drm/i915/display/intel_privacy_screen.c   | 184 ++
 .../drm/i915/display/intel_privacy_screen.h   |  27 +++
 3 files changed, 213 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_privacy_screen.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 9f887a86e555d..da42389107f9c 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -209,7 +209,8 @@ i915-y += \
display/intel_vga.o
 i915-$(CONFIG_ACPI) += \
display/intel_acpi.o \
-   display/intel_opregion.o
+   display/intel_opregion.o \
+   display/intel_privacy_screen.o
 i915-$(CONFIG_DRM_FBDEV_EMULATION) += \
display/intel_fbdev.o
 
diff --git a/drivers/gpu/drm/i915/display/intel_privacy_screen.c 
b/drivers/gpu/drm/i915/display/intel_privacy_screen.c
new file mode 100644
index 0..66039103c821b
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_privacy_screen.c
@@ -0,0 +1,184 @@
+// SPDX-License-Identifier: GPL-2.0 OR MIT
+/*
+ * Intel ACPI privacy screen code
+ *
+ * Copyright © 2020 Google Inc.
+ *
+ * This code can help detect and control an integrated EPS (electronic
+ * privacy screen) via ACPI functions. It expects an ACPI node for the
+ * drm connector device with the following elements:
+ *
+ * UUID should be "c7033113-8720-4ceb-9090-9d52b3e52d73"
+ *
+ * _ADR = ACPI address per Spec (also see intel_acpi_device_id_update())
+ * https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf
+ * Pages 1119 - 1123.
+ *
+ * _DSM method that will perform the following functions according to
+ * Local1 argument passed to it:
+ *  - Local1 = 0 (EPS capabilities): Report EPS presence and capabilities.
+ *  - Local1 = 1 (EPS State)  :  _DSM returns 1 if EPS is enabled, 0 otherwise.
+ *  - Local1 = 2 (EPS Enable) :  _DSM enables EPS
+ *  - Local1 = 3 (EPS Disable):  _DSM disables EPS
+ *
+ * Here is a sample ACPI node:
+ *
+ *  Scope (\_SB.PCI0.GFX0) // Intel graphics device (PCI device)
+ *  {
+ *  Method (_DOD, 0, NotSerialized)  // _DOD: Display Output Devices
+ *  {
+ *  Return (Package (0x01)
+ *  {
+ *  0x80010400
+ *  })
+ *  }
+ *
+ *  Device (LCD)
+ *  {
+ *  Name (_ADR, 0x80010400)  // _ADR: Address
+ *  Name (_STA, 0x0F)  // _STA: Status
+ *
+ *  Method (EPSP, 0, NotSerialized) // EPS Present
+ *  {
+ *  Return (0x01)
+ *  }
+ *
+ *  Method (EPSS, 0, NotSerialized) // EPS State
+ *  {
+ *  Local0 = \_SB.PCI0.GRXS (0xCD)
+ *  Return (Local0)
+ *  }
+ *
+ *  Method (EPSE, 0, NotSerialized) // EPS Enable
+ *  {
+ *  \_SB.PCI0.STXS (0xCD)
+ *  }
+ *
+ *  Method (EPSD, 0, NotSerialized) // EPS Disable
+ *  {
+ *  \_SB.PCI0.CTXS (0xCD)
+ *  }
+ *
+ *  Method (_DSM, 4, Serialized)  // _DSM: Device-Specific Method
+ *  {
+ *  ToBuffer (Arg0, Local0)
+ *  If ((Local0 == ToUUID 
("c7033113-8720-4ceb-9090-9d52b3e52d73")))
+ *  {
+ *  ToInteger (Arg2, Local1)
+ *  If ((Local1 == Zero))
+ *  {
+ *  Local2 = EPSP ()
+ *  If ((Local2 == One))
+ *  {
+ *  Return (Buffer (One)
+ *  {
+ *   0x0F
+ *  })
+ *  }
+ *  }
+ *
+ *  If ((Local1 == One))
+ *  {
+ *  Return (EPSS ())
+ *  }
+ *
+ *  If ((Local1 == 0x02))
+ *  {
+ *  EPSE ()
+ *  }
+ *
+ *  If ((Local1 == 0x03))
+ *  {
+ *  EPSD ()
+ *  }
+ *
+ *  Return (Buffer (One)
+ *  {
+ *   0x00
+ *  })
+ *  }
+ *
+ *  Return (Buffer (One)
+ *  {
+ *   0x00
+ *  })
+ *  }
+ *  }
+ *  }
+ */
+
+#include 
+
+#include "intel_privacy_screen.h"
+
+#define CONN_NAME(conn)\
+   (conn->base.kdev ? dev_name(conn->base.kdev) : "NONAME")
+
+#define CONNECTOR_DSM_REVID 1
+
+#define CONNECTOR_DSM_FN_PRIVACY_ENABLE

Re: [PATCH] mm/hmm: Simplify hmm_vma_walk_pud slightly

On Thu, Mar 12, 2020 at 04:16:33PM +, Steven Price wrote:
> > Actually, while you are looking at this, do you think we should be
> > adding at least READ_ONCE in the pagewalk.c walk_* functions? The
> > multiple references of pmd, pud, etc without locking seems sketchy to
> > me.
> 
> I agree it seems worrying. I'm not entirely sure whether the holding of
> mmap_sem is sufficient,

I looked at this question, and at least for PMD, mmap_sem is not
sufficient. I didn't easilly figure it out for the other ones

I'm guessing if PMD is not safe then none of them are.

> this isn't something that I changed so I've just
> been hoping that it's sufficient since it seems to have been working
> (whether that's by chance because the compiler didn't generate multiple
> reads I've no idea). For walking the kernel's page tables the lack of
> READ_ONCE is also not great, but at least for PTDUMP we don't care too much
> about accuracy and it should be crash proof because there's no RCU grace
> period. And again the code I was replacing didn't have any special
> protection.
>
> I can't see any harm in updating the code to include READ_ONCE and I'm happy
> to review a patch.

The reason I ask is because hmm's walkers often have this pattern
where they get the pointer and then de-ref it (again) then
immediately have to recheck the 'again' conditions of the walker
itself because the re-read may have given a different value.

Having the walker deref the pointer and pass the value it into the ops
for use rather than repeatedly de-refing an unlocked value seems like
a much safer design to me.

If this also implicitly relies on a RCU grace period then it is also
missing RCU locking...

I also didn't quite understand why walk_pte_range() skipped locking
the pte in the no_vma case - I don't get why vma would be related to
locking here.

I also saw that hmm open coded the pte walk, presumably for
performance, so I was thinking of adding some kind of pte_range()
callback to avoid the expensive indirect function call per pte, but
hmm also can't have the pmd locked...

Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: Plumbing explicit synchronization through the Linux ecosystem

2020-03-13 Thread Alexander E. Patrakov

On Thu, Mar 12, 2020 at 6:36 PM Jason Ekstrand  wrote:
> From the perspective of a Wayland compositor (I used to play in this
> space), they'd love to implement the new explicit sync extension but
> can't.  Sure, they could wire up the extension, but the moment they go
> to flip a client buffer to the screen directly, they discover that KMS
> doesn't support any explicit sync APIs.  So, yes, they can technically
> implement the extension assuming the EGL stack they're running on has
> the sync_file extensions but any client buffers which come in using
> the explicit sync Wayland extension have to be composited and can't be
> scanned out directly.  As a 3D driver developer, I absolutely don't
> want compositors doing that because my users will complain about
> performance issues due to the extra blit.

Maybe this is something for the Marketing Department to solve? Sell
the extra processing that can be done during such extra blit as a
feature?

As a former user of a wide-gamut monitor that has no sRGB mode, and a
gamer, I would definitely accept the extra step (color conversion, not
"just a blit"!) between the application and the actual output. In
fact, I have set up compicc just for this purpose. Games with
poisonous oversaturated colors (because none of the game authors care
about wide-gamut monitors) are worse than the same games affected by
the very small performance penalty due to the conversion.

We just need a Marketing Person to come up with a huge list of other
cases where such compositing step is required for correctness, and
declare that direct scanout is something that makes no sense in the
present day, except possibly on embedded devices.

Of course the above trolling does not solve the problem related to
inability to be sure about the correct API usage.

-- 
Alexander E. Patrakov
CV: http://pc.cd/PLz7
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1 1/1] dt-bindings: display: fix panel warnings

2020-03-13 Thread Benjamin GAIGNARD




On 3/8/20 12:50 PM, Sam Ravnborg wrote:
> Fix following type af warnings in the panel bindings:
>
> Warning (unit_address_vs_reg): /example-0/dsi/panel: node has a reg or ranges 
> property, but no unit name
> Warning (unit_address_vs_reg): /example-0/dsi@ff45: node has a unit name, 
> but no reg property
>
> Removing the "@xxx" from the node name fixed first warning.
> Adding a missing reg property fixed the second warning
>
> Signed-off-by: Sam Ravnborg 
> Cc: Thierry Reding 
> Cc: Linus Walleij 
> Cc: Rob Herring 
> Cc: Heiko Stuebner 
> Cc: Maxime Ripard 
> Cc: Benjamin Gaignard 
> Cc: Laurent Pinchart 

I will add W=1 in my command line when check the yaml files to not 
reproduce this later.

Reviewed-by: Benjamin Gaignard 

Thanks,
Benjamin

> ---
>   .../devicetree/bindings/display/panel/elida,kd35t133.yaml | 2 +-
>   .../bindings/display/panel/leadtek,ltk500hd1829.yaml  | 2 +-
>   .../devicetree/bindings/display/panel/novatek,nt35510.yaml| 4 ++--
>   .../devicetree/bindings/display/panel/orisetech,otm8009a.yaml | 2 +-
>   .../devicetree/bindings/display/panel/panel-dpi.yaml  | 2 +-
>   .../devicetree/bindings/display/panel/panel-simple-dsi.yaml   | 2 +-
>   .../devicetree/bindings/display/panel/raydium,rm68200.yaml| 2 +-
>   .../devicetree/bindings/display/panel/xinpeng,xpp055c272.yaml | 2 +-
>   8 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/elida,kd35t133.yaml 
> b/Documentation/devicetree/bindings/display/panel/elida,kd35t133.yaml
> index 4bd74eaa61be..aa761f697b7a 100644
> --- a/Documentation/devicetree/bindings/display/panel/elida,kd35t133.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/elida,kd35t133.yaml
> @@ -34,7 +34,7 @@ additionalProperties: false
>   
>   examples:
> - |
> -dsi@ff45 {
> +dsi {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   panel@0 {
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/leadtek,ltk500hd1829.yaml 
> b/Documentation/devicetree/bindings/display/panel/leadtek,ltk500hd1829.yaml
> index 4ebcea7d0c63..2c9b8aa34815 100644
> --- 
> a/Documentation/devicetree/bindings/display/panel/leadtek,ltk500hd1829.yaml
> +++ 
> b/Documentation/devicetree/bindings/display/panel/leadtek,ltk500hd1829.yaml
> @@ -34,7 +34,7 @@ additionalProperties: false
>   
>   examples:
> - |
> -dsi@ff45 {
> +dsi {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   panel@0 {
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml 
> b/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml
> index 791fc9daa68b..73d2ff3baaff 100644
> --- a/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/novatek,nt35510.yaml
> @@ -40,10 +40,10 @@ examples:
> - |
>   #include 
>   
> -dsi@a0351000 {
> +dsi {
>   #address-cells = <1>;
>   #size-cells = <0>;
> -panel {
> +panel@0 {
>   compatible = "hydis,hva40wv1", "novatek,nt35510";
>   reg = <0>;
>   vdd-supply = <_ldo_aux4_reg>;
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/orisetech,otm8009a.yaml 
> b/Documentation/devicetree/bindings/display/panel/orisetech,otm8009a.yaml
> index 6e6ac995c27b..4b6dda6dbc0f 100644
> --- a/Documentation/devicetree/bindings/display/panel/orisetech,otm8009a.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/orisetech,otm8009a.yaml
> @@ -39,7 +39,7 @@ required:
>   
>   examples:
> - |
> -dsi@0 {
> +dsi {
> #address-cells = <1>;
> #size-cells = <0>;
> panel@0 {
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml 
> b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> index 5275d350f8cb..f63870384c00 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> @@ -48,7 +48,7 @@ additionalProperties: false
>   
>   examples:
> - |
> -panel@0 {
> +panel {
>   compatible = "osddisplays,osd057T0559-34ts", "panel-dpi";
>   label = "osddisplay";
>   power-supply = <_supply>;
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml 
> b/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml
> index 8b60368a2425..cefe19b6bf44 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-simple-dsi.yaml
> @@ -50,7 +50,7 @@ required:
>   
>   examples:
> - |
> -mdss_dsi@fd922800 {
> +mdss_dsi {
> #address-cells = <1>;
> #size-cells = <0>;
> panel@0 {
> diff --git 
> a/Documentation/devicetree/bindings/display/panel/raydium,rm68200.yaml

[PATCH v1] drm/tegra: hdmi: Silence deferred-probe error

2020-03-13 Thread Dmitry Osipenko

Driver fails to probe with -EPROBE_DEFER, which produces a bit noisy error
message in KMSG during kernel's boot up. This happens because voltage
regulators tend to be probed later than the DRM driver.

Signed-off-by: Dmitry Osipenko 
---
 drivers/gpu/drm/tegra/hdmi.c | 34 +-
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index 6f117628f257..e8ee8058c587 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -1648,6 +1648,7 @@ static irqreturn_t tegra_hdmi_irq(int irq, void *data)
 
 static int tegra_hdmi_probe(struct platform_device *pdev)
 {
+   const char *err_level = KERN_ERR;
struct tegra_hdmi *hdmi;
struct resource *regs;
int err;
@@ -1686,21 +1687,36 @@ static int tegra_hdmi_probe(struct platform_device 
*pdev)
}
 
hdmi->hdmi = devm_regulator_get(>dev, "hdmi");
-   if (IS_ERR(hdmi->hdmi)) {
-   dev_err(>dev, "failed to get HDMI regulator\n");
-   return PTR_ERR(hdmi->hdmi);
+   err = PTR_ERR_OR_ZERO(hdmi->hdmi);
+   if (err) {
+   if (err == -EPROBE_DEFER)
+   err_level = KERN_DEBUG;
+
+   dev_printk(err_level, >dev,
+  "failed to get HDMI regulator: %d\n", err);
+   return err;
}
 
hdmi->pll = devm_regulator_get(>dev, "pll");
-   if (IS_ERR(hdmi->pll)) {
-   dev_err(>dev, "failed to get PLL regulator\n");
-   return PTR_ERR(hdmi->pll);
+   err = PTR_ERR_OR_ZERO(hdmi->pll);
+   if (err) {
+   if (err == -EPROBE_DEFER)
+   err_level = KERN_DEBUG;
+
+   dev_printk(err_level, >dev,
+  "failed to get PLL regulator: %d\n", err);
+   return err;
}
 
hdmi->vdd = devm_regulator_get(>dev, "vdd");
-   if (IS_ERR(hdmi->vdd)) {
-   dev_err(>dev, "failed to get VDD regulator\n");
-   return PTR_ERR(hdmi->vdd);
+   err = PTR_ERR_OR_ZERO(hdmi->vdd);
+   if (err) {
+   if (err == -EPROBE_DEFER)
+   err_level = KERN_DEBUG;
+
+   dev_printk(err_level, >dev,
+  "failed to get VDD regulator: %d\n", err);
+   return err;
}
 
hdmi->output.dev = >dev;
-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] lib/scatterlist: add sg_set_dma_addr() function

On Thu, Mar 12, 2020 at 04:39:02PM +0100, Christian König wrote:
> > > The structure for holding dma addresses doesn't really exist
> > > in a generic form, but would be an array of these structures:
> > > 
> > > struct dma_sg {
> > >   dma_addr_t  addr;
> > >   u32 len;
> > > };
> > Same question, RDMA needs to represent gigabytes of pages in a DMA
> > list, we will need some generic way to handle that. I suspect GPU has
> > a similar need? Can it be accomidated in some generic dma_sg?
> 
> Yes, we easily have ranges of >1GB. So I would certainly say u64 for the len
> here.

To be clear, I mean specifically 1GB of dma map composed of 262k
pages, mapped into 262k dma_sg's that take around some 4M of memory to
represent as struct dma_dg.

Really prefer some scheme that doesn't rely on vmalloc..

Some approach to have a single dma_sg > 4G seems less commonly needed?
I don't think any RDMA HW today can handle a single SGL that large at
least.

> >   - Add some generic dma_sg data structure and helper
> >   - Add dma mapping code to go from pages to dma_sg
> >   - Rework RDMA to use dma_sg and the new dma mapping code
> >   - Rework dmabuf to support dma mapping to a dma_sg
> >   - Rework GPU drivers to use dma_sg
> >   - Teach p2pdma to generate a dma_sg from a BAR page list
> >   - This series
> > 
> > ?
> 
> Sounds pretty much like a plan to me, but unfortunately like a rather huge
> one.

I know parts of this have been advancing.. CH has been working on
fixing up the DMA layer enough to do #1 and #2, I think.

> Because of this and cause I don't know if all drivers can live with dma_sg
> I'm not sure if we shouldn't have the switch from scatterlist to dma_sg
> separately to this peer2peer work.

So far any attempts to make sgls without struct page have failed for
various reasons. Too often obscure stuff does actually want the struct
page.

Stuffing BAR memory pages into the SGL is bad enough already. :(

One pragmatic path might be to define this new 'dma_sg' in a way where
it would have the same memory layout as a 'struct scatterlist'

Something like

struct dma_scatterlist {
unsigned long   link;
unsigned intreserved1;
#ifndef CONFIG_NEED_SG_DMA_LENGTH
unsigned intdma_length;
#else
unsigned intreserved2;
#endif
dma_addr_t  dma_address;
#ifdef CONFIG_NEED_SG_DMA_LENGTH
unsigned intdma_length;
#endif
};

struct dma_sg_table {
 union {
 struct dma_scatterlist *dma_sgl;
 struct future_more_efficient_structure *future;
 }
 unsigned int nents;
};

Then a dma_map_sg could be 

struct dma_sg_table *dma_map_sg_attrs_to_dma(
   struct device *dev, struct scatterlist *sg,
   int nents, enum dma_data_direction dir,
   unsigned long attrs)
{
   ret = dma_map_sg_attrs(dev, sg, nents, dir, attrs);
   res = kmalloc(sizeof(dma_sg_table));
   res->dma_sgl = sg;
   return res;
}

Then at least the work can get gets split up, I can switch RDMA
drivers to use dma_sg_table, then I can switch the subsystem to call
dma_map_sg_attrs_to_dma, then when we get dma_map_biovec_attrs() I can
work on switching the input sgl to a biovec without changing the
drivers.

After enough conversions are done we can optimize the memory layout
inside dma_sg_table, after everything is done we can drop support for
'dma_scatterlist'

It doesn't feel objectionable to build a 'dma_sg_table' without a
struct page.

Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] mm/hmm: Simplify hmm_vma_walk_pud slightly

On Thu, Mar 12, 2020 at 02:40:08PM +, Steven Price wrote:
> On 12/03/2020 14:27, Jason Gunthorpe wrote:
> > On Thu, Mar 12, 2020 at 10:28:13AM +, Steven Price wrote:
> > > By refactoring to deal with the !pud_huge(pud) || !pud_devmap(pud)
> > > condition early it's possible to remove the 'ret' variable and remove a
> > > level of indentation from half the function making the code easier to
> > > read.
> > > 
> > > No functional change.
> > > 
> > > Signed-off-by: Steven Price 
> > > Thanks to Jason's changes there were only two code paths left using
> > > the out_unlock label so it seemed like a good opportunity to
> > > refactor.
> > 
> > Yes, I made something very similar, what do you think of this:
> > 
> > https://github.com/jgunthorpe/linux/commit/93f0ed42ab3f9ceb27b58fb7c7c3ecaf60f16b36
> 
> Even better! Sorry I didn't realise you'd already done this. I just saw that
> the function was needlessly complicated after your fix, so I thought I'd do
> a drive-by cleanup since part of the mess was my fault! :)

No worries, I've got a lot of patches for hmm_range_fault right now,
just trying to organize them, test them and post them. Haven't posted
that one yet.

Actually, while you are looking at this, do you think we should be
adding at least READ_ONCE in the pagewalk.c walk_* functions? The
multiple references of pmd, pud, etc without locking seems sketchy to
me.

Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND PATCH v2 1/9] iomap: Constify ioreadX() iomem argument (as in generic implementation)

2020-03-13 Thread Michael Ellerman

Krzysztof Kozlowski  writes:
> diff --git a/arch/powerpc/kernel/iomap.c b/arch/powerpc/kernel/iomap.c
> index 5ac84efc6ede..9fe4fb3b08aa 100644
> --- a/arch/powerpc/kernel/iomap.c
> +++ b/arch/powerpc/kernel/iomap.c
> @@ -15,23 +15,23 @@
>   * Here comes the ppc64 implementation of the IOMAP 
>   * interfaces.
>   */
> -unsigned int ioread8(void __iomem *addr)
> +unsigned int ioread8(const void __iomem *addr)
>  {
>   return readb(addr);
>  }
> -unsigned int ioread16(void __iomem *addr)
> +unsigned int ioread16(const void __iomem *addr)
>  {
>   return readw(addr);
>  }
> -unsigned int ioread16be(void __iomem *addr)
> +unsigned int ioread16be(const void __iomem *addr)
>  {
>   return readw_be(addr);
>  }
> -unsigned int ioread32(void __iomem *addr)
> +unsigned int ioread32(const void __iomem *addr)
>  {
>   return readl(addr);
>  }
> -unsigned int ioread32be(void __iomem *addr)
> +unsigned int ioread32be(const void __iomem *addr)
>  {
>   return readl_be(addr);
>  }
> @@ -41,27 +41,27 @@ EXPORT_SYMBOL(ioread16be);
>  EXPORT_SYMBOL(ioread32);
>  EXPORT_SYMBOL(ioread32be);
>  #ifdef __powerpc64__
> -u64 ioread64(void __iomem *addr)
> +u64 ioread64(const void __iomem *addr)
>  {
>   return readq(addr);
>  }
> -u64 ioread64_lo_hi(void __iomem *addr)
> +u64 ioread64_lo_hi(const void __iomem *addr)
>  {
>   return readq(addr);
>  }
> -u64 ioread64_hi_lo(void __iomem *addr)
> +u64 ioread64_hi_lo(const void __iomem *addr)
>  {
>   return readq(addr);
>  }
> -u64 ioread64be(void __iomem *addr)
> +u64 ioread64be(const void __iomem *addr)
>  {
>   return readq_be(addr);
>  }
> -u64 ioread64be_lo_hi(void __iomem *addr)
> +u64 ioread64be_lo_hi(const void __iomem *addr)
>  {
>   return readq_be(addr);
>  }
> -u64 ioread64be_hi_lo(void __iomem *addr)
> +u64 ioread64be_hi_lo(const void __iomem *addr)
>  {
>   return readq_be(addr);
>  }
> @@ -139,15 +139,15 @@ EXPORT_SYMBOL(iowrite64be_hi_lo);
>   * FIXME! We could make these do EEH handling if we really
>   * wanted. Not clear if we do.
>   */
> -void ioread8_rep(void __iomem *addr, void *dst, unsigned long count)
> +void ioread8_rep(const void __iomem *addr, void *dst, unsigned long count)
>  {
>   readsb(addr, dst, count);
>  }
> -void ioread16_rep(void __iomem *addr, void *dst, unsigned long count)
> +void ioread16_rep(const void __iomem *addr, void *dst, unsigned long count)
>  {
>   readsw(addr, dst, count);
>  }
> -void ioread32_rep(void __iomem *addr, void *dst, unsigned long count)
> +void ioread32_rep(const void __iomem *addr, void *dst, unsigned long count)
>  {
>   readsl(addr, dst, count);
>  }

This looks OK to me.

Acked-by: Michael Ellerman  (powerpc)

cheers
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] mm/hmm: Simplify hmm_vma_walk_pud slightly

On Thu, Mar 12, 2020 at 10:28:13AM +, Steven Price wrote:
> By refactoring to deal with the !pud_huge(pud) || !pud_devmap(pud)
> condition early it's possible to remove the 'ret' variable and remove a
> level of indentation from half the function making the code easier to
> read.
> 
> No functional change.
> 
> Signed-off-by: Steven Price 
> ---
> Thanks to Jason's changes there were only two code paths left using
> the out_unlock label so it seemed like a good opportunity to
> refactor.

Yes, I made something very similar, what do you think of this:

https://github.com/jgunthorpe/linux/commit/93f0ed42ab3f9ceb27b58fb7c7c3ecaf60f16b36

>From 93f0ed42ab3f9ceb27b58fb7c7c3ecaf60f16b36 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe 
Date: Wed, 4 Mar 2020 17:11:10 -0400
Subject: [PATCH] mm/hmm: rework hmm_vma_walk_pud()

At least since commit 3afc423632a1 ("mm: pagewalk: add p4d_entry() and
pgd_entry()") this code has developed a number of strange control flows.

The purpose of the routine is to copy the pfns of a huge devmap PUD into
the pfns output array, without splitting the PUD. Everything that is not a
huge devmap PUD should go back to the walker for splitting.

Rework the logic to show this goal and remove redundant stuff:

- If pud_trans_huge_lock returns !NULL then this is already
  'pud_trans_huge() || pud_devmap()' and 'pud_huge() || pud_devmap()'
  so some of the logic is redundant.

- Hitting pud_none() is a race, treat it as such and return back to the
  walker using ACTION_AGAIN

- !pud_present() gives 0 cpu_flags, so the extra checks are redundant

- Once the *pudp is read there is no need to continue holding the pud
  lock, so drop it. The only thing the following code cares about is the
  pfn from the devmap, and if there is racing then the notifiers will
  resolve everything. Perhaps the unlocked READ_ONCE in an ealier version
  was correct

Signed-off-by: Jason Gunthorpe 
---
 mm/hmm.c | 79 +++-
 1 file changed, 33 insertions(+), 46 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 8fec801a33c9e2..87a376659b5ad4 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -455,66 +455,53 @@ static int hmm_vma_walk_pud(pud_t *pudp, unsigned long 
start, unsigned long end,
 {
struct hmm_vma_walk *hmm_vma_walk = walk->private;
struct hmm_range *range = hmm_vma_walk->range;
-   unsigned long addr = start;
+   unsigned long i, npages, pfn;
+   unsigned int required_fault;
+   uint64_t cpu_flags;
+   uint64_t *pfns;
+   spinlock_t *ptl;
pud_t pud;
-   int ret = 0;
-   spinlock_t *ptl = pud_trans_huge_lock(pudp, walk->vma);
 
+   /*
+* This only handles huge devmap pages, the default return is
+* ACTION_SUBTREE, so everything else is split by the walker and passed
+* to the other routines.
+*/
+   ptl = pud_trans_huge_lock(pudp, walk->vma);
if (!ptl)
return 0;
+   pud = *pudp;
+   spin_unlock(ptl);
 
-   /* Normally we don't want to split the huge page */
-   walk->action = ACTION_CONTINUE;
-
-   pud = READ_ONCE(*pudp);
if (pud_none(pud)) {
-   spin_unlock(ptl);
-   return hmm_vma_walk_hole(start, end, -1, walk);
+   walk->action = ACTION_AGAIN;
+   return 0;
}
 
-   if (pud_huge(pud) && pud_devmap(pud)) {
-   unsigned long i, npages, pfn;
-   unsigned int required_flags;
-   uint64_t *pfns, cpu_flags;
-
-   if (!pud_present(pud)) {
-   spin_unlock(ptl);
-   return hmm_vma_walk_hole(start, end, -1, walk);
-   }
-
-   i = (addr - range->start) >> PAGE_SHIFT;
-   npages = (end - addr) >> PAGE_SHIFT;
-   pfns = >pfns[i];
+   if (!pud_devmap(pud))
+   return 0;
 
-   cpu_flags = pud_to_hmm_pfn_flags(range, pud);
+   pfns = >pfns[(start - range->start) >> PAGE_SHIFT];
+   cpu_flags = pud_to_hmm_pfn_flags(range, pud);
+   required_fault =
hmm_range_need_fault(hmm_vma_walk, pfns, npages, cpu_flags);
-   if (required_flags) {
-   spin_unlock(ptl);
-   return hmm_vma_walk_hole_(addr, end, required_flags,
- walk);
-   }
+   if (required_fault)
+   return hmm_vma_walk_hole_(start, end, required_fault, walk);
 
-   pfn = pud_pfn(pud) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
-   for (i = 0; i < npages; ++i, ++pfn) {
-   hmm_vma_walk->pgmap = get_dev_pagemap(pfn,
- hmm_vma_walk->pgmap);
-   if (unlikely(!hmm_vma_walk->pgmap)) {
-   ret = -EBUSY;
-   goto out_unlock;
-   }
-

Re: [PATCH hmm 7/8] mm/hmm: return -EFAULT when setting HMM_PFN_ERROR on requested valid pages

On Wed, Mar 11, 2020 at 06:36:47PM -0700, Ralph Campbell wrote:
> > @@ -390,8 +384,15 @@ static int hmm_vma_walk_pmd(pmd_t *pmdp,
> > return -EBUSY;
> > }
> > return hmm_pfns_fill(start, end, range, HMM_PFN_NONE);
> > -   } else if (!pmd_present(pmd))
> > +   }
> > +
> > +   if (!pmd_present(pmd)) {
> > +   hmm_range_need_fault(hmm_vma_walk, pfns, npages, 0, ,
> > +_fault);
> > +   if (fault || write_fault)
> > +   return -EFAULT;
> > return hmm_pfns_fill(start, end, range, HMM_PFN_ERROR);
> 
> Shouldn't this fill with HMM_PFN_NONE instead of HMM_PFN_ERROR?
> Otherwise, when a THP is swapped out, you will get a different
> value than if a PTE is swapped out and you are prefetching/snapshotting.

If this is the case then the problem is that the return -EFAULT path
needs to do something else.. ie since the above code can't trigger
swap in, it is correct to return PFN_ERROR.

I'm completely guessing, but do we need to call pmd_to_swp_entry() and
handle things similarly to the pte? What swp_entries are valid for a
pmd?

Do you understand this better, or know how to trigger a !pmd_present
for test?

I suppose another option would be this:

if (!pmd_present(pmd)) {
hmm_range_need_fault(hmm_vma_walk, pfns, npages, 0, ,
 _fault);
/* We can't handle this. Cause the PMD to be split and
 * handle it in the pte handler. */
if (fault || write_fault)
return 0;
return hmm_pfns_fill(start, end, range, HMM_PFN_NONE);
}

Which, I think, must be correct, but inefficient?

Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hmm 9/8] mm/hmm: do not check pmd_protnone twice in hmm_vma_handle_pmd()

pmd_to_hmm_pfn_flags() already checks it and makes the cpu flags 0. If no
fault is requested then the pfns should be returned with the not valid
flags.

It should not unconditionally fault if faulting is not requested.

Fixes: 2aee09d8c116 ("mm/hmm: change hmm_vma_fault() to allow write fault on 
page basis")
Signed-off-by: Jason Gunthorpe 
---
 mm/hmm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Bonus patch, this one got found after I made the series..

diff --git a/mm/hmm.c b/mm/hmm.c
index ca33d086bdc190..6d9da4b0f0a9f8 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -226,7 +226,7 @@ static int hmm_vma_handle_pmd(struct mm_walk *walk, 
unsigned long addr,
hmm_range_need_fault(hmm_vma_walk, pfns, npages, cpu_flags,
 , _fault);
 
-   if (pmd_protnone(pmd) || fault || write_fault)
+   if (fault || write_fault)
return hmm_vma_walk_hole_(addr, end, fault, write_fault, walk);
 
pfn = pmd_pfn(pmd) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
-- 
2.25.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] mm/hmm: Simplify hmm_vma_walk_pud slightly

On Thu, Mar 12, 2020 at 05:02:18PM +, Steven Price wrote:

> > Having the walker deref the pointer and pass the value it into the ops
> > for use rather than repeatedly de-refing an unlocked value seems like
> > a much safer design to me.
> 
> Yeah that sounds like a good idea.

Ok.. let see when I get this hmm & odp stuff cleared off
 
> > I also didn't quite understand why walk_pte_range() skipped locking
> > the pte in the no_vma case - I don't get why vma would be related to
> > locking here.
> 
> The no_vma case is for walking the kernel's page tables and they may have
> entries that are not backed by struct page, so there isn't (reliably) a PTE
> lock to take.

Oh, that is an interesting bit of insight..

> > I also saw that hmm open coded the pte walk, presumably for
> > performance, so I was thinking of adding some kind of pte_range()
> > callback to avoid the expensive indirect function call per pte, but
> > hmm also can't have the pmd locked...
> 
> Yeah the callback per PTE is a bit heavy because of the indirect function
> call. I'm not sure how to optimise it beyond open coding at the PMD level.
> One option would be to provide helper functions to make it a bit more
> generic.
> 
> Do you have an idea of what pte_range() would look like?

Basically just pass in the already mapped pte array just like is
already done at the tail of the pmd

The reason to do it like this is so that the common code in the walker
can correctly prove the pmd is pointing at a pte before trying to map
it.

This is complicated, and hmm at least already got it wrong when trying
to open code at the PMD level.

Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] lib/scatterlist: add sg_set_dma_addr() function

On Thu, Mar 12, 2020 at 03:47:29AM -0700, Christoph Hellwig wrote:
> On Thu, Mar 12, 2020 at 11:31:35AM +0100, Christian König wrote:
> > But how should we then deal with all the existing interfaces which already
> > take a scatterlist/sg_table ?
> >
> > The whole DMA-buf design and a lot of drivers are build around
> > scatterlist/sg_table and to me that actually makes quite a lot of sense.
> > 
> 
> Replace them with a saner interface that doesn't take a scatterlist.
> At very least for new functionality like peer to peer DMA, but
> especially this code would also benefit from a general move away
> from the scatterlist.

If dma buf can do P2P I'd like to see support for consuming a dmabuf
in RDMA. Looking at how.. there is an existing sgl based path starting
from get_user_pages through dma map to the drivers. (ib_umem)

I can replace the driver part with something else (dma_sg), but not
until we get a way to DMA map pages directly into that something
else..

The non-page scatterlist is also a big concern for RDMA as we have
drivers that want the page list, so even if we did as this series
contemplates I'd have still have to split the drivers and create the
notion of a dma-only SGL.

> > I mean we could come up with a new structure for this, but to me that just
> > looks like reinventing the wheel. Especially since drivers need to be able
> > to handle both I/O to system memory and I/O to PCIe BARs.
> 
> The structure for holding the struct page side of the scatterlist is
> called struct bio_vec, so far mostly used by the block and networking
> code.

I haven't used bio_vecs before, do they support chaining like SGL so
they can be very big? RDMA dma maps gigabytes of memory

> The structure for holding dma addresses doesn't really exist
> in a generic form, but would be an array of these structures:
> 
> struct dma_sg {
>   dma_addr_t  addr;
>   u32 len;
> };

Same question, RDMA needs to represent gigabytes of pages in a DMA
list, we will need some generic way to handle that. I suspect GPU has
a similar need? Can it be accomidated in some generic dma_sg?

So I'm guessing the path forward is something like

 - Add some generic dma_sg data structure and helper
 - Add dma mapping code to go from pages to dma_sg
 - Rework RDMA to use dma_sg and the new dma mapping code
 - Rework dmabuf to support dma mapping to a dma_sg
 - Rework GPU drivers to use dma_sg
 - Teach p2pdma to generate a dma_sg from a BAR page list
 - This series

?

Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] lib/scatterlist: add sg_set_dma_addr() function

2020-03-13 Thread Logan Gunthorpe



On 2020-03-12 8:19 a.m., Jason Gunthorpe wrote:
> On Thu, Mar 12, 2020 at 03:47:29AM -0700, Christoph Hellwig wrote:
>> On Thu, Mar 12, 2020 at 11:31:35AM +0100, Christian König wrote:
>>> But how should we then deal with all the existing interfaces which already
>>> take a scatterlist/sg_table ?
>>>
>>> The whole DMA-buf design and a lot of drivers are build around
>>> scatterlist/sg_table and to me that actually makes quite a lot of sense.
>>>
>>
>> Replace them with a saner interface that doesn't take a scatterlist.
>> At very least for new functionality like peer to peer DMA, but
>> especially this code would also benefit from a general move away
>> from the scatterlist.
> 
> If dma buf can do P2P I'd like to see support for consuming a dmabuf
> in RDMA. Looking at how.. there is an existing sgl based path starting
> from get_user_pages through dma map to the drivers. (ib_umem)
> 
> I can replace the driver part with something else (dma_sg), but not
> until we get a way to DMA map pages directly into that something
> else..
> 
> The non-page scatterlist is also a big concern for RDMA as we have
> drivers that want the page list, so even if we did as this series
> contemplates I'd have still have to split the drivers and create the
> notion of a dma-only SGL.
> 
>>> I mean we could come up with a new structure for this, but to me that just
>>> looks like reinventing the wheel. Especially since drivers need to be able
>>> to handle both I/O to system memory and I/O to PCIe BARs.
>>
>> The structure for holding the struct page side of the scatterlist is
>> called struct bio_vec, so far mostly used by the block and networking
>> code.
> 
> I haven't used bio_vecs before, do they support chaining like SGL so
> they can be very big? RDMA dma maps gigabytes of memory

bio_vec's themselves don't support chaining... In the block layer they
are used in a struct bio which handles chaining, splitting and other
features. Each bio, though, has a limit of 256 segments to avoid higher
order allocations. Depending on your use case, you could reuse bios or
write your own container to chain bio_vecs.

>> The structure for holding dma addresses doesn't really exist
>> in a generic form, but would be an array of these structures:
>>
>> struct dma_sg {
>>  dma_addr_t  addr;
>>  u32 len;
>> };


> Yes, we easily have ranges of >1GB. So I would certainly say u64 for the len 
> here.

I'd probably avoid the u64 here and leave space for some flags or
something. If you have >1GB to map you can always just have mulitple
segments. With 4GB per segment and 256 segments per page, a page of DMA
sgs can easily map 1TB of memory in a single call and with chaining or
larger allocations you can extend that further, if needed.

Logan

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH hmm 1/8] mm/hmm: add missing unmaps of the ptep during hmm_vma_handle_pte()