[permalink] [raw]

Subject: [PATCH v4 10/14] drm: mgag200: remove dead code and pointless local lut storage

2017-07-06 12:20:22

by Peter Rosin

[permalink] [raw]

Subject: [PATCH v4 08/14] drm: gma500: remove dead code and pointless local lut storage

The redundant fb helpers .gamma_set and .gamma_get are no longer
used. Remove the dead code and hook up the crtc .gamma_set
to use the crtc gamma_store directly instead of duplicating that
info locally.

Signed-off-by: Peter Rosin <[email protected]>
---
drivers/gpu/drm/gma500/framebuffer.c | 22 --------------------
drivers/gpu/drm/gma500/gma_display.c | 32 ++++++++++--------------------
drivers/gpu/drm/gma500/psb_intel_display.c | 7 +------
drivers/gpu/drm/gma500/psb_intel_drv.h | 1 -
4 files changed, 12 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
index 7da70b6..2570c7f 100644
--- a/drivers/gpu/drm/gma500/framebuffer.c
+++ b/drivers/gpu/drm/gma500/framebuffer.c
@@ -479,26 +479,6 @@ static struct drm_framebuffer *psb_user_framebuffer_create
return psb_framebuffer_create(dev, cmd, r);
}

-static void psbfb_gamma_set(struct drm_crtc *crtc, u16 red, u16 green,
- u16 blue, int regno)
-{
- struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
-
- gma_crtc->lut_r[regno] = red >> 8;
- gma_crtc->lut_g[regno] = green >> 8;
- gma_crtc->lut_b[regno] = blue >> 8;
-}
-
-static void psbfb_gamma_get(struct drm_crtc *crtc, u16 *red,
- u16 *green, u16 *blue, int regno)
-{
- struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
-
- *red = gma_crtc->lut_r[regno] << 8;
- *green = gma_crtc->lut_g[regno] << 8;
- *blue = gma_crtc->lut_b[regno] << 8;
-}
-
static int psbfb_probe(struct drm_fb_helper *helper,
struct drm_fb_helper_surface_size *sizes)
{
@@ -525,8 +505,6 @@ static int psbfb_probe(struct drm_fb_helper *helper,
}

static const struct drm_fb_helper_funcs psb_fb_helper_funcs = {
- .gamma_set = psbfb_gamma_set,
- .gamma_get = psbfb_gamma_get,
.fb_probe = psbfb_probe,
};

diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c
index e7fd356..f3c48a2 100644
--- a/drivers/gpu/drm/gma500/gma_display.c
+++ b/drivers/gpu/drm/gma500/gma_display.c
@@ -144,33 +144,32 @@ void gma_crtc_load_lut(struct drm_crtc *crtc)
struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
const struct psb_offset *map = &dev_priv->regmap[gma_crtc->pipe];
int palreg = map->palette;
+ u16 *r, *g, *b;
int i;

/* The clocks have to be on to load the palette. */
if (!crtc->enabled)
return;

+ r = crtc->gamma_store;
+ g = r + crtc->gamma_size;
+ b = g + crtc->gamma_size;
+
if (gma_power_begin(dev, false)) {
for (i = 0; i < 256; i++) {
REG_WRITE(palreg + 4 * i,
- ((gma_crtc->lut_r[i] +
- gma_crtc->lut_adj[i]) << 16) |
- ((gma_crtc->lut_g[i] +
- gma_crtc->lut_adj[i]) << 8) |
- (gma_crtc->lut_b[i] +
- gma_crtc->lut_adj[i]));
+ (((*r++ >> 8) + gma_crtc->lut_adj[i]) << 16) |
+ (((*g++ >> 8) + gma_crtc->lut_adj[i]) << 8) |
+ ((*b++ >> 8) + gma_crtc->lut_adj[i]));
}
gma_power_end(dev);
} else {
for (i = 0; i < 256; i++) {
/* FIXME: Why pipe[0] and not pipe[..._crtc->pipe]? */
dev_priv->regs.pipe[0].palette[i] =
- ((gma_crtc->lut_r[i] +
- gma_crtc->lut_adj[i]) << 16) |
- ((gma_crtc->lut_g[i] +
- gma_crtc->lut_adj[i]) << 8) |
- (gma_crtc->lut_b[i] +
- gma_crtc->lut_adj[i]);
+ (((*r++ >> 8) + gma_crtc->lut_adj[i]) << 16) |
+ (((*g++ >> 8) + gma_crtc->lut_adj[i]) << 8) |
+ ((*b++ >> 8) + gma_crtc->lut_adj[i]);
}

}
@@ -180,15 +179,6 @@ int gma_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green, u16 *blue,
u32 size,
struct drm_modeset_acquire_ctx *ctx)
{
- struct gma_crtc *gma_crtc = to_gma_crtc(crtc);
- int i;
-
- for (i = 0; i < size; i++) {
- gma_crtc->lut_r[i] = red[i] >> 8;
- gma_crtc->lut_g[i] = green[i] >> 8;
- gma_crtc->lut_b[i] = blue[i] >> 8;
- }
-
gma_crtc_load_lut(crtc);

return 0;
diff --git a/drivers/gpu/drm/gma500/psb_intel_display.c b/drivers/gpu/drm/gma500/psb_intel_display.c
index 7b6c849..8762efa 100644
--- a/drivers/gpu/drm/gma500/psb_intel_display.c
+++ b/drivers/gpu/drm/gma500/psb_intel_display.c
@@ -518,13 +518,8 @@ void psb_intel_crtc_init(struct drm_device *dev, int pipe,
gma_crtc->pipe = pipe;
gma_crtc->plane = pipe;

- for (i = 0; i < 256; i++) {
- gma_crtc->lut_r[i] = i;
- gma_crtc->lut_g[i] = i;
- gma_crtc->lut_b[i] = i;
-
+ for (i = 0; i < 256; i++)
gma_crtc->lut_adj[i] = 0;
- }

gma_crtc->mode_dev = mode_dev;
gma_crtc->cursor_addr = 0;
diff --git a/drivers/gpu/drm/gma500/psb_intel_drv.h b/drivers/gpu/drm/gma500/psb_intel_drv.h
index 6a10215..e8e4ea1 100644
--- a/drivers/gpu/drm/gma500/psb_intel_drv.h
+++ b/drivers/gpu/drm/gma500/psb_intel_drv.h
@@ -172,7 +172,6 @@ struct gma_crtc {
int plane;
uint32_t cursor_addr;
struct gtt_range *cursor_gt;
- u8 lut_r[256], lut_g[256], lut_b[256];
u8 lut_adj[256];
struct psb_intel_framebuffer *fbdev_fb;
/* a mode_set for fbdev users on this crtc */
--
2.1.4

2017-07-06 12:20:32

by Peter Rosin

[permalink] [raw]

2017-07-06 12:21:31

by Peter Rosin

[permalink] [raw]

Subject: [PATCH v4 09/14] drm: i915: remove dead code and pointless local lut storage

The driver stores lut values from the fbdev interface, and is able
to give them back, but does not appear to do anything with these
lut values. The generic fb helpers have replaced this function,
and may even have made the driver work for the C8 mode from the
fbdev interface. But that is untested.

Since the fb helpers .gamma_set and .gamma_get are obsolete,
remove the dead code.

Signed-off-by: Peter Rosin <[email protected]>
---
drivers/gpu/drm/i915/intel_drv.h | 1 -
drivers/gpu/drm/i915/intel_fbdev.c | 31 -------------------------------
2 files changed, 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d93efb4..bc7bfa0 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -786,7 +786,6 @@ struct intel_crtc {
struct drm_crtc base;
enum pipe pipe;
enum plane plane;
- u8 lut_r[256], lut_g[256], lut_b[256];
/*
* Whether the crtc and the connected output pipeline is active. Implies
* that crtc->enabled is set, i.e. the current mode configuration has
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 460ca0b..19d650b 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -281,27 +281,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
return ret;
}

-/** Sets the color ramps on behalf of RandR */
-static void intel_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16 green,
- u16 blue, int regno)
-{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
- intel_crtc->lut_r[regno] = red >> 8;
- intel_crtc->lut_g[regno] = green >> 8;
- intel_crtc->lut_b[regno] = blue >> 8;
-}
-
-static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
- u16 *blue, int regno)
-{
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-
- *red = intel_crtc->lut_r[regno] << 8;
- *green = intel_crtc->lut_g[regno] << 8;
- *blue = intel_crtc->lut_b[regno] << 8;
-}
-
static struct drm_fb_helper_crtc *
intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
{
@@ -376,7 +355,6 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
struct drm_connector *connector;
struct drm_encoder *encoder;
struct drm_fb_helper_crtc *new_crtc;
- struct intel_crtc *intel_crtc;

fb_conn = fb_helper->connector_info[i];
connector = fb_conn->connector;
@@ -418,13 +396,6 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,

num_connectors_enabled++;

- intel_crtc = to_intel_crtc(connector->state->crtc);
- for (j = 0; j < 256; j++) {
- intel_crtc->lut_r[j] = j;
- intel_crtc->lut_g[j] = j;
- intel_crtc->lut_b[j] = j;
- }
-
new_crtc = intel_fb_helper_crtc(fb_helper,
connector->state->crtc);

@@ -527,8 +498,6 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,

static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {
.initial_config = intel_fb_initial_config,
- .gamma_set = intel_crtc_fb_gamma_set,
- .gamma_get = intel_crtc_fb_gamma_get,
.fb_probe = intelfb_create,
};

--
2.1.4

2017-07-06 12:20:14

by Peter Rosin

[permalink] [raw]

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.
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)

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

Cheers,
Peter

2017-07-13 12:42:22

by Daniel Vetter

[permalink] [raw]

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

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