| Message ID | ab1c46aac17a9aaeed525167bb30bea7851d9738.1518780268.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jyri, Thank you for the patch. On Friday, 16 February 2018 13:25:04 EET Jyri Sarha wrote: > Add ovl_name() and mgr_name() to dispc_ops and get rid of adhoc names > here and there in the omapdrm code. This moves the names of hardware > entities to omapdss side where they have to be when new omapdss > backend drivers are introduced. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/dss/dispc.c | 23 +++++++++++++++++++++++ > drivers/gpu/drm/omapdrm/dss/omapdss.h | 5 +++++ > drivers/gpu/drm/omapdrm/omap_crtc.c | 11 ++--------- > drivers/gpu/drm/omapdrm/omap_irq.c | 19 +++++++------------ > drivers/gpu/drm/omapdrm/omap_plane.c | 13 +++---------- > 5 files changed, 40 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c > b/drivers/gpu/drm/omapdrm/dss/dispc.c index 338490d..6f83b3e 100644 > --- a/drivers/gpu/drm/omapdrm/dss/dispc.c > +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c > @@ -694,6 +694,26 @@ void dispc_runtime_put(struct dispc_device *dispc) > WARN_ON(r < 0 && r != -ENOSYS); > } > > +static const char *dispc_ovl_name(struct dispc_device *dispc, > + enum omap_plane_id plane) > +{ > + static const char * const ovl_names[] = { > + [OMAP_DSS_GFX] = "GFX", > + [OMAP_DSS_VIDEO1] = "VID1", > + [OMAP_DSS_VIDEO2] = "VID2", > + [OMAP_DSS_VIDEO3] = "VID3", > + [OMAP_DSS_WB] = "WB", > + }; > + > + return ovl_names[plane]; > +} > + > +static const char *dispc_mgr_name(struct dispc_device *dispc, > + enum omap_channel channel) > +{ > + return mgr_desc[channel].name; > +} > + > static u32 dispc_mgr_get_vsync_irq(struct dispc_device *dispc, > enum omap_channel channel) > { > @@ -4662,6 +4682,9 @@ static const struct dispc_ops dispc_ops = { > .get_num_ovls = dispc_get_num_ovls, > .get_num_mgrs = dispc_get_num_mgrs, > > + .ovl_name = dispc_ovl_name, > + .mgr_name = dispc_mgr_name, > + > .get_memory_bandwidth_limit = dispc_get_memory_bandwidth_limit, > > .mgr_enable = dispc_mgr_enable, > diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h > b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 1299dd6..b84cfd8 100644 > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h > @@ -711,6 +711,11 @@ struct dispc_ops { > int (*get_num_ovls)(struct dispc_device *dispc); > int (*get_num_mgrs)(struct dispc_device *dispc); > > + const char *(*ovl_name)(struct dispc_device *dispc, > + enum omap_plane_id plane); > + const char *(*mgr_name)(struct dispc_device *dispc, > + enum omap_channel channel); > + > u32 (*get_memory_bandwidth_limit)(struct dispc_device *dispc); > > void (*mgr_enable)(struct dispc_device *dispc, > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6c4d40b..00ec959 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -672,13 +672,6 @@ static const struct drm_crtc_helper_funcs > omap_crtc_helper_funcs = { * Init and Cleanup > */ > > -static const char *channel_names[] = { > - [OMAP_DSS_CHANNEL_LCD] = "lcd", > - [OMAP_DSS_CHANNEL_DIGIT] = "tv", > - [OMAP_DSS_CHANNEL_LCD2] = "lcd2", > - [OMAP_DSS_CHANNEL_LCD3] = "lcd3", > -}; > - > void omap_crtc_pre_init(struct omap_drm_private *priv) > { > memset(omap_crtcs, 0, sizeof(omap_crtcs)); > @@ -706,7 +699,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, > channel = out->dispc_channel; > omap_dss_put_device(out); > > - DBG("%s", channel_names[channel]); > + DBG("%s", priv->dispc_ops->mgr_name(priv->dispc, channel)); > > /* Multiple displays on same channel is not allowed */ > if (WARN_ON(omap_crtcs[channel] != NULL)) > @@ -721,7 +714,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, > init_waitqueue_head(&omap_crtc->pending_wait); > > omap_crtc->channel = channel; > - omap_crtc->name = channel_names[channel]; > + omap_crtc->name = priv->dispc_ops->mgr_name(priv->dispc, channel); Possibly a small improvement here, you could cache the name in a local variable instead of calling the mgr_name operation twice. > > ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL, > &omap_crtc_funcs, NULL); > diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c > b/drivers/gpu/drm/omapdrm/omap_irq.c index c8511504..5cc88b6 100644 > --- a/drivers/gpu/drm/omapdrm/omap_irq.c > +++ b/drivers/gpu/drm/omapdrm/omap_irq.c > @@ -146,15 +146,10 @@ static void omap_irq_fifo_underflow(struct > omap_drm_private *priv, { > static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL, > DEFAULT_RATELIMIT_BURST); > - static const struct { > - const char *name; > - u32 mask; > - } sources[] = { > - { "gfx", DISPC_IRQ_GFX_FIFO_UNDERFLOW }, > - { "vid1", DISPC_IRQ_VID1_FIFO_UNDERFLOW }, > - { "vid2", DISPC_IRQ_VID2_FIFO_UNDERFLOW }, > - { "vid3", DISPC_IRQ_VID3_FIFO_UNDERFLOW }, > - }; > + static const u32 irqbits[] = { DISPC_IRQ_GFX_FIFO_UNDERFLOW, > + DISPC_IRQ_VID1_FIFO_UNDERFLOW, > + DISPC_IRQ_VID2_FIFO_UNDERFLOW, > + DISPC_IRQ_VID3_FIFO_UNDERFLOW }; The indentation looks weird, I'd write it static const u32 irqbits[] = { DISPC_IRQ_GFX_FIFO_UNDERFLOW, DISPC_IRQ_VID1_FIFO_UNDERFLOW, DISPC_IRQ_VID2_FIFO_UNDERFLOW, DISPC_IRQ_VID3_FIFO_UNDERFLOW, }; > const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW > | DISPC_IRQ_VID1_FIFO_UNDERFLOW > > @@ -174,9 +169,9 @@ static void omap_irq_fifo_underflow(struct > omap_drm_private *priv, > > DRM_ERROR("FIFO underflow on "); > > - for (i = 0; i < ARRAY_SIZE(sources); ++i) { > - if (sources[i].mask & irqstatus) > - pr_cont("%s ", sources[i].name); > + for (i = 0; i < ARRAY_SIZE(irqbits); ++i) { > + if (irqbits[i] & irqstatus) > + pr_cont("%s ", priv->dispc_ops->ovl_name(priv->dispc, i)); I wonder if it's worth it here, in the sense that you're splitting the name and mask, which are both DISPC-specific, in two. Would it make sense to move the whole omap_irq_fifo_underflow() and omap_irq_ocp_error_handler() IRQ handling to the DSS side, as they're not DRM/KMS-related ? > } > > pr_cont("(0x%08x)\n", irqstatus); > diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c > b/drivers/gpu/drm/omapdrm/omap_plane.c index 2899435..61b0753 100644 > --- a/drivers/gpu/drm/omapdrm/omap_plane.c > +++ b/drivers/gpu/drm/omapdrm/omap_plane.c > @@ -239,13 +239,6 @@ static const struct drm_plane_funcs omap_plane_funcs = > { .atomic_get_property = omap_plane_atomic_get_property, > }; > > -static const char *plane_id_to_name[] = { > - [OMAP_DSS_GFX] = "gfx", > - [OMAP_DSS_VIDEO1] = "vid1", > - [OMAP_DSS_VIDEO2] = "vid2", > - [OMAP_DSS_VIDEO3] = "vid3", > -}; > - > static const enum omap_plane_id plane_idx_to_id[] = { > OMAP_DSS_GFX, > OMAP_DSS_VIDEO1, > @@ -272,7 +265,7 @@ struct drm_plane *omap_plane_init(struct drm_device > *dev, > > id = plane_idx_to_id[idx]; > > - DBG("%s: type=%d", plane_id_to_name[id], type); > + DBG("%s: type=%d", priv->dispc_ops->ovl_name(priv->dispc, id), type); > > omap_plane = kzalloc(sizeof(*omap_plane), GFP_KERNEL); > if (!omap_plane) > @@ -282,7 +275,7 @@ struct drm_plane *omap_plane_init(struct drm_device > *dev, for (nformats = 0; formats[nformats]; ++nformats) > ; > omap_plane->id = id; > - omap_plane->name = plane_id_to_name[id]; > + omap_plane->name = priv->dispc_ops->ovl_name(priv->dispc, id); Same here, we could cache the name. > > plane = &omap_plane->base; > > @@ -301,7 +294,7 @@ struct drm_plane *omap_plane_init(struct drm_device > *dev, > > error: > dev_err(dev->dev, "%s(): could not create plane: %s\n", > - __func__, plane_id_to_name[id]); > + __func__, priv->dispc_ops->ovl_name(priv->dispc, id)); You could use omap_plane->name here. > > kfree(omap_plane); > return NULL;
On 27/02/18 16:35, Laurent Pinchart wrote: > the whole omap_irq_fifo_underflow() and omap_irq_ocp_error_handler() IRQ > handling to the DSS side, as they're not DRM/KMS-related ? I think we should react to both errors somehow (I'm not sure how, disable output probably), and that has to be done on the KMS level. We don't do that now, but moving this to DSS side would make error handling more difficult to do in the future. Tomi
Hi Tomi, On Wednesday, 28 February 2018 13:37:48 EET Tomi Valkeinen wrote: > On 27/02/18 16:35, Laurent Pinchart wrote: > > the whole omap_irq_fifo_underflow() and omap_irq_ocp_error_handler() IRQ > > handling to the DSS side, as they're not DRM/KMS-related ? > > I think we should react to both errors somehow (I'm not sure how, > disable output probably), and that has to be done on the KMS level. We > don't do that now, but moving this to DSS side would make error handling > more difficult to do in the future. Ideally I'd demultiplex interrupts on the DSS side and report events to the KMS side (page flip completion, underflows, ...).
On 28/02/18 15:23, Laurent Pinchart wrote: > Hi Tomi, > > On Wednesday, 28 February 2018 13:37:48 EET Tomi Valkeinen wrote: >> On 27/02/18 16:35, Laurent Pinchart wrote: >>> the whole omap_irq_fifo_underflow() and omap_irq_ocp_error_handler() IRQ >>> handling to the DSS side, as they're not DRM/KMS-related ? >> >> I think we should react to both errors somehow (I'm not sure how, >> disable output probably), and that has to be done on the KMS level. We >> don't do that now, but moving this to DSS side would make error handling >> more difficult to do in the future. > > Ideally I'd demultiplex interrupts on the DSS side and report events to the > KMS side (page flip completion, underflows, ...). That's more or less what Jyri's "drm/omap: Make omapdss API more generic" does, isn't it? Or what is the difference with interrupt and event in your mind? Function calls vs status bits? Tomi
Hi Tomi, On Wednesday, 28 February 2018 16:05:34 EET Tomi Valkeinen wrote: > On 28/02/18 15:23, Laurent Pinchart wrote: > > On Wednesday, 28 February 2018 13:37:48 EET Tomi Valkeinen wrote: > >> On 27/02/18 16:35, Laurent Pinchart wrote: > >>> the whole omap_irq_fifo_underflow() and omap_irq_ocp_error_handler() IRQ > >>> handling to the DSS side, as they're not DRM/KMS-related ? > >> > >> I think we should react to both errors somehow (I'm not sure how, > >> disable output probably), and that has to be done on the KMS level. We > >> don't do that now, but moving this to DSS side would make error handling > >> more difficult to do in the future. > > > > Ideally I'd demultiplex interrupts on the DSS side and report events to > > the KMS side (page flip completion, underflows, ...). > > That's more or less what Jyri's "drm/omap: Make omapdss API more > generic" does, isn't it? Or what is the difference with interrupt and > event in your mind? Function calls vs status bits? Yes, that's the difference in my mind. I'd keep the status bits on the DSS side. We don't have to implement one callback function for each status bit, we could translate them into abstract event bits that are not specific to a particular DSS version. What I'd like to avoid is omapdrm calling into omapdss to retrieve a name for a bit that is DSS-specific. If we want hardware names in debug messages I think they should be printed on the omapdss side, and if we want to handle status bits on the omapdrm side they shouldn't require hardware names.
On 28/02/18 16:24, Laurent Pinchart wrote: > Hi Tomi, > > On Wednesday, 28 February 2018 16:05:34 EET Tomi Valkeinen wrote: >> On 28/02/18 15:23, Laurent Pinchart wrote: >>> On Wednesday, 28 February 2018 13:37:48 EET Tomi Valkeinen wrote: >>>> On 27/02/18 16:35, Laurent Pinchart wrote: >>>>> the whole omap_irq_fifo_underflow() and omap_irq_ocp_error_handler() IRQ >>>>> handling to the DSS side, as they're not DRM/KMS-related ? >>>> >>>> I think we should react to both errors somehow (I'm not sure how, >>>> disable output probably), and that has to be done on the KMS level. We >>>> don't do that now, but moving this to DSS side would make error handling >>>> more difficult to do in the future. >>> >>> Ideally I'd demultiplex interrupts on the DSS side and report events to >>> the KMS side (page flip completion, underflows, ...). >> >> That's more or less what Jyri's "drm/omap: Make omapdss API more >> generic" does, isn't it? Or what is the difference with interrupt and >> event in your mind? Function calls vs status bits? > > Yes, that's the difference in my mind. I'd keep the status bits on the DSS > side. We don't have to implement one callback function for each status bit, we > could translate them into abstract event bits that are not specific to a > particular DSS version. What I'd like to avoid is omapdrm calling into omapdss > to retrieve a name for a bit that is DSS-specific. If we want hardware names > in debug messages I think they should be printed on the omapdss side, and if > we want to handle status bits on the omapdrm side they shouldn't require > hardware names. Ok, yes, I see your point, and agree. Here, I think what's done (after the IRQ change patch) is that we get a an abstracted bit for the underflow. Omapdrm can map that bit to an omap_plane, and should get the name from omap_plane, instead of asking it directly from dispc. Tomi
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 338490d..6f83b3e 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -694,6 +694,26 @@ void dispc_runtime_put(struct dispc_device *dispc) WARN_ON(r < 0 && r != -ENOSYS); } +static const char *dispc_ovl_name(struct dispc_device *dispc, + enum omap_plane_id plane) +{ + static const char * const ovl_names[] = { + [OMAP_DSS_GFX] = "GFX", + [OMAP_DSS_VIDEO1] = "VID1", + [OMAP_DSS_VIDEO2] = "VID2", + [OMAP_DSS_VIDEO3] = "VID3", + [OMAP_DSS_WB] = "WB", + }; + + return ovl_names[plane]; +} + +static const char *dispc_mgr_name(struct dispc_device *dispc, + enum omap_channel channel) +{ + return mgr_desc[channel].name; +} + static u32 dispc_mgr_get_vsync_irq(struct dispc_device *dispc, enum omap_channel channel) { @@ -4662,6 +4682,9 @@ static const struct dispc_ops dispc_ops = { .get_num_ovls = dispc_get_num_ovls, .get_num_mgrs = dispc_get_num_mgrs, + .ovl_name = dispc_ovl_name, + .mgr_name = dispc_mgr_name, + .get_memory_bandwidth_limit = dispc_get_memory_bandwidth_limit, .mgr_enable = dispc_mgr_enable, diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h index 1299dd6..b84cfd8 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -711,6 +711,11 @@ struct dispc_ops { int (*get_num_ovls)(struct dispc_device *dispc); int (*get_num_mgrs)(struct dispc_device *dispc); + const char *(*ovl_name)(struct dispc_device *dispc, + enum omap_plane_id plane); + const char *(*mgr_name)(struct dispc_device *dispc, + enum omap_channel channel); + u32 (*get_memory_bandwidth_limit)(struct dispc_device *dispc); void (*mgr_enable)(struct dispc_device *dispc, diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 6c4d40b..00ec959 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -672,13 +672,6 @@ static const struct drm_crtc_helper_funcs omap_crtc_helper_funcs = { * Init and Cleanup */ -static const char *channel_names[] = { - [OMAP_DSS_CHANNEL_LCD] = "lcd", - [OMAP_DSS_CHANNEL_DIGIT] = "tv", - [OMAP_DSS_CHANNEL_LCD2] = "lcd2", - [OMAP_DSS_CHANNEL_LCD3] = "lcd3", -}; - void omap_crtc_pre_init(struct omap_drm_private *priv) { memset(omap_crtcs, 0, sizeof(omap_crtcs)); @@ -706,7 +699,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, channel = out->dispc_channel; omap_dss_put_device(out); - DBG("%s", channel_names[channel]); + DBG("%s", priv->dispc_ops->mgr_name(priv->dispc, channel)); /* Multiple displays on same channel is not allowed */ if (WARN_ON(omap_crtcs[channel] != NULL)) @@ -721,7 +714,7 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, init_waitqueue_head(&omap_crtc->pending_wait); omap_crtc->channel = channel; - omap_crtc->name = channel_names[channel]; + omap_crtc->name = priv->dispc_ops->mgr_name(priv->dispc, channel); ret = drm_crtc_init_with_planes(dev, crtc, plane, NULL, &omap_crtc_funcs, NULL); diff --git a/drivers/gpu/drm/omapdrm/omap_irq.c b/drivers/gpu/drm/omapdrm/omap_irq.c index c8511504..5cc88b6 100644 --- a/drivers/gpu/drm/omapdrm/omap_irq.c +++ b/drivers/gpu/drm/omapdrm/omap_irq.c @@ -146,15 +146,10 @@ static void omap_irq_fifo_underflow(struct omap_drm_private *priv, { static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL, DEFAULT_RATELIMIT_BURST); - static const struct { - const char *name; - u32 mask; - } sources[] = { - { "gfx", DISPC_IRQ_GFX_FIFO_UNDERFLOW }, - { "vid1", DISPC_IRQ_VID1_FIFO_UNDERFLOW }, - { "vid2", DISPC_IRQ_VID2_FIFO_UNDERFLOW }, - { "vid3", DISPC_IRQ_VID3_FIFO_UNDERFLOW }, - }; + static const u32 irqbits[] = { DISPC_IRQ_GFX_FIFO_UNDERFLOW, + DISPC_IRQ_VID1_FIFO_UNDERFLOW, + DISPC_IRQ_VID2_FIFO_UNDERFLOW, + DISPC_IRQ_VID3_FIFO_UNDERFLOW }; const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW | DISPC_IRQ_VID1_FIFO_UNDERFLOW @@ -174,9 +169,9 @@ static void omap_irq_fifo_underflow(struct omap_drm_private *priv, DRM_ERROR("FIFO underflow on "); - for (i = 0; i < ARRAY_SIZE(sources); ++i) { - if (sources[i].mask & irqstatus) - pr_cont("%s ", sources[i].name); + for (i = 0; i < ARRAY_SIZE(irqbits); ++i) { + if (irqbits[i] & irqstatus) + pr_cont("%s ", priv->dispc_ops->ovl_name(priv->dispc, i)); } pr_cont("(0x%08x)\n", irqstatus); diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 2899435..61b0753 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -239,13 +239,6 @@ static const struct drm_plane_funcs omap_plane_funcs = { .atomic_get_property = omap_plane_atomic_get_property, }; -static const char *plane_id_to_name[] = { - [OMAP_DSS_GFX] = "gfx", - [OMAP_DSS_VIDEO1] = "vid1", - [OMAP_DSS_VIDEO2] = "vid2", - [OMAP_DSS_VIDEO3] = "vid3", -}; - static const enum omap_plane_id plane_idx_to_id[] = { OMAP_DSS_GFX, OMAP_DSS_VIDEO1, @@ -272,7 +265,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, id = plane_idx_to_id[idx]; - DBG("%s: type=%d", plane_id_to_name[id], type); + DBG("%s: type=%d", priv->dispc_ops->ovl_name(priv->dispc, id), type); omap_plane = kzalloc(sizeof(*omap_plane), GFP_KERNEL); if (!omap_plane) @@ -282,7 +275,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, for (nformats = 0; formats[nformats]; ++nformats) ; omap_plane->id = id; - omap_plane->name = plane_id_to_name[id]; + omap_plane->name = priv->dispc_ops->ovl_name(priv->dispc, id); plane = &omap_plane->base; @@ -301,7 +294,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, error: dev_err(dev->dev, "%s(): could not create plane: %s\n", - __func__, plane_id_to_name[id]); + __func__, priv->dispc_ops->ovl_name(priv->dispc, id)); kfree(omap_plane); return NULL;