Re: [RFC PATCH hwc] drm_hwcomposer: set CRTC background color when available
Hey Maarten, On Fri, Jun 29, 2018 at 12:05 PM, Maarten Lankhorst wrote: > Op 22-02-18 om 04:54 schreef Stefan Schake: >> Android assumes an implicit black background layer is always present >> behind all layers it specifies for composition. drm_hwcomposer currently >> punts responsibility for this to the kernel/DRM platform and puts layers >> with per-pixel alpha content on the primary plane when requested. >> >> On some platforms (e.g. VC4) a background color fill has a cycle cost for >> the hardware composer and is not enabled by default. Instead, userland can >> request a background color through a CRTC property. Use this property to >> specify the implicit black background Android expects. >> >> Signed-off-by: Stefan Schake >> --- >> Kernel changes for this (background_color) are available here: >> >> https://github.com/stschake/linux/commits/background-upstream >> >> Sending as RFC because I'm not entirely clear on whose responsibility >> this should be, on most DRM drivers it seems to be implicit. I think >> a case could also be made that VC4 should not accept alpha formats on >> the lowest layer or enable background color fill when given one anyway. >> On the other hand, userland control over background color seems desirable >> regardless and is a feature of multiple hardware composers (i915, vc4, omap). > Ping? Would be nice if we were moving forward. :) I was unclear if DRM specified a black background when writing this. Someone pointed me towards the excerpt in the docs that explicitly mandates black background fill and so I ended up writing a VC4 patch that automatically sets it when required instead of the optional property used here. Adding a background_color property would still be desirable, but I'm unclear on what the userspace would be at the moment. drm_hwc doesn't need any background color other than black and since that is the DRM default, it wouldn't need to use a property. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH hwc] drm_hwcomposer: Support assigning planes in ValidateDisplay
Hey Rob,
On Fri, May 18, 2018 at 12:08 AM, Rob Herring wrote:
> In order to assign planes to layers in ValidateDisplay, testing
> compositing with a DRM atomic modeset test is needed as PresentDisplay
> is too late. This means most of PresentDisplay needs to be run from
> ValidateDisplay, so refactor PresentDisplay and plumb a test flag down
> the stack.
>
> Signed-off-by: Rob Herring
> ---
> Based on my GL comp removal work. Adding atomic test path turned out to
> be easier than using the Planner directly.
>
> drmdisplaycompositor.cpp | 4 +-
> drmdisplaycompositor.h | 2 +-
> drmhwctwo.cpp| 79 ++--
> drmhwctwo.h | 1 +
> 4 files changed, 73 insertions(+), 13 deletions(-)
>
> diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
> index 2f9f6c6772da..ceee6b1a4bd6 100644
> --- a/drmdisplaycompositor.cpp
> +++ b/drmdisplaycompositor.cpp
> @@ -469,7 +469,7 @@ void DrmDisplayCompositor::ApplyFrame(
> }
>
> int DrmDisplayCompositor::ApplyComposition(
> -std::unique_ptr composition) {
> +std::unique_ptr composition, bool test) {
Since ApplyComposition just calls CommitFrame which already has test
support, can we reuse that for a TestComposition function?
>int ret = 0;
>switch (composition->type()) {
> case DRM_COMPOSITION_TYPE_FRAME:
> @@ -481,6 +481,8 @@ int DrmDisplayCompositor::ApplyComposition(
>ALOGE("Commit test failed for display %d, FIXME", display_);
>return ret;
> }
> +if (test)
> + return 0;
Then we could also drop this hunk.
>}
>
>ApplyFrame(std::move(composition), ret);
> diff --git a/drmdisplaycompositor.h b/drmdisplaycompositor.h
> index ed46873c3409..7c5ebb005dc4 100644
> --- a/drmdisplaycompositor.h
> +++ b/drmdisplaycompositor.h
> @@ -43,7 +43,7 @@ class DrmDisplayCompositor {
>int Init(DrmResources *drm, int display);
>
>std::unique_ptr CreateComposition() const;
> - int ApplyComposition(std::unique_ptr composition);
> + int ApplyComposition(std::unique_ptr composition,
> bool test);
And this one.
>int Composite();
>void Dump(std::ostringstream *out) const;
>
> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
> index deaf14363e8c..19f73e721154 100644
> --- a/drmhwctwo.cpp
> +++ b/drmhwctwo.cpp
> @@ -480,8 +480,7 @@ void DrmHwcTwo::HwcDisplay::AddFenceToRetireFence(int fd)
> {
>}
> }
>
> -HWC2::Error DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
> - supported(__func__);
> +HWC2::Error DrmHwcTwo::HwcDisplay::CreateComposition(bool test) {
This is called CreateComposition, but it also tests and even commits.
If this only sets up the DrmDisplayComposition, we could just have the
test and commit in Validate/Present respectively and drop the flag.
Unfortunately it also uses and mutates the layer state, and moving that
into Present/Validate from here looks more difficult.
>std::vector layers_map;
>layers_map.emplace_back();
>DrmCompositionDisplayLayersMap &map = layers_map.back();
> @@ -494,7 +493,13 @@ HWC2::Error
> DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>uint32_t client_z_order = 0;
>std::map z_map;
>for (std::pair &l : layers_) {
> -switch (l.second.validated_type()) {
> +HWC2::Composition comp_type;
> +if (test)
> + comp_type = l.second.sf_type();
> +else
> + comp_type = l.second.validated_type();
> +
> +switch (comp_type) {
>case HWC2::Composition::Device:
> z_map.emplace(std::make_pair(l.second.z_order(), &l.second));
> break;
> @@ -510,6 +515,10 @@ HWC2::Error
> DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
>if (use_client_layer)
> z_map.emplace(std::make_pair(client_z_order, &client_layer_));
>
> + if (z_map.empty())
> +return HWC2::Error::BadLayer;
> +
> +
Extra newline.
>// now that they're ordered by z, add them to the composition
>for (std::pair &l : z_map) {
> DrmHwcLayer layer;
> @@ -521,10 +530,6 @@ HWC2::Error
> DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
> }
> map.layers.emplace_back(std::move(layer));
>}
> - if (map.layers.empty()) {
> -*retire_fence = -1;
> -return HWC2::Error::None;
> - }
>
>std::unique_ptr composition =
>compositor_.CreateComposition();
> @@ -555,13 +560,29 @@ HWC2::Error
> DrmHwcTwo::HwcDisplay::PresentDisplay(int32_t *retire_fence) {
> i = overlay_planes.erase(i);
>}
>
> - AddFenceToRetireFence(composition->take_out_fence());
> + if (!test)
> +AddFenceToRetireFence(composition->take_out_fence());
>
> - ret = compositor_.ApplyComposition(std::move(composition));
> + ret = compositor_.ApplyComposition(std::move(composition), test);
>if (ret) {
> ALOGE("Failed to apply the frame composition ret=%d", ret);
> return HWC2::Error::BadParameter;
>}
> + return HWC2::Error::None;
> +}
> +
> +HWC2::Error DrmHwcTwo::Hwc
Re: [PATCH] drm/vc4: plane: Expand the lower bits using the LSB
On Tue, Apr 24, 2018 at 9:48 AM, Maxime Ripard
wrote:
> The vc4 HVS uses an internal RGB888 representation of the frames, and will
> by default expand formats using a lower depth using zeros.
>
> This causes an issue when we try to use other compositing software such as
> pixman that seems to be filling the missing bits using the format least
> significant bit value. As such, this prevents us from checking the display
> output in a reliable way.
>
> To prevent this, force the same behaviour so that we can do such things.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/vc4/vc4_plane.c | 1 +
> drivers/gpu/drm/vc4/vc4_regs.h | 8
> 2 files changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
> index ce39390be389..8dd33c6e9fd8 100644
> --- a/drivers/gpu/drm/vc4/vc4_plane.c
> +++ b/drivers/gpu/drm/vc4/vc4_plane.c
> @@ -542,6 +542,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
> /* Control word */
> vc4_dlist_write(vc4_state,
> SCALER_CTL0_VALID |
> + VC4_SET_FIELD(SCALER_CTL0_EXPAND_LSB,
> SCALER_CTL0_EXPAND) |
> (format->pixel_order << SCALER_CTL0_ORDER_SHIFT) |
> (format->hvs << SCALER_CTL0_PIXEL_FORMAT_SHIFT) |
> VC4_SET_FIELD(tiling, SCALER_CTL0_TILING) |
> diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> index a141496104a6..7c28e6207ec2 100644
> --- a/drivers/gpu/drm/vc4/vc4_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> @@ -806,12 +806,20 @@ enum hvs_pixel_format {
> #define SCALER_CTL0_ORDER_MASK VC4_MASK(14, 13)
> #define SCALER_CTL0_ORDER_SHIFT13
>
> +#define SCALER_CTL0_EXPAND_MASKVC4_MASK(12, 11)
> +#define SCALER_CTL0_EXPAND_SHIFT 11
> +
> #define SCALER_CTL0_SCL1_MASK VC4_MASK(10, 8)
> #define SCALER_CTL0_SCL1_SHIFT 8
>
> #define SCALER_CTL0_SCL0_MASK VC4_MASK(7, 5)
> #define SCALER_CTL0_SCL0_SHIFT 5
>
> +#define SCALER_CTL0_EXPAND_ZERO0
> +#define SCALER_CTL0_EXPAND_LSB 1
> +#define SCALER_CTL0_EXPAND_MSB 2
> +#define SCALER_CTL0_EXPAND_REPEAT 3
Just a heads up, those defines have recently landed in drm-misc, with
slightly different naming: SCALER_CTL0_RGBA_EXPAND_*
Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [BUG] drm/vc4: vblank wait timed out
On Sat, May 5, 2018 at 1:47 PM, Stefan Wahren wrote: > Hi, > > after submit of the latest bcm2835 clock fixes, i thought this issue has been > fixed. But i've seen this issue with current mainline 4.17-rc3 > (bcm2835_defconfig) on Raspberry Pi 1 B (using U-Boot TFTP Boot). Strangly i > couldn't reproduce this issue with same kernel but using only the Foundation > bootloader (without U-Boot TFTP Boot). > > U-Boot version: 2018.03 > > I triggered the warning using my HDMI switch: > > [ 198.304572] [ cut here ] > [ 198.304693] WARNING: CPU: 0 PID: 10 at > drivers/gpu/drm/drm_atomic_helper.c:1351 > drm_atomic_helper_wait_for_vblanks+0x1d4/0x1f0 > [ 198.304703] [CRTC:68:crtc-2] vblank wait timed out > [ 198.304751] Modules linked in: bcm2835_rng rng_core vchiq(C) > [ 198.304790] CPU: 0 PID: 10 Comm: kworker/0:1 Tainted: G C > 4.17.0-rc3+ #1 > [ 198.304796] Hardware name: BCM2835 > [ 198.304817] Workqueue: events output_poll_execute > [ 198.304867] [] (unwind_backtrace) from [] > (show_stack+0x20/0x24) > [ 198.304934] [] (show_stack) from [] > (dump_stack+0x20/0x28) > [ 198.304971] [] (dump_stack) from [] (__warn+0xec/0x104) > [ 198.305012] [] (__warn) from [] > (warn_slowpath_fmt+0x48/0x50) > [ 198.305048] [] (warn_slowpath_fmt) from [] > (drm_atomic_helper_wait_for_vblanks+0x1d4/0x1f0) > [ 198.305098] [] (drm_atomic_helper_wait_for_vblanks) from > [] (vc4_atomic_complete_commit+0x80/0xb8) > [ 198.305144] [] (vc4_atomic_complete_commit) from [] > (vc4_atomic_commit+0x110/0x11c) > [ 198.305174] [] (vc4_atomic_commit) from [] > (drm_atomic_commit+0x50/0x60) > [ 198.305202] [] (drm_atomic_commit) from [] > (restore_fbdev_mode_atomic+0x80/0x1cc) > [ 198.305228] [] (restore_fbdev_mode_atomic) from [] > (restore_fbdev_mode+0x38/0x144) > [ 198.305270] [] (restore_fbdev_mode) from [] > (drm_fb_helper_restore_fbdev_mode_unlocked+0x58/0x8c) > [ 198.305296] [] (drm_fb_helper_restore_fbdev_mode_unlocked) from > [] (drm_fb_helper_set_par+0x54/0x60) > [ 198.305320] [] (drm_fb_helper_set_par) from [] > (drm_fb_helper_hotplug_event+0xc8/0xd4) > [ 198.305343] [] (drm_fb_helper_hotplug_event) from [] > (drm_fb_helper_output_poll_changed+0x1c/0x20) > [ 198.305382] [] (drm_fb_helper_output_poll_changed) from > [] (drm_kms_helper_hotplug_event+0x34/0x38) > [ 198.305409] [] (drm_kms_helper_hotplug_event) from [] > (output_poll_execute+0x16c/0x17c) > [ 198.305440] [] (output_poll_execute) from [] > (process_one_work+0x1e0/0x368) > [ 198.305466] [] (process_one_work) from [] > (worker_thread+0x2a0/0x418) > [ 198.305511] [] (worker_thread) from [] > (kthread+0x144/0x15c) > [ 198.305539] [] (kthread) from [] > (ret_from_fork+0x14/0x2c) > [ 198.305549] Exception stack(0xc9939fb0 to 0xc9939ff8) > [ 198.305562] 9fa0: > > [ 198.305578] 9fc0: > > [ 198.305591] 9fe0: 0013 > [ 198.305630] ---[ end trace 9c4071c657268b83 ]--- > > I also dumped clk_summary in both cases, but they were identical. > > Are their any helpful information, which i can provide? > > Best regards > Stefan I have seen this happen with one of those cheap Waveshare HDMI displays. More often when connecting its power to the RPi versus on a separate supply. Goes away when using a real, proper HDMI display. They come with bad EDID data so I pretty much just blamed the display at the time. Does this happen during a switch and whats on the other end? Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Add a pad field to align drm_vc4_submit_cl to 64 bits.
On Tue, May 1, 2018 at 1:59 AM, Eric Anholt wrote: > I had originally asked Stefan Schake to drop the pad field from the > syncobj changes that just landed, because I couldn't come up with a > reason to align to 64 bits. > > Talking with Dave Airlie about the new v3d driver's submit ioctl, we > came up with a reason: sizeof() on 64-bit platforms may align to 64 > bits, in which case the userspace will be submitting the aligned size > and the final 32 bits won't be zero-padded by the kernel. If > userspace doesn't zero-fill, then a future ABI change adding a 32-bit > field at the end could potentially cause the kernel to read undefined > data from old userspace (our userspace happens to use structure > initialization that zero-fills, but as a general rule we try not to > rely on that in the kernel). > Did a quick sizeof check on arm64 and that indeed came to 176 mod 8 = 0, suggesting we need this additional pad. So fwiw Reviewed-by: Stefan Schake Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/tegra: hub: Use state directly
On Wed, Apr 18, 2018 at 12:40 PM, Stefan Schake wrote:
> Using drm_atomic_get_private_obj_state after state has been swapped
> will return old state.
>
> Fixes: 0281c4149021 ("drm/tegra: hub: Use private object for global state")
> Signed-off-by: Stefan Schake
> ---
> drivers/gpu/drm/tegra/hub.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
> index 9a3f23d4780f..bdd2cdd0745c 100644
> --- a/drivers/gpu/drm/tegra/hub.c
> +++ b/drivers/gpu/drm/tegra/hub.c
> @@ -683,12 +683,11 @@ void tegra_display_hub_atomic_commit(struct drm_device
> *drm,
> {
> struct tegra_drm *tegra = drm->dev_private;
> struct tegra_display_hub *hub = tegra->hub;
> - struct tegra_display_hub_state *hub_state;
> + struct tegra_display_hub_state *hub_state =
> + to_tegra_display_hub_state(hub->base.state);
> struct device *dev = hub->client.dev;
> int err;
>
> - hub_state = tegra_display_hub_get_state(hub, state);
> -
> if (hub_state->clk) {
> err = clk_set_rate(hub_state->clk, hub_state->rate);
> if (err < 0)
> --
> 2.14.1
>
Ping. I don't really have Tegra hardware to begin with but this is
one of the few examples of DRM private driver state so I figured
I'd send a quick fixup before anyone else tries to copy it.
Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/3] drm/vc4: Syncobj import support
Allow userland to specify a syncobj that is waited on before a render job
starts processing.
v2: Use 0 as invalid syncobj to drop flag (Eric)
Drop extra newline (Eric)
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_drv.h | 1 +
drivers/gpu/drm/vc4/vc4_gem.c | 30 +-
include/uapi/drm/vc4_drm.h| 7 +++
3 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 22589d3..554a4e8 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include "uapi/drm/vc4_drm.h"
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 2107b0d..e305ccde 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "uapi/drm/vc4_drm.h"
#include "vc4_drv.h"
@@ -1115,6 +1116,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
struct drm_vc4_submit_cl *args = data;
struct vc4_exec_info *exec;
struct ww_acquire_ctx acquire_ctx;
+ struct dma_fence *in_fence;
int ret = 0;
if ((args->flags & ~(VC4_SUBMIT_CL_USE_CLEAR_COLOR |
@@ -1125,11 +1127,6 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
return -EINVAL;
}
- if (args->pad2 != 0) {
- DRM_DEBUG("->pad2 must be set to zero\n");
- return -EINVAL;
- }
-
exec = kcalloc(1, sizeof(*exec), GFP_KERNEL);
if (!exec) {
DRM_ERROR("malloc failure on exec struct\n");
@@ -1164,6 +1161,29 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
}
}
+ if (args->in_sync) {
+ ret = drm_syncobj_find_fence(file_priv, args->in_sync,
+&in_fence);
+ if (ret)
+ goto fail;
+
+ /* When the fence (or fence array) is exclusively from our
+* context we can skip the wait since jobs are executed in
+* order of their submission through this ioctl and this can
+* only have fences from a prior job.
+*/
+ if (!dma_fence_match_context(in_fence,
+vc4->dma_fence_context)) {
+ ret = dma_fence_wait(in_fence, true);
+ if (ret) {
+ dma_fence_put(in_fence);
+ goto fail;
+ }
+ }
+
+ dma_fence_put(in_fence);
+ }
+
if (exec->args->bin_cl_size != 0) {
ret = vc4_get_bcl(dev, exec);
if (ret)
diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
index b95a0e1..d97065b 100644
--- a/include/uapi/drm/vc4_drm.h
+++ b/include/uapi/drm/vc4_drm.h
@@ -183,11 +183,10 @@ struct drm_vc4_submit_cl {
/* ID of the perfmon to attach to this job. 0 means no perfmon. */
__u32 perfmonid;
- /* Unused field to align this struct on 64 bits. Must be set to 0.
-* If one ever needs to add an u32 field to this struct, this field
-* can be used.
+ /* Syncobj handle to wait on. If set, processing of this render job
+* will not start until the syncobj is signaled. 0 means ignore.
*/
- __u32 pad2;
+ __u32 in_sync;
};
/**
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/3] drm/vc4: Export fence through syncobj
Allow specifying a syncobj on render job submission where we store the
fence for the job. This gives userland flexible access to the fence.
v2: Use 0 as invalid syncobj to drop flag (Eric)
Don't reintroduce the padding (Eric)
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_gem.c | 30 --
include/uapi/drm/vc4_drm.h| 6 ++
2 files changed, 34 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index e305ccde..a4c4be3 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -656,7 +656,8 @@ vc4_lock_bo_reservations(struct drm_device *dev,
*/
static int
vc4_queue_submit(struct drm_device *dev, struct vc4_exec_info *exec,
-struct ww_acquire_ctx *acquire_ctx)
+struct ww_acquire_ctx *acquire_ctx,
+struct drm_syncobj *out_sync)
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct vc4_exec_info *renderjob;
@@ -679,6 +680,9 @@ vc4_queue_submit(struct drm_device *dev, struct
vc4_exec_info *exec,
fence->seqno = exec->seqno;
exec->fence = &fence->base;
+ if (out_sync)
+ drm_syncobj_replace_fence(out_sync, exec->fence);
+
vc4_update_bo_seqnos(exec, seqno);
vc4_unlock_bo_reservations(dev, exec, acquire_ctx);
@@ -1114,6 +1118,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct vc4_file *vc4file = file_priv->driver_priv;
struct drm_vc4_submit_cl *args = data;
+ struct drm_syncobj *out_sync = NULL;
struct vc4_exec_info *exec;
struct ww_acquire_ctx acquire_ctx;
struct dma_fence *in_fence;
@@ -1201,12 +1206,33 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
+ if (args->out_sync) {
+ out_sync = drm_syncobj_find(file_priv, args->out_sync);
+ if (!out_sync) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* We replace the fence in out_sync in vc4_queue_submit since
+* the render job could execute immediately after that call.
+* If it finishes before our ioctl processing resumes the
+* render job fence could already have been freed.
+*/
+ }
+
/* Clear this out of the struct we'll be putting in the queue,
* since it's part of our stack.
*/
exec->args = NULL;
- ret = vc4_queue_submit(dev, exec, &acquire_ctx);
+ ret = vc4_queue_submit(dev, exec, &acquire_ctx, out_sync);
+
+ /* The syncobj isn't part of the exec data and we need to free our
+* reference even if job submission failed.
+*/
+ if (out_sync)
+ drm_syncobj_put(out_sync);
+
if (ret)
goto fail;
diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
index d97065b..2be4fe3 100644
--- a/include/uapi/drm/vc4_drm.h
+++ b/include/uapi/drm/vc4_drm.h
@@ -187,6 +187,12 @@ struct drm_vc4_submit_cl {
* will not start until the syncobj is signaled. 0 means ignore.
*/
__u32 in_sync;
+
+ /* Syncobj handle to export fence to. If set, the fence in the syncobj
+* will be replaced with a fence that signals upon completion of this
+* render job. 0 means ignore.
+*/
+ __u32 out_sync;
};
/**
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/3] drm/vc4: Syncobj import export
v2 drops the extra syncobj parameter and gets rid of the submit flags since 0 is never a valid syncobj handle. This series allows userspace to submit syncobj handles for importing a fence to wait on or exporting the job fence as part of submission. The primary use of this is to enable native fence fd support in Mesa. Userspace implementation is under review and available here: https://patchwork.freedesktop.org/series/42081/ Stefan Schake (3): drm/vc4: Syncobj import support drm/vc4: Export fence through syncobj drm/vc4: Enable syncobj support drivers/gpu/drm/vc4/vc4_drv.c | 3 ++- drivers/gpu/drm/vc4/vc4_drv.h | 1 + drivers/gpu/drm/vc4/vc4_gem.c | 60 ++- include/uapi/drm/vc4_drm.h| 13 +++--- 4 files changed, 65 insertions(+), 12 deletions(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/3] drm/vc4: Enable syncobj support
This doesn't require any additional functionality from the driver but
is a prerequisite to userland calling the syncobj ioctls.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 40ddeaa..d9b8b70 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -175,7 +175,8 @@ static struct drm_driver vc4_drm_driver = {
DRIVER_GEM |
DRIVER_HAVE_IRQ |
DRIVER_RENDER |
- DRIVER_PRIME),
+ DRIVER_PRIME |
+ DRIVER_SYNCOBJ),
.lastclose = drm_fb_helper_lastclose,
.open = vc4_open,
.postclose = vc4_close,
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH 2/2] drm_hwcomposer: Fall back to client compositon if the gl precompostior fails
On Tue, Apr 24, 2018 at 10:09 AM, Alexandru-Cosmin Gheorghe
wrote:
> On Mon, Apr 23, 2018 at 05:06:44PM -0700, John Stultz wrote:
>> If the gl precompositor isn't being used, we cannot accept
>> every layer as a device composited layer.
>>
>> Thus this patch adds some extra logic in the validate function
>> to try to map layers to available device planes, falling back
>> to client side compositing if we run-out of planes.
>>
>> Credit to Rob Herring, who's work this single plane patch was
>> originally based on.
>>
>> Feedback or alternative ideas would be greatly appreciated!
>>
>> Cc: Marissa Wall
>> Cc: Sean Paul
>> Cc: Dmitry Shmidt
>> Cc: Robert Foss
>> Cc: Matt Szczesiak
>> Cc: Liviu Dudau
>> Cc: David Hanna
>> Cc: Rob Herring
>> Cc: Alexandru-Cosmin Gheorghe
>> Cc: Alistair Strachan
>> Signed-off-by: John Stultz
>> Signed-off-by: John Stultz
>> ---
>> drmhwctwo.cpp | 18 ++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/drmhwctwo.cpp b/drmhwctwo.cpp
>> index dfca1a6..437a439 100644
>> --- a/drmhwctwo.cpp
>> +++ b/drmhwctwo.cpp
>> @@ -686,6 +686,15 @@ HWC2::Error
>> DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>>supported(__func__);
>>*num_types = 0;
>>*num_requests = 0;
>> + int avail_planes = primary_planes_.size() + overlay_planes_.size();
>> +
>> + /*
>> + * If more layers then planes, save one plane
>> + * for client composited layers
>> + */
>> + if (avail_planes < layers_.size())
>> + avail_planes--;
>> +
>>for (std::pair &l : layers_) {
>> DrmHwcTwo::HwcLayer &layer = l.second;
>> switch (layer.sf_type()) {
>> @@ -695,6 +704,15 @@ HWC2::Error
>> DrmHwcTwo::HwcDisplay::ValidateDisplay(uint32_t *num_types,
>> layer.set_validated_type(HWC2::Composition::Client);
>> ++*num_types;
>> break;
>> + case HWC2::Composition::Device:
>> +if (!compositor_.uses_GL() && !avail_planes) {
>
> Something is not entirely correct here, you can't assume just because
> you have planes available they can be used.
> E.g Layer has rotation, but the plane doesn't support it.
> This part is handled by the planning part in presentDisplay which
> falls back on GPU.
>
> I think the easiest and safe way is to just fallback to
> Composition::Client whenever the GLCompositor failed to initialize.
>
I agree, this would only work out for the single plane case where you end
up using client composition for everything.
In the spirit of DRM what we would probably want is to atomic test a
composition, and if that fails we can still fallback to client or GL
compositor depending on its availability. And then in a far far away
future we might even do a few iterations simplifying our composition until
it atomic tests correctly so we can still offload some "simple" parts
like the cursor.
Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/4] drm/vc4: Add parameter for syncobj support
This allows runtime detection of syncobj submission support.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_drv.c | 1 +
include/uapi/drm/vc4_drm.h| 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 806c8004b793..4e2ae2a9a164 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -102,6 +102,7 @@ static int vc4_get_param_ioctl(struct drm_device *dev, void
*data,
case DRM_VC4_PARAM_SUPPORTS_FIXED_RCL_ORDER:
case DRM_VC4_PARAM_SUPPORTS_MADVISE:
case DRM_VC4_PARAM_SUPPORTS_PERFMON:
+ case DRM_VC4_PARAM_SUPPORTS_SYNCOBJ:
args->value = true;
break;
default:
diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
index 3a2ef9b5b60b..6f81cf05f7a3 100644
--- a/include/uapi/drm/vc4_drm.h
+++ b/include/uapi/drm/vc4_drm.h
@@ -338,6 +338,7 @@ struct drm_vc4_get_hang_state {
#define DRM_VC4_PARAM_SUPPORTS_FIXED_RCL_ORDER 6
#define DRM_VC4_PARAM_SUPPORTS_MADVISE 7
#define DRM_VC4_PARAM_SUPPORTS_PERFMON 8
+#define DRM_VC4_PARAM_SUPPORTS_SYNCOBJ 9
struct drm_vc4_get_param {
__u32 param;
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/4] drm/vc4: Enable syncobj support
This doesn't require any additional functionality from the driver but
is a prerequisite to userland calling the syncobj ioctls.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_drv.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 94b99c90425a..806c8004b793 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -175,7 +175,8 @@ static struct drm_driver vc4_drm_driver = {
DRIVER_GEM |
DRIVER_HAVE_IRQ |
DRIVER_RENDER |
- DRIVER_PRIME),
+ DRIVER_PRIME |
+ DRIVER_SYNCOBJ),
.lastclose = drm_fb_helper_lastclose,
.open = vc4_open,
.postclose = vc4_close,
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/4] drm/vc4: Export fence through syncobj
Allow specifying a syncobj on render job submission where we store the
fence for the job. This gives userland flexible access to the fence.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_gem.c | 38 +++---
include/uapi/drm/vc4_drm.h| 13 +
2 files changed, 48 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 232363488125..b39515a4ddcb 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -656,7 +656,8 @@ vc4_lock_bo_reservations(struct drm_device *dev,
*/
static int
vc4_queue_submit(struct drm_device *dev, struct vc4_exec_info *exec,
-struct ww_acquire_ctx *acquire_ctx)
+struct ww_acquire_ctx *acquire_ctx,
+struct drm_syncobj *out_sync)
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct vc4_exec_info *renderjob;
@@ -679,6 +680,9 @@ vc4_queue_submit(struct drm_device *dev, struct
vc4_exec_info *exec,
fence->seqno = exec->seqno;
exec->fence = &fence->base;
+ if (out_sync)
+ drm_syncobj_replace_fence(out_sync, exec->fence);
+
vc4_update_bo_seqnos(exec, seqno);
vc4_unlock_bo_reservations(dev, exec, acquire_ctx);
@@ -1114,6 +1118,7 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct vc4_file *vc4file = file_priv->driver_priv;
struct drm_vc4_submit_cl *args = data;
+ struct drm_syncobj *out_sync = NULL;
struct vc4_exec_info *exec;
struct ww_acquire_ctx acquire_ctx;
struct dma_fence *in_fence;
@@ -1123,11 +1128,17 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
VC4_SUBMIT_CL_FIXED_RCL_ORDER |
VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X |
VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y |
-VC4_SUBMIT_CL_IMPORT_SYNCOBJ)) != 0) {
+VC4_SUBMIT_CL_IMPORT_SYNCOBJ |
+VC4_SUBMIT_CL_EXPORT_SYNCOBJ)) != 0) {
DRM_DEBUG("Unknown flags: 0x%02x\n", args->flags);
return -EINVAL;
}
+ if (args->pad2 != 0) {
+ DRM_DEBUG("->pad2 must be set to zero\n");
+ return -EINVAL;
+ }
+
exec = kcalloc(1, sizeof(*exec), GFP_KERNEL);
if (!exec) {
DRM_ERROR("malloc failure on exec struct\n");
@@ -1202,12 +1213,33 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
if (ret)
goto fail;
+ if (args->flags & VC4_SUBMIT_CL_EXPORT_SYNCOBJ) {
+ out_sync = drm_syncobj_find(file_priv, args->out_sync);
+ if (!out_sync) {
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* We replace the fence in out_sync in vc4_queue_submit since
+* the render job could execute immediately after that call.
+* If it finishes before our ioctl processing resumes the
+* render job fence could already have been freed.
+*/
+ }
+
/* Clear this out of the struct we'll be putting in the queue,
* since it's part of our stack.
*/
exec->args = NULL;
- ret = vc4_queue_submit(dev, exec, &acquire_ctx);
+ ret = vc4_queue_submit(dev, exec, &acquire_ctx, out_sync);
+
+ /* The syncobj isn't part of the exec data and we need to free our
+* reference even if job submission failed.
+*/
+ if (out_sync)
+ drm_syncobj_put(out_sync);
+
if (ret)
goto fail;
diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
index 389f21931c25..3a2ef9b5b60b 100644
--- a/include/uapi/drm/vc4_drm.h
+++ b/include/uapi/drm/vc4_drm.h
@@ -174,6 +174,7 @@ struct drm_vc4_submit_cl {
#define VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X (1 << 2)
#define VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y (1 << 3)
#define VC4_SUBMIT_CL_IMPORT_SYNCOBJ (1 << 4)
+#define VC4_SUBMIT_CL_EXPORT_SYNCOBJ (1 << 5)
__u32 flags;
/* Returned value of the seqno of this render job (for the
@@ -189,6 +190,18 @@ struct drm_vc4_submit_cl {
* syncobj is signalled.
*/
__u32 in_sync;
+
+ /* Syncobj handle to export fence to. Set together with EXPORT_SYNCOBJ
+* flag. If set, the fence in the syncobj will be replaced with a fence
+* that signals upon completion of this render job.
+*/
+ __u32 out_sync;
+
+ /* Unused field to align this struct on 64 bits. Must be set to 0.
+* If one ever needs to
[PATCH 2/4] drm/vc4: Syncobj import support
Allow userland to specify a syncobj that is waited on before a render job
starts processing.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_drv.h | 2 ++
drivers/gpu/drm/vc4/vc4_gem.c | 33 +++--
include/uapi/drm/vc4_drm.h| 9 +
3 files changed, 34 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 4288615b66a2..3105df99cb12 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -10,6 +10,8 @@
#include
#include
#include
+#include
+
#include "uapi/drm/vc4_drm.h"
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 2107b0daf8ef..232363488125 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "uapi/drm/vc4_drm.h"
#include "vc4_drv.h"
@@ -1115,21 +1116,18 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
struct drm_vc4_submit_cl *args = data;
struct vc4_exec_info *exec;
struct ww_acquire_ctx acquire_ctx;
+ struct dma_fence *in_fence;
int ret = 0;
if ((args->flags & ~(VC4_SUBMIT_CL_USE_CLEAR_COLOR |
VC4_SUBMIT_CL_FIXED_RCL_ORDER |
VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X |
-VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y)) != 0) {
+VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y |
+VC4_SUBMIT_CL_IMPORT_SYNCOBJ)) != 0) {
DRM_DEBUG("Unknown flags: 0x%02x\n", args->flags);
return -EINVAL;
}
- if (args->pad2 != 0) {
- DRM_DEBUG("->pad2 must be set to zero\n");
- return -EINVAL;
- }
-
exec = kcalloc(1, sizeof(*exec), GFP_KERNEL);
if (!exec) {
DRM_ERROR("malloc failure on exec struct\n");
@@ -1164,6 +1162,29 @@ vc4_submit_cl_ioctl(struct drm_device *dev, void *data,
}
}
+ if (args->flags & VC4_SUBMIT_CL_IMPORT_SYNCOBJ) {
+ ret = drm_syncobj_find_fence(file_priv, args->in_sync,
+&in_fence);
+ if (ret)
+ goto fail;
+
+ /* When the fence (or fence array) is exclusively from our
+* context we can skip the wait since jobs are executed in
+* order of their submission through this ioctl and this can
+* only have fences from a prior job.
+*/
+ if (!dma_fence_match_context(in_fence,
+vc4->dma_fence_context)) {
+ ret = dma_fence_wait(in_fence, true);
+ if (ret) {
+ dma_fence_put(in_fence);
+ goto fail;
+ }
+ }
+
+ dma_fence_put(in_fence);
+ }
+
if (exec->args->bin_cl_size != 0) {
ret = vc4_get_bcl(dev, exec);
if (ret)
diff --git a/include/uapi/drm/vc4_drm.h b/include/uapi/drm/vc4_drm.h
index b95a0e11cb07..389f21931c25 100644
--- a/include/uapi/drm/vc4_drm.h
+++ b/include/uapi/drm/vc4_drm.h
@@ -173,6 +173,7 @@ struct drm_vc4_submit_cl {
#define VC4_SUBMIT_CL_FIXED_RCL_ORDER (1 << 1)
#define VC4_SUBMIT_CL_RCL_ORDER_INCREASING_X (1 << 2)
#define VC4_SUBMIT_CL_RCL_ORDER_INCREASING_Y (1 << 3)
+#define VC4_SUBMIT_CL_IMPORT_SYNCOBJ (1 << 4)
__u32 flags;
/* Returned value of the seqno of this render job (for the
@@ -183,11 +184,11 @@ struct drm_vc4_submit_cl {
/* ID of the perfmon to attach to this job. 0 means no perfmon. */
__u32 perfmonid;
- /* Unused field to align this struct on 64 bits. Must be set to 0.
-* If one ever needs to add an u32 field to this struct, this field
-* can be used.
+ /* Syncobj handle to wait on. Set together with IMPORT_SYNCOBJ flag.
+* If set, processing of this render job will not start until the
+* syncobj is signalled.
*/
- __u32 pad2;
+ __u32 in_sync;
};
/**
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/4] drm/vc4: Syncobj import export
This series allows userspace to submit syncobj handles for importing a fence to wait on or exporting the job fence as part of submission. The primary use of this is to enable native fence fd support in Mesa. Initial userspace implementation is under review and available here: https://patchwork.freedesktop.org/series/42081/ Stefan Schake (4): drm/vc4: Enable syncobj support drm/vc4: Syncobj import support drm/vc4: Export fence through syncobj drm/vc4: Add parameter for syncobj support drivers/gpu/drm/vc4/vc4_drv.c | 4 ++- drivers/gpu/drm/vc4/vc4_drv.h | 2 ++ drivers/gpu/drm/vc4/vc4_gem.c | 59 --- include/uapi/drm/vc4_drm.h| 15 +++ 4 files changed, 76 insertions(+), 4 deletions(-) -- 2.14.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/vc4: Add support for plane alpha
The HVS supports mixing fixed alpha with per-pixel alpha or
setting a fixed plane alpha in case there is no per-pixel information.
This allows us to support the generic DRM plane alpha property.
Signed-off-by: Stefan Schake
---
v2: Non-opaque plane alpha can trigger the background blending issue
and we need to hint the CRTC that background fill might be required.
drivers/gpu/drm/vc4/vc4_plane.c | 21 +
drivers/gpu/drm/vc4/vc4_regs.h | 1 +
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index c3a37a99e601..3483c05cc3d6 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -201,6 +201,7 @@ static void vc4_plane_reset(struct drm_plane *plane)
return;
plane->state = &vc4_state->base;
+ plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
vc4_state->base.plane = plane;
}
@@ -467,6 +468,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
u32 ctl0_offset = vc4_state->dlist_count;
const struct hvs_format *format =
vc4_get_hvs_format(fb->format->format);
int num_planes = drm_format_num_planes(format->drm);
+ bool mix_plane_alpha;
bool covers_screen;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
@@ -552,7 +554,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
/* Position Word 0: Image Positions and Alpha Value */
vc4_state->pos0_offset = vc4_state->dlist_count;
vc4_dlist_write(vc4_state,
- VC4_SET_FIELD(0xff, SCALER_POS0_FIXED_ALPHA) |
+ VC4_SET_FIELD(state->alpha >> 8,
SCALER_POS0_FIXED_ALPHA) |
VC4_SET_FIELD(vc4_state->crtc_x, SCALER_POS0_START_X) |
VC4_SET_FIELD(vc4_state->crtc_y, SCALER_POS0_START_Y));
@@ -565,6 +567,13 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS1_SCL_HEIGHT));
}
+ /* Don't waste cycles mixing with plane alpha if the set alpha
+* is opaque or there is no per-pixel alpha information.
+* In any case we use the alpha property value as the fixed alpha.
+*/
+ mix_plane_alpha = state->alpha != DRM_BLEND_ALPHA_OPAQUE &&
+ fb->format->has_alpha;
+
/* Position Word 2: Source Image Size, Alpha */
vc4_state->pos2_offset = vc4_state->dlist_count;
vc4_dlist_write(vc4_state,
@@ -572,6 +581,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS2_ALPHA_MODE_PIPELINE :
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
+ (mix_plane_alpha ? SCALER_POS2_ALPHA_MIX : 0) |
(fb->format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0)
|
VC4_SET_FIELD(vc4_state->src_w[0], SCALER_POS2_WIDTH) |
VC4_SET_FIELD(vc4_state->src_h[0], SCALER_POS2_HEIGHT));
@@ -653,10 +663,11 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
vc4_state->crtc_w == state->crtc->mode.hdisplay &&
vc4_state->crtc_h == state->crtc->mode.vdisplay;
/* Background fill might be necessary when the plane has per-pixel
-* alpha content and blends from the background or does not cover
-* the entire screen.
+* alpha content or a non-opaque plane alpha and could blend from the
+* background or does not cover the entire screen.
*/
- vc4_state->needs_bg_fill = fb->format->has_alpha || !covers_screen;
+ vc4_state->needs_bg_fill = fb->format->has_alpha || !covers_screen ||
+ state->alpha != DRM_BLEND_ALPHA_OPAQUE;
return 0;
}
@@ -916,5 +927,7 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
drm_plane_helper_add(plane, &vc4_plane_helper_funcs);
+ drm_plane_create_alpha_property(plane);
+
return plane;
}
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index 4af3e29d076a..d1fb6fec46eb 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -945,6 +945,7 @@ enum hvs_pixel_format {
#define SCALER_POS2_ALPHA_MODE_FIXED_NONZERO 2
#define SCALER_POS2_ALPHA_MODE_FIXED_OVER_0x07 3
#define SCALER_POS2_ALPHA_PREMULT BIT(29)
+#define SCALER_POS2_ALPHA_MIX BIT(28)
#define SCALER_POS2_HEIGHT_MASKVC4_MASK(27, 16)
#define SCALER_POS2_HEIGHT_SHIFT 16
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Add support for plane alpha
On Sat, Apr 21, 2018 at 1:45 AM, Eric Anholt wrote: > Stefan Schake writes: > >> The HVS supports mixing fixed alpha with per-pixel alpha or >> setting a fixed plane alpha in case there is no per-pixel information. >> This allows us to support the generic DRM plane alpha property. > > Don't we need to set needs_bg_fill based on alpha != OPAQUE, as well? > > Other than that, looks good to me. I did forget about the whole background situation, and a quick test shows you are right: anything less than opaque has the funky repeating pattern artifact. Thanks, I'll send a new version. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/vc4: Add support for plane alpha
The HVS supports mixing fixed alpha with per-pixel alpha or
setting a fixed plane alpha in case there is no per-pixel information.
This allows us to support the generic DRM plane alpha property.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_plane.c | 14 +-
drivers/gpu/drm/vc4/vc4_regs.h | 1 +
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index c3a37a99e601..b06e0ec013c5 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -201,6 +201,7 @@ static void vc4_plane_reset(struct drm_plane *plane)
return;
plane->state = &vc4_state->base;
+ plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
vc4_state->base.plane = plane;
}
@@ -467,6 +468,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
u32 ctl0_offset = vc4_state->dlist_count;
const struct hvs_format *format =
vc4_get_hvs_format(fb->format->format);
int num_planes = drm_format_num_planes(format->drm);
+ bool mix_plane_alpha;
bool covers_screen;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
@@ -552,7 +554,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
/* Position Word 0: Image Positions and Alpha Value */
vc4_state->pos0_offset = vc4_state->dlist_count;
vc4_dlist_write(vc4_state,
- VC4_SET_FIELD(0xff, SCALER_POS0_FIXED_ALPHA) |
+ VC4_SET_FIELD(state->alpha >> 8,
SCALER_POS0_FIXED_ALPHA) |
VC4_SET_FIELD(vc4_state->crtc_x, SCALER_POS0_START_X) |
VC4_SET_FIELD(vc4_state->crtc_y, SCALER_POS0_START_Y));
@@ -565,6 +567,13 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS1_SCL_HEIGHT));
}
+ /* Don't waste cycles mixing with plane alpha if the set alpha
+* is opaque or there is no per-pixel alpha information.
+* In any case we use the alpha property value as the fixed alpha.
+*/
+ mix_plane_alpha = state->alpha != DRM_BLEND_ALPHA_OPAQUE &&
+ fb->format->has_alpha;
+
/* Position Word 2: Source Image Size, Alpha */
vc4_state->pos2_offset = vc4_state->dlist_count;
vc4_dlist_write(vc4_state,
@@ -572,6 +581,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS2_ALPHA_MODE_PIPELINE :
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
+ (mix_plane_alpha ? SCALER_POS2_ALPHA_MIX : 0) |
(fb->format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0)
|
VC4_SET_FIELD(vc4_state->src_w[0], SCALER_POS2_WIDTH) |
VC4_SET_FIELD(vc4_state->src_h[0], SCALER_POS2_HEIGHT));
@@ -916,5 +926,7 @@ struct drm_plane *vc4_plane_init(struct drm_device *dev,
drm_plane_helper_add(plane, &vc4_plane_helper_funcs);
+ drm_plane_create_alpha_property(plane);
+
return plane;
}
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index 4af3e29d076a..d1fb6fec46eb 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -945,6 +945,7 @@ enum hvs_pixel_format {
#define SCALER_POS2_ALPHA_MODE_FIXED_NONZERO 2
#define SCALER_POS2_ALPHA_MODE_FIXED_OVER_0x07 3
#define SCALER_POS2_ALPHA_PREMULT BIT(29)
+#define SCALER_POS2_ALPHA_MIX BIT(28)
#define SCALER_POS2_HEIGHT_MASKVC4_MASK(27, 16)
#define SCALER_POS2_HEIGHT_SHIFT 16
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 2/2] drm/vc4: Add CTM registers to debugfs
Now that we set the OLED* registers to do CTM, it's helpful to have them
in the register dump.
Signed-off-by: Stefan Schake
---
v4: new in the series.
drivers/gpu/drm/vc4/vc4_hvs.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_hvs.c b/drivers/gpu/drm/vc4/vc4_hvs.c
index 2b62fc5b8d85..5d8c749c9749 100644
--- a/drivers/gpu/drm/vc4/vc4_hvs.c
+++ b/drivers/gpu/drm/vc4/vc4_hvs.c
@@ -58,6 +58,10 @@ static const struct {
HVS_REG(SCALER_DISPSTAT2),
HVS_REG(SCALER_DISPBASE2),
HVS_REG(SCALER_DISPALPHA2),
+ HVS_REG(SCALER_OLEDOFFS),
+ HVS_REG(SCALER_OLEDCOEF0),
+ HVS_REG(SCALER_OLEDCOEF1),
+ HVS_REG(SCALER_OLEDCOEF2),
};
void vc4_hvs_dump_state(struct drm_device *dev)
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/2] drm/vc4: Add CTM support
The hardware has a single block for applying a CTM prior to gamma lut.
It can be fed with pixels from one of our CRTC at a time and uses a
matrix with S0.9 scalars. Use private atomic state to reject attempts
from userland to apply CTM for more than one CRTC at a time and reject
matrices with scalars that we can't approximate without integer bits.
Signed-off-by: Stefan Schake
Signed-off-by: Eric Anholt
---
Eric, I removed your r-b since there were a bunch more changes.
v4: Handle CTM disabling in check
Don't use drm_atomic_get_private_obj_state in commit
Fix S31.32 -> S0.9 conversion
Squashed in Erics fixup series:
Don't take the CTM lock unless a CTM change is happening (Eric)
Lock across changes to the CTM (Eric)
Handle allocation failure in out CTM priv state (Eric)
Move CTM object init before FBDEV setup (Eric)
Clean up the CTM private object manager on unload (Eric)
v3: New in the series.
drivers/gpu/drm/vc4/vc4_crtc.c | 5 +
drivers/gpu/drm/vc4/vc4_drv.c | 3 +
drivers/gpu/drm/vc4/vc4_drv.h | 4 +
drivers/gpu/drm/vc4/vc4_kms.c | 204 -
4 files changed, 215 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 08fe8dd7d8df..83d3b7912fc2 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -1018,6 +1018,11 @@ static int vc4_crtc_bind(struct device *dev, struct
device *master, void *data)
drm_mode_crtc_set_gamma_size(crtc, ARRAY_SIZE(vc4_crtc->lut_r));
drm_crtc_enable_color_mgmt(crtc, 0, false, crtc->gamma_size);
+ /* We support CTM, but only for one CRTC at a time. It's therefore
+* implemented as private driver state in vc4_kms, not here.
+*/
+ drm_crtc_enable_color_mgmt(crtc, 0, true, crtc->gamma_size);
+
/* Set up some arbitrary number of planes. We're not limited
* by a set number of physical registers, just the space in
* the HVS (16k) and how small an plane can be (28 bytes).
diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 94b99c90425a..52bfe0e9ddda 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -320,6 +320,7 @@ static void vc4_drm_unbind(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct drm_device *drm = platform_get_drvdata(pdev);
+ struct vc4_dev *vc4 = to_vc4_dev(drm);
drm_dev_unregister(drm);
@@ -327,6 +328,8 @@ static void vc4_drm_unbind(struct device *dev)
drm_mode_config_cleanup(drm);
+ drm_atomic_private_obj_fini(&vc4->ctm_manager);
+
drm_dev_unref(drm);
}
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 4288615b66a2..22589d39083c 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include "uapi/drm/vc4_drm.h"
@@ -193,6 +194,9 @@ struct vc4_dev {
} hangcheck;
struct semaphore async_modeset;
+
+ struct drm_modeset_lock ctm_state_lock;
+ struct drm_private_obj ctm_manager;
};
static inline struct vc4_dev *
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index e791e498a3dd..8a411e5f8776 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -23,6 +23,117 @@
#include
#include
#include "vc4_drv.h"
+#include "vc4_regs.h"
+
+struct vc4_ctm_state {
+ struct drm_private_state base;
+ struct drm_color_ctm *ctm;
+ int fifo;
+};
+
+static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
+{
+ return container_of(priv, struct vc4_ctm_state, base);
+}
+
+static struct vc4_ctm_state *vc4_get_ctm_state(struct drm_atomic_state *state,
+ struct drm_private_obj *manager)
+{
+ struct drm_device *dev = state->dev;
+ struct vc4_dev *vc4 = dev->dev_private;
+ struct drm_private_state *priv_state;
+ int ret;
+
+ ret = drm_modeset_lock(&vc4->ctm_state_lock, state->acquire_ctx);
+ if (ret)
+ return ERR_PTR(ret);
+
+ priv_state = drm_atomic_get_private_obj_state(state, manager);
+ if (IS_ERR(priv_state))
+ return ERR_CAST(priv_state);
+
+ return to_vc4_ctm_state(priv_state);
+}
+
+static struct drm_private_state *
+vc4_ctm_duplicate_state(struct drm_private_obj *obj)
+{
+ struct vc4_ctm_state *state;
+
+ state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return NULL;
+
+ __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+ return &state->base;
+}
+
+static void vc4_ctm_destroy_state(struct
[PATCH] drm/tegra: hub: Use state directly
Using drm_atomic_get_private_obj_state after state has been swapped
will return old state.
Fixes: 0281c4149021 ("drm/tegra: hub: Use private object for global state")
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/tegra/hub.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
index 9a3f23d4780f..bdd2cdd0745c 100644
--- a/drivers/gpu/drm/tegra/hub.c
+++ b/drivers/gpu/drm/tegra/hub.c
@@ -683,12 +683,11 @@ void tegra_display_hub_atomic_commit(struct drm_device
*drm,
{
struct tegra_drm *tegra = drm->dev_private;
struct tegra_display_hub *hub = tegra->hub;
- struct tegra_display_hub_state *hub_state;
+ struct tegra_display_hub_state *hub_state =
+ to_tegra_display_hub_state(hub->base.state);
struct device *dev = hub->client.dev;
int err;
- hub_state = tegra_display_hub_get_state(hub, state);
-
if (hub_state->clk) {
err = clk_set_rate(hub_state->clk, hub_state->rate);
if (err < 0)
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/4] drm/vc4: Expose gamma as atomic property
We are an atomic driver so the gamma LUT should also be exposed as a
CRTC property through the DRM atomic color management. This will also
take care of the legacy path for us.
Signed-off-by: Stefan Schake
---
v2: Use drm_color_lut_size for LUT length
v3: Disable gamma lut when gamma_lut is NULL (Eric)
drivers/gpu/drm/vc4/vc4_crtc.c | 37 ++---
1 file changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index bf46674..285f88d 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -298,23 +298,21 @@ vc4_crtc_lut_load(struct drm_crtc *crtc)
HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
}
-static int
-vc4_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b,
- uint32_t size,
- struct drm_modeset_acquire_ctx *ctx)
+static void
+vc4_crtc_update_gamma_lut(struct drm_crtc *crtc)
{
struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+ struct drm_color_lut *lut = crtc->state->gamma_lut->data;
+ u32 length = drm_color_lut_size(crtc->state->gamma_lut);
u32 i;
- for (i = 0; i < size; i++) {
- vc4_crtc->lut_r[i] = r[i] >> 8;
- vc4_crtc->lut_g[i] = g[i] >> 8;
- vc4_crtc->lut_b[i] = b[i] >> 8;
+ for (i = 0; i < length; i++) {
+ vc4_crtc->lut_r[i] = drm_color_lut_extract(lut[i].red, 8);
+ vc4_crtc->lut_g[i] = drm_color_lut_extract(lut[i].green, 8);
+ vc4_crtc->lut_b[i] = drm_color_lut_extract(lut[i].blue, 8);
}
vc4_crtc_lut_load(crtc);
-
- return 0;
}
static u32 vc4_get_fifo_full_level(u32 format)
@@ -699,6 +697,22 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
if (crtc->state->active && old_state->active)
vc4_crtc_update_dlist(crtc);
+ if (crtc->state->color_mgmt_changed) {
+ u32 dispbkgndx = HVS_READ(SCALER_DISPBKGNDX(vc4_crtc->channel));
+
+ if (crtc->state->gamma_lut) {
+ vc4_crtc_update_gamma_lut(crtc);
+ dispbkgndx |= SCALER_DISPBKGND_GAMMA;
+ } else {
+ /* Unsetting DISPBKGND_GAMMA skips the gamma lut step
+* in hardware, which is the same as a linear lut that
+* DRM expects us to use in absence of a user lut.
+*/
+ dispbkgndx &= ~SCALER_DISPBKGND_GAMMA;
+ }
+ HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel), dispbkgndx);
+ }
+
if (debug_dump_regs) {
DRM_INFO("CRTC %d HVS after:\n", drm_crtc_index(crtc));
vc4_hvs_dump_state(dev);
@@ -909,7 +923,7 @@ static const struct drm_crtc_funcs vc4_crtc_funcs = {
.reset = vc4_crtc_reset,
.atomic_duplicate_state = vc4_crtc_duplicate_state,
.atomic_destroy_state = vc4_crtc_destroy_state,
- .gamma_set = vc4_crtc_gamma_set,
+ .gamma_set = drm_atomic_helper_legacy_gamma_set,
.enable_vblank = vc4_enable_vblank,
.disable_vblank = vc4_disable_vblank,
};
@@ -1035,6 +1049,7 @@ static int vc4_crtc_bind(struct device *dev, struct
device *master, void *data)
primary_plane->crtc = crtc;
vc4_crtc->channel = vc4_crtc->data->hvs_channel;
drm_mode_crtc_set_gamma_size(crtc, ARRAY_SIZE(vc4_crtc->lut_r));
+ drm_crtc_enable_color_mgmt(crtc, 0, false, crtc->gamma_size);
/* Set up some arbitrary number of planes. We're not limited
* by a set number of physical registers, just the space in
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 0/4] drm/vc4: Atomic color management support
This series adds support for the gamma and CTM properties to VC4. The CTM support is somewhat limited in that we can only enable it for one CRTC at a time and coefficients are S0.9 in hardware. The latter seems good enough for the various color corrections Android offers. The CTM support in v3 is an entire rewrite from previous versions since I didn't previously model our CTM hardware as private atomic state, which is needed to correctly limit it to one CRTC. Since v2, Eric has also confirmed from the HDL that CTM in VC4 is applied before gamma lut, matching the documented behavior for the DRM property. Eric Anholt (1): drm/vc4: Add some missing HVS register definitions. Stefan Schake (3): drm/vc4: Expose gamma as atomic property drm/vc4: Move CRTC state to header drm/vc4: Add CTM support drivers/gpu/drm/vc4/vc4_crtc.c | 74 -- drivers/gpu/drm/vc4/vc4_drv.h | 36 + drivers/gpu/drm/vc4/vc4_kms.c | 167 - drivers/gpu/drm/vc4/vc4_regs.h | 96 +++ 4 files changed, 328 insertions(+), 45 deletions(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 3/4] drm/vc4: Move CRTC state to header
We need to access the channel for configuring our CTM hardware.
Signed-off-by: Stefan Schake
---
v3: New in the series
drivers/gpu/drm/vc4/vc4_crtc.c | 33 -
drivers/gpu/drm/vc4/vc4_drv.h | 33 +
2 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 285f88d..08fe8dd 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -42,51 +42,18 @@
#include "vc4_drv.h"
#include "vc4_regs.h"
-struct vc4_crtc {
- struct drm_crtc base;
- const struct vc4_crtc_data *data;
- void __iomem *regs;
-
- /* Timestamp at start of vblank irq - unaffected by lock delays. */
- ktime_t t_vblank;
-
- /* Which HVS channel we're using for our CRTC. */
- int channel;
-
- u8 lut_r[256];
- u8 lut_g[256];
- u8 lut_b[256];
- /* Size in pixels of the COB memory allocated to this CRTC. */
- u32 cob_size;
-
- struct drm_pending_vblank_event *event;
-};
-
struct vc4_crtc_state {
struct drm_crtc_state base;
/* Dlist area for this CRTC configuration. */
struct drm_mm_node mm;
};
-static inline struct vc4_crtc *
-to_vc4_crtc(struct drm_crtc *crtc)
-{
- return (struct vc4_crtc *)crtc;
-}
-
static inline struct vc4_crtc_state *
to_vc4_crtc_state(struct drm_crtc_state *crtc_state)
{
return (struct vc4_crtc_state *)crtc_state;
}
-struct vc4_crtc_data {
- /* Which channel of the HVS this pixelvalve sources from. */
- int hvs_channel;
-
- enum vc4_encoder_type encoder_types[4];
-};
-
#define CRTC_WRITE(offset, val) writel(val, vc4_crtc->regs + (offset))
#define CRTC_READ(offset) readl(vc4_crtc->regs + (offset))
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 1b4cd1f..4288615 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -392,6 +392,39 @@ to_vc4_encoder(struct drm_encoder *encoder)
return container_of(encoder, struct vc4_encoder, base);
}
+struct vc4_crtc_data {
+ /* Which channel of the HVS this pixelvalve sources from. */
+ int hvs_channel;
+
+ enum vc4_encoder_type encoder_types[4];
+};
+
+struct vc4_crtc {
+ struct drm_crtc base;
+ const struct vc4_crtc_data *data;
+ void __iomem *regs;
+
+ /* Timestamp at start of vblank irq - unaffected by lock delays. */
+ ktime_t t_vblank;
+
+ /* Which HVS channel we're using for our CRTC. */
+ int channel;
+
+ u8 lut_r[256];
+ u8 lut_g[256];
+ u8 lut_b[256];
+ /* Size in pixels of the COB memory allocated to this CRTC. */
+ u32 cob_size;
+
+ struct drm_pending_vblank_event *event;
+};
+
+static inline struct vc4_crtc *
+to_vc4_crtc(struct drm_crtc *crtc)
+{
+ return (struct vc4_crtc *)crtc;
+}
+
#define V3D_READ(offset) readl(vc4->v3d->regs + offset)
#define V3D_WRITE(offset, val) writel(val, vc4->v3d->regs + offset)
#define HVS_READ(offset) readl(vc4->hvs->regs + offset)
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 1/4] drm/vc4: Add some missing HVS register definitions.
From: Eric Anholt At least the RGBA expand field we should have been setting, because we aren't expanding correctly for 565 -> . Other registers are ones that may be interesting for various projects that have been discussed. Signed-off-by: Eric Anholt Cc: Stefan Schake Acked-by: Stefan Schake --- drivers/gpu/drm/vc4/vc4_regs.h | 96 ++ 1 file changed, 96 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h index a141496..4af3e29 100644 --- a/drivers/gpu/drm/vc4/vc4_regs.h +++ b/drivers/gpu/drm/vc4/vc4_regs.h @@ -330,6 +330,21 @@ #define SCALER_DISPCTRL00x0040 # define SCALER_DISPCTRLX_ENABLE BIT(31) # define SCALER_DISPCTRLX_RESETBIT(30) +/* Generates a single frame when VSTART is seen and stops at the last + * pixel read from the FIFO. + */ +# define SCALER_DISPCTRLX_ONESHOT BIT(29) +/* Processes a single context in the dlist and then task switch, + * instead of an entire line. + */ +# define SCALER_DISPCTRLX_ONECTX BIT(28) +/* Set to have DISPSLAVE return 2 16bpp pixels and no status data. */ +# define SCALER_DISPCTRLX_FIFO32 BIT(27) +/* Turns on output to the DISPSLAVE register instead of the normal + * FIFO. + */ +# define SCALER_DISPCTRLX_FIFOREG BIT(26) + # define SCALER_DISPCTRLX_WIDTH_MASK VC4_MASK(23, 12) # define SCALER_DISPCTRLX_WIDTH_SHIFT 12 # define SCALER_DISPCTRLX_HEIGHT_MASK VC4_MASK(11, 0) @@ -402,6 +417,68 @@ */ # define SCALER_GAMADDR_SRAMENBBIT(30) +#define SCALER_OLEDOFFS 0x0080 +/* Clamps R to [16,235] and G/B to [16,240]. */ +# define SCALER_OLEDOFFS_YUVCLAMP BIT(31) + +/* Chooses which display FIFO the matrix applies to. */ +# define SCALER_OLEDOFFS_DISPFIFO_MASK VC4_MASK(25, 24) +# define SCALER_OLEDOFFS_DISPFIFO_SHIFT 24 +# define SCALER_OLEDOFFS_DISPFIFO_DISABLED 0 +# define SCALER_OLEDOFFS_DISPFIFO_0 1 +# define SCALER_OLEDOFFS_DISPFIFO_1 2 +# define SCALER_OLEDOFFS_DISPFIFO_2 3 + +/* Offsets are 8-bit 2s-complement. */ +# define SCALER_OLEDOFFS_RED_MASK VC4_MASK(23, 16) +# define SCALER_OLEDOFFS_RED_SHIFT 16 +# define SCALER_OLEDOFFS_GREEN_MASK VC4_MASK(15, 8) +# define SCALER_OLEDOFFS_GREEN_SHIFT8 +# define SCALER_OLEDOFFS_BLUE_MASK VC4_MASK(7, 0) +# define SCALER_OLEDOFFS_BLUE_SHIFT 0 + +/* The coefficients are S0.9 fractions. */ +#define SCALER_OLEDCOEF00x0084 +# define SCALER_OLEDCOEF0_B_TO_R_MASK VC4_MASK(29, 20) +# define SCALER_OLEDCOEF0_B_TO_R_SHIFT 20 +# define SCALER_OLEDCOEF0_B_TO_G_MASK VC4_MASK(19, 10) +# define SCALER_OLEDCOEF0_B_TO_G_SHIFT 10 +# define SCALER_OLEDCOEF0_B_TO_B_MASK VC4_MASK(9, 0) +# define SCALER_OLEDCOEF0_B_TO_B_SHIFT 0 + +#define SCALER_OLEDCOEF10x0088 +# define SCALER_OLEDCOEF1_G_TO_R_MASK VC4_MASK(29, 20) +# define SCALER_OLEDCOEF1_G_TO_R_SHIFT 20 +# define SCALER_OLEDCOEF1_G_TO_G_MASK VC4_MASK(19, 10) +# define SCALER_OLEDCOEF1_G_TO_G_SHIFT 10 +# define SCALER_OLEDCOEF1_G_TO_B_MASK VC4_MASK(9, 0) +# define SCALER_OLEDCOEF1_G_TO_B_SHIFT 0 + +#define SCALER_OLEDCOEF20x008c +# define SCALER_OLEDCOEF2_R_TO_R_MASK VC4_MASK(29, 20) +# define SCALER_OLEDCOEF2_R_TO_R_SHIFT 20 +# define SCALER_OLEDCOEF2_R_TO_G_MASK VC4_MASK(19, 10) +# define SCALER_OLEDCOEF2_R_TO_G_SHIFT 10 +# define SCALER_OLEDCOEF2_R_TO_B_MASK VC4_MASK(9, 0) +# define SCALER_OLEDCOEF2_R_TO_B_SHIFT 0 + +/* Slave addresses for DMAing from HVS composition output to other + * devices. The top bits are valid only in !FIFO32 mode. + */ +#define SCALER_DISPSLAVE0 0x00c0 +#define SCALER_DISPSLAVE1 0x00c9 +#define SCALER_DISPSLAVE2 0x00d0 +# define SCALER_DISPSLAVE_ISSUE_VSTART BIT(31) +# define SCALER_DISPSLAVE_ISSUE_HSTART BIT(30) +/* Set when the current line has been read and an HSTART is required. */ +# define SCALER_DISPSLAVE_EOL BIT(26) +/* Set when the display FIFO is empty. */ +# define SCALER_DISPSLAVE_EMPTY BIT(25) +/* Set when there is RGB data ready to read. */ +# define SCALER_DISPSLAVE_VALID BIT(24) +# define SCALER_DISPSLAVE_RGB_MASK VC4_MASK(23, 0) +# define SCALER_DISPSLAVE_RGB_SHIFT 0 + #define SCALER_GAMDATA 0x00e0 #define SCALER_DLIST_START 0x2000 #define SCALER_DLIST_SIZE 0x4000 @@ -767,6 +844,10 @@ enum hvs_pixe
[PATCH v3 4/4] drm/vc4: Add CTM support
The hardware has a single block for applying a CTM prior to gamma lut.
It can be fed with pixels from one of our CRTC at a time and uses a
matrix with S0.9 scalars. Use private atomic state to reject attempts
from userland to apply CTM for more than one CRTC at a time and reject
matrices with scalars that we can't approximate without integer bits.
Signed-off-by: Stefan Schake
---
v3: New in the series
drivers/gpu/drm/vc4/vc4_crtc.c | 6 +-
drivers/gpu/drm/vc4/vc4_drv.h | 3 +
drivers/gpu/drm/vc4/vc4_kms.c | 167 -
3 files changed, 174 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 08fe8dd..32bdf13 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -1016,7 +1016,11 @@ static int vc4_crtc_bind(struct device *dev, struct
device *master, void *data)
primary_plane->crtc = crtc;
vc4_crtc->channel = vc4_crtc->data->hvs_channel;
drm_mode_crtc_set_gamma_size(crtc, ARRAY_SIZE(vc4_crtc->lut_r));
- drm_crtc_enable_color_mgmt(crtc, 0, false, crtc->gamma_size);
+
+ /* We support CTM, but only for one CRTC at a time. It's therefore
+* implemented as private driver state in vc4_kms, not here.
+*/
+ drm_crtc_enable_color_mgmt(crtc, 0, true, crtc->gamma_size);
/* Set up some arbitrary number of planes. We're not limited
* by a set number of physical registers, just the space in
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index 4288615..ee22a5b 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include "uapi/drm/vc4_drm.h"
@@ -193,6 +194,8 @@ struct vc4_dev {
} hangcheck;
struct semaphore async_modeset;
+
+ struct drm_private_obj ctm_manager;
};
static inline struct vc4_dev *
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index ba60153..f2eca4d 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -23,6 +23,102 @@
#include
#include
#include "vc4_drv.h"
+#include "vc4_regs.h"
+
+struct vc4_ctm_state {
+ struct drm_private_state base;
+ struct drm_color_ctm *ctm;
+ int fifo;
+};
+
+static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
+{
+ return container_of(priv, struct vc4_ctm_state, base);
+}
+
+static struct vc4_ctm_state *vc4_get_ctm_state(struct drm_atomic_state *state,
+ struct drm_private_obj *manager)
+{
+ return to_vc4_ctm_state(drm_atomic_get_private_obj_state(state,
manager));
+}
+
+static struct drm_private_state *
+vc4_ctm_duplicate_state(struct drm_private_obj *obj)
+{
+ struct vc4_ctm_state *state;
+
+ state = kmemdup(obj->state, sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return NULL;
+
+ __drm_atomic_helper_private_obj_duplicate_state(obj, &state->base);
+
+ return &state->base;
+}
+
+static void vc4_ctm_destroy_state(struct drm_private_obj *obj,
+ struct drm_private_state *state)
+{
+ struct vc4_ctm_state *ctm_state = to_vc4_ctm_state(state);
+
+ kfree(ctm_state);
+}
+
+static const struct drm_private_state_funcs vc4_ctm_state_funcs = {
+ .atomic_duplicate_state = vc4_ctm_duplicate_state,
+ .atomic_destroy_state = vc4_ctm_destroy_state,
+};
+
+/* Converts a DRM S31.32 value to the HW S0.9 format. */
+static u16 vc4_ctm_s31_32_to_s0_9(u64 in)
+{
+ u16 r;
+
+ /* Sign bit. */
+ r = in & BIT_ULL(63) ? BIT(9) : 0;
+ /* We have zero integer bits so we can only saturate here. */
+ if ((in & GENMASK_ULL(62, 32)) > 0)
+ r |= GENMASK(8, 0);
+ /* Otherwise take the 9 most important fractional bits. */
+ else
+ r |= (in >> 22) & GENMASK(8, 0);
+ return r;
+}
+
+static void
+vc4_ctm_commit(struct vc4_dev *vc4, struct drm_atomic_state *state)
+{
+ struct vc4_ctm_state *ctm_state = vc4_get_ctm_state(state,
+ &vc4->ctm_manager);
+ struct drm_color_ctm *ctm = ctm_state->ctm;
+
+ if (ctm_state->fifo) {
+ HVS_WRITE(SCALER_OLEDCOEF2,
+ VC4_SET_FIELD(vc4_ctm_s31_32_to_s0_9(ctm->matrix[0]),
+ SCALER_OLEDCOEF2_R_TO_R) |
+ VC4_SET_FIELD(vc4_ctm_s31_32_to_s0_9(ctm->matrix[3]),
+ SCALER_OLEDCOEF2_R_TO_G) |
+ VC4_SET_FIELD(vc4_ctm_s31_32_to_s0_9(ctm->matrix[6]),
+ SCALER_OLEDCOEF2_R_TO
Re: [PATCH] drm/vc4: Add some missing HVS register definitions.
On Sat, Mar 3, 2018 at 12:03 AM, Eric Anholt wrote: > At least the RGBA expand field we should have been setting, because we > aren't expanding correctly for 565 -> . Other registers are ones > that may be interesting for various projects that have been discussed. > > Signed-off-by: Eric Anholt > Cc: Stefan Schake > --- > drivers/gpu/drm/vc4/vc4_regs.h | 96 > ++ > 1 file changed, 96 insertions(+) > > diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h > index b9749cb24063..ce8bb7486456 100644 > --- a/drivers/gpu/drm/vc4/vc4_regs.h > +++ b/drivers/gpu/drm/vc4/vc4_regs.h > @@ -330,6 +330,21 @@ > #define SCALER_DISPCTRL00x0040 > # define SCALER_DISPCTRLX_ENABLE BIT(31) > # define SCALER_DISPCTRLX_RESETBIT(30) > +/* Generates a single frame when VSTART is seen and stops at the last > + * pixel read from the FIFO. > + */ > +# define SCALER_DISPCTRLX_ONESHOT BIT(29) > +/* Processes a single context in the dlist and then task switch, > + * instead of an entire line. > + */ > +# define SCALER_DISPCTRLX_ONECTX BIT(28) > +/* Set to have DISPSLAVE return 2 16bpp pixels and no status data. */ > +# define SCALER_DISPCTRLX_FIFO32 BIT(27) > +/* Turns on output to the DISPSLAVE register instead of the normal > + * FIFO. > + */ > +# define SCALER_DISPCTRLX_FIFOREG BIT(26) > + > # define SCALER_DISPCTRLX_WIDTH_MASK VC4_MASK(23, 12) > # define SCALER_DISPCTRLX_WIDTH_SHIFT 12 > # define SCALER_DISPCTRLX_HEIGHT_MASK VC4_MASK(11, 0) > @@ -402,6 +417,68 @@ > */ > # define SCALER_GAMADDR_SRAMENBBIT(30) > > +#define SCALER_OLEDOFFS 0x0080 > +/* Clamps R to [16,235] and G/B to [16,240]. */ > +# define SCALER_OLEDOFFS_YUVCLAMP BIT(31) > + > +/* Chooses which display FIFO the matrix applies to. */ > +# define SCALER_OLEDOFFS_DISPFIFO_MASK VC4_MASK(25, 24) > +# define SCALER_OLEDOFFS_DISPFIFO_SHIFT 24 > +# define SCALER_OLEDOFFS_DISPFIFO_DISABLED 0 > +# define SCALER_OLEDOFFS_DISPFIFO_0 1 > +# define SCALER_OLEDOFFS_DISPFIFO_1 2 > +# define SCALER_OLEDOFFS_DISPFIFO_2 3 > + > +/* Offsets are 8-bit 2s-complement. */ > +# define SCALER_OLEDOFFS_RED_MASK VC4_MASK(23, 16) > +# define SCALER_OLEDOFFS_RED_SHIFT 16 > +# define SCALER_OLEDOFFS_GREEN_MASK VC4_MASK(15, 8) > +# define SCALER_OLEDOFFS_GREEN_SHIFT8 > +# define SCALER_OLEDOFFS_BLUE_MASK VC4_MASK(7, 0) > +# define SCALER_OLEDOFFS_BLUE_SHIFT 0 > + > +/* The coefficients are S0.9 fractions. */ > +#define SCALER_OLEDCOEF00x0084 > +# define SCALER_OLEDCOEF0_B_TO_R_MASK VC4_MASK(29, 20) > +# define SCALER_OLEDCOEF0_B_TO_R_SHIFT 20 > +# define SCALER_OLEDCOEF0_B_TO_G_MASK VC4_MASK(19, 10) > +# define SCALER_OLEDCOEF0_B_TO_G_SHIFT 10 > +# define SCALER_OLEDCOEF0_B_TO_B_MASK VC4_MASK(9, 0) > +# define SCALER_OLEDCOEF0_B_TO_B_SHIFT 0 > + > +#define SCALER_OLEDCOEF10x0088 > +# define SCALER_OLEDCOEF1_G_TO_R_MASK VC4_MASK(29, 20) > +# define SCALER_OLEDCOEF1_G_TO_R_SHIFT 20 > +# define SCALER_OLEDCOEF1_G_TO_G_MASK VC4_MASK(19, 10) > +# define SCALER_OLEDCOEF1_G_TO_G_SHIFT 10 > +# define SCALER_OLEDCOEF1_G_TO_B_MASK VC4_MASK(9, 0) > +# define SCALER_OLEDCOEF1_G_TO_B_SHIFT 0 > + > +#define SCALER_OLEDCOEF20x008c > +# define SCALER_OLEDCOEF2_R_TO_R_MASK VC4_MASK(29, 20) > +# define SCALER_OLEDCOEF2_R_TO_R_SHIFT 20 > +# define SCALER_OLEDCOEF2_R_TO_G_MASK VC4_MASK(19, 10) > +# define SCALER_OLEDCOEF2_R_TO_G_SHIFT 10 > +# define SCALER_OLEDCOEF2_R_TO_B_MASK VC4_MASK(9, 0) > +# define SCALER_OLEDCOEF2_R_TO_B_SHIFT 0 > + > +/* Slave addresses for DMAing from HVS composition output to other > + * devices. The top bits are valid only in !FIFO32 mode. > + */ > +#define SCALER_DISPSLAVE0 0x00c0 > +#define SCALER_DISPSLAVE1 0x00c9 > +#define SCALER_DISPSLAVE2 0x00d0 > +# define SCALER_DISPSLAVE_ISSUE_VSTART BIT(31) > +# define SCALER_DISPSLAVE_ISSUE_HSTART BIT(30) > +/* Set when the current line has been read and an HSTART is required. */ > +# define SCALER_DISPSLAVE_EOL BIT(26) > +/* Set when the display FIFO is
Re: [PATCH v2 4/4] drm/vc4: Restrict active CTM to one CRTC
Hey Daniel,
On Sun, Mar 25, 2018 at 10:01 AM, Daniel Stone wrote:
> Hi Stefan,
>
> On 25 March 2018 at 02:52, Stefan Schake wrote:
>> +static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4)
>> +{
>> + return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS),
>> +SCALER_OLEDOFFS_DISPFIFO);
>> +}
>
> This needs to be managed as a global resource through atomic state
> objects, rather than checking the current hardware state.
Do you mean as a property or some such that is accessible to userland
or merely that this could be raced?
I haven't had much luck finding examples for resources shared between CRTCs
in the current tree. My understanding here was that if userland commits
on CRTC B after a check-only on A, we are no longer bound by the earlier
result for the check-only. Otherwise, I would have to already commit my
CTM block to one CRTC at check (possibly check only) time.
Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/3] drm/vc4: Expose gamma as atomic property
We are an atomic driver so the gamma LUT should also be exposed as a
CRTC property through the DRM atomic color management. This will also
take care of the legacy path for us.
Signed-off-by: Stefan Schake
---
v2: Use drm_color_lut_size for LUT length
drivers/gpu/drm/vc4/vc4_crtc.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index bf4667481935..239215cb3274 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -298,23 +298,21 @@ vc4_crtc_lut_load(struct drm_crtc *crtc)
HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
}
-static int
-vc4_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b,
- uint32_t size,
- struct drm_modeset_acquire_ctx *ctx)
+static void
+vc4_crtc_update_gamma_lut(struct drm_crtc *crtc)
{
struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+ struct drm_color_lut *lut = crtc->state->gamma_lut->data;
+ u32 length = drm_color_lut_size(crtc->state->gamma_lut);
u32 i;
- for (i = 0; i < size; i++) {
- vc4_crtc->lut_r[i] = r[i] >> 8;
- vc4_crtc->lut_g[i] = g[i] >> 8;
- vc4_crtc->lut_b[i] = b[i] >> 8;
+ for (i = 0; i < length; i++) {
+ vc4_crtc->lut_r[i] = drm_color_lut_extract(lut[i].red, 8);
+ vc4_crtc->lut_g[i] = drm_color_lut_extract(lut[i].green, 8);
+ vc4_crtc->lut_b[i] = drm_color_lut_extract(lut[i].blue, 8);
}
vc4_crtc_lut_load(crtc);
-
- return 0;
}
static u32 vc4_get_fifo_full_level(u32 format)
@@ -699,6 +697,9 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
if (crtc->state->active && old_state->active)
vc4_crtc_update_dlist(crtc);
+ if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
+ vc4_crtc_update_gamma_lut(crtc);
+
if (debug_dump_regs) {
DRM_INFO("CRTC %d HVS after:\n", drm_crtc_index(crtc));
vc4_hvs_dump_state(dev);
@@ -909,7 +910,7 @@ static const struct drm_crtc_funcs vc4_crtc_funcs = {
.reset = vc4_crtc_reset,
.atomic_duplicate_state = vc4_crtc_duplicate_state,
.atomic_destroy_state = vc4_crtc_destroy_state,
- .gamma_set = vc4_crtc_gamma_set,
+ .gamma_set = drm_atomic_helper_legacy_gamma_set,
.enable_vblank = vc4_enable_vblank,
.disable_vblank = vc4_disable_vblank,
};
@@ -1035,6 +1036,7 @@ static int vc4_crtc_bind(struct device *dev, struct
device *master, void *data)
primary_plane->crtc = crtc;
vc4_crtc->channel = vc4_crtc->data->hvs_channel;
drm_mode_crtc_set_gamma_size(crtc, ARRAY_SIZE(vc4_crtc->lut_r));
+ drm_crtc_enable_color_mgmt(crtc, 0, false, crtc->gamma_size);
/* Set up some arbitrary number of planes. We're not limited
* by a set number of physical registers, just the space in
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/4] drm/vc4: Add color transformation matrix (CTM) support
The hardware supports a CTM with S0.9 values. We therefore only allow
a value of 1.0 or fractional only and reject all others with integer
parts. This restriction is mostly inconsequential in practice since
commonly used transformation matrices have all scalars <= 1.0.
Signed-off-by: Stefan Schake
---
v2: Simplify CTM atomic check (Ville)
drivers/gpu/drm/vc4/vc4_crtc.c | 97 --
1 file changed, 94 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 8d71098d00c4..bafb0102fe1d 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -315,6 +315,79 @@ vc4_crtc_update_gamma_lut(struct drm_crtc *crtc)
vc4_crtc_lut_load(crtc);
}
+/* Converts a DRM S31.32 value to the HW S0.9 format. */
+static u16 vc4_crtc_s31_32_to_s0_9(u64 in)
+{
+ u16 r;
+
+ /* Sign bit. */
+ r = in & BIT_ULL(63) ? BIT(9) : 0;
+ /* We have zero integer bits so we can only saturate here. */
+ if ((in & GENMASK_ULL(62, 32)) > 0)
+ r |= GENMASK(8, 0);
+ /* Otherwise take the 9 most important fractional bits. */
+ else
+ r |= (in >> 22) & GENMASK(8, 0);
+ return r;
+}
+
+static void
+vc4_crtc_update_ctm(struct drm_crtc *crtc)
+{
+ struct drm_device *dev = crtc->dev;
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+ struct drm_color_ctm *ctm = crtc->state->ctm->data;
+
+ HVS_WRITE(SCALER_OLEDCOEF2,
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[0]),
+ SCALER_OLEDCOEF2_R_TO_R) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[3]),
+ SCALER_OLEDCOEF2_R_TO_G) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[6]),
+ SCALER_OLEDCOEF2_R_TO_B));
+ HVS_WRITE(SCALER_OLEDCOEF1,
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[1]),
+ SCALER_OLEDCOEF1_G_TO_R) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[4]),
+ SCALER_OLEDCOEF1_G_TO_G) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[7]),
+ SCALER_OLEDCOEF1_G_TO_B));
+ HVS_WRITE(SCALER_OLEDCOEF0,
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[2]),
+ SCALER_OLEDCOEF0_B_TO_R) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[5]),
+ SCALER_OLEDCOEF0_B_TO_G) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[8]),
+ SCALER_OLEDCOEF0_B_TO_B));
+
+ /* Channel is 0-based but for DISPFIFO, 0 means disabled. */
+ HVS_WRITE(SCALER_OLEDOFFS, VC4_SET_FIELD(vc4_crtc->channel + 1,
+SCALER_OLEDOFFS_DISPFIFO));
+}
+
+/* Check if the CTM contains valid input.
+ *
+ * DRM exposes CTM with S31.32 scalars, but the HW only supports S0.9.
+ * We don't allow integer values >1, and 1 only without fractional part
+ * to handle the common 1.0 value.
+ */
+static int vc4_crtc_atomic_check_ctm(struct drm_crtc_state *state)
+{
+ struct drm_color_ctm *ctm = state->ctm->data;
+ u32 i;
+
+ for (i = 0; i < ARRAY_SIZE(ctm->matrix); i++) {
+ u64 val = ctm->matrix[i];
+
+ val &= ~BIT_ULL(63);
+ if (val > BIT_ULL(32))
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static u32 vc4_get_fifo_full_level(u32 format)
{
static const u32 fifo_len_bytes = 64;
@@ -621,6 +694,15 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
if (hweight32(state->connector_mask) > 1)
return -EINVAL;
+ if (state->ctm) {
+ /* The CTM hardware has no integer bits, so we check
+* and reject scalars >1.0 that we have no chance of
+* approximating.
+*/
+ if (vc4_crtc_atomic_check_ctm(state))
+ return -EINVAL;
+ }
+
drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state)
dlist_count += vc4_plane_dlist_size(plane_state);
@@ -697,8 +779,17 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
if (crtc->state->active && old_state->active)
vc4_crtc_update_dlist(crtc);
- if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
- vc4_crtc_update_gamma_lut(crtc);
+ if (crtc->state->color_mgmt_changed) {
+ if (crtc->state->gamma_lut)
+ vc4_crtc_upda
[PATCH v2 1/4] drm/vc4: Add some missing HVS register definitions.
From: Eric Anholt At least the RGBA expand field we should have been setting, because we aren't expanding correctly for 565 -> . Other registers are ones that may be interesting for various projects that have been discussed. Signed-off-by: Eric Anholt Cc: Stefan Schake --- v2: New in the series. Included here to keep kbuild robot happy and give the full picture. drivers/gpu/drm/vc4/vc4_regs.h | 96 ++ 1 file changed, 96 insertions(+) diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h index a141496104a6..4af3e29d076a 100644 --- a/drivers/gpu/drm/vc4/vc4_regs.h +++ b/drivers/gpu/drm/vc4/vc4_regs.h @@ -330,6 +330,21 @@ #define SCALER_DISPCTRL00x0040 # define SCALER_DISPCTRLX_ENABLE BIT(31) # define SCALER_DISPCTRLX_RESETBIT(30) +/* Generates a single frame when VSTART is seen and stops at the last + * pixel read from the FIFO. + */ +# define SCALER_DISPCTRLX_ONESHOT BIT(29) +/* Processes a single context in the dlist and then task switch, + * instead of an entire line. + */ +# define SCALER_DISPCTRLX_ONECTX BIT(28) +/* Set to have DISPSLAVE return 2 16bpp pixels and no status data. */ +# define SCALER_DISPCTRLX_FIFO32 BIT(27) +/* Turns on output to the DISPSLAVE register instead of the normal + * FIFO. + */ +# define SCALER_DISPCTRLX_FIFOREG BIT(26) + # define SCALER_DISPCTRLX_WIDTH_MASK VC4_MASK(23, 12) # define SCALER_DISPCTRLX_WIDTH_SHIFT 12 # define SCALER_DISPCTRLX_HEIGHT_MASK VC4_MASK(11, 0) @@ -402,6 +417,68 @@ */ # define SCALER_GAMADDR_SRAMENBBIT(30) +#define SCALER_OLEDOFFS 0x0080 +/* Clamps R to [16,235] and G/B to [16,240]. */ +# define SCALER_OLEDOFFS_YUVCLAMP BIT(31) + +/* Chooses which display FIFO the matrix applies to. */ +# define SCALER_OLEDOFFS_DISPFIFO_MASK VC4_MASK(25, 24) +# define SCALER_OLEDOFFS_DISPFIFO_SHIFT 24 +# define SCALER_OLEDOFFS_DISPFIFO_DISABLED 0 +# define SCALER_OLEDOFFS_DISPFIFO_0 1 +# define SCALER_OLEDOFFS_DISPFIFO_1 2 +# define SCALER_OLEDOFFS_DISPFIFO_2 3 + +/* Offsets are 8-bit 2s-complement. */ +# define SCALER_OLEDOFFS_RED_MASK VC4_MASK(23, 16) +# define SCALER_OLEDOFFS_RED_SHIFT 16 +# define SCALER_OLEDOFFS_GREEN_MASK VC4_MASK(15, 8) +# define SCALER_OLEDOFFS_GREEN_SHIFT8 +# define SCALER_OLEDOFFS_BLUE_MASK VC4_MASK(7, 0) +# define SCALER_OLEDOFFS_BLUE_SHIFT 0 + +/* The coefficients are S0.9 fractions. */ +#define SCALER_OLEDCOEF00x0084 +# define SCALER_OLEDCOEF0_B_TO_R_MASK VC4_MASK(29, 20) +# define SCALER_OLEDCOEF0_B_TO_R_SHIFT 20 +# define SCALER_OLEDCOEF0_B_TO_G_MASK VC4_MASK(19, 10) +# define SCALER_OLEDCOEF0_B_TO_G_SHIFT 10 +# define SCALER_OLEDCOEF0_B_TO_B_MASK VC4_MASK(9, 0) +# define SCALER_OLEDCOEF0_B_TO_B_SHIFT 0 + +#define SCALER_OLEDCOEF10x0088 +# define SCALER_OLEDCOEF1_G_TO_R_MASK VC4_MASK(29, 20) +# define SCALER_OLEDCOEF1_G_TO_R_SHIFT 20 +# define SCALER_OLEDCOEF1_G_TO_G_MASK VC4_MASK(19, 10) +# define SCALER_OLEDCOEF1_G_TO_G_SHIFT 10 +# define SCALER_OLEDCOEF1_G_TO_B_MASK VC4_MASK(9, 0) +# define SCALER_OLEDCOEF1_G_TO_B_SHIFT 0 + +#define SCALER_OLEDCOEF20x008c +# define SCALER_OLEDCOEF2_R_TO_R_MASK VC4_MASK(29, 20) +# define SCALER_OLEDCOEF2_R_TO_R_SHIFT 20 +# define SCALER_OLEDCOEF2_R_TO_G_MASK VC4_MASK(19, 10) +# define SCALER_OLEDCOEF2_R_TO_G_SHIFT 10 +# define SCALER_OLEDCOEF2_R_TO_B_MASK VC4_MASK(9, 0) +# define SCALER_OLEDCOEF2_R_TO_B_SHIFT 0 + +/* Slave addresses for DMAing from HVS composition output to other + * devices. The top bits are valid only in !FIFO32 mode. + */ +#define SCALER_DISPSLAVE0 0x00c0 +#define SCALER_DISPSLAVE1 0x00c9 +#define SCALER_DISPSLAVE2 0x00d0 +# define SCALER_DISPSLAVE_ISSUE_VSTART BIT(31) +# define SCALER_DISPSLAVE_ISSUE_HSTART BIT(30) +/* Set when the current line has been read and an HSTART is required. */ +# define SCALER_DISPSLAVE_EOL BIT(26) +/* Set when the display FIFO is empty. */ +# define SCALER_DISPSLAVE_EMPTY BIT(25) +/* Set when there is RGB data ready to read. */ +# define SCALER_DISPSLAVE_VALID BIT(24) +# define SCALER_DISPSLAVE_RGB_MASK VC4_MASK(23, 0) +# define SCALER_DISPSLAVE_RGB_SHIFT 0 + #define SCALER_GAMDATA 0x00e0 #define SCALER_DLIST_START 0x2000 #define SCALER_DL
[PATCH v2 4/4] drm/vc4: Restrict active CTM to one CRTC
We only have one hardware block to do the CTM and need to reject
attempts to enable it for multiple CRTCs simultaneously.
Signed-off-by: Stefan Schake
---
v2: No change
drivers/gpu/drm/vc4/vc4_crtc.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index bafb0102fe1d..180b93ec447e 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -676,10 +676,17 @@ static enum drm_mode_status vc4_crtc_mode_valid(struct
drm_crtc *crtc,
return MODE_OK;
}
+static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4)
+{
+ return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS),
+SCALER_OLEDOFFS_DISPFIFO);
+}
+
static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)
{
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
+ struct drm_crtc_state *old_state = crtc->state;
struct drm_device *dev = crtc->dev;
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct drm_plane *plane;
@@ -701,6 +708,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
*/
if (vc4_crtc_atomic_check_ctm(state))
return -EINVAL;
+
+ /* We can only enable CTM for one fifo or CRTC at a time */
+ if (!old_state->ctm && vc4_crtc_get_ctm_fifo(vc4))
+ return -EINVAL;
}
drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state)
--
2.14.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/4] drm/vc4: Atomic color management support
This series adds support for the gamma and CTM properties to VC4. I've sent the necessary register defs as part of the series this time around to give the full picture. The CTM support is somewhat limited in that we can only enable it for one CRTC at a time and coefficients are S0.9 in hardware. The latter seems good enough for the various color corrections Android offers. Eric Anholt (1): drm/vc4: Add some missing HVS register definitions. Stefan Schake (3): drm/vc4: Expose gamma as atomic property drm/vc4: Add color transformation matrix (CTM) support drm/vc4: Restrict active CTM to one CRTC drivers/gpu/drm/vc4/vc4_crtc.c | 122 ++--- drivers/gpu/drm/vc4/vc4_regs.h | 96 2 files changed, 209 insertions(+), 9 deletions(-) -- 2.14.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] libdrm: intel/Android.mk: Filter libdrm_intel library requirements on x86/x86_64
On Tue, Mar 20, 2018 at 2:49 AM, John Stultz wrote: > On Tue, Mar 20, 2018 at 6:55 AM, Stefan Schake wrote: >> Hey John, >> >> On Wed, Mar 14, 2018 at 5:47 PM, John Stultz wrote: >>> When building AOSP after updating libdrm project to the >>> freedesktop/master branch, I've seen the following build errors: >>> >>> external/libdrm/intel/Android.mk: error: libdrm_intel >>> (SHARED_LIBRARIES android-arm64) missing libpciaccess >>> (SHARED_LIBRARIES android-arm64) You can set >>> ALLOW_MISSING_DEPENDENCIES=true in your environment if this is >>> intentional, but that may defer real problems until later in the >>> build. >>> >>> Using ALLOW_MISSING_DEPENDENCIES=true when building allows >>> things to function properly, but is not ideal. >>> >>> So basically, while I'm not including the libdrm_intel package >>> into the build, just the fact that the Android.mk file references >>> libpciaccess which isn't a repo included in AOSP causes the build >>> failure. >>> >>> So it seems we need some sort of conditional filter in the >>> Android.mk to skip over it if we're not building for intel. >> >> I'm afraid this change has snowballed straight into the mesa build where >> it's now missing dependencies for i915_dri: >> >> external/mesa3d/src/mesa/drivers/dri/i915/Android.mk: error: >> i915_dri (SHARED_LIBRARIES android-arm) missing libdrm_intel >> >> Maybe that one needs the BOARD_GPU_DRIVERS treatment instead.. > > So tinkering here, it seems to me just changing the conditionalizing > to skipping over just the libpciaccess addition to > LOCAL_SHARED_LIBRARIES might be a simpler solution. > > Or would you see that as too ugly? I guess my real problem is with libpciaccess itself. Okay, we can't ding it for not being in AOSP, plenty of upstream Android isn't. But then the freedesktop/upstream repo of libpciaccess doesn't support Android build, the copies I found on Android-x86 and intel-ia are outdated and the latter doesn't seem to have it in its manifest. Beyond that, the only reference is in intel_bufmgr that if I understood the other discussion correctly is no longer used in Mesa? But that's mostly ranting, I don't even have any Intel hardware I use for Android. So I guess in the spirit of achieving the original goal of making that annoying error go away, ignoring only libpciaccess is just as well. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] libdrm: intel/Android.mk: Filter libdrm_intel library requirements on x86/x86_64
Hey John, On Wed, Mar 14, 2018 at 5:47 PM, John Stultz wrote: > When building AOSP after updating libdrm project to the > freedesktop/master branch, I've seen the following build errors: > > external/libdrm/intel/Android.mk: error: libdrm_intel > (SHARED_LIBRARIES android-arm64) missing libpciaccess > (SHARED_LIBRARIES android-arm64) You can set > ALLOW_MISSING_DEPENDENCIES=true in your environment if this is > intentional, but that may defer real problems until later in the > build. > > Using ALLOW_MISSING_DEPENDENCIES=true when building allows > things to function properly, but is not ideal. > > So basically, while I'm not including the libdrm_intel package > into the build, just the fact that the Android.mk file references > libpciaccess which isn't a repo included in AOSP causes the build > failure. > > So it seems we need some sort of conditional filter in the > Android.mk to skip over it if we're not building for intel. I'm afraid this change has snowballed straight into the mesa build where it's now missing dependencies for i915_dri: external/mesa3d/src/mesa/drivers/dri/i915/Android.mk: error: i915_dri (SHARED_LIBRARIES android-arm) missing libdrm_intel Maybe that one needs the BOARD_GPU_DRIVERS treatment instead.. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH] drm_hwcomposer: Rework platformdrmgeneric.cpp to use libdrm's gralloc handle
Hey John, On Fri, Mar 16, 2018 at 10:48 PM, John Stultz wrote: > LOCAL_C_INCLUDES := \ > - external/drm_gralloc \ > + external/libdrm/android \ > system/core/libsync This shouldn't be necessary if libdrm correctly exports its headers, but it seems that it only did for the static variant of the library. I've sent a patch to fix that, then the explicit path here can be dropped. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH libdrm] android: Add missing include exports
They were set for the static library but not the shared variant. Cc: John Stultz Signed-off-by: Stefan Schake --- Android.mk | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Android.mk b/Android.mk index 8611c5e..1b77c53 100644 --- a/Android.mk +++ b/Android.mk @@ -53,7 +53,9 @@ LOCAL_MODULE := libdrm LOCAL_SRC_FILES := $(LIBDRM_FILES) LOCAL_EXPORT_C_INCLUDE_DIRS := \ -$(LOCAL_PATH)/include/drm + $(LOCAL_PATH) \ + $(LOCAL_PATH)/include/drm \ + $(LOCAL_PATH)/android LOCAL_SHARED_LIBRARIES := \ libcutils -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC][PATCH] libdrm: gralloc_handle.h: Fix build issue with Android
Hey John,
On Fri, Mar 16, 2018 at 10:48 PM, John Stultz wrote:
> In trying to integrate the new gralloc_handle.h with the
> drm_hwcomposer, I started seeing the following compilation
> errors:
>
> In file included from external/drm_hwcomposer/platformdrmgeneric.cpp:28:
> external/libdrm/android/gralloc_handle.h:108:9: error: cannot initialize
> return object of type 'native_handle_t *' (aka 'native_handle *') with an
> lvalue of type 'struct gralloc_handle_t *'
> return handle;
>^~
> 1 error generated.
>
> This seems to be due to the gralloc_handle_create() definition
> claiming to return a native_handle_t * type, rather then a
> gralloc_handle_t *, which is what the code actually returns.
>
> This function isn't actually used in the current drm_hwcomposer,
> so I'm not totally sure that this fix is correct (rather then
> returning the gralloc_handle_t's embadedded native_handle_t ptr).
This changed in 009634e49309 ("android: fix gralloc_handle_create() problems")
to make the return value from gralloc_handle_create opaque to its user.
Since the only one that would use gralloc_handle_create is the gralloc
implementation itself (and libdrm isn't one) and using getters/setters was
rejected (IIRC), I would think there is no reason to obscure the return type.
Since buffer_handle_t is native_handle_t* is gralloc_handle_t* it's also
correct. The alternative to changing the declaration here is returning
nhandle instead of handle.
Adding Rob Herring who authored 009634e49309.
Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] drm/vc4: Atomic color management support
This series adds support for the gamma and CTM properties to VC4. Patches 2 and 3 need the register definitions from here: https://patchwork.freedesktop.org/patch/207863/ The CTM support is somewhat limited in that we can only enable it for one CRTC at a time and coefficients are S0.9 in hardware. The latter still seem good enough for the various color corrections Android offers. Stefan Schake (3): drm/vc4: Expose gamma as atomic property drm/vc4: Add color transformation matrix (CTM) support drm/vc4: Restrict active CTM to one CRTC drivers/gpu/drm/vc4/vc4_crtc.c | 124 ++--- 1 file changed, 115 insertions(+), 9 deletions(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/vc4: Expose gamma as atomic property
We are an atomic driver so the gamma LUT should also be exposed as a
CRTC property through the DRM atomic color management. This will also
take care of the legacy path for us.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_crtc.c | 24 +---
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index bf46674..8d71098 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -298,23 +298,21 @@ vc4_crtc_lut_load(struct drm_crtc *crtc)
HVS_WRITE(SCALER_GAMDATA, vc4_crtc->lut_b[i]);
}
-static int
-vc4_crtc_gamma_set(struct drm_crtc *crtc, u16 *r, u16 *g, u16 *b,
- uint32_t size,
- struct drm_modeset_acquire_ctx *ctx)
+static void
+vc4_crtc_update_gamma_lut(struct drm_crtc *crtc)
{
struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+ struct drm_color_lut *lut = crtc->state->gamma_lut->data;
+ u32 length = crtc->state->gamma_lut->length / sizeof(*lut);
u32 i;
- for (i = 0; i < size; i++) {
- vc4_crtc->lut_r[i] = r[i] >> 8;
- vc4_crtc->lut_g[i] = g[i] >> 8;
- vc4_crtc->lut_b[i] = b[i] >> 8;
+ for (i = 0; i < length; i++) {
+ vc4_crtc->lut_r[i] = drm_color_lut_extract(lut[i].red, 8);
+ vc4_crtc->lut_g[i] = drm_color_lut_extract(lut[i].green, 8);
+ vc4_crtc->lut_b[i] = drm_color_lut_extract(lut[i].blue, 8);
}
vc4_crtc_lut_load(crtc);
-
- return 0;
}
static u32 vc4_get_fifo_full_level(u32 format)
@@ -699,6 +697,9 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
if (crtc->state->active && old_state->active)
vc4_crtc_update_dlist(crtc);
+ if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
+ vc4_crtc_update_gamma_lut(crtc);
+
if (debug_dump_regs) {
DRM_INFO("CRTC %d HVS after:\n", drm_crtc_index(crtc));
vc4_hvs_dump_state(dev);
@@ -909,7 +910,7 @@ static const struct drm_crtc_funcs vc4_crtc_funcs = {
.reset = vc4_crtc_reset,
.atomic_duplicate_state = vc4_crtc_duplicate_state,
.atomic_destroy_state = vc4_crtc_destroy_state,
- .gamma_set = vc4_crtc_gamma_set,
+ .gamma_set = drm_atomic_helper_legacy_gamma_set,
.enable_vblank = vc4_enable_vblank,
.disable_vblank = vc4_disable_vblank,
};
@@ -1035,6 +1036,7 @@ static int vc4_crtc_bind(struct device *dev, struct
device *master, void *data)
primary_plane->crtc = crtc;
vc4_crtc->channel = vc4_crtc->data->hvs_channel;
drm_mode_crtc_set_gamma_size(crtc, ARRAY_SIZE(vc4_crtc->lut_r));
+ drm_crtc_enable_color_mgmt(crtc, 0, false, crtc->gamma_size);
/* Set up some arbitrary number of planes. We're not limited
* by a set number of physical registers, just the space in
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/vc4: Add color transformation matrix (CTM) support
The hardware supports a CTM with S0.9 values. We therefore only allow
a value of 1.0 or fractional only and reject all others with integer
parts. This restriction is mostly inconsequential in practice since
commonly used transformation matrices have all scalars <= 1.0.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_crtc.c | 99 --
1 file changed, 96 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 8d71098..5c83fd2 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -315,6 +315,81 @@ vc4_crtc_update_gamma_lut(struct drm_crtc *crtc)
vc4_crtc_lut_load(crtc);
}
+/* Converts a DRM S31.32 value to the HW S0.9 format. */
+static u16 vc4_crtc_s31_32_to_s0_9(u64 in)
+{
+ u16 r;
+
+ /* Sign bit. */
+ r = in & BIT_ULL(63) ? BIT(9) : 0;
+ /* We have zero integer bits so we can only saturate here. */
+ if ((in & GENMASK_ULL(62, 32)) > 0)
+ r |= GENMASK(8, 0);
+ /* Otherwise take the 9 most important fractional bits. */
+ else
+ r |= (in >> 22) & GENMASK(8, 0);
+ return r;
+}
+
+static void
+vc4_crtc_update_ctm(struct drm_crtc *crtc)
+{
+ struct drm_device *dev = crtc->dev;
+ struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
+ struct drm_color_ctm *ctm = crtc->state->ctm->data;
+
+ HVS_WRITE(SCALER_OLEDCOEF2,
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[0]),
+ SCALER_OLEDCOEF2_R_TO_R) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[3]),
+ SCALER_OLEDCOEF2_R_TO_G) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[6]),
+ SCALER_OLEDCOEF2_R_TO_B));
+ HVS_WRITE(SCALER_OLEDCOEF1,
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[1]),
+ SCALER_OLEDCOEF1_G_TO_R) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[4]),
+ SCALER_OLEDCOEF1_G_TO_G) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[7]),
+ SCALER_OLEDCOEF1_G_TO_B));
+ HVS_WRITE(SCALER_OLEDCOEF0,
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[2]),
+ SCALER_OLEDCOEF0_B_TO_R) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[5]),
+ SCALER_OLEDCOEF0_B_TO_G) |
+ VC4_SET_FIELD(vc4_crtc_s31_32_to_s0_9(ctm->matrix[8]),
+ SCALER_OLEDCOEF0_B_TO_B));
+
+ /* Channel is 0-based but for DISPFIFO, 0 means disabled. */
+ HVS_WRITE(SCALER_OLEDOFFS, VC4_SET_FIELD(vc4_crtc->channel + 1,
+SCALER_OLEDOFFS_DISPFIFO));
+}
+
+/* Check if the CTM contains valid input.
+ *
+ * DRM exposes CTM with S31.32 scalars, but the HW only supports S0.9.
+ * We don't allow integer values >1, and 1 only without fractional part
+ * to handle the common 1.0 value.
+ */
+static int vc4_crtc_atomic_check_ctm(struct drm_crtc_state *state)
+{
+ struct drm_color_ctm *ctm = state->ctm->data;
+ u32 i;
+
+ for (i = 0; i < ARRAY_SIZE(ctm->matrix); i++) {
+ u64 val = ctm->matrix[i];
+
+ val &= ~BIT_ULL(63);
+ if ((val >> 32) > 1)
+ return -EINVAL;
+ if ((val >> 32) == 1 && (val & GENMASK_ULL(31, 0)) != 0)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static u32 vc4_get_fifo_full_level(u32 format)
{
static const u32 fifo_len_bytes = 64;
@@ -621,6 +696,15 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
if (hweight32(state->connector_mask) > 1)
return -EINVAL;
+ if (state->ctm) {
+ /* The CTM hardware has no integer bits, so we check
+* and reject scalars >1.0 that we have no chance of
+* approximating.
+*/
+ if (vc4_crtc_atomic_check_ctm(state))
+ return -EINVAL;
+ }
+
drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state)
dlist_count += vc4_plane_dlist_size(plane_state);
@@ -697,8 +781,17 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
if (crtc->state->active && old_state->active)
vc4_crtc_update_dlist(crtc);
- if (crtc->state->color_mgmt_changed && crtc->state->gamma_lut)
- vc4_crtc_update_gamma_lut(crtc);
+ if (crtc-
[PATCH 3/3] drm/vc4: Restrict active CTM to one CRTC
We only have one hardware block to do the CTM and need to reject
attempts to enable it for multiple CRTCs simultaneously.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_crtc.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 5c83fd2..64ff293 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -678,10 +678,17 @@ static enum drm_mode_status vc4_crtc_mode_valid(struct
drm_crtc *crtc,
return MODE_OK;
}
+static int vc4_crtc_get_ctm_fifo(struct vc4_dev *vc4)
+{
+ return VC4_GET_FIELD(HVS_READ(SCALER_OLEDOFFS),
+SCALER_OLEDOFFS_DISPFIFO);
+}
+
static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)
{
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(state);
+ struct drm_crtc_state *old_state = crtc->state;
struct drm_device *dev = crtc->dev;
struct vc4_dev *vc4 = to_vc4_dev(dev);
struct drm_plane *plane;
@@ -703,6 +710,10 @@ static int vc4_crtc_atomic_check(struct drm_crtc *crtc,
*/
if (vc4_crtc_atomic_check_ctm(state))
return -EINVAL;
+
+ /* We can only enable CTM for one fifo or CRTC at a time */
+ if (!old_state->ctm && vc4_crtc_get_ctm_fifo(vc4))
+ return -EINVAL;
}
drm_atomic_crtc_state_for_each_plane_state(plane, plane_state, state)
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH hwc v1] [RFC] drm_hwcomposer: Flatten composition using writeback connector
Hey Alexandru, On Tue, Mar 13, 2018 at 5:21 PM, Alexandru Gheorghe wrote: > This patchset tries to add support for using writeback connector to > flatten a scene when it doesn't change for a while. This idea had > been floated around on IRC here [1] and here [2]. > > 2. Heuristic for triggering the flattening. > I just created a VSyncWorker which will trigger the flattening of the > scene if it doesn't change for 60 consecutive vsyncs. The countdown > gets reset every time ValidateDisplay is called. This is a relatively > basic heuristic, so I'm open to suggestions. I think when Android deems that the display is sufficiently idle, it will disable VSync through setVsyncEnabled. Presumably we could just trigger the flattening on an enabled -> disabled transition? Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/4] drm/vc4: Set premultiplied for alpha formats
Alpha formats in DRM are assumed to be premultiplied, so we should be
setting the PREMULT bit in the plane configuration for HVS.
Changes from v1:
- Use correct has_alpha
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_plane.c | 3 ++-
drivers/gpu/drm/vc4/vc4_regs.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index c4c7af1..831e195 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -618,13 +618,14 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS1_SCL_HEIGHT));
}
- /* Position Word 2: Source Image Size, Alpha Mode */
+ /* Position Word 2: Source Image Size, Alpha */
vc4_state->pos2_offset = vc4_state->dlist_count;
vc4_dlist_write(vc4_state,
VC4_SET_FIELD(fb->format->has_alpha ?
SCALER_POS2_ALPHA_MODE_PIPELINE :
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
+ (fb->format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0)
|
VC4_SET_FIELD(vc4_state->src_w[0], SCALER_POS2_WIDTH) |
VC4_SET_FIELD(vc4_state->src_h[0], SCALER_POS2_HEIGHT));
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index b9749cb..a141496 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -848,6 +848,7 @@ enum hvs_pixel_format {
#define SCALER_POS2_ALPHA_MODE_FIXED 1
#define SCALER_POS2_ALPHA_MODE_FIXED_NONZERO 2
#define SCALER_POS2_ALPHA_MODE_FIXED_OVER_0x07 3
+#define SCALER_POS2_ALPHA_PREMULT BIT(29)
#define SCALER_POS2_HEIGHT_MASKVC4_MASK(27, 16)
#define SCALER_POS2_HEIGHT_SHIFT 16
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/4] drm/vc4: Check if plane requires background fill
Considering a single plane only, we have to enable background color
when the plane has an alpha format and could be blending from the
background or when it doesn't cover the entire screen.
Changes from v1:
- Drop unrelated change
- Move needs_bg_fill to plane state
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_plane.c | 17 +
1 file changed, 17 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 831e195..8be9a87 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -73,6 +73,12 @@ struct vc4_plane_state {
/* Our allocation in LBM for temporary storage during scaling. */
struct drm_mm_node lbm;
+
+ /* Set when the plane has per-pixel alpha content or does not cover
+* the entire screen. This is a hint to the CRTC that it might need
+* to enable background color fill.
+*/
+ bool needs_bg_fill;
};
static inline struct vc4_plane_state *
@@ -521,6 +527,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
u32 ctl0_offset = vc4_state->dlist_count;
const struct hvs_format *format =
vc4_get_hvs_format(fb->format->format);
int num_planes = drm_format_num_planes(format->drm);
+ bool covers_screen;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
unsigned long irqflags;
@@ -701,6 +708,16 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
vc4_state->dlist[ctl0_offset] |=
VC4_SET_FIELD(vc4_state->dlist_count, SCALER_CTL0_SIZE);
+ /* crtc_* are already clipped coordinates. */
+ covers_screen = vc4_state->crtc_x == 0 && vc4_state->crtc_y == 0 &&
+ vc4_state->crtc_w == state->crtc->mode.hdisplay &&
+ vc4_state->crtc_h == state->crtc->mode.vdisplay;
+ /* Background fill might be necessary when the plane has per-pixel
+* alpha content and blends from the background or does not cover
+* the entire screen.
+*/
+ vc4_state->needs_bg_fill = fb->format->has_alpha || !covers_screen;
+
return 0;
}
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/4] drm/vc4: Move plane state to header
We need to reference it from the CRTC to make a decision for enabling
background color fill.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_drv.h | 60 +
drivers/gpu/drm/vc4/vc4_plane.c | 60 -
2 files changed, 60 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fefa166..1b4cd1f 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -310,6 +310,66 @@ to_vc4_plane(struct drm_plane *plane)
return (struct vc4_plane *)plane;
}
+enum vc4_scaling_mode {
+ VC4_SCALING_NONE,
+ VC4_SCALING_TPZ,
+ VC4_SCALING_PPF,
+};
+
+struct vc4_plane_state {
+ struct drm_plane_state base;
+ /* System memory copy of the display list for this element, computed
+* at atomic_check time.
+*/
+ u32 *dlist;
+ u32 dlist_size; /* Number of dwords allocated for the display list */
+ u32 dlist_count; /* Number of used dwords in the display list. */
+
+ /* Offset in the dlist to various words, for pageflip or
+* cursor updates.
+*/
+ u32 pos0_offset;
+ u32 pos2_offset;
+ u32 ptr0_offset;
+
+ /* Offset where the plane's dlist was last stored in the
+* hardware at vc4_crtc_atomic_flush() time.
+*/
+ u32 __iomem *hw_dlist;
+
+ /* Clipped coordinates of the plane on the display. */
+ int crtc_x, crtc_y, crtc_w, crtc_h;
+ /* Clipped area being scanned from in the FB. */
+ u32 src_x, src_y;
+
+ u32 src_w[2], src_h[2];
+
+ /* Scaling selection for the RGB/Y plane and the Cb/Cr planes. */
+ enum vc4_scaling_mode x_scaling[2], y_scaling[2];
+ bool is_unity;
+ bool is_yuv;
+
+ /* Offset to start scanning out from the start of the plane's
+* BO.
+*/
+ u32 offsets[3];
+
+ /* Our allocation in LBM for temporary storage during scaling. */
+ struct drm_mm_node lbm;
+
+ /* Set when the plane has per-pixel alpha content or does not cover
+* the entire screen. This is a hint to the CRTC that it might need
+* to enable background color fill.
+*/
+ bool needs_bg_fill;
+};
+
+static inline struct vc4_plane_state *
+to_vc4_plane_state(struct drm_plane_state *state)
+{
+ return (struct vc4_plane_state *)state;
+}
+
enum vc4_encoder_type {
VC4_ENCODER_TYPE_NONE,
VC4_ENCODER_TYPE_HDMI,
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 8be9a87..ce39390 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -27,66 +27,6 @@
#include "vc4_drv.h"
#include "vc4_regs.h"
-enum vc4_scaling_mode {
- VC4_SCALING_NONE,
- VC4_SCALING_TPZ,
- VC4_SCALING_PPF,
-};
-
-struct vc4_plane_state {
- struct drm_plane_state base;
- /* System memory copy of the display list for this element, computed
-* at atomic_check time.
-*/
- u32 *dlist;
- u32 dlist_size; /* Number of dwords allocated for the display list */
- u32 dlist_count; /* Number of used dwords in the display list. */
-
- /* Offset in the dlist to various words, for pageflip or
-* cursor updates.
-*/
- u32 pos0_offset;
- u32 pos2_offset;
- u32 ptr0_offset;
-
- /* Offset where the plane's dlist was last stored in the
-* hardware at vc4_crtc_atomic_flush() time.
-*/
- u32 __iomem *hw_dlist;
-
- /* Clipped coordinates of the plane on the display. */
- int crtc_x, crtc_y, crtc_w, crtc_h;
- /* Clipped area being scanned from in the FB. */
- u32 src_x, src_y;
-
- u32 src_w[2], src_h[2];
-
- /* Scaling selection for the RGB/Y plane and the Cb/Cr planes. */
- enum vc4_scaling_mode x_scaling[2], y_scaling[2];
- bool is_unity;
- bool is_yuv;
-
- /* Offset to start scanning out from the start of the plane's
-* BO.
-*/
- u32 offsets[3];
-
- /* Our allocation in LBM for temporary storage during scaling. */
- struct drm_mm_node lbm;
-
- /* Set when the plane has per-pixel alpha content or does not cover
-* the entire screen. This is a hint to the CRTC that it might need
-* to enable background color fill.
-*/
- bool needs_bg_fill;
-};
-
-static inline struct vc4_plane_state *
-to_vc4_plane_state(struct drm_plane_state *state)
-{
- return (struct vc4_plane_state *)state;
-}
-
static const struct hvs_format {
u32 drm; /* DRM_FORMAT_* */
u32 hvs; /* HVS_FORMAT_* */
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 4/4] drm/vc4: Enable background color fill when necessary
Using the hint from the plane state, we turn on the background color
to avoid display corruption from planes blending with the background.
Changes from v1:
- Use needs_bg_fill from plane state
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_crtc.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index ce1e3b9..bf46674 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -643,9 +643,12 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
{
struct drm_device *dev = crtc->dev;
struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
struct drm_plane *plane;
+ struct vc4_plane_state *vc4_plane_state;
bool debug_dump_regs = false;
+ bool enable_bg_fill = false;
u32 __iomem *dlist_start = vc4->hvs->dlist + vc4_state->mm.start;
u32 __iomem *dlist_next = dlist_start;
@@ -656,6 +659,20 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
/* Copy all the active planes' dlist contents to the hardware dlist. */
drm_atomic_crtc_for_each_plane(plane, crtc) {
+ /* Is this the first active plane? */
+ if (dlist_next == dlist_start) {
+ /* We need to enable background fill when a plane
+* could be alpha blending from the background, i.e.
+* where no other plane is underneath. It suffices to
+* consider the first active plane here since we set
+* needs_bg_fill such that either the first plane
+* already needs it or all planes on top blend from
+* the first or a lower plane.
+*/
+ vc4_plane_state = to_vc4_plane_state(plane->state);
+ enable_bg_fill = vc4_plane_state->needs_bg_fill;
+ }
+
dlist_next += vc4_plane_write_dlist(plane, dlist_next);
}
@@ -664,6 +681,14 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
WARN_ON_ONCE(dlist_next - dlist_start != vc4_state->mm.size);
+ if (enable_bg_fill)
+ /* This sets a black background color fill, as is the case
+* with other DRM drivers.
+*/
+ HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
+ HVS_READ(SCALER_DISPBKGNDX(vc4_crtc->channel)) |
+ SCALER_DISPBKGND_FILL);
+
/* Only update DISPLIST if the CRTC was already running and is not
* being disabled.
* vc4_crtc_enable() takes care of updating the dlist just after
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/4] drm/vc4: Improve alpha format plane support
This series improves the handling of alpha formats with the VC4 HVS compositor. Alpha formats are marked as premultiplied as is standard for DRM. Further fix a display corruption issue when planes with per-pixel alpha try blending from the (nonexistent) background by selectively enabling a black background color fill. This series follows the changes suggested by Eric Anholt in a previous patch discussion: https://patchwork.freedesktop.org/patch/207667/ A simple test program for the display corruption issue is available: https://github.com/stschake/vc4-alpha-test v2 of the series fixes the has_alpha confusion and moves needs_bg_fill into the plane state. This unfortunately necessitated moving the plane state to a header where it can be referenced from vc4_crtc. Stefan Schake (4): drm/vc4: Set premultiplied for alpha formats drm/vc4: Check if plane requires background fill drm/vc4: Move plane state to header drm/vc4: Enable background color fill when necessary drivers/gpu/drm/vc4/vc4_crtc.c | 25 +++ drivers/gpu/drm/vc4/vc4_drv.h | 60 drivers/gpu/drm/vc4/vc4_plane.c | 68 - drivers/gpu/drm/vc4/vc4_regs.h | 1 + 4 files changed, 99 insertions(+), 55 deletions(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property
Hey, On Wed, Feb 21, 2018 at 2:04 PM, Maxime Ripard wrote: > > What is the behaviour of the tegra engine when it has both a > pixel-alpha and a plane-alpha? > > Atmel at least will drop one (but I'm not sure which one anymore). Sorry, I have no more on the Tegra beyond that commit. But I did have some more play time with drm_hwcomposer and from the rendering, it certainly expects both to apply at the same time. To further complicate it, from what I can tell with the VC4 HVS, you can actually configure it to 1) use only pixel alpha 2) use only plane alpha or 3) mix both pixel and plane alpha, with 3 being what drm_hwc seems to want. I think once writeback lands it would be a good idea to have some tests that establish DRM plane blending standards beyond text in docs :) Regards, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/3] drm/vc4: Set premultiplied for alpha formats
On Tue, Mar 6, 2018 at 8:35 PM, Eric Anholt wrote: > Stefan Schake writes: > >> Alpha formats in DRM are assumed to be premultiplied, so we should be >> setting the PREMULT bit in the plane configuration for HVS. >> >> Signed-off-by: Stefan Schake >> --- >> drivers/gpu/drm/vc4/vc4_plane.c | 3 ++- >> drivers/gpu/drm/vc4/vc4_regs.h | 1 + >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_plane.c >> b/drivers/gpu/drm/vc4/vc4_plane.c >> index c4c7af1..3d0c8a2 100644 >> --- a/drivers/gpu/drm/vc4/vc4_plane.c >> +++ b/drivers/gpu/drm/vc4/vc4_plane.c >> @@ -618,13 +618,14 @@ static int vc4_plane_mode_set(struct drm_plane *plane, >> SCALER_POS1_SCL_HEIGHT)); >> } >> >> - /* Position Word 2: Source Image Size, Alpha Mode */ >> + /* Position Word 2: Source Image Size, Alpha */ >> vc4_state->pos2_offset = vc4_state->dlist_count; >> vc4_dlist_write(vc4_state, >> VC4_SET_FIELD(fb->format->has_alpha ? >> SCALER_POS2_ALPHA_MODE_PIPELINE : >> SCALER_POS2_ALPHA_MODE_FIXED, >> SCALER_POS2_ALPHA_MODE) | >> + (format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) | > > Looks like you meant fb->format->has_alpha here. I can fix that up when > applying -- everything else looks good to me. I'll let this sit on the > list for a day or two in case anyone else has feedback. I remember fixing that up, and I did - but in 2/3, where of course it doesn't really belong. Sorry, I must have gotten my rebase mixed up. Since that makes two patches that need fixups, would you prefer I send a v2 instead? Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm_hwcomposer: Question about buffer importer
Hey Andrii, On Tue, Mar 6, 2018 at 4:48 PM, Andrii Chepurnyi wrote: > > Could you please suggest me how to overcome that situation with buffers > registration/deregistration? I think drm_hwcomposer would be the wrong place to do any sort of caching. There is a Gralloc usage flag for framebuffers. So I would suggest instead that you do the heavy lifting that makes your AddFB2/RmFB slow already in your Gralloc module upon framebuffer allocation. If you want, you could probably just do the AddFB2/RmFB entirely in Gralloc and store the FB id as part of the native handle. This is what the Android-x86 drm_gralloc does: https://github.com/robclark/drm_gralloc/blob/freedreno/gralloc.c#L236 Regards, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Ignore alpha on primary plane
On Mon, Mar 5, 2018 at 10:15 PM, Eric Anholt wrote: > Stefan Schake writes: > >> We allow alpha formats on the primary plane but a partially transparent >> framebuffer will cause a corrupted display. With this change black pixels >> are output instead, in line with the behavior for other DRM drivers. >> >> Signed-off-by: Stefan Schake >> --- >> Test program is available at https://github.com/stschake/vc4-alpha-test > > How about this as a suggestion for a patch series: > > vc4_plane_mode_set() sets ALPHA_PREMULT (POS2 bit 29) if alpha is > enabled. > > vc4_plane_mode_set() sets a new vc4_plane->needs_bg_fill boolean to > (format->has_alpha || !covers_screen) where covers_screenis the > can_position logic from drm_atomic_helper.c > > vc4_crtc_atomic_flush() updates DISPBKGND to enable background fill > (before vc4_crtc_update_dlist()) if the first plane has needs_bg_fill > set. > > vc4_plane_mode_set() strips off the alpha blend bits if > !vc4_plane->needs_bg_fill. > > This lets us keep avoiding the background fill cost in the normal case, > and fixes the case where the "primary" plane doesn't cover the screen. > It doesn't get the background fill turned back off if you transition > away from primary not covering the screen, but that seems unlikely and > harder to handle (since you would need to wait for the flip to be done > before disabling). I've sent out the series: https://patchwork.freedesktop.org/series/39411/ I don't think we need the final change since !needs_bg_fill <=> !(has_alpha||!covers_screen) <=> covers_screen && !has_alpha so we shouldn't be setting the alpha blend bits in the first place. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/3] drm/vc4: Improve alpha format plane support
This series improves the handling of alpha formats with the VC4 HVS compositor. Alpha formats are marked as premultiplied as is standard for DRM. Further fix a display corruption issue when planes with per-pixel alpha try blending from the (nonexistent) background by selectively enabling a black background color fill. This series follows the changes suggested by Eric Anholt in a previous patch discussion: https://patchwork.freedesktop.org/patch/207667/ A simple test program for the display corruption issue is available: https://github.com/stschake/vc4-alpha-test Stefan Schake (3): drm/vc4: Set premultiplied for alpha formats drm/vc4: Check if plane requires background fill drm/vc4: Enable background color fill when necessary drivers/gpu/drm/vc4/vc4_crtc.c | 22 ++ drivers/gpu/drm/vc4/vc4_drv.h | 6 ++ drivers/gpu/drm/vc4/vc4_plane.c | 16 +++- drivers/gpu/drm/vc4/vc4_regs.h | 1 + 4 files changed, 44 insertions(+), 1 deletion(-) -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/3] drm/vc4: Enable background color fill when necessary
Using the hint from the plane state, we turn on the background color
to avoid display corruption from planes blending with the background.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_crtc.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index ce1e3b9..728845f 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -643,9 +643,11 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
{
struct drm_device *dev = crtc->dev;
struct vc4_dev *vc4 = to_vc4_dev(dev);
+ struct vc4_crtc *vc4_crtc = to_vc4_crtc(crtc);
struct vc4_crtc_state *vc4_state = to_vc4_crtc_state(crtc->state);
struct drm_plane *plane;
bool debug_dump_regs = false;
+ bool enable_bg_fill = false;
u32 __iomem *dlist_start = vc4->hvs->dlist + vc4_state->mm.start;
u32 __iomem *dlist_next = dlist_start;
@@ -656,6 +658,18 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
/* Copy all the active planes' dlist contents to the hardware dlist. */
drm_atomic_crtc_for_each_plane(plane, crtc) {
+ /* Is this the first active plane? */
+ if (dlist_next == dlist_start)
+ /* We need to enable background fill when a plane
+* could be alpha blending from the background, i.e.
+* where no other plane is underneath. It suffices to
+* consider the first active plane here since we set
+* needs_bg_fill such that either the first plane
+* already needs it or all planes on top blend from
+* the first or a lower plane.
+*/
+ enable_bg_fill = to_vc4_plane(plane)->needs_bg_fill;
+
dlist_next += vc4_plane_write_dlist(plane, dlist_next);
}
@@ -664,6 +678,14 @@ static void vc4_crtc_atomic_flush(struct drm_crtc *crtc,
WARN_ON_ONCE(dlist_next - dlist_start != vc4_state->mm.size);
+ if (enable_bg_fill)
+ /* This sets a black background color fill, as is the case
+* with other DRM drivers.
+*/
+ HVS_WRITE(SCALER_DISPBKGNDX(vc4_crtc->channel),
+ HVS_READ(SCALER_DISPBKGNDX(vc4_crtc->channel)) |
+ SCALER_DISPBKGND_FILL);
+
/* Only update DISPLIST if the CRTC was already running and is not
* being disabled.
* vc4_crtc_enable() takes care of updating the dlist just after
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/3] drm/vc4: Check if plane requires background fill
Considering a single plane only, we have to enable background color
when the plane has an alpha format and could be blending from the
background or when it doesn't cover the entire screen.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_drv.h | 6 ++
drivers/gpu/drm/vc4/vc4_plane.c | 15 ++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fefa166..7cc6390 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -302,6 +302,12 @@ struct vc4_hvs {
struct vc4_plane {
struct drm_plane base;
+
+ /* Set when the plane has per-pixel alpha content or does not cover
+* the entire screen. This is a hint to the CRTC that it might need
+* to enable background color fill.
+*/
+ bool needs_bg_fill;
};
static inline struct vc4_plane *
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index 3d0c8a2..c299e29 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -517,10 +517,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
{
struct vc4_dev *vc4 = to_vc4_dev(plane->dev);
struct vc4_plane_state *vc4_state = to_vc4_plane_state(state);
+ struct vc4_plane *vc4_plane = to_vc4_plane(plane);
struct drm_framebuffer *fb = state->fb;
u32 ctl0_offset = vc4_state->dlist_count;
const struct hvs_format *format =
vc4_get_hvs_format(fb->format->format);
int num_planes = drm_format_num_planes(format->drm);
+ bool covers_screen;
u32 scl0, scl1, pitch0;
u32 lbm_size, tiling;
unsigned long irqflags;
@@ -625,7 +627,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS2_ALPHA_MODE_PIPELINE :
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
- (format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) |
+ (fb->format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0)
|
VC4_SET_FIELD(vc4_state->src_w[0], SCALER_POS2_WIDTH) |
VC4_SET_FIELD(vc4_state->src_h[0], SCALER_POS2_HEIGHT));
@@ -701,6 +703,17 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
vc4_state->dlist[ctl0_offset] |=
VC4_SET_FIELD(vc4_state->dlist_count, SCALER_CTL0_SIZE);
+ /* crtc_* are already clipped coordinates. */
+ covers_screen = vc4_state->crtc_x == 0 && vc4_state->crtc_y == 0 &&
+ vc4_state->crtc_w == state->crtc->mode.hdisplay &&
+ vc4_state->crtc_h == state->crtc->mode.vdisplay;
+ /* Background fill might be necessary when the plane has per-pixel
+* alpha content and blends from the background or does not cover
+* the entire screen.
+*/
+ vc4_plane->needs_bg_fill = fb->format->has_alpha || !covers_screen;
+
+
return 0;
}
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/3] drm/vc4: Set premultiplied for alpha formats
Alpha formats in DRM are assumed to be premultiplied, so we should be
setting the PREMULT bit in the plane configuration for HVS.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_plane.c | 3 ++-
drivers/gpu/drm/vc4/vc4_regs.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c
index c4c7af1..3d0c8a2 100644
--- a/drivers/gpu/drm/vc4/vc4_plane.c
+++ b/drivers/gpu/drm/vc4/vc4_plane.c
@@ -618,13 +618,14 @@ static int vc4_plane_mode_set(struct drm_plane *plane,
SCALER_POS1_SCL_HEIGHT));
}
- /* Position Word 2: Source Image Size, Alpha Mode */
+ /* Position Word 2: Source Image Size, Alpha */
vc4_state->pos2_offset = vc4_state->dlist_count;
vc4_dlist_write(vc4_state,
VC4_SET_FIELD(fb->format->has_alpha ?
SCALER_POS2_ALPHA_MODE_PIPELINE :
SCALER_POS2_ALPHA_MODE_FIXED,
SCALER_POS2_ALPHA_MODE) |
+ (format->has_alpha ? SCALER_POS2_ALPHA_PREMULT : 0) |
VC4_SET_FIELD(vc4_state->src_w[0], SCALER_POS2_WIDTH) |
VC4_SET_FIELD(vc4_state->src_h[0], SCALER_POS2_HEIGHT));
diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
index b9749cb..a141496 100644
--- a/drivers/gpu/drm/vc4/vc4_regs.h
+++ b/drivers/gpu/drm/vc4/vc4_regs.h
@@ -848,6 +848,7 @@ enum hvs_pixel_format {
#define SCALER_POS2_ALPHA_MODE_FIXED 1
#define SCALER_POS2_ALPHA_MODE_FIXED_NONZERO 2
#define SCALER_POS2_ALPHA_MODE_FIXED_OVER_0x07 3
+#define SCALER_POS2_ALPHA_PREMULT BIT(29)
#define SCALER_POS2_HEIGHT_MASKVC4_MASK(27, 16)
#define SCALER_POS2_HEIGHT_SHIFT 16
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Ignore alpha on primary plane
On Fri, Mar 2, 2018 at 4:21 PM, Ville Syrjälä wrote: > On Fri, Mar 02, 2018 at 04:06:58PM +0100, Stefan Schake wrote: >> Hey Ville, >> >> On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä >> wrote: >> > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: >> >> If you want the plane to always be opaque you shouldn't expose any >> >> formats with alpha. >> >> >> >> Also what happens if one disables the primary plane? Or does the driver >> >> not allow that? >> > >> > Or just makes the plane not cover the entire screen? >> >> We've exposed alpha formats in the past so disabling that now would break >> userspace, certainly Android that chooses alpha-everything. > > So it refuses to even run on hardware that can't do per-pixel alpha on > the primary plane? Well since we have no real primary plane we'd have to remove support from every plane or add elaborate logic to atomic check that detects and rejects a configuration that has pixels blending from nothing, which presumably is what triggers the display artifacts. >> The VC4 HVS >> has no fixed planes so I'll acknowledge that the concept of a primary plane >> is somewhat dubious and userspace could disable it or make it not cover the >> entire screen, making this ineffective. >> >> But then ultimately there doesn't seem to be a standard for what the display >> is supposed to be if you have transparent pixels with no plane to blend to >> below. > > The standard is black. IMO it's a driver bug if it fails to do that. Then to be sure we should always enable the background fill. Maybe Eric can clarify what the cost for this is, all I have to go on there is the comment on the register set. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vc4: Ignore alpha on primary plane
Hey Ville, On Fri, Mar 2, 2018 at 3:43 PM, Ville Syrjälä wrote: > On Fri, Mar 02, 2018 at 04:39:22PM +0200, Ville Syrjälä wrote: >> If you want the plane to always be opaque you shouldn't expose any >> formats with alpha. >> >> Also what happens if one disables the primary plane? Or does the driver >> not allow that? > > Or just makes the plane not cover the entire screen? We've exposed alpha formats in the past so disabling that now would break userspace, certainly Android that chooses alpha-everything. The VC4 HVS has no fixed planes so I'll acknowledge that the concept of a primary plane is somewhat dubious and userspace could disable it or make it not cover the entire screen, making this ineffective. But then ultimately there doesn't seem to be a standard for what the display is supposed to be if you have transparent pixels with no plane to blend to below. This helps in the common case, the belts&braces fix would be to enable the VC4 HVS background color fill incurring a fixed overhead that in most cases is wasted because the composition ends up being opaque. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH hwc] drm_hwcomposer: set CRTC background color when available
Hey Rob, On Wed, Feb 28, 2018 at 11:53 AM, Robert Foss wrote: > Hey, > > Stefan: Are you looking at an entirely kernel side fix now, or are you > pushing this series forward? I've sent out a kernel side fix for this: https://patchwork.freedesktop.org/patch/207667/ So I guess for now this can be dropped, pending review. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/vc4: Ignore alpha on primary plane
We allow alpha formats on the primary plane but a partially transparent framebuffer will cause a corrupted display. With this change black pixels are output instead, in line with the behavior for other DRM drivers. Signed-off-by: Stefan Schake --- Test program is available at https://github.com/stschake/vc4-alpha-test drivers/gpu/drm/vc4/vc4_plane.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/vc4/vc4_plane.c b/drivers/gpu/drm/vc4/vc4_plane.c index 61ad955..8c829fa 100644 --- a/drivers/gpu/drm/vc4/vc4_plane.c +++ b/drivers/gpu/drm/vc4/vc4_plane.c @@ -521,6 +521,7 @@ static int vc4_plane_mode_set(struct drm_plane *plane, u32 ctl0_offset = vc4_state->dlist_count; const struct hvs_format *format = vc4_get_hvs_format(fb->format->format); int num_planes = drm_format_num_planes(format->drm); + bool primary_plane = state->crtc->primary == plane; u32 scl0, scl1, pitch0; u32 lbm_size, tiling; unsigned long irqflags; @@ -620,8 +621,12 @@ static int vc4_plane_mode_set(struct drm_plane *plane, /* Position Word 2: Source Image Size, Alpha Mode */ vc4_state->pos2_offset = vc4_state->dlist_count; + /* We do not enable the HVS background color fill so the primary plane +* must be opaque to avoid display artifacts. Achieve this by always +* using fixed alpha (initialized to 0xff above) on the primary plane. +*/ vc4_dlist_write(vc4_state, - VC4_SET_FIELD(fb->format->has_alpha ? + VC4_SET_FIELD(fb->format->has_alpha && !primary_plane ? SCALER_POS2_ALPHA_MODE_PIPELINE : SCALER_POS2_ALPHA_MODE_FIXED, SCALER_POS2_ALPHA_MODE) | -- 2.7.4 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH hwc] drm_hwcomposer: set CRTC background color when available
Hey Eric, On Thu, Feb 22, 2018 at 9:47 PM, Eric Anholt wrote: > Stefan Schake writes: > >> Android assumes an implicit black background layer is always present >> behind all layers it specifies for composition. drm_hwcomposer currently >> punts responsibility for this to the kernel/DRM platform and puts layers >> with per-pixel alpha content on the primary plane when requested. >> >> On some platforms (e.g. VC4) a background color fill has a cycle cost for >> the hardware composer and is not enabled by default. Instead, userland can >> request a background color through a CRTC property. Use this property to >> specify the implicit black background Android expects. >> >> Signed-off-by: Stefan Schake >> --- >> Kernel changes for this (background_color) are available here: >> >> https://github.com/stschake/linux/commits/background-upstream >> >> Sending as RFC because I'm not entirely clear on whose responsibility >> this should be, on most DRM drivers it seems to be implicit. I think >> a case could also be made that VC4 should not accept alpha formats on >> the lowest layer or enable background color fill when given one anyway. >> On the other hand, userland control over background color seems desirable >> regardless and is a feature of multiple hardware composers (i915, vc4, omap). > > Couldn't we just ignore the alpha channel on the primary plane, on the > assumption that it's supposed to be all zeroes below it? Or are we not > premultiplied, so we do still need to multiply the primary plane's > colors by the alpha value? I couldn't find any obvious DRM > documentation about whether alpha is premultiplied. That would work, too (for a black background anyway). Though the easiest place to spoof this is presumably in the kernel. From what I have seen, everything in Android is already premultiplied, but technically it can specify either. Not sure if this is correctly handled in drm_hwcomposer yet. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH hwc] drm_hwcomposer: set CRTC background color when available
Hey Robert, On Thu, Feb 22, 2018 at 11:04 AM, Robert Foss wrote: > Hey Stefan, > > On 02/22/2018 04:54 AM, Stefan Schake wrote: >> >> Android assumes an implicit black background layer is always present >> behind all layers it specifies for composition. drm_hwcomposer currently >> punts responsibility for this to the kernel/DRM platform and puts layers >> with per-pixel alpha content on the primary plane when requested. > > > I wasn't aware of this assumption, but given that it is the case, > the patch looks like a good fix for the problem. > > Unfortunately I don't have a hardware platform up and running to test the > patch with at the moment. > HWC1 has this ominous comment: * IMPORTANT NOTE: There is an implicit layer containing opaque black * pixels behind all the layers in the list. It is the responsibility of * the hwcomposer module to make sure black pixels are output (or blended * from). It's not repeated in the HWC2 docs, but I think the assumption carried over given that Android prefers all things RGBA. Admittedly it's rare that you don't have a full-size opaque layer in the composition like a background picture, so the cases where this causes corrupted rendering on VC4 are limited to some time during boot after the animation finishes but the launcher isn't up yet. Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC PATCH hwc] drm_hwcomposer: set CRTC background color when available
Android assumes an implicit black background layer is always present
behind all layers it specifies for composition. drm_hwcomposer currently
punts responsibility for this to the kernel/DRM platform and puts layers
with per-pixel alpha content on the primary plane when requested.
On some platforms (e.g. VC4) a background color fill has a cycle cost for
the hardware composer and is not enabled by default. Instead, userland can
request a background color through a CRTC property. Use this property to
specify the implicit black background Android expects.
Signed-off-by: Stefan Schake
---
Kernel changes for this (background_color) are available here:
https://github.com/stschake/linux/commits/background-upstream
Sending as RFC because I'm not entirely clear on whose responsibility
this should be, on most DRM drivers it seems to be implicit. I think
a case could also be made that VC4 should not accept alpha formats on
the lowest layer or enable background color fill when given one anyway.
On the other hand, userland control over background color seems desirable
regardless and is a feature of multiple hardware composers (i915, vc4, omap).
drmcrtc.cpp | 11 +++
drmcrtc.h| 2 ++
drmdisplaycompositor.cpp | 15 +++
3 files changed, 28 insertions(+)
diff --git a/drmcrtc.cpp b/drmcrtc.cpp
index 1b354fe..d7bcd7a 100644
--- a/drmcrtc.cpp
+++ b/drmcrtc.cpp
@@ -52,6 +52,13 @@ int DrmCrtc::Init() {
ALOGE("Failed to get OUT_FENCE_PTR property");
return ret;
}
+
+ ret = drm_->GetCrtcProperty(*this, "background_color",
+ &background_color_property_);
+ if (ret) {
+ALOGI("Failed to get background_color property");
+// This is an optional property
+ }
return 0;
}
@@ -86,4 +93,8 @@ const DrmProperty &DrmCrtc::mode_property() const {
const DrmProperty &DrmCrtc::out_fence_ptr_property() const {
return out_fence_ptr_property_;
}
+
+const DrmProperty &DrmCrtc::background_color_property() const {
+ return background_color_property_;
+}
}
diff --git a/drmcrtc.h b/drmcrtc.h
index c5a5599..704573a 100644
--- a/drmcrtc.h
+++ b/drmcrtc.h
@@ -46,6 +46,7 @@ class DrmCrtc {
const DrmProperty &active_property() const;
const DrmProperty &mode_property() const;
const DrmProperty &out_fence_ptr_property() const;
+ const DrmProperty &background_color_property() const;
private:
DrmResources *drm_;
@@ -59,6 +60,7 @@ class DrmCrtc {
DrmProperty active_property_;
DrmProperty mode_property_;
DrmProperty out_fence_ptr_property_;
+ DrmProperty background_color_property_;
};
}
diff --git a/drmdisplaycompositor.cpp b/drmdisplaycompositor.cpp
index e556e86..69c52dd 100644
--- a/drmdisplaycompositor.cpp
+++ b/drmdisplaycompositor.cpp
@@ -527,6 +527,21 @@ int
DrmDisplayCompositor::CommitFrame(DrmDisplayComposition *display_comp,
return ret;
}
+if (crtc->background_color_property().id() != 0) {
+ // Background color is specified as a 16 bit per channel RGBA value.
+ // Set a fully opaque black background as Android HWC expects.
+ const uint64_t background_color = 0x;
+
+ ret = drmModeAtomicAddProperty(pset, crtc->id(),
+ crtc->background_color_property().id(),
+ background_color) < 0;
+ if (ret) {
+ALOGE("Failed to add CRTC background_color to pset: %d", ret);
+drmModeAtomicFree(pset);
+return ret;
+ }
+}
+
ret = drmModeAtomicAddProperty(pset, crtc->id(),
crtc->mode_property().id(),
mode_.blob_id) < 0 ||
drmModeAtomicAddProperty(pset, connector->id(),
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 1/8] drm/blend: Add a generic alpha property
On Fri, Feb 16, 2018 at 7:20 PM, Ville Syrjälä
wrote:
> On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
>> Some drivers duplicate the logic to create a property to store a per-plane
>> alpha.
>>
>> This is especially useful if we ever want to support extra protocols for
>> Wayland like:
>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
>>
>> Let's create a helper in order to move that to the core.
>>
>> Cc: Laurent Pinchart
>> Reviewed-by: Boris Brezillon
>> Signed-off-by: Maxime Ripard
>> ---
>> Documentation/gpu/kms-properties.csv | 2 +-
>> drivers/gpu/drm/drm_atomic.c | 4 -
>> drivers/gpu/drm/drm_atomic_helper.c | 4 -
>> drivers/gpu/drm/drm_blend.c | 32 +-
>> include/drm/drm_blend.h | 1 +-
>> include/drm/drm_plane.h | 6 +-
>> 6 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/gpu/kms-properties.csv
>> b/Documentation/gpu/kms-properties.csv
>> index 927b65e14219..25ad3503d663 100644
>> --- a/Documentation/gpu/kms-properties.csv
>> +++ b/Documentation/gpu/kms-properties.csv
>> @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD
>> ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
>> ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
>> ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
>> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
>> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
>> plane from transparent (0) to fully opaque (MAX). If this property is set to
>> a value different than max, and that the pixel will define an alpha
>> component, the property will have precendance and the pixel value will be
>> ignored.
>
> Those semantics don't seem particularly good to me. I think we would want the
> per-pixel alpha and global alpha both to be effecive at the same time. You can
> always decide to ignore the per-pixel alpha by using a pixel format without
> alpha.
>
> Also, where's the userspace that wants this feature?
drm_hwcomposer uses an 8-bit per-plane alpha property, and from what I
can tell the semantics are that both pixel and plane alpha apply
simultaneously if present.
Here is what I think was the kernel-side for this:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306122
I've added Sean Paul, he might be able to give a more definitive answer.
Regards,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/vc4: Move IRQ enable to PM path
We were calling enable_irq on bind, where it was already enabled previously
by the IRQ helper. Additionally, dev->irq is not set correctly until after
postinstall and so was always zero here, triggering a warning in 4.15.
Fix both by moving the enable to the power management resume path, where we
know there was a previous disable invocation during suspend.
Fixes: 253696ccd613 ("drm/vc4: Account for interrupts in flight")
Signed-off-by: Stefan Schake
---
I tested replacing the enable/disable dance with just synchronize_irq,
but that only made the original kernel OOPS more sporadic.
drivers/gpu/drm/vc4/vc4_irq.c | 3 ---
drivers/gpu/drm/vc4/vc4_v3d.c | 3 +++
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 26eddbb..3dd62d7 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -209,9 +209,6 @@
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
- /* Undo the effects of a previous vc4_irq_uninstall. */
- enable_irq(dev->irq);
-
/* Enable both the render done and out of memory interrupts. */
V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS);
diff --git a/drivers/gpu/drm/vc4/vc4_v3d.c b/drivers/gpu/drm/vc4/vc4_v3d.c
index 622cd43..493f392b 100644
--- a/drivers/gpu/drm/vc4/vc4_v3d.c
+++ b/drivers/gpu/drm/vc4/vc4_v3d.c
@@ -327,6 +327,9 @@ static int vc4_v3d_runtime_resume(struct device *dev)
return ret;
vc4_v3d_init_hw(vc4->dev);
+
+ /* We disabled the IRQ as part of vc4_irq_uninstall in suspend. */
+ enable_irq(vc4->dev->irq);
vc4_irq_postinstall(vc4->dev);
return 0;
--
1.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm: vc4: WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 __irq_startup+0x9c/0xa8
On Fri, Dec 8, 2017 at 11:44 PM, Stefan Wahren wrote:
> Hi,
>
> the commit 253696ccd613 ("drm/vc4: Account for interrupts in flight")
> triggers a warning during boot of Raspberry Pi 2 with multi_v7_defconfig:
>
> [7.962699] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi
> mapping ok
> [7.962732] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
> [7.973355] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
> [7.973651] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
> [7.973788] vc4-drm soc:gpu: bound 3f40.hvs (ops vc4_hvs_ops [vc4])
> [7.974157] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops
> [vc4])
> [7.974464] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops
> [vc4])
> [7.974746] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops
> [vc4])
> [8.018820] [ cut here ]
> [8.018861] WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244
> __irq_startup+0x9c/0xa8
> [8.018868] Modules linked in: vc4(+) snd_soc_core ac97_bus
> snd_pcm_dmaengine snd_pcm snd_timer snd soundcore cec
> [8.018911] CPU: 2 PID: 172 Comm: systemd-udevd Not tainted
> 4.15.0-rc1-00291-g75f64f6 #10
> [8.018914] Hardware name: BCM2835
> [8.018950] [] (unwind_backtrace) from []
> (show_stack+0x10/0x14)
> [8.018970] [] (show_stack) from []
> (dump_stack+0x88/0xa4)
> [8.018993] [] (dump_stack) from [] (__warn+0xe4/0x110)
> [8.019012] [] (__warn) from []
> (warn_slowpath_null+0x3c/0x48)
> [8.019029] [] (warn_slowpath_null) from []
> (__irq_startup+0x9c/0xa8)
> [8.019045] [] (__irq_startup) from []
> (irq_startup+0x44/0x134)
> [8.019061] [] (irq_startup) from []
> (enable_irq+0x34/0x6c)
> [8.019210] [] (enable_irq) from []
> (vc4_irq_postinstall+0x14/0x34 [vc4])
> [8.019338] [] (vc4_irq_postinstall [vc4]) from []
> (drm_irq_install+0xc0/0x134)
> [8.019456] [] (drm_irq_install) from []
> (vc4_v3d_bind+0x12c/0x238 [vc4])
> [8.019550] [] (vc4_v3d_bind [vc4]) from []
> (component_bind_all+0xe8/0x21c)
> [8.019635] [] (component_bind_all) from []
> (vc4_drm_bind+0x78/0x130 [vc4])
> [8.019721] [] (vc4_drm_bind [vc4]) from []
> (try_to_bring_up_master+0x13c/0x184)
> [8.019739] [] (try_to_bring_up_master) from []
> (component_master_add_with_match+0x80/0xb8)
> [8.019822] [] (component_master_add_with_match) from
> [] (vc4_platform_drm_probe+0x90/0xa8 [vc4])
> [8.019905] [] (vc4_platform_drm_probe [vc4]) from []
> (platform_drv_probe+0x4c/0xac)
> [8.019924] [] (platform_drv_probe) from []
> (driver_probe_device+0x234/0x338)
> [8.019939] [] (driver_probe_device) from []
> (__driver_attach+0xac/0xb0)
> [8.019953] [] (__driver_attach) from []
> (bus_for_each_dev+0x64/0x94)
> [8.019967] [] (bus_for_each_dev) from []
> (bus_add_driver+0x184/0x20c)
> [8.019981] [] (bus_add_driver) from []
> (driver_register+0x78/0xf8)
> [8.019997] [] (driver_register) from []
> (do_one_initcall+0x3c/0x16c)
> [8.020018] [] (do_one_initcall) from []
> (do_init_module+0x5c/0x1f0)
> [8.020037] [] (do_init_module) from []
> (load_module+0x1ba4/0x2248)
> [8.020058] [] (load_module) from []
> (SyS_finit_module+0x8c/0x9c)
> [8.020076] [] (SyS_finit_module) from []
> (__sys_trace_return+0x0/0x10)
> [8.020085] ---[ end trace d5c350f831cb07d2 ]---
> [8.020201] vc4-drm soc:gpu: bound 3fc0.v3d (ops vc4_v3d_ops [vc4])
So I've done some testing on an RPi 3b that is also affected.
It turns out that dev->irq is only initialized *after* postinstall by
drm_irq_install, so we were calling irq_enable on IRQ 0 and that just happens
to be not activated, hence the warning.
Having learned some more about the IRQ subsystem, my plan would be first to
simply replace the disable/enable dance with one synchronize_irq. We do
reenable the interrupt in the HW registers at the end of the binner work
callback so this may not suffice. In that case, we could simply move the
enable_irq to the power management handler that is calling postinstall.
Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm: vc4: WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244 __irq_startup+0x9c/0xa8
On Sat, Dec 9, 2017 at 1:34 PM, Marc Zyngier wrote:
> On Fri, 08 Dec 2017 22:44:27 +,
> Stefan Wahren wrote:
>>
>> Hi,
>>
>> the commit 253696ccd613 ("drm/vc4: Account for interrupts in
>> flight") triggers a warning during boot of Raspberry Pi 2 with
>> multi_v7_defconfig:
>>
>> [7.962699] vc4_hdmi 3f902000.hdmi: vc4-hdmi-hifi <-> 3f902000.hdmi
>> mapping ok
>> [7.962732] vc4_hdmi 3f902000.hdmi: ASoC: no DMI vendor name!
>> [7.973355] vc4-drm soc:gpu: bound 3f902000.hdmi (ops vc4_hdmi_ops [vc4])
>> [7.973651] vc4-drm soc:gpu: bound 3f806000.vec (ops vc4_vec_ops [vc4])
>> [7.973788] vc4-drm soc:gpu: bound 3f40.hvs (ops vc4_hvs_ops [vc4])
>> [7.974157] vc4-drm soc:gpu: bound 3f206000.pixelvalve (ops vc4_crtc_ops
>> [vc4])
>> [7.974464] vc4-drm soc:gpu: bound 3f207000.pixelvalve (ops vc4_crtc_ops
>> [vc4])
>> [7.974746] vc4-drm soc:gpu: bound 3f807000.pixelvalve (ops vc4_crtc_ops
>> [vc4])
>> [8.018820] [ cut here ]
>> [8.018861] WARNING: CPU: 2 PID: 172 at kernel/irq/chip.c:244
>> __irq_startup+0x9c/0xa8
>> [8.018868] Modules linked in: vc4(+) snd_soc_core ac97_bus
>> snd_pcm_dmaengine snd_pcm snd_timer snd soundcore cec
>> [8.018911] CPU: 2 PID: 172 Comm: systemd-udevd Not tainted
>> 4.15.0-rc1-00291-g75f64f6 #10
>> [8.018914] Hardware name: BCM2835
>> [8.018950] [] (unwind_backtrace) from []
>> (show_stack+0x10/0x14)
>> [8.018970] [] (show_stack) from []
>> (dump_stack+0x88/0xa4)
>> [8.018993] [] (dump_stack) from []
>> (__warn+0xe4/0x110)
>> [8.019012] [] (__warn) from []
>> (warn_slowpath_null+0x3c/0x48)
>> [8.019029] [] (warn_slowpath_null) from []
>> (__irq_startup+0x9c/0xa8)
>> [8.019045] [] (__irq_startup) from []
>> (irq_startup+0x44/0x134)
>> [8.019061] [] (irq_startup) from []
>> (enable_irq+0x34/0x6c)
>> [8.019210] [] (enable_irq) from []
>> (vc4_irq_postinstall+0x14/0x34 [vc4])
>> [8.019338] [] (vc4_irq_postinstall [vc4]) from []
>> (drm_irq_install+0xc0/0x134)
>> [8.019456] [] (drm_irq_install) from []
>> (vc4_v3d_bind+0x12c/0x238 [vc4])
>
> The sequence "request_irq/enable" feels pretty odd, given that the
> interrupt wasn't requested with NOAUTOEN. What are the expected
> semantics of that patch?
>
> Thanks,
>
> M.
>
> --
> Jazz is not dead, it just smell funny.
There is a second path to vc4_irq_postinstall when we return from
power management disable. Maybe the enable would be better situated there?
That said, some more study of the irq code is giving me more questions than
answers. If disable/enable are depth-counted, why isn't it also warning
about that (there is a check)? Depth must be 1 after request_irq.
Similarly, the irq is activated in request_irq, but the warning reported
here is complaining that it is not.
Thanks,
Stefan
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/vc4: Release fence after signalling
We were never releasing the initial fence reference that is obtained
through dma_fence_init.
Link: https://github.com/anholt/linux/issues/122
Fixes: cdec4d361323 ("drm/vc4: Expose dma-buf fences for V3D rendering.")
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_gem.c | 4 +++-
drivers/gpu/drm/vc4/vc4_irq.c | 1 +
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index 6c32c89..6385409 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -888,8 +888,10 @@ struct vc4_hang_state {
/* If we got force-completed because of GPU reset rather than
* through our IRQ handler, signal the fence now.
*/
- if (exec->fence)
+ if (exec->fence) {
dma_fence_signal(exec->fence);
+ dma_fence_put(exec->fence);
+ }
if (exec->bo) {
for (i = 0; i < exec->bo_count; i++) {
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 61b2e53..26eddbb 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -139,6 +139,7 @@
list_move_tail(&exec->head, &vc4->job_done_list);
if (exec->fence) {
dma_fence_signal_locked(exec->fence);
+ dma_fence_put(exec->fence);
exec->fence = NULL;
}
vc4_submit_next_render_job(dev);
--
1.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled
On Tue, Nov 14, 2017 at 8:44 PM, Eric Anholt wrote: > Stefan Schake writes: > >> On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt wrote: >>> Stefan Schake writes: >>> >>>> The overflow mem work callback vc4_overflow_mem_work reenables its >>>> associated interrupt upon completion. To ensure all interrupts are disabled >>>> when we return from vc4_irq_uninstall, we need to disable it again if >>>> cancel_work_sync indicated pending work. >>> >>> Is there a reason we need the interrupts disabled at the V3D level while >>> we have the IRQ disabled at the irqchip level? Once we re-enable at the >>> irqchip, we immediately V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS) anyway. >> >> irqchip will mask it in the ARM interrupt controller, so we will certainly >> never >> see an interrupt. I'm not sure on the exact guarantees V3D_INTENA and >> V3D_INTCTL make - does the state in INTENA affect if V3D will signal an >> interrupt in INTCTL? We're not currently clearing the latter in postinstall. > > INTENA/INTDIS writes update the state of the single register that > controls which bits of INTCTL get ORed together to raise the interrupt > outside the V3D block. Then I certainly agree - this patch doesn't do anything and should be dropped. Good call! Thanks, Stefan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] drm/vc4: Ensure interrupts are disabled
On Tue, Nov 14, 2017 at 1:18 AM, Eric Anholt wrote: > Stefan Schake writes: > >> The overflow mem work callback vc4_overflow_mem_work reenables its >> associated interrupt upon completion. To ensure all interrupts are disabled >> when we return from vc4_irq_uninstall, we need to disable it again if >> cancel_work_sync indicated pending work. > > Is there a reason we need the interrupts disabled at the V3D level while > we have the IRQ disabled at the irqchip level? Once we re-enable at the > irqchip, we immediately V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS) anyway. irqchip will mask it in the ARM interrupt controller, so we will certainly never see an interrupt. I'm not sure on the exact guarantees V3D_INTENA and V3D_INTCTL make - does the state in INTENA affect if V3D will signal an interrupt in INTCTL? We're not currently clearing the latter in postinstall. From a practical perspective, we're not doing anything in between uninstall and postinstall that would trigger the interrupt. So in that sense it's certainly superfluous. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/vc4: Ensure interrupts are disabled
The overflow mem work callback vc4_overflow_mem_work reenables its
associated interrupt upon completion. To ensure all interrupts are disabled
when we return from vc4_irq_uninstall, we need to disable it again if
cancel_work_sync indicated pending work.
Signed-off-by: Stefan Schake
---
drivers/gpu/drm/vc4/vc4_irq.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 61b2e53..7d780149d 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -231,7 +231,14 @@
/* Finish any interrupt handler still in flight. */
disable_irq(dev->irq);
- cancel_work_sync(&vc4->overflow_mem_work);
+ if (cancel_work_sync(&vc4->overflow_mem_work)) {
+ /*
+* Work was still pending. The overflow mem work's
+* callback reenables the OUTOMEM interrupt upon
+* completion, so ensure it is disabled here.
+*/
+ V3D_WRITE(V3D_INTDIS, V3D_INT_OUTOMEM);
+ }
}
/** Reinitializes interrupt registers when a GPU reset is performed. */
--
1.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm/vc4: Correctly uninstall interrupts
This set of patches fixes issues with vc4_irq_uninstall. The first patch fixes a NULL pointer dereference when the binner BO would disappear during an in flight overflow mem work callback. The second patch ensures we return with all interrupts disabled. This was suspected to cause the NULL dereference but turned out to be unrelated. Tested with a Raspberry Pi CM 3 that was previously stuck in a boot loop due to the issue. With the patch applied, the NULL dereference was no longer observed through numerous resets. Stefan Schake (2): drm/vc4: Account for interrupts in flight drm/vc4: Ensure interrupts are disabled drivers/gpu/drm/vc4/vc4_irq.c | 15 ++- 1 file changed, 14 insertions(+), 1 deletion(-) -- 1.9.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/vc4: Account for interrupts in flight
Synchronously disable the IRQ to make the following cancel_work_sync
invocation effective.
An interrupt in flight could enqueue further overflow mem work. As we
free the binner BO immediately following vc4_irq_uninstall this caused
a NULL pointer dereference in the work callback vc4_overflow_mem_work.
Link: https://github.com/anholt/linux/issues/114
Signed-off-by: Stefan Schake
Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.")
---
drivers/gpu/drm/vc4/vc4_irq.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/vc4/vc4_irq.c b/drivers/gpu/drm/vc4/vc4_irq.c
index 7d7af3a..61b2e53 100644
--- a/drivers/gpu/drm/vc4/vc4_irq.c
+++ b/drivers/gpu/drm/vc4/vc4_irq.c
@@ -208,6 +208,9 @@
{
struct vc4_dev *vc4 = to_vc4_dev(dev);
+ /* Undo the effects of a previous vc4_irq_uninstall. */
+ enable_irq(dev->irq);
+
/* Enable both the render done and out of memory interrupts. */
V3D_WRITE(V3D_INTENA, V3D_DRIVER_IRQS);
@@ -225,6 +228,9 @@
/* Clear any pending interrupts we might have left. */
V3D_WRITE(V3D_INTCTL, V3D_DRIVER_IRQS);
+ /* Finish any interrupt handler still in flight. */
+ disable_irq(dev->irq);
+
cancel_work_sync(&vc4->overflow_mem_work);
}
--
1.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
