[PATCH v4 03/14] drm/fb-helper: separate the fb_setcmap helper into atomic and legacy paths

[Daniel Vetter]

On Wed, Jul 12, 2017 at 09:47:27AM +0200, Peter Rosin wrote:
> On 2017-07-12 09:03, Daniel Vetter wrote:
> > On Tue, Jul 11, 2017 at 02:12:26PM +0200, Peter Rosin wrote:
> >> On 2017-07-11 10:10, Daniel Vetter wrote:
> >>> Tiny nit you might want to improve (since you need to respin for my naming
> >>> bikeshed of the property_replace_blob function anyway): Properties are
> >>> refcounting and invariant, which means you can just create the property
> >>> once, and then use it for all the CRTC. Slightly cleaner code.
> >>
> >> Yes, I thought about that, but ended up not. The reason is that as far
> >> as I could tell, all involved crtc need not have the same original
> >> gamma_lut. Sure, if all crtc have the same history, that should be the
> >> case, but isn't it possible to tie things together one way first and
> >> set some clut, then "rewire" things so that the crtc no longer have the
> >> same history?
> >>
> >> But if you in the light of that still think it's wise to set the same
> >> clut for all crtc I will factor that part out of the loop.
> > 
> > Blob properties are invariant, if you want to change a lut you _have_ to
> > create a new blob property. They're also reference-counted, which means
> > users of a blob property can come&go as they wish, it will only get freed
> > when the last one is released.
> > 
> > So even when you change the lut of 1 CRTC the other CRTCs will be able to
> > keep using the existing lut blob property unchanged. That's the beauty of
> > having refcounted objects with invariant data over their lifetime, makes a
> > lot of things a lot simpler. drm_framebuffer work the same (only their
> > metadata is invariant, the data of the actual backing storage can change
> > ofc, but not where that backing storage is). Allows you to do simple
> > pointer comparison of objects to check whether their equal or something
> > has changed.
> > 
> > tldr; sharing blobs is perfectly safe and how this is designed to work.
> 
> Yes, I get that, but that wasn't my problem. At all.
> 
> Say that you have a driver with two crtc, A and B. Then this happens:
> 
> 1. A gets a clut with, say, only various red colors.
> 2. B gets a different clut with various green colors.
> 3. Someone ties things up so that one fbdev is used on both A and B.
>    I don't know if this is possible, but if it is, the two crtc now
>    have different cluts.

That's the default for fbdev on top of kms. fbdev doesn't have a concept
of multi-screen. Some things are mapped to the first output only (like
vblank waits).

> 4. Via fbdev, only part of the clut is updated for this A/B combo.
> 
> If A and B starts sharing clut in 4, the part that is not updated is
> clobbered for either crtc A or B.
>   
> (updating only part of the clut is only possible with fbdev, AFAICT)

Meh. Like, completely meh :-)

> Yes, it's a fringe thing to cater to...

Yeah, let's shrug this under the table. fbdev doesn't work for
multi-screen, ending up with strange behaviour is totally fine.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch