On Tue, Oct 08, 2019 at 04:33:35PM -0300, Ezequiel Garcia wrote:
On Tue, 2019-10-08 at 16:23 -0300, Ezequiel Garcia wrote:
Hello Sean,
On Mon, 2019-10-07 at 14:54 -0400, Sean Paul wrote:
On Mon, Sep 30, 2019 at 07:28:00PM -0300, Ezequiel Garcia wrote:
Add an optional CRTC gamma LUT support, and enable it on RK3288. This is currently enabled via a separate address resource, which needs to be specified in the devicetree.
The address resource is required because on some SoCs, such as RK3288, the LUT address is after the MMU address, and the latter is supported by a different driver. This prevents the DRM driver from requesting an entire register space.
The current implementation works for RGB 10-bit tables, as that is what seems to work on RK3288.
Signed-off-by: Ezequiel Garcia ezequiel@collabora.com Reviewed-by: Douglas Anderson dianders@chromium.org Reviewed-by: Jacopo Mondi jacopo+renesas@jmondi.org
Changes from v2:
- None.
Changes from v1:
- drop explicit linear LUT after finding a proper way to disable gamma correction.
- avoid setting gamma is the CRTC is not active.
- s/int/unsigned int as suggested by Jacopo.
- only enable color management and set gamma size if gamma LUT is supported, suggested by Doug.
- drop the reg-names usage, and instead just use indexed reg specifiers, suggested by Doug.
Changes from RFC:
- Request (an optional) address resource for the LUT.
- Drop support for RK3399, which doesn't seem to work out of the box and needs more research.
- Support pass-thru setting when GAMMA_LUT is NULL.
- Add a check for the gamma size, as suggested by Ilia.
- Move gamma setting to atomic_commit_tail, as pointed out by Jacopo/Laurent, is the correct way.
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 3 + drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 114 ++++++++++++++++++++ drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 7 ++ drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 2 + 4 files changed, 126 insertions(+)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index dba352ec0ee3..fd1d987698ab 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c @@ -17,6 +17,7 @@ #include "rockchip_drm_drv.h" #include "rockchip_drm_fb.h" #include "rockchip_drm_gem.h" +#include "rockchip_drm_vop.h"
static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = { .destroy = drm_gem_fb_destroy, @@ -112,6 +113,8 @@ rockchip_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
drm_atomic_helper_commit_modeset_disables(dev, old_state);
- rockchip_drm_vop_gamma_set(old_state);
Instead of duplicating the commit_tail helper, could you just implement .atomic_begin() and call this from there? I think the only hitch is if you need this to be completed before crtc->atomic_enable(), at which point you might need to call it from vop_crtc_atomic_enable() and then detect that in atomic_begin()
I think moving this to .atomic_begin might be enough. Let me send a new series and we can see how that goes.
Oh, before going forward, pleaste note that the first iteration of this patch (as noted in the changelog) was applying the gamma lut on .atomic_flush. However, Laurent and Jacopo pointed out that it might add some tearing to do so, and that's why it was moved to commit_tail.
I have to admit I'm not too sure about the difference between applying this gamma LUT on atomic_begin or atomic_flush, perhaps you can clarify that?
The only difference between what you have now and calling it in atomic_begin is that as you have it now, it's set before crtc->atomic_enable() is called. I think in order to address Ville's concerns on the other patch, you'll need to set it the lut in .atomic_enable() anyways, so here's what I would suggest:
- Set the LUT in .atomic_enable() wherever it makes sense (you have it at the start now) - Add an .atomic_begin() implementation and check state->color_mgmt_changed and state->active_changed. color_mgmt_changed && !active_changed, set the lut - Remove patches 1 & 5
...I think :-)
Sean
Thanks! Ezequiel