[PATCH] drm/rockchip: Require the YTR modifier for AFBC

[Brian Starkey]

On Tue, Feb 23, 2021 at 05:10:16PM +0000, Alyssa Rosenzweig wrote:
> > If YTR can't be turned off, then according to the AFBC spec - correct.
> 
> There is no public AFBC spec, so I'm not sure how to respond to this.
> 
> > If the hardware allows it to be configured to use YTR with other
> > component orders, I don't know exactly what the impact would be -
> > maybe nothing.
> 
> It's legal to use YTR with a BGR framebuffer regardless of the content
> of the framebuffer, yes? Could I postprocess with the following shader?
> 
> 	void main() {
> 		vec4 colour = ....;
> 		gl_FragColor = colour.bgra;
> 	}
> 
> That's just 3D rendering. But now if I feed that rendered "BGR"
> framebuffer in I get the illusion of RGB out.
> 
> Doing the colourspace conversion in hardware (with the
> GPU's component reordering) is mathematically indistinguishable from
> rendering BGR with that shader.

I agree that *if* all 2^24 possible values of BGR 8:8:8 can be
perfectly and "invert-ably" converted, then it shouldn't matter what
color channel is in which slot, mathematically speaking.

> 
> I sympathize with reducing AFBC's combinatoric explosion, and I realize
> that the Rockchip VOP is probably wrong. I also realize that the
> transform is defined for BGR inputs, not RGB ones, so it might be
> slightly less effective for real content.

Yes, I think the practical impact is likely to be compression
efficiency.

> But it seems to me allowing
> both BGR+YTR and RGB+YTR upstream is the better route than simply
> preventing hardware from using AFBC at all, and there are natural
> encodings for both with fourcc modifiers.
> 

Are those the only options? I see XBGR8888, ABGR8888, BGR888 and
BGR565 are all handled in vop_convert_afbc_format(), which are all
"valid" for use with YTR, and all except XBGR are on the "preferred"
AFBC format list in afbc.rst.

> Maybe there's a deeper reason to require BGR that I'm missing? Please
> let me know if I've misunderstood, I only know AFBC from the GPU side.

If deeper reason means "a reason that the chip will explode if you use
RGB" then I'm not aware of one.

As you've said above, combinatoric explosion is a real issue with
AFBC, so there's a certain subset which Arm has declared "valid", and
an even smaller subset which forms part of conformance testing
(...also not public). The "valid" usage, and a set of preferred
formats are documented in afbc.rst, which is the closest thing I have
to a public spec.

I was _hoping_ we could limit upstream implementations to at least the
subset which Arm declares "valid", with a preference for the subset
which forms part of the conformance set - because those are the ones
likely to be supported across the widest set of hardware. *But*, I'm
also very aware that waving around "invisible" specifications doesn't
carry much weight upstream versus actual code and hardware _doing
stuff_.

So, all I can really do here is say "spec says this isn't allowed",
and make the point that I expect (though I don't 100% know for sure)
all hardware that supports AFBC to also support BGR order AFBC. With
that in mind, if I were trying to implement AFBC in a way that's
likely to work across multiple platforms, I'd go with BGR order.

Thanks,
-Brian