Message-ID: <20171221141025.GA18544@ulmo>
Date:   Thu, 21 Dec 2017 15:10:25 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Dmitry Osipenko <digetx@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/5] drm/tegra: Restore opaque and drop alpha formats
 on Tegra20/30

On Thu, Dec 21, 2017 at 01:38:31AM +0300, Dmitry Osipenko wrote:
> On 21.12.2017 01:23, Dmitry Osipenko wrote:
> > On 21.12.2017 01:02, Thierry Reding wrote:
> >> On Thu, Dec 21, 2017 at 12:05:40AM +0300, Dmitry Osipenko wrote:
> >>> On 20.12.2017 23:16, Thierry Reding wrote:
> >>>> On Wed, Dec 20, 2017 at 11:01:49PM +0300, Dmitry Osipenko wrote:
> >>>>> On 20.12.2017 21:01, Thierry Reding wrote:
> >>>>>> On Wed, Dec 20, 2017 at 06:46:11PM +0300, Dmitry Osipenko wrote:
> >>>>>>> Commit 7772fdaef939 ("drm/tegra: Support ARGB and ABGR formats") broke
> >>>>>>> DRM's MODE_ADDFB IOCTL on Tegra20/30, because IOCTL uses XRGB format if
> >>>>>>> requested FB depth is 24bpp. As a result, Xorg doesn't work anymore with
> >>>>>>> both modesetting and opentegra drivers. On older Tegra's each plane has
> >>>>>>> a blending configuration which should be used to enable / disable alpha
> >>>>>>> blending and right now the blending configs are hardcoded to disabled
> >>>>>>> alpha blending. In order to support alpha formats properly, planes
> >>>>>>> blending configuration must be adjusted, until then the alpha formats
> >>>>>>> are equal to non-alpha.
> >>>>>>>
> >>>>>>> Fixes: 7772fdaef939 ("drm/tegra: Support ARGB and ABGR formats")
> >>>>>>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
> >>>>>>> ---
> >>>>>>>  drivers/gpu/drm/tegra/dc.c    | 29 ++++++++++++++++++-----------
> >>>>>>>  drivers/gpu/drm/tegra/dc.h    |  1 +
> >>>>>>>  drivers/gpu/drm/tegra/fb.c    | 13 -------------
> >>>>>>>  drivers/gpu/drm/tegra/hub.c   |  3 ++-
> >>>>>>>  drivers/gpu/drm/tegra/plane.c | 22 +++++++++++++++++-----
> >>>>>>>  drivers/gpu/drm/tegra/plane.h |  2 +-
> >>>>>>>  6 files changed, 39 insertions(+), 31 deletions(-)
> >>>>>>
> >>>>>> This kept bugging me, so I spent some time looking at the blending
> >>>>>> programming. I came up with the attached patch which seems to work
> >>>>>> for all scenarios and is fairly similar to your patch. It has the
> >>>>>> added benefit that we can keep support for more formats.
> >>>>>>
> >>>>>> Any comments?
> >>>>>>
> >>>>>> Thierry
> >>>>>> --- >8 ---
> >>>>>> From 3d2b7d1a9b8239dc6940477d8783461ac60783bc Mon Sep 17 00:00:00 2001
> >>>>>> From: Thierry Reding <treding@...dia.com>
> >>>>>> Date: Wed, 20 Dec 2017 09:39:14 +0100
> >>>>>> Subject: [PATCH] drm/tegra: dc: Implement legacy blending
> >>>>>>
> >>>>>> This implements alpha blending on legacy display controllers (Tegra20,
> >>>>>> Tegra30 and Tegra114). While it's theoretically possible to support the
> >>>>>> zpos property to enable userspace to specify the Z-order of each plane
> >>>>>> individually, this is not currently supported and the same fixed Z-
> >>>>>> order as previously defined is used.
> >>>>>
> >>>>> Perhaps one variant of implementing zpos could be by making overlays 'virtual',
> >>>>> so each virtual overlay will be backed by the real HW plane and we could swap
> >>>>> the HW planes of the virtual overlays, emulating zpos.
> >>>>>
> >>>>>> Reverts commit 71835caa00e8 ("drm/tegra: fb: Force alpha formats") since
> >>>>>> the opaque formats are now supported.
> >>>>>>
> >>>>>> Reported-by: Dmitry Osipenko <digetx@...il.com>
> >>>>>> Fixes: 7772fdaef939 ("drm/tegra: Support ARGB and ABGR formats")
> >>>>>> Signed-off-by: Thierry Reding <treding@...dia.com>
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/tegra/dc.c    | 74 ++++++++++++++++++++++++++++++++++---------
> >>>>>>  drivers/gpu/drm/tegra/dc.h    | 13 ++++++++
> >>>>>>  drivers/gpu/drm/tegra/fb.c    | 12 -------
> >>>>>>  drivers/gpu/drm/tegra/plane.c | 41 ++++++++++++++++++++++++
> >>>>>>  drivers/gpu/drm/tegra/plane.h |  3 ++
> >>>>>>  5 files changed, 116 insertions(+), 27 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> >>>>>> index bc65c314e00f..07c687d7f615 100644
> >>>>>> --- a/drivers/gpu/drm/tegra/dc.c
> >>>>>> +++ b/drivers/gpu/drm/tegra/dc.c
> >>>>>> @@ -168,32 +168,46 @@ static inline u32 compute_initial_dda(unsigned int in)
> >>>>>>  	return dfixed_frac(inf);
> >>>>>>  }
> >>>>>>  
> >>>>>> -static void tegra_plane_setup_blending_legacy(struct tegra_plane *plane)
> >>>>>> +static void
> >>>>>> +tegra_plane_setup_blending_legacy(struct tegra_plane *plane,
> >>>>>> +				  const struct tegra_dc_window *window)
> >>>>>>  {
> >>>>>> +	u32 foreground = BLEND_WEIGHT1(255) | BLEND_WEIGHT0(255) |
> >>>>>> +			 BLEND_COLOR_KEY_NONE;
> >>>>>> +	u32 background = BLEND_WEIGHT1(0) | BLEND_WEIGHT0(0) |
> >>>>>> +			 BLEND_COLOR_KEY_NONE;
> >>>>>> +	u32 blendnokey = BLEND_WEIGHT1(255) | BLEND_WEIGHT0(255);
> >>>>>> +
> >>>>>> +	/* enable alpha blending if window->alpha */
> >>>>>> +	if (window->alpha) {
> >>>>>> +		background |= BLEND_CONTROL_DEPENDENT;
> >>>>>> +		foreground |= BLEND_CONTROL_ALPHA;
> >>>>>> +	}
> >>>>>
> >>>>> I think dependent weight means that window doesn't have alpha transparency. So
> >>>>> we should set the dependent_weight mode for opaque formats and alpha_weight for
> >>>>> formats with alpha channel.
> >>>>
> >>>> According to the TRM, dependent weight means that its weight will be 1 -
> >>>> the sum of the other overlapping windows. And it certainly seems to work
> >>>> that way in my tests (see below).
> >>>
> >>> Yes, and you are hardcoding the blending modes regardless of the actual plane
> >>> format. So even if underlying window has alpha format, it will be treated as it
> >>> is opaque.
> >>
> >> Ah yes, indeed. The patch currently assumes that if the current plane
> >> has an alpha component, then all the others will, too. That can probably
> >> be done fairly easily by looking at the current atomic state and
> >> inspecting the pixel format for each plane on the same CRTC. Let me take
> >> a stab at that tomorrow.
> > 
> > Okay, please push patch to the public repo once it is ready and let me know.
> > I'll rebase cursor patch on it.
> > 
> >> I'm wondering if we can deal with zpos, too, if we're already going
> >> through the trouble of looking at all planes involved. I think we can
> >> simply permute the WIN_BLEND register writes depending on the Z-order
> >> to implement proper zpos support.
> > I think we will have to permute the whole window state, not only the WIN_BLEND
> > register.
> > 
> > Also I'm not sure how to make topmost window opaque and underlying windows
> > transparent, will have to check how blending works. Maybe it not possible at all..
> 
> Although it is simple to implement using the fixed weights for 2WIN cases, but
> then we will have to enhance the legacy blending code a tad to handle this case
> properly. You'll have to do quite a lot for tomorrow ;) Alternatively you can
> still apply my patch that drops alpha formats on T20/30 and we will try to
> implement alpha + zpos for 4.17.

I think I finally nailed this. Just sent out a v2 and updated my test
program to use different formats for each plane. I tested all opaque &
alpha combinations and it all looks correct now.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists