| Message ID | 20210727182721.17981-4-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Make DRM's IRQ helpers legacy | expand |
Hi Dan, I hope you can fine to test this patch from Thomas. If this works then we can forget about my attempt to do the same. Hi Thomas, IRQ_NOTCONNECTED check seems redundant, as mentioned in another patch already. With that considered: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> We shall wait for testing from Dan before you apply it as I had made a similar patch that failed to work. I assume my patch was buggy but I prefer to be sure. Sam On Tue, Jul 27, 2021 at 08:27:10PM +0200, Thomas Zimmermann wrote: > Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's > IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers > don't benefit from using it. DRM IRQ callbacks are now being called > directly or inlined. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 85 ++++++++++++-------- > 1 file changed, 52 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > index f09b6dd8754c..cfa8c2c9c8aa 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > @@ -22,7 +22,6 @@ > #include <drm/drm_fb_helper.h> > #include <drm/drm_gem_cma_helper.h> > #include <drm/drm_gem_framebuffer_helper.h> > -#include <drm/drm_irq.h> > #include <drm/drm_probe_helper.h> > #include <drm/drm_vblank.h> > > @@ -557,6 +556,56 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq, void *data) > return IRQ_HANDLED; > } > > +static void atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev) > +{ > + struct atmel_hlcdc_dc *dc = dev->dev_private; > + unsigned int cfg = 0; > + int i; > + > + /* Enable interrupts on activated layers */ > + for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) { > + if (dc->layers[i]) > + cfg |= ATMEL_HLCDC_LAYER_STATUS(i); > + } > + > + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg); > +} > + > +static void atmel_hlcdc_dc_irq_disable(struct drm_device *dev) > +{ > + struct atmel_hlcdc_dc *dc = dev->dev_private; > + unsigned int isr; > + > + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0xffffffff); > + regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, &isr); > +} > + > +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq) > +{ > + int ret; > + > + if (irq == IRQ_NOTCONNECTED) > + return -ENOTCONN; > + > + atmel_hlcdc_dc_irq_disable(dev); > + > + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, dev->driver->name, dev); > + if (ret) > + return ret; > + > + atmel_hlcdc_dc_irq_postinstall(dev); > + > + return 0; > +} > + > +static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev) > +{ > + struct atmel_hlcdc_dc *dc = dev->dev_private; > + > + atmel_hlcdc_dc_irq_disable(dev); > + free_irq(dc->hlcdc->irq, dev); > +} > + > static const struct drm_mode_config_funcs mode_config_funcs = { > .fb_create = drm_gem_fb_create, > .atomic_check = drm_atomic_helper_check, > @@ -647,7 +696,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) > drm_mode_config_reset(dev); > > pm_runtime_get_sync(dev->dev); > - ret = drm_irq_install(dev, dc->hlcdc->irq); > + ret = atmel_hlcdc_dc_irq_install(dev, dc->hlcdc->irq); > pm_runtime_put_sync(dev->dev); > if (ret < 0) { > dev_err(dev->dev, "failed to install IRQ handler\n"); > @@ -676,7 +725,7 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) > drm_mode_config_cleanup(dev); > > pm_runtime_get_sync(dev->dev); > - drm_irq_uninstall(dev); > + atmel_hlcdc_dc_irq_uninstall(dev); > pm_runtime_put_sync(dev->dev); > > dev->dev_private = NULL; > @@ -685,40 +734,10 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) > clk_disable_unprepare(dc->hlcdc->periph_clk); > } > > -static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev) > -{ > - struct atmel_hlcdc_dc *dc = dev->dev_private; > - unsigned int cfg = 0; > - int i; > - > - /* Enable interrupts on activated layers */ > - for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) { > - if (dc->layers[i]) > - cfg |= ATMEL_HLCDC_LAYER_STATUS(i); > - } > - > - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg); > - > - return 0; > -} > - > -static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev) > -{ > - struct atmel_hlcdc_dc *dc = dev->dev_private; > - unsigned int isr; > - > - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0xffffffff); > - regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, &isr); > -} > - > DEFINE_DRM_GEM_CMA_FOPS(fops); > > static const struct drm_driver atmel_hlcdc_dc_driver = { > .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, > - .irq_handler = atmel_hlcdc_dc_irq_handler, > - .irq_preinstall = atmel_hlcdc_dc_irq_uninstall, > - .irq_postinstall = atmel_hlcdc_dc_irq_postinstall, > - .irq_uninstall = atmel_hlcdc_dc_irq_uninstall, > DRM_GEM_CMA_DRIVER_OPS, > .fops = &fops, > .name = "atmel-hlcdc", > -- > 2.32.0
On 7/28/21 7:00 AM, Sam Ravnborg wrote: > [You don't often get email from sam@ravnborg.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.] > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Dan, > > I hope you can fine to test this patch from Thomas. > If this works then we can forget about my attempt to do the same. I'll test this as soon as I can and let you know. Thanks, Dan > > Hi Thomas, > > IRQ_NOTCONNECTED check seems redundant, as mentioned in another patch > already. > > With that considered: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > We shall wait for testing from Dan before you apply it as I had made a > similar patch that failed to work. I assume my patch was buggy but I > prefer to be sure. > > Sam > > On Tue, Jul 27, 2021 at 08:27:10PM +0200, Thomas Zimmermann wrote: >> Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's >> IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers >> don't benefit from using it. DRM IRQ callbacks are now being called >> directly or inlined. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 85 ++++++++++++-------- >> 1 file changed, 52 insertions(+), 33 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> index f09b6dd8754c..cfa8c2c9c8aa 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> @@ -22,7 +22,6 @@ >> #include <drm/drm_fb_helper.h> >> #include <drm/drm_gem_cma_helper.h> >> #include <drm/drm_gem_framebuffer_helper.h> >> -#include <drm/drm_irq.h> >> #include <drm/drm_probe_helper.h> >> #include <drm/drm_vblank.h> >> >> @@ -557,6 +556,56 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq, void *data) >> return IRQ_HANDLED; >> } >> >> +static void atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev) >> +{ >> + struct atmel_hlcdc_dc *dc = dev->dev_private; >> + unsigned int cfg = 0; >> + int i; >> + >> + /* Enable interrupts on activated layers */ >> + for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) { >> + if (dc->layers[i]) >> + cfg |= ATMEL_HLCDC_LAYER_STATUS(i); >> + } >> + >> + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg); >> +} >> + >> +static void atmel_hlcdc_dc_irq_disable(struct drm_device *dev) >> +{ >> + struct atmel_hlcdc_dc *dc = dev->dev_private; >> + unsigned int isr; >> + >> + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0xffffffff); >> + regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, &isr); >> +} >> + >> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq) >> +{ >> + int ret; >> + >> + if (irq == IRQ_NOTCONNECTED) >> + return -ENOTCONN; >> + >> + atmel_hlcdc_dc_irq_disable(dev); >> + >> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, dev->driver->name, dev); >> + if (ret) >> + return ret; >> + >> + atmel_hlcdc_dc_irq_postinstall(dev); >> + >> + return 0; >> +} >> + >> +static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev) >> +{ >> + struct atmel_hlcdc_dc *dc = dev->dev_private; >> + >> + atmel_hlcdc_dc_irq_disable(dev); >> + free_irq(dc->hlcdc->irq, dev); >> +} >> + >> static const struct drm_mode_config_funcs mode_config_funcs = { >> .fb_create = drm_gem_fb_create, >> .atomic_check = drm_atomic_helper_check, >> @@ -647,7 +696,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) >> drm_mode_config_reset(dev); >> >> pm_runtime_get_sync(dev->dev); >> - ret = drm_irq_install(dev, dc->hlcdc->irq); >> + ret = atmel_hlcdc_dc_irq_install(dev, dc->hlcdc->irq); >> pm_runtime_put_sync(dev->dev); >> if (ret < 0) { >> dev_err(dev->dev, "failed to install IRQ handler\n"); >> @@ -676,7 +725,7 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) >> drm_mode_config_cleanup(dev); >> >> pm_runtime_get_sync(dev->dev); >> - drm_irq_uninstall(dev); >> + atmel_hlcdc_dc_irq_uninstall(dev); >> pm_runtime_put_sync(dev->dev); >> >> dev->dev_private = NULL; >> @@ -685,40 +734,10 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> } >> >> -static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev) >> -{ >> - struct atmel_hlcdc_dc *dc = dev->dev_private; >> - unsigned int cfg = 0; >> - int i; >> - >> - /* Enable interrupts on activated layers */ >> - for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) { >> - if (dc->layers[i]) >> - cfg |= ATMEL_HLCDC_LAYER_STATUS(i); >> - } >> - >> - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg); >> - >> - return 0; >> -} >> - >> -static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev) >> -{ >> - struct atmel_hlcdc_dc *dc = dev->dev_private; >> - unsigned int isr; >> - >> - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0xffffffff); >> - regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, &isr); >> -} >> - >> DEFINE_DRM_GEM_CMA_FOPS(fops); >> >> static const struct drm_driver atmel_hlcdc_dc_driver = { >> .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, >> - .irq_handler = atmel_hlcdc_dc_irq_handler, >> - .irq_preinstall = atmel_hlcdc_dc_irq_uninstall, >> - .irq_postinstall = atmel_hlcdc_dc_irq_postinstall, >> - .irq_uninstall = atmel_hlcdc_dc_irq_uninstall, >> DRM_GEM_CMA_DRIVER_OPS, >> .fops = &fops, >> .name = "atmel-hlcdc", >> -- >> 2.32.0
Hi Dan, On Wed, Jul 28, 2021 at 03:11:08PM +0000, Dan.Sneddon@microchip.com wrote: > On 7/28/21 7:00 AM, Sam Ravnborg wrote: > > [You don't often get email from sam@ravnborg.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.] > > > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Hi Dan, > > > > I hope you can fine to test this patch from Thomas. > > If this works then we can forget about my attempt to do the same. > > I'll test this as soon as I can and let you know. Thanks, crossing my fingers... (which explains the funny spelling from time to time) Sam
On 7/28/21 8:44 AM, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Dan, > > On Wed, Jul 28, 2021 at 03:11:08PM +0000, Dan.Sneddon@microchip.com wrote: >> On 7/28/21 7:00 AM, Sam Ravnborg wrote: >>> [You don't often get email from sam@ravnborg.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.] >>> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Hi Dan, >>> >>> I hope you can fine to test this patch from Thomas. >>> If this works then we can forget about my attempt to do the same. >> >> I'll test this as soon as I can and let you know. > > Thanks, crossing my fingers... (which explains the funny spelling from > time to time) > > Sam > So I ran the test on an A5D27 XULT board with a PDA5 display. Our graphics demos that come with our linux4sam releases seem to run just fine. modetest -v seems to run just fine. However, vbltest returns "drmWaitVBlank (relative) failed ret: -1". I don't understand why modetest -v is working and vbltest isn't, but that's what I'm seeing. Thanks, Dan
Hi Dan, thanks for the quick feedback! On Wed, Jul 28, 2021 at 05:50:34PM +0000, Dan.Sneddon@microchip.com wrote: > On 7/28/21 8:44 AM, Sam Ravnborg wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Hi Dan, > > > > On Wed, Jul 28, 2021 at 03:11:08PM +0000, Dan.Sneddon@microchip.com wrote: > >> On 7/28/21 7:00 AM, Sam Ravnborg wrote: > >>> [You don't often get email from sam@ravnborg.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.] > >>> > >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>> > >>> Hi Dan, > >>> > >>> I hope you can fine to test this patch from Thomas. > >>> If this works then we can forget about my attempt to do the same. > >> > >> I'll test this as soon as I can and let you know. > > > > Thanks, crossing my fingers... (which explains the funny spelling from > > time to time) > > > > Sam > > So I ran the test on an A5D27 XULT board with a PDA5 display. Our > graphics demos that come with our linux4sam releases seem to run just > fine. modetest -v seems to run just fine. However, vbltest returns > "drmWaitVBlank (relative) failed ret: -1". I don't understand why > modetest -v is working and vbltest isn't, but that's what I'm seeing. Strange indeed. Just to be sure... Can you confirm that vbltest is working OK *before* this patch? Sam
Hi Am 28.07.21 um 20:11 schrieb Sam Ravnborg: > Hi Dan, > > thanks for the quick feedback! > > On Wed, Jul 28, 2021 at 05:50:34PM +0000, Dan.Sneddon@microchip.com wrote: >> On 7/28/21 8:44 AM, Sam Ravnborg wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Hi Dan, >>> >>> On Wed, Jul 28, 2021 at 03:11:08PM +0000, Dan.Sneddon@microchip.com wrote: >>>> On 7/28/21 7:00 AM, Sam Ravnborg wrote: >>>>> [You don't often get email from sam@ravnborg.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.] >>>>> >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>> >>>>> Hi Dan, >>>>> >>>>> I hope you can fine to test this patch from Thomas. >>>>> If this works then we can forget about my attempt to do the same. >>>> >>>> I'll test this as soon as I can and let you know. >>> >>> Thanks, crossing my fingers... (which explains the funny spelling from >>> time to time) >>> >>> Sam >>> So I ran the test on an A5D27 XULT board with a PDA5 display. Our >> graphics demos that come with our linux4sam releases seem to run just >> fine. modetest -v seems to run just fine. However, vbltest returns >> "drmWaitVBlank (relative) failed ret: -1". I don't understand why >> modetest -v is working and vbltest isn't, but that's what I'm seeing. Thanks for testing. > > Strange indeed. > > > Just to be sure... > Can you confirm that vbltest is working OK *before* this patch? Yes, can you please verify that it regressed. If so, this would mean that the driver misses vblank interrupts with the patch applied. Best regards Thomas > > Sam >
On 7/28/21 11:11 AM, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Dan, > > thanks for the quick feedback! > > On Wed, Jul 28, 2021 at 05:50:34PM +0000, Dan.Sneddon@microchip.com wrote: >> On 7/28/21 8:44 AM, Sam Ravnborg wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> Hi Dan, >>> >>> On Wed, Jul 28, 2021 at 03:11:08PM +0000, Dan.Sneddon@microchip.com wrote: >>>> On 7/28/21 7:00 AM, Sam Ravnborg wrote: >>>>> [You don't often get email from sam@ravnborg.org. Learn why this is important at http://aka.ms/LearnAboutSenderIdentification.] >>>>> >>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>> >>>>> Hi Dan, >>>>> >>>>> I hope you can fine to test this patch from Thomas. >>>>> If this works then we can forget about my attempt to do the same. >>>> >>>> I'll test this as soon as I can and let you know. >>> >>> Thanks, crossing my fingers... (which explains the funny spelling from >>> time to time) >>> >>> Sam >>> So I ran the test on an A5D27 XULT board with a PDA5 display. Our >> graphics demos that come with our linux4sam releases seem to run just >> fine. modetest -v seems to run just fine. However, vbltest returns >> "drmWaitVBlank (relative) failed ret: -1". I don't understand why >> modetest -v is working and vbltest isn't, but that's what I'm seeing. > > Strange indeed. > > > Just to be sure... > Can you confirm that vbltest is working OK *before* this patch? > > Sam > I'm afraid vbltest works without the patch, but not with it. Dan
Hi Thomas, On 7/28/21 11:17 AM, Thomas Zimmermann wrote: > Hi > > Am 28.07.21 um 20:11 schrieb Sam Ravnborg: >> Hi Dan, >> >> thanks for the quick feedback! >> >> On Wed, Jul 28, 2021 at 05:50:34PM +0000, Dan.Sneddon@microchip.com >> wrote: >>> On 7/28/21 8:44 AM, Sam Ravnborg wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>> know the content is safe >>>> >>>> Hi Dan, >>>> >>>> On Wed, Jul 28, 2021 at 03:11:08PM +0000, Dan.Sneddon@microchip.com >>>> wrote: >>>>> On 7/28/21 7:00 AM, Sam Ravnborg wrote: >>>>>> [You don't often get email from sam@ravnborg.org. Learn why this >>>>>> is important at http://aka.ms/LearnAboutSenderIdentification.] >>>>>> >>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>>>>> know the content is safe >>>>>> >>>>>> Hi Dan, >>>>>> >>>>>> I hope you can fine to test this patch from Thomas. >>>>>> If this works then we can forget about my attempt to do the same. >>>>> >>>>> I'll test this as soon as I can and let you know. >>>> >>>> Thanks, crossing my fingers... (which explains the funny spelling from >>>> time to time) >>>> >>>> Sam >>>> So I ran the test on an A5D27 XULT board with a PDA5 display. Our >>> graphics demos that come with our linux4sam releases seem to run just >>> fine. modetest -v seems to run just fine. However, vbltest returns >>> "drmWaitVBlank (relative) failed ret: -1". I don't understand why >>> modetest -v is working and vbltest isn't, but that's what I'm seeing. > > Thanks for testing. > >> >> Strange indeed. >> >> >> Just to be sure... >> Can you confirm that vbltest is working OK *before* this patch? > > Yes, can you please verify that it regressed. If so, this would mean > that the driver misses vblank interrupts with the patch applied. Yes, unfortunately the vbltest works before this patch, but fails after this patch is applied. Best Regards, Dan > > Best regards > Thomas > >> >> Sam >> >
Hi Dan, > >> > >> Just to be sure... > >> Can you confirm that vbltest is working OK *before* this patch? > > > > Yes, can you please verify that it regressed. If so, this would mean > > that the driver misses vblank interrupts with the patch applied. > > Yes, unfortunately the vbltest works before this patch, but fails after > this patch is applied. I think I got it - we need to set irq_enabled to true. The documentation says so: " * @irq_enabled: * * Indicates that interrupt handling is enabled, specifically vblank * handling. Drivers which don't use drm_irq_install() need to set this * to true manually. " Can you try to add the following line: +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq) +{ + int ret; + + if (irq == IRQ_NOTCONNECTED) + return -ENOTCONN; + dev->irq_enabled = true; <= THIS LINE + atmel_hlcdc_dc_irq_disable(dev); + + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, dev->driver->name, dev); + if (ret) + return ret; I hope this fixes it. Sam
Hi Sam, On 7/28/21 12:08 PM, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Dan, > >>>> >>>> Just to be sure... >>>> Can you confirm that vbltest is working OK *before* this patch? >>> >>> Yes, can you please verify that it regressed. If so, this would mean >>> that the driver misses vblank interrupts with the patch applied. >> >> Yes, unfortunately the vbltest works before this patch, but fails after >> this patch is applied. > > I think I got it - we need to set irq_enabled to true. > The documentation says so: > " > * @irq_enabled: > * > * Indicates that interrupt handling is enabled, specifically vblank > * handling. Drivers which don't use drm_irq_install() need to set this > * to true manually. > " > > Can you try to add the following line: > > > +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq) > +{ > + int ret; > + > + if (irq == IRQ_NOTCONNECTED) > + return -ENOTCONN; > + > > dev->irq_enabled = true; <= THIS LINE > > > + atmel_hlcdc_dc_irq_disable(dev); > + > + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, dev->driver->name, dev); > + if (ret) > + return ret; > > I hope this fixes it. It does! With the irq_enabled line added everything is looking good. Best Regards, Dan > > Sam >
Hi Dan, > > > > I think I got it - we need to set irq_enabled to true. > > The documentation says so: > > " > > * @irq_enabled: > > * > > * Indicates that interrupt handling is enabled, specifically vblank > > * handling. Drivers which don't use drm_irq_install() need to set this > > * to true manually. > > " > > > > Can you try to add the following line: > > > > > > +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq) > > +{ > > + int ret; > > + > > + if (irq == IRQ_NOTCONNECTED) > > + return -ENOTCONN; > > + > > > > dev->irq_enabled = true; <= THIS LINE > > > > > > + atmel_hlcdc_dc_irq_disable(dev); > > + > > + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, dev->driver->name, dev); > > + if (ret) > > + return ret; > > > > I hope this fixes it. > > It does! With the irq_enabled line added everything is looking good. Great, thanks for testing. Thomas - I assume you will do a re-spin and there is likely some fixes for the applied IRQ conversions too. Note - irq_enabled must be cleared if request_irq fails. I did not include this in the testing here. Sam
Hi Am 28.07.21 um 22:11 schrieb Sam Ravnborg: > Hi Dan, > >>> >>> I think I got it - we need to set irq_enabled to true. >>> The documentation says so: >>> " >>> * @irq_enabled: >>> * >>> * Indicates that interrupt handling is enabled, specifically vblank >>> * handling. Drivers which don't use drm_irq_install() need to set this >>> * to true manually. >>> " >>> >>> Can you try to add the following line: >>> >>> >>> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq) >>> +{ >>> + int ret; >>> + >>> + if (irq == IRQ_NOTCONNECTED) >>> + return -ENOTCONN; >>> + >>> >>> dev->irq_enabled = true; <= THIS LINE >>> >>> >>> + atmel_hlcdc_dc_irq_disable(dev); >>> + >>> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, dev->driver->name, dev); >>> + if (ret) >>> + return ret; >>> >>> I hope this fixes it. >> >> It does! With the irq_enabled line added everything is looking good. Are you sure, you're testing with the latest drm-misc-next or drm-tip? Because using irq_enabled is deprecated and the flag was recently replaced by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK ioctls"). Best regards Thomas > > Great, thanks for testing. > > Thomas - I assume you will do a re-spin and there is likely some fixes > for the applied IRQ conversions too. > > Note - irq_enabled must be cleared if request_irq fails. I did not > include this in the testing here. > > Sam >
Hi Am 29.07.21 um 21:18 schrieb Thomas Zimmermann: > Hi > > Am 28.07.21 um 22:11 schrieb Sam Ravnborg: >> Hi Dan, >> >>>> >>>> I think I got it - we need to set irq_enabled to true. >>>> The documentation says so: >>>> " >>>> * @irq_enabled: >>>> * >>>> * Indicates that interrupt handling is enabled, >>>> specifically vblank >>>> * handling. Drivers which don't use drm_irq_install() >>>> need to set this >>>> * to true manually. >>>> " >>>> >>>> Can you try to add the following line: >>>> >>>> >>>> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, >>>> unsigned int irq) >>>> +{ >>>> + int ret; >>>> + >>>> + if (irq == IRQ_NOTCONNECTED) >>>> + return -ENOTCONN; >>>> + >>>> >>>> dev->irq_enabled = true; <= THIS LINE >>>> >>>> >>>> + atmel_hlcdc_dc_irq_disable(dev); >>>> + >>>> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, >>>> dev->driver->name, dev); >>>> + if (ret) >>>> + return ret; >>>> >>>> I hope this fixes it. >>> >>> It does! With the irq_enabled line added everything is looking good. > > Are you sure, you're testing with the latest drm-misc-next or drm-tip? > Because using irq_enabled is deprecated and the flag was recently > replaced by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in > VBLANK ioctls"). For reference, the cover letter lists base-commit: 2bda1ca4d4acb4892556fec3a7ea1f02afcd40bb which is a recent drm-tip created on July 25. Best regards Thomas > > Best regards > Thomas > >> >> Great, thanks for testing. >> >> Thomas - I assume you will do a re-spin and there is likely some fixes >> for the applied IRQ conversions too. >> >> Note - irq_enabled must be cleared if request_irq fails. I did not >> include this in the testing here. >> >> Sam >> >
Hi Thomas, On 7/29/21 12:18 PM, Thomas Zimmermann wrote: > Hi > > Am 28.07.21 um 22:11 schrieb Sam Ravnborg: >> Hi Dan, >> >>>> >>>> I think I got it - we need to set irq_enabled to true. >>>> The documentation says so: >>>> " >>>> * @irq_enabled: >>>> * >>>> * Indicates that interrupt handling is enabled, >>>> specifically vblank >>>> * handling. Drivers which don't use drm_irq_install() >>>> need to set this >>>> * to true manually. >>>> " >>>> >>>> Can you try to add the following line: >>>> >>>> >>>> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, >>>> unsigned int irq) >>>> +{ >>>> + int ret; >>>> + >>>> + if (irq == IRQ_NOTCONNECTED) >>>> + return -ENOTCONN; >>>> + >>>> >>>> dev->irq_enabled = true; <= THIS LINE >>>> >>>> >>>> + atmel_hlcdc_dc_irq_disable(dev); >>>> + >>>> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, >>>> dev->driver->name, dev); >>>> + if (ret) >>>> + return ret; >>>> >>>> I hope this fixes it. >>> >>> It does! With the irq_enabled line added everything is looking good. > > Are you sure, you're testing with the latest drm-misc-next or drm-tip? > Because using irq_enabled is deprecated and the flag was recently > replaced by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in > VBLANK ioctls"). > > Best regards > Thomas > I was testing with 5.14-rc3. I can test with drm-tip or drm-misc-next. There a preferred branch to test from? Thanks and regards, Dan >> >> Great, thanks for testing. >> >> Thomas - I assume you will do a re-spin and there is likely some fixes >> for the applied IRQ conversions too. >> >> Note - irq_enabled must be cleared if request_irq fails. I did not >> include this in the testing here. >> >> Sam >> >
Hi Am 29.07.21 um 21:24 schrieb Dan.Sneddon@microchip.com: > Hi Thomas, > > On 7/29/21 12:18 PM, Thomas Zimmermann wrote: >> Hi >> >> Am 28.07.21 um 22:11 schrieb Sam Ravnborg: >>> Hi Dan, >>> >>>>> >>>>> I think I got it - we need to set irq_enabled to true. >>>>> The documentation says so: >>>>> " >>>>> * @irq_enabled: >>>>> * >>>>> * Indicates that interrupt handling is enabled, >>>>> specifically vblank >>>>> * handling. Drivers which don't use drm_irq_install() >>>>> need to set this >>>>> * to true manually. >>>>> " >>>>> >>>>> Can you try to add the following line: >>>>> >>>>> >>>>> +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, >>>>> unsigned int irq) >>>>> +{ >>>>> + int ret; >>>>> + >>>>> + if (irq == IRQ_NOTCONNECTED) >>>>> + return -ENOTCONN; >>>>> + >>>>> >>>>> dev->irq_enabled = true; <= THIS LINE >>>>> >>>>> >>>>> + atmel_hlcdc_dc_irq_disable(dev); >>>>> + >>>>> + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, >>>>> dev->driver->name, dev); >>>>> + if (ret) >>>>> + return ret; >>>>> >>>>> I hope this fixes it. >>>> >>>> It does! With the irq_enabled line added everything is looking good. >> >> Are you sure, you're testing with the latest drm-misc-next or drm-tip? >> Because using irq_enabled is deprecated and the flag was recently >> replaced by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in >> VBLANK ioctls"). >> >> Best regards >> Thomas >> > > I was testing with 5.14-rc3. I can test with drm-tip or drm-misc-next. > There a preferred branch to test from? I use drm-tip for development, but all the relevant patches go through drm-misc-next. So either is fine. Best regards Thomas > > Thanks and regards, > Dan > >>> >>> Great, thanks for testing. >>> >>> Thomas - I assume you will do a re-spin and there is likely some fixes >>> for the applied IRQ conversions too. >>> >>> Note - irq_enabled must be cleared if request_irq fails. I did not >>> include this in the testing here. >>> >>> Sam >>> >> >
Hi Thomas, > > Are you sure, you're testing with the latest drm-misc-next or drm-tip? > Because using irq_enabled is deprecated and the flag was recently replaced > by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK ioctls"). I was looking at drm-misc-fixes which did not have this commit :-( Just my silly excuse why I was convinced this was the issue. Sam
Hi Thomas and Sam, On 7/29/21 12:48 PM, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Thomas, > >> >> Are you sure, you're testing with the latest drm-misc-next or drm-tip? >> Because using irq_enabled is deprecated and the flag was recently replaced >> by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK ioctls"). Ok, My fault for testing on the wrong branch. When I test this patch on drm-misc-next it works great. Sorry for the confusion! > > I was looking at drm-misc-fixes which did not have this commit :-( > Just my silly excuse why I was convinced this was the issue. > > Sam > Best regards, Dan
Hi Dan and Sam Am 29.07.21 um 21:55 schrieb Dan.Sneddon@microchip.com: > Hi Thomas and Sam, > On 7/29/21 12:48 PM, Sam Ravnborg wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> Hi Thomas, >> >>> >>> Are you sure, you're testing with the latest drm-misc-next or drm-tip? >>> Because using irq_enabled is deprecated and the flag was recently replaced >>> by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK ioctls"). > > Ok, My fault for testing on the wrong branch. When I test this patch on > drm-misc-next it works great. Sorry for the confusion! > >> >> I was looking at drm-misc-fixes which did not have this commit :-( >> Just my silly excuse why I was convinced this was the issue. Don't worry. I'll add Sam's R-b and a Tested-by from Dan to the patch. Is that ok? Best regards Thomas >> >> Sam >> > > Best regards, > Dan >
On 7/30/21 1:31 AM, Thomas Zimmermann wrote: > Hi Dan and Sam > > Am 29.07.21 um 21:55 schrieb Dan.Sneddon@microchip.com: >> Hi Thomas and Sam, >> On 7/29/21 12:48 PM, Sam Ravnborg wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you >>> know the content is safe >>> >>> Hi Thomas, >>> >>>> >>>> Are you sure, you're testing with the latest drm-misc-next or drm-tip? >>>> Because using irq_enabled is deprecated and the flag was recently >>>> replaced >>>> by commit 1e4cd78ed493 ("drm: Don't test for IRQ support in VBLANK >>>> ioctls"). >> >> Ok, My fault for testing on the wrong branch. When I test this patch on >> drm-misc-next it works great. Sorry for the confusion! >> >>> >>> I was looking at drm-misc-fixes which did not have this commit :-( >>> Just my silly excuse why I was convinced this was the issue. > > Don't worry. > > I'll add Sam's R-b and a Tested-by from Dan to the patch. Is that ok? The tested-by works for me! Thanks! > > Best regards > Thomas > > >>> >>> Sam >>> >> >> Best regards, >> Dan >> >
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index f09b6dd8754c..cfa8c2c9c8aa 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -22,7 +22,6 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem_cma_helper.h> #include <drm/drm_gem_framebuffer_helper.h> -#include <drm/drm_irq.h> #include <drm/drm_probe_helper.h> #include <drm/drm_vblank.h> @@ -557,6 +556,56 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq, void *data) return IRQ_HANDLED; } +static void atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev) +{ + struct atmel_hlcdc_dc *dc = dev->dev_private; + unsigned int cfg = 0; + int i; + + /* Enable interrupts on activated layers */ + for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) { + if (dc->layers[i]) + cfg |= ATMEL_HLCDC_LAYER_STATUS(i); + } + + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg); +} + +static void atmel_hlcdc_dc_irq_disable(struct drm_device *dev) +{ + struct atmel_hlcdc_dc *dc = dev->dev_private; + unsigned int isr; + + regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0xffffffff); + regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, &isr); +} + +static int atmel_hlcdc_dc_irq_install(struct drm_device *dev, unsigned int irq) +{ + int ret; + + if (irq == IRQ_NOTCONNECTED) + return -ENOTCONN; + + atmel_hlcdc_dc_irq_disable(dev); + + ret = request_irq(irq, atmel_hlcdc_dc_irq_handler, 0, dev->driver->name, dev); + if (ret) + return ret; + + atmel_hlcdc_dc_irq_postinstall(dev); + + return 0; +} + +static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev) +{ + struct atmel_hlcdc_dc *dc = dev->dev_private; + + atmel_hlcdc_dc_irq_disable(dev); + free_irq(dc->hlcdc->irq, dev); +} + static const struct drm_mode_config_funcs mode_config_funcs = { .fb_create = drm_gem_fb_create, .atomic_check = drm_atomic_helper_check, @@ -647,7 +696,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) drm_mode_config_reset(dev); pm_runtime_get_sync(dev->dev); - ret = drm_irq_install(dev, dc->hlcdc->irq); + ret = atmel_hlcdc_dc_irq_install(dev, dc->hlcdc->irq); pm_runtime_put_sync(dev->dev); if (ret < 0) { dev_err(dev->dev, "failed to install IRQ handler\n"); @@ -676,7 +725,7 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) drm_mode_config_cleanup(dev); pm_runtime_get_sync(dev->dev); - drm_irq_uninstall(dev); + atmel_hlcdc_dc_irq_uninstall(dev); pm_runtime_put_sync(dev->dev); dev->dev_private = NULL; @@ -685,40 +734,10 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) clk_disable_unprepare(dc->hlcdc->periph_clk); } -static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev) -{ - struct atmel_hlcdc_dc *dc = dev->dev_private; - unsigned int cfg = 0; - int i; - - /* Enable interrupts on activated layers */ - for (i = 0; i < ATMEL_HLCDC_MAX_LAYERS; i++) { - if (dc->layers[i]) - cfg |= ATMEL_HLCDC_LAYER_STATUS(i); - } - - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, cfg); - - return 0; -} - -static void atmel_hlcdc_dc_irq_uninstall(struct drm_device *dev) -{ - struct atmel_hlcdc_dc *dc = dev->dev_private; - unsigned int isr; - - regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IDR, 0xffffffff); - regmap_read(dc->hlcdc->regmap, ATMEL_HLCDC_ISR, &isr); -} - DEFINE_DRM_GEM_CMA_FOPS(fops); static const struct drm_driver atmel_hlcdc_dc_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, - .irq_handler = atmel_hlcdc_dc_irq_handler, - .irq_preinstall = atmel_hlcdc_dc_irq_uninstall, - .irq_postinstall = atmel_hlcdc_dc_irq_postinstall, - .irq_uninstall = atmel_hlcdc_dc_irq_uninstall, DRM_GEM_CMA_DRIVER_OPS, .fops = &fops, .name = "atmel-hlcdc",
Drop the DRM IRQ midlayer in favor of Linux IRQ interfaces. DRM's IRQ helpers are mostly useful for UMS drivers. Modern KMS drivers don't benefit from using it. DRM IRQ callbacks are now being called directly or inlined. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 85 ++++++++++++-------- 1 file changed, 52 insertions(+), 33 deletions(-)