| Message ID | 1520300919-103427-3-git-send-email-stschake@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote: > 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 <stschake@gmail.com> > --- > 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; Looks to me like that should really be a bitmask (or something similar) in the crtc state. > }; > > 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
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote: >> 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 <stschake@gmail.com> >> --- >> 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; > > Looks to me like that should really be a bitmask (or something similar) > in the crtc state. Why? In particular, VC4 really doesn't have a fixed number of planes, and the fact that we're exposing just a handful so far is probably a bug.
On Tue, Mar 06, 2018 at 04:10:33PM -0800, Eric Anholt wrote: > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > > On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote: > >> 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 <stschake@gmail.com> > >> --- > >> 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; > > > > Looks to me like that should really be a bitmask (or something similar) > > in the crtc state. > > Why? > > In particular, VC4 really doesn't have a fixed number of planes, and the > fact that we're exposing just a handful so far is probably a bug. The problem is that this seems to clobber the device state from the .atomic_check() hook. So if you do a CHECK_ONLY atomic ioctl (or some later check simply fails and the operation is aborted) you've already modified the state of the device, and some later operation may then end up doing the wrong thing. I guess you could track this in the plane struct like here, but as with the actual hardware state that shouldn't get modified until we're sure the checked state is really meant to be commited to the hardware.
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Tue, Mar 06, 2018 at 04:10:33PM -0800, Eric Anholt wrote: >> Ville Syrjälä <ville.syrjala@linux.intel.com> writes: >> >> > On Tue, Mar 06, 2018 at 02:48:38AM +0100, Stefan Schake wrote: >> >> 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 <stschake@gmail.com> >> >> --- >> >> 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; >> > >> > Looks to me like that should really be a bitmask (or something similar) >> > in the crtc state. >> >> Why? >> >> In particular, VC4 really doesn't have a fixed number of planes, and the >> fact that we're exposing just a handful so far is probably a bug. > > The problem is that this seems to clobber the device state from the > .atomic_check() hook. So if you do a CHECK_ONLY atomic ioctl (or > some later check simply fails and the operation is aborted) you've > already modified the state of the device, and some later operation > may then end up doing the wrong thing. > > I guess you could track this in the plane struct like here, but as > with the actual hardware state that shouldn't get modified until > we're sure the checked state is really meant to be commited to the > hardware. Oh, I hadn't noticed it was in vc4_plane, not vc4_plane_state. Yeah, it should be in the plane state.
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; }
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 <stschake@gmail.com> --- drivers/gpu/drm/vc4/vc4_drv.h | 6 ++++++ drivers/gpu/drm/vc4/vc4_plane.c | 15 ++++++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-)