Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On Mon, Aug 17, 2015 at 07:17:53PM +0100, Jon Medhurst (Tixy) wrote:
> I haven't reviewed the code in detail, just had one comment I alluded to
> in a private email the other day...
>
> On Wed, 2015-08-05 at 15:28 +0100, Liviu Dudau wrote:
>
> > diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
> > b/drivers/gpu/drm/arm/hdlcd_crtc.c
> [...]
> > +void hdlcd_set_scanout(struct hdlcd_drm_private *hdlcd)
> > +{
> > + struct drm_framebuffer *fb = hdlcd->crtc.primary->fb;
> > + struct drm_gem_cma_object *gem;
> > + unsigned int depth, bpp;
> > + dma_addr_t scanout_start;
> > +
> > + drm_fb_get_bpp_depth(fb->pixel_format, , );
> > + gem = drm_fb_cma_get_gem_obj(fb, 0);
> > +
> > + scanout_start = gem->paddr + fb->offsets[0] +
> > + (hdlcd->crtc.y * fb->pitches[0]) + (hdlcd->crtc.x * bpp/8);
> > +
> > + hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
> > +}
> > +
>
> The above function accesses various pointers and values, which
> presumably all need to be valid and consistent. However...
>
> [...]
> > diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c
> > b/drivers/gpu/drm/arm/hdlcd_drv.c
> [...]
> > +static irqreturn_t hdlcd_irq(int irq, void *arg)
> > +{
> > + struct drm_device *dev = arg;
> > + struct hdlcd_drm_private *hdlcd = dev->dev_private;
> > + unsigned long irq_status;
> > +
> > + irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
> > +
> > +#ifdef CONFIG_DEBUG_FS
> > + if (irq_status & HDLCD_INTERRUPT_UNDERRUN) {
> > + atomic_inc(>buffer_underrun_count);
> > + }
> > + if (irq_status & HDLCD_INTERRUPT_DMA_END) {
> > + atomic_inc(>dma_end_count);
> > + }
> > + if (irq_status & HDLCD_INTERRUPT_BUS_ERROR) {
> > + atomic_inc(>bus_error_count);
> > + }
> > + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> > + atomic_inc(>vsync_count);
> > + }
> > +#endif
> > + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> > + struct drm_pending_vblank_event *event;
> > + unsigned long flags;
> > +
> > + hdlcd_set_scanout(hdlcd);
>
> hdlcd_set_scanout is being called on every vsync interrupt, can we
> guarantee that is safe? What if we're in the middle of a page flip or
> panning operation? Seems to me that there is at least scope for
> incorrect addresses being calculated leading to a nasty glitch on the
> screen for one frame. And in the worst case, possibly invalid pointer
> being dereferenced.
Agree, there is a risk of corruption here. I'm going to look at the atomic
mode setting which should hopefully resolve most of these issues.
>
> So, if scanout needs to be set on every vsync, would it not be safer
> (and more efficient) to have a single variable storing the value to use
> during interrupts, and recalculate that value outside of interrupts
> whenever things are changed?
hdlcd_set_scanout() function is merely a convenience function to calculate
the scanout_start variable. The interrupt handler probably doesn't need to
call that and it was mostly a shortcut to make sure the HDLCD_REG_FB_BASE
register was up-to-date when the vsync interrupt happened. I hope the
atomic modeset will cleanup things here.
Best regards,
Liviu
>
> --
> Tixy
>
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On Tue, Aug 18, 2015 at 05:41:59PM +0100, Emil Velikov wrote: > On 17 August 2015 at 16:15, Liviu Dudau wrote: > > On Sun, Aug 16, 2015 at 09:56:33AM +0100, Emil Velikov wrote: > >> Hi Liviu, > > > > Hi Emil, > > > >> > >> On 5 August 2015 at 15:28, Liviu Dudau wrote: > >> > The HDLCD controller is a display controller that supports resolutions > >> > up to 4096x4096 pixels. It is present on various development boards > >> > produced by ARM Ltd and emulated by the latest Fast Models from the > >> > company. > >> > > >> I believe there is a unofficial requirement(?) for new drm drivers to > >> use atomic modesetting. Not 100% sure on this one though. The > >> following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] > >> [2] cat provide some information on the topic. > > > > I am also interested to know if this is a requirement or not. > > Thanks for the pointers, I will review them again to see if I did miss > > anything. I remember reading them at the beginning of the year but probably > > the Christmas haze did not clear enough when I did. > > > There was a similar question from the freescale dcu people [1]. I > believe that the answer still applies. > > [1] http://lists.freedesktop.org/archives/dri-devel/2015-March/078689.html > > >> > >> The driver seems to has has a bit of dead code guarded by > >> HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime > >> switch(es) ? > > > > There is a comment in hdlcd_drv.h explaining what HDLCD_SHOW_UNDERRUN can > > be used for. As for HDLCD_COUNT_BUFFERUNDERRUNS, you are right, it's dead > > code from the earlier debugging code that got removed before submission. > > I will correct that. > > > I haven't seen other drm drivers follow the "set a macro that is > documented in a header and rebuild" approach. Devs either add a CONFIG > option for it, or expose it as a module parameter. IIRC the nouveau > driver does even more - it enables the underrun machinery > unconditionally. Adding a module parameter probably makes more sense, I will look into that. > > >> > >> Most DRM drivers do not threat dma, bus_error, vsync and/or underrun > >> interrupts as debug functionality. They are of limited use in this > >> driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange > >> imho. > > > > The HDLCD device has only 1 interrupt that can fire and there is no other > > way to show the reason for the interrupt in the driver. It was useful when > > debugging underruns and/or vsync issues so I thought it might be useful > > to keep around. Putting it the other way: if you are going to use this > > device > > and the image is not completely rendered I would not be able to give you > > support to figure out what went wrong without this debugging information. > > With this in place I can tell the difference between a busy system vs one > > where the interrupt latency is large. > > > Out of curiosity - are there any implications/side-effects from having > these enabled ? Having the counters counting the interrupts should not affect the latency and having the default colour red means parts of the screen will flash with that colour when the data that was supposed to be there could not be fetched in time (underrun). Yes, probably not something casual users of the ARM boards will complain about and a highly visible effect when you have bandwidth starvation issues. Best regards, Liviu > > Thanks, > Emil > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On Mon, Aug 17, 2015 at 07:17:53PM +0100, Jon Medhurst (Tixy) wrote:
I haven't reviewed the code in detail, just had one comment I alluded to
in a private email the other day...
On Wed, 2015-08-05 at 15:28 +0100, Liviu Dudau wrote:
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
b/drivers/gpu/drm/arm/hdlcd_crtc.c
[...]
+void hdlcd_set_scanout(struct hdlcd_drm_private *hdlcd)
+{
+ struct drm_framebuffer *fb = hdlcd-crtc.primary-fb;
+ struct drm_gem_cma_object *gem;
+ unsigned int depth, bpp;
+ dma_addr_t scanout_start;
+
+ drm_fb_get_bpp_depth(fb-pixel_format, depth, bpp);
+ gem = drm_fb_cma_get_gem_obj(fb, 0);
+
+ scanout_start = gem-paddr + fb-offsets[0] +
+ (hdlcd-crtc.y * fb-pitches[0]) + (hdlcd-crtc.x * bpp/8);
+
+ hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
+}
+
The above function accesses various pointers and values, which
presumably all need to be valid and consistent. However...
[...]
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c
b/drivers/gpu/drm/arm/hdlcd_drv.c
[...]
+static irqreturn_t hdlcd_irq(int irq, void *arg)
+{
+ struct drm_device *dev = arg;
+ struct hdlcd_drm_private *hdlcd = dev-dev_private;
+ unsigned long irq_status;
+
+ irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
+
+#ifdef CONFIG_DEBUG_FS
+ if (irq_status HDLCD_INTERRUPT_UNDERRUN) {
+ atomic_inc(hdlcd-buffer_underrun_count);
+ }
+ if (irq_status HDLCD_INTERRUPT_DMA_END) {
+ atomic_inc(hdlcd-dma_end_count);
+ }
+ if (irq_status HDLCD_INTERRUPT_BUS_ERROR) {
+ atomic_inc(hdlcd-bus_error_count);
+ }
+ if (irq_status HDLCD_INTERRUPT_VSYNC) {
+ atomic_inc(hdlcd-vsync_count);
+ }
+#endif
+ if (irq_status HDLCD_INTERRUPT_VSYNC) {
+ struct drm_pending_vblank_event *event;
+ unsigned long flags;
+
+ hdlcd_set_scanout(hdlcd);
hdlcd_set_scanout is being called on every vsync interrupt, can we
guarantee that is safe? What if we're in the middle of a page flip or
panning operation? Seems to me that there is at least scope for
incorrect addresses being calculated leading to a nasty glitch on the
screen for one frame. And in the worst case, possibly invalid pointer
being dereferenced.
Agree, there is a risk of corruption here. I'm going to look at the atomic
mode setting which should hopefully resolve most of these issues.
So, if scanout needs to be set on every vsync, would it not be safer
(and more efficient) to have a single variable storing the value to use
during interrupts, and recalculate that value outside of interrupts
whenever things are changed?
hdlcd_set_scanout() function is merely a convenience function to calculate
the scanout_start variable. The interrupt handler probably doesn't need to
call that and it was mostly a shortcut to make sure the HDLCD_REG_FB_BASE
register was up-to-date when the vsync interrupt happened. I hope the
atomic modeset will cleanup things here.
Best regards,
Liviu
--
Tixy
--
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---
¯\_(ツ)_/¯
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On Tue, Aug 18, 2015 at 05:41:59PM +0100, Emil Velikov wrote: On 17 August 2015 at 16:15, Liviu Dudau liviu.du...@arm.com wrote: On Sun, Aug 16, 2015 at 09:56:33AM +0100, Emil Velikov wrote: Hi Liviu, Hi Emil, On 5 August 2015 at 15:28, Liviu Dudau liviu.du...@arm.com wrote: The HDLCD controller is a display controller that supports resolutions up to 4096x4096 pixels. It is present on various development boards produced by ARM Ltd and emulated by the latest Fast Models from the company. I believe there is a unofficial requirement(?) for new drm drivers to use atomic modesetting. Not 100% sure on this one though. The following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] [2] cat provide some information on the topic. I am also interested to know if this is a requirement or not. Thanks for the pointers, I will review them again to see if I did miss anything. I remember reading them at the beginning of the year but probably the Christmas haze did not clear enough when I did. There was a similar question from the freescale dcu people [1]. I believe that the answer still applies. [1] http://lists.freedesktop.org/archives/dri-devel/2015-March/078689.html The driver seems to has has a bit of dead code guarded by HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime switch(es) ? There is a comment in hdlcd_drv.h explaining what HDLCD_SHOW_UNDERRUN can be used for. As for HDLCD_COUNT_BUFFERUNDERRUNS, you are right, it's dead code from the earlier debugging code that got removed before submission. I will correct that. I haven't seen other drm drivers follow the set a macro that is documented in a header and rebuild approach. Devs either add a CONFIG option for it, or expose it as a module parameter. IIRC the nouveau driver does even more - it enables the underrun machinery unconditionally. Adding a module parameter probably makes more sense, I will look into that. Most DRM drivers do not threat dma, bus_error, vsync and/or underrun interrupts as debug functionality. They are of limited use in this driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange imho. The HDLCD device has only 1 interrupt that can fire and there is no other way to show the reason for the interrupt in the driver. It was useful when debugging underruns and/or vsync issues so I thought it might be useful to keep around. Putting it the other way: if you are going to use this device and the image is not completely rendered I would not be able to give you support to figure out what went wrong without this debugging information. With this in place I can tell the difference between a busy system vs one where the interrupt latency is large. Out of curiosity - are there any implications/side-effects from having these enabled ? Having the counters counting the interrupts should not affect the latency and having the default colour red means parts of the screen will flash with that colour when the data that was supposed to be there could not be fetched in time (underrun). Yes, probably not something casual users of the ARM boards will complain about and a highly visible effect when you have bandwidth starvation issues. Best regards, Liviu Thanks, Emil -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On 17 August 2015 at 16:15, Liviu Dudau wrote: > On Sun, Aug 16, 2015 at 09:56:33AM +0100, Emil Velikov wrote: >> Hi Liviu, > > Hi Emil, > >> >> On 5 August 2015 at 15:28, Liviu Dudau wrote: >> > The HDLCD controller is a display controller that supports resolutions >> > up to 4096x4096 pixels. It is present on various development boards >> > produced by ARM Ltd and emulated by the latest Fast Models from the >> > company. >> > >> I believe there is a unofficial requirement(?) for new drm drivers to >> use atomic modesetting. Not 100% sure on this one though. The >> following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] >> [2] cat provide some information on the topic. > > I am also interested to know if this is a requirement or not. > Thanks for the pointers, I will review them again to see if I did miss > anything. I remember reading them at the beginning of the year but probably > the Christmas haze did not clear enough when I did. > There was a similar question from the freescale dcu people [1]. I believe that the answer still applies. [1] http://lists.freedesktop.org/archives/dri-devel/2015-March/078689.html >> >> The driver seems to has has a bit of dead code guarded by >> HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime >> switch(es) ? > > There is a comment in hdlcd_drv.h explaining what HDLCD_SHOW_UNDERRUN can > be used for. As for HDLCD_COUNT_BUFFERUNDERRUNS, you are right, it's dead > code from the earlier debugging code that got removed before submission. > I will correct that. > I haven't seen other drm drivers follow the "set a macro that is documented in a header and rebuild" approach. Devs either add a CONFIG option for it, or expose it as a module parameter. IIRC the nouveau driver does even more - it enables the underrun machinery unconditionally. >> >> Most DRM drivers do not threat dma, bus_error, vsync and/or underrun >> interrupts as debug functionality. They are of limited use in this >> driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange >> imho. > > The HDLCD device has only 1 interrupt that can fire and there is no other > way to show the reason for the interrupt in the driver. It was useful when > debugging underruns and/or vsync issues so I thought it might be useful > to keep around. Putting it the other way: if you are going to use this device > and the image is not completely rendered I would not be able to give you > support to figure out what went wrong without this debugging information. > With this in place I can tell the difference between a busy system vs one > where the interrupt latency is large. > Out of curiosity - are there any implications/side-effects from having these enabled ? Thanks, Emil -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On 17 August 2015 at 16:15, Liviu Dudau liviu.du...@arm.com wrote: On Sun, Aug 16, 2015 at 09:56:33AM +0100, Emil Velikov wrote: Hi Liviu, Hi Emil, On 5 August 2015 at 15:28, Liviu Dudau liviu.du...@arm.com wrote: The HDLCD controller is a display controller that supports resolutions up to 4096x4096 pixels. It is present on various development boards produced by ARM Ltd and emulated by the latest Fast Models from the company. I believe there is a unofficial requirement(?) for new drm drivers to use atomic modesetting. Not 100% sure on this one though. The following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] [2] cat provide some information on the topic. I am also interested to know if this is a requirement or not. Thanks for the pointers, I will review them again to see if I did miss anything. I remember reading them at the beginning of the year but probably the Christmas haze did not clear enough when I did. There was a similar question from the freescale dcu people [1]. I believe that the answer still applies. [1] http://lists.freedesktop.org/archives/dri-devel/2015-March/078689.html The driver seems to has has a bit of dead code guarded by HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime switch(es) ? There is a comment in hdlcd_drv.h explaining what HDLCD_SHOW_UNDERRUN can be used for. As for HDLCD_COUNT_BUFFERUNDERRUNS, you are right, it's dead code from the earlier debugging code that got removed before submission. I will correct that. I haven't seen other drm drivers follow the set a macro that is documented in a header and rebuild approach. Devs either add a CONFIG option for it, or expose it as a module parameter. IIRC the nouveau driver does even more - it enables the underrun machinery unconditionally. Most DRM drivers do not threat dma, bus_error, vsync and/or underrun interrupts as debug functionality. They are of limited use in this driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange imho. The HDLCD device has only 1 interrupt that can fire and there is no other way to show the reason for the interrupt in the driver. It was useful when debugging underruns and/or vsync issues so I thought it might be useful to keep around. Putting it the other way: if you are going to use this device and the image is not completely rendered I would not be able to give you support to figure out what went wrong without this debugging information. With this in place I can tell the difference between a busy system vs one where the interrupt latency is large. Out of curiosity - are there any implications/side-effects from having these enabled ? Thanks, Emil -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
I haven't reviewed the code in detail, just had one comment I alluded to
in a private email the other day...
On Wed, 2015-08-05 at 15:28 +0100, Liviu Dudau wrote:
> diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
> b/drivers/gpu/drm/arm/hdlcd_crtc.c
[...]
> +void hdlcd_set_scanout(struct hdlcd_drm_private *hdlcd)
> +{
> + struct drm_framebuffer *fb = hdlcd->crtc.primary->fb;
> + struct drm_gem_cma_object *gem;
> + unsigned int depth, bpp;
> + dma_addr_t scanout_start;
> +
> + drm_fb_get_bpp_depth(fb->pixel_format, , );
> + gem = drm_fb_cma_get_gem_obj(fb, 0);
> +
> + scanout_start = gem->paddr + fb->offsets[0] +
> + (hdlcd->crtc.y * fb->pitches[0]) + (hdlcd->crtc.x * bpp/8);
> +
> + hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
> +}
> +
The above function accesses various pointers and values, which
presumably all need to be valid and consistent. However...
[...]
> diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
[...]
> +static irqreturn_t hdlcd_irq(int irq, void *arg)
> +{
> + struct drm_device *dev = arg;
> + struct hdlcd_drm_private *hdlcd = dev->dev_private;
> + unsigned long irq_status;
> +
> + irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
> +
> +#ifdef CONFIG_DEBUG_FS
> + if (irq_status & HDLCD_INTERRUPT_UNDERRUN) {
> + atomic_inc(>buffer_underrun_count);
> + }
> + if (irq_status & HDLCD_INTERRUPT_DMA_END) {
> + atomic_inc(>dma_end_count);
> + }
> + if (irq_status & HDLCD_INTERRUPT_BUS_ERROR) {
> + atomic_inc(>bus_error_count);
> + }
> + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> + atomic_inc(>vsync_count);
> + }
> +#endif
> + if (irq_status & HDLCD_INTERRUPT_VSYNC) {
> + struct drm_pending_vblank_event *event;
> + unsigned long flags;
> +
> + hdlcd_set_scanout(hdlcd);
hdlcd_set_scanout is being called on every vsync interrupt, can we
guarantee that is safe? What if we're in the middle of a page flip or
panning operation? Seems to me that there is at least scope for
incorrect addresses being calculated leading to a nasty glitch on the
screen for one frame. And in the worst case, possibly invalid pointer
being dereferenced.
So, if scanout needs to be set on every vsync, would it not be safer
(and more efficient) to have a single variable storing the value to use
during interrupts, and recalculate that value outside of interrupts
whenever things are changed?
--
Tixy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On Sun, Aug 16, 2015 at 09:56:33AM +0100, Emil Velikov wrote: > Hi Liviu, Hi Emil, > > On 5 August 2015 at 15:28, Liviu Dudau wrote: > > The HDLCD controller is a display controller that supports resolutions > > up to 4096x4096 pixels. It is present on various development boards > > produced by ARM Ltd and emulated by the latest Fast Models from the > > company. > > > I believe there is a unofficial requirement(?) for new drm drivers to > use atomic modesetting. Not 100% sure on this one though. The > following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] > [2] cat provide some information on the topic. I am also interested to know if this is a requirement or not. Thanks for the pointers, I will review them again to see if I did miss anything. I remember reading them at the beginning of the year but probably the Christmas haze did not clear enough when I did. > > The driver seems to has has a bit of dead code guarded by > HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime > switch(es) ? There is a comment in hdlcd_drv.h explaining what HDLCD_SHOW_UNDERRUN can be used for. As for HDLCD_COUNT_BUFFERUNDERRUNS, you are right, it's dead code from the earlier debugging code that got removed before submission. I will correct that. > > Most DRM drivers do not threat dma, bus_error, vsync and/or underrun > interrupts as debug functionality. They are of limited use in this > driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange > imho. The HDLCD device has only 1 interrupt that can fire and there is no other way to show the reason for the interrupt in the driver. It was useful when debugging underruns and/or vsync issues so I thought it might be useful to keep around. Putting it the other way: if you are going to use this device and the image is not completely rendered I would not be able to give you support to figure out what went wrong without this debugging information. With this in place I can tell the difference between a busy system vs one where the interrupt latency is large. Best regards, Liviu > > Cheers, > Emil > > [1] http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html > [2] http://blog.ffwll.ch/2015/01/update-for-atomic-display-updates.html > -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
On Sun, Aug 16, 2015 at 09:56:33AM +0100, Emil Velikov wrote: Hi Liviu, Hi Emil, On 5 August 2015 at 15:28, Liviu Dudau liviu.du...@arm.com wrote: The HDLCD controller is a display controller that supports resolutions up to 4096x4096 pixels. It is present on various development boards produced by ARM Ltd and emulated by the latest Fast Models from the company. I believe there is a unofficial requirement(?) for new drm drivers to use atomic modesetting. Not 100% sure on this one though. The following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] [2] cat provide some information on the topic. I am also interested to know if this is a requirement or not. Thanks for the pointers, I will review them again to see if I did miss anything. I remember reading them at the beginning of the year but probably the Christmas haze did not clear enough when I did. The driver seems to has has a bit of dead code guarded by HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime switch(es) ? There is a comment in hdlcd_drv.h explaining what HDLCD_SHOW_UNDERRUN can be used for. As for HDLCD_COUNT_BUFFERUNDERRUNS, you are right, it's dead code from the earlier debugging code that got removed before submission. I will correct that. Most DRM drivers do not threat dma, bus_error, vsync and/or underrun interrupts as debug functionality. They are of limited use in this driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange imho. The HDLCD device has only 1 interrupt that can fire and there is no other way to show the reason for the interrupt in the driver. It was useful when debugging underruns and/or vsync issues so I thought it might be useful to keep around. Putting it the other way: if you are going to use this device and the image is not completely rendered I would not be able to give you support to figure out what went wrong without this debugging information. With this in place I can tell the difference between a busy system vs one where the interrupt latency is large. Best regards, Liviu Cheers, Emil [1] http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html [2] http://blog.ffwll.ch/2015/01/update-for-atomic-display-updates.html -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
I haven't reviewed the code in detail, just had one comment I alluded to
in a private email the other day...
On Wed, 2015-08-05 at 15:28 +0100, Liviu Dudau wrote:
diff --git a/drivers/gpu/drm/arm/hdlcd_crtc.c
b/drivers/gpu/drm/arm/hdlcd_crtc.c
[...]
+void hdlcd_set_scanout(struct hdlcd_drm_private *hdlcd)
+{
+ struct drm_framebuffer *fb = hdlcd-crtc.primary-fb;
+ struct drm_gem_cma_object *gem;
+ unsigned int depth, bpp;
+ dma_addr_t scanout_start;
+
+ drm_fb_get_bpp_depth(fb-pixel_format, depth, bpp);
+ gem = drm_fb_cma_get_gem_obj(fb, 0);
+
+ scanout_start = gem-paddr + fb-offsets[0] +
+ (hdlcd-crtc.y * fb-pitches[0]) + (hdlcd-crtc.x * bpp/8);
+
+ hdlcd_write(hdlcd, HDLCD_REG_FB_BASE, scanout_start);
+}
+
The above function accesses various pointers and values, which
presumably all need to be valid and consistent. However...
[...]
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
[...]
+static irqreturn_t hdlcd_irq(int irq, void *arg)
+{
+ struct drm_device *dev = arg;
+ struct hdlcd_drm_private *hdlcd = dev-dev_private;
+ unsigned long irq_status;
+
+ irq_status = hdlcd_read(hdlcd, HDLCD_REG_INT_STATUS);
+
+#ifdef CONFIG_DEBUG_FS
+ if (irq_status HDLCD_INTERRUPT_UNDERRUN) {
+ atomic_inc(hdlcd-buffer_underrun_count);
+ }
+ if (irq_status HDLCD_INTERRUPT_DMA_END) {
+ atomic_inc(hdlcd-dma_end_count);
+ }
+ if (irq_status HDLCD_INTERRUPT_BUS_ERROR) {
+ atomic_inc(hdlcd-bus_error_count);
+ }
+ if (irq_status HDLCD_INTERRUPT_VSYNC) {
+ atomic_inc(hdlcd-vsync_count);
+ }
+#endif
+ if (irq_status HDLCD_INTERRUPT_VSYNC) {
+ struct drm_pending_vblank_event *event;
+ unsigned long flags;
+
+ hdlcd_set_scanout(hdlcd);
hdlcd_set_scanout is being called on every vsync interrupt, can we
guarantee that is safe? What if we're in the middle of a page flip or
panning operation? Seems to me that there is at least scope for
incorrect addresses being calculated leading to a nasty glitch on the
screen for one frame. And in the worst case, possibly invalid pointer
being dereferenced.
So, if scanout needs to be set on every vsync, would it not be safer
(and more efficient) to have a single variable storing the value to use
during interrupts, and recalculate that value outside of interrupts
whenever things are changed?
--
Tixy
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
Hi Liviu, On 5 August 2015 at 15:28, Liviu Dudau wrote: > The HDLCD controller is a display controller that supports resolutions > up to 4096x4096 pixels. It is present on various development boards > produced by ARM Ltd and emulated by the latest Fast Models from the > company. > I believe there is a unofficial requirement(?) for new drm drivers to use atomic modesetting. Not 100% sure on this one though. The following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] [2] cat provide some information on the topic. The driver seems to has has a bit of dead code guarded by HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime switch(es) ? Most DRM drivers do not threat dma, bus_error, vsync and/or underrun interrupts as debug functionality. They are of limited use in this driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange imho. Cheers, Emil [1] http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html [2] http://blog.ffwll.ch/2015/01/update-for-atomic-display-updates.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4] drm: Add support for ARM's HDLCD controller.
Hi Liviu, On 5 August 2015 at 15:28, Liviu Dudau liviu.du...@arm.com wrote: The HDLCD controller is a display controller that supports resolutions up to 4096x4096 pixels. It is present on various development boards produced by ARM Ltd and emulated by the latest Fast Models from the company. I believe there is a unofficial requirement(?) for new drm drivers to use atomic modesetting. Not 100% sure on this one though. The following drivers: tegra, msm, rcar-du, i915, and Daniel's blog [1] [2] cat provide some information on the topic. The driver seems to has has a bit of dead code guarded by HDLCD_*_UNDERRUN. Perhaps these macros should become build or runtime switch(es) ? Most DRM drivers do not threat dma, bus_error, vsync and/or underrun interrupts as debug functionality. They are of limited use in this driver, presently, yet the CONFIG_DEBUG_FS guard seems a bit strange imho. Cheers, Emil [1] http://blog.ffwll.ch/2014/11/atomic-modeset-support-for-kms-drivers.html [2] http://blog.ffwll.ch/2015/01/update-for-atomic-display-updates.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
