Re: [PATCH 2/4] WIP: drm: Introduce rvkms

[Lyude Paul <lyude@redhat.com>]

On Wed, 2024-03-27 at 21:06 +0000, Benno Lossin wrote:
> On 22.03.24 23:03, Lyude Paul wrote:
> > diff --git a/drivers/gpu/drm/rvkms/connector.rs
> > b/drivers/gpu/drm/rvkms/connector.rs
> > new file mode 100644
> > index 0000000000000..40f84d38437ee
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rvkms/connector.rs
> > @@ -0,0 +1,55 @@
> > +// TODO: License and stuff
> > +// Contain's rvkms's drm_connector implementation
> > +
> > +use super::{RvkmsDriver, RvkmsDevice, MAX_RES, DEFAULT_RES};
> > +use kernel::{
> > +    prelude::*,
> > +    drm::{
> > +        device::Device,
> > +        kms::{
> > +            connector::{self, ConnectorGuard},
> > +            ModeConfigGuard
> > +        }
> > +    },
> > +    prelude::*
> > +};
> > +use core::marker::PhantomPinned;
> > +
> > +#[pin_data]
> > +pub(crate) struct DriverConnector {
> > +    #[pin]
> > +    _p: PhantomPinned
> > +}
> 
> This struct does not need to be annotated with `#[pin_data]`, this
> should just work:
> 
> pub(crate) struct DriverConnector;
> 
> > +
> > +pub(crate) type Connector = connector::Connector<DriverConnector>;
> > +
> > +impl connector::DriverConnector for DriverConnector {
> > +    type Initializer = impl PinInit<Self, Error>;
> > +
> > +    type State = ConnectorState;
> > +
> > +    type Driver = RvkmsDriver;
> > +
> > +    type Args = ();
> > +
> > +    fn new(dev: &Device<Self::Driver>, args: Self::Args) ->
> > Self::Initializer {
> 
> And then here just return `Self`.
> 
> This works, since there is a blanket impl `PinInit<T, E> for T`.
> 
> Looking at how you use this API, I am not sure if you actually need
> pin-init for the type that implements `DriverConnector`.
> Do you need to store eg `Mutex<T>` or something else that needs
> pin-init in here in a more complex driver?

Most likely yes - a lot of drivers have various private locks contained
within their subclassed mode objects. I'm not sure we will in rvkms's
connector since vkms doesn't really do much with connectors - but we at
a minimum be using pinned types (spinlocks and hrtimers) in our
DriverCrtc implementation once I've started implementing support for
vblanks[1]

[1]
https://www.kernel.org/doc/html/v6.9-rc5/gpu/drm-kms.html?highlight=vblank#vertical-blanking

In nova (the main reason I'm working on rvkms in the first place),
we'll definitely have locks in our connectors and possibly other types.

> 

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat