Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops

[Tomi Valkeinen]

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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH RFC 3/9] drm/omap: Add ovl_name() and mgr_name() to dispc_ops

[Laurent Pinchart]

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 
> Reviewed-by: Tomi Valkeinen 
> ---
>  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(_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,
>   _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,