| Message ID | 20240902105546.792625-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bochs: Modernize driver | expand |
On Mon, Sep 02, 2024 at 12:53:40PM GMT, Thomas Zimmermann wrote: > Implement a read function for struct drm_edid and read the EDID data > with drm_edit_read_custom(). Update the connector data accordingly. > > The EDID data comes from the emulator itself and the connector stores > a copy in its EDID property. The drm_edid field in struct bochs_device > is therefore not required. Remove it. > > If qemu provides no EDID data, install default display modes as before. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > --- > drivers/gpu/drm/tiny/bochs.c | 48 +++++++++++++++++------------------- > 1 file changed, 22 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c > index 47a45a14306c..197fc00b373f 100644 > --- a/drivers/gpu/drm/tiny/bochs.c > +++ b/drivers/gpu/drm/tiny/bochs.c > @@ -85,7 +85,6 @@ struct bochs_device { > u16 yres_virtual; > u32 stride; > u32 bpp; > - const struct drm_edid *drm_edid; > > /* drm */ > struct drm_device *dev; > @@ -172,12 +171,14 @@ static void bochs_hw_set_little_endian(struct bochs_device *bochs) > #define bochs_hw_set_native_endian(_b) bochs_hw_set_little_endian(_b) > #endif > > -static int bochs_get_edid_block(void *data, u8 *buf, > - unsigned int block, size_t len) > +static int bochs_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) > { > struct bochs_device *bochs = data; > size_t i, start = block * EDID_LENGTH; > > + if (!bochs->mmio) > + return -1; > + > if (start + len > 0x400 /* vga register offset */) > return -1; > > @@ -187,25 +188,20 @@ static int bochs_get_edid_block(void *data, u8 *buf, > return 0; > } > > -static int bochs_hw_load_edid(struct bochs_device *bochs) > +static const struct drm_edid *bochs_hw_read_edid(struct drm_connector *connector) > { > + struct drm_device *dev = connector->dev; > + struct bochs_device *bochs = dev->dev_private; > u8 header[8]; > > - if (!bochs->mmio) > - return -1; > - > /* check header to detect whenever edid support is enabled in qemu */ > bochs_get_edid_block(bochs, header, 0, ARRAY_SIZE(header)); > if (drm_edid_header_is_valid(header) != 8) I understand that you probably don't want to change the behaviour of the driver, but maybe it's better to check drm_edid_read_custom return value? Bochs, amdgpu and radeon are the only drivers that call drm_edid_header_is_valid(). > - return -1; > + return NULL; >
Hi Am 23.09.24 um 06:35 schrieb Dmitry Baryshkov: > On Mon, Sep 02, 2024 at 12:53:40PM GMT, Thomas Zimmermann wrote: >> Implement a read function for struct drm_edid and read the EDID data >> with drm_edit_read_custom(). Update the connector data accordingly. >> >> The EDID data comes from the emulator itself and the connector stores >> a copy in its EDID property. The drm_edid field in struct bochs_device >> is therefore not required. Remove it. >> >> If qemu provides no EDID data, install default display modes as before. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >> --- >> drivers/gpu/drm/tiny/bochs.c | 48 +++++++++++++++++------------------- >> 1 file changed, 22 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c >> index 47a45a14306c..197fc00b373f 100644 >> --- a/drivers/gpu/drm/tiny/bochs.c >> +++ b/drivers/gpu/drm/tiny/bochs.c >> @@ -85,7 +85,6 @@ struct bochs_device { >> u16 yres_virtual; >> u32 stride; >> u32 bpp; >> - const struct drm_edid *drm_edid; >> >> /* drm */ >> struct drm_device *dev; >> @@ -172,12 +171,14 @@ static void bochs_hw_set_little_endian(struct bochs_device *bochs) >> #define bochs_hw_set_native_endian(_b) bochs_hw_set_little_endian(_b) >> #endif >> >> -static int bochs_get_edid_block(void *data, u8 *buf, >> - unsigned int block, size_t len) >> +static int bochs_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) >> { >> struct bochs_device *bochs = data; >> size_t i, start = block * EDID_LENGTH; >> >> + if (!bochs->mmio) >> + return -1; >> + >> if (start + len > 0x400 /* vga register offset */) >> return -1; >> >> @@ -187,25 +188,20 @@ static int bochs_get_edid_block(void *data, u8 *buf, >> return 0; >> } >> >> -static int bochs_hw_load_edid(struct bochs_device *bochs) >> +static const struct drm_edid *bochs_hw_read_edid(struct drm_connector *connector) >> { >> + struct drm_device *dev = connector->dev; >> + struct bochs_device *bochs = dev->dev_private; >> u8 header[8]; >> >> - if (!bochs->mmio) >> - return -1; >> - >> /* check header to detect whenever edid support is enabled in qemu */ >> bochs_get_edid_block(bochs, header, 0, ARRAY_SIZE(header)); >> if (drm_edid_header_is_valid(header) != 8) > I understand that you probably don't want to change the behaviour of the > driver, but maybe it's better to check drm_edid_read_custom return > value? Bochs, amdgpu and radeon are the only drivers that call > drm_edid_header_is_valid(). The nearby comment indicates that EDID support might not be present in qemu. But drm_edid_read_custom() warns in this case, even though it's a legal state. Hence the test. Best regards Thomas > >> - return -1; >> + return NULL; >>
On Wed, Sep 25, 2024 at 10:15:49AM GMT, Thomas Zimmermann wrote: > Hi > > Am 23.09.24 um 06:35 schrieb Dmitry Baryshkov: > > On Mon, Sep 02, 2024 at 12:53:40PM GMT, Thomas Zimmermann wrote: > > > Implement a read function for struct drm_edid and read the EDID data > > > with drm_edit_read_custom(). Update the connector data accordingly. > > > > > > The EDID data comes from the emulator itself and the connector stores > > > a copy in its EDID property. The drm_edid field in struct bochs_device > > > is therefore not required. Remove it. > > > > > > If qemu provides no EDID data, install default display modes as before. > > > > > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > > --- > > > drivers/gpu/drm/tiny/bochs.c | 48 +++++++++++++++++------------------- > > > 1 file changed, 22 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c > > > index 47a45a14306c..197fc00b373f 100644 > > > --- a/drivers/gpu/drm/tiny/bochs.c > > > +++ b/drivers/gpu/drm/tiny/bochs.c > > > @@ -85,7 +85,6 @@ struct bochs_device { > > > u16 yres_virtual; > > > u32 stride; > > > u32 bpp; > > > - const struct drm_edid *drm_edid; > > > /* drm */ > > > struct drm_device *dev; > > > @@ -172,12 +171,14 @@ static void bochs_hw_set_little_endian(struct bochs_device *bochs) > > > #define bochs_hw_set_native_endian(_b) bochs_hw_set_little_endian(_b) > > > #endif > > > -static int bochs_get_edid_block(void *data, u8 *buf, > > > - unsigned int block, size_t len) > > > +static int bochs_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) > > > { > > > struct bochs_device *bochs = data; > > > size_t i, start = block * EDID_LENGTH; > > > + if (!bochs->mmio) > > > + return -1; > > > + > > > if (start + len > 0x400 /* vga register offset */) > > > return -1; > > > @@ -187,25 +188,20 @@ static int bochs_get_edid_block(void *data, u8 *buf, > > > return 0; > > > } > > > -static int bochs_hw_load_edid(struct bochs_device *bochs) > > > +static const struct drm_edid *bochs_hw_read_edid(struct drm_connector *connector) > > > { > > > + struct drm_device *dev = connector->dev; > > > + struct bochs_device *bochs = dev->dev_private; > > > u8 header[8]; > > > - if (!bochs->mmio) > > > - return -1; > > > - > > > /* check header to detect whenever edid support is enabled in qemu */ > > > bochs_get_edid_block(bochs, header, 0, ARRAY_SIZE(header)); > > > if (drm_edid_header_is_valid(header) != 8) > > I understand that you probably don't want to change the behaviour of the > > driver, but maybe it's better to check drm_edid_read_custom return > > value? Bochs, amdgpu and radeon are the only drivers that call > > drm_edid_header_is_valid(). > > The nearby comment indicates that EDID support might not be present in qemu. > But drm_edid_read_custom() warns in this case, even though it's a legal > state. Hence the test. Ack, thanks for the explanation. > > Best regards > Thomas > > > > > > - return -1; > > > + return NULL; > > -- > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Frankenstrasse 146, 90461 Nuernberg, Germany > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman > HRB 36809 (AG Nuernberg) >
diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c index 47a45a14306c..197fc00b373f 100644 --- a/drivers/gpu/drm/tiny/bochs.c +++ b/drivers/gpu/drm/tiny/bochs.c @@ -85,7 +85,6 @@ struct bochs_device { u16 yres_virtual; u32 stride; u32 bpp; - const struct drm_edid *drm_edid; /* drm */ struct drm_device *dev; @@ -172,12 +171,14 @@ static void bochs_hw_set_little_endian(struct bochs_device *bochs) #define bochs_hw_set_native_endian(_b) bochs_hw_set_little_endian(_b) #endif -static int bochs_get_edid_block(void *data, u8 *buf, - unsigned int block, size_t len) +static int bochs_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len) { struct bochs_device *bochs = data; size_t i, start = block * EDID_LENGTH; + if (!bochs->mmio) + return -1; + if (start + len > 0x400 /* vga register offset */) return -1; @@ -187,25 +188,20 @@ static int bochs_get_edid_block(void *data, u8 *buf, return 0; } -static int bochs_hw_load_edid(struct bochs_device *bochs) +static const struct drm_edid *bochs_hw_read_edid(struct drm_connector *connector) { + struct drm_device *dev = connector->dev; + struct bochs_device *bochs = dev->dev_private; u8 header[8]; - if (!bochs->mmio) - return -1; - /* check header to detect whenever edid support is enabled in qemu */ bochs_get_edid_block(bochs, header, 0, ARRAY_SIZE(header)); if (drm_edid_header_is_valid(header) != 8) - return -1; + return NULL; - drm_edid_free(bochs->drm_edid); - bochs->drm_edid = drm_edid_read_custom(&bochs->connector, - bochs_get_edid_block, bochs); - if (!bochs->drm_edid) - return -1; + drm_dbg(dev, "Found EDID data blob.\n"); - return 0; + return drm_edid_read_custom(connector, bochs_get_edid_block, bochs); } static int bochs_hw_init(struct drm_device *dev) @@ -303,7 +299,6 @@ static void bochs_hw_fini(struct drm_device *dev) if (bochs->fb_map) iounmap(bochs->fb_map); pci_release_regions(to_pci_dev(dev->dev)); - drm_edid_free(bochs->drm_edid); } static void bochs_hw_blank(struct bochs_device *bochs, bool blank) @@ -469,21 +464,28 @@ static const struct drm_simple_display_pipe_funcs bochs_pipe_funcs = { .cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb, }; -static int bochs_connector_get_modes(struct drm_connector *connector) +static int bochs_connector_helper_get_modes(struct drm_connector *connector) { + const struct drm_edid *edid; int count; - count = drm_edid_connector_add_modes(connector); + edid = bochs_hw_read_edid(connector); - if (!count) { + if (edid) { + drm_edid_connector_update(connector, edid); + count = drm_edid_connector_add_modes(connector); + drm_edid_free(edid); + } else { + drm_edid_connector_update(connector, NULL); count = drm_add_modes_noedid(connector, 8192, 8192); drm_set_preferred_mode(connector, defx, defy); } + return count; } static const struct drm_connector_helper_funcs bochs_connector_connector_helper_funcs = { - .get_modes = bochs_connector_get_modes, + .get_modes = bochs_connector_helper_get_modes, }; static const struct drm_connector_funcs bochs_connector_connector_funcs = { @@ -501,14 +503,8 @@ static void bochs_connector_init(struct drm_device *dev) drm_connector_init(dev, connector, &bochs_connector_connector_funcs, DRM_MODE_CONNECTOR_VIRTUAL); + drm_connector_attach_edid_property(connector); drm_connector_helper_add(connector, &bochs_connector_connector_helper_funcs); - - bochs_hw_load_edid(bochs); - if (bochs->drm_edid) { - DRM_INFO("Found EDID data blob.\n"); - drm_connector_attach_edid_property(connector); - drm_edid_connector_update(&bochs->connector, bochs->drm_edid); - } } static const struct drm_mode_config_funcs bochs_mode_funcs = {