[PATCH/RFC] drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

[PATCH/RFC] drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

[Laurent Pinchart]

DRM_IOCTL_MODESET_CTL must only be used for UMS drivers. Make it a no-op
for KMS drivers.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/drm_irq.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

My understanding of the DRM framework tells me that calling
DRM_IOCTL_MODESET_CTL on a KMS driver is not only unneeded, but could also
mess up its internal state. Did I get it right ?

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index c798eea..03f16f3 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -974,7 +974,6 @@ EXPORT_SYMBOL(drm_vblank_off);
  * drm_vblank_pre_modeset - account for vblanks across mode sets
  * @dev: DRM device
  * @crtc: CRTC in question
- * @post: post or pre mode set?
  *
  * Account for vblank events across mode setting events, which will likely
  * reset the hardware frame counter.
@@ -1037,6 +1036,10 @@ int drm_modeset_ctl(struct drm_device *dev, void *data,
 	if (!dev->num_crtcs)
 		return 0;
 
+	/* KMS drivers handle this internally */
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		return 0;
+
 	crtc = modeset->crtc;
 	if (crtc >= dev->num_crtcs)
 		return -EINVAL;
-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC] drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

[Michel Dänzer]

On Mit, 2012-05-30 at 00:58 +0200, Laurent Pinchart wrote: 
> DRM_IOCTL_MODESET_CTL must only be used for UMS drivers. Make it a no-op
> for KMS drivers.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/gpu/drm/drm_irq.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> My understanding of the DRM framework tells me that calling
> DRM_IOCTL_MODESET_CTL on a KMS driver is not only unneeded, but could also
> mess up its internal state. Did I get it right ?

Yes, good catch.


> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index c798eea..03f16f3 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -974,7 +974,6 @@ EXPORT_SYMBOL(drm_vblank_off);
>   * drm_vblank_pre_modeset - account for vblanks across mode sets
>   * @dev: DRM device
>   * @crtc: CRTC in question
> - * @post: post or pre mode set?
>   *
>   * Account for vblank events across mode setting events, which will likely
>   * reset the hardware frame counter.

This hunk should really be in a separate patch, but other than that

Reviewed-by: Michel Dänzer <michel@daenzer.net>


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH/RFC] drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

[Laurent Pinchart]

Hi Michel,

On Wednesday 30 May 2012 13:29:06 Michel Dänzer wrote:
> On Mit, 2012-05-30 at 00:58 +0200, Laurent Pinchart wrote:
> > DRM_IOCTL_MODESET_CTL must only be used for UMS drivers. Make it a no-op
> > for KMS drivers.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_irq.c |    5 ++++-
> >  1 files changed, 4 insertions(+), 1 deletions(-)
> > 
> > My understanding of the DRM framework tells me that calling
> > DRM_IOCTL_MODESET_CTL on a KMS driver is not only unneeded, but could also
> > mess up its internal state. Did I get it right ?
> 
> Yes, good catch.
> 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index c798eea..03f16f3 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -974,7 +974,6 @@ EXPORT_SYMBOL(drm_vblank_off);
> > 
> >   * drm_vblank_pre_modeset - account for vblanks across mode sets
> >   * @dev: DRM device
> >   * @crtc: CRTC in question
> > - * @post: post or pre mode set?
> >   *
> >   * Account for vblank events across mode setting events, which will
> >   likely
> >   * reset the hardware frame counter.
> 
> This hunk should really be in a separate patch,

I've thought about sending a separate patch, but as the change only touched a 
single line of comment related to the same function, I ended up not splitting 
it. I can resubmit two patches if needed.

> but other than that
> 
> Reviewed-by: Michel Dänzer <michel@daenzer.net>

Thank you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC] drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

[Laurent Pinchart]

On Wednesday 30 May 2012 19:12:11 Laurent Pinchart wrote:
> On Wednesday 30 May 2012 13:29:06 Michel Dänzer wrote:
> > On Mit, 2012-05-30 at 00:58 +0200, Laurent Pinchart wrote:
> > > DRM_IOCTL_MODESET_CTL must only be used for UMS drivers. Make it a no-op
> > > for KMS drivers.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/drm_irq.c |    5 ++++-
> > >  1 files changed, 4 insertions(+), 1 deletions(-)
> > > 
> > > My understanding of the DRM framework tells me that calling
> > > DRM_IOCTL_MODESET_CTL on a KMS driver is not only unneeded, but could
> > > also
> > > mess up its internal state. Did I get it right ?
> > 
> > Yes, good catch.
> > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index c798eea..03f16f3 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -974,7 +974,6 @@ EXPORT_SYMBOL(drm_vblank_off);
> > > 
> > >   * drm_vblank_pre_modeset - account for vblanks across mode sets
> > >   * @dev: DRM device
> > >   * @crtc: CRTC in question
> > > - * @post: post or pre mode set?
> > >   *
> > >   * Account for vblank events across mode setting events, which will
> > >   likely
> > >   * reset the hardware frame counter.
> > 
> > This hunk should really be in a separate patch,
> 
> I've thought about sending a separate patch, but as the change only touched
> a single line of comment related to the same function, I ended up not
> splitting it. I can resubmit two patches if needed.

Ping ? Dave, could you take the patch in your tree ? I can split it if needed.

> > but other than that
> > 
> > Reviewed-by: Michel Dänzer <michel@daenzer.net>
> 
> Thank you.

-- 
Regards,

Laurent Pinchart

Re: [PATCH/RFC] drm: Disallow DRM_IOCTL_MODESET_CTL for KMS drivers

[Dave Airlie]

On Wed, Jul 18, 2012 at 1:01 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wednesday 30 May 2012 19:12:11 Laurent Pinchart wrote:
>> On Wednesday 30 May 2012 13:29:06 Michel Dänzer wrote:
>> > On Mit, 2012-05-30 at 00:58 +0200, Laurent Pinchart wrote:
>> > > DRM_IOCTL_MODESET_CTL must only be used for UMS drivers. Make it a no-op
>> > > for KMS drivers.
>> > >
>> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > > ---
>> > >
>> > >  drivers/gpu/drm/drm_irq.c |    5 ++++-
>> > >  1 files changed, 4 insertions(+), 1 deletions(-)
>> > >
>> > > My understanding of the DRM framework tells me that calling
>> > > DRM_IOCTL_MODESET_CTL on a KMS driver is not only unneeded, but could
>> > > also
>> > > mess up its internal state. Did I get it right ?
>> >
>> > Yes, good catch.
>> >
>> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> > > index c798eea..03f16f3 100644
>> > > --- a/drivers/gpu/drm/drm_irq.c
>> > > +++ b/drivers/gpu/drm/drm_irq.c
>> > > @@ -974,7 +974,6 @@ EXPORT_SYMBOL(drm_vblank_off);
>> > >
>> > >   * drm_vblank_pre_modeset - account for vblanks across mode sets
>> > >   * @dev: DRM device
>> > >   * @crtc: CRTC in question
>> > > - * @post: post or pre mode set?
>> > >   *
>> > >   * Account for vblank events across mode setting events, which will
>> > >   likely
>> > >   * reset the hardware frame counter.
>> >
>> > This hunk should really be in a separate patch,
>>
>> I've thought about sending a separate patch, but as the change only touched
>> a single line of comment related to the same function, I ended up not
>> splitting it. I can resubmit two patches if needed.
>
> Ping ? Dave, could you take the patch in your tree ? I can split it if needed.

Pulled as-is, thanks, sorry for delay.

Dave.